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

[John Johansen]

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 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?
> 
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.