[apparmor] [PATCH 1/4] security: add security_path_chdir hook

[John Johansen]

On 11/28/2013 10:32 AM, Christian Boltz wrote:
> Hello,
>
> Am Donnerstag, 28. November 2013 schrieb Seth Arnold:
>> On Tue, Nov 05, 2013 at 05:34:58AM -0800, John Johansen wrote:
>
>>> diff --git a/fs/open.c b/fs/open.c
>>> index d420331..9505fc5 100644
>>> --- a/fs/open.c
>>> +++ b/fs/open.c
>>> @@ -387,6 +387,10 @@ retry:
>>>   	if (error)
>>>   		goto out;
>>>
>>> +	error = security_path_chdir(&path);
>>> +	if (error)
>>> +		goto dput_and_out;
>>> +
>>>
>>>   	error = inode_permission(path.dentry->d_inode, MAY_EXEC |
>>>   	MAY_CHDIR);
>>>   	if (error)
>>>   		goto dput_and_out;
>
> Hmm, does that mean that first the AppArmor permissions are checked and
> then the file/directory permissions?
>
It varies from hook to hook but yes some times the path hooks are checked
before DAC.

> I reported some time ago that the audit.log contains stuff that would be
> denied by file/directory permissions anyway (which also means logging it
> more confusing than useful ;-) and the answer was that this (IMHO buggy)
> behaviour is caused by the kernel.
>
It is, and there is nothing we can do about it. We spent 2 almost 3 years
trying to get hooks inserted in better places. The path hooks are a
compromise that allowed apparmor to be accepted into the upstream kernel.

> It might be a good idea to check the file/directory permissions first,
> and, _if_ access would be allowed, ask AppArmor via the security hook.
>
>>> @@ -419,6 +423,10 @@ SYSCALL_DEFINE1(fchdir, unsigned int, fd)
>>>
>>>   	if (!S_ISDIR(inode->i_mode))
>>>   		goto out_putf;
>>>
>>> +	error = security_path_chdir(&f.file->f_path);
>>> +	if (error)
>>> +		goto out_putf;
>>> +
>>>
>>>   	error = inode_permission(inode, MAY_EXEC | MAY_CHDIR);
>
> Same here.
>
yes we could swap the ordering on these ones

>
> Regards,
>
> Christian Boltz
>