[PATCH 1/2] virSetUIDGIDWithCaps: Don't drop CAP_SETPCAP right away

[Martin Kletzander]

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 at 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 at 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210628/764975ce/attachment-0001.sig>