Message-ID: <87bnvpvz3y.fsf@x220.int.ebiederm.org>
Date:	Fri, 25 Apr 2014 15:11:13 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Dmitry Kasatkin <dmitry.kasatkin@...il.com>
Cc:	Oleg Nesterov <oleg@...hat.com>,
	Dmitry Kasatkin <d.kasatkin@...sung.com>,
	linux-security-module <linux-security-module@...r.kernel.org>,
	John Johansen <john.johansen@...onical.com>,
	Mimi Zohar <zohar@...ux.vnet.ibm.com>,
	James Morris <jmorris@...ei.org>, viro@...iv.linux.org.uk,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	kernel-team <kernel-team@...ts.ubuntu.com>
Subject: Re: Kernel panic at Ubuntu: IMA + Apparmor

Dmitry Kasatkin <dmitry.kasatkin@...il.com> writes:

> On 26 April 2014 00:27, Eric W. Biederman <ebiederm@...ssion.com> wrote:
>> Dmitry Kasatkin <dmitry.kasatkin@...il.com> writes:
>>
>>> On 25 April 2014 23:45, Eric W. Biederman <ebiederm@...ssion.com> wrote:
>>>> Dmitry Kasatkin <dmitry.kasatkin@...il.com> writes:
>>>>
>>>>> On 25 April 2014 23:01, Oleg Nesterov <oleg@...hat.com> wrote:
>>>>>> On 04/25, Eric W. Biederman wrote:
>>>>>>>
>>>>>>> Oleg Nesterov <oleg@...hat.com> writes:
>>>>>>>
>>>>>>> > Well. I _think_ that __fput() and ima_file_free() in particular should not
>>>>>>> > depend on current and/or current->nsproxy. If nothing else, fput() can be
>>>>>>> > called by the unrelated task which looks into /proc/pid/.
>>>>>>> >
>>>>>>> > But again, task_work_add() has more and more users, and it seems that even
>>>>>>> > __fput() paths can do "everything", so perhaps it would be safer to allow
>>>>>>> > to use ->nsproxy in task_work_run.
>>>>>>>
>>>>>>> Like I said, give me a clear motivating case.
>>>>>>
>>>>>> I agree, we need a reason. Currently I do not see one.
>>>>>>
>>>>>>> Right now not allowing
>>>>>>> nsproxy is turning up bugs in __fput.  Which seems like a good thing.
>>>>>>
>>>>>> This is what I certainly agree with ;)
>>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> IMA uses kernel_read API which does not know anything about caller.
>>>>> And of course security frameworks are at guard as usual.
>>>>>
>>>>> Exactly after reading first Eric's respons, I thought why to scratch
>>>>> the head when task work queues are indeed designed for tasks...
>>>>
>>>> __fput has no guarantee of running in the task that close the file
>>>> descriptor.  If your code depends on that your code is broken.
>>>>
>>>>> And if you to dig for the history, IMA-appraisal was stuck due to
>>>>> lockdep reporting even though it was on non-everlaping cases.
>>>>> IIRC files vs. directories...
>>>>>
>>>>> After that IIRC Al Viro discussed about delayed fput and IIRC Oleg
>>>>> (sorry if I am wrong) introduced task work queues.
>>>>>
>>>>> So IMA-appraisal was able to be upstreamed... That was ~3.4 time frame, IIRC
>>>>>
>>>>> Name space also dated around ~3.4??
>>>>> Apparmor namespace change was also around that time.
>>>>>
>>>>> 3.10 introduces this name space order change and broke IMA-appraisal.
>>>>
>>>> IMA-appraisal is fundamentally broken because I can take a mandatory
>>>> file lock and prevent IMA-apprasial.
>>>>
>>>
>>> What file lock are you talking about?
>>> IMA-appraisal does not depends on file locks...
>>
>> It honors them.  Look at rw_verify_area, in vfs_read, in kernel_read.
>>
>> It sure looks like locks_mandatory_area can cause your kernel_read to
>> fail.
>>
>>>> Using kernel_read is what allows this.
>>>>
>>>>> Isn't it a clear motivating case???
>>>>
>>>> kernel_read is not appropriate for IMA use.  The rest of this is just
>>>> the messenger.
>>>>
>>>> IMA needs to use a cousin of kernel_read that operates at a lower level
>>>> than vfs_read.  A function that all of the permission checks and the
>>>> fsnotify work.
>>>>
>>>> I am sorry to be the bearer of bad news.  But kernel_read is totally
>>>> inappropriate for IMA.
>>>>
>>>
>>> So you break IMA-appraisal and declare that it cannot be used now?
>>
>> I didn't break it.  I read the code, and I read the back trace to see
>> where the bug was.
>>
>> I see IMA-appraisal trying to read file data as if it were a user space
>> application in such a way that it can get permission denied for a whole
>> host of reasons.
>>
>> My understanding of IMA-appraisal is that using a code path that can
>> give use permission denined when performing appraisal is a way for
>> clever people to attack and avoid IMA-appraisal without violating any
>> security policy.
>>
>
> Interesting thing is that file was already open before and LSM gave OK for this.
> Then re-reading the file on close in fact does not need any LSM
> permission checks.

And LSM are typically an implementation of mandatory access control.
Which means they do the crazy thing of checking every file operation.
Which means from one operation to the next they can give you -EPERM.

> But as kernel_read API is still the same, it goes via the same checks...
>
> But on close with delayed fput nsproxy is missing ....

And you might right in the same processor or you might be called in a
completely different process with completely different credentials
and security labels and the the lsm instead of crashing would silently
tell you no, and you have even stranger bugs to track down.

>> Am I wrong.  Is it ok for IMA-appraisal to get permission denied when it
>> wants to appraise a file?
>>
>
> IMA is called after may_open...

Which in the case of mandatory file locking, or lsms means little.

I am afraid your mental model of where linux does permission checks is
about 20 years out of date.  Which is interesting to say to an lsm maintainer...

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

Powered by blists - more mailing lists