| Message ID | 1284739355-17542-3-git-send-email-john.johansen@canonical.com |
|---|---|
| State | Accepted |
| Delegated to: | Leann Ogasawara |
| Headers | show |
John Johansen wrote: > for (i = 0; i < size; i++) { > + /* discard extraneous rules that this kernel will > + * never request > + */ > + if (size > AF_MAX) { Do you want to discard all rules rather than extraneous rules? I think this should be (i >= AF_MAX) rather than (size > AF_MAX). > + u16 tmp; > + if (!unpack_u16(e, &tmp, NULL) || > + !unpack_u16(e, &tmp, NULL) || > + !unpack_u16(e, &tmp, NULL)) > + goto fail; > + continue; > + } > if (!unpack_u16(e, &profile->net.allow[i], NULL)) > goto fail; > if (!unpack_u16(e, &profile->net.audit[i], NULL))
On 09/17/2010 04:54 PM, Tetsuo Handa wrote: > John Johansen wrote: >> for (i = 0; i< size; i++) { >> + /* discard extraneous rules that this kernel will >> + * never request >> + */ >> + if (size> AF_MAX) { > > Do you want to discard all rules rather than extraneous rules? > I think this should be (i>= AF_MAX) rather than (size> AF_MAX). > >> + u16 tmp; >> + if (!unpack_u16(e,&tmp, NULL) || >> + !unpack_u16(e,&tmp, NULL) || >> + !unpack_u16(e,&tmp, NULL)) >> + goto fail; >> + continue; >> + } >> if (!unpack_u16(e,&profile->net.allow[i], NULL)) >> goto fail; >> if (!unpack_u16(e,&profile->net.audit[i], NULL)) sigh, yes. I can't believe I did that :( thanks Tetsuo
On 09/21/2010 04:32 PM, John Johansen wrote: > On 09/17/2010 04:54 PM, Tetsuo Handa wrote: >> John Johansen wrote: >>> for (i = 0; i< size; i++) { >>> + /* discard extraneous rules that this kernel will >>> + * never request >>> + */ >>> + if (size> AF_MAX) { >> >> Do you want to discard all rules rather than extraneous rules? >> I think this should be (i>= AF_MAX) rather than (size> AF_MAX). >> >>> + u16 tmp; >>> + if (!unpack_u16(e,&tmp, NULL) || >>> + !unpack_u16(e,&tmp, NULL) || >>> + !unpack_u16(e,&tmp, NULL)) >>> + goto fail; >>> + continue; >>> + } >>> if (!unpack_u16(e,&profile->net.allow[i], NULL)) >>> goto fail; >>> if (!unpack_u16(e,&profile->net.audit[i], NULL)) > > sigh, yes. I can't believe I did that :( > > thanks Tetsuo > So, whats the impact? Does this mean that we're dropping all AA rules? rtg
Tim Gardner wrote:
> So, whats the impact? Does this mean that we're dropping all AA rules?
At first, I thought the impact of this error is
When a profile with address family which currently running kernel does not
know is loaded, loading the profile will succeed but all networking
permissions are discarded. Therefore, currently running kernel will reject
all socket operations (e.g. socket(), bind(), sendmsg()) for all families
(except AF_UNIX and AF_NETLINK) with -EACCES unless the process is
unconfined. This means that networking applications (e.g. firefox, cupsd,
dhclient) which will be confined by profiles won't work properly.
But after reading security/apparmor/net.c , it changed to:
No impact at all because Maverick kernel does not provide networking
mediation functionality.
What? Excuse me, John. I assumed that networking mediation functionality is
included into Maverick kernel. But according to
http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-maverick.git;a=blob;f=security/apparmor/net.c;hb=HEAD
(as of "ALSA: seq/oss - Fix double-free at error path of snd_seq_oss_open()"),
I can't find a line that stores error code to sa.aad.error within audit_net().
This means that sa.aad.error is always 0 and therefore aa_net_perm() will
always return 0 (rather than -EACCESS) no matter how "net_allowed_af" is
specified.
I hope I'm missing something...
On 09/21/2010 04:31 AM, Tim Gardner wrote: > On 09/21/2010 04:32 PM, John Johansen wrote: >> On 09/17/2010 04:54 PM, Tetsuo Handa wrote: >>> John Johansen wrote: >>>> for (i = 0; i< size; i++) { >>>> + /* discard extraneous rules that this kernel will >>>> + * never request >>>> + */ >>>> + if (size> AF_MAX) { >>> >>> Do you want to discard all rules rather than extraneous rules? >>> I think this should be (i>= AF_MAX) rather than (size> AF_MAX). >>> >>>> + u16 tmp; >>>> + if (!unpack_u16(e,&tmp, NULL) || >>>> + !unpack_u16(e,&tmp, NULL) || >>>> + !unpack_u16(e,&tmp, NULL)) >>>> + goto fail; >>>> + continue; >>>> + } >>>> if (!unpack_u16(e,&profile->net.allow[i], NULL)) >>>> goto fail; >>>> if (!unpack_u16(e,&profile->net.audit[i], NULL)) >> >> sigh, yes. I can't believe I did that :( >> >> thanks Tetsuo >> > > So, whats the impact? Does this mean that we're dropping all AA rules? > No. It means we will drop network rules if the tools are built against a newer kernel tree that has added a new address family. Against the current tools everything works. To load policy the user has to be an unconfined root, at which point they can load modules and do other nasties so there isn't a potential escalation out of this. It should just potentially affect machines on upgrade. So we need to SRU a patch for this but it is not release critical, but I have the patch and after I take a second look at it to make sure it is right this time. I will kick it out this morning.
On 09/21/2010 07:13 AM, Tetsuo Handa wrote: > Tim Gardner wrote: >> So, whats the impact? Does this mean that we're dropping all AA rules? > > At first, I thought the impact of this error is > > When a profile with address family which currently running kernel does not > know is loaded, loading the profile will succeed but all networking > permissions are discarded. Therefore, currently running kernel will reject > all socket operations (e.g. socket(), bind(), sendmsg()) for all families > (except AF_UNIX and AF_NETLINK) with -EACCES unless the process is > unconfined. This means that networking applications (e.g. firefox, cupsd, > dhclient) which will be confined by profiles won't work properly. > > But after reading security/apparmor/net.c , it changed to: > > No impact at all because Maverick kernel does not provide networking > mediation functionality. > > What? Excuse me, John. I assumed that networking mediation functionality is > included into Maverick kernel. But according to > http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-maverick.git;a=blob;f=security/apparmor/net.c;hb=HEAD > (as of "ALSA: seq/oss - Fix double-free at error path of snd_seq_oss_open()"), > I can't find a line that stores error code to sa.aad.error within audit_net(). > This means that sa.aad.error is always 0 and therefore aa_net_perm() will > always return 0 (rather than -EACCESS) no matter how "net_allowed_af" is > specified. > > I hope I'm missing something... Unfortunately, no. The error is being set but dropped in the audit fn, it seems I broke it during the auditing update, and that the regression test suite for networking is broken. I'll get the SRU patch out immediately.
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 6b0637b..9c51b03 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -575,10 +575,18 @@ static struct aa_profile *unpack_profile(struct aa_ext *e) size = unpack_array(e, "net_allowed_af"); if (size) { - if (size > AF_MAX) - goto fail; - for (i = 0; i < size; i++) { + /* discard extraneous rules that this kernel will + * never request + */ + if (size > AF_MAX) { + u16 tmp; + if (!unpack_u16(e, &tmp, NULL) || + !unpack_u16(e, &tmp, NULL) || + !unpack_u16(e, &tmp, NULL)) + goto fail; + continue; + } if (!unpack_u16(e, &profile->net.allow[i], NULL)) goto fail; if (!unpack_u16(e, &profile->net.audit[i], NULL))
BugLink: http://bugs.launchpad.net/bugs/639758 Remove an unnecessary restriction from the AppArmor network capability patch When a newer version of the tools is used with an older kernel, it may build in extra rules for newer networking protocols that the older kernel does not know about. The older kernel can safely discard the extra rules as it should never create sockets using the newer protocols, and the LSM hooks will never pass requests matching these rules. Signed-off-by: John Johansen <john.johansen@canonical.com> --- security/apparmor/policy_unpack.c | 14 +++++++++++--- 1 files changed, 11 insertions(+), 3 deletions(-)