Re: Alternative libio vtable hardening approach

[Carlos O'Donell <carlos at redhat dot com>]

On 05/31/2016 11:56 AM, Florian Weimer wrote:
> On 05/31/2016 05:09 PM, Carlos O'Donell wrote:
> 
>> The question I'm asking myself is: Should we be pointer mangling any of
>> these pointers to make it harder to scan for the table?
> 
> I'm not sure what you are trying to address. The table itself should
> be in RELRO memory, and its address should be at a fixed offset
> relative to the PC at the site of the check, so all that should be
> pretty safe.

That's exactly the question I'm asking.

If I understand your reasoning:

(a) The table is in .rodata/.data.rel.ro (verified present tables are in .rodata)

(b) The table is always PC-relative addressable from ROP gadgets
    within the library so finding it is easy.
    - Hiding the pointers with mangling doesn't make this harder.

(c) Mangling degrades performance.

Seems fine to me, I just wanted to be as explicit as possible for our
choices.

> I'm only worried about the flag that turns of the check.

I don't see any way to provide backwards compatibility than to disable
the check with a global.

We could encrypt the value of the global to make it harder to unset?

Mike suggests this too.

>>> The attached patch is not completely finished.  We need to disable
>>> vtable validation on both sides of a static dlopen boundary because
>>> the vtables are not shared across the boundary.  This is what causes
>>> dlfcn/tststatic2 to fail.
>>
>> How do you plan to disable the hardening at the static dlopen boundary?
> 
> Set the flag on both sides once dlopen is called. I see no way to
> avoid that because we cannot know if any of the functions in the
> loaded DSOs access stdio streams on both sides of the boundary.

You answer this below.
 
> We could presumably add a flag that inhibits this for internal DSOs
> which we know do not have such behavior. But I'm not sure if this is
> worthwhile.

Let us stay focused on the hardening aspect for this first change.
 
>> What we really want is a set of internal initializers that are run during
>> static dlopen / dlmopen that allow the library to copy key state into the
>> other namespace.
> 
> We don't need pass across any actual data, or affect relocation, so
> it's easier. malloc in libc.so does the following to detect whether
> it's running across a linking boundary:
> 
> #ifdef SHARED
>   /* In case this libc copy is in a non-default namespace, never use brk.
>      Likewise if dlopened from statically linked program.  */
>   Dl_info di;
>   struct link_map *l;
> 
>   if (_dl_open_hook != NULL
>       || (_dl_addr (ptmalloc_init, &di, &l, NULL) != 0
>           && l->l_ns != LM_ID_BASE))
>     __morecore = __failing_morecore;
> #endif

Ah, OK, that answers my question. You lookup your own link map and then
determine you're not in the default namespace and reset the variables.
That's a sufficient solution. Thanks.
 
> I just need to stick this code in some place where it runs early
> enough (before any ELF constructors and the like). Suggestions
> welcome. :)

That sounds good. Thanks.

> (And we already have some mechanism because it seems that the dlopen
> hooks are sent across. But I don't have all the details.)

Interesting.

Again, my thoughts were along the lines of some kind of callback
registration process where the old structure pointers are registered
along with a callback. Then in the new namespace the callback runs
with access to the new structure address (dynamic lookup) and does
some kind of custom copy to reinitialize the dynamic namespace.

That's a lot more work than what you propose. I do not suggest you
follow my suggestion. Stick with what works and solve the security
issue first.

>>> A future optimization would change the virtual function calls to
>>> compare the function pointer in the vtable against an expected
>>> function pointer.  Only if that comparison fails, the vtable pointer
>>> is validated.  This will allow us to inline parts of xsputn into
>>> vfprintf, especially if we change the various implementations to
>>> share more code.
>>
>> You suggest this because a pointer comparison is cheaper than an
>> address range comparison? Could we make the tables read-only and use
>> another flag to elide all checking?

Note that this was before I checked to verify the tables were in .rodata
or .data.rel.ro. The present tables are in .rodata.

> The current checking code looks like this:
> 
>      a27:       48 8b 85 80 fb ff ff    mov    -0x480(%rbp),%rax
>      a2e:       4c 8b a0 d8 00 00 00    mov    0xd8(%rax),%r12
>      a35:       49 81 fc 00 00 00 00    cmp    $IO_vtables,%r12
>      a3c:       0f 82 0e 03 00 00       jb     d50
>      a42:       49 81 fc 00 00 00 00    cmp    $IO_vtables+0xbd0,%r12
>      a49:       0f 83 01 03 00 00       jae    d50
>      a4f:       49 8b 75 18             mov    0x18(%r13),%rsi
>      a53:       49 8b 55 20             mov    0x20(%r13),%rdx
>      a57:       48 8b bd 80 fb ff ff    mov    -0x480(%rbp),%rdi
>      a5e:       48 29 f2                sub    %rsi,%rdx
>      a61:       41 ff 54 24 38          callq  *0x38(%r12)
> â
>      d50:       e8 00 00 00 00          callq  IO_vtable_check
>      d55:       e9 f5 fc ff ff          jmpq   a4f
> 
> Instead we could do something like this (the call setup sequence is somewhat different here):
> 
>      a40:       49 8b 44 24 38          mov    0x38(%r12),%rax
>      a45:       48 29 f2                sub    %rsi,%rdx
>      a48:       48 3d 00 00 00 00       cmp   $_IO_file_xsputn_fast,%rax
>      a4e:       0f 85 f7 11 00 00       jne    1c4b
>      a54:       4c 89 ff                mov    %r15,%rdi
>      a57:       e8 00 00 00 00          callq  _IO_file_xsputn_fast
> â
>     1c4b:       49 81 fc 00 00 00 00    cmp    $IO_vtables+0xbd0,%r12
>     1c52:       0f 83 4e 04 00 00       jae    20a6
>     1c58:       49 81 fc 00 00 00 00    cmp    $IO_vtables,%r12
>     1c5f:       0f 82 41 04 00 00       jb     20a6
>     1c65:       4c 89 ff                mov    %r15,%rdi
>     1c68:       ff d0                   callq  *%rax
>     1c6a:       e9 ed ed ff ff          jmpq   a5c
> â
>     20a6:       48 89 95 78 fb ff ff    mov    %rdx,-0x488(%rbp)
>     20ad:       48 89 b5 88 fb ff ff    mov    %rsi,-0x478(%rbp)
>     20b4:       e8 00 00 00 00          callq  IO_vtable_check
>     20b9:       49 8b 44 24 38          mov    0x38(%r12),%rax
>     20be:       48 8b 95 78 fb ff ff    mov    -0x488(%rbp),%rdx
>     20c5:       48 8b b5 88 fb ff ff    mov    -0x478(%rbp),%rsi
>     20cc:       e9 94 fb ff ff          jmpq   1c65
> 
> So devirtualize the call to _IO_file_xsputn_fast, which can then be
> inlined. If the slow path is only needed for compatibility (after we
> have carefully rewritten xsputn so that we only need one version,
> which seems doable), we could call some slow-path xsputn function, so
> the

Sounds doable.

> 
>      a4c:       48 8b 83 d8 00 00 00    mov    0xd8(%rbx),%rax
>      a53:       48 29 f2                sub    %rsi,%rdx
>      a56:       48 81 78 38 00 00 00    cmpq $_IO_file_xsputn_fast,0x38(%rax)
>      a5e:       0f 85 e6 11 00 00       jne    1c4a
>      a64:       e8 00 00 00 00          callq  _IO_file_xsputn_fast
> â
>     1c4a:       e8 00 00 00 00          callq  _IO_file_xsputn_slow
>     1c4f:       e9 15 ee ff ff          jmpq   a69
> 
> This expansion has much more reasonable size, but only makes sense if
> the call site is monomorphic in practice.

I don't follow. The slow path can still use the vtables as required by
legacy binaries and the call site there can be polymorphic? Maybe I've
misunderstood the suggestion.

>> Agreed. My only worry is static dlopen.
> 
> My worry is that the hardening is insufficient. :)

Would it worry you if I said "the hardening is _always_ insufficient." ;-)

-- 
Cheers,
Carlos.