[libvirt] [PATCH 0/8] Honour current process label when generating SELinux labels
This patch series makes a number of changes to the SELinux label generation code. This is intended to make it fully honour the current process label when generating VM labels, so that dynamic label generation works better with custom policies, or confined user accounts.
From: "Daniel P. Berrange" <berrange@redhat.com> The current virRandomBits() API is only usable if the caller wants a random number in the range [0, (n-1)] where n is a power of two. This adds a virRandom() API which works for upper limits which are not a power of two. It works by using virRandomBits() to generate a number to the next nearest power of 2 limit, and then scales it down. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virrandom.c | 34 ++++++++++++++++++++++++++++++++++ src/util/virrandom.h | 1 + 3 files changed, 36 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c023dbf..d406785 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1640,6 +1640,7 @@ virPidFileDeletePath; # virrandom.h +virRandom; virRandomBits; virRandomGenerateWWN; diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 50bed46..260cc82 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -108,6 +108,40 @@ uint64_t virRandomBits(int nbits) return ret; } + +/** + * virRandom: + * @max: Upper bound on random number (not inclusive) + * + * Generate an evenly distributed random number between [0,max-1] + * If @max is a power of 2, then use virRandomBits instead + * + * Return: a random number with @nbits entropy + */ +uint64_t virRandom(unsigned long long max) +{ + int bits = 0; + unsigned long long tmp = max - 1; + uint64_t ret; + + while (tmp) { + tmp >>= 1; + bits++; + } + + ret = virRandomBits(bits); + + if ((1 << bits) != max) { + double d = ret; + d *= max; + d /= (1 << bits); + ret = (uint64_t)d; + } + + return ret; +} + + #define QUMRANET_OUI "001a4a" #define VMWARE_OUI "000569" #define MICROSOFT_OUI "0050f2" diff --git a/src/util/virrandom.h b/src/util/virrandom.h index 29a055d..16d9fc7 100644 --- a/src/util/virrandom.h +++ b/src/util/virrandom.h @@ -25,6 +25,7 @@ # include "internal.h" uint64_t virRandomBits(int nbits); +uint64_t virRandom(unsigned long long max); int virRandomGenerateWWN(char **wwn, const char *virt_type); #endif /* __VIR_RANDOM_H__ */ -- 1.7.11.2
On 08/10/2012 07:47 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The current virRandomBits() API is only usable if the caller wants a random number in the range [0, (n-1)] where n is a power of two. This adds a virRandom() API which works for upper limits which are not a power of two. It works by using virRandomBits() to generate a number to the next nearest power of 2 limit, and then scales it down.
I like the idea, but NAK to this version of the patch. That algorithm is not uniform.
+/** + * virRandom: + * @max: Upper bound on random number (not inclusive) + * + * Generate an evenly distributed random number between [0,max-1] + * If @max is a power of 2, then use virRandomBits instead + * + * Return: a random number with @nbits entropy + */ +uint64_t virRandom(unsigned long long max) +{ + int bits = 0; + unsigned long long tmp = max - 1; + uint64_t ret; + + while (tmp) { + tmp >>= 1; + bits++; + }
Slow. I would suggest using gcc's __builtin_clz (the way qemu does), except that gnulib doesn't yet have a module exposing it. I guess that means I'd better write a quick gnulib module. Once you have clz(), this is much simpler as: assert(max); bits = 64 - clz(max);
+ + ret = virRandomBits(bits); + + if ((1 << bits) != max) { + double d = ret; + d *= max; + d /= (1 << bits); + ret = (uint64_t)d; + }
Wrong. Consider when max == 3 (i.e. generate a random choice between 0, 1, or 2). bits is 2, so d starts as 0, 1, 2, or 3, each with 25% probability. Then after this computation: 0 -> 0*3 / 4.0 -> 0 1 -> 1*3 / 4.0 -> 0 2 -> 2*3 / 4.0 -> 1 3 -> 3*3 / 4.0 -> 2 that is, you have a 50% chance of getting 0, but only a 25% chance of getting 1 or 2 - that is not a uniform distribution. The ONLY way to do this is: do { ret = virRandomBits(bits); } while (ret >= max); Then, with the same input of max == 3, you have a 25% chance of calling virRandomBits at least twice, a 12.5% chance of calling it at least three times, and so on, but you are guaranteed that you will eventually get a result that is uniformly distributed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Aug 10, 2012 at 08:58:04AM -0600, Eric Blake wrote:
On 08/10/2012 07:47 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The current virRandomBits() API is only usable if the caller wants a random number in the range [0, (n-1)] where n is a power of two. This adds a virRandom() API which works for upper limits which are not a power of two. It works by using virRandomBits() to generate a number to the next nearest power of 2 limit, and then scales it down.
I like the idea, but NAK to this version of the patch. That algorithm is not uniform.
+/** + * virRandom: + * @max: Upper bound on random number (not inclusive) + * + * Generate an evenly distributed random number between [0,max-1] + * If @max is a power of 2, then use virRandomBits instead + * + * Return: a random number with @nbits entropy + */ +uint64_t virRandom(unsigned long long max) +{ + int bits = 0; + unsigned long long tmp = max - 1; + uint64_t ret; + + while (tmp) { + tmp >>= 1; + bits++; + }
Slow. I would suggest using gcc's __builtin_clz (the way qemu does), except that gnulib doesn't yet have a module exposing it. I guess that means I'd better write a quick gnulib module. Once you have clz(), this is much simpler as:
assert(max); bits = 64 - clz(max);
+ + ret = virRandomBits(bits); + + if ((1 << bits) != max) { + double d = ret; + d *= max; + d /= (1 << bits); + ret = (uint64_t)d; + }
Wrong. Consider when max == 3 (i.e. generate a random choice between 0, 1, or 2). bits is 2, so d starts as 0, 1, 2, or 3, each with 25% probability. Then after this computation: 0 -> 0*3 / 4.0 -> 0 1 -> 1*3 / 4.0 -> 0 2 -> 2*3 / 4.0 -> 1 3 -> 3*3 / 4.0 -> 2
that is, you have a 50% chance of getting 0, but only a 25% chance of getting 1 or 2 - that is not a uniform distribution. The ONLY way to do this is:
do { ret = virRandomBits(bits); } while (ret >= max);
Then, with the same input of max == 3, you have a 25% chance of calling virRandomBits at least twice, a 12.5% chance of calling it at least three times, and so on, but you are guaranteed that you will eventually get a result that is uniformly distributed.
Hmm, it was the looping that I used originally, but I wanted to get something that had a deterministic upper bound, instead of just a probablistic bound. The alternative I wanted to use was drand48_t() which returns a double in the range 0.0 -> 1.0, with 48 bits of entropy. You could multiply by 'max', and cast to int. But this isn't portable and is not fixed by GNULIB. I'm wondering if we could emulate drand48_t() ourselves by doing something like this: #include <ieee754.h> double virRandom(void) { union ieee754_double val; val.ieee.negative = 0; val.ieee.exponent = IEEE754_DOUBLE_BIAS; val.ieee.mantissa0 = virRandomBits(20); val.ieee.mantissa0 = virRandomBits(32); return val.d - 1.0; } int virRandomInt(int max) { return virRandom() * max; } Guess it depends whether ieee754.h is portable ? Or can we in fact just define that union ourselves ? union ieee754_double { double d; /* This is the IEEE 754 double-precision format. */ struct { #if __BYTE_ORDER == __BIG_ENDIAN unsigned int negative:1; unsigned int exponent:11; /* Together these comprise the mantissa. */ unsigned int mantissa0:20; unsigned int mantissa1:32; #endif /* Big endian. */ #if __BYTE_ORDER == __LITTLE_ENDIAN # if __FLOAT_WORD_ORDER == __BIG_ENDIAN unsigned int mantissa0:20; unsigned int exponent:11; unsigned int negative:1; unsigned int mantissa1:32; # else /* Together these comprise the mantissa. */ unsigned int mantissa1:32; unsigned int mantissa0:20; unsigned int exponent:11; unsigned int negative:1; # endif #endif /* Little endian. */ } ieee; }; Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 08/10/2012 09:31 AM, Daniel P. Berrange wrote:
On Fri, Aug 10, 2012 at 08:58:04AM -0600, Eric Blake wrote:
On 08/10/2012 07:47 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The current virRandomBits() API is only usable if the caller wants a random number in the range [0, (n-1)] where n is a power of two. This adds a virRandom() API which works for upper limits which are not a power of two. It works by using virRandomBits() to generate a number to the next nearest power of 2 limit, and then scales it down.
I like the idea, but NAK to this version of the patch. That algorithm is not uniform.
+ while (tmp) { + tmp >>= 1; + bits++; + }
Slow. I would suggest using gcc's __builtin_clz (the way qemu does), except that gnulib doesn't yet have a module exposing it. I guess that means I'd better write a quick gnulib module. Once you have clz(), this is much simpler as:
assert(max); bits = 64 - clz(max);
This part is independently useful, while we still discuss the algorithm; I'll have the gnulib module complete later today.
that is, you have a 50% chance of getting 0, but only a 25% chance of getting 1 or 2 - that is not a uniform distribution. The ONLY way to do this is:
do { ret = virRandomBits(bits); } while (ret >= max);
Then, with the same input of max == 3, you have a 25% chance of calling virRandomBits at least twice, a 12.5% chance of calling it at least three times, and so on, but you are guaranteed that you will eventually get a result that is uniformly distributed.
Hmm, it was the looping that I used originally, but I wanted to get something that had a deterministic upper bound, instead of just a probablistic bound.
Oh, I see your point - you don't want to drag on for a long time on the rare occasions where probabilities are against you.
The alternative I wanted to use was drand48_t() which returns a double in the range 0.0 -> 1.0, with 48 bits of entropy. You could multiply by 'max', and cast to int. But this isn't portable and is not fixed by GNULIB.
Might not be too hard to make drand48_r into a gnulib module (harder than clz, but not intractible, so I'll take a shot at it, but no promises). Using drand48_r does still hit non-uniformity due to rounding, but with much smaller fuzz factor (that is, instead of my example of 50/25/25, you'd be more like 25.000004, 24.999993, 24.999993), which I can live with as being in the noise. Furthermore, since it only gives 48 bits of entropy, if 'max' is larger than 1<<47 you'll have to do some legwork yourself (I'm not quite sure what that legwork involves to still be fair), so I'd feel more comfortable if we capped this function to uint32_t and require the use of powers-of-2 for anything larger than a 32-bit max.
I'm wondering if we could emulate drand48_t() ourselves by doing something like this:
#include <ieee754.h>
double virRandom(void) { union ieee754_double val;
val.ieee.negative = 0; val.ieee.exponent = IEEE754_DOUBLE_BIAS; val.ieee.mantissa0 = virRandomBits(20); val.ieee.mantissa0 = virRandomBits(32);
return val.d - 1.0; }
No need to do that. -lm already gives us a very nice function: ldexp(). Basically, you take an integer in the range [0,1<<48), then multiply that by 2**-48 using ldexp(). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/10/2012 10:06 AM, Eric Blake wrote:
The alternative I wanted to use was drand48_t() which returns a double
drand48_r
in the range 0.0 -> 1.0, with 48 bits of entropy. You could multiply by 'max', and cast to int. But this isn't portable and is not fixed by GNULIB.
Might not be too hard to make drand48_r into a gnulib module (harder than clz, but not intractible, so I'll take a shot at it, but no promises). Using drand48_r does still hit non-uniformity due to rounding, but with much smaller fuzz factor (that is, instead of my example of 50/25/25, you'd be more like 25.000004, 24.999993, 24.999993), which I can live with as being in the noise.
If it wasn't obvious, I was typing this off the cuff rather than giving actual numbers (still off the cuff, but real numbers would be more like 33.3333334, 33.3333333, 33.3333333 - something that actually adds up to 100% distribution).
No need to do that. -lm already gives us a very nice function: ldexp(). Basically, you take an integer in the range [0,1<<48), then multiply that by 2**-48 using ldexp().
As we discussed on IRC, no need for drand48[_r], just use ldexp(virRandomBits(48), -48). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/10/2012 08:58 AM, Eric Blake wrote:
On 08/10/2012 07:47 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The current virRandomBits() API is only usable if the caller wants a random number in the range [0, (n-1)] where n is a power of two. This adds a virRandom() API which works for upper limits which are not a power of two. It works by using virRandomBits() to generate a number to the next nearest power of 2 limit, and then scales it down.
+ + while (tmp) { + tmp >>= 1; + bits++; + }
Slow. I would suggest using gcc's __builtin_clz (the way qemu does), except that gnulib doesn't yet have a module exposing it. I guess that means I'd better write a quick gnulib module. Once you have clz(), this is much simpler as:
assert(max); bits = 64 - clz(max);
Gnulib side is done. If you apply this patch to libvirt (including an update to the latest gnulib), then you can #include "count-leading-zeros.h" and call count_leading_zeros_ll(max) (spelled that way in gnulib rather than clz to minimize potential clashes, since it is not a standardized function). diff --git i/.gnulib w/.gnulib index dbd9144..0da76a9 160000 --- i/.gnulib +++ w/.gnulib @@ -1 +1 @@ -Subproject commit dbd914496c99c52220e5f5ba4121d6cb55fb3beb +Subproject commit 0da76a94c8668319c68af5e8c2e71c80528db5de diff --git i/bootstrap.conf w/bootstrap.conf index c112ccd..58558c8 100644 --- i/bootstrap.conf +++ w/bootstrap.conf @@ -33,6 +33,7 @@ chown close connect configmake +count-leading-zeros count-one-bits crypto/md5 dirname-lgpl -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> The security_manager.h header is not self-contained because it uses the virDomainDefPtr without first including domain_conf.h Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_manager.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 36d801b..325fc1e 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -23,6 +23,8 @@ #ifndef VIR_SECURITY_MANAGER_H__ # define VIR_SECURITY_MANAGER_H__ +# include "domain_conf.h" + typedef struct _virSecurityManager virSecurityManager; typedef virSecurityManager *virSecurityManagerPtr; -- 1.7.11.2
On 08/10/2012 07:47 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The security_manager.h header is not self-contained because it uses the virDomainDefPtr without first including domain_conf.h
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_manager.h | 2 ++ 1 file changed, 2 insertions(+)
ACK; trivial. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> There is currently no way to distinguish the case that a requested security driver was disabled, from the case where no security driver was available. Use VIR_ERR_CONFIG_UNSUPPORTED as the error when an explicitly requested security driver was disabled Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/security/security_driver.c b/src/security/security_driver.c index 7ff5f17..f450a94 100644 --- a/src/security/security_driver.c +++ b/src/security/security_driver.c @@ -72,6 +72,12 @@ virSecurityDriverPtr virSecurityDriverLookup(const char *name, case SECURITY_DRIVER_DISABLE: VIR_DEBUG("Not enabled name=%s", tmp->name); + if (name && STREQ(tmp->name, name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Security driver %s not enabled"), + name); + return NULL; + } break; default: -- 1.7.11.2
On 08/10/2012 07:47 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
There is currently no way to distinguish the case that a requested security driver was disabled, from the case where no security driver was available. Use VIR_ERR_CONFIG_UNSUPPORTED as the error when an explicitly requested security driver was disabled
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/security/security_driver.c b/src/security/security_driver.c index 7ff5f17..f450a94 100644 --- a/src/security/security_driver.c +++ b/src/security/security_driver.c @@ -72,6 +72,12 @@ virSecurityDriverPtr virSecurityDriverLookup(const char *name,
case SECURITY_DRIVER_DISABLE: VIR_DEBUG("Not enabled name=%s", tmp->name); + if (name && STREQ(tmp->name, name)) {
Possibly simpler as 'if (STREQ_NULLABLE(tmp->name, name))'
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Security driver %s not enabled"), + name); + return NULL; + }
ACK, whether or not you micro-optimize. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> The virSecuritySELinuxGenNewContext method was not reporting any errors, leaving it upto the caller to report a generic error. In addition it could potentially trigger a strdup(NULL) in an OOM scenario. Move all error porting into the virSecuritySELinuxGenNewContext method where accurate info can be provided Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_selinux.c | 67 +++++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 30 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index ca19b70..1b5c02e 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -99,20 +99,37 @@ virSecuritySELinuxMCSRemove(virSecurityManagerPtr mgr, } static char * -virSecuritySELinuxGenNewContext(const char *oldcontext, const char *mcs) +virSecuritySELinuxGenNewContext(const char *basecontext, const char *mcs) { - char *newcontext = NULL; - char *scontext = strdup(oldcontext); - context_t con; - if (!scontext) goto err; - con = context_new(scontext); - if (!con) goto err; - context_range_set(con, mcs); - newcontext = strdup(context_str(con)); - context_free(con); -err: - freecon(scontext); - return newcontext; + context_t context; + char *ret = NULL; + char *str; + + if (!(context = context_new(basecontext))) { + virReportSystemError(errno, + _("Unable to parse base SELinux context '%s'"), + basecontext); + goto cleanup; + } + + if (context_range_set(context, mcs) != 0) { + virReportSystemError(errno, + _("Unable to set SELinux context MCS '%s'"), + mcs); + goto cleanup; + } + if (!(str = context_str(context))) { + virReportSystemError(errno, "%s", + _("Unable to format SELinux context")); + goto cleanup; + } + if (!(ret = strdup(str))) { + virReportOOMError(); + goto cleanup; + } +cleanup: + context_free(context); + return ret; } @@ -348,15 +365,11 @@ virSecuritySELinuxGenSecurityLabel(virSecurityManagerPtr mgr, break; } - def->seclabel.label = - virSecuritySELinuxGenNewContext(def->seclabel.baselabel ? - def->seclabel.baselabel : - data->domain_context, mcs); - if (! def->seclabel.label) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot generate selinux context for %s"), mcs); + if (!(def->seclabel.label = + virSecuritySELinuxGenNewContext(def->seclabel.baselabel ? + def->seclabel.baselabel : + data->domain_context, mcs))) goto cleanup; - } break; case VIR_DOMAIN_SECLABEL_NONE: @@ -371,12 +384,9 @@ virSecuritySELinuxGenSecurityLabel(virSecurityManagerPtr mgr, } if (!def->seclabel.norelabel) { - def->seclabel.imagelabel = virSecuritySELinuxGenNewContext(data->file_context, mcs); - if (!def->seclabel.imagelabel) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot generate selinux context for %s"), mcs); + if (!(def->seclabel.imagelabel = + virSecuritySELinuxGenNewContext(data->file_context, mcs))) goto cleanup; - } } if (!def->seclabel.model && @@ -1576,11 +1586,8 @@ virSecuritySELinuxGenImageLabel(virSecurityManagerPtr mgr, virReportOOMError(); goto cleanup; } - label = virSecuritySELinuxGenNewContext(data->file_context, mcs); - if (!label) { - virReportOOMError(); + if (!(label = virSecuritySELinuxGenNewContext(data->file_context, mcs))) goto cleanup; - } } } -- 1.7.11.2
On 08/10/2012 07:47 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The virSecuritySELinuxGenNewContext method was not reporting any errors, leaving it upto the caller to report a generic error.
s/upto/up to/
In addition it could potentially trigger a strdup(NULL) in an OOM scenario. Move all error porting into the
s/porting/reporting/
virSecuritySELinuxGenNewContext method where accurate info can be provided
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_selinux.c | 67 +++++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 30 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> When generating an SELinux context for a VM from the template "system_u:system_r:svirt_t:s0", copy the role + user from the current process instead of the template context. So if the current process is unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 then the VM context ends up as unconfined_u:unconfined_r:svirt_t:s0:c386,c703 instead of system_u:system_r:svirt_t:s0:c177,c424 Ideally the /etc/selinux/targeted/contexts/virtual_domain_context file would have just shown the 'svirt_t' type, and not the full context, but that can't be changed now for compatibility reasons. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_selinux.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 1b5c02e..5c917ea 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -101,9 +101,23 @@ virSecuritySELinuxMCSRemove(virSecurityManagerPtr mgr, static char * virSecuritySELinuxGenNewContext(const char *basecontext, const char *mcs) { - context_t context; + context_t context = NULL; char *ret = NULL; char *str; + security_context_t curseccontext = NULL; + context_t curcontext = NULL; + + if (getcon(&curseccontext) < 0) { + virReportSystemError(errno, "%s", + _("Unable to get current process SELinux context")); + goto cleanup; + } + if (!(curcontext = context_new(curseccontext))) { + virReportSystemError(errno, + _("Unable to parse current SELinux context '%s'"), + curseccontext); + goto cleanup; + } if (!(context = context_new(basecontext))) { virReportSystemError(errno, @@ -112,6 +126,22 @@ virSecuritySELinuxGenNewContext(const char *basecontext, const char *mcs) goto cleanup; } + if (context_user_set(context, + context_user_get(curcontext)) != 0) { + virReportSystemError(errno, + _("Unable to set SELinux context user '%s'"), + context_user_get(curcontext)); + goto cleanup; + } + + if (context_role_set(context, + context_role_get(curcontext)) != 0) { + virReportSystemError(errno, + _("Unable to set SELinux context user '%s'"), + context_role_get(curcontext)); + goto cleanup; + } + if (context_range_set(context, mcs) != 0) { virReportSystemError(errno, _("Unable to set SELinux context MCS '%s'"), @@ -127,7 +157,11 @@ virSecuritySELinuxGenNewContext(const char *basecontext, const char *mcs) virReportOOMError(); goto cleanup; } + VIR_DEBUG("Generated context '%s' from '%s' and '%s'", + ret, basecontext, curseccontext); cleanup: + freecon(curseccontext); + context_free(curcontext); context_free(context); return ret; } -- 1.7.11.2
On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
When generating an SELinux context for a VM from the template "system_u:system_r:svirt_t:s0", copy the role + user from the current process instead of the template context. So if the current process is
unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
then the VM context ends up as
unconfined_u:unconfined_r:svirt_t:s0:c386,c703
instead of
system_u:system_r:svirt_t:s0:c177,c424
virSecuritySELinuxGenNewContext(const char *basecontext, const char *mcs) { - context_t context; + context_t context = NULL; char *ret = NULL; char *str; + security_context_t curseccontext = NULL;
When I first read this, I wondered why you felt the context of the C language was worth cursing - is it really hard to manage security labels in the C language? Adding some underscores would not hurt, since you meant cur_sec_context and not curse_c_context :) ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Aug 10, 2012 at 02:55:24PM -0600, Eric Blake wrote:
On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
When generating an SELinux context for a VM from the template "system_u:system_r:svirt_t:s0", copy the role + user from the current process instead of the template context. So if the current process is
unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
then the VM context ends up as
unconfined_u:unconfined_r:svirt_t:s0:c386,c703
instead of
system_u:system_r:svirt_t:s0:c177,c424
virSecuritySELinuxGenNewContext(const char *basecontext, const char *mcs) { - context_t context; + context_t context = NULL; char *ret = NULL; char *str; + security_context_t curseccontext = NULL;
When I first read this, I wondered why you felt the context of the C language was worth cursing - is it really hard to manage security labels in the C language? Adding some underscores would not hurt, since you meant cur_sec_context and not curse_c_context :)
Changed it to ourSecContext :-) Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
From: "Daniel P. Berrange" <berrange@redhat.com> The code for picking a MCS label is about to get significantly more complicated, so it deserves to be in a standlone method, instead of a switch/case body. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_selinux.c | 75 ++++++++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 28 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 5c917ea..4963ef5 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -98,6 +98,48 @@ virSecuritySELinuxMCSRemove(virSecurityManagerPtr mgr, virHashRemoveEntry(data->mcs, mcs); } + +static char * +virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) +{ + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); + int c1 = 0; + int c2 = 0; + char *mcs = NULL; + + for (;;) { + c1 = virRandomBits(10); + c2 = virRandomBits(10); + + if (c1 == c2) { + if (virAsprintf(&mcs, "s0:c%d", c1) < 0) { + virReportOOMError(); + return NULL; + } + } else { + if (c1 > c2) { + c1 ^= c2; + c2 ^= c1; + c1 ^= c2; + } + if (virAsprintf(&mcs, "s0:c%d,c%d", c1, c2) < 0) { + virReportOOMError(); + return NULL; + } + } + + if (virHashLookup(data->mcs, mcs) == NULL) + goto cleanup; + + VIR_FREE(mcs); + } + +cleanup: + VIR_DEBUG("Found context '%s'", NULLSTR(mcs)); + return mcs; +} + + static char * virSecuritySELinuxGenNewContext(const char *basecontext, const char *mcs) { @@ -316,8 +358,6 @@ virSecuritySELinuxGenSecurityLabel(virSecurityManagerPtr mgr, int rc = -1; char *mcs = NULL; char *scontext = NULL; - int c1 = 0; - int c2 = 0; context_t ctx = NULL; const char *range; virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); @@ -372,32 +412,11 @@ virSecuritySELinuxGenSecurityLabel(virSecurityManagerPtr mgr, break; case VIR_DOMAIN_SECLABEL_DYNAMIC: - for (;;) { - int rv; - c1 = virRandomBits(10); - c2 = virRandomBits(10); - - if ( c1 == c2 ) { - if (virAsprintf(&mcs, "s0:c%d", c1) < 0) { - virReportOOMError(); - goto cleanup; - } - } else { - if (c1 > c2) { - c1 ^= c2; - c2 ^= c1; - c1 ^= c2; - } - if (virAsprintf(&mcs, "s0:c%d,c%d", c1, c2) < 0) { - virReportOOMError(); - goto cleanup; - } - } - if ((rv = virSecuritySELinuxMCSAdd(mgr, mcs)) < 0) - goto cleanup; - if (rv == 0) - break; - } + if (!(mcs = virSecuritySELinuxMCSFind(mgr))) + goto cleanup; + + if (virSecuritySELinuxMCSAdd(mgr, mcs) < 0) + goto cleanup; if (!(def->seclabel.label = virSecuritySELinuxGenNewContext(def->seclabel.baselabel ? -- 1.7.11.2
On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The code for picking a MCS label is about to get significantly more complicated, so it deserves to be in a standlone method, instead of a switch/case body.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_selinux.c | 75 ++++++++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 28 deletions(-)
ACK.
+ +static char * +virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) +{ + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); + int c1 = 0; + int c2 = 0; + char *mcs = NULL; + + for (;;) { + c1 = virRandomBits(10); + c2 = virRandomBits(10); + + if (c1 == c2) { + if (virAsprintf(&mcs, "s0:c%d", c1) < 0) { + virReportOOMError(); + return NULL; + } + } else { + if (c1 > c2) { + c1 ^= c2; + c2 ^= c1; + c1 ^= c2;
I know this is just code motion, but is it really that hard to just write the more straightforward: int tmp = c1; c1 = c2; c2 = tmp; which probably optimizes better in the compiler? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Aug 10, 2012 at 03:02:24PM -0600, Eric Blake wrote:
On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The code for picking a MCS label is about to get significantly more complicated, so it deserves to be in a standlone method, instead of a switch/case body.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_selinux.c | 75 ++++++++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 28 deletions(-)
ACK.
+ +static char * +virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) +{ + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); + int c1 = 0; + int c2 = 0; + char *mcs = NULL; + + for (;;) { + c1 = virRandomBits(10); + c2 = virRandomBits(10); + + if (c1 == c2) { + if (virAsprintf(&mcs, "s0:c%d", c1) < 0) { + virReportOOMError(); + return NULL; + } + } else { + if (c1 > c2) { + c1 ^= c2; + c2 ^= c1; + c1 ^= c2;
I know this is just code motion, but is it really that hard to just write the more straightforward:
int tmp = c1; c1 = c2; c2 = tmp;
which probably optimizes better in the compiler?
Ok, changed this. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
From: "Daniel P. Berrange" <berrange@redhat.com> Currently the dynamic label generation code will create labels with a sensitivity of s0, and a category pair in the range 0-1023. This is fine when running a standard MCS policy because libvirtd will run with a label system_u:system_r:virtd_t:s0-s0:c0.c1023 With custom policies though, it is possible for libvirtd to have a different sensitivity, or category range. For example system_u:system_r:virtd_t:s2-s3:c512.c1023 In this case we must assign the VM a sensitivity matching the current lower sensitivity value, and categories in the range 512-1023 Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/security/security_selinux.c | 88 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 84 insertions(+), 4 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 4963ef5..714ec4d 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -106,13 +106,90 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) int c1 = 0; int c2 = 0; char *mcs = NULL; + security_context_t curseccontext = NULL; + context_t curcontext = NULL; + char *sens, *cat, *tmp; + int catMin, catMax, catRange; + + if (getcon(&curseccontext) < 0) { + virReportSystemError(errno, "%s", + _("Unable to get current process SELinux context")); + goto cleanup; + } + if (!(curcontext = context_new(curseccontext))) { + virReportSystemError(errno, + _("Unable to parse current SELinux context '%s'"), + curseccontext); + goto cleanup; + } + + if (!(sens = strdup(context_range_get(curcontext)))) { + virReportOOMError(); + goto cleanup; + } + + /* Find and blank out the category part */ + if (!(tmp = strchr(sens, ':'))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse sensitivity level in %s"), + sens); + goto cleanup; + } + *tmp = '\0'; + cat = tmp + 1; + /* Find and blank out the sensitivity upper bound */ + if ((tmp = strchr(sens, '-'))) + *tmp = '\0'; + + /* sens now just contains the sensitivity lower bound */ + tmp = cat; + if (tmp[0] != 'c') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + } + tmp++; + if (virStrToLong_i(tmp, &tmp, 10, &catMin) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + } + if (tmp[0] != '.') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + } + tmp++; + if (tmp[0] != 'c') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + } + tmp++; + if (virStrToLong_i(tmp, &tmp, 10, &catMax) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + } + + /* max is inclusive, hence the + 1 */ + catRange = (catMax - catMin) + 1; + + VIR_DEBUG("Using sensitivity level '%s' cat min %d max %d range %d", + sens, catMin, catMax, catRange); for (;;) { - c1 = virRandomBits(10); - c2 = virRandomBits(10); + c1 = virRandom(catRange); + c2 = virRandom(catRange); + VIR_DEBUG("Try cat %s:c%d,c%d", sens, c1, c2); if (c1 == c2) { - if (virAsprintf(&mcs, "s0:c%d", c1) < 0) { + if (virAsprintf(&mcs, "%s:c%d", sens, catMin + c1) < 0) { virReportOOMError(); return NULL; } @@ -122,7 +199,7 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) c2 ^= c1; c1 ^= c2; } - if (virAsprintf(&mcs, "s0:c%d,c%d", c1, c2) < 0) { + if (virAsprintf(&mcs, "%s:c%d,c%d", sens, catMin + c1, catMin + c2) < 0) { virReportOOMError(); return NULL; } @@ -136,6 +213,9 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) cleanup: VIR_DEBUG("Found context '%s'", NULLSTR(mcs)); + VIR_FREE(sens); + freecon(curseccontext); + context_free(curcontext); return mcs; } -- 1.7.11.2
On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Currently the dynamic label generation code will create labels with a sensitivity of s0, and a category pair in the range 0-1023. This is fine when running a standard MCS policy because libvirtd will run with a label
system_u:system_r:virtd_t:s0-s0:c0.c1023
With custom policies though, it is possible for libvirtd to have a different sensitivity, or category range. For example
system_u:system_r:virtd_t:s2-s3:c512.c1023
In this case we must assign the VM a sensitivity matching the current lower sensitivity value, and categories in the range 512-1023
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
+++ b/src/security/security_selinux.c @@ -106,13 +106,90 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) int c1 = 0; int c2 = 0; char *mcs = NULL; + security_context_t curseccontext = NULL;
More of that fun naming. Underscoresreallydomakeiteasiertoread.
+ context_t curcontext = NULL; + char *sens, *cat, *tmp; + int catMin, catMax, catRange;
OK, so camel case would also make things easier to read. I don't know if we have a coding guideline which says which to use, but consistency argues for the same type of separation for all local variables in a single function.
+ + if (getcon(&curseccontext) < 0) { + virReportSystemError(errno, "%s", + _("Unable to get current process SELinux context")); + goto cleanup; + } + if (!(curcontext = context_new(curseccontext))) { + virReportSystemError(errno, + _("Unable to parse current SELinux context '%s'"), + curseccontext); + goto cleanup; + } + + if (!(sens = strdup(context_range_get(curcontext)))) {
Just to make sure I'm following here, I'll assume two different strings for sens at this point; one from your commit message (s2-s3:c512.c1023) and one from a degenerate s0:c0 (as the previous patch showed that libvirt will create a single context instead of a range if two random numbers match).
+ virReportOOMError(); + goto cleanup; + } + + /* Find and blank out the category part */ + if (!(tmp = strchr(sens, ':'))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse sensitivity level in %s"), + sens); + goto cleanup; + } + *tmp = '\0'; + cat = tmp + 1;
so here, with my examples, we would sens with s2-s3 (or s0), and cat with c512.c1023 (or c0)
+ /* Find and blank out the sensitivity upper bound */ + if ((tmp = strchr(sens, '-'))) + *tmp = '\0';
and here, sens is now shortened to s2 (or still s0)
+ + /* sens now just contains the sensitivity lower bound */ + tmp = cat;
The spacing for this comment was awkward; it made me look for sens in the next block of code, even though it is an assertion about the results after the prior block of code.
+ if (tmp[0] != 'c') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + } + tmp++; + if (virStrToLong_i(tmp, &tmp, 10, &catMin) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + }
here, we've parsed out either 512 from c512.c1023, or 0 from c0
+ if (tmp[0] != '.') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup;
and here, we fall flat on our face if we were in the degenerate case of a single category. Oops. You need to start: if (!tmp[0]) { catMax = catMin; } else if (tmp[0] != '.') {
+ } + tmp++; + if (tmp[0] != 'c') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + } + tmp++; + if (virStrToLong_i(tmp, &tmp, 10, &catMax) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + }
and this parsed out 1023 on the c512.c1023 example.
+ + /* max is inclusive, hence the + 1 */ + catRange = (catMax - catMin) + 1; + + VIR_DEBUG("Using sensitivity level '%s' cat min %d max %d range %d", + sens, catMin, catMax, catRange);
for (;;) { - c1 = virRandomBits(10); - c2 = virRandomBits(10); + c1 = virRandom(catRange); + c2 = virRandom(catRange); + VIR_DEBUG("Try cat %s:c%d,c%d", sens, c1, c2);
This debug message would make more sense as c1+catMin, c2+catMin.
if (c1 == c2) { - if (virAsprintf(&mcs, "s0:c%d", c1) < 0) { + if (virAsprintf(&mcs, "%s:c%d", sens, catMin + c1) < 0) {
If you fix the parsing of the degenerate case, then for the c0 input case, you would always reach here, with catMin == 0 and c1 == 0 (since virRandom(1) == 0 - actually, you need to make sure that we don't trigger undefined behavior when calling virRandom(1), with whatever algorithm we finally end up with for that function; but I do agree that virRandom(0) should trigger an assertion failure.)
virReportOOMError(); return NULL; } @@ -122,7 +199,7 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) c2 ^= c1; c1 ^= c2; } - if (virAsprintf(&mcs, "s0:c%d,c%d", c1, c2) < 0) { + if (virAsprintf(&mcs, "%s:c%d,c%d", sens, catMin + c1, catMin + c2) < 0) {
and most of the time for the c512.c1023 case, you would get here with two random numbers more or less evenly distributed.
virReportOOMError(); return NULL; } @@ -136,6 +213,9 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr)
cleanup: VIR_DEBUG("Found context '%s'", NULLSTR(mcs)); + VIR_FREE(sens); + freecon(curseccontext); + context_free(curcontext); return mcs; }
Looks mostly sane. ACK if you clean up the degenerate context case. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Aug 10, 2012 at 03:20:58PM -0600, Eric Blake wrote:
On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
+ + if (getcon(&curseccontext) < 0) { + virReportSystemError(errno, "%s", + _("Unable to get current process SELinux context")); + goto cleanup; + } + if (!(curcontext = context_new(curseccontext))) { + virReportSystemError(errno, + _("Unable to parse current SELinux context '%s'"), + curseccontext); + goto cleanup; + } + + if (!(sens = strdup(context_range_get(curcontext)))) {
Just to make sure I'm following here, I'll assume two different strings for sens at this point; one from your commit message (s2-s3:c512.c1023) and one from a degenerate s0:c0 (as the previous patch showed that libvirt will create a single context instead of a range if two random numbers match).
Having a single sensitivity (s0) is valid, but having just a single category (c0) is not valid, since then we have no range of categories from which to allocate VM categories. I'd added code which explicitly raises an error in the case of a 'c0' category.
+ if (tmp[0] != '.') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup;
and here, we fall flat on our face if we were in the degenerate case of a single category. Oops. You need to start:
if (!tmp[0]) { catMax = catMin; } else if (tmp[0] != '.') {
I'm actually adding /* We *must* have a pair of categories otherwise * there's no range to allocate VM categories from */ if (!tmp[0]) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No category range available")); goto cleanup; }
+ } + tmp++; + if (tmp[0] != 'c') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + } + tmp++; + if (virStrToLong_i(tmp, &tmp, 10, &catMax) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse category in %s"), + cat); + goto cleanup; + }
and this parsed out 1023 on the c512.c1023 example.
+ + /* max is inclusive, hence the + 1 */ + catRange = (catMax - catMin) + 1; + + VIR_DEBUG("Using sensitivity level '%s' cat min %d max %d range %d", + sens, catMin, catMax, catRange);
And here I'm adding if (catRange < 8) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Category range c%d-c%d too small"), catMin, catMax); goto cleanup; } since we really need a few categories in the pool to play with.
for (;;) { - c1 = virRandomBits(10); - c2 = virRandomBits(10); + c1 = virRandom(catRange); + c2 = virRandom(catRange); + VIR_DEBUG("Try cat %s:c%d,c%d", sens, c1, c2);
This debug message would make more sense as c1+catMin, c2+catMin.
Yep.
if (c1 == c2) { - if (virAsprintf(&mcs, "s0:c%d", c1) < 0) { + if (virAsprintf(&mcs, "%s:c%d", sens, catMin + c1) < 0) {
If you fix the parsing of the degenerate case, then for the c0 input case, you would always reach here, with catMin == 0 and c1 == 0 (since virRandom(1) == 0 - actually, you need to make sure that we don't trigger undefined behavior when calling virRandom(1), with whatever algorithm we finally end up with for that function; but I do agree that virRandom(0) should trigger an assertion failure.)
The check on catRange will address this Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
From: "Daniel P. Berrange" <berrange@redhat.com> This test case validates the correct generation of SELinux labels for VMs, wrt the current process label. Since we can't actually change the label of the test program process, we create a shared library libsecurityselinuxhelper.so which overrides the getcon() and setcon() libselinux.so functions. When started the test case will check to see if LD_PRELOAD is set, and if not, it will re-exec() itself setting LD_PRELOAD=libsecurityselinuxhelper.so Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- .gitignore | 1 + tests/Makefile.am | 26 ++++ tests/securityselinuxhelper.c | 65 +++++++++ tests/securityselinuxtest.c | 300 ++++++++++++++++++++++++++++++++++++++++++ tests/testutils.h | 9 ++ 5 files changed, 401 insertions(+) create mode 100644 tests/securityselinuxhelper.c create mode 100644 tests/securityselinuxtest.c diff --git a/.gitignore b/.gitignore index 17dcae4..5041ddf 100644 --- a/.gitignore +++ b/.gitignore @@ -146,6 +146,7 @@ /tests/reconnect /tests/secaatest /tests/seclabeltest +/tests/securityselinuxtest /tests/sexpr2xmltest /tests/shunloadtest /tests/sockettest diff --git a/tests/Makefile.am b/tests/Makefile.am index 60d322d..e97a487 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -94,6 +94,10 @@ test_programs = virshtest sockettest \ virtimetest viruritest virkeyfiletest \ virauthconfigtest +if WITH_SECDRIVER_SELINUX +test_programs += securityselinuxtest +endif + if WITH_DRIVER_MODULES test_programs += virdrivermoduletest endif @@ -523,6 +527,28 @@ seclabeltest_SOURCES = \ seclabeltest.c seclabeltest_LDADD = $(LDADDS) +if WITH_SECDRIVER_SELINUX +if WITH_TESTS +noinst_LTLIBRARIES += libsecurityselinuxhelper.la +else +check_LTLIBRARIES += libsecurityselinuxhelper.la +endif + +libsecurityselinuxhelper_la_SOURCES = \ + securityselinuxhelper.c +libsecurityselinuxhelper_la_CFLAGS = $(AM_CFLAGS) +libsecurityselinuxhelper_la_LDFLAGS = -module -avoid-version \ + -rpath /evil/libtool/hack/to/force/shared/lib/creation + +securityselinuxtest_SOURCES = \ + securityselinuxtest.c testutils.h testutils.c +securityselinuxtest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) +securityselinuxtest_LDADD = $(LDADDS) +securityselinuxtest_DEPENDENCIES = libsecurityselinuxhelper.la +else +EXTRA_DIST += securityselinuxtest.c securityselinuxhelper.c +endif + virbuftest_SOURCES = \ virbuftest.c testutils.h testutils.c virbuftest_LDADD = $(LDADDS) diff --git a/tests/securityselinuxhelper.c b/tests/securityselinuxhelper.c new file mode 100644 index 0000000..744512a --- /dev/null +++ b/tests/securityselinuxhelper.c @@ -0,0 +1,65 @@ +/* + * Copyright (C) 2011-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include <selinux/selinux.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> +#include <errno.h> +/* + * The kernel policy will not allow us to arbitrarily change + * test process context. This helper is used as an LD_PRELOAD + * so that the libvirt code /thinks/ it is changing/reading + * the process context, where as in fact we're faking it all + */ + +int getcon(security_context_t *context) +{ + if (getenv("FAKE_CONTEXT") == NULL) { + *context = NULL; + errno = EINVAL; + return -1; + } + *context = strdup(getenv("FAKE_CONTEXT")); + return 0; +} + +int getpidcon(pid_t pid, security_context_t *context) +{ + if (pid != getpid()) { + *context = NULL; + errno = ESRCH; + return -1; + } + if (getenv("FAKE_CONTEXT") == NULL) { + *context = NULL; + errno = EINVAL; + return -1; + } + *context = strdup(getenv("FAKE_CONTEXT")); + return 0; +} + +int setcon(security_context_t context) +{ + return setenv("FAKE_CONTEXT", context, 1); +} diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c new file mode 100644 index 0000000..b8e85ab --- /dev/null +++ b/tests/securityselinuxtest.c @@ -0,0 +1,300 @@ +/* + * Copyright (C) 2011-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + */ + + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <time.h> + +#include <selinux/selinux.h> +#include <selinux/context.h> + +#include "internal.h" +#include "testutils.h" +#include "memory.h" +#include "util.h" +#include "logging.h" +#include "virterror_internal.h" +#include "security/security_manager.h" + + +#define VIR_FROM_THIS VIR_FROM_NONE + +struct testSELinuxGenLabelData { + virSecurityManagerPtr mgr; + + const char *pidcon; + + bool dynamic; + const char *label; + const char *baselabel; + + const char *user; + const char *role; + const char *type; + const char *imagetype; + + int sensMin; + int sensMax; + int catMin; + int catMax; +}; + +static virDomainDefPtr +testBuildDomainDef(bool dynamic, + const char *label, + const char *baselabel) +{ + virDomainDefPtr def; + + if (VIR_ALLOC(def) < 0) + goto no_memory; + + def->seclabel.type = dynamic ? VIR_DOMAIN_SECLABEL_DYNAMIC : VIR_DOMAIN_SECLABEL_STATIC; +// if (!(def->seclabel.model = strdup("selinux"))) +// goto no_memory; + + if (label && + !(def->seclabel.label = strdup(label))) + goto no_memory; + + if (baselabel && + !(def->seclabel.baselabel = strdup(baselabel))) + goto no_memory; + + return def; + +no_memory: + virReportOOMError(); + virDomainDefFree(def); + return NULL; +} + + +static bool +testSELinuxCheckCon(context_t con, + const char *user, + const char *role, + const char *type, + int sensMin, + int sensMax, + int catMin, + int catMax) +{ + const char *range; + char *tmp; + int gotSens; + int gotCatOne; + int gotCatTwo; + + if (STRNEQ(context_user_get(con), user)) { + fprintf(stderr, "Expect user %s got %s\n", + user, context_user_get(con)); + return false; + } + if (STRNEQ(context_role_get(con), role)) { + fprintf(stderr, "Expect role %s got %s\n", + role, context_role_get(con)); + return false; + } + if (STRNEQ(context_type_get(con), type)) { + fprintf(stderr, "Expect type %s got %s\n", + type, context_type_get(con)); + return false; + } + + range = context_range_get(con); + if (range[0] != 's') { + fprintf(stderr, "Malformed range %s, cannot find sensitivity\n", + range); + return false; + } + if (virStrToLong_i(range + 1, &tmp, 10, &gotSens) < 0 || + !tmp) { + fprintf(stderr, "Malformed range %s, cannot parse sensitivity\n", + range + 1); + return false; + } + if (*tmp != ':') { + fprintf(stderr, "Malformed range %s, too many sensitivity values\n", + tmp); + return false; + } + tmp++; + if (*tmp != 'c') { + fprintf(stderr, "Malformed range %s, cannot find first category\n", + tmp); + return false; + } + tmp++; + if (virStrToLong_i(tmp, &tmp, 10, &gotCatOne) < 0) { + fprintf(stderr, "Malformed range %s, cannot parse category one\n", + tmp); + return false; + } + if (tmp && *tmp == ',') + tmp++; + if (tmp && *tmp == 'c') { + tmp++; + if (virStrToLong_i(tmp, &tmp, 10, &gotCatTwo) < 0) { + fprintf(stderr, "Malformed range %s, cannot parse category two\n", + tmp); + return false; + } + } else { + gotCatTwo = gotCatOne; + } + + if (gotSens < sensMin || + gotSens > sensMax) { + fprintf(stderr, "Sensitivity %d is out of range %d-%d\n", + gotSens, sensMin, sensMax); + return false; + } + if (gotCatOne < catMin || + gotCatOne > catMax) { + fprintf(stderr, "Category one %d is out of range %d-%d\n", + gotCatTwo, catMin, catMax); + return false; + } + if (gotCatTwo < catMin || + gotCatTwo > catMax) { + fprintf(stderr, "Category two %d is out of range %d-%d\n", + gotCatTwo, catMin, catMax); + return false; + } + return true; +} + +static int +testSELinuxGenLabel(const void *opaque) +{ + const struct testSELinuxGenLabelData *data = opaque; + int ret = -1; + virDomainDefPtr def; + context_t con = NULL; + context_t imgcon = NULL; + + if (setcon((security_context_t)data->pidcon) < 0) { + perror("Cannot set process security context"); + return -1; + } + + if (!(def = testBuildDomainDef(data->dynamic, + data->label, + data->baselabel))) + goto cleanup; + + if (virSecurityManagerGenLabel(data->mgr, def) < 0) { + virErrorPtr err = virGetLastError(); + fprintf(stderr, "Cannot generated label %s\n", err->message); + goto cleanup; + } + + VIR_DEBUG("label=%s imagelabel=%s", + def->seclabel.label, def->seclabel.imagelabel); + + if (!(con = context_new(def->seclabel.label))) + goto cleanup; + if (!(imgcon = context_new(def->seclabel.imagelabel))) + goto cleanup; + + if (!testSELinuxCheckCon(con, + data->user, data->role, data->type, + data->sensMin, data->sensMax, + data->catMin, data->catMax)) + goto cleanup; + + if (!testSELinuxCheckCon(imgcon, + data->user, data->role, data->imagetype, + data->sensMin, data->sensMax, + data->catMin, data->catMax)) + goto cleanup; + + ret = 0; + +cleanup: + context_free(con); + context_free(imgcon); + virDomainDefFree(def); + return ret; +} + + + +static int +mymain(void) +{ + int ret = 0; + virSecurityManagerPtr mgr; + + if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true, false))) { + virErrorPtr err = virGetLastError(); + if (err->code == VIR_ERR_CONFIG_UNSUPPORTED) + exit(EXIT_AM_SKIP); + + fprintf(stderr, "Unable to initialize security driver: %s\n", + err->message); + exit(EXIT_FAILURE); + } + +#define DO_TEST_GEN_LABEL(desc, pidcon, \ + dynamic, label, baselabel, \ + user, role, type, imageType, \ + sensMin, sensMax, catMin, catMax) \ + do { \ + struct testSELinuxGenLabelData data = { \ + mgr, pidcon, dynamic, label, baselabel, \ + user, role, type, imageType, \ + sensMin, sensMax, catMin, catMax \ + }; \ + if (virtTestRun("GenLabel " # desc, 1, testSELinuxGenLabel, &data) < 0) \ + ret = -1; \ + } while (0) + + /* We can only run these tests if we're unconfined */ + DO_TEST_GEN_LABEL("dynamic unconfined, s0, c0.c1023", + "unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023", + true, NULL, NULL, + "unconfined_u", "unconfined_r", "svirt_t", "svirt_image_t", + 0, 0, 0, 1023); + DO_TEST_GEN_LABEL("dynamic virtd, s0, c0.c1023", + "system_u:system_r:virtd_t:s0-s0:c0.c1023", + true, NULL, NULL, + "system_u", "system_r", "svirt_t", "svirt_image_t", + 0, 0, 0, 1023); + DO_TEST_GEN_LABEL("dynamic virtd, s0, c0.c10", + "system_u:system_r:virtd_t:s0-s0:c0.c10", + true, NULL, NULL, + "system_u", "system_r", "svirt_t", "svirt_image_t", + 0, 0, 0, 10); + DO_TEST_GEN_LABEL("dynamic virtd, s2-s3, c0.c1023", + "system_u:system_r:virtd_t:s2-s3:c0.c1023", + true, NULL, NULL, + "system_u", "system_r", "svirt_t", "svirt_image_t", + 2, 3, 0, 1023); + + return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/libsecurityselinuxhelper.so") diff --git a/tests/testutils.h b/tests/testutils.h index f372c23..16414d9 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -70,4 +70,13 @@ int virtTestMain(int argc, return virtTestMain(argc, argv, func); \ } +# define VIRT_TEST_MAIN_PRELOAD(func, lib) \ + int main(int argc, char **argv) { \ + if (getenv("LD_PRELOAD") == NULL) { \ + setenv("LD_PRELOAD", lib, 1); \ + execv(argv[0], argv); \ + } \ + return virtTestMain(argc, argv, func); \ + } + #endif /* __VIT_TEST_UTILS_H__ */ -- 1.7.11.2
On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
This test case validates the correct generation of SELinux labels for VMs, wrt the current process label. Since we can't actually change the label of the test program process, we create a shared library libsecurityselinuxhelper.so which overrides the getcon() and setcon() libselinux.so functions. When started the test case will check to see if LD_PRELOAD is set, and if not, it will re-exec() itself setting LD_PRELOAD=libsecurityselinuxhelper.so
Since the test is already Linux only (hence the _name_ selinux), I'm okay with taking advantage of backdoors like this.
+++ b/tests/securityselinuxhelper.c @@ -0,0 +1,65 @@ +/* + * Copyright (C) 2011-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include <selinux/selinux.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> +#include <errno.h> +/* + * The kernel policy will not allow us to arbitrarily change + * test process context. This helper is used as an LD_PRELOAD + * so that the libvirt code /thinks/ it is changing/reading + * the process context, where as in fact we're faking it all + */ + +int getcon(security_context_t *context) +{ + if (getenv("FAKE_CONTEXT") == NULL) { + *context = NULL; + errno = EINVAL; + return -1; + } + *context = strdup(getenv("FAKE_CONTEXT"));
No check for allocation failure? (not that it's likely, but it never hurts to be thorough)
+ return 0; +} + +int getpidcon(pid_t pid, security_context_t *context) +{ + if (pid != getpid()) { + *context = NULL; + errno = ESRCH; + return -1; + } + if (getenv("FAKE_CONTEXT") == NULL) { + *context = NULL; + errno = EINVAL; + return -1; + } + *context = strdup(getenv("FAKE_CONTEXT"));
And again...
+++ b/tests/securityselinuxtest.c
+ +static virDomainDefPtr +testBuildDomainDef(bool dynamic, + const char *label, + const char *baselabel) +{ + virDomainDefPtr def; + + if (VIR_ALLOC(def) < 0) + goto no_memory; + + def->seclabel.type = dynamic ? VIR_DOMAIN_SECLABEL_DYNAMIC : VIR_DOMAIN_SECLABEL_STATIC; +// if (!(def->seclabel.model = strdup("selinux"))) +// goto no_memory;
Why the comments?
+static bool +testSELinuxCheckCon(context_t con, + const char *user, + const char *role, + const char *type, + int sensMin, + int sensMax, + int catMin, + int catMax) +{
+ tmp++; + if (virStrToLong_i(tmp, &tmp, 10, &gotCatOne) < 0) { + fprintf(stderr, "Malformed range %s, cannot parse category one\n", + tmp); + return false; + } + if (tmp && *tmp == ',')
Did you mean '.' instead of ','?
+ tmp++; + if (tmp && *tmp == 'c') { + tmp++; + if (virStrToLong_i(tmp, &tmp, 10, &gotCatTwo) < 0) { + fprintf(stderr, "Malformed range %s, cannot parse category two\n", + tmp); + return false; + } + } else { + gotCatTwo = gotCatOne; + }
Where do you check that theres no junk after the last category?
+ + if (gotSens < sensMin || + gotSens > sensMax) { + fprintf(stderr, "Sensitivity %d is out of range %d-%d\n", + gotSens, sensMin, sensMax); + return false; + }
Are you meaning to do a range check here (where input of s2-s3 could let libvirt choose s3), or actually enforcing that libvirt always picks the lowest end of the range?
+ if (gotCatOne < catMin || + gotCatOne > catMax) { + fprintf(stderr, "Category one %d is out of range %d-%d\n", + gotCatTwo, catMin, catMax); + return false; + } + if (gotCatTwo < catMin || + gotCatTwo > catMax) { + fprintf(stderr, "Category two %d is out of range %d-%d\n", + gotCatTwo, catMin, catMax); + return false; + }
No check that gotCatOne <= gotCatTwo?
+static int +mymain(void) +{
+ + /* We can only run these tests if we're unconfined */
Does the test gracefully skip when run in a different context?
+ DO_TEST_GEN_LABEL("dynamic unconfined, s0, c0.c1023", + "unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023", + true, NULL, NULL, + "unconfined_u", "unconfined_r", "svirt_t", "svirt_image_t", + 0, 0, 0, 1023); + DO_TEST_GEN_LABEL("dynamic virtd, s0, c0.c1023", + "system_u:system_r:virtd_t:s0-s0:c0.c1023", + true, NULL, NULL, + "system_u", "system_r", "svirt_t", "svirt_image_t", + 0, 0, 0, 1023); + DO_TEST_GEN_LABEL("dynamic virtd, s0, c0.c10", + "system_u:system_r:virtd_t:s0-s0:c0.c10", + true, NULL, NULL, + "system_u", "system_r", "svirt_t", "svirt_image_t", + 0, 0, 0, 10); + DO_TEST_GEN_LABEL("dynamic virtd, s2-s3, c0.c1023", + "system_u:system_r:virtd_t:s2-s3:c0.c1023", + true, NULL, NULL, + "system_u", "system_r", "svirt_t", "svirt_image_t", + 2, 3, 0, 1023);
No test of a degenerate single-range category?
+ + return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/libsecurityselinuxhelper.so") diff --git a/tests/testutils.h b/tests/testutils.h index f372c23..16414d9 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -70,4 +70,13 @@ int virtTestMain(int argc, return virtTestMain(argc, argv, func); \ }
+# define VIRT_TEST_MAIN_PRELOAD(func, lib) \ + int main(int argc, char **argv) { \ + if (getenv("LD_PRELOAD") == NULL) { \ + setenv("LD_PRELOAD", lib, 1); \ + execv(argv[0], argv); \
This is insufficient - if LD_PRELOAD is already set for other reasons (such as valgrind), then you still want to modify it and re-exec. I think you need to strstr() the existing contents for the new lib before deciding whether to re-exec. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Aug 10, 2012 at 03:50:25PM -0600, Eric Blake wrote:
On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
+++ b/tests/securityselinuxhelper.c @@ -0,0 +1,65 @@ +/* + * Copyright (C) 2011-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include <selinux/selinux.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> +#include <errno.h> +/* + * The kernel policy will not allow us to arbitrarily change + * test process context. This helper is used as an LD_PRELOAD + * so that the libvirt code /thinks/ it is changing/reading + * the process context, where as in fact we're faking it all + */ + +int getcon(security_context_t *context) +{ + if (getenv("FAKE_CONTEXT") == NULL) { + *context = NULL; + errno = EINVAL; + return -1; + } + *context = strdup(getenv("FAKE_CONTEXT"));
No check for allocation failure? (not that it's likely, but it never hurts to be thorough)
Ok, adding these
+static virDomainDefPtr +testBuildDomainDef(bool dynamic, + const char *label, + const char *baselabel) +{ + virDomainDefPtr def; + + if (VIR_ALLOC(def) < 0) + goto no_memory; + + def->seclabel.type = dynamic ? VIR_DOMAIN_SECLABEL_DYNAMIC : VIR_DOMAIN_SECLABEL_STATIC; +// if (!(def->seclabel.model = strdup("selinux"))) +// goto no_memory;
Why the comments?
Obsolete code, removing it.
+static bool +testSELinuxCheckCon(context_t con, + const char *user, + const char *role, + const char *type, + int sensMin, + int sensMax, + int catMin, + int catMax) +{
+ tmp++; + if (virStrToLong_i(tmp, &tmp, 10, &gotCatOne) < 0) { + fprintf(stderr, "Malformed range %s, cannot parse category one\n", + tmp); + return false; + } + if (tmp && *tmp == ',')
Did you mean '.' instead of ','?
No. Subtle distinction in semantics. 'cN.cM' is used to denote a category range, while 'cM,cM' is used to denate a category pair
+ tmp++; + if (tmp && *tmp == 'c') { + tmp++; + if (virStrToLong_i(tmp, &tmp, 10, &gotCatTwo) < 0) { + fprintf(stderr, "Malformed range %s, cannot parse category two\n", + tmp); + return false; + } + } else { + gotCatTwo = gotCatOne; + }
Where do you check that theres no junk after the last category?
Added a check for this.
+ + if (gotSens < sensMin || + gotSens > sensMax) { + fprintf(stderr, "Sensitivity %d is out of range %d-%d\n", + gotSens, sensMin, sensMax); + return false; + }
Are you meaning to do a range check here (where input of s2-s3 could let libvirt choose s3), or actually enforcing that libvirt always picks the lowest end of the range?
Good point, I'm changing this to enforce gotSens == sensMin
+ + /* We can only run these tests if we're unconfined */
Does the test gracefully skip when run in a different context?
This is an obsolete comment now - it predates my use of the LD_PRELOAD hack
+ DO_TEST_GEN_LABEL("dynamic unconfined, s0, c0.c1023", + "unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023", + true, NULL, NULL, + "unconfined_u", "unconfined_r", "svirt_t", "svirt_image_t", + 0, 0, 0, 1023); + DO_TEST_GEN_LABEL("dynamic virtd, s0, c0.c1023", + "system_u:system_r:virtd_t:s0-s0:c0.c1023", + true, NULL, NULL, + "system_u", "system_r", "svirt_t", "svirt_image_t", + 0, 0, 0, 1023); + DO_TEST_GEN_LABEL("dynamic virtd, s0, c0.c10", + "system_u:system_r:virtd_t:s0-s0:c0.c10", + true, NULL, NULL, + "system_u", "system_r", "svirt_t", "svirt_image_t", + 0, 0, 0, 10); + DO_TEST_GEN_LABEL("dynamic virtd, s2-s3, c0.c1023", + "system_u:system_r:virtd_t:s2-s3:c0.c1023", + true, NULL, NULL, + "system_u", "system_r", "svirt_t", "svirt_image_t", + 2, 3, 0, 1023);
No test of a degenerate single-range category?
I'll change one of the 's0-s0' case to just 's0'
index f372c23..16414d9 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -70,4 +70,13 @@ int virtTestMain(int argc, return virtTestMain(argc, argv, func); \ }
+# define VIRT_TEST_MAIN_PRELOAD(func, lib) \ + int main(int argc, char **argv) { \ + if (getenv("LD_PRELOAD") == NULL) { \ + setenv("LD_PRELOAD", lib, 1); \ + execv(argv[0], argv); \
This is insufficient - if LD_PRELOAD is already set for other reasons (such as valgrind), then you still want to modify it and re-exec. I think you need to strstr() the existing contents for the new lib before deciding whether to re-exec.
Ah yes, good point Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 08/10/2012 03:47 PM, Daniel P. Berrange wrote:
This patch series makes a number of changes to the SELinux label generation code. This is intended to make it fully honour the current process label when generating VM labels, so that dynamic label generation works better with custom policies, or confined user accounts.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Unfortunately I am not selinux-savvy enough to understand exactly why, but I cannot start guests any more after pulling master. The issue is that the virtual disk's security context (a block device in this case) cannot be set, message shown below. 012-08-16 15:02:18.891+0000: 1536: error : virSecuritySELinuxSetFileconHelper:652 : unable to set security context 'system_u:system_r:svirt_image_t:s0:c786,c986' on '/dev/disk/by-path/ccw-0.0.3770-part1': Invalid argument Prior to that the security context would have looked like this system_u:object_r:svirt_image_t:s0:c153,c923, i.e. using object_r instead of system_r. I am running on RHEL 6.2, not sure whether this is relevant. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 08/16/2012 11:41 AM, Viktor Mihajlovski wrote:
On 08/10/2012 03:47 PM, Daniel P. Berrange wrote:
This patch series makes a number of changes to the SELinux label generation code. This is intended to make it fully honour the current process label when generating VM labels, so that dynamic label generation works better with custom policies, or confined user accounts.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Unfortunately I am not selinux-savvy enough to understand exactly why, but I cannot start guests any more after pulling master.
The issue is that the virtual disk's security context (a block device in this case) cannot be set, message shown below.
012-08-16 15:02:18.891+0000: 1536: error : virSecuritySELinuxSetFileconHelper:652 : unable to set security context 'system_u:system_r:svirt_image_t:s0:c786,c986' on '/dev/disk/by-path/ccw-0.0.3770-part1': Invalid argument
Prior to that the security context would have looked like this system_u:object_r:svirt_image_t:s0:c153,c923, i.e. using object_r instead of system_r.
I am running on RHEL 6.2, not sure whether this is relevant.
Yes the security context should be system_u:object_r:svirt_image_t:s0:c786,c986 These patches should have just affected the Process label not the file label. On the file label we should alter the role on the file label to include object_r. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAlAtMVIACgkQrlYvE4MpobMYqQCgz+d7yeXhYXTz0IGFIsRYUqJl GGgAniHHX21m7D5BHZgeMHskS8zww4B1 =Ex2S -----END PGP SIGNATURE-----
On 08/16/2012 07:43 PM, Daniel J Walsh wrote:
Yes the security context should be system_u:object_r:svirt_image_t:s0:c786,c986 These patches should have just affected the Process label not the file label. On the file label we should alter the role on the file label to include object_r. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAlAtMVIACgkQrlYvE4MpobMYqQCgz+d7yeXhYXTz0IGFIsRYUqJl GGgAniHHX21m7D5BHZgeMHskS8zww4B1 =Ex2S -----END PGP SIGNATURE-----
I've submitted a patch to leave the role unaltered if the base context role is object_r. Maybe not the most elegant way, but it works for me. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (4)
-
Daniel J Walsh -
Daniel P. Berrange -
Eric Blake -
Viktor Mihajlovski