On Sat, Feb 16, 2013 at 4:53 PM, Laine Stump <laine@xxxxxxxxx> wrote:
> On 02/16/2013 12:20 AM, Doug Goldstein wrote:
>> On Tue, Feb 12, 2013 at 2:15 PM, Laine Stump <laine@xxxxxxxxx> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list