[PATCH 0/2] Another round of capng_apply() fixes
See 2/2 for explanation. Michal Prívozník (2): virSetUIDGIDWithCaps: Check for capng_apply() retval properly virSetUIDGIDWithCaps: Set bounding capabilities only with CAP_SETPCAP src/util/virutil.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) -- 2.31.1
After all capabilities were set (except for CAP_SETGID, CAP_SETUID and CAP_SETPCAP) and after UID:GID was changed we drop the last aforementioned capabilities (we couldn't drop them before because we needed UID:GID and capabilities change). Therefore, there's final capng_apply() call. However, it's return value is not checked for properly. It's typical problem of: var = func() < 0 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index ed3d57662b..aba0aea0ff 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1261,7 +1261,7 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups, if (need_setpcap) capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP); - if (((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0)) { + if ((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot apply process capabilities %d"), capng_ret); return -1; -- 2.31.1
On 7/22/21 11:29 AM, Michal Privoznik wrote:
After all capabilities were set (except for CAP_SETGID, CAP_SETUID and CAP_SETPCAP) and after UID:GID was changed we drop the last aforementioned capabilities (we couldn't drop them before because we needed UID:GID and capabilities change). Therefore, there's final capng_apply() call. However, it's return value is not checked for properly. It's typical problem of:
var = func() < 0
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index ed3d57662b..aba0aea0ff 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1261,7 +1261,7 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups, if (need_setpcap) capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP);
- if (((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0)) { + if ((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot apply process capabilities %d"), capng_ret); return -1;
Does this have any functional impact? before: if (((a = b()) < c)) after: if ((a = b()) < c) Looks like a paren was dropped off outside, which shouldn't make a difference. So IMO amend the commit message and push as trivial. - Cole
In one of my previous patches I've tried to postpone dropping CAP_SETPCAP until the very end because it's needed for capng_apply(). What I did not realize back then was that we might not have the capability to begin with. Because of unknown reasons capng_apply() pollutes logs only for CAPNG_SELECT_BOUNDS and not for CAPNG_SELECT_CAPS. Reproducer is really simple: run libvirtd as a regular user. During its initialization, libvirtd will spawn some binaries (dnsmasq, qemu-*, etc.) and while doing so it will try to drop capabilities. Anyway, let's call capng_apply(CAPNG_SELECT_BOUNDS) only if we have the CAP_SETPCAP (which is tracked in need_setpcap variable). Fixes: 438b50dda8a863fdc988e9ab612f097cc1626e8a Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1924218 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virutil.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index aba0aea0ff..00cd56e2b2 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1250,7 +1250,8 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups, * do this if we failed to get the capability above, so ignore the * return value. */ - capng_apply(CAPNG_SELECT_BOUNDS); + if (!need_setpcap) + capng_apply(CAPNG_SELECT_BOUNDS); /* Drop the caps that allow setuid/gid (unless they were requested) */ if (need_setgid) -- 2.31.1
On 7/22/21 11:29 AM, Michal Privoznik wrote:
In one of my previous patches I've tried to postpone dropping CAP_SETPCAP until the very end because it's needed for capng_apply(). What I did not realize back then was that we might not have the capability to begin with. Because of unknown reasons capng_apply() pollutes logs only for CAPNG_SELECT_BOUNDS and not for CAPNG_SELECT_CAPS.
Reproducer is really simple: run libvirtd as a regular user. During its initialization, libvirtd will spawn some binaries (dnsmasq, qemu-*, etc.) and while doing so it will try to drop capabilities.
Anyway, let's call capng_apply(CAPNG_SELECT_BOUNDS) only if we have the CAP_SETPCAP (which is tracked in need_setpcap variable).
Fixes: 438b50dda8a863fdc988e9ab612f097cc1626e8a Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1924218 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virutil.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index aba0aea0ff..00cd56e2b2 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1250,7 +1250,8 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups, * do this if we failed to get the capability above, so ignore the * return value. */ - capng_apply(CAPNG_SELECT_BOUNDS); + if (!need_setpcap) + capng_apply(CAPNG_SELECT_BOUNDS);
/* Drop the caps that allow setuid/gid (unless they were requested) */ if (need_setgid)
Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole - Cole
participants (2)
-
Cole Robinson -
Michal Privoznik