Re: [kernel-hardening] Re: [PATCH v2 2/4] usercopy: avoid direct copying to userspace

[Rik van Riel <riel@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2793 bytes --]

On Fri, 2016-06-10 at 14:09 -0700, Kees Cook wrote:
> On Wed, Jun 8, 2016 at 2:11 PM, Kees Cook <keescook@chromium.org>
> wrote:
> > 
> > Some non-whitelisted heap memory has small areas that need to be
> > copied
> > to userspace. For these cases, explicitly copy the needed contents
> > out
> > to stack first before sending to userspace. This lets their
> > respective
> > caches remain un-whitelisted (i.e. no SLAB_USERCOPY), since the
> > bulk of
> > their contents should not be exposed to userspace.
> I've spent some time thinking about these kinds of
> non-whitelisted-slab-workaround changes, and I would like to see if
> we
> can design a better solution. So, to that end, here's what I see:
> 
> - HARDENED_USERCOPY verifies object addresses and sizes
> - whitelisted caches (via HARDENED_USERCOPY_WHITELIST's
> SLAB_USERCOPY)
> are intentionally rare
> - Some code uses small parts of non-whitelisted cache memory for
> userspace work (I think the auxv ("mm_struct") and signal frames
> ("task_struct") are good examples of this: neither cache should be
> entirely exposed to userspace, yet tiny pieces are sent to
> userspace.)
> - non-whitelist-workarounds are open-coded
> - non-whitelist-workarounds require a double-copy
> - non-whitelist-workarounds have explicit size maximums (e.g.
> AT_VECTOR_SIZE, sizeof(sigset_t))
> - non-whitelist-workarounds _bypass_ HARDENED_USERCOPY object address
> checking
> 
> So, while the workarounds do have a max-size sanity-check, they
> actually lack the object address checking that would normally happen
> with the usercopy checks. I think to solve the open-coding and
> double-copy problems without compromising on the whitelisting or the
> explicit size checking, we could also gain back the address checking
> if we created something like:
> 
> copy_to_user_n(user, kernel, dynamic-size, const-max-size);
> 
> If "const-max-size" isn't detected as a builtin_constant it could
> fail
> to build. When run, it would a) verify dynamic-size wasn't larger
> that
> const-max-size, and b) perform the regular usercopy checks (without
> the SLAB_USERCOPY check).
> 
> So, for the auxv example, instead of the new stack variable, the
> memcpy, etc, it could just be a one-line change replacing the
> existing
> copy_to_user() call:
> 
> copy_to_user_n(sp, elf_info, ei_index * sizeof(elf_addr_t),
> AT_VECTOR_SIZE);
> 
> (Bike-shedding: copy_to_user_bounded(), ..._limited(),
> ..._whitelist_hole(), ?)
> 
> What do people think?

I like your idea a lot.

For some kinds of objects, we could go one further.

Eg. for objects we know to be in the slab, we could use
copy_to_user_slab, and fail the copy if the pointer is
not a slab object.

-- 
All Rights Reversed.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]