[PATCH] xen/xsm: Generate the permission in a spec-compliant way

[PATCH] xen/xsm: Generate the permission in a spec-compliant way

[Julien Grall]

Each class can contains 32 permisions which are encoded on a word (one
bit per permission).

Currently the awk script will generate an hexadecimal value for each
permission. This may result to generate an invalid value on some version
of awk.

For instance debian jessie is using a version of mawk where (1 << 31)
will result to 0x7fffffff.

This is because the awk specification requires to do the arithmetic with
float. So the resulting integer may vary following the implementation.

As the generated headers are only used by C code, generate the
permission define via "1UL << n".

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
https://bugs.launchpad.net/ubuntu/+source/mawk/+bug/1159933

This bug impacts every version of Xen built on Debian Jessie when XSM is
enabled.

It would be interesting to backport it.
---
 xen/xsm/flask/policy/mkaccess_vector.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/xsm/flask/policy/mkaccess_vector.sh b/xen/xsm/flask/policy/mkaccess_vector.sh
index 8ec87f7..7fa4aaf 100644
--- a/xen/xsm/flask/policy/mkaccess_vector.sh
+++ b/xen/xsm/flask/policy/mkaccess_vector.sh
@@ -42,7 +42,7 @@ $1 == "class"	{
 			} 
 			av_defined[tclass] = 1;
 
-			permission = 1;
+			permission = 0;
 
 			nextstate = "INHERITS_OR_CLASS-OPENBRACKET";
 			next;
@@ -108,8 +108,8 @@ $1 == "{"	{
 
 			for (i = 0; i < spaces; i++) 
 				printf(" ") > outfile; 
-			printf("0x%08xUL\n", permission) > outfile; 
-			permission = permission * 2;
+			printf("(1UL << %u)\n", permission) > outfile;
+			permission = permission + 1;
 		}
 $1 == "}"	{
 			if (nextstate != "CLASS-CLOSEBRACKET" && 
-- 
2.1.4

Re: [PATCH] xen/xsm: Generate the permission in a spec-compliant way

[Daniel De Graaf]

On 02/20/2015 10:58 AM, Julien Grall wrote:
> Each class can contains 32 permisions which are encoded on a word (one
> bit per permission).
>
> Currently the awk script will generate an hexadecimal value for each
> permission. This may result to generate an invalid value on some version
> of awk.
>
> For instance debian jessie is using a version of mawk where (1 << 31)
> will result to 0x7fffffff.
>
> This is because the awk specification requires to do the arithmetic with
> float. So the resulting integer may vary following the implementation.
>
> As the generated headers are only used by C code, generate the
> permission define via "1UL << n".
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

The fix looks correct.  For backporting: this is only a problem since the
auto-generation was moved into the hypervisor build (between 4.2 and 4.3).
Prior to this, the headers were manually generated, and apparently nobody
ran the script on a system with this bug - in part because nobody ran

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Wow, that's quite an annoying bug.  Thankfully, it's more likely to make a
broken system than an insecure one, since doing an access check on the
permission 0x7fffffff will result in checking for access to all 31 other
permissions instead of the one you intended to check for.  For Xen, it
looks like this is unlikely to succeed, and also won't do something like
prevent the system from booting:

class xen: setscheduler would check kexec & others

class domain: set_virq_handler would check transition & destroy
  - in the example policy, transition is only allowed for *_building -> *
  - in any other policy, transition is unlikely to be allowed at the same
    time as destroy

There are no other uses of permission bit 32.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH] xen/xsm: Generate the permission in a spec-compliant way

[Ian Campbell]

On Fri, 2015-02-20 at 18:01 -0500, Daniel De Graaf wrote:
> On 02/20/2015 10:58 AM, Julien Grall wrote:
> > Each class can contains 32 permisions which are encoded on a word (one
> > bit per permission).
> >
> > Currently the awk script will generate an hexadecimal value for each
> > permission. This may result to generate an invalid value on some version
> > of awk.
> >
> > For instance debian jessie is using a version of mawk where (1 << 31)
> > will result to 0x7fffffff.
> >
> > This is because the awk specification requires to do the arithmetic with
> > float. So the resulting integer may vary following the implementation.
> >
> > As the generated headers are only used by C code, generate the
> > permission define via "1UL << n".
> >
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> The fix looks correct.  For backporting: this is only a problem since the
> auto-generation was moved into the hypervisor build (between 4.2 and 4.3).
> Prior to this, the headers were manually generated, and apparently nobody
> ran the script on a system with this bug - in part because nobody ran

... truncated sentence?

> 
> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> 
> Wow, that's quite an annoying bug.  Thankfully, it's more likely to make a
> broken system than an insecure one, since doing an access check on the
> permission 0x7fffffff will result in checking for access to all 31 other
> permissions instead of the one you intended to check for.  For Xen, it
> looks like this is unlikely to succeed, and also won't do something like
> prevent the system from booting:
> 
> class xen: setscheduler would check kexec & others
> 
> class domain: set_virq_handler would check transition & destroy
>   - in the example policy, transition is only allowed for *_building -> *
>   - in any other policy, transition is unlikely to be allowed at the same
>     time as destroy

Thanks for the analysis.

> 
> There are no other uses of permission bit 32.
> 

Re: [PATCH] xen/xsm: Generate the permission in a spec-compliant way

[Julien Grall]

Hi Daniel,

On 20/02/15 23:01, Daniel De Graaf wrote:
> On 02/20/2015 10:58 AM, Julien Grall wrote:
>> Each class can contains 32 permisions which are encoded on a word (one
>> bit per permission).
>>
>> Currently the awk script will generate an hexadecimal value for each
>> permission. This may result to generate an invalid value on some version
>> of awk.
>>
>> For instance debian jessie is using a version of mawk where (1 << 31)
>> will result to 0x7fffffff.
>>
>> This is because the awk specification requires to do the arithmetic with
>> float. So the resulting integer may vary following the implementation.
>>
>> As the generated headers are only used by C code, generate the
>> permission define via "1UL << n".
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> The fix looks correct.  For backporting: this is only a problem since the
> auto-generation was moved into the hypervisor build (between 4.2 and 4.3).
> Prior to this, the headers were manually generated, and apparently nobody
> ran the script on a system with this bug - in part because nobody ran
> 
> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> 
> Wow, that's quite an annoying bug.  Thankfully, it's more likely to make a
> broken system than an insecure one, since doing an access check on the
> permission 0x7fffffff will result in checking for access to all 31 other
> permissions instead of the one you intended to check for.  For Xen, it
> looks like this is unlikely to succeed, and also won't do something like
> prevent the system from booting:

Actually I think the policy is not even loaded.

>From the log I got
(XEN) Flask:  Initializing.
(XEN) AVC INITIALIZED
(XEN) Flask: 128 avtab hash slots, 278 rules.
(XEN) Flask: 128 avtab hash slots, 278 rules.
(XEN) Flask:  3 users, 3 roles, 39 types, 1 bools
(XEN) Flask:  12 classes, 278 rules
(XEN) Flask:  permission setscheduler in class xen has incorrect value
(XEN) Flask:  the definition of a class is incorrect
(XEN) Flask:  Starting in enforcing mode.

As the policy is not valid (see validate_classes in
security_load_policy), we bail out directly.

But I don't understand why we continue to boot and everything is
working. Flask is not even correctly initialized...

Did I miss something?

Regards,

-- 
Julien Grall

Re: [PATCH] xen/xsm: Generate the permission in a spec-compliant way

[Daniel De Graaf]

On 02/23/2015 10:04 AM, Julien Grall wrote:
> Hi Daniel,
>
> On 20/02/15 23:01, Daniel De Graaf wrote:
>> On 02/20/2015 10:58 AM, Julien Grall wrote:
>>> Each class can contains 32 permisions which are encoded on a word (one
>>> bit per permission).
>>>
>>> Currently the awk script will generate an hexadecimal value for each
>>> permission. This may result to generate an invalid value on some version
>>> of awk.
>>>
>>> For instance debian jessie is using a version of mawk where (1 << 31)
>>> will result to 0x7fffffff.
>>>
>>> This is because the awk specification requires to do the arithmetic with
>>> float. So the resulting integer may vary following the implementation.
>>>
>>> As the generated headers are only used by C code, generate the
>>> permission define via "1UL << n".
>>>
>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> The fix looks correct.  For backporting: this is only a problem since the
>> auto-generation was moved into the hypervisor build (between 4.2 and 4.3).
>> Prior to this, the headers were manually generated, and apparently nobody
>> ran the script on a system with this bug - in part because nobody ran
... because nobody ran it.

>>
>> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>
>> Wow, that's quite an annoying bug.  Thankfully, it's more likely to make a
>> broken system than an insecure one, since doing an access check on the
>> permission 0x7fffffff will result in checking for access to all 31 other
>> permissions instead of the one you intended to check for.  For Xen, it
>> looks like this is unlikely to succeed, and also won't do something like
>> prevent the system from booting:
>
> Actually I think the policy is not even loaded.
>
>>From the log I got
> (XEN) Flask:  Initializing.
> (XEN) AVC INITIALIZED
> (XEN) Flask: 128 avtab hash slots, 278 rules.
> (XEN) Flask: 128 avtab hash slots, 278 rules.
> (XEN) Flask:  3 users, 3 roles, 39 types, 1 bools
> (XEN) Flask:  12 classes, 278 rules
> (XEN) Flask:  permission setscheduler in class xen has incorrect value
> (XEN) Flask:  the definition of a class is incorrect
> (XEN) Flask:  Starting in enforcing mode.
>
> As the policy is not valid (see validate_classes in
> security_load_policy), we bail out directly.
>
> But I don't understand why we continue to boot and everything is
> working. Flask is not even correctly initialized...
>
> Did I miss something?

It seems that failure returns from the XSM initcall are never checked or
acted upon; xsm_core.c just runs "(*call) ();" and ignores the return.
This should probably be fixed; I'm going to send a patch to address this
by changing the return value to void and calling panic if an invalid
policy is specified at boot.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH] xen/xsm: Generate the permission in a spec-compliant way

[Ian Campbell]

On Fri, 2015-02-20 at 18:01 -0500, Daniel De Graaf wrote:

> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Applied, thanks.