8:47 a.m.
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.
8:47 a.m.
New subject: [libvirt] [PATCH 1/8] Add virRandom() API to generate numbers with non-power-of-2 limit
From: "Daniel P. Berrange" <berrange(a)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(a)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
9:58 a.m.
New subject: [libvirt] [PATCH 1/8] Add virRandom() API to generate numbers with non-power-of-2 limit
On 08/10/2012 07:47 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:31 a.m.
New subject: [libvirt] [PATCH 1/8] Add virRandom() API to generate numbers with non-power-of-2 limit
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(a)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 :|
11:06 a.m.
New subject: [libvirt] [PATCH 1/8] Add virRandom() API to generate numbers with non-power-of-2 limit
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(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:41 p.m.
New subject: [libvirt] [PATCH 1/8] Add virRandom() API to generate numbers with non-power-of-2 limit
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
6:20 p.m.
New subject: [libvirt] [PATCH 1/8] Add virRandom() API to generate numbers with non-power-of-2 limit
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(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:47 a.m.
New subject: [libvirt] [PATCH 2/8] Add missing domain_conf.h include in security_manager.h
From: "Daniel P. Berrange" <berrange(a)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(a)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
12:43 p.m.
New subject: [libvirt] [PATCH 2/8] Add missing domain_conf.h include in security_manager.h
On 08/10/2012 07:47 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com>
---
src/security/security_manager.h | 2 ++
1 file changed, 2 insertions(+)
ACK; trivial.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:47 a.m.
New subject: [libvirt] [PATCH 3/8] Use VIR_ERR_CONFIG_UNSUPPORTED if requested security driver is disabled
From: "Daniel P. Berrange" <berrange(a)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(a)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
12:44 p.m.
New subject: [libvirt] [PATCH 3/8] Use VIR_ERR_CONFIG_UNSUPPORTED if requested security driver is disabled
On 08/10/2012 07:47 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:47 a.m.
New subject: [libvirt] [PATCH 4/8] Fix error reporting in virSecuritySELinuxGenNewContext
From: "Daniel P. Berrange" <berrange(a)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(a)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
12:48 p.m.
New subject: [libvirt] [PATCH 4/8] Fix error reporting in virSecuritySELinuxGenNewContext
On 08/10/2012 07:47 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com>
---
src/security/security_selinux.c | 67 +++++++++++++++++++++++------------------
1 file changed, 37 insertions(+), 30 deletions(-)
ACK.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:48 a.m.
New subject: [libvirt] [PATCH 5/8] Honour current user and role in SELinux label generation
From: "Daniel P. Berrange" <berrange(a)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(a)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
3:55 p.m.
New subject: [libvirt] [PATCH 5/8] Honour current user and role in SELinux label generation
On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:22 a.m.
New subject: [libvirt] [PATCH 5/8] Honour current user and role in SELinux label generation
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(a)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 :|
8:48 a.m.
New subject: [libvirt] [PATCH 6/8] Pull code which finds a free MCS label out into its own method
From: "Daniel P. Berrange" <berrange(a)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(a)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
4:02 p.m.
New subject: [libvirt] [PATCH 6/8] Pull code which finds a free MCS label out into its own method
On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:22 a.m.
New subject: [libvirt] [PATCH 6/8] Pull code which finds a free MCS label out into its own method
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(a)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(a)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 :|
8:48 a.m.
New subject: [libvirt] [PATCH 7/8] Honour current sensitivity and category ranges in SELinux label generation
From: "Daniel P. Berrange" <berrange(a)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(a)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
4:20 p.m.
New subject: [libvirt] [PATCH 7/8] Honour current sensitivity and category ranges in SELinux label generation
On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:27 a.m.
New subject: [libvirt] [PATCH 7/8] Honour current sensitivity and category ranges in SELinux label generation
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 :|
8:48 a.m.
New subject: [libvirt] [PATCH 8/8] Add test case for SELinux label generation
From: "Daniel P. Berrange" <berrange(a)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(a)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
4:50 p.m.
New subject: [libvirt] [PATCH 8/8] Add test case for SELinux label generation
On 08/10/2012 07:48 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:30 a.m.
New subject: [libvirt] [PATCH 8/8] Add test case for SELinux label generation
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 :|
10:41 a.m.
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(a)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
12:43 p.m.
-----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(a)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-----
7:56 a.m.
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.
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
-----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-----
4480
days inactive
4487
days old
27 comments
4 participants
participants (4)
-
Daniel J Walsh -
Daniel P. Berrange -
Eric Blake -
Viktor Mihajlovski