Commit 5cd9c58fbe9ec92b45b27e131719af4f2bd9eb40
Committed by
James Morris
1 parent
8d0968abd0
Exists in
master
and in
4 other branches
security: Fix setting of PF_SUPERPRIV by __capable()
Fix the setting of PF_SUPERPRIV by __capable() as it could corrupt the flags the target process if that is not the current process and it is trying to change its own flags in a different way at the same time. __capable() is using neither atomic ops nor locking to protect t->flags. This patch removes __capable() and introduces has_capability() that doesn't set PF_SUPERPRIV on the process being queried. This patch further splits security_ptrace() in two: (1) security_ptrace_may_access(). This passes judgement on whether one process may access another only (PTRACE_MODE_ATTACH for ptrace() and PTRACE_MODE_READ for /proc), and takes a pointer to the child process. current is the parent. (2) security_ptrace_traceme(). This passes judgement on PTRACE_TRACEME only, and takes only a pointer to the parent process. current is the child. In Smack and commoncap, this uses has_capability() to determine whether the parent will be permitted to use PTRACE_ATTACH if normal checks fail. This does not set PF_SUPERPRIV. Two of the instances of __capable() actually only act on current, and so have been changed to calls to capable(). Of the places that were using __capable(): (1) The OOM killer calls __capable() thrice when weighing the killability of a process. All of these now use has_capability(). (2) cap_ptrace() and smack_ptrace() were using __capable() to check to see whether the parent was allowed to trace any process. As mentioned above, these have been split. For PTRACE_ATTACH and /proc, capable() is now used, and for PTRACE_TRACEME, has_capability() is used. (3) cap_safe_nice() only ever saw current, so now uses capable(). (4) smack_setprocattr() rejected accesses to tasks other than current just after calling __capable(), so the order of these two tests have been switched and capable() is used instead. (5) In smack_file_send_sigiotask(), we need to allow privileged processes to receive SIGIO on files they're manipulating. (6) In smack_task_wait(), we let a process wait for a privileged process, whether or not the process doing the waiting is privileged. I've tested this with the LTP SELinux and syscalls testscripts. Signed-off-by: David Howells <dhowells@redhat.com> Acked-by: Serge Hallyn <serue@us.ibm.com> Acked-by: Casey Schaufler <casey@schaufler-ca.com> Acked-by: Andrew G. Morgan <morgan@kernel.org> Acked-by: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: James Morris <jmorris@namei.org>
Showing 11 changed files with 137 additions and 63 deletions Side-by-side Diff
include/linux/capability.h
... | ... | @@ -503,8 +503,19 @@ |
503 | 503 | |
504 | 504 | kernel_cap_t cap_set_effective(const kernel_cap_t pE_new); |
505 | 505 | |
506 | -int capable(int cap); | |
507 | -int __capable(struct task_struct *t, int cap); | |
506 | +/** | |
507 | + * has_capability - Determine if a task has a superior capability available | |
508 | + * @t: The task in question | |
509 | + * @cap: The capability to be tested for | |
510 | + * | |
511 | + * Return true if the specified task has the given superior capability | |
512 | + * currently in effect, false if not. | |
513 | + * | |
514 | + * Note that this does not set PF_SUPERPRIV on the task. | |
515 | + */ | |
516 | +#define has_capability(t, cap) (security_capable((t), (cap)) == 0) | |
517 | + | |
518 | +extern int capable(int cap); | |
508 | 519 | |
509 | 520 | #endif /* __KERNEL__ */ |
510 | 521 |
include/linux/security.h
... | ... | @@ -46,8 +46,8 @@ |
46 | 46 | */ |
47 | 47 | extern int cap_capable(struct task_struct *tsk, int cap); |
48 | 48 | extern int cap_settime(struct timespec *ts, struct timezone *tz); |
49 | -extern int cap_ptrace(struct task_struct *parent, struct task_struct *child, | |
50 | - unsigned int mode); | |
49 | +extern int cap_ptrace_may_access(struct task_struct *child, unsigned int mode); | |
50 | +extern int cap_ptrace_traceme(struct task_struct *parent); | |
51 | 51 | extern int cap_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted); |
52 | 52 | extern int cap_capset_check(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted); |
53 | 53 | extern void cap_capset_set(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted); |
54 | 54 | |
55 | 55 | |
... | ... | @@ -1157,17 +1157,24 @@ |
1157 | 1157 | * @alter contains the flag indicating whether changes are to be made. |
1158 | 1158 | * Return 0 if permission is granted. |
1159 | 1159 | * |
1160 | - * @ptrace: | |
1161 | - * Check permission before allowing the @parent process to trace the | |
1160 | + * @ptrace_may_access: | |
1161 | + * Check permission before allowing the current process to trace the | |
1162 | 1162 | * @child process. |
1163 | 1163 | * Security modules may also want to perform a process tracing check |
1164 | 1164 | * during an execve in the set_security or apply_creds hooks of |
1165 | 1165 | * binprm_security_ops if the process is being traced and its security |
1166 | 1166 | * attributes would be changed by the execve. |
1167 | - * @parent contains the task_struct structure for parent process. | |
1168 | - * @child contains the task_struct structure for child process. | |
1167 | + * @child contains the task_struct structure for the target process. | |
1169 | 1168 | * @mode contains the PTRACE_MODE flags indicating the form of access. |
1170 | 1169 | * Return 0 if permission is granted. |
1170 | + * @ptrace_traceme: | |
1171 | + * Check that the @parent process has sufficient permission to trace the | |
1172 | + * current process before allowing the current process to present itself | |
1173 | + * to the @parent process for tracing. | |
1174 | + * The parent process will still have to undergo the ptrace_may_access | |
1175 | + * checks before it is allowed to trace this one. | |
1176 | + * @parent contains the task_struct structure for debugger process. | |
1177 | + * Return 0 if permission is granted. | |
1171 | 1178 | * @capget: |
1172 | 1179 | * Get the @effective, @inheritable, and @permitted capability sets for |
1173 | 1180 | * the @target process. The hook may also perform permission checking to |
... | ... | @@ -1287,8 +1294,8 @@ |
1287 | 1294 | struct security_operations { |
1288 | 1295 | char name[SECURITY_NAME_MAX + 1]; |
1289 | 1296 | |
1290 | - int (*ptrace) (struct task_struct *parent, struct task_struct *child, | |
1291 | - unsigned int mode); | |
1297 | + int (*ptrace_may_access) (struct task_struct *child, unsigned int mode); | |
1298 | + int (*ptrace_traceme) (struct task_struct *parent); | |
1292 | 1299 | int (*capget) (struct task_struct *target, |
1293 | 1300 | kernel_cap_t *effective, |
1294 | 1301 | kernel_cap_t *inheritable, kernel_cap_t *permitted); |
... | ... | @@ -1560,8 +1567,8 @@ |
1560 | 1567 | extern void securityfs_remove(struct dentry *dentry); |
1561 | 1568 | |
1562 | 1569 | /* Security operations */ |
1563 | -int security_ptrace(struct task_struct *parent, struct task_struct *child, | |
1564 | - unsigned int mode); | |
1570 | +int security_ptrace_may_access(struct task_struct *child, unsigned int mode); | |
1571 | +int security_ptrace_traceme(struct task_struct *parent); | |
1565 | 1572 | int security_capget(struct task_struct *target, |
1566 | 1573 | kernel_cap_t *effective, |
1567 | 1574 | kernel_cap_t *inheritable, |
1568 | 1575 | |
... | ... | @@ -1742,11 +1749,15 @@ |
1742 | 1749 | return 0; |
1743 | 1750 | } |
1744 | 1751 | |
1745 | -static inline int security_ptrace(struct task_struct *parent, | |
1746 | - struct task_struct *child, | |
1747 | - unsigned int mode) | |
1752 | +static inline int security_ptrace_may_access(struct task_struct *child, | |
1753 | + unsigned int mode) | |
1748 | 1754 | { |
1749 | - return cap_ptrace(parent, child, mode); | |
1755 | + return cap_ptrace_may_access(child, mode); | |
1756 | +} | |
1757 | + | |
1758 | +static inline int security_ptrace_traceme(struct task_struct *child) | |
1759 | +{ | |
1760 | + return cap_ptrace_traceme(parent); | |
1750 | 1761 | } |
1751 | 1762 | |
1752 | 1763 | static inline int security_capget(struct task_struct *target, |
kernel/capability.c
... | ... | @@ -486,18 +486,23 @@ |
486 | 486 | return ret; |
487 | 487 | } |
488 | 488 | |
489 | -int __capable(struct task_struct *t, int cap) | |
489 | +/** | |
490 | + * capable - Determine if the current task has a superior capability in effect | |
491 | + * @cap: The capability to be tested for | |
492 | + * | |
493 | + * Return true if the current task has the given superior capability currently | |
494 | + * available for use, false if not. | |
495 | + * | |
496 | + * This sets PF_SUPERPRIV on the task if the capability is available on the | |
497 | + * assumption that it's about to be used. | |
498 | + */ | |
499 | +int capable(int cap) | |
490 | 500 | { |
491 | - if (security_capable(t, cap) == 0) { | |
492 | - t->flags |= PF_SUPERPRIV; | |
501 | + if (has_capability(current, cap)) { | |
502 | + current->flags |= PF_SUPERPRIV; | |
493 | 503 | return 1; |
494 | 504 | } |
495 | 505 | return 0; |
496 | -} | |
497 | - | |
498 | -int capable(int cap) | |
499 | -{ | |
500 | - return __capable(current, cap); | |
501 | 506 | } |
502 | 507 | EXPORT_SYMBOL(capable); |
kernel/ptrace.c
... | ... | @@ -140,7 +140,7 @@ |
140 | 140 | if (!dumpable && !capable(CAP_SYS_PTRACE)) |
141 | 141 | return -EPERM; |
142 | 142 | |
143 | - return security_ptrace(current, task, mode); | |
143 | + return security_ptrace_may_access(task, mode); | |
144 | 144 | } |
145 | 145 | |
146 | 146 | bool ptrace_may_access(struct task_struct *task, unsigned int mode) |
... | ... | @@ -499,8 +499,7 @@ |
499 | 499 | goto repeat; |
500 | 500 | } |
501 | 501 | |
502 | - ret = security_ptrace(current->parent, current, | |
503 | - PTRACE_MODE_ATTACH); | |
502 | + ret = security_ptrace_traceme(current->parent); | |
504 | 503 | |
505 | 504 | /* |
506 | 505 | * Set the ptrace bit in the process ptrace flags. |
mm/oom_kill.c
... | ... | @@ -26,6 +26,7 @@ |
26 | 26 | #include <linux/module.h> |
27 | 27 | #include <linux/notifier.h> |
28 | 28 | #include <linux/memcontrol.h> |
29 | +#include <linux/security.h> | |
29 | 30 | |
30 | 31 | int sysctl_panic_on_oom; |
31 | 32 | int sysctl_oom_kill_allocating_task; |
... | ... | @@ -128,7 +129,8 @@ |
128 | 129 | * Superuser processes are usually more important, so we make it |
129 | 130 | * less likely that we kill those. |
130 | 131 | */ |
131 | - if (__capable(p, CAP_SYS_ADMIN) || __capable(p, CAP_SYS_RESOURCE)) | |
132 | + if (has_capability(p, CAP_SYS_ADMIN) || | |
133 | + has_capability(p, CAP_SYS_RESOURCE)) | |
132 | 134 | points /= 4; |
133 | 135 | |
134 | 136 | /* |
... | ... | @@ -137,7 +139,7 @@ |
137 | 139 | * tend to only have this flag set on applications they think |
138 | 140 | * of as important. |
139 | 141 | */ |
140 | - if (__capable(p, CAP_SYS_RAWIO)) | |
142 | + if (has_capability(p, CAP_SYS_RAWIO)) | |
141 | 143 | points /= 4; |
142 | 144 | |
143 | 145 | /* |
security/capability.c
... | ... | @@ -811,7 +811,8 @@ |
811 | 811 | |
812 | 812 | void security_fixup_ops(struct security_operations *ops) |
813 | 813 | { |
814 | - set_to_cap_if_null(ops, ptrace); | |
814 | + set_to_cap_if_null(ops, ptrace_may_access); | |
815 | + set_to_cap_if_null(ops, ptrace_traceme); | |
815 | 816 | set_to_cap_if_null(ops, capget); |
816 | 817 | set_to_cap_if_null(ops, capset_check); |
817 | 818 | set_to_cap_if_null(ops, capset_set); |
security/commoncap.c
... | ... | @@ -63,16 +63,26 @@ |
63 | 63 | return 0; |
64 | 64 | } |
65 | 65 | |
66 | -int cap_ptrace (struct task_struct *parent, struct task_struct *child, | |
67 | - unsigned int mode) | |
66 | +int cap_ptrace_may_access(struct task_struct *child, unsigned int mode) | |
68 | 67 | { |
69 | 68 | /* Derived from arch/i386/kernel/ptrace.c:sys_ptrace. */ |
70 | - if (!cap_issubset(child->cap_permitted, parent->cap_permitted) && | |
71 | - !__capable(parent, CAP_SYS_PTRACE)) | |
72 | - return -EPERM; | |
73 | - return 0; | |
69 | + if (cap_issubset(child->cap_permitted, current->cap_permitted)) | |
70 | + return 0; | |
71 | + if (capable(CAP_SYS_PTRACE)) | |
72 | + return 0; | |
73 | + return -EPERM; | |
74 | 74 | } |
75 | 75 | |
76 | +int cap_ptrace_traceme(struct task_struct *parent) | |
77 | +{ | |
78 | + /* Derived from arch/i386/kernel/ptrace.c:sys_ptrace. */ | |
79 | + if (cap_issubset(current->cap_permitted, parent->cap_permitted)) | |
80 | + return 0; | |
81 | + if (has_capability(parent, CAP_SYS_PTRACE)) | |
82 | + return 0; | |
83 | + return -EPERM; | |
84 | +} | |
85 | + | |
76 | 86 | int cap_capget (struct task_struct *target, kernel_cap_t *effective, |
77 | 87 | kernel_cap_t *inheritable, kernel_cap_t *permitted) |
78 | 88 | { |
... | ... | @@ -534,7 +544,7 @@ |
534 | 544 | static inline int cap_safe_nice(struct task_struct *p) |
535 | 545 | { |
536 | 546 | if (!cap_issubset(p->cap_permitted, current->cap_permitted) && |
537 | - !__capable(current, CAP_SYS_NICE)) | |
547 | + !capable(CAP_SYS_NICE)) | |
538 | 548 | return -EPERM; |
539 | 549 | return 0; |
540 | 550 | } |
security/root_plug.c
... | ... | @@ -72,7 +72,8 @@ |
72 | 72 | |
73 | 73 | static struct security_operations rootplug_security_ops = { |
74 | 74 | /* Use the capability functions for some of the hooks */ |
75 | - .ptrace = cap_ptrace, | |
75 | + .ptrace_may_access = cap_ptrace_may_access, | |
76 | + .ptrace_traceme = cap_ptrace_traceme, | |
76 | 77 | .capget = cap_capget, |
77 | 78 | .capset_check = cap_capset_check, |
78 | 79 | .capset_set = cap_capset_set, |
security/security.c
... | ... | @@ -127,10 +127,14 @@ |
127 | 127 | |
128 | 128 | /* Security operations */ |
129 | 129 | |
130 | -int security_ptrace(struct task_struct *parent, struct task_struct *child, | |
131 | - unsigned int mode) | |
130 | +int security_ptrace_may_access(struct task_struct *child, unsigned int mode) | |
132 | 131 | { |
133 | - return security_ops->ptrace(parent, child, mode); | |
132 | + return security_ops->ptrace_may_access(child, mode); | |
133 | +} | |
134 | + | |
135 | +int security_ptrace_traceme(struct task_struct *parent) | |
136 | +{ | |
137 | + return security_ops->ptrace_traceme(parent); | |
134 | 138 | } |
135 | 139 | |
136 | 140 | int security_capget(struct task_struct *target, |
security/selinux/hooks.c
... | ... | @@ -1738,26 +1738,36 @@ |
1738 | 1738 | |
1739 | 1739 | /* Hook functions begin here. */ |
1740 | 1740 | |
1741 | -static int selinux_ptrace(struct task_struct *parent, | |
1742 | - struct task_struct *child, | |
1743 | - unsigned int mode) | |
1741 | +static int selinux_ptrace_may_access(struct task_struct *child, | |
1742 | + unsigned int mode) | |
1744 | 1743 | { |
1745 | 1744 | int rc; |
1746 | 1745 | |
1747 | - rc = secondary_ops->ptrace(parent, child, mode); | |
1746 | + rc = secondary_ops->ptrace_may_access(child, mode); | |
1748 | 1747 | if (rc) |
1749 | 1748 | return rc; |
1750 | 1749 | |
1751 | 1750 | if (mode == PTRACE_MODE_READ) { |
1752 | - struct task_security_struct *tsec = parent->security; | |
1751 | + struct task_security_struct *tsec = current->security; | |
1753 | 1752 | struct task_security_struct *csec = child->security; |
1754 | 1753 | return avc_has_perm(tsec->sid, csec->sid, |
1755 | 1754 | SECCLASS_FILE, FILE__READ, NULL); |
1756 | 1755 | } |
1757 | 1756 | |
1758 | - return task_has_perm(parent, child, PROCESS__PTRACE); | |
1757 | + return task_has_perm(current, child, PROCESS__PTRACE); | |
1759 | 1758 | } |
1760 | 1759 | |
1760 | +static int selinux_ptrace_traceme(struct task_struct *parent) | |
1761 | +{ | |
1762 | + int rc; | |
1763 | + | |
1764 | + rc = secondary_ops->ptrace_traceme(parent); | |
1765 | + if (rc) | |
1766 | + return rc; | |
1767 | + | |
1768 | + return task_has_perm(parent, current, PROCESS__PTRACE); | |
1769 | +} | |
1770 | + | |
1761 | 1771 | static int selinux_capget(struct task_struct *target, kernel_cap_t *effective, |
1762 | 1772 | kernel_cap_t *inheritable, kernel_cap_t *permitted) |
1763 | 1773 | { |
... | ... | @@ -5346,7 +5356,8 @@ |
5346 | 5356 | static struct security_operations selinux_ops = { |
5347 | 5357 | .name = "selinux", |
5348 | 5358 | |
5349 | - .ptrace = selinux_ptrace, | |
5359 | + .ptrace_may_access = selinux_ptrace_may_access, | |
5360 | + .ptrace_traceme = selinux_ptrace_traceme, | |
5350 | 5361 | .capget = selinux_capget, |
5351 | 5362 | .capset_check = selinux_capset_check, |
5352 | 5363 | .capset_set = selinux_capset_set, |
security/smack/smack_lsm.c
... | ... | @@ -87,27 +87,46 @@ |
87 | 87 | */ |
88 | 88 | |
89 | 89 | /** |
90 | - * smack_ptrace - Smack approval on ptrace | |
91 | - * @ptp: parent task pointer | |
90 | + * smack_ptrace_may_access - Smack approval on PTRACE_ATTACH | |
92 | 91 | * @ctp: child task pointer |
93 | 92 | * |
94 | 93 | * Returns 0 if access is OK, an error code otherwise |
95 | 94 | * |
96 | 95 | * Do the capability checks, and require read and write. |
97 | 96 | */ |
98 | -static int smack_ptrace(struct task_struct *ptp, struct task_struct *ctp, | |
99 | - unsigned int mode) | |
97 | +static int smack_ptrace_may_access(struct task_struct *ctp, unsigned int mode) | |
100 | 98 | { |
101 | 99 | int rc; |
102 | 100 | |
103 | - rc = cap_ptrace(ptp, ctp, mode); | |
101 | + rc = cap_ptrace_may_access(ctp, mode); | |
104 | 102 | if (rc != 0) |
105 | 103 | return rc; |
106 | 104 | |
107 | - rc = smk_access(ptp->security, ctp->security, MAY_READWRITE); | |
108 | - if (rc != 0 && __capable(ptp, CAP_MAC_OVERRIDE)) | |
105 | + rc = smk_access(current->security, ctp->security, MAY_READWRITE); | |
106 | + if (rc != 0 && capable(CAP_MAC_OVERRIDE)) | |
109 | 107 | return 0; |
108 | + return rc; | |
109 | +} | |
110 | 110 | |
111 | +/** | |
112 | + * smack_ptrace_traceme - Smack approval on PTRACE_TRACEME | |
113 | + * @ptp: parent task pointer | |
114 | + * | |
115 | + * Returns 0 if access is OK, an error code otherwise | |
116 | + * | |
117 | + * Do the capability checks, and require read and write. | |
118 | + */ | |
119 | +static int smack_ptrace_traceme(struct task_struct *ptp) | |
120 | +{ | |
121 | + int rc; | |
122 | + | |
123 | + rc = cap_ptrace_traceme(ptp); | |
124 | + if (rc != 0) | |
125 | + return rc; | |
126 | + | |
127 | + rc = smk_access(ptp->security, current->security, MAY_READWRITE); | |
128 | + if (rc != 0 && has_capability(ptp, CAP_MAC_OVERRIDE)) | |
129 | + return 0; | |
111 | 130 | return rc; |
112 | 131 | } |
113 | 132 | |
... | ... | @@ -923,7 +942,7 @@ |
923 | 942 | */ |
924 | 943 | file = container_of(fown, struct file, f_owner); |
925 | 944 | rc = smk_access(file->f_security, tsk->security, MAY_WRITE); |
926 | - if (rc != 0 && __capable(tsk, CAP_MAC_OVERRIDE)) | |
945 | + if (rc != 0 && has_capability(tsk, CAP_MAC_OVERRIDE)) | |
927 | 946 | return 0; |
928 | 947 | return rc; |
929 | 948 | } |
930 | 949 | |
... | ... | @@ -1164,12 +1183,12 @@ |
1164 | 1183 | * account for the smack labels having gotten to |
1165 | 1184 | * be different in the first place. |
1166 | 1185 | * |
1167 | - * This breaks the strict subjet/object access | |
1186 | + * This breaks the strict subject/object access | |
1168 | 1187 | * control ideal, taking the object's privilege |
1169 | 1188 | * state into account in the decision as well as |
1170 | 1189 | * the smack value. |
1171 | 1190 | */ |
1172 | - if (capable(CAP_MAC_OVERRIDE) || __capable(p, CAP_MAC_OVERRIDE)) | |
1191 | + if (capable(CAP_MAC_OVERRIDE) || has_capability(p, CAP_MAC_OVERRIDE)) | |
1173 | 1192 | return 0; |
1174 | 1193 | |
1175 | 1194 | return rc; |
... | ... | @@ -2016,9 +2035,6 @@ |
2016 | 2035 | { |
2017 | 2036 | char *newsmack; |
2018 | 2037 | |
2019 | - if (!__capable(p, CAP_MAC_ADMIN)) | |
2020 | - return -EPERM; | |
2021 | - | |
2022 | 2038 | /* |
2023 | 2039 | * Changing another process' Smack value is too dangerous |
2024 | 2040 | * and supports no sane use case. |
... | ... | @@ -2026,6 +2042,9 @@ |
2026 | 2042 | if (p != current) |
2027 | 2043 | return -EPERM; |
2028 | 2044 | |
2045 | + if (!capable(CAP_MAC_ADMIN)) | |
2046 | + return -EPERM; | |
2047 | + | |
2029 | 2048 | if (value == NULL || size == 0 || size >= SMK_LABELLEN) |
2030 | 2049 | return -EINVAL; |
2031 | 2050 | |
... | ... | @@ -2552,7 +2571,8 @@ |
2552 | 2571 | struct security_operations smack_ops = { |
2553 | 2572 | .name = "smack", |
2554 | 2573 | |
2555 | - .ptrace = smack_ptrace, | |
2574 | + .ptrace_may_access = smack_ptrace_may_access, | |
2575 | + .ptrace_traceme = smack_ptrace_traceme, | |
2556 | 2576 | .capget = cap_capget, |
2557 | 2577 | .capset_check = cap_capset_check, |
2558 | 2578 | .capset_set = cap_capset_set, |