9:55 a.m.
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
9:55 a.m.
New subject: [libvirt] [PATCH 1/2] util: Keep errno set to the root error after when returning from virSetUIDGID
---
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
11:55 a.m.
New subject: [libvirt] [PATCH 1/2] util: Keep errno set to the root error after when returning from virSetUIDGID
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.
9:55 a.m.
New subject: [libvirt] [PATCH 2/2] Replace all remaining setgid/setuid calls with virSetUIDGID
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
11:55 a.m.
New subject: [libvirt] [PATCH 2/2] Replace all remaining setgid/setuid calls with virSetUIDGID
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.
5:21 p.m.
New subject: [libvirt] [PATCH] maint: avoid future setgid problems
* 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
7:26 p.m.
New subject: [libvirt] [PATCH] maint: avoid future setgid problems
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.
9:06 a.m.
New subject: [libvirt] [PATCH] maint: avoid future setgid problems
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4902
days inactive
4934
days old
8 comments
3 participants
participants (3)
-
Eric Blake -
Jiri Denemark -
Laine Stump