Patchwork [2/2] AppArmor: Allow dfa backward compatibility with broken userspace

login
register
mail settings
Submitter John Johansen
Date Dec. 3, 2010, 10:14 p.m.
Message ID <1291414464-6518-3-git-send-email-john.johansen@canonical.com>
Download mbox | patch
Permalink /patch/74217/
State Accepted
Delegated to: Brad Figg
Headers show

Comments

John Johansen - Dec. 3, 2010, 10:14 p.m.
BugLink: http://bugs.launchpad.net/bugs/581525

SRU Justification:
   See bug link for full justification

NOTE:
   This patch is OPTIONAL, and is only needed if the user space tools have
   not been updated.  The userspace tools update has been pulled in, so
   unless there is the need to support users updating the kernel without
   updating the userspace tools, I am NOT recommending this patch for
   SRU, but including it for completeness.

   If the the patch to fix the bounds check on the next/check table has
   been applied, and the userspace has not been updated.  The it is likely
   that part or all of policy will fail to load due to the bounds check
   rejecting policy.

   The Justification for this patch is that it supports using a non-updated
   (broken) userspace with a kernel that has the kernel fix for bug#581525
   applied.

The apparmor_parser when compiling policy could generate invalid dfas
that did not have sufficient padding to avoid invalid references, when
used by the kernel.  The kernels check to verify the next/check table
size was broken meaning invalid dfas were being created by userspace
and not caught.

To remain compatible with old tools that are not fixed, pad the loaded
dfas next/check table.  The dfa's themselves are valid except for the
high padding for potentially invalid transitions (high bounds error),
which have a maximimum is 256 entries.  So just allocate an extra null filled
256 entries for the next/check tables.  This will guarentee all bounds
are good and invalid transitions go to the null (0) state.

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/include/apparmor.h |    6 ++++++
 security/apparmor/match.c            |    9 ++++++++-
 2 files changed, 14 insertions(+), 1 deletions(-)
Tim Gardner - Dec. 6, 2010, 8:16 p.m.
On 12/03/2010 03:14 PM, John Johansen wrote:
> BugLink: http://bugs.launchpad.net/bugs/581525
>
> SRU Justification:
>     See bug link for full justification
>
> NOTE:
>     This patch is OPTIONAL, and is only needed if the user space tools have
>     not been updated.  The userspace tools update has been pulled in, so
>     unless there is the need to support users updating the kernel without
>     updating the userspace tools, I am NOT recommending this patch for
>     SRU, but including it for completeness.
>
>     If the the patch to fix the bounds check on the next/check table has
>     been applied, and the userspace has not been updated.  The it is likely
>     that part or all of policy will fail to load due to the bounds check
>     rejecting policy.
>
>     The Justification for this patch is that it supports using a non-updated
>     (broken) userspace with a kernel that has the kernel fix for bug#581525
>     applied.
>
> The apparmor_parser when compiling policy could generate invalid dfas
> that did not have sufficient padding to avoid invalid references, when
> used by the kernel.  The kernels check to verify the next/check table
> size was broken meaning invalid dfas were being created by userspace
> and not caught.
>
> To remain compatible with old tools that are not fixed, pad the loaded
> dfas next/check table.  The dfa's themselves are valid except for the
> high padding for potentially invalid transitions (high bounds error),
> which have a maximimum is 256 entries.  So just allocate an extra null filled
> 256 entries for the next/check tables.  This will guarentee all bounds
> are good and invalid transitions go to the null (0) state.
>
> Signed-off-by: John Johansen<john.johansen@canonical.com>
> ---
>   security/apparmor/include/apparmor.h |    6 ++++++
>   security/apparmor/match.c            |    9 ++++++++-
>   2 files changed, 14 insertions(+), 1 deletions(-)
>
> diff --git a/security/apparmor/include/apparmor.h b/security/apparmor/include/apparmor.h
> index 3f8d866..b4ddc95 100644
> --- a/security/apparmor/include/apparmor.h
> +++ b/security/apparmor/include/apparmor.h
> @@ -46,6 +46,12 @@ extern unsigned int aa_g_path_max;
>   			printk(KERN_ERR "AppArmor: " fmt, ##args);	\
>   	} while (0)
>
> +#define AA_WARN(fmt, args...)						\
> +	do {								\
> +		if (printk_ratelimit())					\
> +			printk(KERN_WARN "AppArmor: " fmt, ##args);	\
> +	} while (0)
> +
>   /* Flag indicating whether initialization completed */
>   extern int apparmor_initialized;
>   void apparmor_disable(void);
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index b456495..f4486c1 100644
> --- a/security/apparmor/match.c
> +++ b/security/apparmor/match.c
> @@ -91,6 +91,13 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>   	if (bsize<  tsize)
>   		goto out;
>
> +	/* Pad table allocation for next/check by 256 entries to remain
> +	 * backwards compatible with old (buggy) tools and remain safe without
> +	 * run time checks
> +	 */
> +	if (th.td_id == YYTD_ID_NXT || th.td_id == YYTD_ID_CHK)
> +		tsize += 256 * th.td_flags;
> +
>   	/* freed by free_table */
>   	table = kmalloc(tsize, GFP_KERNEL | __GFP_NOWARN);
>   	if (!table) {
> @@ -178,7 +185,7 @@ static int verify_dfa(struct aa_dfa *dfa, int flags)
>   				goto out;
>   			/* TODO: do check that DEF state recursion terminates */
>   			if (BASE_TABLE(dfa)[i] + 255>= trans_count) {
> -				printk("AppArmor DFA next/check upper bounds error\n");
> +				AA_WARN("DFA next/check upper bounds fixed, upgrade user space tools\n");
>   				goto out;
>   			}
>   		}

I'm having a hard time parsing the commit log message. Are you trying to 
say that the Lucid kernel won't work if the current Maverick apparmor 
user space is backported to Lucid? Confused...

rtg
John Johansen - Dec. 6, 2010, 8:49 p.m.
On 12/06/2010 12:16 PM, Tim Gardner wrote:
> On 12/03/2010 03:14 PM, John Johansen wrote:
>> BugLink: http://bugs.launchpad.net/bugs/581525
>>
>> SRU Justification:
>>     See bug link for full justification
>>
>> NOTE:
>>     This patch is OPTIONAL, and is only needed if the user space tools have
>>     not been updated.  The userspace tools update has been pulled in, so
>>     unless there is the need to support users updating the kernel without
>>     updating the userspace tools, I am NOT recommending this patch for
>>     SRU, but including it for completeness.
>>
>>     If the the patch to fix the bounds check on the next/check table has
>>     been applied, and the userspace has not been updated.  The it is likely
>>     that part or all of policy will fail to load due to the bounds check
>>     rejecting policy.
>>
>>     The Justification for this patch is that it supports using a non-updated
>>     (broken) userspace with a kernel that has the kernel fix for bug#581525
>>     applied.
>>
>> The apparmor_parser when compiling policy could generate invalid dfas
>> that did not have sufficient padding to avoid invalid references, when
>> used by the kernel.  The kernels check to verify the next/check table
>> size was broken meaning invalid dfas were being created by userspace
>> and not caught.
>>
>> To remain compatible with old tools that are not fixed, pad the loaded
>> dfas next/check table.  The dfa's themselves are valid except for the
>> high padding for potentially invalid transitions (high bounds error),
>> which have a maximimum is 256 entries.  So just allocate an extra null filled
>> 256 entries for the next/check tables.  This will guarentee all bounds
>> are good and invalid transitions go to the null (0) state.
>>
>> Signed-off-by: John Johansen<john.johansen@canonical.com>
>> ---
>>   security/apparmor/include/apparmor.h |    6 ++++++
>>   security/apparmor/match.c            |    9 ++++++++-
>>   2 files changed, 14 insertions(+), 1 deletions(-)
>>
>> diff --git a/security/apparmor/include/apparmor.h b/security/apparmor/include/apparmor.h
>> index 3f8d866..b4ddc95 100644
>> --- a/security/apparmor/include/apparmor.h
>> +++ b/security/apparmor/include/apparmor.h
>> @@ -46,6 +46,12 @@ extern unsigned int aa_g_path_max;
>>               printk(KERN_ERR "AppArmor: " fmt, ##args);    \
>>       } while (0)
>>
>> +#define AA_WARN(fmt, args...)                        \
>> +    do {                                \
>> +        if (printk_ratelimit())                    \
>> +            printk(KERN_WARN "AppArmor: " fmt, ##args);    \
>> +    } while (0)
>> +
>>   /* Flag indicating whether initialization completed */
>>   extern int apparmor_initialized;
>>   void apparmor_disable(void);
>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>> index b456495..f4486c1 100644
>> --- a/security/apparmor/match.c
>> +++ b/security/apparmor/match.c
>> @@ -91,6 +91,13 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>>       if (bsize<  tsize)
>>           goto out;
>>
>> +    /* Pad table allocation for next/check by 256 entries to remain
>> +     * backwards compatible with old (buggy) tools and remain safe without
>> +     * run time checks
>> +     */
>> +    if (th.td_id == YYTD_ID_NXT || th.td_id == YYTD_ID_CHK)
>> +        tsize += 256 * th.td_flags;
>> +
>>       /* freed by free_table */
>>       table = kmalloc(tsize, GFP_KERNEL | __GFP_NOWARN);
>>       if (!table) {
>> @@ -178,7 +185,7 @@ static int verify_dfa(struct aa_dfa *dfa, int flags)
>>                   goto out;
>>               /* TODO: do check that DEF state recursion terminates */
>>               if (BASE_TABLE(dfa)[i] + 255>= trans_count) {
>> -                printk("AppArmor DFA next/check upper bounds error\n");
>> +                AA_WARN("DFA next/check upper bounds fixed, upgrade user space tools\n");
>>                   goto out;
>>               }
>>           }
> 
> I'm having a hard time parsing the commit log message. Are you trying to say that the Lucid kernel won't work if the current Maverick apparmor user space is backported to Lucid? Confused...
> 
no.  I am saying there is a bug in the Lucid user space that will cause policy loading to some times break if the previous patch 1 is applied
and either the Lucid userspace is not updated, or this patch #2 is not applied

The fix for Lucid userspace is in -proposed, and as long as it goes to updates before a kernel with patch 1 hits -updates this patch is NOT needed unless, we care about the odd situation where a user installs an updated kernel but doesn't update their userspace.

If the maverick userspace is used with the Lucid kernel with or without these patches everything is fine.
Tim Gardner - Dec. 6, 2010, 8:53 p.m.
On 12/06/2010 01:49 PM, John Johansen wrote:
> On 12/06/2010 12:16 PM, Tim Gardner wrote:
>> On 12/03/2010 03:14 PM, John Johansen wrote:
>>> BugLink: http://bugs.launchpad.net/bugs/581525
>>>
>>> SRU Justification: See bug link for full justification
>>>
>>> NOTE: This patch is OPTIONAL, and is only needed if the user
>>> space tools have not been updated.  The userspace tools update
>>> has been pulled in, so unless there is the need to support users
>>> updating the kernel without updating the userspace tools, I am
>>> NOT recommending this patch for SRU, but including it for
>>> completeness.
>>>
>>> If the the patch to fix the bounds check on the next/check table
>>> has been applied, and the userspace has not been updated.  The it
>>> is likely that part or all of policy will fail to load due to the
>>> bounds check rejecting policy.
>>>
>>> The Justification for this patch is that it supports using a
>>> non-updated (broken) userspace with a kernel that has the kernel
>>> fix for bug#581525 applied.
>>>
>>> The apparmor_parser when compiling policy could generate invalid
>>> dfas that did not have sufficient padding to avoid invalid
>>> references, when used by the kernel.  The kernels check to verify
>>> the next/check table size was broken meaning invalid dfas were
>>> being created by userspace and not caught.
>>>
>>> To remain compatible with old tools that are not fixed, pad the
>>> loaded dfas next/check table.  The dfa's themselves are valid
>>> except for the high padding for potentially invalid transitions
>>> (high bounds error), which have a maximimum is 256 entries.  So
>>> just allocate an extra null filled 256 entries for the next/check
>>> tables.  This will guarentee all bounds are good and invalid
>>> transitions go to the null (0) state.
>>>
>>> Signed-off-by: John Johansen<john.johansen@canonical.com> ---
>>> security/apparmor/include/apparmor.h |    6 ++++++
>>> security/apparmor/match.c            |    9 ++++++++- 2 files
>>> changed, 14 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/security/apparmor/include/apparmor.h
>>> b/security/apparmor/include/apparmor.h index 3f8d866..b4ddc95
>>> 100644 --- a/security/apparmor/include/apparmor.h +++
>>> b/security/apparmor/include/apparmor.h @@ -46,6 +46,12 @@ extern
>>> unsigned int aa_g_path_max; printk(KERN_ERR "AppArmor: " fmt,
>>> ##args);    \ } while (0)
>>>
>>> +#define AA_WARN(fmt, args...)                        \ +    do {
>>> \ +        if (printk_ratelimit())                    \ +
>>> printk(KERN_WARN "AppArmor: " fmt, ##args);    \ +    } while
>>> (0) + /* Flag indicating whether initialization completed */
>>> extern int apparmor_initialized; void apparmor_disable(void);
>>> diff --git a/security/apparmor/match.c
>>> b/security/apparmor/match.c index b456495..f4486c1 100644 ---
>>> a/security/apparmor/match.c +++ b/security/apparmor/match.c @@
>>> -91,6 +91,13 @@ static struct table_header *unpack_table(char
>>> *blob, size_t bsize) if (bsize<   tsize) goto out;
>>>
>>> +    /* Pad table allocation for next/check by 256 entries to
>>> remain +     * backwards compatible with old (buggy) tools and
>>> remain safe without +     * run time checks +     */ +    if
>>> (th.td_id == YYTD_ID_NXT || th.td_id == YYTD_ID_CHK) +
>>> tsize += 256 * th.td_flags; + /* freed by free_table */ table =
>>> kmalloc(tsize, GFP_KERNEL | __GFP_NOWARN); if (!table) { @@
>>> -178,7 +185,7 @@ static int verify_dfa(struct aa_dfa *dfa, int
>>> flags) goto out; /* TODO: do check that DEF state recursion
>>> terminates */ if (BASE_TABLE(dfa)[i] + 255>= trans_count) { -
>>> printk("AppArmor DFA next/check upper bounds error\n"); +
>>> AA_WARN("DFA next/check upper bounds fixed, upgrade user space
>>> tools\n"); goto out; } }
>>
>> I'm having a hard time parsing the commit log message. Are you
>> trying to say that the Lucid kernel won't work if the current
>> Maverick apparmor user space is backported to Lucid? Confused...
>>
> no.  I am saying there is a bug in the Lucid user space that will
> cause policy loading to some times break if the previous patch 1 is
> applied and either the Lucid userspace is not updated, or this patch
> #2 is not applied
>
> The fix for Lucid userspace is in -proposed, and as long as it goes
> to updates before a kernel with patch 1 hits -updates this patch is
> NOT needed unless, we care about the odd situation where a user
> installs an updated kernel but doesn't update their userspace.
>
> If the maverick userspace is used with the Lucid kernel with or
> without these patches everything is fine.
>

Ah, so the aberrant case would be if a user is only subscribed to 
-security and then installs an LTS backport kernel?

rtg
John Johansen - Dec. 6, 2010, 10:42 p.m.
On 12/06/2010 12:53 PM, Tim Gardner wrote:
> On 12/06/2010 01:49 PM, John Johansen wrote:
>> On 12/06/2010 12:16 PM, Tim Gardner wrote:
>>> On 12/03/2010 03:14 PM, John Johansen wrote:
>>>> BugLink: http://bugs.launchpad.net/bugs/581525
>>>>
>>>> SRU Justification: See bug link for full justification
>>>>
>>>> NOTE: This patch is OPTIONAL, and is only needed if the user
>>>> space tools have not been updated.  The userspace tools update
>>>> has been pulled in, so unless there is the need to support users
>>>> updating the kernel without updating the userspace tools, I am
>>>> NOT recommending this patch for SRU, but including it for
>>>> completeness.
>>>>
>>>> If the the patch to fix the bounds check on the next/check table
>>>> has been applied, and the userspace has not been updated.  The it
>>>> is likely that part or all of policy will fail to load due to the
>>>> bounds check rejecting policy.
>>>>
>>>> The Justification for this patch is that it supports using a
>>>> non-updated (broken) userspace with a kernel that has the kernel
>>>> fix for bug#581525 applied.
>>>>
>>>> The apparmor_parser when compiling policy could generate invalid
>>>> dfas that did not have sufficient padding to avoid invalid
>>>> references, when used by the kernel.  The kernels check to verify
>>>> the next/check table size was broken meaning invalid dfas were
>>>> being created by userspace and not caught.
>>>>
>>>> To remain compatible with old tools that are not fixed, pad the
>>>> loaded dfas next/check table.  The dfa's themselves are valid
>>>> except for the high padding for potentially invalid transitions
>>>> (high bounds error), which have a maximimum is 256 entries.  So
>>>> just allocate an extra null filled 256 entries for the next/check
>>>> tables.  This will guarentee all bounds are good and invalid
>>>> transitions go to the null (0) state.
>>>>
>>>> Signed-off-by: John Johansen<john.johansen@canonical.com> ---
>>>> security/apparmor/include/apparmor.h |    6 ++++++
>>>> security/apparmor/match.c            |    9 ++++++++- 2 files
>>>> changed, 14 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/security/apparmor/include/apparmor.h
>>>> b/security/apparmor/include/apparmor.h index 3f8d866..b4ddc95
>>>> 100644 --- a/security/apparmor/include/apparmor.h +++
>>>> b/security/apparmor/include/apparmor.h @@ -46,6 +46,12 @@ extern
>>>> unsigned int aa_g_path_max; printk(KERN_ERR "AppArmor: " fmt,
>>>> ##args);    \ } while (0)
>>>>
>>>> +#define AA_WARN(fmt, args...)                        \ +    do {
>>>> \ +        if (printk_ratelimit())                    \ +
>>>> printk(KERN_WARN "AppArmor: " fmt, ##args);    \ +    } while
>>>> (0) + /* Flag indicating whether initialization completed */
>>>> extern int apparmor_initialized; void apparmor_disable(void);
>>>> diff --git a/security/apparmor/match.c
>>>> b/security/apparmor/match.c index b456495..f4486c1 100644 ---
>>>> a/security/apparmor/match.c +++ b/security/apparmor/match.c @@
>>>> -91,6 +91,13 @@ static struct table_header *unpack_table(char
>>>> *blob, size_t bsize) if (bsize<   tsize) goto out;
>>>>
>>>> +    /* Pad table allocation for next/check by 256 entries to
>>>> remain +     * backwards compatible with old (buggy) tools and
>>>> remain safe without +     * run time checks +     */ +    if
>>>> (th.td_id == YYTD_ID_NXT || th.td_id == YYTD_ID_CHK) +
>>>> tsize += 256 * th.td_flags; + /* freed by free_table */ table =
>>>> kmalloc(tsize, GFP_KERNEL | __GFP_NOWARN); if (!table) { @@
>>>> -178,7 +185,7 @@ static int verify_dfa(struct aa_dfa *dfa, int
>>>> flags) goto out; /* TODO: do check that DEF state recursion
>>>> terminates */ if (BASE_TABLE(dfa)[i] + 255>= trans_count) { -
>>>> printk("AppArmor DFA next/check upper bounds error\n"); +
>>>> AA_WARN("DFA next/check upper bounds fixed, upgrade user space
>>>> tools\n"); goto out; } }
>>>
>>> I'm having a hard time parsing the commit log message. Are you
>>> trying to say that the Lucid kernel won't work if the current
>>> Maverick apparmor user space is backported to Lucid? Confused...
>>>
>> no.  I am saying there is a bug in the Lucid user space that will
>> cause policy loading to some times break if the previous patch 1 is
>> applied and either the Lucid userspace is not updated, or this patch
>> #2 is not applied
>>
>> The fix for Lucid userspace is in -proposed, and as long as it goes
>> to updates before a kernel with patch 1 hits -updates this patch is
>> NOT needed unless, we care about the odd situation where a user
>> installs an updated kernel but doesn't update their userspace.
>>
>> If the maverick userspace is used with the Lucid kernel with or
>> without these patches everything is fine.
>>
> 
> Ah, so the aberrant case would be if a user is only subscribed to -security and then installs an LTS backport kernel?
> 
well no.  Not as long as jdstrand rolls the userspace update through -security as we discussed.  If that happens the only case is someone manually installing a kernel with the bound fix applied (via dpkg, or make) and they haven't updated their userspace via either -updates,
-security, or manually.

Its a case I don't believe we care about, and I would rather not have to carry a version of this patch in Natty and Natty+1, but if
supporting this case is something we care about for the Lucid kernel then we will need it for Natty and and Natty+1 as well.

Patch

diff --git a/security/apparmor/include/apparmor.h b/security/apparmor/include/apparmor.h
index 3f8d866..b4ddc95 100644
--- a/security/apparmor/include/apparmor.h
+++ b/security/apparmor/include/apparmor.h
@@ -46,6 +46,12 @@  extern unsigned int aa_g_path_max;
 			printk(KERN_ERR "AppArmor: " fmt, ##args);	\
 	} while (0)
 
+#define AA_WARN(fmt, args...)						\
+	do {								\
+		if (printk_ratelimit())					\
+			printk(KERN_WARN "AppArmor: " fmt, ##args);	\
+	} while (0)
+
 /* Flag indicating whether initialization completed */
 extern int apparmor_initialized;
 void apparmor_disable(void);
diff --git a/security/apparmor/match.c b/security/apparmor/match.c
index b456495..f4486c1 100644
--- a/security/apparmor/match.c
+++ b/security/apparmor/match.c
@@ -91,6 +91,13 @@  static struct table_header *unpack_table(char *blob, size_t bsize)
 	if (bsize < tsize)
 		goto out;
 
+	/* Pad table allocation for next/check by 256 entries to remain
+	 * backwards compatible with old (buggy) tools and remain safe without
+	 * run time checks
+	 */
+	if (th.td_id == YYTD_ID_NXT || th.td_id == YYTD_ID_CHK)
+		tsize += 256 * th.td_flags;
+
 	/* freed by free_table */
 	table = kmalloc(tsize, GFP_KERNEL | __GFP_NOWARN);
 	if (!table) {
@@ -178,7 +185,7 @@  static int verify_dfa(struct aa_dfa *dfa, int flags)
 				goto out;
 			/* TODO: do check that DEF state recursion terminates */
 			if (BASE_TABLE(dfa)[i] + 255 >= trans_count) {
-				printk("AppArmor DFA next/check upper bounds error\n");
+				AA_WARN("DFA next/check upper bounds fixed, upgrade user space tools\n");
 				goto out;
 			}
 		}