2:22 a.m.
The first one fixes a problem I've started seeing with RHEL-9 and the
other is just removal of check for tautology.
Michal Prívozník (2):
virSetUIDGIDWithCaps: Don't drop CAP_SETPCAP right away
virSetUIDGIDWithCaps: Assume PR_CAPBSET_DROP is always defined
src/util/virutil.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
--
2.31.1
2:22 a.m.
New subject: [PATCH 1/2] virSetUIDGIDWithCaps: Don't drop CAP_SETPCAP right away
There are few cases where we execute a virCommand with all caps
cleared (virCommandClearCaps()). For instance
dnsmasqCapsRefreshInternal() does just that. This means, that
after fork() and before exec() the virSetUIDGIDWithCaps() is
called. But since the caller did not want to change anything,
just drop capabilities, these are the values of arguments:
virSetUIDGIDWithCaps (uid=-1, gid=-1, groups=0x0, ngroups=0,
capBits=0, clearExistingCaps=true)
This means that indeed all capabilities will be dropped,
including CAP_SETPCAP. But this capability controls whether
capabilities can be set, IOW whether capng_apply() succeeds.
There are two calls of capng_apply() in the function. The
CAP_SETPCAP is dropped after the first call and thus the other
call (capng_apply(CAPNG_SELECT_BOUNDS);) fails.
The solution is to keep the capability for as long as needed
(just like CAP_SETGID and CAP_SETUID) and drop it only at the
very end (just like CAP_SETGID and CAP_SETUID).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1949388
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virutil.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 311cbbf93a..199d405286 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1184,12 +1184,10 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int
ngroups,
}
# ifdef PR_CAPBSET_DROP
/* If newer kernel, we need also need setpcap to change the bounding set */
- if ((capBits || need_setgid || need_setuid) &&
- !capng_have_capability(CAPNG_EFFECTIVE, CAP_SETPCAP)) {
+ if (!capng_have_capability(CAPNG_EFFECTIVE, CAP_SETPCAP)) {
need_setpcap = true;
- }
- if (need_setpcap)
capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP);
+ }
# endif
/* Tell system we want to keep caps across uid change */
--
2.31.1
10:04 a.m.
New subject: [PATCH 1/2] virSetUIDGIDWithCaps: Don't drop CAP_SETPCAP right away
On Fri, Jun 25, 2021 at 09:22:55AM +0200, Michal Privoznik wrote:
There are few cases where we execute a virCommand with all caps
cleared (virCommandClearCaps()). For instance
dnsmasqCapsRefreshInternal() does just that. This means, that
after fork() and before exec() the virSetUIDGIDWithCaps() is
called. But since the caller did not want to change anything,
just drop capabilities, these are the values of arguments:
virSetUIDGIDWithCaps (uid=-1, gid=-1, groups=0x0, ngroups=0,
capBits=0, clearExistingCaps=true)
This means that indeed all capabilities will be dropped,
including CAP_SETPCAP. But this capability controls whether
capabilities can be set, IOW whether capng_apply() succeeds.
There are two calls of capng_apply() in the function. The
CAP_SETPCAP is dropped after the first call and thus the other
call (capng_apply(CAPNG_SELECT_BOUNDS);) fails.
The solution is to keep the capability for as long as needed
(just like CAP_SETGID and CAP_SETUID) and drop it only at the
very end (just like CAP_SETGID and CAP_SETUID).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1949388
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virutil.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 311cbbf93a..199d405286 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1184,12 +1184,10 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int
ngroups,
}
# ifdef PR_CAPBSET_DROP
/* If newer kernel, we need also need setpcap to change the bounding set */
- if ((capBits || need_setgid || need_setuid) &&
- !capng_have_capability(CAPNG_EFFECTIVE, CAP_SETPCAP)) {
+ if (!capng_have_capability(CAPNG_EFFECTIVE, CAP_SETPCAP)) {
need_setpcap = true;
- }
- if (need_setpcap)
capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP);
+ }
# endif
I find both the man pages and our implementation of virSetUIDGIDWithCaps() very
difficult to understand, but I can clearly see that SETPCAP is cleared in the
end the same way that the other two are. Few inconsistencies found in the
function are unrelated, so
Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com>
and since this fixes a bug it should be safe for freeze
/* Tell system we want to keep caps across uid change */
--
2.31.1
2:22 a.m.
New subject: [PATCH 2/2] virSetUIDGIDWithCaps: Assume PR_CAPBSET_DROP is always defined
Bounding set capabilities were introduced in kernel commit of
v2.6.25-rc1~912. I guess it is safe to assume that all Linux
hosts we ran on have at least that version or newer.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virutil.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 199d405286..ed3d57662b 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1182,13 +1182,12 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int
ngroups,
need_setuid = true;
capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETUID);
}
-# ifdef PR_CAPBSET_DROP
- /* If newer kernel, we need also need setpcap to change the bounding set */
+
+ /* We need also need setpcap to change the bounding set */
if (!capng_have_capability(CAPNG_EFFECTIVE, CAP_SETPCAP)) {
need_setpcap = true;
capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP);
}
-# endif
/* Tell system we want to keep caps across uid change */
if (prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0)) {
--
2.31.1
10:08 a.m.
New subject: [PATCH 2/2] virSetUIDGIDWithCaps: Assume PR_CAPBSET_DROP is always defined
On Fri, Jun 25, 2021 at 09:22:56AM +0200, Michal Privoznik wrote:
Bounding set capabilities were introduced in kernel commit of
v2.6.25-rc1~912. I guess it is safe to assume that all Linux
hosts we ran on have at least that version or newer.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virutil.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com>
I guess this one can wait after the release
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 199d405286..ed3d57662b 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1182,13 +1182,12 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int
ngroups,
need_setuid = true;
capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETUID);
}
-# ifdef PR_CAPBSET_DROP
- /* If newer kernel, we need also need setpcap to change the bounding set */
+
+ /* We need also need setpcap to change the bounding set */
if (!capng_have_capability(CAPNG_EFFECTIVE, CAP_SETPCAP)) {
need_setpcap = true;
capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP);
}
-# endif
/* Tell system we want to keep caps across uid change */
if (prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0)) {
--
2.31.1
1244
days inactive
1247
days old
4 comments
2 participants
participants (2)
-
Martin Kletzander -
Michal Privoznik