[libvirt] [PATCH 0/2] Replace all remaining setgid calls with virSetUIDGID
Should we also ban setgid/setuid usage since we probably want to call initgroups after them anyway? Jiri Denemark (2): util: Keep errno set to the root error after when returning from virSetUIDGID Replace all remaining setgid/setuid calls with virSetUIDGID src/storage/storage_backend.c | 15 +---------- src/util/util.c | 53 +++++++++++++++++------------------------ 2 files changed, 24 insertions(+), 44 deletions(-) -- 1.7.5.rc3
--- src/util/util.c | 31 ++++++++++++++++++++----------- 1 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index d1a08a6..0b4370b 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2889,17 +2889,20 @@ int virGetGroupID(const char *name, /* Set the real and effective uid and gid to the given values, and call * initgroups so that the process has all the assumed group membership of - * that uid. return 0 on success, -1 on failure. + * that uid. return 0 on success, -1 on failure (the original system error + * remains in errno). */ int virSetUIDGID(uid_t uid, gid_t gid) { + int err; + if (gid > 0) { if (setregid(gid, gid) < 0) { - virReportSystemError(errno, + virReportSystemError(err = errno, _("cannot change to '%d' group"), (unsigned int) gid); - return -1; + goto error; } } @@ -2916,39 +2919,45 @@ virSetUIDGID(uid_t uid, gid_t gid) if (VIR_ALLOC_N(buf, bufsize) < 0) { virReportOOMError(); - return -1; + err = ENOMEM; + goto error; } while ((rc = getpwuid_r(uid, &pwd, buf, bufsize, &pwd_result)) == ERANGE) { if (VIR_RESIZE_N(buf, bufsize, bufsize, bufsize) < 0) { virReportOOMError(); VIR_FREE(buf); - return -1; + err = ENOMEM; + goto error; } } if (rc || !pwd_result) { - virReportSystemError(rc, _("cannot getpwuid_r(%d)"), + virReportSystemError(err = rc, _("cannot getpwuid_r(%d)"), (unsigned int) uid); VIR_FREE(buf); - return -1; + goto error; } if (initgroups(pwd.pw_name, pwd.pw_gid) < 0) { - virReportSystemError(errno, + virReportSystemError(err = errno, _("cannot initgroups(\"%s\", %d)"), pwd.pw_name, (unsigned int) pwd.pw_gid); VIR_FREE(buf); - return -1; + goto error; } VIR_FREE(buf); # endif if (setreuid(uid, uid) < 0) { - virReportSystemError(errno, + virReportSystemError(err = errno, _("cannot change to uid to '%d'"), (unsigned int) uid); - return -1; + goto error; } } return 0; + +error: + errno = err; + return -1; } #else /* HAVE_GETPWUID_R */ -- 1.7.5.rc3
On 05/22/2011 10:55 AM, Jiri Denemark wrote:
--- src/util/util.c | 31 ++++++++++++++++++++----------- 1 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index d1a08a6..0b4370b 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2889,17 +2889,20 @@ int virGetGroupID(const char *name,
/* Set the real and effective uid and gid to the given values, and call * initgroups so that the process has all the assumed group membership of - * that uid. return 0 on success, -1 on failure. + * that uid. return 0 on success, -1 on failure (the original system error + * remains in errno). */ int virSetUIDGID(uid_t uid, gid_t gid) { + int err; + if (gid> 0) { if (setregid(gid, gid)< 0) { - virReportSystemError(errno, + virReportSystemError(err = errno, _("cannot change to '%d' group"), (unsigned int) gid); - return -1; + goto error; } }
@@ -2916,39 +2919,45 @@ virSetUIDGID(uid_t uid, gid_t gid)
if (VIR_ALLOC_N(buf, bufsize)< 0) { virReportOOMError(); - return -1; + err = ENOMEM; + goto error; } while ((rc = getpwuid_r(uid,&pwd, buf, bufsize, &pwd_result)) == ERANGE) { if (VIR_RESIZE_N(buf, bufsize, bufsize, bufsize)< 0) { virReportOOMError(); VIR_FREE(buf); - return -1; + err = ENOMEM; + goto error; } } if (rc || !pwd_result) { - virReportSystemError(rc, _("cannot getpwuid_r(%d)"), + virReportSystemError(err = rc, _("cannot getpwuid_r(%d)"), (unsigned int) uid); VIR_FREE(buf); - return -1; + goto error; } if (initgroups(pwd.pw_name, pwd.pw_gid)< 0) { - virReportSystemError(errno, + virReportSystemError(err = errno, _("cannot initgroups(\"%s\", %d)"), pwd.pw_name, (unsigned int) pwd.pw_gid); VIR_FREE(buf); - return -1; + goto error; } VIR_FREE(buf); # endif if (setreuid(uid, uid)< 0) { - virReportSystemError(errno, + virReportSystemError(err = errno, _("cannot change to uid to '%d'"), (unsigned int) uid); - return -1; + goto error; } } return 0; + +error: + errno = err; + return -1; }
#else /* HAVE_GETPWUID_R */
ACK.
Two additional places need initgroups call to properly work in an environment where the UID is allowed to open/create stuff through its supplementary groups. --- src/storage/storage_backend.c | 15 ++------------- src/util/util.c | 22 ++-------------------- 2 files changed, 4 insertions(+), 33 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f90425a..a209f88 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -535,20 +535,9 @@ static int virStorageBuildSetUIDHook(void *data) { if (tmp->skip) return 0; - if ((vol->target.perms.gid != -1) - && (setgid(vol->target.perms.gid) != 0)) { - virReportSystemError(errno, - _("Cannot set gid to %u before creating %s"), - vol->target.perms.gid, vol->target.path); - return -1; - } - if ((vol->target.perms.uid != -1) - && (setuid(vol->target.perms.uid) != 0)) { - virReportSystemError(errno, - _("Cannot set uid to %u before creating %s"), - vol->target.perms.uid, vol->target.path); + if (virSetUIDGID(vol->target.perms.uid, vol->target.perms.gid) < 0) return -1; - } + return 0; } diff --git a/src/util/util.c b/src/util/util.c index 0b4370b..e221abe 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1476,18 +1476,8 @@ parenterror: /* set desired uid/gid, then attempt to create the file */ - if ((gid != 0) && (setgid(gid) != 0)) { + if (virSetUIDGID(uid, gid) < 0) { ret = -errno; - virReportSystemError(errno, - _("cannot set gid %u creating '%s'"), - (unsigned int) gid, path); - goto childerror; - } - if ((uid != 0) && (setuid(uid) != 0)) { - ret = -errno; - virReportSystemError(errno, - _("cannot set uid %u creating '%s'"), - (unsigned int) uid, path); goto childerror; } if ((fd = open(path, openflags, mode)) < 0) { @@ -1595,16 +1585,8 @@ parenterror: /* set desired uid/gid, then attempt to create the directory */ - if ((gid != 0) && (setgid(gid) != 0)) { - ret = -errno; - virReportSystemError(errno, _("cannot set gid %u creating '%s'"), - (unsigned int) gid, path); - goto childerror; - } - if ((uid != 0) && (setuid(uid) != 0)) { + if (virSetUIDGID(uid, gid) < 0) { ret = -errno; - virReportSystemError(errno, _("cannot set uid %u creating '%s'"), - (unsigned int) uid, path); goto childerror; } if (mkdir(path, mode) < 0) { -- 1.7.5.rc3
On 05/22/2011 10:55 AM, Jiri Denemark wrote:
Two additional places need initgroups call to properly work in an environment where the UID is allowed to open/create stuff through its supplementary groups. --- src/storage/storage_backend.c | 15 ++------------- src/util/util.c | 22 ++-------------------- 2 files changed, 4 insertions(+), 33 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f90425a..a209f88 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -535,20 +535,9 @@ static int virStorageBuildSetUIDHook(void *data) { if (tmp->skip) return 0;
- if ((vol->target.perms.gid != -1) -&& (setgid(vol->target.perms.gid) != 0)) { - virReportSystemError(errno, - _("Cannot set gid to %u before creating %s"), - vol->target.perms.gid, vol->target.path); - return -1; - } - if ((vol->target.perms.uid != -1) -&& (setuid(vol->target.perms.uid) != 0)) { - virReportSystemError(errno, - _("Cannot set uid to %u before creating %s"), - vol->target.perms.uid, vol->target.path); + if (virSetUIDGID(vol->target.perms.uid, vol->target.perms.gid)< 0) return -1; - } + return 0; }
diff --git a/src/util/util.c b/src/util/util.c index 0b4370b..e221abe 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1476,18 +1476,8 @@ parenterror:
/* set desired uid/gid, then attempt to create the file */
- if ((gid != 0)&& (setgid(gid) != 0)) { + if (virSetUIDGID(uid, gid)< 0) { ret = -errno; - virReportSystemError(errno, - _("cannot set gid %u creating '%s'"), - (unsigned int) gid, path); - goto childerror; - } - if ((uid != 0)&& (setuid(uid) != 0)) { - ret = -errno; - virReportSystemError(errno, - _("cannot set uid %u creating '%s'"), - (unsigned int) uid, path); goto childerror; } if ((fd = open(path, openflags, mode))< 0) { @@ -1595,16 +1585,8 @@ parenterror:
/* set desired uid/gid, then attempt to create the directory */
- if ((gid != 0)&& (setgid(gid) != 0)) { - ret = -errno; - virReportSystemError(errno, _("cannot set gid %u creating '%s'"), - (unsigned int) gid, path); - goto childerror; - } - if ((uid != 0)&& (setuid(uid) != 0)) { + if (virSetUIDGID(uid, gid)< 0) { ret = -errno; - virReportSystemError(errno, _("cannot set uid %u creating '%s'"), - (unsigned int) uid, path); goto childerror; } if (mkdir(path, mode)< 0) {
ACK.
* cfg.mk (sc_prohibit_setuid) (exclude_file_name_regexp--sc_prohibit_setuid): New rule. (VC_LIST_ALWAYS_EXCLUDE_REGEX): Always exempt po files. (exclude_file_name_regexp--sc_prohibit_asprintf): Simplify. (exclude_file_name_regexp--sc_prohibit_can_not): Drop. (exclude_file_name_regexp--sc_prohibit_doubled_word): Likewise. --- Tested by temporarily reverting commit 5e09aea7. cfg.mk | 16 ++++++++++------ 1 files changed, 10 insertions(+), 6 deletions(-) diff --git a/cfg.mk b/cfg.mk index c2230b8..a5f343f 100644 --- a/cfg.mk +++ b/cfg.mk @@ -74,7 +74,7 @@ local-checks-to-skip = \ sc_useless_cpp_parens # Files that should never cause syntax check failures. -VC_LIST_ALWAYS_EXCLUDE_REGEX = ^(HACKING|docs/news\.html\.in)$$ +VC_LIST_ALWAYS_EXCLUDE_REGEX = (^(HACKING|docs/news\.html\.in)|\.po)$$ # Functions like free() that are no-ops on NULL arguments. useless_free_options = \ @@ -290,6 +290,12 @@ sc_prohibit_asprintf: halt='use virAsprintf, not as'printf \ $(_sc_search_regexp) +# Prefer virSetUIDGID. +sc_prohibit_setuid: + @prohibit='\<set(re)?[ug]id\> *\(' \ + halt='use virSetUIDGID, not raw set*id' \ + $(_sc_search_regexp) + # Use snprintf rather than s'printf, even if buffer is provably large enough, # since gnulib has more guarantees for snprintf portability sc_prohibit_sprintf: @@ -607,15 +613,11 @@ exclude_file_name_regexp--sc_prohibit_always_true_header_tests = \ (^docs|^python/(libvirt-override|typewrappers)\.c$$) exclude_file_name_regexp--sc_prohibit_asprintf = \ - ^(bootstrap.conf$$|po/|src/util/util\.c$$|examples/domain-events/events-c/event-test\.c$$) - -exclude_file_name_regexp--sc_prohibit_can_not = ^po/ + ^(bootstrap.conf$$|src/util/util\.c$$|examples/domain-events/events-c/event-test\.c$$) exclude_file_name_regexp--sc_prohibit_close = \ (\.p[yl]$$|^docs/|(src/util/files\.c|src/libvirt\.c)$$) -exclude_file_name_regexp--sc_prohibit_doubled_word = ^po/ - exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \ (^docs/api_extension/|^tests/qemuhelpdata/|\.(gif|ico|png)$$) @@ -635,6 +637,8 @@ exclude_file_name_regexp--sc_prohibit_nonreentrant = \ exclude_file_name_regexp--sc_prohibit_readlink = ^src/util/util\.c$$ +exclude_file_name_regexp--sc_prohibit_setuid = ^src/util/util\.c$$ + exclude_file_name_regexp--sc_prohibit_sprintf = ^(docs/|HACKING$$) exclude_file_name_regexp--sc_prohibit_strncpy = \ -- 1.7.4.4
On 06/22/2011 06:21 PM, Eric Blake wrote:
* cfg.mk (sc_prohibit_setuid) (exclude_file_name_regexp--sc_prohibit_setuid): New rule. (VC_LIST_ALWAYS_EXCLUDE_REGEX): Always exempt po files. (exclude_file_name_regexp--sc_prohibit_asprintf): Simplify. (exclude_file_name_regexp--sc_prohibit_can_not): Drop. (exclude_file_name_regexp--sc_prohibit_doubled_word): Likewise. ---
Tested by temporarily reverting commit 5e09aea7.
cfg.mk | 16 ++++++++++------ 1 files changed, 10 insertions(+), 6 deletions(-)
ACK.
On 06/22/2011 06:26 PM, Laine Stump wrote:
On 06/22/2011 06:21 PM, Eric Blake wrote:
* cfg.mk (sc_prohibit_setuid) (exclude_file_name_regexp--sc_prohibit_setuid): New rule. (VC_LIST_ALWAYS_EXCLUDE_REGEX): Always exempt po files. (exclude_file_name_regexp--sc_prohibit_asprintf): Simplify. (exclude_file_name_regexp--sc_prohibit_can_not): Drop. (exclude_file_name_regexp--sc_prohibit_doubled_word): Likewise. ---
Tested by temporarily reverting commit 5e09aea7.
cfg.mk | 16 ++++++++++------ 1 files changed, 10 insertions(+), 6 deletions(-)
ACK.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Jiri Denemark -
Laine Stump