Re: [PATCH v2 0/9] procfs: protect /proc/<pid>/* files with file->f_cred

[Andy Lutomirski]

On Thu, Oct 3, 2013 at 4:40 PM, Djalal Harouni <tixxdz@xxxxxxxxxx> wrote:
> On Thu, Oct 03, 2013 at 04:15:43PM +0100, Andy Lutomirski wrote:
>> On Thu, Oct 3, 2013 at 1:29 PM, Djalal Harouni <tixxdz@xxxxxxxxxx> wrote:
>> > On Thu, Oct 03, 2013 at 08:12:44AM +0200, Ingo Molnar wrote:
>> >> Now procfs might be special, as by its nature of a pseudofilesystem it's
>> >> far more atomic than other filesystems, but still IMHO it's a lot more
>> >
>> >
>> >> robust security design wise to revoke access to objects you should not
>> >> have a reference to when a security boundary is crossed, than letting
>> >> stuff leak through and sprinking all the potential cases that may leak
>> >> information with permission checks ...
>> > I agree, but those access should also be checked at the beginning, IOW
>> > during ->open(). revoke will not help if revoke is not involved at all,
>> > the task /proc entries may still be valide (no execve).
>> >
>> > Currently security boundary is crossed for example arbitrary /proc/*/stack
>> > (and others).
>> > 1) The target task does not do an execve (no revoke).
>> > 2) current task will open these files and *want* and *will* pass the fd to a
>> > more privileged process to pass the ptrace check which is done only during
>> > ->read().
>>
>> What does this?  Or are you saying that this is a bad thing?
> I'm not sure to understand you, revoke if implemented correctly is not a
> bad thing! In the other hand, here I try to explain what if the target task
> did not execve, revoke will never be involved, file descriptors are
> still valid!

Ah. You're saying that both revoke and checking permissions at open
time (or using f_cred) is important.  I think I agree.  (Except that,
arguably, /proc/self/stat should always be fully functional even if
passed to a different process and yama is in use.  This seems minor.)

>
>
>> (And *please* don't write software that *depends* on different
>> processes having different read()/write() permissions on the *same*
>> struct file.  I've already found multiple privilege escalations based
>> on that, and I'm pretty sure I can find some more.)
> Sorry, can't follow you here! examples related to what we discuss here ?

There were various bugs (CVE-2013-1959) in /proc/pid/uid_map, etc,
that were exploitable to obtain uid 0.  They happened because write()
checked its caller's credentials.

>
>
>> >
>> >
>> >> It's also probably faster: security boundary crossings are relatively rare
>> >> slowpaths, while read()s are much more common...
>> > Hmm, These two are related? can't get rid of permission checks
>> > especially on this pseudofilesystem!
>> >
>> >
>> >> So please first get consensus on this fundamental design question before
>> >> spreading your solution to more areas.
>> > Of course, I did clean the patchset to prove that it will work, and I
>> > only implemented full protection for /proc/*/{stack,syscall,stat} other
>> > files will wait.
>> >
>> > But Ingo you can't ignore the fact that:
>> > /proc/*/{stack,syscall} are 0444 mode
>> > /proc/*/{stack,syscall} do not have ptrace_may_access() during open()
>> > /proc/*/{stack,syscall} have the ptrace_may_access() during read()
>>
>> I think everyone agrees that this is broken.  We don't agree on the
>> fix check.  (Also, as described in my other email, your approach may
>> be really hard to get right.)
> Well, yes we don't agree perhaps on the fix, but currently there are no
> other fixes, will be happy to see other propositions! these files have
> been vulnerable for years now...
>
> And for the record it's not my approache. Please just read the emails
> correctly. It was proposed and suggested by Eric and perhaps Linus.
>
> I did an experiment with it, and found it easy without any extra
> overhead: If cred have changed do extra checkes on the original opener.
> It will let you pass file descritors if cred did not change.
>
>
> Where is this other email that says this approach is hard?
> It's not hard, very minor change and it works. Perhaps there is a
> better solution yes, but currently it's not implemented!

I just sent it a couple minutes ago -- it may not have made it yet.
It's here, though:

http://www.openwall.com/lists/kernel-hardening/2013/10/03/9

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/