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

[Kees Cook]

On Sun, Nov 18, 2012 at 10:57 PM, P J P <ppandit@xxxxxxxxxx> wrote:
> +-- On Sun, 18 Nov 2012, Kees Cook wrote --+
> | This is the second problem. I view this as less critical because it's only
> | 64 instead of 4, but it certainly should be solved as well.
>
>   I don't mean to be rude, but the patch I had sent solves both of these
> problems with much less performance hit.
>
> Please see -> https://lkml.org/lkml/2012/10/26/442
>
> Worst case: instead of 2^6(64) recursions, it would make only 4 calls to
> request_module() function. find_module() does not resolve module aliases, it
> could be removed from the above patch.
>
> Please pardon me if I came across rude or offensive, I'm only trying to make a
> case.

I don't think you're being rude at all. You're defending your
solution. :) I wasn't very verbose in my objection to it, so I
apologize for that and now attempt to make up for it in this email. :)

I felt that your patch made certain things worse, but perhaps I was
seeing it incorrectly. Just to have a common point of reference, here
are some of the exec cases I've been considering. This is without
either of our patches applied:

An exec of ELF directly:

- bprm created
- search_binary_handler called
  - each fmt->load_binary called
    - ELF handler hits
- done: 0 request_module calls, 0 leaks

An exec of a a file handled by binfmt_misc:

- bprm created
- search_binary_handler called (depth == 0)
  - each fmt->load_binary called
    - misc handler hits
      - rewrites various bprm things, including bprm->interp
      - search_binary_handler called (depth == 1)
        - each fmt->load_binary called
        - ELF handler hits (usually, depends on specific binfmt_misc
interpreter config)
- done: 0 request_module calls, 0 leaks

An exec of a script that calls ELF:

- bprm created
- search_binary_handler called (depth == 0)
  - each fmt->load_binary called
    - script handler hits
      - rewrites various bprm things, including bprm->interp
      - search_binary_handler called (depth == 1)
        - each fmt->load_binary called
        - ELF handler hits
- done: 0 request_module calls, 0 leaks

An exec of an binary with unprintables where a module exists:

- bprm created
- search_binary_handler called (depth == 0)
  - each fmt->load_binary called
  - nothing hits, retval == ENOEXEC
  - request_module("binfmt-%04x") called
  - search_binary_handler loop starts again
  - each fmt->load_binary called
  - new handler hits
- done: 1 request_module call, 1 leak

An exec of a chain of abusive scripts that contains unprintables:

- bprm created
- search_binary_handler called (depth == 0)
  - each fmt->load_binary called
    - script handler hits
      - rewrites various bprm things, including bprm->interp
      - search_binary_handler called (depth == 1)
        - each fmt->load_binary called
          - script handler hits
          - rewrites various bprm things, including bprm->interp
          - search_binary_handler called (depth == 2 ...)
            ...
            - each fmt->load_binary called
              - at depth > BINPRM_MAX_RECURSION, script handler returns -ENOEXEC
            - retval == ENOEXEC
            - request_module("binfmt-%04x") called
            - search_binary_handler loop starts again
            - each fmt->load_binary called
              - at depth > BINPRM_MAX_RECURSION, script handler returns -ENOEXEC
       ... unwind and retry continues all the way back up
  - retval == ENOEXEC
  - request_module("binfmt-%04x") called
  - search_binary_handler loop starts again
  - each fmt->load_binary called
- done: 63 calls to request_module (one in each search_binary_handler
call, to depth 5 (32 wide)), 63 leaks

So, my interp-on-heap patch doesn't change the request_module call
count, but it changes the stack reference leaks to kalloc/kfree pairs.

Your patch changes a number of things. The obvious part is that is
solves the abusive chain of script example by stopping at depth == 5,
after 5 calls to request_module, and eliminates the retries so there
are 0 stack reference leaks.

However, it also changes the conditions for when a module is loaded
(i.e. 0x7f no longer triggers a module_load, so anything needing that
would break -- I'm not sure if this really qualifies for ABI breakage,
I don't use any obscure binfmt modules so I can't say).

And, most importantly, it triggers request_module for any binary with
unprintables that binfmt_misc may already handle (for example, the
very common case of handling DOS MZ files, which only define 2 bytes
as magic (MZ) and exampes I find show things like "@\x00" trailing it,
or JAR files which are PK\x03\x04). Which means each exec of these
kinds of files would trigger a needless request_module() call on every
exec.

I feel that this too radically changes the behavioral characteristics
of exec for common cases. The original logic is:
- try all existing binfmt handlers
- if none found, try to load a module for it in it had unprintables
(including 0x7f)
- try them all again

Your new logic would be:
- try to load a module if there are unprintables (excluding 0x7f)
- try all existing binfmt handlers

I really don't think this change is sensible. We need to only do the
module request when none of the handlers hit, and things haven't gone
wrong.

I think to avoid the explosion of request_module calls in the abusive
case, we could simply return ELOOP instead of ENOEXEC on max
recursion. This would mean an immediate return on max depth failures.
Also, after looking at the recursion tracking here, it really seems
like search_binary_handler should track it, not the binfmt handlers.
I'll send the patch.

Both the interp-on-heap patch and this proposed ELOOP patch are needed
to handle the case of binfmt_script and/or binfmt_misc being modules
(first binfmt walk fails with -ENOEXEC, loads binfmt_script, retries
loop, hits binfmt_script rewriting interp to a PE file, recurses,
fails with -ENOEXEC, loads binfmt_misc via a modalias for PE files,
retries loop, hits binfmt_misc rewriting interp to an ELF, recurses,
loads ELF, happiness). Without the heap patch, we could be pointing
into old stack (rewritten e.g. during module load or taking an
interrupt, etc) on the loop retries. Without the ELOOP patch, the
recursion could explode with an abusive script chain.

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/