Re: [Qemu-devel] [PATCH v2 3/4] Add cap reduction support to enable use as SUID

[Anthony Liguori <aliguori@us.ibm.com>]

On 10/24/2011 09:13 AM, Corey Bryant wrote:
>
>
> On 10/23/2011 09:22 AM, Blue Swirl wrote:
>> On Fri, Oct 21, 2011 at 15:07, Corey Bryant<coreyb@linux.vnet.ibm.com> wrote:
>>> The ideal way to use qemu-bridge-helper is to give it an fscap of using:
>>>
>>> setcap cap_net_admin=ep qemu-bridge-helper
>>>
>>> Unfortunately, most distros still do not have a mechanism to package files
>>> with fscaps applied. This means they'll have to SUID the qemu-bridge-helper
>>> binary.
>>>
>>> To improve security, use libcap to reduce our capability set to just
>>> cap_net_admin, then reduce privileges down to the calling user. This is
>>> hopefully close to equivalent to fscap support from a security perspective.
>>>
>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>> Signed-off-by: Richa Marwaha<rmarwah@linux.vnet.ibm.com>
>>> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com>
>>> ---
>>> configure | 34 ++++++++++++++++++++++++++++++++++
>>> qemu-bridge-helper.c | 39 +++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 73 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/configure b/configure
>>> index 6c8b659..fed66b0 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -128,6 +128,7 @@ vnc_thread="no"
>>> xen=""
>>> xen_ctrl_version=""
>>> linux_aio=""
>>> +cap=""
>>> attr=""
>>> xfs=""
>>>
>>> @@ -653,6 +654,10 @@ for opt do
>>> ;;
>>> --enable-kvm) kvm="yes"
>>> ;;
>>> + --disable-cap) cap="no"
>>> + ;;
>>> + --enable-cap) cap="yes"
>>> + ;;
>>> --disable-spice) spice="no"
>>> ;;
>>> --enable-spice) spice="yes"
>>> @@ -1032,6 +1037,8 @@ echo " --disable-vde disable support for vde network"
>>> echo " --enable-vde enable support for vde network"
>>> echo " --disable-linux-aio disable Linux AIO support"
>>> echo " --enable-linux-aio enable Linux AIO support"
>>> +echo " --disable-cap disable libcap-ng support"
>>> +echo " --enable-cap enable libcap-ng support"
>>> echo " --disable-attr disables attr and xattr support"
>>> echo " --enable-attr enable attr and xattr support"
>>> echo " --disable-blobs disable installing provided firmware blobs"
>>> @@ -1638,6 +1645,29 @@ EOF
>>> fi
>>>
>>> ##########################################
>>> +# libcap-ng library probe
>>> +if test "$cap" != "no" ; then
>>> + cap_libs="-lcap-ng"
>>> + cat> $TMPC<< EOF
>>> +#include<cap-ng.h>
>>> +int main(void)
>>> +{
>>> + capng_capability_to_name(CAPNG_EFFECTIVE);
>>> + return 0;
>>> +}
>>> +EOF
>>> + if compile_prog "" "$cap_libs" ; then
>>> + cap=yes
>>> + libs_tools="$cap_libs $libs_tools"
>>> + else
>>> + if test "$cap" = "yes" ; then
>>> + feature_not_found "cap"
>>> + fi
>>> + cap=no
>>> + fi
>>> +fi
>>> +
>>> +##########################################
>>> # Sound support libraries probe
>>>
>>> audio_drv_probe()
>>> @@ -2735,6 +2765,7 @@ echo "fdatasync $fdatasync"
>>> echo "madvise $madvise"
>>> echo "posix_madvise $posix_madvise"
>>> echo "uuid support $uuid"
>>> +echo "libcap-ng support $cap"
>>> echo "vhost-net support $vhost_net"
>>> echo "Trace backend $trace_backend"
>>> echo "Trace output file $trace_file-<pid>"
>>> @@ -2846,6 +2877,9 @@ fi
>>> if test "$vde" = "yes" ; then
>>> echo "CONFIG_VDE=y">> $config_host_mak
>>> fi
>>> +if test "$cap" = "yes" ; then
>>> + echo "CONFIG_LIBCAP=y">> $config_host_mak
>>> +fi
>>> for card in $audio_card_list; do
>>> def=CONFIG_`echo $card | tr '[:lower:]' '[:upper:]'`
>>> echo "$def=y">> $config_host_mak
>>> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
>>> index db257d5..b1562eb 100644
>>> --- a/qemu-bridge-helper.c
>>> +++ b/qemu-bridge-helper.c
>>> @@ -33,6 +33,10 @@
>>>
>>> #include "net/tap-linux.h"
>>>
>>> +#ifdef CONFIG_LIBCAP
>>> +#include<cap-ng.h>
>>> +#endif
>>> +
>>> #define MAX_ACLS (128)
>>> #define DEFAULT_ACL_FILE CONFIG_QEMU_CONFDIR "/bridge.conf"
>>>
>>> @@ -185,6 +189,27 @@ static int send_fd(int c, int fd)
>>> return sendmsg(c,&msg, 0);
>>> }
>>>
>>> +#ifdef CONFIG_LIBCAP
>>> +static int drop_privileges(void)
>>> +{
>>> + /* clear all capabilities */
>>> + capng_clear(CAPNG_SELECT_BOTH);
>>> +
>>> + if (capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED,
>>> + CAP_NET_ADMIN)< 0) {
>>> + return -1;
>>> + }
>>> +
>>> + /* change to calling user's real uid and gid, retaining supplemental
>>> + * groups and CAP_NET_ADMIN */
>>> + if (capng_change_id(getuid(), getgid(), CAPNG_CLEAR_BOUNDING)) {
>>> + return -1;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +#endif
>>> +
>>> int main(int argc, char **argv)
>>> {
>>> struct ifreq ifr;
>>> @@ -198,6 +223,20 @@ int main(int argc, char **argv)
>>> int acl_count = 0;
>>> int i, access_allowed, access_denied;
>>>
>>> + /* if we're run from an suid binary, immediately drop privileges preserving
>>> + * cap_net_admin -- exit immediately if libcap not configured */
>>> + if (geteuid() == 0&& getuid() != geteuid()) {
>>> +#ifdef CONFIG_LIBCAP
>>> + if (drop_privileges() == -1) {
>>> + fprintf(stderr, "failed to drop privileges\n");
>>> + return 1;
>>> + }
>>> +#else
>>> + fprintf(stderr, "failed to drop privileges\n");
>>
>> This makes the tool useless without CONFIG_LIBCAP. Wouldn't it be
>> possible to use setfsuid() instead for Linux?
>>
>> Some fork+setuid helper could be used for other Unix and for the lame
>> OSes without any file system DAC capabilities, a different syntax that
>> does not rely on underlying FS may need to be introduced. Again, I
>> don't know if the tool is even interesting for non-Linux.
>>
>
> I just want to make sure that there is no chance that the helper is run as root
> beyond this point.

But the whole pointer of the helper is to run as root.  It's a small trusted 
piece of code.

Obviously, it's better to drop unneeded privileges when that's possible but in 
the event that is isn't, we shouldn't bail out completely.

Regards,

Anthony Liguori