Re: [PATCH] exec: do not leave bprm->interp on stack

[Al Viro <viro@ZenIV.linux.org.uk>]

On Thu, Oct 25, 2012 at 01:09:53PM +0100, Al Viro wrote:
> On Thu, Oct 25, 2012 at 05:16:22PM +0530, P J P wrote:
> > 
> >   Hello Kees,
> > 
> > +-- On Wed, 24 Oct 2012, Kees Cook wrote --+
> > | What should the code here _actually_ be doing? The _script and _misc 
> > | handlers expect to rewrite the bprm contents and recurse, but the module 
> > | loader want to try again. It's not clear to me what the binfmt module 
> > | handler is even there for; I don't see any binfmt-XXXX aliases in the tree. 
> > | If nothing uses it, should we just rip it out? That would solve it too.
> 
> ; grep binfmt- /etc/*/* 2>/dev/null 
> /etc/modprobe.d/aliases.conf:install binfmt-0000 /bin/true
> /etc/modprobe.d/aliases.conf:alias binfmt-204 binfmt_aout
> /etc/modprobe.d/aliases.conf:alias binfmt-263 binfmt_aout
> /etc/modprobe.d/aliases.conf:alias binfmt-264 binfmt_aout
> /etc/modprobe.d/aliases.conf:alias binfmt-267 binfmt_aout
> /etc/modprobe.d/aliases.conf:alias binfmt-387 binfmt_aout
> ; dpkg -S /etc/modprobe.d/aliases.conf 
> module-init-tools: /etc/modprobe.d/aliases.conf
> 
> > I've been following this issue and updated versions of HDs patch. Below is a 
> > small patch to search_binary_handler() routine, which attempts to make the 
> > request_module call before calling load_script routine.
> > 
> > Besides fixing the stack disclosure issue it also helps to *simplify* the 
> > search_binary_handler routine by removing the -for (try=0;try<2;try++)- loop.
> > 
> > I'd really appreciate any comments/suggestions you may have.
> 
> Suggestion: try testing your patches once in a while.  Stopping to think
> for a minute would also help - you've turned every execve() into "do
> request_module() first".  How do you suppose request_module() works?  And
> how would modprobe be able to run?  IOW, this request_module() will be
> stopped by protection against infinite loops, at which point execve will
> proceed with already present binfmt, without having loaded anything.
> But that's even worse than slowdown on each execve (with a lot of whining
> in process), because *every* request_module() will fail now due to the same
> loop prevention.

... and after the second look at your patch, looks like another breakage
in there will have a different effect - it doesn't just eliminate the
first pass through the loop, it inverts the test for "should I try
request_module()".  Overall result is a bit less painful - request_module()
isn't broken on loop prevention, but
	* every bleeding script will have bogus execution of modprobe done
at execve time (and you'd better pray that /sbin/modprobe isn't a shell
script wrapper around the actual binary, or you *will* get loop prevention
kick in)
	* none of the existing binfmt-<...> aliases is going to be hit
now; IOW, all usecases got broken.  Granted, realistically it just means
broken modular aout support, but then it's the only reason to have that
request_module() there in the first place.