* [PATCH v5 1/2] LSM: SafeSetID: gate setgid transitions
@ 2019-03-05 15:49 mortonm
2019-03-05 15:52 ` [PATCH v4 2/2] " mortonm
2019-03-29 18:06 ` [PATCH v5 1/2] " James Morris
0 siblings, 2 replies; 10+ messages in thread
From: mortonm @ 2019-03-05 15:49 UTC (permalink / raw)
To: jmorris, serge, keescook, casey, sds, linux-security-module; +Cc: Micah Morton
From: Micah Morton <mortonm@chromium.org>
This patch generalizes the 'task_fix_setuid' LSM hook to enable hooking
setgid transitions as well as setuid transitions. The hook is renamed to
'task_fix_setid'. The patch introduces calls to this hook from the
setgid functions in kernel/sys.c. This will allow the SafeSetID LSM to
govern setgid transitions in addition to setuid transitions. This patch
also makes sure the setgid functions in kernel/sys.c call
security_capable_setid rather than the ordinary security_capable
function, so that the security_capable hook in the SafeSetID LSM knows
it is being invoked from a setid function.
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
Changes since the last patch: Only one break is needed for the four
LSM_SETGID_* cases in security/commoncap.c.
Documentation/admin-guide/LSM/SafeSetID.rst | 2 +-
include/linux/lsm_hooks.h | 8 ++---
include/linux/security.h | 36 ++++++++++++++-------
kernel/sys.c | 35 ++++++++++++++------
security/commoncap.c | 22 ++++++++-----
security/safesetid/lsm.c | 12 +++----
security/security.c | 4 +--
7 files changed, 76 insertions(+), 43 deletions(-)
diff --git a/Documentation/admin-guide/LSM/SafeSetID.rst b/Documentation/admin-guide/LSM/SafeSetID.rst
index 212434ef65ad..670a6544fd39 100644
--- a/Documentation/admin-guide/LSM/SafeSetID.rst
+++ b/Documentation/admin-guide/LSM/SafeSetID.rst
@@ -88,7 +88,7 @@ other system interactions, including use of pid namespaces and device creation.
Use an existing LSM
-------------------
None of the other in-tree LSMs have the capability to gate setid transitions, or
-even employ the security_task_fix_setuid hook at all. SELinux says of that hook:
+even employ the security_task_fix_setid hook at all. SELinux says of that hook:
"Since setuid only affects the current process, and since the SELinux controls
are not based on the Linux identity attributes, SELinux does not need to control
this operation."
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 22fc786d723a..47fd04410fde 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -594,14 +594,14 @@
* @size length of the file contents.
* @id kernel read file identifier
* Return 0 if permission is granted.
- * @task_fix_setuid:
+ * @task_fix_setid:
* Update the module's state after setting one or more of the user
* identity attributes of the current process. The @flags parameter
* indicates which of the set*uid system calls invoked this hook. If
* @new is the set of credentials that will be installed. Modifications
* should be made to this rather than to @current->cred.
* @old is the set of credentials that are being replaces
- * @flags contains one of the LSM_SETID_* values.
+ * @flags contains one of the LSM_SET*ID_* values.
* Return 0 on success.
* @task_setpgid:
* Check permission before setting the process group identifier of the
@@ -1594,7 +1594,7 @@ union security_list_options {
int (*kernel_read_file)(struct file *file, enum kernel_read_file_id id);
int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size,
enum kernel_read_file_id id);
- int (*task_fix_setuid)(struct cred *new, const struct cred *old,
+ int (*task_fix_setid)(struct cred *new, const struct cred *old,
int flags);
int (*task_setpgid)(struct task_struct *p, pid_t pgid);
int (*task_getpgid)(struct task_struct *p);
@@ -1886,7 +1886,7 @@ struct security_hook_heads {
struct hlist_head kernel_read_file;
struct hlist_head kernel_post_read_file;
struct hlist_head kernel_module_request;
- struct hlist_head task_fix_setuid;
+ struct hlist_head task_fix_setid;
struct hlist_head task_setpgid;
struct hlist_head task_getpgid;
struct hlist_head task_getsid;
diff --git a/include/linux/security.h b/include/linux/security.h
index 13537a49ae97..76df3e22fed1 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -95,7 +95,7 @@ extern int cap_inode_getsecurity(struct inode *inode, const char *name,
extern int cap_mmap_addr(unsigned long addr);
extern int cap_mmap_file(struct file *file, unsigned long reqprot,
unsigned long prot, unsigned long flags);
-extern int cap_task_fix_setuid(struct cred *new, const struct cred *old, int flags);
+extern int cap_task_fix_setid(struct cred *new, const struct cred *old, int flags);
extern int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
unsigned long arg4, unsigned long arg5);
extern int cap_task_setscheduler(struct task_struct *p);
@@ -128,17 +128,29 @@ extern unsigned long dac_mmap_min_addr;
/*
* Values used in the task_security_ops calls
*/
-/* setuid or setgid, id0 == uid or gid */
-#define LSM_SETID_ID 1
+/* setuid, id0 == uid */
+#define LSM_SETUID_ID 1
-/* setreuid or setregid, id0 == real, id1 == eff */
-#define LSM_SETID_RE 2
+/* setreuid, id0 == real, id1 == eff */
+#define LSM_SETUID_RE 2
-/* setresuid or setresgid, id0 == real, id1 == eff, uid2 == saved */
-#define LSM_SETID_RES 4
+/* setresuid, id0 == real, id1 == eff, uid2 == saved */
+#define LSM_SETUID_RES 4
-/* setfsuid or setfsgid, id0 == fsuid or fsgid */
-#define LSM_SETID_FS 8
+/* setfsuid, id0 == fsgid */
+#define LSM_SETUID_FS 8
+
+/* setgid, id0 == gid */
+#define LSM_SETGID_ID 16
+
+/* setregid, id0 == real, id1 == eff */
+#define LSM_SETGID_RE 32
+
+/* setresgid, id0 == real, id1 == eff, uid2 == saved */
+#define LSM_SETGID_RES 64
+
+/* setfsgid, id0 == fsgid */
+#define LSM_SETGID_FS 128
/* Flags for security_task_prlimit(). */
#define LSM_PRLIMIT_READ 1
@@ -324,7 +336,7 @@ int security_kernel_load_data(enum kernel_load_data_id id);
int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
enum kernel_read_file_id id);
-int security_task_fix_setuid(struct cred *new, const struct cred *old,
+int security_task_fix_setid(struct cred *new, const struct cred *old,
int flags);
int security_task_setpgid(struct task_struct *p, pid_t pgid);
int security_task_getpgid(struct task_struct *p);
@@ -923,11 +935,11 @@ static inline int security_kernel_post_read_file(struct file *file,
return 0;
}
-static inline int security_task_fix_setuid(struct cred *new,
+static inline int security_task_fix_setid(struct cred *new,
const struct cred *old,
int flags)
{
- return cap_task_fix_setuid(new, old, flags);
+ return cap_task_fix_setid(new, old, flags);
}
static inline int security_task_setpgid(struct task_struct *p, pid_t pgid)
diff --git a/kernel/sys.c b/kernel/sys.c
index c5f875048aef..615b44939238 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -372,7 +372,7 @@ long __sys_setregid(gid_t rgid, gid_t egid)
if (rgid != (gid_t) -1) {
if (gid_eq(old->gid, krgid) ||
gid_eq(old->egid, krgid) ||
- ns_capable(old->user_ns, CAP_SETGID))
+ ns_capable_setid(old->user_ns, CAP_SETGID))
new->gid = krgid;
else
goto error;
@@ -381,7 +381,7 @@ long __sys_setregid(gid_t rgid, gid_t egid)
if (gid_eq(old->gid, kegid) ||
gid_eq(old->egid, kegid) ||
gid_eq(old->sgid, kegid) ||
- ns_capable(old->user_ns, CAP_SETGID))
+ ns_capable_setid(old->user_ns, CAP_SETGID))
new->egid = kegid;
else
goto error;
@@ -392,6 +392,10 @@ long __sys_setregid(gid_t rgid, gid_t egid)
new->sgid = new->egid;
new->fsgid = new->egid;
+ retval = security_task_fix_setid(new, old, LSM_SETGID_RE);
+ if (retval < 0)
+ goto error;
+
return commit_creds(new);
error:
@@ -427,13 +431,17 @@ long __sys_setgid(gid_t gid)
old = current_cred();
retval = -EPERM;
- if (ns_capable(old->user_ns, CAP_SETGID))
+ if (ns_capable_setid(old->user_ns, CAP_SETGID))
new->gid = new->egid = new->sgid = new->fsgid = kgid;
else if (gid_eq(kgid, old->gid) || gid_eq(kgid, old->sgid))
new->egid = new->fsgid = kgid;
else
goto error;
+ retval = security_task_fix_setid(new, old, LSM_SETGID_ID);
+ if (retval < 0)
+ goto error;
+
return commit_creds(new);
error:
@@ -539,7 +547,7 @@ long __sys_setreuid(uid_t ruid, uid_t euid)
new->suid = new->euid;
new->fsuid = new->euid;
- retval = security_task_fix_setuid(new, old, LSM_SETID_RE);
+ retval = security_task_fix_setid(new, old, LSM_SETUID_RE);
if (retval < 0)
goto error;
@@ -597,7 +605,7 @@ long __sys_setuid(uid_t uid)
new->fsuid = new->euid = kuid;
- retval = security_task_fix_setuid(new, old, LSM_SETID_ID);
+ retval = security_task_fix_setid(new, old, LSM_SETUID_ID);
if (retval < 0)
goto error;
@@ -672,7 +680,7 @@ long __sys_setresuid(uid_t ruid, uid_t euid, uid_t suid)
new->suid = ksuid;
new->fsuid = new->euid;
- retval = security_task_fix_setuid(new, old, LSM_SETID_RES);
+ retval = security_task_fix_setid(new, old, LSM_SETUID_RES);
if (retval < 0)
goto error;
@@ -735,7 +743,7 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid)
old = current_cred();
retval = -EPERM;
- if (!ns_capable(old->user_ns, CAP_SETGID)) {
+ if (!ns_capable_setid(old->user_ns, CAP_SETGID)) {
if (rgid != (gid_t) -1 && !gid_eq(krgid, old->gid) &&
!gid_eq(krgid, old->egid) && !gid_eq(krgid, old->sgid))
goto error;
@@ -755,6 +763,10 @@ long __sys_setresgid(gid_t rgid, gid_t egid, gid_t sgid)
new->sgid = ksgid;
new->fsgid = new->egid;
+ retval = security_task_fix_setid(new, old, LSM_SETGID_RES);
+ if (retval < 0)
+ goto error;
+
return commit_creds(new);
error:
@@ -817,7 +829,7 @@ long __sys_setfsuid(uid_t uid)
ns_capable_setid(old->user_ns, CAP_SETUID)) {
if (!uid_eq(kuid, old->fsuid)) {
new->fsuid = kuid;
- if (security_task_fix_setuid(new, old, LSM_SETID_FS) == 0)
+ if (security_task_fix_setid(new, old, LSM_SETUID_FS) == 0)
goto change_okay;
}
}
@@ -858,10 +870,13 @@ long __sys_setfsgid(gid_t gid)
if (gid_eq(kgid, old->gid) || gid_eq(kgid, old->egid) ||
gid_eq(kgid, old->sgid) || gid_eq(kgid, old->fsgid) ||
- ns_capable(old->user_ns, CAP_SETGID)) {
+ ns_capable_setid(old->user_ns, CAP_SETGID)) {
if (!gid_eq(kgid, old->fsgid)) {
new->fsgid = kgid;
- goto change_okay;
+ if (security_task_fix_setid(new,
+ old,
+ LSM_SETGID_FS) == 0)
+ goto change_okay;
}
}
diff --git a/security/commoncap.c b/security/commoncap.c
index f1d117c3d8ae..4a038d3c7196 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1026,27 +1026,27 @@ static inline void cap_emulate_setxuid(struct cred *new, const struct cred *old)
}
/**
- * cap_task_fix_setuid - Fix up the results of setuid() call
+ * cap_task_fix_setid - Fix up the results of setid() call
* @new: The proposed credentials
* @old: The current task's current credentials
* @flags: Indications of what has changed
*
- * Fix up the results of setuid() call before the credential changes are
+ * Fix up the results of setid() call before the credential changes are
* actually applied, returning 0 to grant the changes, -ve to deny them.
*/
-int cap_task_fix_setuid(struct cred *new, const struct cred *old, int flags)
+int cap_task_fix_setid(struct cred *new, const struct cred *old, int flags)
{
switch (flags) {
- case LSM_SETID_RE:
- case LSM_SETID_ID:
- case LSM_SETID_RES:
+ case LSM_SETUID_RE:
+ case LSM_SETUID_ID:
+ case LSM_SETUID_RES:
/* juggle the capabilities to follow [RES]UID changes unless
* otherwise suppressed */
if (!issecure(SECURE_NO_SETUID_FIXUP))
cap_emulate_setxuid(new, old);
break;
- case LSM_SETID_FS:
+ case LSM_SETUID_FS:
/* juggle the capabilties to follow FSUID changes, unless
* otherwise suppressed
*
@@ -1066,6 +1066,12 @@ int cap_task_fix_setuid(struct cred *new, const struct cred *old, int flags)
}
break;
+ case LSM_SETGID_RE:
+ case LSM_SETGID_ID:
+ case LSM_SETGID_RES:
+ case LSM_SETGID_FS:
+ break;
+
default:
return -EINVAL;
}
@@ -1355,7 +1361,7 @@ struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(inode_getsecurity, cap_inode_getsecurity),
LSM_HOOK_INIT(mmap_addr, cap_mmap_addr),
LSM_HOOK_INIT(mmap_file, cap_mmap_file),
- LSM_HOOK_INIT(task_fix_setuid, cap_task_fix_setuid),
+ LSM_HOOK_INIT(task_fix_setid, cap_task_fix_setid),
LSM_HOOK_INIT(task_prctl, cap_task_prctl),
LSM_HOOK_INIT(task_setscheduler, cap_task_setscheduler),
LSM_HOOK_INIT(task_setioprio, cap_task_setioprio),
diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
index cecd38e2ac80..5deffa92f25f 100644
--- a/security/safesetid/lsm.c
+++ b/security/safesetid/lsm.c
@@ -120,7 +120,7 @@ static int check_uid_transition(kuid_t parent, kuid_t child)
* set*uid to user under new cred struct, or the UID transition is allowed (by
* Linux set*uid rules) even without CAP_SETUID.
*/
-static int safesetid_task_fix_setuid(struct cred *new,
+static int safesetid_task_fix_setid(struct cred *new,
const struct cred *old,
int flags)
{
@@ -130,7 +130,7 @@ static int safesetid_task_fix_setuid(struct cred *new,
return 0;
switch (flags) {
- case LSM_SETID_RE:
+ case LSM_SETUID_RE:
/*
* Users for which setuid restrictions exist can only set the
* real UID to the real UID or the effective UID, unless an
@@ -152,7 +152,7 @@ static int safesetid_task_fix_setuid(struct cred *new,
return check_uid_transition(old->euid, new->euid);
}
break;
- case LSM_SETID_ID:
+ case LSM_SETUID_ID:
/*
* Users for which setuid restrictions exist cannot change the
* real UID or saved set-UID unless an explicit whitelist
@@ -163,7 +163,7 @@ static int safesetid_task_fix_setuid(struct cred *new,
if (!uid_eq(old->suid, new->suid))
return check_uid_transition(old->suid, new->suid);
break;
- case LSM_SETID_RES:
+ case LSM_SETUID_RES:
/*
* Users for which setuid restrictions exist cannot change the
* real UID, effective UID, or saved set-UID to anything but
@@ -187,7 +187,7 @@ static int safesetid_task_fix_setuid(struct cred *new,
return check_uid_transition(old->suid, new->suid);
}
break;
- case LSM_SETID_FS:
+ case LSM_SETUID_FS:
/*
* Users for which setuid restrictions exist cannot change the
* filesystem UID to anything but one of: the current real UID,
@@ -256,7 +256,7 @@ void flush_safesetid_whitelist_entries(void)
}
static struct security_hook_list safesetid_security_hooks[] = {
- LSM_HOOK_INIT(task_fix_setuid, safesetid_task_fix_setuid),
+ LSM_HOOK_INIT(task_fix_setid, safesetid_task_fix_setid),
LSM_HOOK_INIT(capable, safesetid_security_capable)
};
diff --git a/security/security.c b/security/security.c
index ed9b8cbf21cf..450784fd1d2b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1568,10 +1568,10 @@ int security_kernel_load_data(enum kernel_load_data_id id)
}
EXPORT_SYMBOL_GPL(security_kernel_load_data);
-int security_task_fix_setuid(struct cred *new, const struct cred *old,
+int security_task_fix_setid(struct cred *new, const struct cred *old,
int flags)
{
- return call_int_hook(task_fix_setuid, 0, new, old, flags);
+ return call_int_hook(task_fix_setid, 0, new, old, flags);
}
int security_task_setpgid(struct task_struct *p, pid_t pgid)
--
2.21.0.352.gf09ad66450-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 2/2] LSM: SafeSetID: gate setgid transitions
2019-03-05 15:49 [PATCH v5 1/2] LSM: SafeSetID: gate setgid transitions mortonm
@ 2019-03-05 15:52 ` mortonm
2019-03-11 17:25 ` Micah Morton
2019-03-29 18:06 ` [PATCH v5 1/2] " James Morris
1 sibling, 1 reply; 10+ messages in thread
From: mortonm @ 2019-03-05 15:52 UTC (permalink / raw)
To: jmorris, serge, keescook, casey, sds, linux-security-module; +Cc: Micah Morton
From: Micah Morton <mortonm@chromium.org>
The SafeSetID LSM already gates setuid transitions for UIDs on the
system whose use of CAP_SETUID has been 'restricted'. This patch
implements the analogous functionality for setgid transitions, in order
to restrict the use of CAP_SETGID for certain UIDs on the system. One
notable consequence of this addition is that a process running under a
restricted UID (i.e. one that is only allowed to setgid to certain
approved GIDs) will not be allowed to call the setgroups() syscall to
set its supplementary group IDs. For now, we leave such support for
restricted setgroups() to future work, as it would require hooking the
logic in setgroups() and verifying that the array of GIDs passed in from
userspace only consists of approved GIDs.
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
Changes since the last patch: Minor updates since patch 1/2 combined the
'task_fix_setuid' and 'task_fix_setgid' hooks into one hook called
'task_fix_setid'.
Documentation/admin-guide/LSM/SafeSetID.rst | 9 +-
security/safesetid/lsm.c | 272 +++++++++++++++++---
security/safesetid/lsm.h | 11 +-
security/safesetid/securityfs.c | 105 +++++---
4 files changed, 320 insertions(+), 77 deletions(-)
diff --git a/Documentation/admin-guide/LSM/SafeSetID.rst b/Documentation/admin-guide/LSM/SafeSetID.rst
index 670a6544fd39..33307a8e9555 100644
--- a/Documentation/admin-guide/LSM/SafeSetID.rst
+++ b/Documentation/admin-guide/LSM/SafeSetID.rst
@@ -98,10 +98,13 @@ Directions for use
==================
This LSM hooks the setid syscalls to make sure transitions are allowed if an
applicable restriction policy is in place. Policies are configured through
-securityfs by writing to the safesetid/add_whitelist_policy and
+securityfs by writing to the safesetid/add_whitelist_{uid/gid}_policy* and
safesetid/flush_whitelist_policies files at the location where securityfs is
-mounted. The format for adding a policy is '<UID>:<UID>', using literal
+mounted. The format for adding a policy is '<ID>:<ID>', using literal
numbers, such as '123:456'. To flush the policies, any write to the file is
sufficient. Again, configuring a policy for a UID will prevent that UID from
obtaining auxiliary setid privileges, such as allowing a user to set up user
-namespace UID mappings.
+namespace ID mappings.
+
+*safesetid/add_whitelist_policy can also be used for UID policies, since it is
+an alias for safesetid/add_whitelist_uid_policy
diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
index 5deffa92f25f..95c6f35b29f3 100644
--- a/security/safesetid/lsm.c
+++ b/security/safesetid/lsm.c
@@ -26,27 +26,30 @@ int safesetid_initialized;
#define NUM_BITS 8 /* 128 buckets in hash table */
-static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS);
+static DEFINE_HASHTABLE(safesetid_whitelist_uid_hashtable, NUM_BITS);
+static DEFINE_HASHTABLE(safesetid_whitelist_gid_hashtable, NUM_BITS);
+
+static DEFINE_SPINLOCK(safesetid_whitelist_uid_hashtable_spinlock);
+static DEFINE_SPINLOCK(safesetid_whitelist_gid_hashtable_spinlock);
/*
* Hash table entry to store safesetid policy signifying that 'parent' user
- * can setid to 'child' user.
+ * can setid to 'child' user. This struct is used in both the uid and gid
+ * hashtables.
*/
-struct entry {
+struct id_entry {
struct hlist_node next;
struct hlist_node dlist; /* for deletion cleanup */
uint64_t parent_kuid;
- uint64_t child_kuid;
+ uint64_t child_kid; /* Represents either a UID or a GID */
};
-static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock);
-
static bool check_setuid_policy_hashtable_key(kuid_t parent)
{
- struct entry *entry;
+ struct id_entry *entry;
rcu_read_lock();
- hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
+ hash_for_each_possible_rcu(safesetid_whitelist_uid_hashtable,
entry, next, __kuid_val(parent)) {
if (entry->parent_kuid == __kuid_val(parent)) {
rcu_read_unlock();
@@ -61,13 +64,49 @@ static bool check_setuid_policy_hashtable_key(kuid_t parent)
static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
kuid_t child)
{
- struct entry *entry;
+ struct id_entry *entry;
+
+ rcu_read_lock();
+ hash_for_each_possible_rcu(safesetid_whitelist_uid_hashtable,
+ entry, next, __kuid_val(parent)) {
+ if (entry->parent_kuid == __kuid_val(parent) &&
+ entry->child_kid == __kuid_val(child)) {
+ rcu_read_unlock();
+ return true;
+ }
+ }
+ rcu_read_unlock();
+
+ return false;
+}
+
+static bool check_setgid_policy_hashtable_key(kuid_t parent)
+{
+ struct id_entry *entry;
+
+ rcu_read_lock();
+ hash_for_each_possible_rcu(safesetid_whitelist_gid_hashtable,
+ entry, next, __kuid_val(parent)) {
+ if (entry->parent_kuid == __kuid_val(parent)) {
+ rcu_read_unlock();
+ return true;
+ }
+ }
+ rcu_read_unlock();
+
+ return false;
+}
+
+static bool check_setgid_policy_hashtable_key_value(kuid_t parent,
+ kgid_t child)
+{
+ struct id_entry *entry;
rcu_read_lock();
- hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
+ hash_for_each_possible_rcu(safesetid_whitelist_gid_hashtable,
entry, next, __kuid_val(parent)) {
if (entry->parent_kuid == __kuid_val(parent) &&
- entry->child_kuid == __kuid_val(child)) {
+ entry->child_kid == __kgid_val(child)) {
rcu_read_unlock();
return true;
}
@@ -77,6 +116,12 @@ static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
return false;
}
+/*
+ * This hook causes the security_capable check to fail when there are
+ * restriction policies for a UID and the process is trying to do something
+ * (other than a setid transition) that is gated by CAP_SETUID/CAP_SETGID
+ * (e.g. allowing user to set up userns UID/GID mappings).
+ */
static int safesetid_security_capable(const struct cred *cred,
struct user_namespace *ns,
int cap,
@@ -85,17 +130,19 @@ static int safesetid_security_capable(const struct cred *cred,
if (cap == CAP_SETUID &&
check_setuid_policy_hashtable_key(cred->uid)) {
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",
__kuid_val(cred->uid));
return -1;
}
}
+ if (cap == CAP_SETGID &&
+ check_setgid_policy_hashtable_key(cred->uid)) {
+ if (!(opts & CAP_OPT_INSETID)) {
+ pr_warn("Operation requires CAP_SETGID, which is not available to UID %u for operations besides approved set*gid transitions",
+ __kuid_val(cred->uid));
+ return -1;
+ }
+ }
return 0;
}
@@ -115,19 +162,48 @@ static int check_uid_transition(kuid_t parent, kuid_t child)
return -EACCES;
}
+static int check_gid_transition(kuid_t parent, kgid_t child)
+{
+ if (check_setgid_policy_hashtable_key_value(parent, child))
+ return 0;
+ pr_warn("Denied UID %d setting GID to %d",
+ __kuid_val(parent),
+ __kgid_val(child));
+ /*
+ * 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, current);
+ return -EACCES;
+}
+
/*
* Check whether there is either an exception for user under old cred struct to
- * set*uid to user under new cred struct, or the UID transition is allowed (by
- * Linux set*uid rules) even without CAP_SETUID.
+ * set*id to user under new cred struct, or the ID transition is allowed (by
+ * Linux set*id rules) even without CAP_SET*ID.
*/
static int safesetid_task_fix_setid(struct cred *new,
const struct cred *old,
int flags)
{
+ if (flags == LSM_SETUID_RE ||
+ flags == LSM_SETUID_ID ||
+ flags == LSM_SETUID_RES ||
+ flags == LSM_SETUID_FS) {
+ /* Do nothing if there are no setuid restrictions for this UID. */
+ if (!check_setuid_policy_hashtable_key(old->uid))
+ return 0;
+ }
- /* Do nothing if there are no setuid restrictions for this UID. */
- if (!check_setuid_policy_hashtable_key(old->uid))
+ if (flags == LSM_SETGID_RE ||
+ flags == LSM_SETGID_ID ||
+ flags == LSM_SETGID_RES ||
+ flags == LSM_SETGID_FS) {
+ /* Do nothing if there are no setgid restrictions for this UID. */
+ if (!check_setgid_policy_hashtable_key(old->uid))
return 0;
+ }
switch (flags) {
case LSM_SETUID_RE:
@@ -201,6 +277,77 @@ static int safesetid_task_fix_setid(struct cred *new,
return check_uid_transition(old->fsuid, new->fsuid);
}
break;
+ case LSM_SETGID_RE:
+ /*
+ * Users for which setgid restrictions exist can only set the
+ * real GID to the real GID or the effective GID, unless an
+ * explicit whitelist policy allows the transition.
+ */
+ if (!gid_eq(old->gid, new->gid) &&
+ !gid_eq(old->egid, new->gid)) {
+ return check_gid_transition(old->uid, new->gid);
+ }
+ /*
+ * Users for which setgid restrictions exist can only set the
+ * effective GID to the real GID, the effective GID, or the
+ * saved set-GID, unless an explicit whitelist policy allows
+ * the transition.
+ */
+ if (!gid_eq(old->gid, new->egid) &&
+ !gid_eq(old->egid, new->egid) &&
+ !gid_eq(old->sgid, new->egid)) {
+ return check_gid_transition(old->euid, new->egid);
+ }
+ break;
+ case LSM_SETGID_ID:
+ /*
+ * Users for which setgid restrictions exist cannot change the
+ * real GID or saved set-GID unless an explicit whitelist
+ * policy allows the transition.
+ */
+ if (!gid_eq(old->gid, new->gid))
+ return check_gid_transition(old->uid, new->gid);
+ if (!gid_eq(old->sgid, new->sgid))
+ return check_gid_transition(old->suid, new->sgid);
+ break;
+ case LSM_SETGID_RES:
+ /*
+ * Users for which setgid restrictions exist cannot change the
+ * real GID, effective GID, or saved set-GID to anything but
+ * one of: the current real GID, the current effective GID or
+ * the current saved set-user-ID unless an explicit whitelist
+ * policy allows the transition.
+ */
+ if (!gid_eq(new->gid, old->gid) &&
+ !gid_eq(new->gid, old->egid) &&
+ !gid_eq(new->gid, old->sgid)) {
+ return check_gid_transition(old->uid, new->gid);
+ }
+ if (!gid_eq(new->egid, old->gid) &&
+ !gid_eq(new->egid, old->egid) &&
+ !gid_eq(new->egid, old->sgid)) {
+ return check_gid_transition(old->euid, new->egid);
+ }
+ if (!gid_eq(new->sgid, old->gid) &&
+ !gid_eq(new->sgid, old->egid) &&
+ !gid_eq(new->sgid, old->sgid)) {
+ return check_gid_transition(old->suid, new->sgid);
+ }
+ break;
+ case LSM_SETGID_FS:
+ /*
+ * Users for which setgid restrictions exist cannot change the
+ * filesystem GID to anything but one of: the current real GID,
+ * the current effective GID or the current saved set-GID
+ * unless an explicit whitelist policy allows the transition.
+ */
+ if (!gid_eq(new->fsgid, old->gid) &&
+ !gid_eq(new->fsgid, old->egid) &&
+ !gid_eq(new->fsgid, old->sgid) &&
+ !gid_eq(new->fsgid, old->fsgid)) {
+ return check_gid_transition(old->fsuid, new->fsgid);
+ }
+ break;
default:
pr_warn("Unknown setid state %d\n", flags);
force_sig(SIGKILL, current);
@@ -209,49 +356,96 @@ static int safesetid_task_fix_setid(struct cred *new,
return 0;
}
-int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
+int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child)
{
- struct entry *new;
+ struct id_entry *new;
/* Return if entry already exists */
if (check_setuid_policy_hashtable_key_value(parent, child))
return 0;
- new = kzalloc(sizeof(struct entry), GFP_KERNEL);
+ new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
+ if (!new)
+ return -ENOMEM;
+ new->parent_kuid = __kuid_val(parent);
+ new->child_kid = __kuid_val(child);
+ spin_lock(&safesetid_whitelist_uid_hashtable_spinlock);
+ /* Return if the entry got added since we checked above */
+ if (check_setuid_policy_hashtable_key_value(parent, child)) {
+ spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
+ kfree(new);
+ return 0;
+ }
+ hash_add_rcu(safesetid_whitelist_uid_hashtable,
+ &new->next,
+ __kuid_val(parent));
+ spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
+ return 0;
+}
+
+int add_safesetid_whitelist_gid_entry(kuid_t parent, kgid_t child)
+{
+ struct id_entry *new;
+
+ /* Return if entry already exists */
+ if (check_setgid_policy_hashtable_key_value(parent, child))
+ return 0;
+
+ new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
if (!new)
return -ENOMEM;
new->parent_kuid = __kuid_val(parent);
- new->child_kuid = __kuid_val(child);
- spin_lock(&safesetid_whitelist_hashtable_spinlock);
- hash_add_rcu(safesetid_whitelist_hashtable,
+ new->child_kid = __kgid_val(child);
+ spin_lock(&safesetid_whitelist_gid_hashtable_spinlock);
+ /* Return if the entry got added since we checked above */
+ if (check_setgid_policy_hashtable_key_value(parent, child)) {
+ spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
+ kfree(new);
+ return 0;
+ }
+ hash_add_rcu(safesetid_whitelist_gid_hashtable,
&new->next,
__kuid_val(parent));
- spin_unlock(&safesetid_whitelist_hashtable_spinlock);
+ spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
return 0;
}
void flush_safesetid_whitelist_entries(void)
{
- struct entry *entry;
+ struct id_entry *id_entry;
struct hlist_node *hlist_node;
unsigned int bkt_loop_cursor;
- HLIST_HEAD(free_list);
+ HLIST_HEAD(uid_free_list);
+ HLIST_HEAD(gid_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_lock(&safesetid_whitelist_uid_hashtable_spinlock);
+ hash_for_each_safe(safesetid_whitelist_uid_hashtable, bkt_loop_cursor,
+ hlist_node, id_entry, next) {
+ hash_del_rcu(&id_entry->next);
+ hlist_add_head(&id_entry->dlist, &uid_free_list);
+ }
+ spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
+ synchronize_rcu();
+ hlist_for_each_entry_safe(id_entry, hlist_node, &uid_free_list, dlist) {
+ hlist_del(&id_entry->dlist);
+ kfree(id_entry);
+ }
+
+ spin_lock(&safesetid_whitelist_gid_hashtable_spinlock);
+ hash_for_each_safe(safesetid_whitelist_gid_hashtable, bkt_loop_cursor,
+ hlist_node, id_entry, next) {
+ hash_del_rcu(&id_entry->next);
+ hlist_add_head(&id_entry->dlist, &gid_free_list);
}
- spin_unlock(&safesetid_whitelist_hashtable_spinlock);
+ spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
synchronize_rcu();
- hlist_for_each_entry_safe(entry, hlist_node, &free_list, dlist) {
- hlist_del(&entry->dlist);
- kfree(entry);
+ hlist_for_each_entry_safe(id_entry, hlist_node, &gid_free_list, dlist) {
+ hlist_del(&id_entry->dlist);
+ kfree(id_entry);
}
}
diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
index c1ea3c265fcf..e9ae192caff2 100644
--- a/security/safesetid/lsm.h
+++ b/security/safesetid/lsm.h
@@ -21,13 +21,16 @@ extern int safesetid_initialized;
/* Function type. */
enum safesetid_whitelist_file_write_type {
- SAFESETID_WHITELIST_ADD, /* Add whitelist policy. */
+ SAFESETID_WHITELIST_ADD_UID, /* Add UID whitelist policy. */
+ SAFESETID_WHITELIST_ADD_GID, /* Add GID whitelist policy. */
SAFESETID_WHITELIST_FLUSH, /* Flush whitelist policies. */
};
-/* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */
-int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child);
-
+/* Add entry to safesetid whitelist to allow 'parent' to setuid to 'child'. */
+int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child);
+/* Add entry to safesetid whitelist to allow 'parent' to setgid to 'child'. */
+int add_safesetid_whitelist_gid_entry(kgid_t parent, kgid_t child);
+/* Flush all UID/GID whitelist policies. */
void flush_safesetid_whitelist_entries(void);
#endif /* _SAFESETID_H */
diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index 2c6c829be044..c4c25ba7275f 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -26,20 +26,19 @@ struct safesetid_file_entry {
static struct safesetid_file_entry safesetid_files[] = {
{.name = "add_whitelist_policy",
- .type = SAFESETID_WHITELIST_ADD},
+ .type = SAFESETID_WHITELIST_ADD_UID},
+ {.name = "add_whitelist_uid_policy",
+ .type = SAFESETID_WHITELIST_ADD_UID},
+ {.name = "add_whitelist_gid_policy",
+ .type = SAFESETID_WHITELIST_ADD_GID},
{.name = "flush_whitelist_policies",
.type = SAFESETID_WHITELIST_FLUSH},
};
-/*
- * 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
- * function will return an error.
- */
-static int parse_safesetid_whitelist_policy(const char __user *buf,
+static int parse_userbuf_to_longs(const char __user *buf,
size_t len,
- kuid_t *parent,
- kuid_t *child)
+ long *parent,
+ long *child)
{
char *kern_buf;
char *parent_buf;
@@ -47,8 +46,6 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
const char separator[] = ":";
int ret;
size_t first_substring_length;
- long parsed_parent;
- long parsed_child;
/* Duplicate string from user memory and NULL-terminate */
kern_buf = memdup_user_nul(buf, len);
@@ -71,27 +68,15 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
goto free_kern;
}
- ret = kstrtol(parent_buf, 0, &parsed_parent);
+ ret = kstrtol(parent_buf, 0, parent);
if (ret)
goto free_both;
child_buf = kern_buf + first_substring_length + 1;
- ret = kstrtol(child_buf, 0, &parsed_child);
+ ret = kstrtol(child_buf, 0, child);
if (ret)
goto free_both;
- *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;
- }
-
free_both:
kfree(parent_buf);
free_kern:
@@ -99,6 +84,52 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
return ret;
}
+static int parse_safesetid_whitelist_uid_policy(const char __user *buf,
+ size_t len,
+ kuid_t *parent_uid,
+ kuid_t *child_uid)
+{
+ int ret;
+ long parent, child;
+
+ ret = parse_userbuf_to_longs(buf, len, &parent, &child);
+ if (ret)
+ return ret;
+
+ *parent_uid = make_kuid(current_user_ns(), parent);
+ if (!uid_valid(*parent_uid))
+ return -EINVAL;
+
+ *child_uid = make_kuid(current_user_ns(), child);
+ if (!uid_valid(*child_uid))
+ return -EINVAL;
+
+ return 0;
+}
+
+static int parse_safesetid_whitelist_gid_policy(const char __user *buf,
+ size_t len,
+ kgid_t *parent_gid,
+ kgid_t *child_gid)
+{
+ int ret;
+ long parent, child;
+
+ ret = parse_userbuf_to_longs(buf, len, &parent, &child);
+ if (ret)
+ return ret;
+
+ *parent_gid = make_kgid(current_user_ns(), parent);
+ if (!gid_valid(*parent_gid))
+ return -EINVAL;
+
+ *child_gid = make_kgid(current_user_ns(), child);
+ if (!gid_valid(*child_gid))
+ return -EINVAL;
+
+ return 0;
+}
+
static ssize_t safesetid_file_write(struct file *file,
const char __user *buf,
size_t len,
@@ -106,8 +137,10 @@ static ssize_t safesetid_file_write(struct file *file,
{
struct safesetid_file_entry *file_entry =
file->f_inode->i_private;
- kuid_t parent;
- kuid_t child;
+ kuid_t uid_parent;
+ kuid_t uid_child;
+ kgid_t gid_parent;
+ kgid_t gid_child;
int ret;
if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN))
@@ -120,13 +153,23 @@ static ssize_t safesetid_file_write(struct file *file,
case SAFESETID_WHITELIST_FLUSH:
flush_safesetid_whitelist_entries();
break;
- case SAFESETID_WHITELIST_ADD:
- ret = parse_safesetid_whitelist_policy(buf, len, &parent,
- &child);
+ case SAFESETID_WHITELIST_ADD_UID:
+ ret = parse_safesetid_whitelist_uid_policy(buf, len, &uid_parent,
+ &uid_child);
+ if (ret)
+ return ret;
+
+ ret = add_safesetid_whitelist_uid_entry(uid_parent, uid_child);
+ if (ret)
+ return ret;
+ break;
+ case SAFESETID_WHITELIST_ADD_GID:
+ ret = parse_safesetid_whitelist_gid_policy(buf, len, &gid_parent,
+ &gid_child);
if (ret)
return ret;
- ret = add_safesetid_whitelist_entry(parent, child);
+ ret = add_safesetid_whitelist_gid_entry(gid_parent, gid_child);
if (ret)
return ret;
break;
--
2.21.0.352.gf09ad66450-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/2] LSM: SafeSetID: gate setgid transitions
2019-03-05 15:52 ` [PATCH v4 2/2] " mortonm
@ 2019-03-11 17:25 ` Micah Morton
2019-03-11 18:38 ` James Morris
0 siblings, 1 reply; 10+ messages in thread
From: Micah Morton @ 2019-03-11 17:25 UTC (permalink / raw)
To: James Morris, Serge E. Hallyn, Kees Cook, Casey Schaufler,
Stephen Smalley, linux-security-module
Is this ready to be merged if there are no further comments?
On Tue, Mar 5, 2019 at 7:52 AM <mortonm@chromium.org> wrote:
>
> From: Micah Morton <mortonm@chromium.org>
>
> The SafeSetID LSM already gates setuid transitions for UIDs on the
> system whose use of CAP_SETUID has been 'restricted'. This patch
> implements the analogous functionality for setgid transitions, in order
> to restrict the use of CAP_SETGID for certain UIDs on the system. One
> notable consequence of this addition is that a process running under a
> restricted UID (i.e. one that is only allowed to setgid to certain
> approved GIDs) will not be allowed to call the setgroups() syscall to
> set its supplementary group IDs. For now, we leave such support for
> restricted setgroups() to future work, as it would require hooking the
> logic in setgroups() and verifying that the array of GIDs passed in from
> userspace only consists of approved GIDs.
>
> Signed-off-by: Micah Morton <mortonm@chromium.org>
> ---
> Changes since the last patch: Minor updates since patch 1/2 combined the
> 'task_fix_setuid' and 'task_fix_setgid' hooks into one hook called
> 'task_fix_setid'.
>
> Documentation/admin-guide/LSM/SafeSetID.rst | 9 +-
> security/safesetid/lsm.c | 272 +++++++++++++++++---
> security/safesetid/lsm.h | 11 +-
> security/safesetid/securityfs.c | 105 +++++---
> 4 files changed, 320 insertions(+), 77 deletions(-)
>
> diff --git a/Documentation/admin-guide/LSM/SafeSetID.rst b/Documentation/admin-guide/LSM/SafeSetID.rst
> index 670a6544fd39..33307a8e9555 100644
> --- a/Documentation/admin-guide/LSM/SafeSetID.rst
> +++ b/Documentation/admin-guide/LSM/SafeSetID.rst
> @@ -98,10 +98,13 @@ Directions for use
> ==================
> This LSM hooks the setid syscalls to make sure transitions are allowed if an
> applicable restriction policy is in place. Policies are configured through
> -securityfs by writing to the safesetid/add_whitelist_policy and
> +securityfs by writing to the safesetid/add_whitelist_{uid/gid}_policy* and
> safesetid/flush_whitelist_policies files at the location where securityfs is
> -mounted. The format for adding a policy is '<UID>:<UID>', using literal
> +mounted. The format for adding a policy is '<ID>:<ID>', using literal
> numbers, such as '123:456'. To flush the policies, any write to the file is
> sufficient. Again, configuring a policy for a UID will prevent that UID from
> obtaining auxiliary setid privileges, such as allowing a user to set up user
> -namespace UID mappings.
> +namespace ID mappings.
> +
> +*safesetid/add_whitelist_policy can also be used for UID policies, since it is
> +an alias for safesetid/add_whitelist_uid_policy
> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index 5deffa92f25f..95c6f35b29f3 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -26,27 +26,30 @@ int safesetid_initialized;
>
> #define NUM_BITS 8 /* 128 buckets in hash table */
>
> -static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS);
> +static DEFINE_HASHTABLE(safesetid_whitelist_uid_hashtable, NUM_BITS);
> +static DEFINE_HASHTABLE(safesetid_whitelist_gid_hashtable, NUM_BITS);
> +
> +static DEFINE_SPINLOCK(safesetid_whitelist_uid_hashtable_spinlock);
> +static DEFINE_SPINLOCK(safesetid_whitelist_gid_hashtable_spinlock);
>
> /*
> * Hash table entry to store safesetid policy signifying that 'parent' user
> - * can setid to 'child' user.
> + * can setid to 'child' user. This struct is used in both the uid and gid
> + * hashtables.
> */
> -struct entry {
> +struct id_entry {
> struct hlist_node next;
> struct hlist_node dlist; /* for deletion cleanup */
> uint64_t parent_kuid;
> - uint64_t child_kuid;
> + uint64_t child_kid; /* Represents either a UID or a GID */
> };
>
> -static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock);
> -
> static bool check_setuid_policy_hashtable_key(kuid_t parent)
> {
> - struct entry *entry;
> + struct id_entry *entry;
>
> rcu_read_lock();
> - hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> + hash_for_each_possible_rcu(safesetid_whitelist_uid_hashtable,
> entry, next, __kuid_val(parent)) {
> if (entry->parent_kuid == __kuid_val(parent)) {
> rcu_read_unlock();
> @@ -61,13 +64,49 @@ static bool check_setuid_policy_hashtable_key(kuid_t parent)
> static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
> kuid_t child)
> {
> - struct entry *entry;
> + struct id_entry *entry;
> +
> + rcu_read_lock();
> + hash_for_each_possible_rcu(safesetid_whitelist_uid_hashtable,
> + entry, next, __kuid_val(parent)) {
> + if (entry->parent_kuid == __kuid_val(parent) &&
> + entry->child_kid == __kuid_val(child)) {
> + rcu_read_unlock();
> + return true;
> + }
> + }
> + rcu_read_unlock();
> +
> + return false;
> +}
> +
> +static bool check_setgid_policy_hashtable_key(kuid_t parent)
> +{
> + struct id_entry *entry;
> +
> + rcu_read_lock();
> + hash_for_each_possible_rcu(safesetid_whitelist_gid_hashtable,
> + entry, next, __kuid_val(parent)) {
> + if (entry->parent_kuid == __kuid_val(parent)) {
> + rcu_read_unlock();
> + return true;
> + }
> + }
> + rcu_read_unlock();
> +
> + return false;
> +}
> +
> +static bool check_setgid_policy_hashtable_key_value(kuid_t parent,
> + kgid_t child)
> +{
> + struct id_entry *entry;
>
> rcu_read_lock();
> - hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> + hash_for_each_possible_rcu(safesetid_whitelist_gid_hashtable,
> entry, next, __kuid_val(parent)) {
> if (entry->parent_kuid == __kuid_val(parent) &&
> - entry->child_kuid == __kuid_val(child)) {
> + entry->child_kid == __kgid_val(child)) {
> rcu_read_unlock();
> return true;
> }
> @@ -77,6 +116,12 @@ static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
> return false;
> }
>
> +/*
> + * This hook causes the security_capable check to fail when there are
> + * restriction policies for a UID and the process is trying to do something
> + * (other than a setid transition) that is gated by CAP_SETUID/CAP_SETGID
> + * (e.g. allowing user to set up userns UID/GID mappings).
> + */
> static int safesetid_security_capable(const struct cred *cred,
> struct user_namespace *ns,
> int cap,
> @@ -85,17 +130,19 @@ static int safesetid_security_capable(const struct cred *cred,
> if (cap == CAP_SETUID &&
> check_setuid_policy_hashtable_key(cred->uid)) {
> 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",
> __kuid_val(cred->uid));
> return -1;
> }
> }
> + if (cap == CAP_SETGID &&
> + check_setgid_policy_hashtable_key(cred->uid)) {
> + if (!(opts & CAP_OPT_INSETID)) {
> + pr_warn("Operation requires CAP_SETGID, which is not available to UID %u for operations besides approved set*gid transitions",
> + __kuid_val(cred->uid));
> + return -1;
> + }
> + }
> return 0;
> }
>
> @@ -115,19 +162,48 @@ static int check_uid_transition(kuid_t parent, kuid_t child)
> return -EACCES;
> }
>
> +static int check_gid_transition(kuid_t parent, kgid_t child)
> +{
> + if (check_setgid_policy_hashtable_key_value(parent, child))
> + return 0;
> + pr_warn("Denied UID %d setting GID to %d",
> + __kuid_val(parent),
> + __kgid_val(child));
> + /*
> + * 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, current);
> + return -EACCES;
> +}
> +
> /*
> * Check whether there is either an exception for user under old cred struct to
> - * set*uid to user under new cred struct, or the UID transition is allowed (by
> - * Linux set*uid rules) even without CAP_SETUID.
> + * set*id to user under new cred struct, or the ID transition is allowed (by
> + * Linux set*id rules) even without CAP_SET*ID.
> */
> static int safesetid_task_fix_setid(struct cred *new,
> const struct cred *old,
> int flags)
> {
> + if (flags == LSM_SETUID_RE ||
> + flags == LSM_SETUID_ID ||
> + flags == LSM_SETUID_RES ||
> + flags == LSM_SETUID_FS) {
> + /* Do nothing if there are no setuid restrictions for this UID. */
> + if (!check_setuid_policy_hashtable_key(old->uid))
> + return 0;
> + }
>
> - /* Do nothing if there are no setuid restrictions for this UID. */
> - if (!check_setuid_policy_hashtable_key(old->uid))
> + if (flags == LSM_SETGID_RE ||
> + flags == LSM_SETGID_ID ||
> + flags == LSM_SETGID_RES ||
> + flags == LSM_SETGID_FS) {
> + /* Do nothing if there are no setgid restrictions for this UID. */
> + if (!check_setgid_policy_hashtable_key(old->uid))
> return 0;
> + }
>
> switch (flags) {
> case LSM_SETUID_RE:
> @@ -201,6 +277,77 @@ static int safesetid_task_fix_setid(struct cred *new,
> return check_uid_transition(old->fsuid, new->fsuid);
> }
> break;
> + case LSM_SETGID_RE:
> + /*
> + * Users for which setgid restrictions exist can only set the
> + * real GID to the real GID or the effective GID, unless an
> + * explicit whitelist policy allows the transition.
> + */
> + if (!gid_eq(old->gid, new->gid) &&
> + !gid_eq(old->egid, new->gid)) {
> + return check_gid_transition(old->uid, new->gid);
> + }
> + /*
> + * Users for which setgid restrictions exist can only set the
> + * effective GID to the real GID, the effective GID, or the
> + * saved set-GID, unless an explicit whitelist policy allows
> + * the transition.
> + */
> + if (!gid_eq(old->gid, new->egid) &&
> + !gid_eq(old->egid, new->egid) &&
> + !gid_eq(old->sgid, new->egid)) {
> + return check_gid_transition(old->euid, new->egid);
> + }
> + break;
> + case LSM_SETGID_ID:
> + /*
> + * Users for which setgid restrictions exist cannot change the
> + * real GID or saved set-GID unless an explicit whitelist
> + * policy allows the transition.
> + */
> + if (!gid_eq(old->gid, new->gid))
> + return check_gid_transition(old->uid, new->gid);
> + if (!gid_eq(old->sgid, new->sgid))
> + return check_gid_transition(old->suid, new->sgid);
> + break;
> + case LSM_SETGID_RES:
> + /*
> + * Users for which setgid restrictions exist cannot change the
> + * real GID, effective GID, or saved set-GID to anything but
> + * one of: the current real GID, the current effective GID or
> + * the current saved set-user-ID unless an explicit whitelist
> + * policy allows the transition.
> + */
> + if (!gid_eq(new->gid, old->gid) &&
> + !gid_eq(new->gid, old->egid) &&
> + !gid_eq(new->gid, old->sgid)) {
> + return check_gid_transition(old->uid, new->gid);
> + }
> + if (!gid_eq(new->egid, old->gid) &&
> + !gid_eq(new->egid, old->egid) &&
> + !gid_eq(new->egid, old->sgid)) {
> + return check_gid_transition(old->euid, new->egid);
> + }
> + if (!gid_eq(new->sgid, old->gid) &&
> + !gid_eq(new->sgid, old->egid) &&
> + !gid_eq(new->sgid, old->sgid)) {
> + return check_gid_transition(old->suid, new->sgid);
> + }
> + break;
> + case LSM_SETGID_FS:
> + /*
> + * Users for which setgid restrictions exist cannot change the
> + * filesystem GID to anything but one of: the current real GID,
> + * the current effective GID or the current saved set-GID
> + * unless an explicit whitelist policy allows the transition.
> + */
> + if (!gid_eq(new->fsgid, old->gid) &&
> + !gid_eq(new->fsgid, old->egid) &&
> + !gid_eq(new->fsgid, old->sgid) &&
> + !gid_eq(new->fsgid, old->fsgid)) {
> + return check_gid_transition(old->fsuid, new->fsgid);
> + }
> + break;
> default:
> pr_warn("Unknown setid state %d\n", flags);
> force_sig(SIGKILL, current);
> @@ -209,49 +356,96 @@ static int safesetid_task_fix_setid(struct cred *new,
> return 0;
> }
>
> -int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
> +int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child)
> {
> - struct entry *new;
> + struct id_entry *new;
>
> /* Return if entry already exists */
> if (check_setuid_policy_hashtable_key_value(parent, child))
> return 0;
>
> - new = kzalloc(sizeof(struct entry), GFP_KERNEL);
> + new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
> + if (!new)
> + return -ENOMEM;
> + new->parent_kuid = __kuid_val(parent);
> + new->child_kid = __kuid_val(child);
> + spin_lock(&safesetid_whitelist_uid_hashtable_spinlock);
> + /* Return if the entry got added since we checked above */
> + if (check_setuid_policy_hashtable_key_value(parent, child)) {
> + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> + kfree(new);
> + return 0;
> + }
> + hash_add_rcu(safesetid_whitelist_uid_hashtable,
> + &new->next,
> + __kuid_val(parent));
> + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> + return 0;
> +}
> +
> +int add_safesetid_whitelist_gid_entry(kuid_t parent, kgid_t child)
> +{
> + struct id_entry *new;
> +
> + /* Return if entry already exists */
> + if (check_setgid_policy_hashtable_key_value(parent, child))
> + return 0;
> +
> + new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
> if (!new)
> return -ENOMEM;
> new->parent_kuid = __kuid_val(parent);
> - new->child_kuid = __kuid_val(child);
> - spin_lock(&safesetid_whitelist_hashtable_spinlock);
> - hash_add_rcu(safesetid_whitelist_hashtable,
> + new->child_kid = __kgid_val(child);
> + spin_lock(&safesetid_whitelist_gid_hashtable_spinlock);
> + /* Return if the entry got added since we checked above */
> + if (check_setgid_policy_hashtable_key_value(parent, child)) {
> + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> + kfree(new);
> + return 0;
> + }
> + hash_add_rcu(safesetid_whitelist_gid_hashtable,
> &new->next,
> __kuid_val(parent));
> - spin_unlock(&safesetid_whitelist_hashtable_spinlock);
> + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> return 0;
> }
>
> void flush_safesetid_whitelist_entries(void)
> {
> - struct entry *entry;
> + struct id_entry *id_entry;
> struct hlist_node *hlist_node;
> unsigned int bkt_loop_cursor;
> - HLIST_HEAD(free_list);
> + HLIST_HEAD(uid_free_list);
> + HLIST_HEAD(gid_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_lock(&safesetid_whitelist_uid_hashtable_spinlock);
> + hash_for_each_safe(safesetid_whitelist_uid_hashtable, bkt_loop_cursor,
> + hlist_node, id_entry, next) {
> + hash_del_rcu(&id_entry->next);
> + hlist_add_head(&id_entry->dlist, &uid_free_list);
> + }
> + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> + synchronize_rcu();
> + hlist_for_each_entry_safe(id_entry, hlist_node, &uid_free_list, dlist) {
> + hlist_del(&id_entry->dlist);
> + kfree(id_entry);
> + }
> +
> + spin_lock(&safesetid_whitelist_gid_hashtable_spinlock);
> + hash_for_each_safe(safesetid_whitelist_gid_hashtable, bkt_loop_cursor,
> + hlist_node, id_entry, next) {
> + hash_del_rcu(&id_entry->next);
> + hlist_add_head(&id_entry->dlist, &gid_free_list);
> }
> - spin_unlock(&safesetid_whitelist_hashtable_spinlock);
> + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> synchronize_rcu();
> - hlist_for_each_entry_safe(entry, hlist_node, &free_list, dlist) {
> - hlist_del(&entry->dlist);
> - kfree(entry);
> + hlist_for_each_entry_safe(id_entry, hlist_node, &gid_free_list, dlist) {
> + hlist_del(&id_entry->dlist);
> + kfree(id_entry);
> }
> }
>
> diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> index c1ea3c265fcf..e9ae192caff2 100644
> --- a/security/safesetid/lsm.h
> +++ b/security/safesetid/lsm.h
> @@ -21,13 +21,16 @@ extern int safesetid_initialized;
>
> /* Function type. */
> enum safesetid_whitelist_file_write_type {
> - SAFESETID_WHITELIST_ADD, /* Add whitelist policy. */
> + SAFESETID_WHITELIST_ADD_UID, /* Add UID whitelist policy. */
> + SAFESETID_WHITELIST_ADD_GID, /* Add GID whitelist policy. */
> SAFESETID_WHITELIST_FLUSH, /* Flush whitelist policies. */
> };
>
> -/* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */
> -int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child);
> -
> +/* Add entry to safesetid whitelist to allow 'parent' to setuid to 'child'. */
> +int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child);
> +/* Add entry to safesetid whitelist to allow 'parent' to setgid to 'child'. */
> +int add_safesetid_whitelist_gid_entry(kgid_t parent, kgid_t child);
> +/* Flush all UID/GID whitelist policies. */
> void flush_safesetid_whitelist_entries(void);
>
> #endif /* _SAFESETID_H */
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 2c6c829be044..c4c25ba7275f 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -26,20 +26,19 @@ struct safesetid_file_entry {
>
> static struct safesetid_file_entry safesetid_files[] = {
> {.name = "add_whitelist_policy",
> - .type = SAFESETID_WHITELIST_ADD},
> + .type = SAFESETID_WHITELIST_ADD_UID},
> + {.name = "add_whitelist_uid_policy",
> + .type = SAFESETID_WHITELIST_ADD_UID},
> + {.name = "add_whitelist_gid_policy",
> + .type = SAFESETID_WHITELIST_ADD_GID},
> {.name = "flush_whitelist_policies",
> .type = SAFESETID_WHITELIST_FLUSH},
> };
>
> -/*
> - * 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
> - * function will return an error.
> - */
> -static int parse_safesetid_whitelist_policy(const char __user *buf,
> +static int parse_userbuf_to_longs(const char __user *buf,
> size_t len,
> - kuid_t *parent,
> - kuid_t *child)
> + long *parent,
> + long *child)
> {
> char *kern_buf;
> char *parent_buf;
> @@ -47,8 +46,6 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> const char separator[] = ":";
> int ret;
> size_t first_substring_length;
> - long parsed_parent;
> - long parsed_child;
>
> /* Duplicate string from user memory and NULL-terminate */
> kern_buf = memdup_user_nul(buf, len);
> @@ -71,27 +68,15 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> goto free_kern;
> }
>
> - ret = kstrtol(parent_buf, 0, &parsed_parent);
> + ret = kstrtol(parent_buf, 0, parent);
> if (ret)
> goto free_both;
>
> child_buf = kern_buf + first_substring_length + 1;
> - ret = kstrtol(child_buf, 0, &parsed_child);
> + ret = kstrtol(child_buf, 0, child);
> if (ret)
> goto free_both;
>
> - *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;
> - }
> -
> free_both:
> kfree(parent_buf);
> free_kern:
> @@ -99,6 +84,52 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> return ret;
> }
>
> +static int parse_safesetid_whitelist_uid_policy(const char __user *buf,
> + size_t len,
> + kuid_t *parent_uid,
> + kuid_t *child_uid)
> +{
> + int ret;
> + long parent, child;
> +
> + ret = parse_userbuf_to_longs(buf, len, &parent, &child);
> + if (ret)
> + return ret;
> +
> + *parent_uid = make_kuid(current_user_ns(), parent);
> + if (!uid_valid(*parent_uid))
> + return -EINVAL;
> +
> + *child_uid = make_kuid(current_user_ns(), child);
> + if (!uid_valid(*child_uid))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int parse_safesetid_whitelist_gid_policy(const char __user *buf,
> + size_t len,
> + kgid_t *parent_gid,
> + kgid_t *child_gid)
> +{
> + int ret;
> + long parent, child;
> +
> + ret = parse_userbuf_to_longs(buf, len, &parent, &child);
> + if (ret)
> + return ret;
> +
> + *parent_gid = make_kgid(current_user_ns(), parent);
> + if (!gid_valid(*parent_gid))
> + return -EINVAL;
> +
> + *child_gid = make_kgid(current_user_ns(), child);
> + if (!gid_valid(*child_gid))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static ssize_t safesetid_file_write(struct file *file,
> const char __user *buf,
> size_t len,
> @@ -106,8 +137,10 @@ static ssize_t safesetid_file_write(struct file *file,
> {
> struct safesetid_file_entry *file_entry =
> file->f_inode->i_private;
> - kuid_t parent;
> - kuid_t child;
> + kuid_t uid_parent;
> + kuid_t uid_child;
> + kgid_t gid_parent;
> + kgid_t gid_child;
> int ret;
>
> if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN))
> @@ -120,13 +153,23 @@ static ssize_t safesetid_file_write(struct file *file,
> case SAFESETID_WHITELIST_FLUSH:
> flush_safesetid_whitelist_entries();
> break;
> - case SAFESETID_WHITELIST_ADD:
> - ret = parse_safesetid_whitelist_policy(buf, len, &parent,
> - &child);
> + case SAFESETID_WHITELIST_ADD_UID:
> + ret = parse_safesetid_whitelist_uid_policy(buf, len, &uid_parent,
> + &uid_child);
> + if (ret)
> + return ret;
> +
> + ret = add_safesetid_whitelist_uid_entry(uid_parent, uid_child);
> + if (ret)
> + return ret;
> + break;
> + case SAFESETID_WHITELIST_ADD_GID:
> + ret = parse_safesetid_whitelist_gid_policy(buf, len, &gid_parent,
> + &gid_child);
> if (ret)
> return ret;
>
> - ret = add_safesetid_whitelist_entry(parent, child);
> + ret = add_safesetid_whitelist_gid_entry(gid_parent, gid_child);
> if (ret)
> return ret;
> break;
> --
> 2.21.0.352.gf09ad66450-goog
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/2] LSM: SafeSetID: gate setgid transitions
2019-03-11 17:25 ` Micah Morton
@ 2019-03-11 18:38 ` James Morris
0 siblings, 0 replies; 10+ messages in thread
From: James Morris @ 2019-03-11 18:38 UTC (permalink / raw)
To: Micah Morton
Cc: Serge E. Hallyn, Kees Cook, Casey Schaufler, Stephen Smalley,
linux-security-module
On Mon, 11 Mar 2019, Micah Morton wrote:
> Is this ready to be merged if there are no further comments?
Nothing gets merged to the subsystem trees until the main merge window is
closed (i.e. v5.1-rc1).
I'd expect / hope for more review of this to happen, too.
>
> On Tue, Mar 5, 2019 at 7:52 AM <mortonm@chromium.org> wrote:
> >
> > From: Micah Morton <mortonm@chromium.org>
> >
> > The SafeSetID LSM already gates setuid transitions for UIDs on the
> > system whose use of CAP_SETUID has been 'restricted'. This patch
> > implements the analogous functionality for setgid transitions, in order
> > to restrict the use of CAP_SETGID for certain UIDs on the system. One
> > notable consequence of this addition is that a process running under a
> > restricted UID (i.e. one that is only allowed to setgid to certain
> > approved GIDs) will not be allowed to call the setgroups() syscall to
> > set its supplementary group IDs. For now, we leave such support for
> > restricted setgroups() to future work, as it would require hooking the
> > logic in setgroups() and verifying that the array of GIDs passed in from
> > userspace only consists of approved GIDs.
> >
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
> > ---
> > Changes since the last patch: Minor updates since patch 1/2 combined the
> > 'task_fix_setuid' and 'task_fix_setgid' hooks into one hook called
> > 'task_fix_setid'.
> >
> > Documentation/admin-guide/LSM/SafeSetID.rst | 9 +-
> > security/safesetid/lsm.c | 272 +++++++++++++++++---
> > security/safesetid/lsm.h | 11 +-
> > security/safesetid/securityfs.c | 105 +++++---
> > 4 files changed, 320 insertions(+), 77 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/LSM/SafeSetID.rst b/Documentation/admin-guide/LSM/SafeSetID.rst
> > index 670a6544fd39..33307a8e9555 100644
> > --- a/Documentation/admin-guide/LSM/SafeSetID.rst
> > +++ b/Documentation/admin-guide/LSM/SafeSetID.rst
> > @@ -98,10 +98,13 @@ Directions for use
> > ==================
> > This LSM hooks the setid syscalls to make sure transitions are allowed if an
> > applicable restriction policy is in place. Policies are configured through
> > -securityfs by writing to the safesetid/add_whitelist_policy and
> > +securityfs by writing to the safesetid/add_whitelist_{uid/gid}_policy* and
> > safesetid/flush_whitelist_policies files at the location where securityfs is
> > -mounted. The format for adding a policy is '<UID>:<UID>', using literal
> > +mounted. The format for adding a policy is '<ID>:<ID>', using literal
> > numbers, such as '123:456'. To flush the policies, any write to the file is
> > sufficient. Again, configuring a policy for a UID will prevent that UID from
> > obtaining auxiliary setid privileges, such as allowing a user to set up user
> > -namespace UID mappings.
> > +namespace ID mappings.
> > +
> > +*safesetid/add_whitelist_policy can also be used for UID policies, since it is
> > +an alias for safesetid/add_whitelist_uid_policy
> > diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> > index 5deffa92f25f..95c6f35b29f3 100644
> > --- a/security/safesetid/lsm.c
> > +++ b/security/safesetid/lsm.c
> > @@ -26,27 +26,30 @@ int safesetid_initialized;
> >
> > #define NUM_BITS 8 /* 128 buckets in hash table */
> >
> > -static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS);
> > +static DEFINE_HASHTABLE(safesetid_whitelist_uid_hashtable, NUM_BITS);
> > +static DEFINE_HASHTABLE(safesetid_whitelist_gid_hashtable, NUM_BITS);
> > +
> > +static DEFINE_SPINLOCK(safesetid_whitelist_uid_hashtable_spinlock);
> > +static DEFINE_SPINLOCK(safesetid_whitelist_gid_hashtable_spinlock);
> >
> > /*
> > * Hash table entry to store safesetid policy signifying that 'parent' user
> > - * can setid to 'child' user.
> > + * can setid to 'child' user. This struct is used in both the uid and gid
> > + * hashtables.
> > */
> > -struct entry {
> > +struct id_entry {
> > struct hlist_node next;
> > struct hlist_node dlist; /* for deletion cleanup */
> > uint64_t parent_kuid;
> > - uint64_t child_kuid;
> > + uint64_t child_kid; /* Represents either a UID or a GID */
> > };
> >
> > -static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock);
> > -
> > static bool check_setuid_policy_hashtable_key(kuid_t parent)
> > {
> > - struct entry *entry;
> > + struct id_entry *entry;
> >
> > rcu_read_lock();
> > - hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> > + hash_for_each_possible_rcu(safesetid_whitelist_uid_hashtable,
> > entry, next, __kuid_val(parent)) {
> > if (entry->parent_kuid == __kuid_val(parent)) {
> > rcu_read_unlock();
> > @@ -61,13 +64,49 @@ static bool check_setuid_policy_hashtable_key(kuid_t parent)
> > static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
> > kuid_t child)
> > {
> > - struct entry *entry;
> > + struct id_entry *entry;
> > +
> > + rcu_read_lock();
> > + hash_for_each_possible_rcu(safesetid_whitelist_uid_hashtable,
> > + entry, next, __kuid_val(parent)) {
> > + if (entry->parent_kuid == __kuid_val(parent) &&
> > + entry->child_kid == __kuid_val(child)) {
> > + rcu_read_unlock();
> > + return true;
> > + }
> > + }
> > + rcu_read_unlock();
> > +
> > + return false;
> > +}
> > +
> > +static bool check_setgid_policy_hashtable_key(kuid_t parent)
> > +{
> > + struct id_entry *entry;
> > +
> > + rcu_read_lock();
> > + hash_for_each_possible_rcu(safesetid_whitelist_gid_hashtable,
> > + entry, next, __kuid_val(parent)) {
> > + if (entry->parent_kuid == __kuid_val(parent)) {
> > + rcu_read_unlock();
> > + return true;
> > + }
> > + }
> > + rcu_read_unlock();
> > +
> > + return false;
> > +}
> > +
> > +static bool check_setgid_policy_hashtable_key_value(kuid_t parent,
> > + kgid_t child)
> > +{
> > + struct id_entry *entry;
> >
> > rcu_read_lock();
> > - hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> > + hash_for_each_possible_rcu(safesetid_whitelist_gid_hashtable,
> > entry, next, __kuid_val(parent)) {
> > if (entry->parent_kuid == __kuid_val(parent) &&
> > - entry->child_kuid == __kuid_val(child)) {
> > + entry->child_kid == __kgid_val(child)) {
> > rcu_read_unlock();
> > return true;
> > }
> > @@ -77,6 +116,12 @@ static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
> > return false;
> > }
> >
> > +/*
> > + * This hook causes the security_capable check to fail when there are
> > + * restriction policies for a UID and the process is trying to do something
> > + * (other than a setid transition) that is gated by CAP_SETUID/CAP_SETGID
> > + * (e.g. allowing user to set up userns UID/GID mappings).
> > + */
> > static int safesetid_security_capable(const struct cred *cred,
> > struct user_namespace *ns,
> > int cap,
> > @@ -85,17 +130,19 @@ static int safesetid_security_capable(const struct cred *cred,
> > if (cap == CAP_SETUID &&
> > check_setuid_policy_hashtable_key(cred->uid)) {
> > 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",
> > __kuid_val(cred->uid));
> > return -1;
> > }
> > }
> > + if (cap == CAP_SETGID &&
> > + check_setgid_policy_hashtable_key(cred->uid)) {
> > + if (!(opts & CAP_OPT_INSETID)) {
> > + pr_warn("Operation requires CAP_SETGID, which is not available to UID %u for operations besides approved set*gid transitions",
> > + __kuid_val(cred->uid));
> > + return -1;
> > + }
> > + }
> > return 0;
> > }
> >
> > @@ -115,19 +162,48 @@ static int check_uid_transition(kuid_t parent, kuid_t child)
> > return -EACCES;
> > }
> >
> > +static int check_gid_transition(kuid_t parent, kgid_t child)
> > +{
> > + if (check_setgid_policy_hashtable_key_value(parent, child))
> > + return 0;
> > + pr_warn("Denied UID %d setting GID to %d",
> > + __kuid_val(parent),
> > + __kgid_val(child));
> > + /*
> > + * 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, current);
> > + return -EACCES;
> > +}
> > +
> > /*
> > * Check whether there is either an exception for user under old cred struct to
> > - * set*uid to user under new cred struct, or the UID transition is allowed (by
> > - * Linux set*uid rules) even without CAP_SETUID.
> > + * set*id to user under new cred struct, or the ID transition is allowed (by
> > + * Linux set*id rules) even without CAP_SET*ID.
> > */
> > static int safesetid_task_fix_setid(struct cred *new,
> > const struct cred *old,
> > int flags)
> > {
> > + if (flags == LSM_SETUID_RE ||
> > + flags == LSM_SETUID_ID ||
> > + flags == LSM_SETUID_RES ||
> > + flags == LSM_SETUID_FS) {
> > + /* Do nothing if there are no setuid restrictions for this UID. */
> > + if (!check_setuid_policy_hashtable_key(old->uid))
> > + return 0;
> > + }
> >
> > - /* Do nothing if there are no setuid restrictions for this UID. */
> > - if (!check_setuid_policy_hashtable_key(old->uid))
> > + if (flags == LSM_SETGID_RE ||
> > + flags == LSM_SETGID_ID ||
> > + flags == LSM_SETGID_RES ||
> > + flags == LSM_SETGID_FS) {
> > + /* Do nothing if there are no setgid restrictions for this UID. */
> > + if (!check_setgid_policy_hashtable_key(old->uid))
> > return 0;
> > + }
> >
> > switch (flags) {
> > case LSM_SETUID_RE:
> > @@ -201,6 +277,77 @@ static int safesetid_task_fix_setid(struct cred *new,
> > return check_uid_transition(old->fsuid, new->fsuid);
> > }
> > break;
> > + case LSM_SETGID_RE:
> > + /*
> > + * Users for which setgid restrictions exist can only set the
> > + * real GID to the real GID or the effective GID, unless an
> > + * explicit whitelist policy allows the transition.
> > + */
> > + if (!gid_eq(old->gid, new->gid) &&
> > + !gid_eq(old->egid, new->gid)) {
> > + return check_gid_transition(old->uid, new->gid);
> > + }
> > + /*
> > + * Users for which setgid restrictions exist can only set the
> > + * effective GID to the real GID, the effective GID, or the
> > + * saved set-GID, unless an explicit whitelist policy allows
> > + * the transition.
> > + */
> > + if (!gid_eq(old->gid, new->egid) &&
> > + !gid_eq(old->egid, new->egid) &&
> > + !gid_eq(old->sgid, new->egid)) {
> > + return check_gid_transition(old->euid, new->egid);
> > + }
> > + break;
> > + case LSM_SETGID_ID:
> > + /*
> > + * Users for which setgid restrictions exist cannot change the
> > + * real GID or saved set-GID unless an explicit whitelist
> > + * policy allows the transition.
> > + */
> > + if (!gid_eq(old->gid, new->gid))
> > + return check_gid_transition(old->uid, new->gid);
> > + if (!gid_eq(old->sgid, new->sgid))
> > + return check_gid_transition(old->suid, new->sgid);
> > + break;
> > + case LSM_SETGID_RES:
> > + /*
> > + * Users for which setgid restrictions exist cannot change the
> > + * real GID, effective GID, or saved set-GID to anything but
> > + * one of: the current real GID, the current effective GID or
> > + * the current saved set-user-ID unless an explicit whitelist
> > + * policy allows the transition.
> > + */
> > + if (!gid_eq(new->gid, old->gid) &&
> > + !gid_eq(new->gid, old->egid) &&
> > + !gid_eq(new->gid, old->sgid)) {
> > + return check_gid_transition(old->uid, new->gid);
> > + }
> > + if (!gid_eq(new->egid, old->gid) &&
> > + !gid_eq(new->egid, old->egid) &&
> > + !gid_eq(new->egid, old->sgid)) {
> > + return check_gid_transition(old->euid, new->egid);
> > + }
> > + if (!gid_eq(new->sgid, old->gid) &&
> > + !gid_eq(new->sgid, old->egid) &&
> > + !gid_eq(new->sgid, old->sgid)) {
> > + return check_gid_transition(old->suid, new->sgid);
> > + }
> > + break;
> > + case LSM_SETGID_FS:
> > + /*
> > + * Users for which setgid restrictions exist cannot change the
> > + * filesystem GID to anything but one of: the current real GID,
> > + * the current effective GID or the current saved set-GID
> > + * unless an explicit whitelist policy allows the transition.
> > + */
> > + if (!gid_eq(new->fsgid, old->gid) &&
> > + !gid_eq(new->fsgid, old->egid) &&
> > + !gid_eq(new->fsgid, old->sgid) &&
> > + !gid_eq(new->fsgid, old->fsgid)) {
> > + return check_gid_transition(old->fsuid, new->fsgid);
> > + }
> > + break;
> > default:
> > pr_warn("Unknown setid state %d\n", flags);
> > force_sig(SIGKILL, current);
> > @@ -209,49 +356,96 @@ static int safesetid_task_fix_setid(struct cred *new,
> > return 0;
> > }
> >
> > -int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
> > +int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child)
> > {
> > - struct entry *new;
> > + struct id_entry *new;
> >
> > /* Return if entry already exists */
> > if (check_setuid_policy_hashtable_key_value(parent, child))
> > return 0;
> >
> > - new = kzalloc(sizeof(struct entry), GFP_KERNEL);
> > + new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
> > + if (!new)
> > + return -ENOMEM;
> > + new->parent_kuid = __kuid_val(parent);
> > + new->child_kid = __kuid_val(child);
> > + spin_lock(&safesetid_whitelist_uid_hashtable_spinlock);
> > + /* Return if the entry got added since we checked above */
> > + if (check_setuid_policy_hashtable_key_value(parent, child)) {
> > + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> > + kfree(new);
> > + return 0;
> > + }
> > + hash_add_rcu(safesetid_whitelist_uid_hashtable,
> > + &new->next,
> > + __kuid_val(parent));
> > + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> > + return 0;
> > +}
> > +
> > +int add_safesetid_whitelist_gid_entry(kuid_t parent, kgid_t child)
> > +{
> > + struct id_entry *new;
> > +
> > + /* Return if entry already exists */
> > + if (check_setgid_policy_hashtable_key_value(parent, child))
> > + return 0;
> > +
> > + new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
> > if (!new)
> > return -ENOMEM;
> > new->parent_kuid = __kuid_val(parent);
> > - new->child_kuid = __kuid_val(child);
> > - spin_lock(&safesetid_whitelist_hashtable_spinlock);
> > - hash_add_rcu(safesetid_whitelist_hashtable,
> > + new->child_kid = __kgid_val(child);
> > + spin_lock(&safesetid_whitelist_gid_hashtable_spinlock);
> > + /* Return if the entry got added since we checked above */
> > + if (check_setgid_policy_hashtable_key_value(parent, child)) {
> > + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> > + kfree(new);
> > + return 0;
> > + }
> > + hash_add_rcu(safesetid_whitelist_gid_hashtable,
> > &new->next,
> > __kuid_val(parent));
> > - spin_unlock(&safesetid_whitelist_hashtable_spinlock);
> > + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> > return 0;
> > }
> >
> > void flush_safesetid_whitelist_entries(void)
> > {
> > - struct entry *entry;
> > + struct id_entry *id_entry;
> > struct hlist_node *hlist_node;
> > unsigned int bkt_loop_cursor;
> > - HLIST_HEAD(free_list);
> > + HLIST_HEAD(uid_free_list);
> > + HLIST_HEAD(gid_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_lock(&safesetid_whitelist_uid_hashtable_spinlock);
> > + hash_for_each_safe(safesetid_whitelist_uid_hashtable, bkt_loop_cursor,
> > + hlist_node, id_entry, next) {
> > + hash_del_rcu(&id_entry->next);
> > + hlist_add_head(&id_entry->dlist, &uid_free_list);
> > + }
> > + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> > + synchronize_rcu();
> > + hlist_for_each_entry_safe(id_entry, hlist_node, &uid_free_list, dlist) {
> > + hlist_del(&id_entry->dlist);
> > + kfree(id_entry);
> > + }
> > +
> > + spin_lock(&safesetid_whitelist_gid_hashtable_spinlock);
> > + hash_for_each_safe(safesetid_whitelist_gid_hashtable, bkt_loop_cursor,
> > + hlist_node, id_entry, next) {
> > + hash_del_rcu(&id_entry->next);
> > + hlist_add_head(&id_entry->dlist, &gid_free_list);
> > }
> > - spin_unlock(&safesetid_whitelist_hashtable_spinlock);
> > + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> > synchronize_rcu();
> > - hlist_for_each_entry_safe(entry, hlist_node, &free_list, dlist) {
> > - hlist_del(&entry->dlist);
> > - kfree(entry);
> > + hlist_for_each_entry_safe(id_entry, hlist_node, &gid_free_list, dlist) {
> > + hlist_del(&id_entry->dlist);
> > + kfree(id_entry);
> > }
> > }
> >
> > diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> > index c1ea3c265fcf..e9ae192caff2 100644
> > --- a/security/safesetid/lsm.h
> > +++ b/security/safesetid/lsm.h
> > @@ -21,13 +21,16 @@ extern int safesetid_initialized;
> >
> > /* Function type. */
> > enum safesetid_whitelist_file_write_type {
> > - SAFESETID_WHITELIST_ADD, /* Add whitelist policy. */
> > + SAFESETID_WHITELIST_ADD_UID, /* Add UID whitelist policy. */
> > + SAFESETID_WHITELIST_ADD_GID, /* Add GID whitelist policy. */
> > SAFESETID_WHITELIST_FLUSH, /* Flush whitelist policies. */
> > };
> >
> > -/* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */
> > -int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child);
> > -
> > +/* Add entry to safesetid whitelist to allow 'parent' to setuid to 'child'. */
> > +int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child);
> > +/* Add entry to safesetid whitelist to allow 'parent' to setgid to 'child'. */
> > +int add_safesetid_whitelist_gid_entry(kgid_t parent, kgid_t child);
> > +/* Flush all UID/GID whitelist policies. */
> > void flush_safesetid_whitelist_entries(void);
> >
> > #endif /* _SAFESETID_H */
> > diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> > index 2c6c829be044..c4c25ba7275f 100644
> > --- a/security/safesetid/securityfs.c
> > +++ b/security/safesetid/securityfs.c
> > @@ -26,20 +26,19 @@ struct safesetid_file_entry {
> >
> > static struct safesetid_file_entry safesetid_files[] = {
> > {.name = "add_whitelist_policy",
> > - .type = SAFESETID_WHITELIST_ADD},
> > + .type = SAFESETID_WHITELIST_ADD_UID},
> > + {.name = "add_whitelist_uid_policy",
> > + .type = SAFESETID_WHITELIST_ADD_UID},
> > + {.name = "add_whitelist_gid_policy",
> > + .type = SAFESETID_WHITELIST_ADD_GID},
> > {.name = "flush_whitelist_policies",
> > .type = SAFESETID_WHITELIST_FLUSH},
> > };
> >
> > -/*
> > - * 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
> > - * function will return an error.
> > - */
> > -static int parse_safesetid_whitelist_policy(const char __user *buf,
> > +static int parse_userbuf_to_longs(const char __user *buf,
> > size_t len,
> > - kuid_t *parent,
> > - kuid_t *child)
> > + long *parent,
> > + long *child)
> > {
> > char *kern_buf;
> > char *parent_buf;
> > @@ -47,8 +46,6 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> > const char separator[] = ":";
> > int ret;
> > size_t first_substring_length;
> > - long parsed_parent;
> > - long parsed_child;
> >
> > /* Duplicate string from user memory and NULL-terminate */
> > kern_buf = memdup_user_nul(buf, len);
> > @@ -71,27 +68,15 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> > goto free_kern;
> > }
> >
> > - ret = kstrtol(parent_buf, 0, &parsed_parent);
> > + ret = kstrtol(parent_buf, 0, parent);
> > if (ret)
> > goto free_both;
> >
> > child_buf = kern_buf + first_substring_length + 1;
> > - ret = kstrtol(child_buf, 0, &parsed_child);
> > + ret = kstrtol(child_buf, 0, child);
> > if (ret)
> > goto free_both;
> >
> > - *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;
> > - }
> > -
> > free_both:
> > kfree(parent_buf);
> > free_kern:
> > @@ -99,6 +84,52 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> > return ret;
> > }
> >
> > +static int parse_safesetid_whitelist_uid_policy(const char __user *buf,
> > + size_t len,
> > + kuid_t *parent_uid,
> > + kuid_t *child_uid)
> > +{
> > + int ret;
> > + long parent, child;
> > +
> > + ret = parse_userbuf_to_longs(buf, len, &parent, &child);
> > + if (ret)
> > + return ret;
> > +
> > + *parent_uid = make_kuid(current_user_ns(), parent);
> > + if (!uid_valid(*parent_uid))
> > + return -EINVAL;
> > +
> > + *child_uid = make_kuid(current_user_ns(), child);
> > + if (!uid_valid(*child_uid))
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int parse_safesetid_whitelist_gid_policy(const char __user *buf,
> > + size_t len,
> > + kgid_t *parent_gid,
> > + kgid_t *child_gid)
> > +{
> > + int ret;
> > + long parent, child;
> > +
> > + ret = parse_userbuf_to_longs(buf, len, &parent, &child);
> > + if (ret)
> > + return ret;
> > +
> > + *parent_gid = make_kgid(current_user_ns(), parent);
> > + if (!gid_valid(*parent_gid))
> > + return -EINVAL;
> > +
> > + *child_gid = make_kgid(current_user_ns(), child);
> > + if (!gid_valid(*child_gid))
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > static ssize_t safesetid_file_write(struct file *file,
> > const char __user *buf,
> > size_t len,
> > @@ -106,8 +137,10 @@ static ssize_t safesetid_file_write(struct file *file,
> > {
> > struct safesetid_file_entry *file_entry =
> > file->f_inode->i_private;
> > - kuid_t parent;
> > - kuid_t child;
> > + kuid_t uid_parent;
> > + kuid_t uid_child;
> > + kgid_t gid_parent;
> > + kgid_t gid_child;
> > int ret;
> >
> > if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN))
> > @@ -120,13 +153,23 @@ static ssize_t safesetid_file_write(struct file *file,
> > case SAFESETID_WHITELIST_FLUSH:
> > flush_safesetid_whitelist_entries();
> > break;
> > - case SAFESETID_WHITELIST_ADD:
> > - ret = parse_safesetid_whitelist_policy(buf, len, &parent,
> > - &child);
> > + case SAFESETID_WHITELIST_ADD_UID:
> > + ret = parse_safesetid_whitelist_uid_policy(buf, len, &uid_parent,
> > + &uid_child);
> > + if (ret)
> > + return ret;
> > +
> > + ret = add_safesetid_whitelist_uid_entry(uid_parent, uid_child);
> > + if (ret)
> > + return ret;
> > + break;
> > + case SAFESETID_WHITELIST_ADD_GID:
> > + ret = parse_safesetid_whitelist_gid_policy(buf, len, &gid_parent,
> > + &gid_child);
> > if (ret)
> > return ret;
> >
> > - ret = add_safesetid_whitelist_entry(parent, child);
> > + ret = add_safesetid_whitelist_gid_entry(gid_parent, gid_child);
> > if (ret)
> > return ret;
> > break;
> > --
> > 2.21.0.352.gf09ad66450-goog
> >
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/2] LSM: SafeSetID: gate setgid transitions
2019-03-05 15:49 [PATCH v5 1/2] LSM: SafeSetID: gate setgid transitions mortonm
2019-03-05 15:52 ` [PATCH v4 2/2] " mortonm
@ 2019-03-29 18:06 ` James Morris
2019-03-29 19:44 ` Casey Schaufler
1 sibling, 1 reply; 10+ messages in thread
From: James Morris @ 2019-03-29 18:06 UTC (permalink / raw)
To: Micah Morton; +Cc: serge, keescook, casey, sds, linux-security-module
On Tue, 5 Mar 2019, mortonm@chromium.org wrote:
> From: Micah Morton <mortonm@chromium.org>
>
> This patch generalizes the 'task_fix_setuid' LSM hook to enable hooking
> setgid transitions as well as setuid transitions. The hook is renamed to
> 'task_fix_setid'. The patch introduces calls to this hook from the
> setgid functions in kernel/sys.c. This will allow the SafeSetID LSM to
> govern setgid transitions in addition to setuid transitions. This patch
> also makes sure the setgid functions in kernel/sys.c call
> security_capable_setid rather than the ordinary security_capable
> function, so that the security_capable hook in the SafeSetID LSM knows
> it is being invoked from a setid function.
>
> Signed-off-by: Micah Morton <mortonm@chromium.org>
Wondering if there are any further comments or reviews for this before it
is merged?
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/2] LSM: SafeSetID: gate setgid transitions
2019-03-29 18:06 ` [PATCH v5 1/2] " James Morris
@ 2019-03-29 19:44 ` Casey Schaufler
2019-04-10 15:14 ` Micah Morton
0 siblings, 1 reply; 10+ messages in thread
From: Casey Schaufler @ 2019-03-29 19:44 UTC (permalink / raw)
To: James Morris, Micah Morton; +Cc: serge, keescook, sds, linux-security-module
On 3/29/2019 11:06 AM, James Morris wrote:
> On Tue, 5 Mar 2019, mortonm@chromium.org wrote:
>
>> From: Micah Morton <mortonm@chromium.org>
>>
>> This patch generalizes the 'task_fix_setuid' LSM hook to enable hooking
>> setgid transitions as well as setuid transitions. The hook is renamed to
>> 'task_fix_setid'. The patch introduces calls to this hook from the
>> setgid functions in kernel/sys.c. This will allow the SafeSetID LSM to
>> govern setgid transitions in addition to setuid transitions. This patch
>> also makes sure the setgid functions in kernel/sys.c call
>> security_capable_setid rather than the ordinary security_capable
>> function, so that the security_capable hook in the SafeSetID LSM knows
>> it is being invoked from a setid function.
>>
>> Signed-off-by: Micah Morton <mortonm@chromium.org>
> Wondering if there are any further comments or reviews for this before it
> is merged?
My comments have been addressed.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/2] LSM: SafeSetID: gate setgid transitions
2019-03-29 19:44 ` Casey Schaufler
@ 2019-04-10 15:14 ` Micah Morton
2019-04-10 17:21 ` James Morris
0 siblings, 1 reply; 10+ messages in thread
From: Micah Morton @ 2019-04-10 15:14 UTC (permalink / raw)
To: Casey Schaufler
Cc: James Morris, Serge E. Hallyn, Kees Cook, Stephen Smalley,
linux-security-module
Lets hold off on merging this for now. We have some fixes that will be
going in for the existing LSM code and we can circle back to this once
those have been merged.
On Fri, Mar 29, 2019 at 12:44 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 3/29/2019 11:06 AM, James Morris wrote:
> > On Tue, 5 Mar 2019, mortonm@chromium.org wrote:
> >
> >> From: Micah Morton <mortonm@chromium.org>
> >>
> >> This patch generalizes the 'task_fix_setuid' LSM hook to enable hooking
> >> setgid transitions as well as setuid transitions. The hook is renamed to
> >> 'task_fix_setid'. The patch introduces calls to this hook from the
> >> setgid functions in kernel/sys.c. This will allow the SafeSetID LSM to
> >> govern setgid transitions in addition to setuid transitions. This patch
> >> also makes sure the setgid functions in kernel/sys.c call
> >> security_capable_setid rather than the ordinary security_capable
> >> function, so that the security_capable hook in the SafeSetID LSM knows
> >> it is being invoked from a setid function.
> >>
> >> Signed-off-by: Micah Morton <mortonm@chromium.org>
> > Wondering if there are any further comments or reviews for this before it
> > is merged?
>
> My comments have been addressed.
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/2] LSM: SafeSetID: gate setgid transitions
2019-04-10 15:14 ` Micah Morton
@ 2019-04-10 17:21 ` James Morris
0 siblings, 0 replies; 10+ messages in thread
From: James Morris @ 2019-04-10 17:21 UTC (permalink / raw)
To: Micah Morton
Cc: Casey Schaufler, Serge E. Hallyn, Kees Cook, Stephen Smalley,
linux-security-module
On Wed, 10 Apr 2019, Micah Morton wrote:
> Lets hold off on merging this for now. We have some fixes that will be
> going in for the existing LSM code and we can circle back to this once
> those have been merged.
Ok.
>
> On Fri, Mar 29, 2019 at 12:44 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >
> > On 3/29/2019 11:06 AM, James Morris wrote:
> > > On Tue, 5 Mar 2019, mortonm@chromium.org wrote:
> > >
> > >> From: Micah Morton <mortonm@chromium.org>
> > >>
> > >> This patch generalizes the 'task_fix_setuid' LSM hook to enable hooking
> > >> setgid transitions as well as setuid transitions. The hook is renamed to
> > >> 'task_fix_setid'. The patch introduces calls to this hook from the
> > >> setgid functions in kernel/sys.c. This will allow the SafeSetID LSM to
> > >> govern setgid transitions in addition to setuid transitions. This patch
> > >> also makes sure the setgid functions in kernel/sys.c call
> > >> security_capable_setid rather than the ordinary security_capable
> > >> function, so that the security_capable hook in the SafeSetID LSM knows
> > >> it is being invoked from a setid function.
> > >>
> > >> Signed-off-by: Micah Morton <mortonm@chromium.org>
> > > Wondering if there are any further comments or reviews for this before it
> > > is merged?
> >
> > My comments have been addressed.
> >
> >
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/2] LSM: SafeSetID: gate setgid transitions
2019-02-28 20:20 ` [PATCH v4 2/2] " mortonm
@ 2019-02-28 22:50 ` Casey Schaufler
0 siblings, 0 replies; 10+ messages in thread
From: Casey Schaufler @ 2019-02-28 22:50 UTC (permalink / raw)
To: mortonm, jmorris, serge, keescook, sds, linux-security-module
On 2/28/2019 12:20 PM, mortonm@chromium.org wrote:
> From: Micah Morton <mortonm@chromium.org>
>
> The SafeSetID LSM already gates setuid transitions for UIDs on the
> system whose use of CAP_SETUID has been 'restricted'. This patch
> implements the analogous functionality for setgid transitions, in order
> to restrict the use of CAP_SETGID for certain UIDs on the system. One
> notable consequence of this addition is that a process running under a
> restricted UID (i.e. one that is only allowed to setgid to certain
> approved GIDs) will not be allowed to call the setgroups() syscall to
> set its supplementary group IDs. For now, we leave such support for
> restricted setgroups() to future work, as it would require hooking the
> logic in setgroups() and verifying that the array of GIDs passed in from
> userspace only consists of approved GIDs.
>
> Signed-off-by: Micah Morton <mortonm@chromium.org>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> Changes since the last patch: Minor updates since patch 1/2 combined the
> 'task_fix_setuid' and 'task_fix_setgid' hooks into one hook called
> 'task_fix_setid'.
>
> Documentation/admin-guide/LSM/SafeSetID.rst | 9 +-
> security/safesetid/lsm.c | 272 +++++++++++++++++---
> security/safesetid/lsm.h | 11 +-
> security/safesetid/securityfs.c | 105 +++++---
> 4 files changed, 320 insertions(+), 77 deletions(-)
>
> diff --git a/Documentation/admin-guide/LSM/SafeSetID.rst b/Documentation/admin-guide/LSM/SafeSetID.rst
> index 670a6544fd39..33307a8e9555 100644
> --- a/Documentation/admin-guide/LSM/SafeSetID.rst
> +++ b/Documentation/admin-guide/LSM/SafeSetID.rst
> @@ -98,10 +98,13 @@ Directions for use
> ==================
> This LSM hooks the setid syscalls to make sure transitions are allowed if an
> applicable restriction policy is in place. Policies are configured through
> -securityfs by writing to the safesetid/add_whitelist_policy and
> +securityfs by writing to the safesetid/add_whitelist_{uid/gid}_policy* and
> safesetid/flush_whitelist_policies files at the location where securityfs is
> -mounted. The format for adding a policy is '<UID>:<UID>', using literal
> +mounted. The format for adding a policy is '<ID>:<ID>', using literal
> numbers, such as '123:456'. To flush the policies, any write to the file is
> sufficient. Again, configuring a policy for a UID will prevent that UID from
> obtaining auxiliary setid privileges, such as allowing a user to set up user
> -namespace UID mappings.
> +namespace ID mappings.
> +
> +*safesetid/add_whitelist_policy can also be used for UID policies, since it is
> +an alias for safesetid/add_whitelist_uid_policy
> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index 5deffa92f25f..95c6f35b29f3 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -26,27 +26,30 @@ int safesetid_initialized;
>
> #define NUM_BITS 8 /* 128 buckets in hash table */
>
> -static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS);
> +static DEFINE_HASHTABLE(safesetid_whitelist_uid_hashtable, NUM_BITS);
> +static DEFINE_HASHTABLE(safesetid_whitelist_gid_hashtable, NUM_BITS);
> +
> +static DEFINE_SPINLOCK(safesetid_whitelist_uid_hashtable_spinlock);
> +static DEFINE_SPINLOCK(safesetid_whitelist_gid_hashtable_spinlock);
>
> /*
> * Hash table entry to store safesetid policy signifying that 'parent' user
> - * can setid to 'child' user.
> + * can setid to 'child' user. This struct is used in both the uid and gid
> + * hashtables.
> */
> -struct entry {
> +struct id_entry {
> struct hlist_node next;
> struct hlist_node dlist; /* for deletion cleanup */
> uint64_t parent_kuid;
> - uint64_t child_kuid;
> + uint64_t child_kid; /* Represents either a UID or a GID */
> };
>
> -static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock);
> -
> static bool check_setuid_policy_hashtable_key(kuid_t parent)
> {
> - struct entry *entry;
> + struct id_entry *entry;
>
> rcu_read_lock();
> - hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> + hash_for_each_possible_rcu(safesetid_whitelist_uid_hashtable,
> entry, next, __kuid_val(parent)) {
> if (entry->parent_kuid == __kuid_val(parent)) {
> rcu_read_unlock();
> @@ -61,13 +64,49 @@ static bool check_setuid_policy_hashtable_key(kuid_t parent)
> static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
> kuid_t child)
> {
> - struct entry *entry;
> + struct id_entry *entry;
> +
> + rcu_read_lock();
> + hash_for_each_possible_rcu(safesetid_whitelist_uid_hashtable,
> + entry, next, __kuid_val(parent)) {
> + if (entry->parent_kuid == __kuid_val(parent) &&
> + entry->child_kid == __kuid_val(child)) {
> + rcu_read_unlock();
> + return true;
> + }
> + }
> + rcu_read_unlock();
> +
> + return false;
> +}
> +
> +static bool check_setgid_policy_hashtable_key(kuid_t parent)
> +{
> + struct id_entry *entry;
> +
> + rcu_read_lock();
> + hash_for_each_possible_rcu(safesetid_whitelist_gid_hashtable,
> + entry, next, __kuid_val(parent)) {
> + if (entry->parent_kuid == __kuid_val(parent)) {
> + rcu_read_unlock();
> + return true;
> + }
> + }
> + rcu_read_unlock();
> +
> + return false;
> +}
> +
> +static bool check_setgid_policy_hashtable_key_value(kuid_t parent,
> + kgid_t child)
> +{
> + struct id_entry *entry;
>
> rcu_read_lock();
> - hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> + hash_for_each_possible_rcu(safesetid_whitelist_gid_hashtable,
> entry, next, __kuid_val(parent)) {
> if (entry->parent_kuid == __kuid_val(parent) &&
> - entry->child_kuid == __kuid_val(child)) {
> + entry->child_kid == __kgid_val(child)) {
> rcu_read_unlock();
> return true;
> }
> @@ -77,6 +116,12 @@ static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
> return false;
> }
>
> +/*
> + * This hook causes the security_capable check to fail when there are
> + * restriction policies for a UID and the process is trying to do something
> + * (other than a setid transition) that is gated by CAP_SETUID/CAP_SETGID
> + * (e.g. allowing user to set up userns UID/GID mappings).
> + */
> static int safesetid_security_capable(const struct cred *cred,
> struct user_namespace *ns,
> int cap,
> @@ -85,17 +130,19 @@ static int safesetid_security_capable(const struct cred *cred,
> if (cap == CAP_SETUID &&
> check_setuid_policy_hashtable_key(cred->uid)) {
> 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",
> __kuid_val(cred->uid));
> return -1;
> }
> }
> + if (cap == CAP_SETGID &&
> + check_setgid_policy_hashtable_key(cred->uid)) {
> + if (!(opts & CAP_OPT_INSETID)) {
> + pr_warn("Operation requires CAP_SETGID, which is not available to UID %u for operations besides approved set*gid transitions",
> + __kuid_val(cred->uid));
> + return -1;
> + }
> + }
> return 0;
> }
>
> @@ -115,19 +162,48 @@ static int check_uid_transition(kuid_t parent, kuid_t child)
> return -EACCES;
> }
>
> +static int check_gid_transition(kuid_t parent, kgid_t child)
> +{
> + if (check_setgid_policy_hashtable_key_value(parent, child))
> + return 0;
> + pr_warn("Denied UID %d setting GID to %d",
> + __kuid_val(parent),
> + __kgid_val(child));
> + /*
> + * 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, current);
> + return -EACCES;
> +}
> +
> /*
> * Check whether there is either an exception for user under old cred struct to
> - * set*uid to user under new cred struct, or the UID transition is allowed (by
> - * Linux set*uid rules) even without CAP_SETUID.
> + * set*id to user under new cred struct, or the ID transition is allowed (by
> + * Linux set*id rules) even without CAP_SET*ID.
> */
> static int safesetid_task_fix_setid(struct cred *new,
> const struct cred *old,
> int flags)
> {
> + if (flags == LSM_SETUID_RE ||
> + flags == LSM_SETUID_ID ||
> + flags == LSM_SETUID_RES ||
> + flags == LSM_SETUID_FS) {
> + /* Do nothing if there are no setuid restrictions for this UID. */
> + if (!check_setuid_policy_hashtable_key(old->uid))
> + return 0;
> + }
>
> - /* Do nothing if there are no setuid restrictions for this UID. */
> - if (!check_setuid_policy_hashtable_key(old->uid))
> + if (flags == LSM_SETGID_RE ||
> + flags == LSM_SETGID_ID ||
> + flags == LSM_SETGID_RES ||
> + flags == LSM_SETGID_FS) {
> + /* Do nothing if there are no setgid restrictions for this UID. */
> + if (!check_setgid_policy_hashtable_key(old->uid))
> return 0;
> + }
>
> switch (flags) {
> case LSM_SETUID_RE:
> @@ -201,6 +277,77 @@ static int safesetid_task_fix_setid(struct cred *new,
> return check_uid_transition(old->fsuid, new->fsuid);
> }
> break;
> + case LSM_SETGID_RE:
> + /*
> + * Users for which setgid restrictions exist can only set the
> + * real GID to the real GID or the effective GID, unless an
> + * explicit whitelist policy allows the transition.
> + */
> + if (!gid_eq(old->gid, new->gid) &&
> + !gid_eq(old->egid, new->gid)) {
> + return check_gid_transition(old->uid, new->gid);
> + }
> + /*
> + * Users for which setgid restrictions exist can only set the
> + * effective GID to the real GID, the effective GID, or the
> + * saved set-GID, unless an explicit whitelist policy allows
> + * the transition.
> + */
> + if (!gid_eq(old->gid, new->egid) &&
> + !gid_eq(old->egid, new->egid) &&
> + !gid_eq(old->sgid, new->egid)) {
> + return check_gid_transition(old->euid, new->egid);
> + }
> + break;
> + case LSM_SETGID_ID:
> + /*
> + * Users for which setgid restrictions exist cannot change the
> + * real GID or saved set-GID unless an explicit whitelist
> + * policy allows the transition.
> + */
> + if (!gid_eq(old->gid, new->gid))
> + return check_gid_transition(old->uid, new->gid);
> + if (!gid_eq(old->sgid, new->sgid))
> + return check_gid_transition(old->suid, new->sgid);
> + break;
> + case LSM_SETGID_RES:
> + /*
> + * Users for which setgid restrictions exist cannot change the
> + * real GID, effective GID, or saved set-GID to anything but
> + * one of: the current real GID, the current effective GID or
> + * the current saved set-user-ID unless an explicit whitelist
> + * policy allows the transition.
> + */
> + if (!gid_eq(new->gid, old->gid) &&
> + !gid_eq(new->gid, old->egid) &&
> + !gid_eq(new->gid, old->sgid)) {
> + return check_gid_transition(old->uid, new->gid);
> + }
> + if (!gid_eq(new->egid, old->gid) &&
> + !gid_eq(new->egid, old->egid) &&
> + !gid_eq(new->egid, old->sgid)) {
> + return check_gid_transition(old->euid, new->egid);
> + }
> + if (!gid_eq(new->sgid, old->gid) &&
> + !gid_eq(new->sgid, old->egid) &&
> + !gid_eq(new->sgid, old->sgid)) {
> + return check_gid_transition(old->suid, new->sgid);
> + }
> + break;
> + case LSM_SETGID_FS:
> + /*
> + * Users for which setgid restrictions exist cannot change the
> + * filesystem GID to anything but one of: the current real GID,
> + * the current effective GID or the current saved set-GID
> + * unless an explicit whitelist policy allows the transition.
> + */
> + if (!gid_eq(new->fsgid, old->gid) &&
> + !gid_eq(new->fsgid, old->egid) &&
> + !gid_eq(new->fsgid, old->sgid) &&
> + !gid_eq(new->fsgid, old->fsgid)) {
> + return check_gid_transition(old->fsuid, new->fsgid);
> + }
> + break;
> default:
> pr_warn("Unknown setid state %d\n", flags);
> force_sig(SIGKILL, current);
> @@ -209,49 +356,96 @@ static int safesetid_task_fix_setid(struct cred *new,
> return 0;
> }
>
> -int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
> +int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child)
> {
> - struct entry *new;
> + struct id_entry *new;
>
> /* Return if entry already exists */
> if (check_setuid_policy_hashtable_key_value(parent, child))
> return 0;
>
> - new = kzalloc(sizeof(struct entry), GFP_KERNEL);
> + new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
> + if (!new)
> + return -ENOMEM;
> + new->parent_kuid = __kuid_val(parent);
> + new->child_kid = __kuid_val(child);
> + spin_lock(&safesetid_whitelist_uid_hashtable_spinlock);
> + /* Return if the entry got added since we checked above */
> + if (check_setuid_policy_hashtable_key_value(parent, child)) {
> + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> + kfree(new);
> + return 0;
> + }
> + hash_add_rcu(safesetid_whitelist_uid_hashtable,
> + &new->next,
> + __kuid_val(parent));
> + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> + return 0;
> +}
> +
> +int add_safesetid_whitelist_gid_entry(kuid_t parent, kgid_t child)
> +{
> + struct id_entry *new;
> +
> + /* Return if entry already exists */
> + if (check_setgid_policy_hashtable_key_value(parent, child))
> + return 0;
> +
> + new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
> if (!new)
> return -ENOMEM;
> new->parent_kuid = __kuid_val(parent);
> - new->child_kuid = __kuid_val(child);
> - spin_lock(&safesetid_whitelist_hashtable_spinlock);
> - hash_add_rcu(safesetid_whitelist_hashtable,
> + new->child_kid = __kgid_val(child);
> + spin_lock(&safesetid_whitelist_gid_hashtable_spinlock);
> + /* Return if the entry got added since we checked above */
> + if (check_setgid_policy_hashtable_key_value(parent, child)) {
> + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> + kfree(new);
> + return 0;
> + }
> + hash_add_rcu(safesetid_whitelist_gid_hashtable,
> &new->next,
> __kuid_val(parent));
> - spin_unlock(&safesetid_whitelist_hashtable_spinlock);
> + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> return 0;
> }
>
> void flush_safesetid_whitelist_entries(void)
> {
> - struct entry *entry;
> + struct id_entry *id_entry;
> struct hlist_node *hlist_node;
> unsigned int bkt_loop_cursor;
> - HLIST_HEAD(free_list);
> + HLIST_HEAD(uid_free_list);
> + HLIST_HEAD(gid_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_lock(&safesetid_whitelist_uid_hashtable_spinlock);
> + hash_for_each_safe(safesetid_whitelist_uid_hashtable, bkt_loop_cursor,
> + hlist_node, id_entry, next) {
> + hash_del_rcu(&id_entry->next);
> + hlist_add_head(&id_entry->dlist, &uid_free_list);
> + }
> + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> + synchronize_rcu();
> + hlist_for_each_entry_safe(id_entry, hlist_node, &uid_free_list, dlist) {
> + hlist_del(&id_entry->dlist);
> + kfree(id_entry);
> + }
> +
> + spin_lock(&safesetid_whitelist_gid_hashtable_spinlock);
> + hash_for_each_safe(safesetid_whitelist_gid_hashtable, bkt_loop_cursor,
> + hlist_node, id_entry, next) {
> + hash_del_rcu(&id_entry->next);
> + hlist_add_head(&id_entry->dlist, &gid_free_list);
> }
> - spin_unlock(&safesetid_whitelist_hashtable_spinlock);
> + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> synchronize_rcu();
> - hlist_for_each_entry_safe(entry, hlist_node, &free_list, dlist) {
> - hlist_del(&entry->dlist);
> - kfree(entry);
> + hlist_for_each_entry_safe(id_entry, hlist_node, &gid_free_list, dlist) {
> + hlist_del(&id_entry->dlist);
> + kfree(id_entry);
> }
> }
>
> diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> index c1ea3c265fcf..e9ae192caff2 100644
> --- a/security/safesetid/lsm.h
> +++ b/security/safesetid/lsm.h
> @@ -21,13 +21,16 @@ extern int safesetid_initialized;
>
> /* Function type. */
> enum safesetid_whitelist_file_write_type {
> - SAFESETID_WHITELIST_ADD, /* Add whitelist policy. */
> + SAFESETID_WHITELIST_ADD_UID, /* Add UID whitelist policy. */
> + SAFESETID_WHITELIST_ADD_GID, /* Add GID whitelist policy. */
> SAFESETID_WHITELIST_FLUSH, /* Flush whitelist policies. */
> };
>
> -/* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */
> -int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child);
> -
> +/* Add entry to safesetid whitelist to allow 'parent' to setuid to 'child'. */
> +int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child);
> +/* Add entry to safesetid whitelist to allow 'parent' to setgid to 'child'. */
> +int add_safesetid_whitelist_gid_entry(kgid_t parent, kgid_t child);
> +/* Flush all UID/GID whitelist policies. */
> void flush_safesetid_whitelist_entries(void);
>
> #endif /* _SAFESETID_H */
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 2c6c829be044..c4c25ba7275f 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -26,20 +26,19 @@ struct safesetid_file_entry {
>
> static struct safesetid_file_entry safesetid_files[] = {
> {.name = "add_whitelist_policy",
> - .type = SAFESETID_WHITELIST_ADD},
> + .type = SAFESETID_WHITELIST_ADD_UID},
> + {.name = "add_whitelist_uid_policy",
> + .type = SAFESETID_WHITELIST_ADD_UID},
> + {.name = "add_whitelist_gid_policy",
> + .type = SAFESETID_WHITELIST_ADD_GID},
> {.name = "flush_whitelist_policies",
> .type = SAFESETID_WHITELIST_FLUSH},
> };
>
> -/*
> - * 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
> - * function will return an error.
> - */
> -static int parse_safesetid_whitelist_policy(const char __user *buf,
> +static int parse_userbuf_to_longs(const char __user *buf,
> size_t len,
> - kuid_t *parent,
> - kuid_t *child)
> + long *parent,
> + long *child)
> {
> char *kern_buf;
> char *parent_buf;
> @@ -47,8 +46,6 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> const char separator[] = ":";
> int ret;
> size_t first_substring_length;
> - long parsed_parent;
> - long parsed_child;
>
> /* Duplicate string from user memory and NULL-terminate */
> kern_buf = memdup_user_nul(buf, len);
> @@ -71,27 +68,15 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> goto free_kern;
> }
>
> - ret = kstrtol(parent_buf, 0, &parsed_parent);
> + ret = kstrtol(parent_buf, 0, parent);
> if (ret)
> goto free_both;
>
> child_buf = kern_buf + first_substring_length + 1;
> - ret = kstrtol(child_buf, 0, &parsed_child);
> + ret = kstrtol(child_buf, 0, child);
> if (ret)
> goto free_both;
>
> - *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;
> - }
> -
> free_both:
> kfree(parent_buf);
> free_kern:
> @@ -99,6 +84,52 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> return ret;
> }
>
> +static int parse_safesetid_whitelist_uid_policy(const char __user *buf,
> + size_t len,
> + kuid_t *parent_uid,
> + kuid_t *child_uid)
> +{
> + int ret;
> + long parent, child;
> +
> + ret = parse_userbuf_to_longs(buf, len, &parent, &child);
> + if (ret)
> + return ret;
> +
> + *parent_uid = make_kuid(current_user_ns(), parent);
> + if (!uid_valid(*parent_uid))
> + return -EINVAL;
> +
> + *child_uid = make_kuid(current_user_ns(), child);
> + if (!uid_valid(*child_uid))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int parse_safesetid_whitelist_gid_policy(const char __user *buf,
> + size_t len,
> + kgid_t *parent_gid,
> + kgid_t *child_gid)
> +{
> + int ret;
> + long parent, child;
> +
> + ret = parse_userbuf_to_longs(buf, len, &parent, &child);
> + if (ret)
> + return ret;
> +
> + *parent_gid = make_kgid(current_user_ns(), parent);
> + if (!gid_valid(*parent_gid))
> + return -EINVAL;
> +
> + *child_gid = make_kgid(current_user_ns(), child);
> + if (!gid_valid(*child_gid))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static ssize_t safesetid_file_write(struct file *file,
> const char __user *buf,
> size_t len,
> @@ -106,8 +137,10 @@ static ssize_t safesetid_file_write(struct file *file,
> {
> struct safesetid_file_entry *file_entry =
> file->f_inode->i_private;
> - kuid_t parent;
> - kuid_t child;
> + kuid_t uid_parent;
> + kuid_t uid_child;
> + kgid_t gid_parent;
> + kgid_t gid_child;
> int ret;
>
> if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN))
> @@ -120,13 +153,23 @@ static ssize_t safesetid_file_write(struct file *file,
> case SAFESETID_WHITELIST_FLUSH:
> flush_safesetid_whitelist_entries();
> break;
> - case SAFESETID_WHITELIST_ADD:
> - ret = parse_safesetid_whitelist_policy(buf, len, &parent,
> - &child);
> + case SAFESETID_WHITELIST_ADD_UID:
> + ret = parse_safesetid_whitelist_uid_policy(buf, len, &uid_parent,
> + &uid_child);
> + if (ret)
> + return ret;
> +
> + ret = add_safesetid_whitelist_uid_entry(uid_parent, uid_child);
> + if (ret)
> + return ret;
> + break;
> + case SAFESETID_WHITELIST_ADD_GID:
> + ret = parse_safesetid_whitelist_gid_policy(buf, len, &gid_parent,
> + &gid_child);
> if (ret)
> return ret;
>
> - ret = add_safesetid_whitelist_entry(parent, child);
> + ret = add_safesetid_whitelist_gid_entry(gid_parent, gid_child);
> if (ret)
> return ret;
> break;
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 2/2] LSM: SafeSetID: gate setgid transitions
2019-02-28 19:12 [PATCH v3 " Casey Schaufler
@ 2019-02-28 20:20 ` mortonm
2019-02-28 22:50 ` Casey Schaufler
0 siblings, 1 reply; 10+ messages in thread
From: mortonm @ 2019-02-28 20:20 UTC (permalink / raw)
To: jmorris, serge, keescook, casey, sds, linux-security-module; +Cc: Micah Morton
From: Micah Morton <mortonm@chromium.org>
The SafeSetID LSM already gates setuid transitions for UIDs on the
system whose use of CAP_SETUID has been 'restricted'. This patch
implements the analogous functionality for setgid transitions, in order
to restrict the use of CAP_SETGID for certain UIDs on the system. One
notable consequence of this addition is that a process running under a
restricted UID (i.e. one that is only allowed to setgid to certain
approved GIDs) will not be allowed to call the setgroups() syscall to
set its supplementary group IDs. For now, we leave such support for
restricted setgroups() to future work, as it would require hooking the
logic in setgroups() and verifying that the array of GIDs passed in from
userspace only consists of approved GIDs.
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
Changes since the last patch: Minor updates since patch 1/2 combined the
'task_fix_setuid' and 'task_fix_setgid' hooks into one hook called
'task_fix_setid'.
Documentation/admin-guide/LSM/SafeSetID.rst | 9 +-
security/safesetid/lsm.c | 272 +++++++++++++++++---
security/safesetid/lsm.h | 11 +-
security/safesetid/securityfs.c | 105 +++++---
4 files changed, 320 insertions(+), 77 deletions(-)
diff --git a/Documentation/admin-guide/LSM/SafeSetID.rst b/Documentation/admin-guide/LSM/SafeSetID.rst
index 670a6544fd39..33307a8e9555 100644
--- a/Documentation/admin-guide/LSM/SafeSetID.rst
+++ b/Documentation/admin-guide/LSM/SafeSetID.rst
@@ -98,10 +98,13 @@ Directions for use
==================
This LSM hooks the setid syscalls to make sure transitions are allowed if an
applicable restriction policy is in place. Policies are configured through
-securityfs by writing to the safesetid/add_whitelist_policy and
+securityfs by writing to the safesetid/add_whitelist_{uid/gid}_policy* and
safesetid/flush_whitelist_policies files at the location where securityfs is
-mounted. The format for adding a policy is '<UID>:<UID>', using literal
+mounted. The format for adding a policy is '<ID>:<ID>', using literal
numbers, such as '123:456'. To flush the policies, any write to the file is
sufficient. Again, configuring a policy for a UID will prevent that UID from
obtaining auxiliary setid privileges, such as allowing a user to set up user
-namespace UID mappings.
+namespace ID mappings.
+
+*safesetid/add_whitelist_policy can also be used for UID policies, since it is
+an alias for safesetid/add_whitelist_uid_policy
diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
index 5deffa92f25f..95c6f35b29f3 100644
--- a/security/safesetid/lsm.c
+++ b/security/safesetid/lsm.c
@@ -26,27 +26,30 @@ int safesetid_initialized;
#define NUM_BITS 8 /* 128 buckets in hash table */
-static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS);
+static DEFINE_HASHTABLE(safesetid_whitelist_uid_hashtable, NUM_BITS);
+static DEFINE_HASHTABLE(safesetid_whitelist_gid_hashtable, NUM_BITS);
+
+static DEFINE_SPINLOCK(safesetid_whitelist_uid_hashtable_spinlock);
+static DEFINE_SPINLOCK(safesetid_whitelist_gid_hashtable_spinlock);
/*
* Hash table entry to store safesetid policy signifying that 'parent' user
- * can setid to 'child' user.
+ * can setid to 'child' user. This struct is used in both the uid and gid
+ * hashtables.
*/
-struct entry {
+struct id_entry {
struct hlist_node next;
struct hlist_node dlist; /* for deletion cleanup */
uint64_t parent_kuid;
- uint64_t child_kuid;
+ uint64_t child_kid; /* Represents either a UID or a GID */
};
-static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock);
-
static bool check_setuid_policy_hashtable_key(kuid_t parent)
{
- struct entry *entry;
+ struct id_entry *entry;
rcu_read_lock();
- hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
+ hash_for_each_possible_rcu(safesetid_whitelist_uid_hashtable,
entry, next, __kuid_val(parent)) {
if (entry->parent_kuid == __kuid_val(parent)) {
rcu_read_unlock();
@@ -61,13 +64,49 @@ static bool check_setuid_policy_hashtable_key(kuid_t parent)
static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
kuid_t child)
{
- struct entry *entry;
+ struct id_entry *entry;
+
+ rcu_read_lock();
+ hash_for_each_possible_rcu(safesetid_whitelist_uid_hashtable,
+ entry, next, __kuid_val(parent)) {
+ if (entry->parent_kuid == __kuid_val(parent) &&
+ entry->child_kid == __kuid_val(child)) {
+ rcu_read_unlock();
+ return true;
+ }
+ }
+ rcu_read_unlock();
+
+ return false;
+}
+
+static bool check_setgid_policy_hashtable_key(kuid_t parent)
+{
+ struct id_entry *entry;
+
+ rcu_read_lock();
+ hash_for_each_possible_rcu(safesetid_whitelist_gid_hashtable,
+ entry, next, __kuid_val(parent)) {
+ if (entry->parent_kuid == __kuid_val(parent)) {
+ rcu_read_unlock();
+ return true;
+ }
+ }
+ rcu_read_unlock();
+
+ return false;
+}
+
+static bool check_setgid_policy_hashtable_key_value(kuid_t parent,
+ kgid_t child)
+{
+ struct id_entry *entry;
rcu_read_lock();
- hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
+ hash_for_each_possible_rcu(safesetid_whitelist_gid_hashtable,
entry, next, __kuid_val(parent)) {
if (entry->parent_kuid == __kuid_val(parent) &&
- entry->child_kuid == __kuid_val(child)) {
+ entry->child_kid == __kgid_val(child)) {
rcu_read_unlock();
return true;
}
@@ -77,6 +116,12 @@ static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
return false;
}
+/*
+ * This hook causes the security_capable check to fail when there are
+ * restriction policies for a UID and the process is trying to do something
+ * (other than a setid transition) that is gated by CAP_SETUID/CAP_SETGID
+ * (e.g. allowing user to set up userns UID/GID mappings).
+ */
static int safesetid_security_capable(const struct cred *cred,
struct user_namespace *ns,
int cap,
@@ -85,17 +130,19 @@ static int safesetid_security_capable(const struct cred *cred,
if (cap == CAP_SETUID &&
check_setuid_policy_hashtable_key(cred->uid)) {
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",
__kuid_val(cred->uid));
return -1;
}
}
+ if (cap == CAP_SETGID &&
+ check_setgid_policy_hashtable_key(cred->uid)) {
+ if (!(opts & CAP_OPT_INSETID)) {
+ pr_warn("Operation requires CAP_SETGID, which is not available to UID %u for operations besides approved set*gid transitions",
+ __kuid_val(cred->uid));
+ return -1;
+ }
+ }
return 0;
}
@@ -115,19 +162,48 @@ static int check_uid_transition(kuid_t parent, kuid_t child)
return -EACCES;
}
+static int check_gid_transition(kuid_t parent, kgid_t child)
+{
+ if (check_setgid_policy_hashtable_key_value(parent, child))
+ return 0;
+ pr_warn("Denied UID %d setting GID to %d",
+ __kuid_val(parent),
+ __kgid_val(child));
+ /*
+ * 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, current);
+ return -EACCES;
+}
+
/*
* Check whether there is either an exception for user under old cred struct to
- * set*uid to user under new cred struct, or the UID transition is allowed (by
- * Linux set*uid rules) even without CAP_SETUID.
+ * set*id to user under new cred struct, or the ID transition is allowed (by
+ * Linux set*id rules) even without CAP_SET*ID.
*/
static int safesetid_task_fix_setid(struct cred *new,
const struct cred *old,
int flags)
{
+ if (flags == LSM_SETUID_RE ||
+ flags == LSM_SETUID_ID ||
+ flags == LSM_SETUID_RES ||
+ flags == LSM_SETUID_FS) {
+ /* Do nothing if there are no setuid restrictions for this UID. */
+ if (!check_setuid_policy_hashtable_key(old->uid))
+ return 0;
+ }
- /* Do nothing if there are no setuid restrictions for this UID. */
- if (!check_setuid_policy_hashtable_key(old->uid))
+ if (flags == LSM_SETGID_RE ||
+ flags == LSM_SETGID_ID ||
+ flags == LSM_SETGID_RES ||
+ flags == LSM_SETGID_FS) {
+ /* Do nothing if there are no setgid restrictions for this UID. */
+ if (!check_setgid_policy_hashtable_key(old->uid))
return 0;
+ }
switch (flags) {
case LSM_SETUID_RE:
@@ -201,6 +277,77 @@ static int safesetid_task_fix_setid(struct cred *new,
return check_uid_transition(old->fsuid, new->fsuid);
}
break;
+ case LSM_SETGID_RE:
+ /*
+ * Users for which setgid restrictions exist can only set the
+ * real GID to the real GID or the effective GID, unless an
+ * explicit whitelist policy allows the transition.
+ */
+ if (!gid_eq(old->gid, new->gid) &&
+ !gid_eq(old->egid, new->gid)) {
+ return check_gid_transition(old->uid, new->gid);
+ }
+ /*
+ * Users for which setgid restrictions exist can only set the
+ * effective GID to the real GID, the effective GID, or the
+ * saved set-GID, unless an explicit whitelist policy allows
+ * the transition.
+ */
+ if (!gid_eq(old->gid, new->egid) &&
+ !gid_eq(old->egid, new->egid) &&
+ !gid_eq(old->sgid, new->egid)) {
+ return check_gid_transition(old->euid, new->egid);
+ }
+ break;
+ case LSM_SETGID_ID:
+ /*
+ * Users for which setgid restrictions exist cannot change the
+ * real GID or saved set-GID unless an explicit whitelist
+ * policy allows the transition.
+ */
+ if (!gid_eq(old->gid, new->gid))
+ return check_gid_transition(old->uid, new->gid);
+ if (!gid_eq(old->sgid, new->sgid))
+ return check_gid_transition(old->suid, new->sgid);
+ break;
+ case LSM_SETGID_RES:
+ /*
+ * Users for which setgid restrictions exist cannot change the
+ * real GID, effective GID, or saved set-GID to anything but
+ * one of: the current real GID, the current effective GID or
+ * the current saved set-user-ID unless an explicit whitelist
+ * policy allows the transition.
+ */
+ if (!gid_eq(new->gid, old->gid) &&
+ !gid_eq(new->gid, old->egid) &&
+ !gid_eq(new->gid, old->sgid)) {
+ return check_gid_transition(old->uid, new->gid);
+ }
+ if (!gid_eq(new->egid, old->gid) &&
+ !gid_eq(new->egid, old->egid) &&
+ !gid_eq(new->egid, old->sgid)) {
+ return check_gid_transition(old->euid, new->egid);
+ }
+ if (!gid_eq(new->sgid, old->gid) &&
+ !gid_eq(new->sgid, old->egid) &&
+ !gid_eq(new->sgid, old->sgid)) {
+ return check_gid_transition(old->suid, new->sgid);
+ }
+ break;
+ case LSM_SETGID_FS:
+ /*
+ * Users for which setgid restrictions exist cannot change the
+ * filesystem GID to anything but one of: the current real GID,
+ * the current effective GID or the current saved set-GID
+ * unless an explicit whitelist policy allows the transition.
+ */
+ if (!gid_eq(new->fsgid, old->gid) &&
+ !gid_eq(new->fsgid, old->egid) &&
+ !gid_eq(new->fsgid, old->sgid) &&
+ !gid_eq(new->fsgid, old->fsgid)) {
+ return check_gid_transition(old->fsuid, new->fsgid);
+ }
+ break;
default:
pr_warn("Unknown setid state %d\n", flags);
force_sig(SIGKILL, current);
@@ -209,49 +356,96 @@ static int safesetid_task_fix_setid(struct cred *new,
return 0;
}
-int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
+int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child)
{
- struct entry *new;
+ struct id_entry *new;
/* Return if entry already exists */
if (check_setuid_policy_hashtable_key_value(parent, child))
return 0;
- new = kzalloc(sizeof(struct entry), GFP_KERNEL);
+ new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
+ if (!new)
+ return -ENOMEM;
+ new->parent_kuid = __kuid_val(parent);
+ new->child_kid = __kuid_val(child);
+ spin_lock(&safesetid_whitelist_uid_hashtable_spinlock);
+ /* Return if the entry got added since we checked above */
+ if (check_setuid_policy_hashtable_key_value(parent, child)) {
+ spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
+ kfree(new);
+ return 0;
+ }
+ hash_add_rcu(safesetid_whitelist_uid_hashtable,
+ &new->next,
+ __kuid_val(parent));
+ spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
+ return 0;
+}
+
+int add_safesetid_whitelist_gid_entry(kuid_t parent, kgid_t child)
+{
+ struct id_entry *new;
+
+ /* Return if entry already exists */
+ if (check_setgid_policy_hashtable_key_value(parent, child))
+ return 0;
+
+ new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
if (!new)
return -ENOMEM;
new->parent_kuid = __kuid_val(parent);
- new->child_kuid = __kuid_val(child);
- spin_lock(&safesetid_whitelist_hashtable_spinlock);
- hash_add_rcu(safesetid_whitelist_hashtable,
+ new->child_kid = __kgid_val(child);
+ spin_lock(&safesetid_whitelist_gid_hashtable_spinlock);
+ /* Return if the entry got added since we checked above */
+ if (check_setgid_policy_hashtable_key_value(parent, child)) {
+ spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
+ kfree(new);
+ return 0;
+ }
+ hash_add_rcu(safesetid_whitelist_gid_hashtable,
&new->next,
__kuid_val(parent));
- spin_unlock(&safesetid_whitelist_hashtable_spinlock);
+ spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
return 0;
}
void flush_safesetid_whitelist_entries(void)
{
- struct entry *entry;
+ struct id_entry *id_entry;
struct hlist_node *hlist_node;
unsigned int bkt_loop_cursor;
- HLIST_HEAD(free_list);
+ HLIST_HEAD(uid_free_list);
+ HLIST_HEAD(gid_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_lock(&safesetid_whitelist_uid_hashtable_spinlock);
+ hash_for_each_safe(safesetid_whitelist_uid_hashtable, bkt_loop_cursor,
+ hlist_node, id_entry, next) {
+ hash_del_rcu(&id_entry->next);
+ hlist_add_head(&id_entry->dlist, &uid_free_list);
+ }
+ spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
+ synchronize_rcu();
+ hlist_for_each_entry_safe(id_entry, hlist_node, &uid_free_list, dlist) {
+ hlist_del(&id_entry->dlist);
+ kfree(id_entry);
+ }
+
+ spin_lock(&safesetid_whitelist_gid_hashtable_spinlock);
+ hash_for_each_safe(safesetid_whitelist_gid_hashtable, bkt_loop_cursor,
+ hlist_node, id_entry, next) {
+ hash_del_rcu(&id_entry->next);
+ hlist_add_head(&id_entry->dlist, &gid_free_list);
}
- spin_unlock(&safesetid_whitelist_hashtable_spinlock);
+ spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
synchronize_rcu();
- hlist_for_each_entry_safe(entry, hlist_node, &free_list, dlist) {
- hlist_del(&entry->dlist);
- kfree(entry);
+ hlist_for_each_entry_safe(id_entry, hlist_node, &gid_free_list, dlist) {
+ hlist_del(&id_entry->dlist);
+ kfree(id_entry);
}
}
diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
index c1ea3c265fcf..e9ae192caff2 100644
--- a/security/safesetid/lsm.h
+++ b/security/safesetid/lsm.h
@@ -21,13 +21,16 @@ extern int safesetid_initialized;
/* Function type. */
enum safesetid_whitelist_file_write_type {
- SAFESETID_WHITELIST_ADD, /* Add whitelist policy. */
+ SAFESETID_WHITELIST_ADD_UID, /* Add UID whitelist policy. */
+ SAFESETID_WHITELIST_ADD_GID, /* Add GID whitelist policy. */
SAFESETID_WHITELIST_FLUSH, /* Flush whitelist policies. */
};
-/* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */
-int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child);
-
+/* Add entry to safesetid whitelist to allow 'parent' to setuid to 'child'. */
+int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child);
+/* Add entry to safesetid whitelist to allow 'parent' to setgid to 'child'. */
+int add_safesetid_whitelist_gid_entry(kgid_t parent, kgid_t child);
+/* Flush all UID/GID whitelist policies. */
void flush_safesetid_whitelist_entries(void);
#endif /* _SAFESETID_H */
diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index 2c6c829be044..c4c25ba7275f 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -26,20 +26,19 @@ struct safesetid_file_entry {
static struct safesetid_file_entry safesetid_files[] = {
{.name = "add_whitelist_policy",
- .type = SAFESETID_WHITELIST_ADD},
+ .type = SAFESETID_WHITELIST_ADD_UID},
+ {.name = "add_whitelist_uid_policy",
+ .type = SAFESETID_WHITELIST_ADD_UID},
+ {.name = "add_whitelist_gid_policy",
+ .type = SAFESETID_WHITELIST_ADD_GID},
{.name = "flush_whitelist_policies",
.type = SAFESETID_WHITELIST_FLUSH},
};
-/*
- * 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
- * function will return an error.
- */
-static int parse_safesetid_whitelist_policy(const char __user *buf,
+static int parse_userbuf_to_longs(const char __user *buf,
size_t len,
- kuid_t *parent,
- kuid_t *child)
+ long *parent,
+ long *child)
{
char *kern_buf;
char *parent_buf;
@@ -47,8 +46,6 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
const char separator[] = ":";
int ret;
size_t first_substring_length;
- long parsed_parent;
- long parsed_child;
/* Duplicate string from user memory and NULL-terminate */
kern_buf = memdup_user_nul(buf, len);
@@ -71,27 +68,15 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
goto free_kern;
}
- ret = kstrtol(parent_buf, 0, &parsed_parent);
+ ret = kstrtol(parent_buf, 0, parent);
if (ret)
goto free_both;
child_buf = kern_buf + first_substring_length + 1;
- ret = kstrtol(child_buf, 0, &parsed_child);
+ ret = kstrtol(child_buf, 0, child);
if (ret)
goto free_both;
- *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;
- }
-
free_both:
kfree(parent_buf);
free_kern:
@@ -99,6 +84,52 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
return ret;
}
+static int parse_safesetid_whitelist_uid_policy(const char __user *buf,
+ size_t len,
+ kuid_t *parent_uid,
+ kuid_t *child_uid)
+{
+ int ret;
+ long parent, child;
+
+ ret = parse_userbuf_to_longs(buf, len, &parent, &child);
+ if (ret)
+ return ret;
+
+ *parent_uid = make_kuid(current_user_ns(), parent);
+ if (!uid_valid(*parent_uid))
+ return -EINVAL;
+
+ *child_uid = make_kuid(current_user_ns(), child);
+ if (!uid_valid(*child_uid))
+ return -EINVAL;
+
+ return 0;
+}
+
+static int parse_safesetid_whitelist_gid_policy(const char __user *buf,
+ size_t len,
+ kgid_t *parent_gid,
+ kgid_t *child_gid)
+{
+ int ret;
+ long parent, child;
+
+ ret = parse_userbuf_to_longs(buf, len, &parent, &child);
+ if (ret)
+ return ret;
+
+ *parent_gid = make_kgid(current_user_ns(), parent);
+ if (!gid_valid(*parent_gid))
+ return -EINVAL;
+
+ *child_gid = make_kgid(current_user_ns(), child);
+ if (!gid_valid(*child_gid))
+ return -EINVAL;
+
+ return 0;
+}
+
static ssize_t safesetid_file_write(struct file *file,
const char __user *buf,
size_t len,
@@ -106,8 +137,10 @@ static ssize_t safesetid_file_write(struct file *file,
{
struct safesetid_file_entry *file_entry =
file->f_inode->i_private;
- kuid_t parent;
- kuid_t child;
+ kuid_t uid_parent;
+ kuid_t uid_child;
+ kgid_t gid_parent;
+ kgid_t gid_child;
int ret;
if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN))
@@ -120,13 +153,23 @@ static ssize_t safesetid_file_write(struct file *file,
case SAFESETID_WHITELIST_FLUSH:
flush_safesetid_whitelist_entries();
break;
- case SAFESETID_WHITELIST_ADD:
- ret = parse_safesetid_whitelist_policy(buf, len, &parent,
- &child);
+ case SAFESETID_WHITELIST_ADD_UID:
+ ret = parse_safesetid_whitelist_uid_policy(buf, len, &uid_parent,
+ &uid_child);
+ if (ret)
+ return ret;
+
+ ret = add_safesetid_whitelist_uid_entry(uid_parent, uid_child);
+ if (ret)
+ return ret;
+ break;
+ case SAFESETID_WHITELIST_ADD_GID:
+ ret = parse_safesetid_whitelist_gid_policy(buf, len, &gid_parent,
+ &gid_child);
if (ret)
return ret;
- ret = add_safesetid_whitelist_entry(parent, child);
+ ret = add_safesetid_whitelist_gid_entry(gid_parent, gid_child);
if (ret)
return ret;
break;
--
2.21.0.352.gf09ad66450-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-04-10 17:21 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-05 15:49 [PATCH v5 1/2] LSM: SafeSetID: gate setgid transitions mortonm
2019-03-05 15:52 ` [PATCH v4 2/2] " mortonm
2019-03-11 17:25 ` Micah Morton
2019-03-11 18:38 ` James Morris
2019-03-29 18:06 ` [PATCH v5 1/2] " James Morris
2019-03-29 19:44 ` Casey Schaufler
2019-04-10 15:14 ` Micah Morton
2019-04-10 17:21 ` James Morris
-- strict thread matches above, loose matches on Subject: below --
2019-02-28 19:12 [PATCH v3 " Casey Schaufler
2019-02-28 20:20 ` [PATCH v4 2/2] " mortonm
2019-02-28 22:50 ` Casey Schaufler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).