Patchwork [1/2] AppArmor: fix the upper bound check for the next/check table

login
register
mail settings
Submitter John Johansen
Date Dec. 3, 2010, 10:14 p.m.
Message ID <1291414464-6518-2-git-send-email-john.johansen@canonical.com>
Download mbox | patch
Permalink /patch/74215/
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 (apparmor)

   impact of the bug is medium for stable releases. There are two parts to
   this bug: the kernel side OOPSing when a the parser generates invalid
   tables, and the parser generating correct tables. The lucid kernel should
   receive the fix sometime in the future, but the userspace should also be
   fixed.

   The kernel bug was a broken test in verifying the dfa next/check table
   size (so the userspace bug was not caught when it should have been). This
   means that it can at times reference beyond the dfa table (by at most 255
   entries).

   The userspace bug is that the next/check table is not correctly padded
   with 0 entries, so that it is impossible to reference beyond the end of
   the table when in the states that use the end of the table for their
   references.

The next/check table needs to be >= largest base value + 255 (byte range
being 0..255) to avoid any possible bounds violations.  Fix the test which
incorrectly was testing that the next/check table + 256 <= base values.

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/match.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Tim Gardner - Dec. 6, 2010, 7:49 p.m.
On 12/03/2010 03:14 PM, John Johansen wrote:
> BugLink: http://bugs.launchpad.net/bugs/581525
>
> SRU Justification (apparmor)
>
>     impact of the bug is medium for stable releases. There are two parts to
>     this bug: the kernel side OOPSing when a the parser generates invalid
>     tables, and the parser generating correct tables. The lucid kernel should
>     receive the fix sometime in the future, but the userspace should also be
>     fixed.
>
>     The kernel bug was a broken test in verifying the dfa next/check table
>     size (so the userspace bug was not caught when it should have been). This
>     means that it can at times reference beyond the dfa table (by at most 255
>     entries).
>
>     The userspace bug is that the next/check table is not correctly padded
>     with 0 entries, so that it is impossible to reference beyond the end of
>     the table when in the states that use the end of the table for their
>     references.
>
> The next/check table needs to be>= largest base value + 255 (byte range
> being 0..255) to avoid any possible bounds violations.  Fix the test which
> incorrectly was testing that the next/check table + 256<= base values.
>
> Signed-off-by: John Johansen<john.johansen@canonical.com>
> ---
>   security/apparmor/match.c |    4 +++-
>   1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index a3730e2..b456495 100644
> --- a/security/apparmor/match.c
> +++ b/security/apparmor/match.c
> @@ -177,8 +177,10 @@ static int verify_dfa(struct aa_dfa *dfa, int flags)
>   			if (DEFAULT_TABLE(dfa)[i]>= state_count)
>   				goto out;
>   			/* TODO: do check that DEF state recursion terminates */
> -			if (BASE_TABLE(dfa)[i]>= trans_count + 256)
> +			if (BASE_TABLE(dfa)[i] + 255>= trans_count) {
> +				printk("AppArmor DFA next/check upper bounds error\n");
>   				goto out;
> +			}
>   		}
>
>   		for (i = 0; i<  trans_count; i++) {

Matches the upstream code.

Acked-by: Tim Gardner <tim.gardner@canonical.com>
Brad Figg - Dec. 8, 2010, 8:13 p.m.
On 12/06/2010 11:49 AM, Tim Gardner wrote:
> On 12/03/2010 03:14 PM, John Johansen wrote:
>> BugLink: http://bugs.launchpad.net/bugs/581525
>>
>> SRU Justification (apparmor)
>>
>>      impact of the bug is medium for stable releases. There are two parts to
>>      this bug: the kernel side OOPSing when a the parser generates invalid
>>      tables, and the parser generating correct tables. The lucid kernel should
>>      receive the fix sometime in the future, but the userspace should also be
>>      fixed.
>>
>>      The kernel bug was a broken test in verifying the dfa next/check table
>>      size (so the userspace bug was not caught when it should have been). This
>>      means that it can at times reference beyond the dfa table (by at most 255
>>      entries).
>>
>>      The userspace bug is that the next/check table is not correctly padded
>>      with 0 entries, so that it is impossible to reference beyond the end of
>>      the table when in the states that use the end of the table for their
>>      references.
>>
>> The next/check table needs to be>= largest base value + 255 (byte range
>> being 0..255) to avoid any possible bounds violations.  Fix the test which
>> incorrectly was testing that the next/check table + 256<= base values.
>>
>> Signed-off-by: John Johansen<john.johansen@canonical.com>
>> ---
>>    security/apparmor/match.c |    4 +++-
>>    1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>> index a3730e2..b456495 100644
>> --- a/security/apparmor/match.c
>> +++ b/security/apparmor/match.c
>> @@ -177,8 +177,10 @@ static int verify_dfa(struct aa_dfa *dfa, int flags)
>>    			if (DEFAULT_TABLE(dfa)[i]>= state_count)
>>    				goto out;
>>    			/* TODO: do check that DEF state recursion terminates */
>> -			if (BASE_TABLE(dfa)[i]>= trans_count + 256)
>> +			if (BASE_TABLE(dfa)[i] + 255>= trans_count) {
>> +				printk("AppArmor DFA next/check upper bounds error\n");
>>    				goto out;
>> +			}
>>    		}
>>
>>    		for (i = 0; i<   trans_count; i++) {
>
> Matches the upstream code.
>
> Acked-by: Tim Gardner<tim.gardner@canonical.com>
>

Acked-by: Brad Figg <brad.figg@canonical.com>

Applied to Lucid master-next.

Patch

diff --git a/security/apparmor/match.c b/security/apparmor/match.c
index a3730e2..b456495 100644
--- a/security/apparmor/match.c
+++ b/security/apparmor/match.c
@@ -177,8 +177,10 @@  static int verify_dfa(struct aa_dfa *dfa, int flags)
 			if (DEFAULT_TABLE(dfa)[i] >= state_count)
 				goto out;
 			/* TODO: do check that DEF state recursion terminates */
-			if (BASE_TABLE(dfa)[i] >= trans_count + 256)
+			if (BASE_TABLE(dfa)[i] + 255 >= trans_count) {
+				printk("AppArmor DFA next/check upper bounds error\n");
 				goto out;
+			}
 		}
 
 		for (i = 0; i < trans_count; i++) {