[v2,3/3] selinux: require EXECMEM for forced ptrace poke
diff mbox

Message ID 1475103281-7989-4-git-send-email-jann@thejh.net
State New
Headers show

Commit Message

Jann Horn Sept. 28, 2016, 10:54 p.m. UTC
This is a breaking change for SELinux users that restrict EXECMEM: It might
break gdb if gdb is executed in a domain that does not have EXECMEM
privilege over the debuggee domain.

Unlike most other SELinux hooks, this one takes the subject credentials as
an argument instead of looking up current_cred(). This is done because the
security_forced_write() LSM hook can be invoked from within the write
handler of /proc/$pid/mem, where current_cred() is pretty useless.

Signed-off-by: Jann Horn <jann@thejh.net>
Reviewed-by: Janis Danisevskis <jdanis@android.com>
---
 security/selinux/hooks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Ingo Molnar Sept. 29, 2016, 6:19 a.m. UTC | #1
* Jann Horn <jann@thejh.net> wrote:

> +{
> +	/* Permitting a write to readonly memory is fine - making the readonly
> +	 * memory executable afterwards would require EXECMOD permission because
> +	 * anon_vma would be non-NULL.
> +	 */

Minor stylistic nit: please fix this comment to be visually symmetric. (There's 
another one introduced by one of your other patches.)

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Smalley Sept. 29, 2016, 4:38 p.m. UTC | #2
On 09/28/2016 06:54 PM, Jann Horn wrote:
> This is a breaking change for SELinux users that restrict EXECMEM: It might
> break gdb if gdb is executed in a domain that does not have EXECMEM
> privilege over the debuggee domain.

Since this would break compatibility with existing SELinux policies, you
have to wrap it with a conditional on a policy capability that you can
then enable in newer policies.  See commit
2be4d74f2fd45460d70d4fe65cc1972ef45bf849 for an example.  This requires
a corresponding update to libsepol, and then adding the new policy
capability to your policy (in the policy_capabilities file).

> 
> Unlike most other SELinux hooks, this one takes the subject credentials as
> an argument instead of looking up current_cred(). This is done because the
> security_forced_write() LSM hook can be invoked from within the write
> handler of /proc/$pid/mem, where current_cred() is pretty useless.
> 
> Signed-off-by: Jann Horn <jann@thejh.net>
> Reviewed-by: Janis Danisevskis <jdanis@android.com>
> ---
>  security/selinux/hooks.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 13185a6..e36682a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2149,6 +2149,20 @@ static int selinux_ptrace_traceme(struct task_struct *parent)
>  	return task_has_perm(parent, current, PROCESS__PTRACE);
>  }
>  
> +static int selinux_forced_write(struct vm_area_struct *vma,
> +				const struct cred *subject_cred,
> +				const struct cred *object_cred)
> +{
> +	/* Permitting a write to readonly memory is fine - making the readonly
> +	 * memory executable afterwards would require EXECMOD permission because
> +	 * anon_vma would be non-NULL.
> +	 */
> +	if ((vma->vm_flags & VM_EXEC) == 0)
> +		return 0;
> +
> +	return cred_has_perm(subject_cred, object_cred, PROCESS__EXECMEM);
> +}
> +
>  static int selinux_capget(struct task_struct *target, kernel_cap_t *effective,
>  			  kernel_cap_t *inheritable, kernel_cap_t *permitted)
>  {
> @@ -6033,6 +6047,7 @@ static struct security_hook_list selinux_hooks[] = {
>  
>  	LSM_HOOK_INIT(ptrace_access_check, selinux_ptrace_access_check),
>  	LSM_HOOK_INIT(ptrace_traceme, selinux_ptrace_traceme),
> +	LSM_HOOK_INIT(forced_write, selinux_forced_write),
>  	LSM_HOOK_INIT(capget, selinux_capget),
>  	LSM_HOOK_INIT(capset, selinux_capset),
>  	LSM_HOOK_INIT(capable, selinux_capable),
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 13185a6..e36682a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2149,6 +2149,20 @@  static int selinux_ptrace_traceme(struct task_struct *parent)
 	return task_has_perm(parent, current, PROCESS__PTRACE);
 }
 
+static int selinux_forced_write(struct vm_area_struct *vma,
+				const struct cred *subject_cred,
+				const struct cred *object_cred)
+{
+	/* Permitting a write to readonly memory is fine - making the readonly
+	 * memory executable afterwards would require EXECMOD permission because
+	 * anon_vma would be non-NULL.
+	 */
+	if ((vma->vm_flags & VM_EXEC) == 0)
+		return 0;
+
+	return cred_has_perm(subject_cred, object_cred, PROCESS__EXECMEM);
+}
+
 static int selinux_capget(struct task_struct *target, kernel_cap_t *effective,
 			  kernel_cap_t *inheritable, kernel_cap_t *permitted)
 {
@@ -6033,6 +6047,7 @@  static struct security_hook_list selinux_hooks[] = {
 
 	LSM_HOOK_INIT(ptrace_access_check, selinux_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, selinux_ptrace_traceme),
+	LSM_HOOK_INIT(forced_write, selinux_forced_write),
 	LSM_HOOK_INIT(capget, selinux_capget),
 	LSM_HOOK_INIT(capset, selinux_capset),
 	LSM_HOOK_INIT(capable, selinux_capable),