[PATCH] libio: Disable vtable validation in case of interposition [BZ #23313]

[PATCH] libio: Disable vtable validation in case of interposition [BZ #23313]

[Florian Weimer]

2018-06-19  Florian Weimer  <fweimer@redhat.com>

	[BZ #23313]
	* libio/stdfiles.c (STDFILE_SECTION): Define macro.
	(DEF_STDFILE): Use it.
	* libio/vtables.c [SHARED] (__libc_IO_stdfiles): Declare as symbol
	set.
	[SHARED] (interposed_stdfiles_variable): New function.
	(stdfiles_interposed): Likewise.
	(_IO_vtable_check): Call stdfiles_interposed.

diff --git a/libio/stdfiles.c b/libio/stdfiles.c
index 18e1172ad0..2435f412f2 100644
--- a/libio/stdfiles.c
+++ b/libio/stdfiles.c
@@ -33,11 +33,19 @@
 
 #include "libioP.h"
 
+#ifdef SHARED
+/* Place the variables defined below in a separate section for the
+   interposition check in vtables.c.  */
+# define STDFILE_SECTION __attribute__ ((section ("__libc_IO_stdfiles")))
+#else
+# define STDFILE_SECTION
+#endif
+
 #define DEF_STDFILE(NAME, FD, CHAIN, FLAGS) \
   static _IO_lock_t _IO_stdfile_##FD##_lock = _IO_lock_initializer; \
   static struct _IO_wide_data _IO_wide_data_##FD \
     = { ._wide_vtable = &_IO_wfile_jumps }; \
-  struct _IO_FILE_plus NAME \
+  struct _IO_FILE_plus NAME STDFILE_SECTION \
     = {FILEBUF_LITERAL(CHAIN, FLAGS, FD, &_IO_wide_data_##FD), \
        &_IO_file_jumps};
 
diff --git a/libio/vtables.c b/libio/vtables.c
index 9fd4ccf642..aba414ef2d 100644
--- a/libio/vtables.c
+++ b/libio/vtables.c
@@ -29,11 +29,44 @@ void (*IO_accept_foreign_vtables) (void) attribute_hidden;
 extern struct dl_open_hook *_dl_open_hook;
 libc_hidden_proto (_dl_open_hook);
 
+/* Used to detect interposition of _IO_2_1_stdin_, _IO_2_1_stdout_,
+   _IO_2_1_stderr_.  See stdfiles.c.  */
+symbol_set_declare (__libc_IO_stdfiles)
+
+/* Check whether the standard file variable at *PTR has been
+   interposed, by comparing its address against the start and end of
+   the __libc_IO_stdfiles section.  */
+static inline bool
+interposed_stdfiles_variable (void *ptr)
+{
+  uintptr_t uptr = (uintptr_t) ptr; /* Avoid ptrdiff_t overflow.  */
+  uintptr_t length = __stop___libc_IO_stdfiles - __start___libc_IO_stdfiles;
+  uintptr_t start = uptr - (uintptr_t) __start___libc_IO_stdfiles;
+  return start >= length;
+}
+
+/* Return true if any of the standard variable definitions for stdin,
+   stdout, stderr have been interposed.  */
+static inline bool
+stdfiles_interposed (void)
+{
+  return interposed_stdfiles_variable (&_IO_2_1_stdin_)
+    || interposed_stdfiles_variable (&_IO_2_1_stdout_)
+    || interposed_stdfiles_variable (&_IO_2_1_stderr_);
+}
+
 #else  /* !SHARED */
 
 /* Used to check whether static dlopen support is needed.  */
 # pragma weak __dlopen
 
+/* No interposition is possible in the statically linked case.  */
+static inline bool
+stdfiles_interposed (void)
+{
+  return false;
+}
+
 #endif
 
 void attribute_hidden
@@ -48,6 +81,13 @@ _IO_vtable_check (void)
   if (flag == &_IO_vtable_check)
     return;
 
+  /* If any of the standard file variable definitions have been
+     interposed, assume that the interposition came along with its own
+     vtable pointer.  This is needed to support some legacy libstdc++
+     libraries.  */
+  if (stdfiles_interposed ())
+    return;
+
   /* In case this libc copy is in a non-default namespace, we always
      need to accept foreign vtables because there is always a
      possibility that FILE * objects are passed across the linking

Re: [PATCH] libio: Disable vtable validation in case of interposition [BZ #23313]

[Szabolcs Nagy]

On 19/06/18 10:56, Florian Weimer wrote:
> 2018-06-19  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #23313]
> 	* libio/stdfiles.c (STDFILE_SECTION): Define macro.
> 	(DEF_STDFILE): Use it.
> 	* libio/vtables.c [SHARED] (__libc_IO_stdfiles): Declare as symbol
> 	set.
> 	[SHARED] (interposed_stdfiles_variable): New function.
> 	(stdfiles_interposed): Likewise.
> 	(_IO_vtable_check): Call stdfiles_interposed.
> 
> diff --git a/libio/stdfiles.c b/libio/stdfiles.c
> index 18e1172ad0..2435f412f2 100644
> --- a/libio/stdfiles.c
> +++ b/libio/stdfiles.c
> @@ -33,11 +33,19 @@
>   
>   #include "libioP.h"
>   
> +#ifdef SHARED
> +/* Place the variables defined below in a separate section for the
> +   interposition check in vtables.c.  */
> +# define STDFILE_SECTION __attribute__ ((section ("__libc_IO_stdfiles")))
> +#else
> +# define STDFILE_SECTION
> +#endif
> +
>   #define DEF_STDFILE(NAME, FD, CHAIN, FLAGS) \
>     static _IO_lock_t _IO_stdfile_##FD##_lock = _IO_lock_initializer; \
>     static struct _IO_wide_data _IO_wide_data_##FD \
>       = { ._wide_vtable = &_IO_wfile_jumps }; \
> -  struct _IO_FILE_plus NAME \
> +  struct _IO_FILE_plus NAME STDFILE_SECTION \
>       = {FILEBUF_LITERAL(CHAIN, FLAGS, FD, &_IO_wide_data_##FD), \
>          &_IO_file_jumps};
>   
> diff --git a/libio/vtables.c b/libio/vtables.c
> index 9fd4ccf642..aba414ef2d 100644
> --- a/libio/vtables.c
> +++ b/libio/vtables.c
> @@ -29,11 +29,44 @@ void (*IO_accept_foreign_vtables) (void) attribute_hidden;
>   extern struct dl_open_hook *_dl_open_hook;
>   libc_hidden_proto (_dl_open_hook);
>   
> +/* Used to detect interposition of _IO_2_1_stdin_, _IO_2_1_stdout_,
> +   _IO_2_1_stderr_.  See stdfiles.c.  */
> +symbol_set_declare (__libc_IO_stdfiles)
> +
> +/* Check whether the standard file variable at *PTR has been
> +   interposed, by comparing its address against the start and end of
> +   the __libc_IO_stdfiles section.  */
> +static inline bool
> +interposed_stdfiles_variable (void *ptr)
> +{
> +  uintptr_t uptr = (uintptr_t) ptr; /* Avoid ptrdiff_t overflow.  */
> +  uintptr_t length = __stop___libc_IO_stdfiles - __start___libc_IO_stdfiles;
> +  uintptr_t start = uptr - (uintptr_t) __start___libc_IO_stdfiles;
> +  return start >= length;
> +}
> +
> +/* Return true if any of the standard variable definitions for stdin,
> +   stdout, stderr have been interposed.  */
> +static inline bool
> +stdfiles_interposed (void)
> +{
> +  return interposed_stdfiles_variable (&_IO_2_1_stdin_)
> +    || interposed_stdfiles_variable (&_IO_2_1_stdout_)
> +    || interposed_stdfiles_variable (&_IO_2_1_stderr_);
> +}

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

Re: [PATCH] libio: Disable vtable validation in case of interposition [BZ #23313]

[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.

diff --git a/libio/libioP.h b/libio/libioP.h
index 8afe7032e3..3598eb490f 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -839,4 +839,12 @@ IO_validate_vtable (const struct _IO_jump_t *vtable)
   return vtable;
 }
 
+/* Used in vtables.h to check for interposition of the corresponding
+   variables _IO_2_1_stdin_, _IO_2_1_stdout_, _IO_2_1_stderr_.  */
+#ifdef SHARED
+extern struct _IO_FILE_plus _IO_2_1_stdin_hidden attribute_hidden;
+extern struct _IO_FILE_plus _IO_2_1_stdout_hidden attribute_hidden;
+extern struct _IO_FILE_plus _IO_2_1_stderr_hidden attribute_hidden;
+#endif
+
 #endif /* libioP.h.  */
diff --git a/libio/stdfiles.c b/libio/stdfiles.c
index 18e1172ad0..2af6e9db66 100644
--- a/libio/stdfiles.c
+++ b/libio/stdfiles.c
@@ -45,5 +45,11 @@ DEF_STDFILE(_IO_2_1_stdin_, 0, 0, _IO_NO_WRITES);
 DEF_STDFILE(_IO_2_1_stdout_, 1, &_IO_2_1_stdin_, _IO_NO_READS);
 DEF_STDFILE(_IO_2_1_stderr_, 2, &_IO_2_1_stdout_, _IO_NO_READS+_IO_UNBUFFERED);
 
+#ifdef SHARED
+strong_alias (_IO_2_1_stdin_, _IO_2_1_stdin_hidden)
+strong_alias (_IO_2_1_stdout_, _IO_2_1_stdout_hidden)
+strong_alias (_IO_2_1_stderr_, _IO_2_1_stderr_hidden)
+#endif
+
 struct _IO_FILE_plus *_IO_list_all = &_IO_2_1_stderr_;
 libc_hidden_data_def (_IO_list_all)
diff --git a/libio/vtables.c b/libio/vtables.c
index 9fd4ccf642..03055fa4ed 100644
--- a/libio/vtables.c
+++ b/libio/vtables.c
@@ -29,11 +29,28 @@ void (*IO_accept_foreign_vtables) (void) attribute_hidden;
 extern struct dl_open_hook *_dl_open_hook;
 libc_hidden_proto (_dl_open_hook);
 
+/* Return true if any of the standard variable definitions for stdin,
+   stdout, stderr have been interposed.  */
+static inline bool
+stdfiles_interposed (void)
+{
+  return !(&_IO_2_1_stdin_ == &_IO_2_1_stdin_hidden
+           && &_IO_2_1_stdout_ == &_IO_2_1_stdout_hidden
+           && &_IO_2_1_stderr_ == &_IO_2_1_stderr_hidden);
+}
+
 #else  /* !SHARED */
 
 /* Used to check whether static dlopen support is needed.  */
 # pragma weak __dlopen
 
+/* No interposition is possible in the statically linked case.  */
+static inline bool
+stdfiles_interposed (void)
+{
+  return false;
+}
+
 #endif
 
 void attribute_hidden
@@ -48,6 +65,13 @@ _IO_vtable_check (void)
   if (flag == &_IO_vtable_check)
     return;
 
+  /* If any of the standard file variable definitions have been
+     interposed, assume that the interposition came along with its own
+     vtable pointer.  This is needed to support some legacy libstdc++
+     libraries.  */
+  if (stdfiles_interposed ())
+    return;
+
   /* In case this libc copy is in a non-default namespace, we always
      need to accept foreign vtables because there is always a
      possibility that FILE * objects are passed across the linking

Re: [PATCH] libio: Disable vtable validation in case of interposition [BZ #23313]

[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.

Re: [PATCH] libio: Disable vtable validation in case of interposition [BZ #23313]

[Florian Weimer]

* 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.

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);
+}

Re: [PATCH] libio: Disable vtable validation in case of interposition [BZ #23313]

[Florian Weimer]

* 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?

Re: [PATCH] libio: Disable vtable validation in case of interposition [BZ #23313]

[Szabolcs Nagy]

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)

Re: [PATCH] libio: Disable vtable validation in case of interposition [BZ #23313]

[Florian Weimer]

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

Re: [PATCH] libio: Disable vtable validation in case of interposition [BZ #23313]

[Florian Weimer]

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

On 06/22/2018 12:51 PM, Florian Weimer wrote:
> 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.

Here's what I've committed.

Thanks,
Florian

[-- Attachment #2: vtable-interposition.patch --]
[-- Type: text/x-patch, Size: 1091 bytes --]

Subject: [PATCH COMMITTED] libio: Disable vtable validation in case of interposition [BZ #23313]
To: libc-alpha@sourceware.org

2018-06-26  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..9df75668c8 100644
--- a/libio/vtables.c
+++ b/libio/vtables.c
@@ -71,3 +71,19 @@ _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.  */
+#ifdef SHARED
+__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);
+}
+#endif