libio: Disable vtable validation in case of interposition [BZ #23313]
Commit Message
* Florian Weimer:
> * Florian Weimer:
>
>> * Szabolcs Nagy:
>>
>>> the check looks ok to me
>>> i think hidden symbol alias would work too
>>>
>>> return &_IO_2_1_stdin == &_IO_2_1_stdin_internal_alias || ...;
>>>
>>> and it may be a bit nicer than checking the section.. i'm not sure
>>
>> I had hoped that GCC would combine the comparisons in some way, but it
>> doesn't do that either way.
>>
>> The hidden alias approach also has the advantage that it does not
>> touch the _IO_MTSAFE_IO code.
>>
>>
>> 2018-06-19 Florian Weimer <fweimer@redhat.com>
>>
>> [BZ #23313]
>> * libio/libioP.h [SHARED] (_IO_2_1_stdin_hidden)
>> (_IO_2_1_stdout_hidden, _IO_2_1_stderr_hidden): Declare.
>> * libio/stdfiles.c [SHARED] (_IO_2_1_stdin_hidden)
>> (_IO_2_1_stdout_hidden, _IO_2_1_stderr_hidden): Define aliases.
>> * libio/vtables.c (stdfiles_interposed): New function.
>> (_IO_vtable_check): Call it.
>
> This seems to be overly conservative. I think we do not actually have
> to care about interposition. We should read the vtables in the
> libc.so startup code and check if they match the values in stdfiles.c.
> This way, vtable validation is not disabled if there is merely a copy
> relocation.
I suppose this is the best way to implement this. No regressions in
the test suite in x86-64, the vtables test I posted still passes, and
yadex still starts, too.
Since this code only runs once, it should be okay from a hardening
standpoint.
libio: Detect foreign vtables in interposed standard streams [BZ #23313]
2018-06-20 Florian Weimer <fweimer@redhat.com>
[BZ #23313]
* libio/vtables.c (check_stdfiles_vtables): New ELF constructor.
Comments
* Florian Weimer:
> libio: Detect foreign vtables in interposed standard streams [BZ #23313]
>
> 2018-06-20 Florian Weimer <fweimer@redhat.com>
>
> [BZ #23313]
> * libio/vtables.c (check_stdfiles_vtables): New ELF constructor.
>
> diff --git a/libio/vtables.c b/libio/vtables.c
> index 9fd4ccf642..5b1b581984 100644
> --- a/libio/vtables.c
> +++ b/libio/vtables.c
> @@ -71,3 +71,17 @@ _IO_vtable_check (void)
>
> __libc_fatal ("Fatal error: glibc detected an invalid stdio handle\n");
> }
> +
> +/* Some variants of libstdc++ interpose _IO_2_1_stdin_ etc. and
> + install their own vtables directly, without calling _IO_init or
> + other functions. Detect this by looking at the vtables values
> + during startup, and disable vtable validation in this case. */
> +__attribute__ ((constructor))
> +static void
> +check_stdfiles_vtables (void)
> +{
> + if (_IO_2_1_stdin_.vtable != &_IO_file_jumps
> + || _IO_2_1_stdout_.vtable != &_IO_file_jumps
> + || _IO_2_1_stderr_.vtable != &_IO_file_jumps)
> + IO_set_accept_foreign_vtables (&_IO_vtable_check);
> +}
Any comments on this?
On 21/06/18 22:20, Florian Weimer wrote:
>> +/* Some variants of libstdc++ interpose _IO_2_1_stdin_ etc. and
>> + install their own vtables directly, without calling _IO_init or
>> + other functions. Detect this by looking at the vtables values
>> + during startup, and disable vtable validation in this case. */
>> +__attribute__ ((constructor))
>> +static void
>> +check_stdfiles_vtables (void)
>> +{
>> + if (_IO_2_1_stdin_.vtable != &_IO_file_jumps
>> + || _IO_2_1_stdout_.vtable != &_IO_file_jumps
>> + || _IO_2_1_stderr_.vtable != &_IO_file_jumps)
>> + IO_set_accept_foreign_vtables (&_IO_vtable_check);
>> +}
>
> Any comments on this?
>
is this useful to do with static linking too?
(i'd assume ctor ordering is not well defined then
so stdio access can happen before this check)
with dynamic linking the check looks ok to me
(i did not think about the copy relocation issue)
On 06/22/2018 12:15 PM, Szabolcs Nagy wrote:
> On 21/06/18 22:20, Florian Weimer wrote:
>>> +/* Some variants of libstdc++ interpose _IO_2_1_stdin_ etc. and
>>> + install their own vtables directly, without calling _IO_init or
>>> + other functions. Detect this by looking at the vtables values
>>> + during startup, and disable vtable validation in this case. */
>>> +__attribute__ ((constructor))
>>> +static void
>>> +check_stdfiles_vtables (void)
>>> +{
>>> + if (_IO_2_1_stdin_.vtable != &_IO_file_jumps
>>> + || _IO_2_1_stdout_.vtable != &_IO_file_jumps
>>> + || _IO_2_1_stderr_.vtable != &_IO_file_jumps)
>>> + IO_set_accept_foreign_vtables (&_IO_vtable_check);
>>> +}
>>
>> Any comments on this?
>>
>
> is this useful to do with static linking too?
> (i'd assume ctor ordering is not well defined then
> so stdio access can happen before this check)
>
> with dynamic linking the check looks ok to me
> (i did not think about the copy relocation issue)
Right. I don't think we need to support static linking for backwards
compatibility with these old libraries, so I'll “#ifndef SHARED” around
the constructor.
Thanks,
Florian
@@ -71,3 +71,17 @@ _IO_vtable_check (void)
__libc_fatal ("Fatal error: glibc detected an invalid stdio handle\n");
}
+
+/* Some variants of libstdc++ interpose _IO_2_1_stdin_ etc. and
+ install their own vtables directly, without calling _IO_init or
+ other functions. Detect this by looking at the vtables values
+ during startup, and disable vtable validation in this case. */
+__attribute__ ((constructor))
+static void
+check_stdfiles_vtables (void)
+{
+ if (_IO_2_1_stdin_.vtable != &_IO_file_jumps
+ || _IO_2_1_stdout_.vtable != &_IO_file_jumps
+ || _IO_2_1_stderr_.vtable != &_IO_file_jumps)
+ IO_set_accept_foreign_vtables (&_IO_vtable_check);
+}