qemu-devel
[Top][All Lists]
Advanced

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

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


From: Corey Bryant
Subject: Re: [Qemu-devel] [PATCH 3/4] Add cap reduction support to enable use as SUID
Date: Thu, 06 Oct 2011 14:05:41 -0400
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.15) Gecko/20110303 Lightning/1.0b2 Thunderbird/3.1.9



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.

--
Regards,
Corey



Regards,
Daniel

[1] http://people.redhat.com/sgrubb/libcap-ng/






reply via email to

[Prev in Thread] Current Thread [Next in Thread]