Patchwork [3/4] Add cap reduction support to enable use as SUID

login
register
mail settings
Submitter Richa Marwaha
Date Oct. 6, 2011, 3:38 p.m.
Message ID <1317915508-15491-4-git-send-email-rmarwah@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/118110/
State New
Headers show

Comments

Richa Marwaha - Oct. 6, 2011, 3:38 p.m.
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: Richa Marwaha <rmarwah@linux.vnet.ibm.com>
---
 configure            |   34 ++++++++++++++++++++++++++++++
 qemu-bridge-helper.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+), 0 deletions(-)
Daniel P. Berrange - Oct. 6, 2011, 4:34 p.m.
On Thu, Oct 06, 2011 at 11:38:27AM -0400, Richa Marwaha 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.
> +#ifdef CONFIG_LIBCAP
> +static int drop_privileges(void)
> +{
> +    cap_t cap;
> +    cap_value_t new_caps[] = {CAP_NET_ADMIN};
> +
> +    cap = cap_init();

Check for NULL ?

> +
> +    /* set capabilities to be permitted and inheritable.  we don't need the
> +     * caps to be effective right now as they'll get reset when we seteuid
> +     * anyway */
> +    cap_set_flag(cap, CAP_PERMITTED, 1, new_caps, CAP_SET);
> +    cap_set_flag(cap, CAP_INHERITABLE, 1, new_caps, CAP_SET);

Check for failure ?

> +
> +    if (cap_set_proc(cap) == -1) {
> +        return -1;
> +    }
> +
> +    cap_free(cap);

Check for failure ?

> +
> +    /* reduce our privileges to a normal user */
> +    setegid(getgid());
> +    seteuid(getuid());

Check for failure ?

> +    cap = cap_init();

Check for NULL ?

> +
> +    /* enable the our capabilities.  we marked them as inheritable earlier
> +     * which is what allows this to work. */
> +    cap_set_flag(cap, CAP_EFFECTIVE, 1, new_caps, CAP_SET);
> +    cap_set_flag(cap, CAP_PERMITTED, 1, new_caps, CAP_SET);

Check for failure ?

> +
> +    if (cap_set_proc(cap) == -1) {
> +        return -1;
> +    }
> +
> +    cap_free(cap);

Check for failure ?

> +
> +    return 0;
> +}
> +#endif

It may seem like checking for failure on cap_free/cap_set_flag is
not required because they can only return EINVAL for invalid
args, but since this is missing the check for NULL on cap_init
you can actually see errors from those latter functions in an
OOM cenario.

I think I'd suggest not using libcap, instead try libcap-ng [1] whose
APIs are designed with safety in mind & result in much simpler and
clearer code:

eg, that entire function above can be expressed using capng with
something approximating:

     capng_clear(CAPNG_SELECT_BOTH);
     if (capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_NET_ADMIN) < 0)
         error(...);
     if (capng_change_id(getuid(), getgid(), CAPNG_DROP_SUPP_GRP | CAPNG_CLEAR_BOUNDING))
         error(...);


Regards,
Daniel

[1] http://people.redhat.com/sgrubb/libcap-ng/
Anthony Liguori - Oct. 6, 2011, 5:42 p.m.
On 10/06/2011 11:34 AM, Daniel P. Berrange wrote:
> On Thu, Oct 06, 2011 at 11:38:27AM -0400, Richa Marwaha 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.
>> +#ifdef CONFIG_LIBCAP
>> +static int drop_privileges(void)
>> +{
>> +    cap_t cap;
>> +    cap_value_t new_caps[] = {CAP_NET_ADMIN};
>> +
>> +    cap = cap_init();
>
> Check for NULL ?
>
>> +
>> +    /* set capabilities to be permitted and inheritable.  we don't need the
>> +     * caps to be effective right now as they'll get reset when we seteuid
>> +     * anyway */
>> +    cap_set_flag(cap, CAP_PERMITTED, 1, new_caps, CAP_SET);
>> +    cap_set_flag(cap, CAP_INHERITABLE, 1, new_caps, CAP_SET);
>
> Check for failure ?
>
>> +
>> +    if (cap_set_proc(cap) == -1) {
>> +        return -1;
>> +    }
>> +
>> +    cap_free(cap);
>
> Check for failure ?
>
>> +
>> +    /* reduce our privileges to a normal user */
>> +    setegid(getgid());
>> +    seteuid(getuid());
>
> Check for failure ?
>
>> +    cap = cap_init();
>
> Check for NULL ?
>
>> +
>> +    /* enable the our capabilities.  we marked them as inheritable earlier
>> +     * which is what allows this to work. */
>> +    cap_set_flag(cap, CAP_EFFECTIVE, 1, new_caps, CAP_SET);
>> +    cap_set_flag(cap, CAP_PERMITTED, 1, new_caps, CAP_SET);
>
> Check for failure ?
>
>> +
>> +    if (cap_set_proc(cap) == -1) {
>> +        return -1;
>> +    }
>> +
>> +    cap_free(cap);
>
> Check for failure ?
>
>> +
>> +    return 0;
>> +}
>> +#endif
>
> It may seem like checking for failure on cap_free/cap_set_flag is
> not required because they can only return EINVAL for invalid
> args, but since this is missing the check for NULL on cap_init
> you can actually see errors from those latter functions in an
> OOM cenario.
>
> I think I'd suggest not using libcap, instead try libcap-ng [1] whose
> APIs are designed with safety in mind&  result in much simpler and
> clearer code:
>
> eg, that entire function above can be expressed using capng with
> something approximating:
>
>       capng_clear(CAPNG_SELECT_BOTH);
>       if (capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_NET_ADMIN)<  0)
>           error(...);
>       if (capng_change_id(getuid(), getgid(), CAPNG_DROP_SUPP_GRP | CAPNG_CLEAR_BOUNDING))
>           error(...);

Ah, libcap-ng didn't exist when the code was initially written but I agree, it 
looks like a nice library.

Regards,

Anthony Liguori

>
>
> Regards,
> Daniel
>
> [1] http://people.redhat.com/sgrubb/libcap-ng/
>
Corey Bryant - Oct. 6, 2011, 6:05 p.m.
On 10/06/2011 01:42 PM, Anthony Liguori wrote:
> On 10/06/2011 11:34 AM, Daniel P. Berrange wrote:
>> On Thu, Oct 06, 2011 at 11:38:27AM -0400, Richa Marwaha 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.
>>> +#ifdef CONFIG_LIBCAP
>>> +static int drop_privileges(void)
>>> +{
>>> + cap_t cap;
>>> + cap_value_t new_caps[] = {CAP_NET_ADMIN};
>>> +
>>> + cap = cap_init();
>>
>> Check for NULL ?
>>
>>> +
>>> + /* set capabilities to be permitted and inheritable. we don't need the
>>> + * caps to be effective right now as they'll get reset when we seteuid
>>> + * anyway */
>>> + cap_set_flag(cap, CAP_PERMITTED, 1, new_caps, CAP_SET);
>>> + cap_set_flag(cap, CAP_INHERITABLE, 1, new_caps, CAP_SET);
>>
>> Check for failure ?
>>
>>> +
>>> + if (cap_set_proc(cap) == -1) {
>>> + return -1;
>>> + }
>>> +
>>> + cap_free(cap);
>>
>> Check for failure ?
>>
>>> +
>>> + /* reduce our privileges to a normal user */
>>> + setegid(getgid());
>>> + seteuid(getuid());
>>
>> Check for failure ?
>>
>>> + cap = cap_init();
>>
>> Check for NULL ?
>>
>>> +
>>> + /* enable the our capabilities. we marked them as inheritable earlier
>>> + * which is what allows this to work. */
>>> + cap_set_flag(cap, CAP_EFFECTIVE, 1, new_caps, CAP_SET);
>>> + cap_set_flag(cap, CAP_PERMITTED, 1, new_caps, CAP_SET);
>>
>> Check for failure ?
>>
>>> +
>>> + if (cap_set_proc(cap) == -1) {
>>> + return -1;
>>> + }
>>> +
>>> + cap_free(cap);
>>
>> Check for failure ?
>>
>>> +
>>> + return 0;
>>> +}
>>> +#endif
>>
>> It may seem like checking for failure on cap_free/cap_set_flag is
>> not required because they can only return EINVAL for invalid
>> args, but since this is missing the check for NULL on cap_init
>> you can actually see errors from those latter functions in an
>> OOM cenario.
>>
>> I think I'd suggest not using libcap, instead try libcap-ng [1] whose
>> APIs are designed with safety in mind& result in much simpler and
>> clearer code:
>>
>> eg, that entire function above can be expressed using capng with
>> something approximating:
>>
>> capng_clear(CAPNG_SELECT_BOTH);
>> if (capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED,
>> CAP_NET_ADMIN)< 0)
>> error(...);
>> if (capng_change_id(getuid(), getgid(), CAPNG_DROP_SUPP_GRP |
>> CAPNG_CLEAR_BOUNDING))
>> error(...);
>
> Ah, libcap-ng didn't exist when the code was initially written but I
> agree, it looks like a nice library.
>
> Regards,
>
> Anthony Liguori
>

This looks a lot simpler.  We'll definitely look into implementing this 
in v2.
Corey Bryant - Oct. 6, 2011, 6:08 p.m.
On 10/06/2011 01:42 PM, Anthony Liguori wrote:
> On 10/06/2011 11:34 AM, Daniel P. Berrange wrote:
>> On Thu, Oct 06, 2011 at 11:38:27AM -0400, Richa Marwaha 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.
>>> +#ifdef CONFIG_LIBCAP
>>> +static int drop_privileges(void)
>>> +{
>>> + cap_t cap;
>>> + cap_value_t new_caps[] = {CAP_NET_ADMIN};
>>> +
>>> + cap = cap_init();
>>
>> Check for NULL ?
>>
>>> +
>>> + /* set capabilities to be permitted and inheritable. we don't need the
>>> + * caps to be effective right now as they'll get reset when we seteuid
>>> + * anyway */
>>> + cap_set_flag(cap, CAP_PERMITTED, 1, new_caps, CAP_SET);
>>> + cap_set_flag(cap, CAP_INHERITABLE, 1, new_caps, CAP_SET);
>>
>> Check for failure ?
>>
>>> +
>>> + if (cap_set_proc(cap) == -1) {
>>> + return -1;
>>> + }
>>> +
>>> + cap_free(cap);
>>
>> Check for failure ?
>>
>>> +
>>> + /* reduce our privileges to a normal user */
>>> + setegid(getgid());
>>> + seteuid(getuid());
>>
>> Check for failure ?
>>
>>> + cap = cap_init();
>>
>> Check for NULL ?
>>
>>> +
>>> + /* enable the our capabilities. we marked them as inheritable earlier
>>> + * which is what allows this to work. */
>>> + cap_set_flag(cap, CAP_EFFECTIVE, 1, new_caps, CAP_SET);
>>> + cap_set_flag(cap, CAP_PERMITTED, 1, new_caps, CAP_SET);
>>
>> Check for failure ?
>>
>>> +
>>> + if (cap_set_proc(cap) == -1) {
>>> + return -1;
>>> + }
>>> +
>>> + cap_free(cap);
>>
>> Check for failure ?
>>
>>> +
>>> + return 0;
>>> +}
>>> +#endif
>>
>> It may seem like checking for failure on cap_free/cap_set_flag is
>> not required because they can only return EINVAL for invalid
>> args, but since this is missing the check for NULL on cap_init
>> you can actually see errors from those latter functions in an
>> OOM cenario.
>>
>> I think I'd suggest not using libcap, instead try libcap-ng [1] whose
>> APIs are designed with safety in mind& result in much simpler and
>> clearer code:
>>
>> eg, that entire function above can be expressed using capng with
>> something approximating:
>>
>> capng_clear(CAPNG_SELECT_BOTH);
>> if (capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED,
>> CAP_NET_ADMIN)< 0)
>> error(...);
>> if (capng_change_id(getuid(), getgid(), CAPNG_DROP_SUPP_GRP |
>> CAPNG_CLEAR_BOUNDING))
>> error(...);
>
> Ah, libcap-ng didn't exist when the code was initially written but I
> agree, it looks like a nice library.
>
> Regards,
>
> Anthony Liguori
>

This looks a lot simpler.  We'll definitely look into implementing this
in v2.

Patch

diff --git a/configure b/configure
index 3e32834..f46e9b7 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 support"
+echo "  --enable-cap             enable libcap 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
 
 ##########################################
+# cap library probe
+if test "$cap" != "no" ; then
+  cap_libs="-lcap"
+  cat > $TMPC << EOF
+#include <sys/capability.h>
+int main(void)
+{
+    cap_init();
+    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()
@@ -2710,6 +2740,7 @@  echo "fdatasync         $fdatasync"
 echo "madvise           $madvise"
 echo "posix_madvise     $posix_madvise"
 echo "uuid support      $uuid"
+echo "libcap support    $cap"
 echo "vhost-net support $vhost_net"
 echo "Trace backend     $trace_backend"
 echo "Trace output file $trace_file-<pid>"
@@ -2821,6 +2852,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 5e09fea..b1519e0 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -33,6 +33,10 @@ 
 
 #include "net/tap-linux.h"
 
+#ifdef CONFIG_LIBCAP
+#include <sys/capability.h>
+#endif
+
 #define MAX_ACLS (128)
 #define DEFAULT_ACL_FILE CONFIG_QEMU_CONFDIR "/bridge.conf"
 
@@ -185,6 +189,47 @@  static int send_fd(int c, int fd)
     return sendmsg(c, &msg, 0);
 }
 
+#ifdef CONFIG_LIBCAP
+static int drop_privileges(void)
+{
+    cap_t cap;
+    cap_value_t new_caps[] = {CAP_NET_ADMIN};
+
+    cap = cap_init();
+
+    /* set capabilities to be permitted and inheritable.  we don't need the
+     * caps to be effective right now as they'll get reset when we seteuid
+     * anyway */
+    cap_set_flag(cap, CAP_PERMITTED, 1, new_caps, CAP_SET);
+    cap_set_flag(cap, CAP_INHERITABLE, 1, new_caps, CAP_SET);
+
+    if (cap_set_proc(cap) == -1) {
+        return -1;
+    }
+
+    cap_free(cap);
+
+    /* reduce our privileges to a normal user */
+    setegid(getgid());
+    seteuid(getuid());
+
+    cap = cap_init();
+
+    /* enable the our capabilities.  we marked them as inheritable earlier
+     * which is what allows this to work. */
+    cap_set_flag(cap, CAP_EFFECTIVE, 1, new_caps, CAP_SET);
+    cap_set_flag(cap, CAP_PERMITTED, 1, new_caps, CAP_SET);
+
+    if (cap_set_proc(cap) == -1) {
+        return -1;
+    }
+
+    cap_free(cap);
+
+    return 0;
+}
+#endif
+
 int main(int argc, char **argv)
 {
     struct ifreq ifr;
@@ -198,6 +243,17 @@  int main(int argc, char **argv)
     int acl_count = 0;
     int i, access_allowed, access_denied;
 
+#ifdef CONFIG_LIBCAP
+    /* if we're run from an suid binary, immediately drop privileges preserving
+     * cap_net_admin */
+    if (geteuid() == 0 && getuid() != geteuid()) {
+        if (drop_privileges() == -1) {
+            fprintf(stderr, "failed to drop privileges\n");
+            return 1;
+        }
+    }
+#endif
+
     /* parse arguments */
     if (argc < 3 || argc > 4) {
         fprintf(stderr, "Usage: %s [--use-vnet] BRIDGE FD\n", argv[0]);