Date:	Sun, 4 Jan 2009 21:18:27 -0600
From:	"Serge E. Hallyn" <serue@...ibm.com>
To:	James Morris <jmorris@...ei.org>
Cc:	David Howells <dhowells@...hat.com>,
	Christoph Hellwig <hch@....de>, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	Andrew Morgan <morgan@...nel.org>
Subject: Re: [PATCH] CRED: Fix regression in cap_capable() as shown up by
	sys_faccessat() [ver #2]

Quoting James Morris (jmorris@...ei.org):
> On Wed, 31 Dec 2008, David Howells wrote:
> 
> > 
> > Here's an improved patch.  It differentiates the use of objective and
> > subjective capabilities by making capable() only check current's subjective
> > caps, but making has_capability() check only the objective caps of whatever
> > process is specified.
> > 
> > It's a bit more involved, but I think it's the right thing to do.
> 
> I think it's the right approach, too, and the patch seems ok to me.  I've 
> applied it to 
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next
> 
> and expect to push it to Linus in the next day or so.  It's not a trivial 
> change, and could do with more review (Serge?).

(Andrew Morgan cc'd for more review)

Yes, I'm sorry, I've been staring at it on and off all weekend...  From
a high level it looks right, but i think there's a problem with naming
here (which also could be said to have obfuscated the original bug).
The security_capable() should be security_capable_eff() or somesuch,
and security_task_capable() should be security_task_capable_real() or
something.  In fact the current-as-implicit optimization could be put
off for a kernel release or so while we just have
security_capable_eff(tsk, cap) and security_real_eff(tsk, cap), or
just security_capable(tsk, cap, flag) where flag is CAP_EFF or CAP_REAL.

I've been holding off on saying this since sat night bc when i read it
back it looks petty, but every time i read the patch i'm more convinced
that the type of capability checked must be made explicit to make this
maintainable (maybe even reviewable).

I'm also not thrilled about security_task_capable() and
security_ops->task_capable() having different args and semantics, but
of course I see the reason for it and figure if there's a way to improve
on that we can do it later.

Anyway David the patch on its own doesn't look incorrect.  So far
the only code which manipulates subjective caps is in fact
sys_faccessat through cap_set_effective() right?  If so this at
least looks safe, looking through capable/has_capability callers.

Finally, may i just say that i love the fact that a syscall is
checking the real user's access rights and so sets eff creds to
have the caps of the real user :)  Hmm, did you at one time call
the subjective creds 'working' or 'acting' creds?  Might be a good
name.  Subj/obj always makes me pause to think, and real/eff while
seemingly natural could be confusing in cases like this.
cap_set_acting() -  i like it...

thanks,
-serge

> It seems that more testing should be done in linux-next vs. waiting for 
> the merge window.
> 
> 
> - James
> -- 
> James Morris
> <jmorris@...ei.org>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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