From c783d525f9e7e1fd6a5dc3379f3c1fb041495b76 Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Wed, 10 Apr 2019 09:54:34 -0700 Subject: LSM: SafeSetID: fix pr_warn() to include newline Fix the pr_warn() calls in the SafeSetID LSM to have newlines at the end. Without this, denial messages will be buffered as incomplete lines in log_output(), and will then only show up once something else prints into dmesg. Signed-off-by: Jann Horn Signed-off-by: Micah Morton --- security/safesetid/lsm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c index 06d4259f9ab1..ac55bf193213 100644 --- a/security/safesetid/lsm.c +++ b/security/safesetid/lsm.c @@ -91,7 +91,7 @@ static int safesetid_security_capable(const struct cred *cred, * to functionality other than calling set*uid() (e.g. * allowing user to set up userns uid mappings). */ - pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions", + pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n", __kuid_val(cred->uid)); return -1; } @@ -103,7 +103,7 @@ static int check_uid_transition(kuid_t parent, kuid_t child) { if (check_setuid_policy_hashtable_key_value(parent, child)) return 0; - pr_warn("UID transition (%d -> %d) blocked", + pr_warn("UID transition (%d -> %d) blocked\n", __kuid_val(parent), __kuid_val(child)); /* -- cgit v1.2.3 From 7ef6b3062fb9f0b9dbaaec182495189459100807 Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Wed, 10 Apr 2019 09:55:19 -0700 Subject: LSM: SafeSetID: fix check for setresuid(new1, new2, new3) With the old code, when a process with the (real,effective,saved) UID set (1,1,1) calls setresuid(2,3,4), safesetid_task_fix_setuid() only checks whether the transition 1->2 is permitted; the transitions 1->3 and 1->4 are not checked. Fix this. This is also a good opportunity to refactor safesetid_task_fix_setuid() to be less verbose - having one branch per set*uid() syscall is unnecessary. Note that this slightly changes semantics: The UID transition check for UIDs that were not in the old cred struct is now always performed against the policy of the RUID. I think that's more consistent anyway, since the RUID is also the one that decides whether any policy is enforced at all. Signed-off-by: Jann Horn Signed-off-by: Micah Morton --- security/safesetid/lsm.c | 125 +++++++++++++---------------------------------- 1 file changed, 35 insertions(+), 90 deletions(-) diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c index ac55bf193213..0770447d51f0 100644 --- a/security/safesetid/lsm.c +++ b/security/safesetid/lsm.c @@ -99,20 +99,30 @@ static int safesetid_security_capable(const struct cred *cred, return 0; } -static int check_uid_transition(kuid_t parent, kuid_t child) +/* + * Check whether a caller with old credentials @old is allowed to switch to + * credentials that contain @new_uid. + */ +static bool uid_permitted_for_cred(const struct cred *old, kuid_t new_uid) { - if (check_setuid_policy_hashtable_key_value(parent, child)) - return 0; - pr_warn("UID transition (%d -> %d) blocked\n", - __kuid_val(parent), - __kuid_val(child)); + bool permitted; + + /* If our old creds already had this UID in it, it's fine. */ + if (uid_eq(new_uid, old->uid) || uid_eq(new_uid, old->euid) || + uid_eq(new_uid, old->suid)) + return true; + /* - * Kill this process to avoid potential security vulnerabilities - * that could arise from a missing whitelist entry preventing a - * privileged process from dropping to a lesser-privileged one. + * Transitions to new UIDs require a check against the policy of the old + * RUID. */ - force_sig(SIGKILL); - return -EACCES; + permitted = check_setuid_policy_hashtable_key_value(old->uid, new_uid); + if (!permitted) { + pr_warn("UID transition ((%d,%d,%d) -> %d) blocked\n", + __kuid_val(old->uid), __kuid_val(old->euid), + __kuid_val(old->suid), __kuid_val(new_uid)); + } + return permitted; } /* @@ -125,88 +135,23 @@ static int safesetid_task_fix_setuid(struct cred *new, int flags) { - /* Do nothing if there are no setuid restrictions for this UID. */ + /* Do nothing if there are no setuid restrictions for our old RUID. */ if (!check_setuid_policy_hashtable_key(old->uid)) return 0; - switch (flags) { - case LSM_SETID_RE: - /* - * Users for which setuid restrictions exist can only set the - * real UID to the real UID or the effective UID, unless an - * explicit whitelist policy allows the transition. - */ - if (!uid_eq(old->uid, new->uid) && - !uid_eq(old->euid, new->uid)) { - return check_uid_transition(old->uid, new->uid); - } - /* - * Users for which setuid restrictions exist can only set the - * effective UID to the real UID, the effective UID, or the - * saved set-UID, unless an explicit whitelist policy allows - * the transition. - */ - if (!uid_eq(old->uid, new->euid) && - !uid_eq(old->euid, new->euid) && - !uid_eq(old->suid, new->euid)) { - return check_uid_transition(old->euid, new->euid); - } - break; - case LSM_SETID_ID: - /* - * Users for which setuid restrictions exist cannot change the - * real UID or saved set-UID unless an explicit whitelist - * policy allows the transition. - */ - if (!uid_eq(old->uid, new->uid)) - return check_uid_transition(old->uid, new->uid); - if (!uid_eq(old->suid, new->suid)) - return check_uid_transition(old->suid, new->suid); - break; - case LSM_SETID_RES: - /* - * Users for which setuid restrictions exist cannot change the - * real UID, effective UID, or saved set-UID to anything but - * one of: the current real UID, the current effective UID or - * the current saved set-user-ID unless an explicit whitelist - * policy allows the transition. - */ - if (!uid_eq(new->uid, old->uid) && - !uid_eq(new->uid, old->euid) && - !uid_eq(new->uid, old->suid)) { - return check_uid_transition(old->uid, new->uid); - } - if (!uid_eq(new->euid, old->uid) && - !uid_eq(new->euid, old->euid) && - !uid_eq(new->euid, old->suid)) { - return check_uid_transition(old->euid, new->euid); - } - if (!uid_eq(new->suid, old->uid) && - !uid_eq(new->suid, old->euid) && - !uid_eq(new->suid, old->suid)) { - return check_uid_transition(old->suid, new->suid); - } - break; - case LSM_SETID_FS: - /* - * Users for which setuid restrictions exist cannot change the - * filesystem UID to anything but one of: the current real UID, - * the current effective UID or the current saved set-UID - * unless an explicit whitelist policy allows the transition. - */ - if (!uid_eq(new->fsuid, old->uid) && - !uid_eq(new->fsuid, old->euid) && - !uid_eq(new->fsuid, old->suid) && - !uid_eq(new->fsuid, old->fsuid)) { - return check_uid_transition(old->fsuid, new->fsuid); - } - break; - default: - pr_warn("Unknown setid state %d\n", flags); - force_sig(SIGKILL); - return -EINVAL; - } - return 0; + if (uid_permitted_for_cred(old, new->uid) && + uid_permitted_for_cred(old, new->euid) && + uid_permitted_for_cred(old, new->suid) && + uid_permitted_for_cred(old, new->fsuid)) + return 0; + + /* + * Kill this process to avoid potential security vulnerabilities + * that could arise from a missing whitelist entry preventing a + * privileged process from dropping to a lesser-privileged one. + */ + force_sig(SIGKILL); + return -EACCES; } int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child) -- cgit v1.2.3 From 1cd02a27a9473fed0294561137cfb7dcc9b3aaa0 Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Wed, 10 Apr 2019 09:55:34 -0700 Subject: LSM: SafeSetID: refactor policy hash table parent_kuid and child_kuid are kuids, there is no reason to make them uint64_t. (And anyway, in the kernel, the normal name for that would be u64, not uint64_t.) check_setuid_policy_hashtable_key() and check_setuid_policy_hashtable_key_value() are basically the same thing, merge them. Also fix the comment that claimed that (1<<8)==128. Signed-off-by: Jann Horn Signed-off-by: Micah Morton --- security/safesetid/lsm.c | 62 ++++++++++++++---------------------------------- security/safesetid/lsm.h | 19 +++++++++++++++ 2 files changed, 37 insertions(+), 44 deletions(-) diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c index 0770447d51f0..56e1b285a4ae 100644 --- a/security/safesetid/lsm.c +++ b/security/safesetid/lsm.c @@ -14,67 +14,40 @@ #define pr_fmt(fmt) "SafeSetID: " fmt -#include #include #include #include #include #include +#include "lsm.h" /* Flag indicating whether initialization completed */ int safesetid_initialized; -#define NUM_BITS 8 /* 128 buckets in hash table */ +#define NUM_BITS 8 /* 256 buckets in hash table */ static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS); -/* - * Hash table entry to store safesetid policy signifying that 'parent' user - * can setid to 'child' user. - */ -struct entry { - struct hlist_node next; - struct hlist_node dlist; /* for deletion cleanup */ - uint64_t parent_kuid; - uint64_t child_kuid; -}; - static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock); -static bool check_setuid_policy_hashtable_key(kuid_t parent) +static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst) { struct entry *entry; + enum sid_policy_type result = SIDPOL_DEFAULT; rcu_read_lock(); hash_for_each_possible_rcu(safesetid_whitelist_hashtable, - entry, next, __kuid_val(parent)) { - if (entry->parent_kuid == __kuid_val(parent)) { + entry, next, __kuid_val(src)) { + if (!uid_eq(entry->src_uid, src)) + continue; + if (uid_eq(entry->dst_uid, dst)) { rcu_read_unlock(); - return true; + return SIDPOL_ALLOWED; } + result = SIDPOL_CONSTRAINED; } rcu_read_unlock(); - - return false; -} - -static bool check_setuid_policy_hashtable_key_value(kuid_t parent, - kuid_t child) -{ - struct entry *entry; - - rcu_read_lock(); - hash_for_each_possible_rcu(safesetid_whitelist_hashtable, - entry, next, __kuid_val(parent)) { - if (entry->parent_kuid == __kuid_val(parent) && - entry->child_kuid == __kuid_val(child)) { - rcu_read_unlock(); - return true; - } - } - rcu_read_unlock(); - - return false; + return result; } static int safesetid_security_capable(const struct cred *cred, @@ -83,7 +56,7 @@ static int safesetid_security_capable(const struct cred *cred, unsigned int opts) { if (cap == CAP_SETUID && - check_setuid_policy_hashtable_key(cred->uid)) { + setuid_policy_lookup(cred->uid, INVALID_UID) != SIDPOL_DEFAULT) { if (!(opts & CAP_OPT_INSETID)) { /* * Deny if we're not in a set*uid() syscall to avoid @@ -116,7 +89,8 @@ static bool uid_permitted_for_cred(const struct cred *old, kuid_t new_uid) * Transitions to new UIDs require a check against the policy of the old * RUID. */ - permitted = check_setuid_policy_hashtable_key_value(old->uid, new_uid); + permitted = + setuid_policy_lookup(old->uid, new_uid) != SIDPOL_CONSTRAINED; if (!permitted) { pr_warn("UID transition ((%d,%d,%d) -> %d) blocked\n", __kuid_val(old->uid), __kuid_val(old->euid), @@ -136,7 +110,7 @@ static int safesetid_task_fix_setuid(struct cred *new, { /* Do nothing if there are no setuid restrictions for our old RUID. */ - if (!check_setuid_policy_hashtable_key(old->uid)) + if (setuid_policy_lookup(old->uid, INVALID_UID) == SIDPOL_DEFAULT) return 0; if (uid_permitted_for_cred(old, new->uid) && @@ -159,14 +133,14 @@ int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child) struct entry *new; /* Return if entry already exists */ - if (check_setuid_policy_hashtable_key_value(parent, child)) + if (setuid_policy_lookup(parent, child) == SIDPOL_ALLOWED) return 0; new = kzalloc(sizeof(struct entry), GFP_KERNEL); if (!new) return -ENOMEM; - new->parent_kuid = __kuid_val(parent); - new->child_kuid = __kuid_val(child); + new->src_uid = parent; + new->dst_uid = child; spin_lock(&safesetid_whitelist_hashtable_spinlock); hash_add_rcu(safesetid_whitelist_hashtable, &new->next, diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h index c1ea3c265fcf..6806f902794c 100644 --- a/security/safesetid/lsm.h +++ b/security/safesetid/lsm.h @@ -15,6 +15,8 @@ #define _SAFESETID_H #include +#include +#include /* Flag indicating whether initialization completed */ extern int safesetid_initialized; @@ -25,6 +27,23 @@ enum safesetid_whitelist_file_write_type { SAFESETID_WHITELIST_FLUSH, /* Flush whitelist policies. */ }; +enum sid_policy_type { + SIDPOL_DEFAULT, /* source ID is unaffected by policy */ + SIDPOL_CONSTRAINED, /* source ID is affected by policy */ + SIDPOL_ALLOWED /* target ID explicitly allowed */ +}; + +/* + * Hash table entry to store safesetid policy signifying that 'src_uid' + * can setid to 'dst_uid'. + */ +struct entry { + struct hlist_node next; + struct hlist_node dlist; /* for deletion cleanup */ + kuid_t src_uid; + kuid_t dst_uid; +}; + /* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */ int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child); -- cgit v1.2.3 From 8068866c4af124345e2a129be921278aada7830f Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Wed, 10 Apr 2019 09:55:41 -0700 Subject: LSM: SafeSetID: refactor safesetid_security_capable() At the moment, safesetid_security_capable() has two nested conditional blocks, and one big comment for all the logic. Chop it up and reduce the amount of indentation. Signed-off-by: Jann Horn Signed-off-by: Micah Morton --- security/safesetid/lsm.c | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c index 56e1b285a4ae..9db1c7a51d3d 100644 --- a/security/safesetid/lsm.c +++ b/security/safesetid/lsm.c @@ -55,21 +55,32 @@ static int safesetid_security_capable(const struct cred *cred, int cap, unsigned int opts) { - if (cap == CAP_SETUID && - setuid_policy_lookup(cred->uid, INVALID_UID) != SIDPOL_DEFAULT) { - if (!(opts & CAP_OPT_INSETID)) { - /* - * Deny if we're not in a set*uid() syscall to avoid - * giving powers gated by CAP_SETUID that are related - * to functionality other than calling set*uid() (e.g. - * allowing user to set up userns uid mappings). - */ - pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n", - __kuid_val(cred->uid)); - return -1; - } - } - return 0; + /* We're only interested in CAP_SETUID. */ + if (cap != CAP_SETUID) + return 0; + + /* + * If CAP_SETUID is currently used for a set*uid() syscall, we want to + * let it go through here; the real security check happens later, in the + * task_fix_setuid hook. + */ + if ((opts & CAP_OPT_INSETID) != 0) + return 0; + + /* + * If no policy applies to this task, allow the use of CAP_SETUID for + * other purposes. + */ + if (setuid_policy_lookup(cred->uid, INVALID_UID) == SIDPOL_DEFAULT) + return 0; + + /* + * Reject use of CAP_SETUID for functionality other than calling + * set*uid() (e.g. setting up userns uid mappings). + */ + pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n", + __kuid_val(cred->uid)); + return -1; } /* -- cgit v1.2.3 From 78ae7df96d647627ceae0b65eea9e4f83a0a4b66 Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Wed, 10 Apr 2019 09:55:48 -0700 Subject: LSM: SafeSetID: refactor policy parsing In preparation for changing the policy parsing logic, refactor the line parsing logic to be less verbose and move it into a separate function. Signed-off-by: Jann Horn Signed-off-by: Micah Morton --- security/safesetid/securityfs.c | 84 ++++++++++++++++------------------------- 1 file changed, 33 insertions(+), 51 deletions(-) diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c index 2c6c829be044..90784a8d950a 100644 --- a/security/safesetid/securityfs.c +++ b/security/safesetid/securityfs.c @@ -33,68 +33,50 @@ static struct safesetid_file_entry safesetid_files[] = { /* * In the case the input buffer contains one or more invalid UIDs, the kuid_t - * variables pointed to by 'parent' and 'child' will get updated but this + * variables pointed to by @parent and @child will get updated but this * function will return an error. + * Contents of @buf may be modified. */ -static int parse_safesetid_whitelist_policy(const char __user *buf, - size_t len, - kuid_t *parent, - kuid_t *child) +static int parse_policy_line( + struct file *file, char *buf, kuid_t *parent, kuid_t *child) { - char *kern_buf; - char *parent_buf; - char *child_buf; - const char separator[] = ":"; + char *child_str; int ret; - size_t first_substring_length; - long parsed_parent; - long parsed_child; + u32 parsed_parent, parsed_child; - /* Duplicate string from user memory and NULL-terminate */ - kern_buf = memdup_user_nul(buf, len); - if (IS_ERR(kern_buf)) - return PTR_ERR(kern_buf); - - /* - * Format of |buf| string should be :. - * Find location of ":" in kern_buf (copied from |buf|). - */ - first_substring_length = strcspn(kern_buf, separator); - if (first_substring_length == 0 || first_substring_length == len) { - ret = -EINVAL; - goto free_kern; - } - - parent_buf = kmemdup_nul(kern_buf, first_substring_length, GFP_KERNEL); - if (!parent_buf) { - ret = -ENOMEM; - goto free_kern; - } + /* Format of |buf| string should be :. */ + child_str = strchr(buf, ':'); + if (child_str == NULL) + return -EINVAL; + *child_str = '\0'; + child_str++; - ret = kstrtol(parent_buf, 0, &parsed_parent); + ret = kstrtou32(buf, 0, &parsed_parent); if (ret) - goto free_both; + return ret; - child_buf = kern_buf + first_substring_length + 1; - ret = kstrtol(child_buf, 0, &parsed_child); + ret = kstrtou32(child_str, 0, &parsed_child); if (ret) - goto free_both; + return ret; *parent = make_kuid(current_user_ns(), parsed_parent); - if (!uid_valid(*parent)) { - ret = -EINVAL; - goto free_both; - } - *child = make_kuid(current_user_ns(), parsed_child); - if (!uid_valid(*child)) { - ret = -EINVAL; - goto free_both; - } + if (!uid_valid(*parent) || !uid_valid(*child)) + return -EINVAL; -free_both: - kfree(parent_buf); -free_kern: + return 0; +} + +static int parse_safesetid_whitelist_policy( + struct file *file, const char __user *buf, size_t len, + kuid_t *parent, kuid_t *child) +{ + char *kern_buf = memdup_user_nul(buf, len); + int ret; + + if (IS_ERR(kern_buf)) + return PTR_ERR(kern_buf); + ret = parse_policy_line(file, kern_buf, parent, child); kfree(kern_buf); return ret; } @@ -121,8 +103,8 @@ static ssize_t safesetid_file_write(struct file *file, flush_safesetid_whitelist_entries(); break; case SAFESETID_WHITELIST_ADD: - ret = parse_safesetid_whitelist_policy(buf, len, &parent, - &child); + ret = parse_safesetid_whitelist_policy(file, buf, len, + &parent, &child); if (ret) return ret; -- cgit v1.2.3 From 71a98971b932174e121bc19056475c601598132f Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Wed, 10 Apr 2019 09:55:58 -0700 Subject: LSM: SafeSetID: fix userns handling in securityfs Looking at current_cred() in write handlers is bad form, stop doing that. Also, let's just require that the write is coming from the initial user namespace. Especially SAFESETID_WHITELIST_FLUSH requires privilege over all namespaces, and SAFESETID_WHITELIST_ADD should probably require it as well. Signed-off-by: Jann Horn Signed-off-by: Micah Morton --- security/safesetid/securityfs.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c index 90784a8d950a..77d301f0ff7a 100644 --- a/security/safesetid/securityfs.c +++ b/security/safesetid/securityfs.c @@ -59,8 +59,8 @@ static int parse_policy_line( if (ret) return ret; - *parent = make_kuid(current_user_ns(), parsed_parent); - *child = make_kuid(current_user_ns(), parsed_child); + *parent = make_kuid(file->f_cred->user_ns, parsed_parent); + *child = make_kuid(file->f_cred->user_ns, parsed_child); if (!uid_valid(*parent) || !uid_valid(*child)) return -EINVAL; @@ -92,7 +92,7 @@ static ssize_t safesetid_file_write(struct file *file, kuid_t child; int ret; - if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN)) + if (!file_ns_capable(file, &init_user_ns, CAP_MAC_ADMIN)) return -EPERM; if (*ppos != 0) -- cgit v1.2.3 From 03638e62f55f27e7a96d6b1175e75b7a81e562b3 Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Wed, 10 Apr 2019 09:56:05 -0700 Subject: LSM: SafeSetID: rewrite userspace API to atomic updates The current API of the SafeSetID LSM uses one write() per rule, and applies each written rule instantly. This has several downsides: - While a policy is being loaded, once a single parent-child pair has been loaded, the parent is restricted to that specific child, even if subsequent rules would allow transitions to other child UIDs. This means that during policy loading, set*uid() can randomly fail. - To replace the policy without rebooting, it is necessary to first flush all old rules. This creates a time window in which no constraints are placed on the use of CAP_SETUID. - If we want to perform sanity checks on the final policy, this requires that the policy isn't constructed in a piecemeal fashion without telling the kernel when it's done. Other kernel APIs - including things like the userns code and netfilter - avoid this problem by performing updates atomically. Luckily, SafeSetID hasn't landed in a stable (upstream) release yet, so maybe it's not too late to completely change the API. The new API for SafeSetID is: If you want to change the policy, open "safesetid/whitelist_policy" and write the entire policy, newline-delimited, in there. Signed-off-by: Jann Horn Signed-off-by: Micah Morton --- security/safesetid/lsm.c | 84 +++------ security/safesetid/lsm.h | 24 +-- security/safesetid/securityfs.c | 194 ++++++++++++--------- tools/testing/selftests/safesetid/safesetid-test.c | 16 +- 4 files changed, 149 insertions(+), 169 deletions(-) diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c index 9db1c7a51d3d..22964e2a6187 100644 --- a/security/safesetid/lsm.c +++ b/security/safesetid/lsm.c @@ -24,28 +24,38 @@ /* Flag indicating whether initialization completed */ int safesetid_initialized; -#define NUM_BITS 8 /* 256 buckets in hash table */ +struct setuid_ruleset __rcu *safesetid_setuid_rules; -static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS); - -static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock); - -static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst) +/* Compute a decision for a transition from @src to @dst under @policy. */ +enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy, + kuid_t src, kuid_t dst) { - struct entry *entry; + struct setuid_rule *rule; enum sid_policy_type result = SIDPOL_DEFAULT; - rcu_read_lock(); - hash_for_each_possible_rcu(safesetid_whitelist_hashtable, - entry, next, __kuid_val(src)) { - if (!uid_eq(entry->src_uid, src)) + hash_for_each_possible(policy->rules, rule, next, __kuid_val(src)) { + if (!uid_eq(rule->src_uid, src)) continue; - if (uid_eq(entry->dst_uid, dst)) { - rcu_read_unlock(); + if (uid_eq(rule->dst_uid, dst)) return SIDPOL_ALLOWED; - } result = SIDPOL_CONSTRAINED; } + return result; +} + +/* + * Compute a decision for a transition from @src to @dst under the active + * policy. + */ +static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst) +{ + enum sid_policy_type result = SIDPOL_DEFAULT; + struct setuid_ruleset *pol; + + rcu_read_lock(); + pol = rcu_dereference(safesetid_setuid_rules); + if (pol) + result = _setuid_policy_lookup(pol, src, dst); rcu_read_unlock(); return result; } @@ -139,52 +149,6 @@ static int safesetid_task_fix_setuid(struct cred *new, return -EACCES; } -int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child) -{ - struct entry *new; - - /* Return if entry already exists */ - if (setuid_policy_lookup(parent, child) == SIDPOL_ALLOWED) - return 0; - - new = kzalloc(sizeof(struct entry), GFP_KERNEL); - if (!new) - return -ENOMEM; - new->src_uid = parent; - new->dst_uid = child; - spin_lock(&safesetid_whitelist_hashtable_spinlock); - hash_add_rcu(safesetid_whitelist_hashtable, - &new->next, - __kuid_val(parent)); - spin_unlock(&safesetid_whitelist_hashtable_spinlock); - return 0; -} - -void flush_safesetid_whitelist_entries(void) -{ - struct entry *entry; - struct hlist_node *hlist_node; - unsigned int bkt_loop_cursor; - HLIST_HEAD(free_list); - - /* - * Could probably use hash_for_each_rcu here instead, but this should - * be fine as well. - */ - spin_lock(&safesetid_whitelist_hashtable_spinlock); - hash_for_each_safe(safesetid_whitelist_hashtable, bkt_loop_cursor, - hlist_node, entry, next) { - hash_del_rcu(&entry->next); - hlist_add_head(&entry->dlist, &free_list); - } - spin_unlock(&safesetid_whitelist_hashtable_spinlock); - synchronize_rcu(); - hlist_for_each_entry_safe(entry, hlist_node, &free_list, dlist) { - hlist_del(&entry->dlist); - kfree(entry); - } -} - static struct security_hook_list safesetid_security_hooks[] = { LSM_HOOK_INIT(task_fix_setuid, safesetid_task_fix_setuid), LSM_HOOK_INIT(capable, safesetid_security_capable) diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h index 6806f902794c..4a34f558d964 100644 --- a/security/safesetid/lsm.h +++ b/security/safesetid/lsm.h @@ -21,12 +21,6 @@ /* Flag indicating whether initialization completed */ extern int safesetid_initialized; -/* Function type. */ -enum safesetid_whitelist_file_write_type { - SAFESETID_WHITELIST_ADD, /* Add whitelist policy. */ - SAFESETID_WHITELIST_FLUSH, /* Flush whitelist policies. */ -}; - enum sid_policy_type { SIDPOL_DEFAULT, /* source ID is unaffected by policy */ SIDPOL_CONSTRAINED, /* source ID is affected by policy */ @@ -35,18 +29,24 @@ enum sid_policy_type { /* * Hash table entry to store safesetid policy signifying that 'src_uid' - * can setid to 'dst_uid'. + * can setuid to 'dst_uid'. */ -struct entry { +struct setuid_rule { struct hlist_node next; - struct hlist_node dlist; /* for deletion cleanup */ kuid_t src_uid; kuid_t dst_uid; }; -/* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */ -int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child); +#define SETID_HASH_BITS 8 /* 256 buckets in hash table */ + +struct setuid_ruleset { + DECLARE_HASHTABLE(rules, SETID_HASH_BITS); + struct rcu_head rcu; +}; + +enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy, + kuid_t src, kuid_t dst); -void flush_safesetid_whitelist_entries(void); +extern struct setuid_ruleset __rcu *safesetid_setuid_rules; #endif /* _SAFESETID_H */ diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c index 77d301f0ff7a..250d59e046c1 100644 --- a/security/safesetid/securityfs.c +++ b/security/safesetid/securityfs.c @@ -11,25 +11,15 @@ * published by the Free Software Foundation. * */ + +#define pr_fmt(fmt) "SafeSetID: " fmt + #include #include #include "lsm.h" -static struct dentry *safesetid_policy_dir; - -struct safesetid_file_entry { - const char *name; - enum safesetid_whitelist_file_write_type type; - struct dentry *dentry; -}; - -static struct safesetid_file_entry safesetid_files[] = { - {.name = "add_whitelist_policy", - .type = SAFESETID_WHITELIST_ADD}, - {.name = "flush_whitelist_policies", - .type = SAFESETID_WHITELIST_FLUSH}, -}; +static DEFINE_SPINLOCK(policy_update_lock); /* * In the case the input buffer contains one or more invalid UIDs, the kuid_t @@ -37,8 +27,8 @@ static struct safesetid_file_entry safesetid_files[] = { * function will return an error. * Contents of @buf may be modified. */ -static int parse_policy_line( - struct file *file, char *buf, kuid_t *parent, kuid_t *child) +static int parse_policy_line(struct file *file, char *buf, + struct setuid_rule *rule) { char *child_str; int ret; @@ -59,26 +49,103 @@ static int parse_policy_line( if (ret) return ret; - *parent = make_kuid(file->f_cred->user_ns, parsed_parent); - *child = make_kuid(file->f_cred->user_ns, parsed_child); - if (!uid_valid(*parent) || !uid_valid(*child)) + rule->src_uid = make_kuid(file->f_cred->user_ns, parsed_parent); + rule->dst_uid = make_kuid(file->f_cred->user_ns, parsed_child); + if (!uid_valid(rule->src_uid) || !uid_valid(rule->dst_uid)) return -EINVAL; return 0; } -static int parse_safesetid_whitelist_policy( - struct file *file, const char __user *buf, size_t len, - kuid_t *parent, kuid_t *child) +static void __release_ruleset(struct rcu_head *rcu) { - char *kern_buf = memdup_user_nul(buf, len); - int ret; + struct setuid_ruleset *pol = + container_of(rcu, struct setuid_ruleset, rcu); + int bucket; + struct setuid_rule *rule; + struct hlist_node *tmp; + + hash_for_each_safe(pol->rules, bucket, tmp, rule, next) + kfree(rule); + kfree(pol); +} - if (IS_ERR(kern_buf)) - return PTR_ERR(kern_buf); - ret = parse_policy_line(file, kern_buf, parent, child); - kfree(kern_buf); - return ret; +static void release_ruleset(struct setuid_ruleset *pol) +{ + call_rcu(&pol->rcu, __release_ruleset); +} + +static ssize_t handle_policy_update(struct file *file, + const char __user *ubuf, size_t len) +{ + struct setuid_ruleset *pol; + char *buf, *p, *end; + int err; + + pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL); + if (!pol) + return -ENOMEM; + hash_init(pol->rules); + + p = buf = memdup_user_nul(ubuf, len); + if (IS_ERR(buf)) { + err = PTR_ERR(buf); + goto out_free_pol; + } + + /* policy lines, including the last one, end with \n */ + while (*p != '\0') { + struct setuid_rule *rule; + + end = strchr(p, '\n'); + if (end == NULL) { + err = -EINVAL; + goto out_free_buf; + } + *end = '\0'; + + rule = kmalloc(sizeof(struct setuid_rule), GFP_KERNEL); + if (!rule) { + err = -ENOMEM; + goto out_free_buf; + } + + err = parse_policy_line(file, p, rule); + if (err) + goto out_free_rule; + + if (_setuid_policy_lookup(pol, rule->src_uid, rule->dst_uid) == + SIDPOL_ALLOWED) { + pr_warn("bad policy: duplicate entry\n"); + err = -EEXIST; + goto out_free_rule; + } + + hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid)); + p = end + 1; + continue; + +out_free_rule: + kfree(rule); + goto out_free_buf; + } + + /* + * Everything looks good, apply the policy and release the old one. + * What we really want here is an xchg() wrapper for RCU, but since that + * doesn't currently exist, just use a spinlock for now. + */ + spin_lock(&policy_update_lock); + rcu_swap_protected(safesetid_setuid_rules, pol, + lockdep_is_held(&policy_update_lock)); + spin_unlock(&policy_update_lock); + err = len; + +out_free_buf: + kfree(buf); +out_free_pol: + release_ruleset(pol); + return err; } static ssize_t safesetid_file_write(struct file *file, @@ -86,90 +153,45 @@ static ssize_t safesetid_file_write(struct file *file, size_t len, loff_t *ppos) { - struct safesetid_file_entry *file_entry = - file->f_inode->i_private; - kuid_t parent; - kuid_t child; - int ret; - if (!file_ns_capable(file, &init_user_ns, CAP_MAC_ADMIN)) return -EPERM; if (*ppos != 0) return -EINVAL; - switch (file_entry->type) { - case SAFESETID_WHITELIST_FLUSH: - flush_safesetid_whitelist_entries(); - break; - case SAFESETID_WHITELIST_ADD: - ret = parse_safesetid_whitelist_policy(file, buf, len, - &parent, &child); - if (ret) - return ret; - - ret = add_safesetid_whitelist_entry(parent, child); - if (ret) - return ret; - break; - default: - pr_warn("Unknown securityfs file %d\n", file_entry->type); - break; - } - - /* Return len on success so caller won't keep trying to write */ - return len; + return handle_policy_update(file, buf, len); } static const struct file_operations safesetid_file_fops = { .write = safesetid_file_write, }; -static void safesetid_shutdown_securityfs(void) -{ - int i; - - for (i = 0; i < ARRAY_SIZE(safesetid_files); ++i) { - struct safesetid_file_entry *entry = - &safesetid_files[i]; - securityfs_remove(entry->dentry); - entry->dentry = NULL; - } - - securityfs_remove(safesetid_policy_dir); - safesetid_policy_dir = NULL; -} - static int __init safesetid_init_securityfs(void) { - int i; int ret; + struct dentry *policy_dir; + struct dentry *policy_file; if (!safesetid_initialized) return 0; - safesetid_policy_dir = securityfs_create_dir("safesetid", NULL); - if (IS_ERR(safesetid_policy_dir)) { - ret = PTR_ERR(safesetid_policy_dir); + policy_dir = securityfs_create_dir("safesetid", NULL); + if (IS_ERR(policy_dir)) { + ret = PTR_ERR(policy_dir); goto error; } - for (i = 0; i < ARRAY_SIZE(safesetid_files); ++i) { - struct safesetid_file_entry *entry = - &safesetid_files[i]; - entry->dentry = securityfs_create_file( - entry->name, 0200, safesetid_policy_dir, - entry, &safesetid_file_fops); - if (IS_ERR(entry->dentry)) { - ret = PTR_ERR(entry->dentry); - goto error; - } + policy_file = securityfs_create_file("whitelist_policy", 0200, + policy_dir, NULL, &safesetid_file_fops); + if (IS_ERR(policy_file)) { + ret = PTR_ERR(policy_file); + goto error; } return 0; error: - safesetid_shutdown_securityfs(); + securityfs_remove(policy_dir); return ret; } fs_initcall(safesetid_init_securityfs); diff --git a/tools/testing/selftests/safesetid/safesetid-test.c b/tools/testing/selftests/safesetid/safesetid-test.c index 892c8e8b1b8b..4f03813d1911 100644 --- a/tools/testing/selftests/safesetid/safesetid-test.c +++ b/tools/testing/selftests/safesetid/safesetid-test.c @@ -142,23 +142,17 @@ static void ensure_securityfs_mounted(void) static void write_policies(void) { + static char *policy_str = + "1:2\n" + "1:3\n"; ssize_t written; int fd; fd = open(add_whitelist_policy_file, O_WRONLY); if (fd < 0) die("cant open add_whitelist_policy file\n"); - written = write(fd, "1:2", strlen("1:2")); - if (written != strlen("1:2")) { - if (written >= 0) { - die("short write to %s\n", add_whitelist_policy_file); - } else { - die("write to %s failed: %s\n", - add_whitelist_policy_file, strerror(errno)); - } - } - written = write(fd, "1:3", strlen("1:3")); - if (written != strlen("1:3")) { + written = write(fd, policy_str, strlen(policy_str)); + if (written != strlen(policy_str)) { if (written >= 0) { die("short write to %s\n", add_whitelist_policy_file); } else { -- cgit v1.2.3 From fbd9acb2dc2aa55902c48a83f157082849209fba Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Thu, 11 Apr 2019 13:11:54 -0700 Subject: LSM: SafeSetID: add read handler For debugging a running system, it is very helpful to be able to see what policy the system is using. Add a read handler that can dump out a copy of the loaded policy. Signed-off-by: Jann Horn Signed-off-by: Micah Morton --- security/safesetid/lsm.h | 1 + security/safesetid/securityfs.c | 35 +++++++++++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h index 4a34f558d964..db6d16e6bbc3 100644 --- a/security/safesetid/lsm.h +++ b/security/safesetid/lsm.h @@ -41,6 +41,7 @@ struct setuid_rule { struct setuid_ruleset { DECLARE_HASHTABLE(rules, SETID_HASH_BITS); + char *policy_str; struct rcu_head rcu; }; diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c index 250d59e046c1..997b403c6255 100644 --- a/security/safesetid/securityfs.c +++ b/security/safesetid/securityfs.c @@ -19,7 +19,7 @@ #include "lsm.h" -static DEFINE_SPINLOCK(policy_update_lock); +static DEFINE_MUTEX(policy_update_lock); /* * In the case the input buffer contains one or more invalid UIDs, the kuid_t @@ -67,6 +67,7 @@ static void __release_ruleset(struct rcu_head *rcu) hash_for_each_safe(pol->rules, bucket, tmp, rule, next) kfree(rule); + kfree(pol->policy_str); kfree(pol); } @@ -85,6 +86,7 @@ static ssize_t handle_policy_update(struct file *file, pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL); if (!pol) return -ENOMEM; + pol->policy_str = NULL; hash_init(pol->rules); p = buf = memdup_user_nul(ubuf, len); @@ -92,6 +94,11 @@ static ssize_t handle_policy_update(struct file *file, err = PTR_ERR(buf); goto out_free_pol; } + pol->policy_str = kstrdup(buf, GFP_KERNEL); + if (pol->policy_str == NULL) { + err = -ENOMEM; + goto out_free_buf; + } /* policy lines, including the last one, end with \n */ while (*p != '\0') { @@ -135,10 +142,10 @@ out_free_rule: * What we really want here is an xchg() wrapper for RCU, but since that * doesn't currently exist, just use a spinlock for now. */ - spin_lock(&policy_update_lock); + mutex_lock(&policy_update_lock); rcu_swap_protected(safesetid_setuid_rules, pol, lockdep_is_held(&policy_update_lock)); - spin_unlock(&policy_update_lock); + mutex_unlock(&policy_update_lock); err = len; out_free_buf: @@ -162,7 +169,27 @@ static ssize_t safesetid_file_write(struct file *file, return handle_policy_update(file, buf, len); } +static ssize_t safesetid_file_read(struct file *file, char __user *buf, + size_t len, loff_t *ppos) +{ + ssize_t res = 0; + struct setuid_ruleset *pol; + const char *kbuf; + + mutex_lock(&policy_update_lock); + pol = rcu_dereference_protected(safesetid_setuid_rules, + lockdep_is_held(&policy_update_lock)); + if (pol) { + kbuf = pol->policy_str; + res = simple_read_from_buffer(buf, len, ppos, + kbuf, strlen(kbuf)); + } + mutex_unlock(&policy_update_lock); + return res; +} + static const struct file_operations safesetid_file_fops = { + .read = safesetid_file_read, .write = safesetid_file_write, }; @@ -181,7 +208,7 @@ static int __init safesetid_init_securityfs(void) goto error; } - policy_file = securityfs_create_file("whitelist_policy", 0200, + policy_file = securityfs_create_file("whitelist_policy", 0600, policy_dir, NULL, &safesetid_file_fops); if (IS_ERR(policy_file)) { ret = PTR_ERR(policy_file); -- cgit v1.2.3 From 4f72123da579655855301b591535a1415224f123 Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Thu, 11 Apr 2019 13:12:43 -0700 Subject: LSM: SafeSetID: verify transitive constrainedness Someone might write a ruleset like the following, expecting that it securely constrains UID 1 to UIDs 1, 2 and 3: 1:2 1:3 However, because no constraints are applied to UIDs 2 and 3, an attacker with UID 1 can simply first switch to UID 2, then switch to any UID from there. The secure way to write this ruleset would be: 1:2 1:3 2:2 3:3 , which uses "transition to self" as a way to inhibit the default-allow policy without allowing anything specific. This is somewhat unintuitive. To make sure that policy authors don't accidentally write insecure policies because of this, let the kernel verify that a new ruleset does not contain any entries that are constrained, but transitively unconstrained. Signed-off-by: Jann Horn Signed-off-by: Micah Morton --- security/safesetid/securityfs.c | 38 +++++++++++++++++++++- tools/testing/selftests/safesetid/safesetid-test.c | 4 ++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c index 997b403c6255..d568e17dd773 100644 --- a/security/safesetid/securityfs.c +++ b/security/safesetid/securityfs.c @@ -76,6 +76,37 @@ static void release_ruleset(struct setuid_ruleset *pol) call_rcu(&pol->rcu, __release_ruleset); } +static void insert_rule(struct setuid_ruleset *pol, struct setuid_rule *rule) +{ + hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid)); +} + +static int verify_ruleset(struct setuid_ruleset *pol) +{ + int bucket; + struct setuid_rule *rule, *nrule; + int res = 0; + + hash_for_each(pol->rules, bucket, rule, next) { + if (_setuid_policy_lookup(pol, rule->dst_uid, INVALID_UID) == + SIDPOL_DEFAULT) { + pr_warn("insecure policy detected: uid %d is constrained but transitively unconstrained through uid %d\n", + __kuid_val(rule->src_uid), + __kuid_val(rule->dst_uid)); + res = -EINVAL; + + /* fix it up */ + nrule = kmalloc(sizeof(struct setuid_rule), GFP_KERNEL); + if (!nrule) + return -ENOMEM; + nrule->src_uid = rule->dst_uid; + nrule->dst_uid = rule->dst_uid; + insert_rule(pol, nrule); + } + } + return res; +} + static ssize_t handle_policy_update(struct file *file, const char __user *ubuf, size_t len) { @@ -128,7 +159,7 @@ static ssize_t handle_policy_update(struct file *file, goto out_free_rule; } - hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid)); + insert_rule(pol, rule); p = end + 1; continue; @@ -137,6 +168,11 @@ out_free_rule: goto out_free_buf; } + err = verify_ruleset(pol); + /* bogus policy falls through after fixing it up */ + if (err && err != -EINVAL) + goto out_free_buf; + /* * Everything looks good, apply the policy and release the old one. * What we really want here is an xchg() wrapper for RCU, but since that diff --git a/tools/testing/selftests/safesetid/safesetid-test.c b/tools/testing/selftests/safesetid/safesetid-test.c index 4f03813d1911..8f40c6ecdad1 100644 --- a/tools/testing/selftests/safesetid/safesetid-test.c +++ b/tools/testing/selftests/safesetid/safesetid-test.c @@ -144,7 +144,9 @@ static void write_policies(void) { static char *policy_str = "1:2\n" - "1:3\n"; + "1:3\n" + "2:2\n" + "3:3\n"; ssize_t written; int fd; -- cgit v1.2.3 From e10337daefecb47209fd2af5f4fab0d1a370737f Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Wed, 10 Apr 2019 09:56:27 -0700 Subject: LSM: SafeSetID: fix use of literal -1 in capable hook The capable() hook returns an error number. -EPERM is actually the same as -1, so this doesn't make a difference in behavior. Signed-off-by: Jann Horn Signed-off-by: Micah Morton --- security/safesetid/lsm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c index 22964e2a6187..7760019ad35d 100644 --- a/security/safesetid/lsm.c +++ b/security/safesetid/lsm.c @@ -90,7 +90,7 @@ static int safesetid_security_capable(const struct cred *cred, */ pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n", __kuid_val(cred->uid)); - return -1; + return -EPERM; } /* -- cgit v1.2.3