Bug#676515: linux-2.6: AppArmor totally broken
On 06/23/2012 11:53 AM, intrigeri wrote:
> Hi John,
>
> John Johansen wrote (17 Jun 2012 19:08:20 GMT) :
>> On 06/15/2012 05:08 PM, Ben Hutchings wrote:
>>>>
>>>>>> If we don't want to restrict sockets used by the kernel, don't we need
>>>>>> to store the kern flag for later use by aa_revalidate_sk()?
>>>>>>
>>>>> For how apparmor is generally deployed it can get away with this, the
>>>>> kernel bits generally bail out earlier on the check for unconfined.
>>>>
>>>>> That is not to say it isn't a good idea, or that it shouldn't be done.
>>>>> The fact is this patch is going to be replaced with completely rewritten
>>>>> controls, that do store info on the socket, it just hasn't happened yet
>>>>> due to resources and priorities (not my priorities).
>>>>
>>>> Ben, is this a blocker?
>>>
>>> I want to be convinced that this is not a bug, or else get a fix for it.
>>>
>> I am looking at the kernel bits here, but I don't have a patch yet
>
> Do you think you'll manage to do it in time for the Wheezy freeze
> (June 30th)?
>
>>>>>> Since denied has already been masked with ~quiet_mask, this condition
>>>>>> can never be true.
>>>>>>
>>>>> indeed
>>>>
>>>> Ben, is this a blocker?
>>> [...]
>>>
>>> This clearly is a bug and I want to be convinced that it is harmless or
>>> else get a fix for it.
>>>
>> Right this breaks the controls over quieting of denial messages. Basically
>> if policy specifies a reject should not be logged then the global controls
>> that turn quieting off so that all rejects get logged aren't working for
>> networking.
>
>> This is an easy patch that I can provide separately or with the
>> patch I am working on for the larger issue.
>
> Do you think you'll manage to prepare at least the easy fix it in time
> for the Wheezy freeze?
>
Okay, there are 4 kernel patches, not all of them are needed depending on whether
the network patch is applied or not.
If you don't want to apply the networking patch
0001-apparmor-remove-advertising-the-support-of-network-r.patch
Stops the kernel interface from incorrectly advertising that it supports network
rules. A further patch (not attached) to userspace will also have to be applied
If the networking patch is applied
these two patches can be applied or ignored, 0001 will be folded into the compat
interface patch upstream, and then 0002 will be folded into the networking patch
0001-apparmor-remove-advertising-the-support-of-network-r.patch
0002-apparmor-Advertise-network-mediation-from-the-compat.patch
these two patches address the two bugs pointed out in the networking patch
0003-apparmor-Fix-quieting-of-audit-messages-for-network-.patch
0004-apparmor-Ensure-apparmor-does-not-mediate-kernel-bas.patch
>From 873143ceca69a2e54e7face1be49ad6b5514525d Mon Sep 17 00:00:00 2001
From: John Johansen <john.johansen@canonical.com>
Date: Tue, 26 Jun 2012 02:12:10 -0700
Subject: [PATCH 1/4] apparmor: remove advertising the support of network
rules from compat iface
The interface compatibility patch was advertising support of network rules,
however this is not true if the networking patch is not applied. Move
advertising of network rules into a third patch that can be applied if
both the compatibility and network patches are applied.
Signed-off-by: John Johansen <john.johansen@canonical.com>
---
security/apparmor/apparmorfs-24.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/apparmor/apparmorfs-24.c b/security/apparmor/apparmorfs-24.c
index dc8c744..367c7ea 100644
--- a/security/apparmor/apparmorfs-24.c
+++ b/security/apparmor/apparmorfs-24.c
@@ -49,7 +49,7 @@ const struct file_operations aa_fs_matching_fops = {
static ssize_t aa_features_read(struct file *file, char __user *buf,
size_t size, loff_t *ppos)
{
- const char features[] = "file=3.1 capability=2.0 network=1.0 "
+ const char features[] = "file=3.1 capability=2.0 "
"change_hat=1.5 change_profile=1.1 " "aanamespaces=1.1 rlimit=1.1";
return simple_read_from_buffer(buf, size, ppos, features,
--
1.7.9.5
>From 89a577e1bd55ee589c66bb82babb1dbbb8d73dad Mon Sep 17 00:00:00 2001
From: John Johansen <john.johansen@canonical.com>
Date: Tue, 26 Jun 2012 02:15:36 -0700
Subject: [PATCH 2/4] apparmor: Advertise network mediation from the
compatibility interface
The userspace needs to know if the apparmor kernel module supports network
mediation. Apply this patch if both the v2.3 compatibility patch and
network mediation patches are applied.
Signed-off-by: John Johansen <john.johansen@canonical.com>
---
security/apparmor/apparmorfs-24.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/apparmor/apparmorfs-24.c b/security/apparmor/apparmorfs-24.c
index 367c7ea..dc8c744 100644
--- a/security/apparmor/apparmorfs-24.c
+++ b/security/apparmor/apparmorfs-24.c
@@ -49,7 +49,7 @@ const struct file_operations aa_fs_matching_fops = {
static ssize_t aa_features_read(struct file *file, char __user *buf,
size_t size, loff_t *ppos)
{
- const char features[] = "file=3.1 capability=2.0 "
+ const char features[] = "file=3.1 capability=2.0 network=1.0 "
"change_hat=1.5 change_profile=1.1 " "aanamespaces=1.1 rlimit=1.1";
return simple_read_from_buffer(buf, size, ppos, features,
--
1.7.9.5
>From a875a5ada51351ce79bec19e4b6f138fceb62f90 Mon Sep 17 00:00:00 2001
From: John Johansen <john.johansen@canonical.com>
Date: Sun, 24 Jun 2012 15:58:00 -0700
Subject: [PATCH 3/4] apparmor: Fix quieting of audit messages for network
mediation
If a profile specified a quieting of network denials for a given rule by
either the quiet or deny rule qualifiers, the resultant quiet mask for
denied requests was applied incorrectly, resulting in two potential bugs.
1. The misapplied quiet mask would prevent denials from being correctly
tested against the kill mask/mode. Thus network access requests that
should have resulted in the application being killed did not.
2. The actual quieting of the denied network request was not being applied.
This would result in network rejections always being logged even when
they had been specifically marked as quieted.
Signed-off-by: John Johansen <john.johansen@canonical.com>
---
security/apparmor/net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/apparmor/net.c b/security/apparmor/net.c
index 1765901..cf90973 100644
--- a/security/apparmor/net.c
+++ b/security/apparmor/net.c
@@ -96,7 +96,7 @@ static int audit_net(struct aa_profile *profile, int op, u16 family, int type,
} else {
u16 quiet_mask = profile->net.quiet[sa.u.net.family];
u16 kill_mask = 0;
- u16 denied = (1 << sa.aad.net.type) & ~quiet_mask;
+ u16 denied = (1 << sa.aad.net.type);
if (denied & kill_mask)
audit_type = AUDIT_APPARMOR_KILL;
--
1.7.9.5
>From 1aeab576d4007a1cc3ad6cb25bb0af9e3fb6c36c Mon Sep 17 00:00:00 2001
From: John Johansen <john.johansen@canonical.com>
Date: Tue, 26 Jun 2012 02:07:35 -0700
Subject: [PATCH 4/4] apparmor: Ensure apparmor does not mediate kernel based
sockets
Currently apparmor makes the assumption that kernel sockets are unmediated
because mediation is only done against tasks that have a profile attached.
Ensure we never get in a situation where a kernel socket is being mediated
by tagging the sk_security field for kernel sockets.
Signed-off-by: John Johansen <john.johansen@canonical.com>
---
security/apparmor/include/net.h | 2 ++
security/apparmor/lsm.c | 18 ++++++++++++++++++
security/apparmor/net.c | 3 +++
3 files changed, 23 insertions(+)
diff --git a/security/apparmor/include/net.h b/security/apparmor/include/net.h
index 3c7d599..cdb3320 100644
--- a/security/apparmor/include/net.h
+++ b/security/apparmor/include/net.h
@@ -17,6 +17,8 @@
#include <net/sock.h>
+#define AA_SOCK_KERN 0xAA
+
/* struct aa_net - network confinement data
* @allowed: basic network families permissions
* @audit_network: which network permissions to force audit
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 7459547..8cebf66 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -637,6 +637,16 @@ static int apparmor_socket_create(int family, int type, int protocol, int kern)
return error;
}
+static int apparmor_socket_post_create(struct socket *sock, int family,
+ int type, int protocol, int kern)
+{
+ if (kern)
+ /* tag kernel sockets so we don't mediate them later */
+ sock->sk->sk_security = (void *) AA_SOCK_KERN;
+
+ return 0;
+}
+
static int apparmor_socket_bind(struct socket *sock,
struct sockaddr *address, int addrlen)
{
@@ -720,6 +730,12 @@ static int apparmor_socket_shutdown(struct socket *sock, int how)
return aa_revalidate_sk(OP_SOCK_SHUTDOWN, sk);
}
+static void apparmor_sk_clone_security(const struct sock *sk,
+ struct sock *newsk)
+{
+ newsk->sk_security = sk->sk_security;
+}
+
static struct security_operations apparmor_ops = {
.name = "apparmor",
@@ -752,6 +768,7 @@ static struct security_operations apparmor_ops = {
.setprocattr = apparmor_setprocattr,
.socket_create = apparmor_socket_create,
+ .socket_post_create = apparmor_socket_post_create,
.socket_bind = apparmor_socket_bind,
.socket_connect = apparmor_socket_connect,
.socket_listen = apparmor_socket_listen,
@@ -763,6 +780,7 @@ static struct security_operations apparmor_ops = {
.socket_getsockopt = apparmor_socket_getsockopt,
.socket_setsockopt = apparmor_socket_setsockopt,
.socket_shutdown = apparmor_socket_shutdown,
+ .sk_clone_security = apparmor_sk_clone_security,
.cred_alloc_blank = apparmor_cred_alloc_blank,
.cred_free = apparmor_cred_free,
diff --git a/security/apparmor/net.c b/security/apparmor/net.c
index cf90973..f56987e 100644
--- a/security/apparmor/net.c
+++ b/security/apparmor/net.c
@@ -161,6 +161,9 @@ int aa_revalidate_sk(int op, struct sock *sk)
if (in_interrupt())
return 0;
+ if (sk->sk_security == (void *) AA_SOCK_KERN)
+ return 0;
+
profile = __aa_current_profile();
if (!unconfined(profile))
error = aa_net_perm(op, profile, sk->sk_family, sk->sk_type,
--
1.7.9.5
Reply to: