linux-kernel - [PATCH 3.10 120/268] apparmor: fix module parameters can be changed after policy is locked

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 19 Jun 2017 20:30:19 +0200
From:   Willy Tarreau <w@....eu>
To:     linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        linux@...ck-us.net
Cc:     John Johansen <john.johansen@...onical.com>,
        Willy Tarreau <w@....eu>
Subject: [PATCH 3.10 120/268] apparmor: fix module parameters can be changed after policy is locked

From: John Johansen <john.johansen@...onical.com>

commit 58acf9d911c8831156634a44d0b022d683e1e50c upstream.

the policy_lock parameter is a one way switch that prevents policy
from being further modified. Unfortunately some of the module parameters
can effectively modify policy by turning off enforcement.

split policy_admin_capable into a view check and a full admin check,
and update the admin check to test the policy_lock parameter.

Signed-off-by: John Johansen <john.johansen@...onical.com>
Signed-off-by: Willy Tarreau <w@....eu>
---
 security/apparmor/include/policy.h |  2 ++
 security/apparmor/lsm.c            | 22 ++++++++++------------
 security/apparmor/policy.c         | 18 +++++++++++++++++-
 3 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h
index bda4569..0c9d121 100644
--- a/security/apparmor/include/policy.h
+++ b/security/apparmor/include/policy.h
@@ -313,6 +313,8 @@ static inline int AUDIT_MODE(struct aa_profile *profile)
 	return profile->audit;
 }
 
+bool policy_view_capable(void);
+bool policy_admin_capable(void);
 bool aa_may_manage_policy(int op);
 
 #endif /* __AA_POLICY_H */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index b21830e..6eeaab8 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -759,51 +759,49 @@ __setup("apparmor=", apparmor_enabled_setup);
 /* set global flag turning off the ability to load policy */
 static int param_set_aalockpolicy(const char *val, const struct kernel_param *kp)
 {
-	if (!capable(CAP_MAC_ADMIN))
+	if (!policy_admin_capable())
 		return -EPERM;
-	if (aa_g_lock_policy)
-		return -EACCES;
 	return param_set_bool(val, kp);
 }
 
 static int param_get_aalockpolicy(char *buffer, const struct kernel_param *kp)
 {
-	if (!capable(CAP_MAC_ADMIN))
+	if (!policy_view_capable())
 		return -EPERM;
 	return param_get_bool(buffer, kp);
 }
 
 static int param_set_aabool(const char *val, const struct kernel_param *kp)
 {
-	if (!capable(CAP_MAC_ADMIN))
+	if (!policy_admin_capable())
 		return -EPERM;
 	return param_set_bool(val, kp);
 }
 
 static int param_get_aabool(char *buffer, const struct kernel_param *kp)
 {
-	if (!capable(CAP_MAC_ADMIN))
+	if (!policy_view_capable())
 		return -EPERM;
 	return param_get_bool(buffer, kp);
 }
 
 static int param_set_aauint(const char *val, const struct kernel_param *kp)
 {
-	if (!capable(CAP_MAC_ADMIN))
+	if (!policy_admin_capable())
 		return -EPERM;
 	return param_set_uint(val, kp);
 }
 
 static int param_get_aauint(char *buffer, const struct kernel_param *kp)
 {
-	if (!capable(CAP_MAC_ADMIN))
+	if (!policy_view_capable())
 		return -EPERM;
 	return param_get_uint(buffer, kp);
 }
 
 static int param_get_audit(char *buffer, struct kernel_param *kp)
 {
-	if (!capable(CAP_MAC_ADMIN))
+	if (!policy_view_capable())
 		return -EPERM;
 
 	if (!apparmor_enabled)
@@ -815,7 +813,7 @@ static int param_get_audit(char *buffer, struct kernel_param *kp)
 static int param_set_audit(const char *val, struct kernel_param *kp)
 {
 	int i;
-	if (!capable(CAP_MAC_ADMIN))
+	if (!policy_admin_capable())
 		return -EPERM;
 
 	if (!apparmor_enabled)
@@ -836,7 +834,7 @@ static int param_set_audit(const char *val, struct kernel_param *kp)
 
 static int param_get_mode(char *buffer, struct kernel_param *kp)
 {
-	if (!capable(CAP_MAC_ADMIN))
+	if (!policy_admin_capable())
 		return -EPERM;
 
 	if (!apparmor_enabled)
@@ -848,7 +846,7 @@ static int param_get_mode(char *buffer, struct kernel_param *kp)
 static int param_set_mode(const char *val, struct kernel_param *kp)
 {
 	int i;
-	if (!capable(CAP_MAC_ADMIN))
+	if (!policy_admin_capable())
 		return -EPERM;
 
 	if (!apparmor_enabled)
diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index 8132003..c4780e1 100644
--- a/security/apparmor/policy.c
+++ b/security/apparmor/policy.c
@@ -1002,6 +1002,22 @@ static int audit_policy(int op, gfp_t gfp, const char *name, const char *info,
 			&sa, NULL);
 }
 
+bool policy_view_capable(void)
+{
+	struct user_namespace *user_ns = current_user_ns();
+	bool response = false;
+
+	if (ns_capable(user_ns, CAP_MAC_ADMIN))
+		response = true;
+
+	return response;
+}
+
+bool policy_admin_capable(void)
+{
+	return policy_view_capable() && !aa_g_lock_policy;
+}
+
 /**
  * aa_may_manage_policy - can the current task manage policy
  * @op: the policy manipulation operation being done
@@ -1016,7 +1032,7 @@ bool aa_may_manage_policy(int op)
 		return 0;
 	}
 
-	if (!capable(CAP_MAC_ADMIN)) {
+	if (!policy_admin_capable()) {
 		audit_policy(op, GFP_KERNEL, NULL, "not policy admin", -EACCES);
 		return 0;
 	}
-- 
2.8.0.rc2.1.gbe9624a