[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2 13/15] util: virSetUIDGIDWithCaps - change uid while keeping caps



On Sat, Feb 16, 2013 at 4:53 PM, Laine Stump <laine laine org> wrote:
> On 02/16/2013 12:20 AM, Doug Goldstein wrote:
>> On Tue, Feb 12, 2013 at 2:15 PM, Laine Stump <laine laine org> wrote:
>>> Normally when a process' uid is changed to non-0, all the capabilities
>>> bits are cleared, even those explicitly set with calls to
>>> capng_update()/capng_apply() made immediately before setuid. And
>>> *after* the process' uid has been changed, it no longer has the
>>> necessary privileges to add capabilities back to the process.
>>>
>>> In order to set a non-0 uid while still maintaining any capabilities
>>> bits, it is necessary to either call capng_change_id() (which
>>> unfortunately doesn't currently call initgroups to setup auxiliary
>>> group membership), or to perform the small amount of calisthenics
>>> contained in the new utility function virSetUIDGIDWithCaps().
>>>
>>> Another very important difference between the capabilities
>>> setting/clearing in virSetUIDGIDWithCaps() and virCommand's
>>> virSetCapabilities() (which it will replace in the next patch) is that
>>> the new function properly clears the capabilities bounding set, so it
>>> will not be possible for a child process to set any new
>>> capabilities.
>>>
>>> A short description of what is done by virSetUIDGIDWithCaps():
>>>
>>> 1) clear all capabilities then set all those desired by the caller (in
>>> capBits) plus CAP_SETGID, CAP_SETUID, and CAP_SETPCAP (which is needed
>>> to change the capabilities bounding set).
>>>
>>> 2) call prctl(), telling it that we want to maintain current
>>> capabilities across an upcoming setuid().
>>>
>>> 3) switch to the new uid/gid
>>>
>>> 4) again call prctl(), telling it we will no longer want capabilities
>>> maintained if this process does another setuid().
>>>
>>> 5) clear the capabilities that we added to allow us to
>>> setuid/setgid/change the bounding set (unless they were also requested
>>> by the caller via the virCommand API).
>>>
>>> Because the modification/maintaining of capabilities is intermingled
>>> with setting the uid, this is necessarily done in a single function,
>>> rather than having two independent functions.
>>>
>>> Note that, due to the way that effective capabilities are computed (at
>>> time of execve) for a process that has uid != 0, the *file*
>>> capabilities of the binary being executed must also have the desired
>>> capabilities bit(s) set (see "man 7 capabilities"). This can be done
>>> with the "filecap" command. (e.g. "filecap /usr/bin/qemu-kvm sys_rawio").
>>> ---
>>> Change from V1:
>>> * properly cast when comparing gid/uid, and only short circuit for -1 (not 0)
>>> * fix // style comments
>>> * add ATTRIBUTE_UNUSED where appropriate for capBits argument.
>>>
>>>  src/libvirt_private.syms |   1 +
>>>  src/util/virutil.c       | 111 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  src/util/virutil.h       |   1 +
>>>  3 files changed, 113 insertions(+)
>>>
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index dcdcb67..d8d5877 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -1312,6 +1312,7 @@ virSetDeviceUnprivSGIO;
>>>  virSetInherit;
>>>  virSetNonBlock;
>>>  virSetUIDGID;
>>> +virSetUIDGIDWithCaps;
>>>  virSkipSpaces;
>>>  virSkipSpacesAndBackslash;
>>>  virSkipSpacesBackwards;
>>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>>> index 0d7db00..28fcc2f 100644
>>> --- a/src/util/virutil.c
>>> +++ b/src/util/virutil.c
>>> @@ -60,6 +60,7 @@
>>>  #endif
>>>  #if WITH_CAPNG
>>>  # include <cap-ng.h>
>>> +# include <sys/prctl.h>
>>>  #endif
>>>  #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R
>>>  # include <mntent.h>
>>> @@ -2990,6 +2991,116 @@ virGetGroupName(gid_t gid ATTRIBUTE_UNUSED)
>>>  }
>>>  #endif /* HAVE_GETPWUID_R */
>>>
>>> +#if WITH_CAPNG
>>> +/* Set the real and effective uid and gid to the given values, while
>>> + * maintaining the capabilities indicated by bits in @capBits. return
>>> + * 0 on success, -1 on failure (the original system error remains in
>>> + * errno).
>>> + */
>>> +int
>>> +virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits)
>>> +{
>>> +    int ii, capng_ret, ret = -1;
>>> +    bool need_setgid = false, need_setuid = false;
>>> +    bool need_prctl = false, need_setpcap = false;
>>> +
>>> +    /* First drop all caps except those in capBits + the extra ones we
>>> +     * need to change uid/gid and change the capabilities bounding
>>> +     * set.
>>> +     */
>>> +
>>> +    capng_clear(CAPNG_SELECT_BOTH);
>>> +
>>> +    for (ii = 0; ii <= CAP_LAST_CAP; ii++) {
>>> +        if (capBits & (1ULL << ii)) {
>>> +            capng_update(CAPNG_ADD,
>>> +                         CAPNG_EFFECTIVE|CAPNG_INHERITABLE|
>>> +                         CAPNG_PERMITTED|CAPNG_BOUNDING_SET,
>>> +                         ii);
>>> +        }
>>> +    }
>>> +
>>> +    if (gid != (gid_t)-1 &&
>>> +        !capng_have_capability(CAPNG_EFFECTIVE, CAP_SETGID)) {
>>> +        need_setgid = true;
>>> +        capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETGID);
>>> +    }
>>> +    if (uid != (uid_t)-1 &&
>>> +        !capng_have_capability(CAPNG_EFFECTIVE, CAP_SETUID)) {
>>> +        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 */
>>> +    if ((capBits || need_setgid || need_setuid) &&
>>> +        !capng_have_capability(CAPNG_EFFECTIVE, CAP_SETPCAP)) {
>>> +        need_setpcap = true;
>>> +    }
>>> +    if (need_setpcap)
>>> +        capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP);
>>> +# endif
>>> +
>>> +    need_prctl = capBits || need_setgid || need_setuid || need_setpcap;
>>> +
>>> +    /* Tell system we want to keep caps across uid change */
>>> +    if (need_prctl && prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0)) {
>>> +        virReportSystemError(errno, "%s",
>>> +                             _("prctl failed to set KEEPCAPS"));
>>> +        goto cleanup;
>>> +    }
>>> +
>>> +    /* Change to the temp capabilities */
>>> +    if ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) < 0) {
>>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +                       _("cannot apply process capabilities %d"), capng_ret);
>>> +        goto cleanup;
>>> +    }
>>> +
>>> +    if (virSetUIDGID(uid, gid) < 0)
>>> +        goto cleanup;
>>> +
>>> +    /* Tell it we are done keeping capabilities */
>>> +    if (need_prctl && prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0)) {
>>> +        virReportSystemError(errno, "%s",
>>> +                             _("prctl failed to reset KEEPCAPS"));
>>> +        goto cleanup;
>>> +    }
>>> +
>>> +    /* Drop the caps that allow setuid/gid (unless they were requested) */
>>> +    if (need_setgid)
>>> +        capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETGID);
>>> +    if (need_setuid)
>>> +        capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETUID);
>>> +    /* Throw away CAP_SETPCAP so no more changes */
>>> +    if (need_setpcap)
>>> +        capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP);
>>> +
>>> +    if (need_prctl && ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) < 0)) {
>>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +                       _("cannot apply process capabilities %d"), capng_ret);
>>> +        ret = -1;
>>> +        goto cleanup;
>>> +    }
>>> +
>>> +    ret = 0;
>>> +cleanup:
>>> +    return ret;
>>> +}
>>> +
>>> +#else
>>> +/*
>>> + * On platforms without libcapng, the capabilities setting is treated
>>> + * as a NOP.
>>> + */
>>> +
>>> +int
>>> +virSetUIDGIDWithCaps(uid_t uid, gid_t gid,
>>> +                     unsigned long long capBits ATTRIBUTE_UNUSED)
>>> +{
>>> +    return virSetUIDGID(uid, gid);
>>> +}
>>> +#endif
>>> +
>>>
>>>  #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R
>>>  /* search /proc/mounts for mount point of *type; return pointer to
>>> diff --git a/src/util/virutil.h b/src/util/virutil.h
>>> index 4201aa1..2dc3403 100644
>>> --- a/src/util/virutil.h
>>> +++ b/src/util/virutil.h
>>> @@ -54,6 +54,7 @@ int virPipeReadUntilEOF(int outfd, int errfd,
>>>                          char **outbuf, char **errbuf);
>>>
>>>  int virSetUIDGID(uid_t uid, gid_t gid);
>>> +int virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits);
>>>
>>>  int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK;
>>>
>>> --
>>> 1.8.1
>> The following error bisect's down to this commit when running out of
>> my local checkout for testing.
>>
>> 2013-02-16 05:16:55.102+0000: 29992: error : virCommandWait:2270 :
>> internal error Child process (LC_ALL=C
>> LD_LIBRARY_PATH=/home/cardoe/work/libvirt/src/.libs
>> PATH=/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/4.6.3:/usr/games/bin
>> HOME=/home/cardoe USER=cardoe LOGNAME=cardoe /usr/bin/qemu-kvm -help)
>> unexpected exit status 1: libvir:  error : internal error cannot apply
>> process capabilities -1
>>
>
> Ugh. Can you manage to get that trapped in gdb and find out the value of
> uid, gid, and capBits, as well as whether it is failing on the first
> call to capng_apply() or the second (they both have the same error
> messsage. (Whatever happened to the function name/line number that used
> to be logged with the error messages?) I wonder if perhaps on debian
> it's failing the capng_apply() call that happens after the uid is changed...
>
>

Oops. Guess that would have been helpful to include. Its Gentoo btw,
not Debian. Its in the first call. I guess the exit message is
overriding the original line number in the error message.

2013-02-17 18:08:03.696+0000: 21164: debug : virExec:641 : Setting
child uid:gid to 0:0 with caps 0
2013-02-17 18:08:03.696+0000: 21164: error : virSetUIDGIDWithCaps:3055
: internal error cannot apply process capabilities -1

-- 
Doug Goldstein


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]