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

[Kees Cook <keescook@chromium.org>]

On Wed, Oct 24, 2012 at 9:16 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Oct 24, 2012 at 04:20:32PM -0700, Kees Cook wrote:
>> If a series of scripts are executed, each triggering module loading via
>> unprintable bytes in the script header, kernel stack contents can leak
>> into the command line.
>>
>> Normally execution of binfmt_script and binfmt_misc happens
>> recursively. However, when modules are enabled, and unprintable bytes
>> exist in the bprm->buf, execution will restart after attempting to load
>> matching binfmt modules. Unfortunately, the logic in binfmt_script and
>> binfmt_misc does not expect to get restarted. They leave bprm->interp
>> pointing to their local stack. This means on restart bprm->interp is
>> left pointing into unused stack memory which can then be copied into
>> the userspace argv areas.
>>
>> This changes the logic to require allocation for any changes to the
>> bprm->interp. To avoid adding a new kmalloc to every exec, the default
>> value is left as-is. Only when passing through binfmt_script or
>> binfmt_misc does an allocation take place.
>
> I really don't like that.  It papers over the problem, but doesn't really
> solve the underlying stupidity.  We have no good reason to retry a binfmt
> we'd already attempted on this level of recursion.  And your patch doesn't
> deal with that at all.

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.

-Kees

-- 
Kees Cook
Chrome OS Security