[PATCH] Add in_execve flag into task_struct.

[PATCH] Add in_execve flag into task_struct.

[Tetsuo Handa]

Serge,

James is now reviewing TOMOYO Linux patch and he is caring about
your comment below.

Serge E. Hallyn wrote:
> I don't like the 'in_exec' bit in the task_struct, but adding LSM hooks
> to let just TOMOYO mark whether you're in exec seems even uglier.

Let me (once again) ask your comment on 'in_exec' approach
originally suggested by David Howells ( http://lkml.org/lkml/2008/10/2/127 ).

Re: [PATCH] Add in_execve flag into task_struct.

[Serge E. Hallyn]

Quoting Tetsuo Handa (penguin-kernel@i-love.sakura.ne.jp):
> Serge,
> 
> James is now reviewing TOMOYO Linux patch and he is caring about
> your comment below.
> 
> Serge E. Hallyn wrote:
> > I don't like the 'in_exec' bit in the task_struct, but adding LSM hooks
> > to let just TOMOYO mark whether you're in exec seems even uglier.
> 
> Let me (once again) ask your comment on 'in_exec' approach
> originally suggested by David Howells ( http://lkml.org/lkml/2008/10/2/127 ).

I still don't like it.  Now I gather the reason for this is that you
want to allow a less trusted domain to execute a file (in a new domain)
without giving it the right to read it?  I'd be interested in hearing
whether others think that's a worthy goal.

-serge

Re: [PATCH] Add in_execve flag into task_struct.

[Shaya Potter]

Serge E. Hallyn wrote:
> Quoting Tetsuo Handa (penguin-kernel@i-love.sakura.ne.jp):
>> Serge,
>>
>> James is now reviewing TOMOYO Linux patch and he is caring about
>> your comment below.
>>
>> Serge E. Hallyn wrote:
>>> I don't like the 'in_exec' bit in the task_struct, but adding LSM hooks
>>> to let just TOMOYO mark whether you're in exec seems even uglier.
>> Let me (once again) ask your comment on 'in_exec' approach
>> originally suggested by David Howells ( http://lkml.org/lkml/2008/10/2/127 ).
> 
> I still don't like it.  Now I gather the reason for this is that you
> want to allow a less trusted domain to execute a file (in a new domain)
> without giving it the right to read it?  I'd be interested in hearing
> whether others think that's a worthy goal.

I assume this has been asked and answered by whats the advantage of this
in_exec flag over a single function that set the security domain, oncee
at the beginning of exec to set the new domain (B) and once in the
fail/exit path to reset it back (to A) in case of failure.

the only difference I can see is that you want what is basically a 3rd
security domain of "execing B".

The question would then be, why is "execing B" different than B itself?

otherwise, the same changes you do to set/unset the in_exec flag could
be used to set and reset the security domains.

it just seems weird to say that we are technically in security domain A,
but are going to treat the process as if its in security domain B when
you can just as easily put it in security domain B and put it back in
security domain A if the operation fails.

but again, I could very well be missing something.

Re: [PATCH] Add in_execve flag into task_struct.

[Stephen Smalley]

On Tue, 2009-01-06 at 12:09 -0500, Shaya Potter wrote:
> Serge E. Hallyn wrote:
> > Quoting Tetsuo Handa (penguin-kernel@i-love.sakura.ne.jp):
> >> Serge,
> >>
> >> James is now reviewing TOMOYO Linux patch and he is caring about
> >> your comment below.
> >>
> >> Serge E. Hallyn wrote:
> >>> I don't like the 'in_exec' bit in the task_struct, but adding LSM hooks
> >>> to let just TOMOYO mark whether you're in exec seems even uglier.
> >> Let me (once again) ask your comment on 'in_exec' approach
> >> originally suggested by David Howells ( http://lkml.org/lkml/2008/10/2/127 ).
> > 
> > I still don't like it.  Now I gather the reason for this is that you
> > want to allow a less trusted domain to execute a file (in a new domain)
> > without giving it the right to read it?  I'd be interested in hearing
> > whether others think that's a worthy goal.
> 
> I assume this has been asked and answered by whats the advantage of this
> in_exec flag over a single function that set the security domain, oncee
> at the beginning of exec to set the new domain (B) and once in the
> fail/exit path to reset it back (to A) in case of failure.
> 
> the only difference I can see is that you want what is basically a 3rd
> security domain of "execing B".
> 
> The question would then be, why is "execing B" different than B itself?
> 
> otherwise, the same changes you do to set/unset the in_exec flag could
> be used to set and reset the security domains.
> 
> it just seems weird to say that we are technically in security domain A,
> but are going to treat the process as if its in security domain B when
> you can just as easily put it in security domain B and put it back in
> security domain A if the operation fails.
> 
> but again, I could very well be missing something.

As I understand it, they want to apply the exec check against the
caller's credential as usual but not apply a read check against the
caller's credential.  This is just like the existing DAC checking (read
access to a program is not required to execute it under DAC, and you can
likely find examples of such programs in a /usr/bin near you - Xorg and
sudo are examples on a recent Fedora).  The reason it doesn't work for
them at present is that they are applying their check from the
dentry_open hook (in order to have the vfsmount available) rather than
from the inode_permission hook.

Note that they would have the same issue if they implemented
file_permission or mmap hooks that revalidate read permission (as in
SELinux).

Note btw that in the DAC case the dumpable flag gets cleared if the
caller lacks read access to the program, so TOMOYO should do likewise if
they are going to support this "execute-without-read" mode.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH] Add in_execve flag into task_struct.

[Tetsuo Handa]

Stephen Smalley wrote:
> As I understand it, they want to apply the exec check against the
> caller's credential as usual but not apply a read check against the
> caller's credential.
Right.

> Note btw that in the DAC case the dumpable flag gets cleared if the
> caller lacks read access to the program, so TOMOYO should do likewise if
> they are going to support this "execute-without-read" mode.
That's this "in_execve" flag.



Shaya Potter wrote:
> I assume this has been asked and answered by whats the advantage of this
> in_exec flag over a single function that set the security domain, oncee
> at the beginning of exec to set the new domain (B) and once in the
> fail/exit path to reset it back (to A) in case of failure.
Old implementation (where security_bprm_alloc() and security_bprm_free()
were available) used that approach.
But new implementation (where security_bprm_alloc() and security_bprm_free()
are no longer available) uses an approach which won't rollback.

> it just seems weird to say that we are technically in security domain A,
> but are going to treat the process as if its in security domain B when
> you can just as easily put it in security domain B and put it back in
> security domain A if the operation fails.
A LSM hook for doing rollback operation no longer exists.
If we can introduce security_start_execve()/security_finish_execve() hooks
in place of security_bprm_alloc() and security_bprm_free(),
TOMOYO can use that approach.

But by utilizing existing LSM hooks, TOMOYO finally came to the point
where TOMOYO can live without more LSM hooks if current process can
remember "whether I'm doing an execve() operation or not".
David Howells prefers this 'in_execve' approach over introducing
security_start_execve()/security_finish_execve() hooks.



Serge E. Hallyn wrote:
> I still don't like it.  Now I gather the reason for this is that you
> want to allow a less trusted domain to execute a file (in a new domain)
> without giving it the right to read it?  I'd be interested in hearing
> whether others think that's a worthy goal.

I see you don't like 'in_execve' flag.
But, this approach is the result of negotiation with David Howells
and was authorized by him.
Serge, will you please accept this 'in_execve' flag?

Re: [PATCH] Add in_execve flag into task_struct.

[Serge E. Hallyn]

Quoting Tetsuo Handa (penguin-kernel@i-love.sakura.ne.jp):
> Stephen Smalley wrote:
> > As I understand it, they want to apply the exec check against the
> > caller's credential as usual but not apply a read check against the
> > caller's credential.
> Right.
> 
> > Note btw that in the DAC case the dumpable flag gets cleared if the
> > caller lacks read access to the program, so TOMOYO should do likewise if
> > they are going to support this "execute-without-read" mode.
> That's this "in_execve" flag.

So you refuse to dump if in_execve is set?

> Shaya Potter wrote:
> > I assume this has been asked and answered by whats the advantage of this
> > in_exec flag over a single function that set the security domain, oncee
> > at the beginning of exec to set the new domain (B) and once in the
> > fail/exit path to reset it back (to A) in case of failure.
> Old implementation (where security_bprm_alloc() and security_bprm_free()
> were available) used that approach.
> But new implementation (where security_bprm_alloc() and security_bprm_free()
> are no longer available) uses an approach which won't rollback.
> 
> > it just seems weird to say that we are technically in security domain A,
> > but are going to treat the process as if its in security domain B when
> > you can just as easily put it in security domain B and put it back in
> > security domain A if the operation fails.
> A LSM hook for doing rollback operation no longer exists.
> If we can introduce security_start_execve()/security_finish_execve() hooks
> in place of security_bprm_alloc() and security_bprm_free(),
> TOMOYO can use that approach.
> 
> But by utilizing existing LSM hooks, TOMOYO finally came to the point
> where TOMOYO can live without more LSM hooks if current process can
> remember "whether I'm doing an execve() operation or not".
> David Howells prefers this 'in_execve' approach over introducing
> security_start_execve()/security_finish_execve() hooks.

There is already a security_bprm_committed_creds() hook.  So you
could  hook abort_creds() and prepare_exec_creds(), and use them
to set a flag in your tomoyo task->security state.  Is that what
David nacked in favor of this flag?

> Serge E. Hallyn wrote:
> > I still don't like it.  Now I gather the reason for this is that you
> > want to allow a less trusted domain to execute a file (in a new domain)
> > without giving it the right to read it?  I'd be interested in hearing
> > whether others think that's a worthy goal.
> 
> I see you don't like 'in_execve' flag.
> But, this approach is the result of negotiation with David Howells
> and was authorized by him.
> Serge, will you please accept this 'in_execve' flag?

It's ugly, you can't get me to say it isn't ugly :), and it sets a scary
bad precedent.  But if David insists (in a reply to this msg) that this
flag really is tops, then just ignore me.  Anyway my point wasn't to
block the patch but to raise discussion (so someone else could decide to
block it :) on both the flag and security implications of these
semantics.

-serge

Re: [PATCH] Add in_execve flag into task_struct.

[Tetsuo Handa]

Serge E. Hallyn wrote:
> > > Note btw that in the DAC case the dumpable flag gets cleared if the
> > > caller lacks read access to the program, so TOMOYO should do likewise if
> > > they are going to support this "execute-without-read" mode.
> > That's this "in_execve" flag.
> 
> So you refuse to dump if in_execve is set?

No. TOMOYO refuses to check read permission in security_dentry_open()
if current->in_execve is set.

To utilize existing LSM hooks as much as possible, TOMOYO uses
security_dentry_open() for checking permissions when a file is opened for
reading and/or writing.

There is one exception. TOMOYO doesn't use security_dentry_open() for checking
permissions when a file is opened for executing.
TOMOYO uses security_bprm_check_security() instead.

Therefore, I need to make it possible to tell whether security_dentry_open()
was called by do_execve() or not.
Unfortunately, as I describe later, there are no LSM hooks which receive
sufficient information for telling whether security_dentry_open() was called by
do_execve() or not. As a result, I'm proposing 'in_execve' flag.

In TOMOYO, security_dentry_open() becomes a no-op function if
current->in_execve flag is set.

If you want TOMOYO not to access current->in_execve flag, it will be possible
to store "whether I'm inside do_execve() or not" into a private hash; but that
will require introduction of new LSM hooks for recording and erasing.

> There is already a security_bprm_committed_creds() hook.  So you
> could  hook abort_creds() and prepare_exec_creds(), and use them
> to set a flag in your tomoyo task->security state.  Is that what
> David nacked in favor of this flag?

First of all, task->security no longer exists, and I'm not allowed to
set a flag in task->cred->security because task->cred is shared.

Next, I have no methods for knowing whether security_creds_prepare() was
called from prepare_exec_creds() or not. Examining function call stack is
far from clean approach. Introducing security_prepare_exec_creds() hook makes
no difference from introducing security_start_execve() hook.

Also, I have no methods for knowing whether security_creds_free() was called
by failure of an execve() operation or not. Introducing
security_free_exec_creds() hook makes no difference from introducing
security_finish_execve() hook.

So, I still believe this 'in_execve' flag is the tops.

> It's ugly, you can't get me to say it isn't ugly  :) , and it sets a scary
> bad precedent.  But if David insists (in a reply to this msg) that this
> flag really is tops, then just ignore me.  Anyway my point wasn't to
> block the patch but to raise discussion (so someone else could decide to
> block it  :)  on both the flag and security implications of these
> semantics.

I'd like to listen to the David's opinion.

Re: [PATCH] Add in_execve flag into task_struct.

[David Howells]

Serge E. Hallyn <serue@us.ibm.com> wrote:

> It's ugly, you can't get me to say it isn't ugly :), and it sets a scary
> bad precedent.  But if David insists (in a reply to this msg) that this
> flag really is tops, then just ignore me.  Anyway my point wasn't to
> block the patch but to raise discussion (so someone else could decide to
> block it :) on both the flag and security implications of these
> semantics.

I think it's probably the best way to support Tomoyo's security model without
reworking a chunk of execve().

David

Re: [PATCH] Add in_execve flag into task_struct.

[James Morris]

On Thu, 5 Feb 2009, David Howells wrote:

> Serge E. Hallyn <serue@us.ibm.com> wrote:
> 
> > It's ugly, you can't get me to say it isn't ugly :), and it sets a scary
> > bad precedent.  But if David insists (in a reply to this msg) that this
> > flag really is tops, then just ignore me.  Anyway my point wasn't to
> > block the patch but to raise discussion (so someone else could decide to
> > block it :) on both the flag and security implications of these
> > semantics.
> 
> I think it's probably the best way to support Tomoyo's security model without
> reworking a chunk of execve().

Agreed.  It's somewhat less than perfect, but I don't see a better 
alternative.


- James
-- 
James Morris
<jmorris@namei.org>