On Wed, Sep 28, 2016 at 04:22:53PM -0700, Andy Lutomirski wrote:

On Wed, Sep 28, 2016 at 3:54 PM, Jann Horn [off-list ref] wrote:

quoted

SELinux attempts to make it possible to whitelist trustworthy sources of
code that may be mapped into memory, and Android makes use of this feature.
To prevent an attacker from bypassing this by modifying R+X memory through
/proc/$pid/mem or PTRACE_POKETEXT, it is necessary to call a security hook
in check_vma_flags().

If selinux policy allows PTRACE_POKETEXT, is it really so bad for that
to result in code execution?

Have a look at __ptrace_may_access():

	/* Don't let security modules deny introspection */
	if (same_thread_group(task, current))
		return 0;

This means thread A can attach to thread B and poke its memory, and SELinux
can't do anything about it.

I guess another perspective on this would be that it's a problem that
interfaces usable for poking user memory are subject to introspection rules
(as opposed to e.g. /proc/self/maps, where it is actually useful).

quoted

-struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
+struct mm_struct *proc_mem_open(struct inode *inode,
+                               const struct cred **object_cred,
+                               unsigned int mode)
 {

Why are you passing object_cred all over the place like this?  You
have an inode, and an inode implies a task.

But the task's mm and objective credentials can change, and only mm_access()
holds the cred_guard_mutex during the mm lookup. Although, if the objective
credentials change because of a setuid execution, being able to poke in the
old mm would be pretty harmless...

For that matter, would it possibly make sense to use MEMCG's mm->owner
and get rid of object_cred entirely?

I guess it might.

I can see this causing issues in
strange threading cases, e.g. accessing your own /proc/$$/mem vs
another thread in your process's.

Can you elaborate on that?