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

[Tim Gardner]

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 at 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
-- 
Tim Gardner tim.gardner at canonical.com