[v4] Improve DST handling (Bug 23102, Bug 21942, Bug 18018, Bug, 23259, CVE-2011-0536 ).

  On 06/06/2018 04:18 PM, Carlos O'Donell wrote:
> On 06/06/2018 12:10 PM, Carlos O'Donell wrote:
>> On 06/06/2018 01:02 AM, Carlos O'Donell wrote:
>>> This commit improves DST handling significantly in the following
>>
>> v2
>>
>> - Fix is_dst() by adding back string comparison, and clarify comment.
>>   Added extra test testcases (49 now) to cover this error.
>> - Renamed parameters as 'input' for data from ELF file, and 'ref' as
>>   reference DST to compare against.
>> - Removed dead update to name in is_dst().
>> - Killed DL_DST_COUNT, not needed really, and a weak optimization.
>> - Did not remove len from _dl_dst_count because we use it to advance
>>   input more quickly to the next DST.
>>
> 
> v3
> - Use memcmp in is_dst().
> - In _dl_dst_substitute use 'len != 0' conditional to clarify that
>   all we are looking to do is distinguish between a valid DST
>   for which we don't recognize the DST, and an invalid DST which
>   may just be stray characters which we will copy to the result.
> - Added a few more tests: Small invalid sequence e.g. ${}, and
>   large valid sequence with unknown DST.
> 

v4
- Remove ":" logic from _dl_dst_substitute, and rewrite exception
  logic without the double negative, and in general cleanup the
  function to make it clear we accept only single path elements
  for processing.
- Cleanup comments regarding RESULT size and what is required.
- Adjust is_dst() comment to note that $ is not counted in length
  returned.
- Fix _dl_dst_substitute exception logic to check for NULL separator
  between non-first path since fillin_rpath and other callers will
  use strsep to split colon separated path elements. This fixes
  the case tested by test 19.

This commit improves DST handling significantly in the following
ways: firstly is_dst () is overhauled to correctly process DST
sequences that would be accepted given the ELF gABI. This means that
we actually now accept slightly more sequences than before.  Now we
accept $ORIGIN$ORIGIN, but in the past we accepted only $ORIGIN\0 or
$ORIGIN/..., but this kind of behaviour results in unexpected
and uninterpreted DST sequences being used as literal search paths
leading to security defects. Therefore the first step in correcting
this defect is making is_dst () properly account for all DSTs
and making the function context free in the sense that it counts
DSTs without knowledge of path, or AT_SECURE. Next, _dl_dst_count ()
is also simplified to count all DSTs regardless of context.
Then in _dl_dst_substitute () we reintroduce context-dependent
processing for such things as AT_SECURE handling. At the level of
_dl_dst_substitute we can have access to things like the true start
of the string sequence to validate $ORIGIN-based paths rooted in
trusted directories.  Lastly, callers of _dl_dst_substitute () are
adjusted to pass in the start of their string sequences, this includes
expand_dynamic_string_token () and fillin_rpath (). Lastly, after this
commit we tighten up the accepted sequences in AT_SECURE, and avoid
leaving unexpanded DSTs, this is noted in the NEWS entry.

Verified with a sequence of 63 tests on x86_64 that cover
non-AT_SECURE and AT_SECURE testing using a sysroot (requires root
to run). The tests cover cases for bug 23102, bug 21942, bug 18018,
and bug 23259. These tests are not yet appropriate for the glibc
regression testsuite, but with the upcoming test-in-container testing
framework it should be possible to include these tests upstream soon.

See the mailing list for the tests:
https://www.sourceware.org/ml/libc-alpha/2018-06/msg00073.html
---
 ChangeLog                  |  20 +++++
 NEWS                       |  10 +++
 elf/dl-deps.c              |   4 +-
 elf/dl-dst.h               |  13 ---
 elf/dl-load.c              | 210 ++++++++++++++++++++++++++++++---------------
 sysdeps/generic/ldsodefs.h |   5 +-
 6 files changed, 177 insertions(+), 85 deletions(-)
  

On 06/08/2018 07:45 AM, Carlos O'Donell wrote:
> +	      /* For SUID/GUID programs we normally ignore the path with
> +		 a DST in DT_RUNPATH, or DT_RPATH.  However, there is
> +		 one exception to this rule, and it is:
> +
> +		   * $ORIGIN appears first in the path element, and is
> +		     the only thing in the element or is immediately
> +		     followed by a path separator and the rest of the
> +		     path.
> +
> +		   * The path element is rooted in a trusted directory.
> +
> +		 This exception allows such programs to reference
> +		 shared libraries in subdirectories of trusted
> +		 directories.  The use case is one of general
> +		 organization and deployment flexibility.
> +		 Trusted directories are usually such paths as "/lib64"
> +		 or "/lib".  */
> +	      if (__glibc_unlikely (__libc_enable_secure)
> +		  && !((input == start + 1
> +			|| (input > start + 1 && input[-2] == '\0'))
> +		       && (input[len] == '\0' || input[len] == '/')))
> +		repl = (const char *) -1;

The comment does not match the code: The code checks that $ORIGIN comes 
first in the *path*, not *path element* (hence the need for the start 
variable).  I'm not sure what the right behavior is here.  Going by path 
element seems more correct.

(The begin variable doesn't seem to add much value, as you noted.)

Thanks,
Florian
  

On 06/08/2018 01:51 AM, Florian Weimer wrote:
> On 06/08/2018 07:45 AM, Carlos O'Donell wrote:
>> +          /* For SUID/GUID programs we normally ignore the path with
>> +         a DST in DT_RUNPATH, or DT_RPATH.  However, there is
>> +         one exception to this rule, and it is:
>> +
>> +           * $ORIGIN appears first in the path element, and is
>> +             the only thing in the element or is immediately
>> +             followed by a path separator and the rest of the
>> +             path.
>> +
>> +           * The path element is rooted in a trusted directory.
>> +
>> +         This exception allows such programs to reference
>> +         shared libraries in subdirectories of trusted
>> +         directories.  The use case is one of general
>> +         organization and deployment flexibility.
>> +         Trusted directories are usually such paths as "/lib64"
>> +         or "/lib".  */
>> +          if (__glibc_unlikely (__libc_enable_secure)
>> +          && !((input == start + 1
>> +            || (input > start + 1 && input[-2] == '\0'))
>> +               && (input[len] == '\0' || input[len] == '/')))
>> +        repl = (const char *) -1;
> 
> The comment does not match the code: The code checks that $ORIGIN
> comes first in the *path*, not *path element* (hence the need for the
> start variable).  I'm not sure what the right behavior is here.
> Going by path element seems more correct.
Sorry, there is a bit of confusion regarding terminology here.

When you have multiple colon separated paths in DT_RUNPATH or
DT_RPATH, what is each path called? Are they path elements?

Or are we calling path elements only those things that are
between slashes in a singular path?

It seems context dependent, but I understand the confusion.

[/path/dir:/path/dir1:/path/dir2]
 ^^^^^^^^^ --- What do we call this in the context of multiple paths?

Just a 'path' ?

I consider the following valid:

[$ORIGIN/../$LIB]

I do *not* consider the following valid:

[$LIB/../$ORIGIN/../$LIB]

Even though they are the same path.

I believe the glibc exception was a very *narrow* exception which
allowed only $ORIGIN-starting paths rooted in trusted directories
to be allowed for SUID/SGID. It's just easier to explain to
developers.

In the abstract all we really care about is that the result be
rooted in a trusted directory. So in that case the appearance
of *any* DST would enable check_for_trusted. That would be an
expansion of what is allowed for SUID/SGID though, and I'm not
sure I want to do that any further.

Thoughts?

Cheers,
Carlos.
  

On 06/08/2018 08:03 AM, Carlos O'Donell wrote:
> On 06/08/2018 01:51 AM, Florian Weimer wrote:
>> On 06/08/2018 07:45 AM, Carlos O'Donell wrote:
>>> +          /* For SUID/GUID programs we normally ignore the path with
>>> +         a DST in DT_RUNPATH, or DT_RPATH.  However, there is
>>> +         one exception to this rule, and it is:
>>> +
>>> +           * $ORIGIN appears first in the path element, and is
>>> +             the only thing in the element or is immediately
>>> +             followed by a path separator and the rest of the
>>> +             path.
>>> +
>>> +           * The path element is rooted in a trusted directory.
>>> +
>>> +         This exception allows such programs to reference
>>> +         shared libraries in subdirectories of trusted
>>> +         directories.  The use case is one of general
>>> +         organization and deployment flexibility.
>>> +         Trusted directories are usually such paths as "/lib64"
>>> +         or "/lib".  */
>>> +          if (__glibc_unlikely (__libc_enable_secure)
>>> +          && !((input == start + 1
>>> +            || (input > start + 1 && input[-2] == '\0'))
>>> +               && (input[len] == '\0' || input[len] == '/')))
>>> +        repl = (const char *) -1;
>>
>> The comment does not match the code: The code checks that $ORIGIN
>> comes first in the *path*, not *path element* (hence the need for the
>> start variable).  I'm not sure what the right behavior is here.
>> Going by path element seems more correct.
> Sorry, there is a bit of confusion regarding terminology here.
> 
> When you have multiple colon separated paths in DT_RUNPATH or
> DT_RPATH, what is each path called? Are they path elements?
> 
> Or are we calling path elements only those things that are
> between slashes in a singular path?
> 
> It seems context dependent, but I understand the confusion.
> 
> [/path/dir:/path/dir1:/path/dir2]
>   ^^^^^^^^^ --- What do we call this in the context of multiple paths?
> 
> Just a 'path' ?
> 
> I consider the following valid:
> 
> [$ORIGIN/../$LIB]

I'm actually asking about this:

[$LIB:$ORIGIN/../$LIB]

Doesn't the current code reject this?

(Some of the callers split at :, not /.)

Thanks,
Florian
  

On 06/08/2018 02:25 AM, Florian Weimer wrote:
> On 06/08/2018 08:03 AM, Carlos O'Donell wrote:
>> On 06/08/2018 01:51 AM, Florian Weimer wrote:
>>> On 06/08/2018 07:45 AM, Carlos O'Donell wrote:
>>>> +          /* For SUID/GUID programs we normally ignore the path with
>>>> +         a DST in DT_RUNPATH, or DT_RPATH.  However, there is
>>>> +         one exception to this rule, and it is:
>>>> +
>>>> +           * $ORIGIN appears first in the path element, and is
>>>> +             the only thing in the element or is immediately
>>>> +             followed by a path separator and the rest of the
>>>> +             path.
>>>> +
>>>> +           * The path element is rooted in a trusted directory.
>>>> +
>>>> +         This exception allows such programs to reference
>>>> +         shared libraries in subdirectories of trusted
>>>> +         directories.  The use case is one of general
>>>> +         organization and deployment flexibility.
>>>> +         Trusted directories are usually such paths as "/lib64"
>>>> +         or "/lib".  */
>>>> +          if (__glibc_unlikely (__libc_enable_secure)
>>>> +          && !((input == start + 1
>>>> +            || (input > start + 1 && input[-2] == '\0'))
>>>> +               && (input[len] == '\0' || input[len] == '/')))
>>>> +        repl = (const char *) -1;
>>>
>>> The comment does not match the code: The code checks that $ORIGIN
>>> comes first in the *path*, not *path element* (hence the need for the
>>> start variable).  I'm not sure what the right behavior is here.
>>> Going by path element seems more correct.
>> Sorry, there is a bit of confusion regarding terminology here.
>>
>> When you have multiple colon separated paths in DT_RUNPATH or
>> DT_RPATH, what is each path called? Are they path elements?
>>
>> Or are we calling path elements only those things that are
>> between slashes in a singular path?
>>
>> It seems context dependent, but I understand the confusion.
>>
>> [/path/dir:/path/dir1:/path/dir2]
>>   ^^^^^^^^^ --- What do we call this in the context of multiple paths?
>>
>> Just a 'path' ?
>>
>> I consider the following valid:
>>
>> [$ORIGIN/../$LIB]
> 
> I'm actually asking about this:
> 
> [$LIB:$ORIGIN/../$LIB]
> 
> Doesn't the current code reject this?

The current code does not reject this. In fact it accepts $LIB in RPATH
et. al. because there is no code to reject it. Only $ORIGIN has any
restrictions.

The patched code continues to accept [$LIB:$ORIGIN/../$LIB].

> (Some of the callers split at :, not /.)

The fillin_rpath caller splits at ':', generally for DT_RUNPATH and
DT_RPATH. While the other callers do no splitting. Do you see a caller
that splits at '/'?

In summary, the current patch doesn't change the handling of *just* $LIB
for SUID/SGID binaries.

Should all paths get tested for trusted paths for a SUID/SGID binary?
It seems like that's the right idea.

@@ -365,7 +383,8 @@ _dl_dst_substitute (struct link_map *l, const char *start,
 
   /* In SUID/SGID programs, after DST expansion the normalized
      path must be rooted in one of the trusted directories.  */
-  if (__glibc_unlikely (check_for_trusted)
+  if (__glibc_unlikely (__libc_enable_secure)
+      && l->l_type == lt_executable
       && !is_trusted_path_normalize (result, wp - result))
     {
       *result = '\0';

Just drop check_for_trusted, and execute the is_trusted_path_normalize
check for all SUID/SGID paths?

Cheers,
Carlos.
  

On 06/11/2018 04:54 AM, Carlos O'Donell wrote:
>>> I consider the following valid:
>>>
>>> [$ORIGIN/../$LIB]
>> I'm actually asking about this:
>>
>> [$LIB:$ORIGIN/../$LIB]
>>
>> Doesn't the current code reject this?
> The current code does not reject this. In fact it accepts $LIB in RPATH
> et. al. because there is no code to reject it. Only $ORIGIN has any
> restrictions.

I meant the $ORIGN part.  As far as I understand it, start will not be 
close to input for the second part containing origin (after the :), so 
this check in _dl_dst_substitute should reject it:

		      || (input != start + 1
			  || (input > start + 2 && input[-2] != ':'))))

I'm not sure that this is right.

> Should all paths get tested for trusted paths for a SUID/SGID binary?
> It seems like that's the right idea.
> 
> @@ -365,7 +383,8 @@ _dl_dst_substitute (struct link_map *l, const char *start,
>  
>    /* In SUID/SGID programs, after DST expansion the normalized
>       path must be rooted in one of the trusted directories.  */
> -  if (__glibc_unlikely (check_for_trusted)
> +  if (__glibc_unlikely (__libc_enable_secure)
> +      && l->l_type == lt_executable
>        && !is_trusted_path_normalize (result, wp - result))
>      {
>        *result = '\0';
> 
> Just drop check_for_trusted, and execute the is_trusted_path_normalize
> check for all SUID/SGID paths?

No, $ORIGIN in a trusted path is itself trusted (because you cannot 
manipulate it using hard links).  I think there are installations out 
there which depend on this.

Thanks,
Florian
  

On 06/11/2018 03:28 AM, Florian Weimer wrote:
> On 06/11/2018 04:54 AM, Carlos O'Donell wrote:
>>>> I consider the following valid:
>>>>
>>>> [$ORIGIN/../$LIB]
>>> I'm actually asking about this:
>>>
>>> [$LIB:$ORIGIN/../$LIB]
>>>
>>> Doesn't the current code reject this?
>> The current code does not reject this. In fact it accepts $LIB in RPATH
>> et. al. because there is no code to reject it. Only $ORIGIN has any
>> restrictions.
> 
> I meant the $ORIGN part.  As far as I understand it, start will not
> be close to input for the second part containing origin (after the
> :), so this check in _dl_dst_substitute should reject it:

The only code that splits on ':' is fillin_rpath, and it passes start
as the start of the whole DT_RUNPATH/DT_RPATH, and uses strsep so there
is no ':' to find.
 
>               || (input != start + 1
>               || (input > start + 2 && input[-2] != ':'))))
> 
> I'm not sure that this is right.

You are correct, and I fixed this for you based on earlier comments when
I sent out v4. Notice the v4 patch calls this out and I added a test
 case for it when you commented on it.

- Fix _dl_dst_substitute exception logic to check for NULL separator
  between non-first path since fillin_rpath and other callers will
  use strsep to split colon separated path elements. This fixes
  the case tested by test 19.

+             /* For SUID/GUID programs we normally ignore the path with
+                a DST in DT_RUNPATH, or DT_RPATH.  However, there is
+                one exception to this rule, and it is:
+
+                  * $ORIGIN appears first in the path element, and is
+                    the only thing in the element or is immediately
+                    followed by a path separator and the rest of the
+                    path.
+
+                  * The path element is rooted in a trusted directory.
+
+                This exception allows such programs to reference
+                shared libraries in subdirectories of trusted
+                directories.  The use case is one of general
+                organization and deployment flexibility.
+                Trusted directories are usually such paths as "/lib64"
+                or "/lib".  */
+             if (__glibc_unlikely (__libc_enable_secure)
+                 && !((input == start + 1
+                       || (input > start + 1 && input[-2] == '\0'))
+                      && (input[len] == '\0' || input[len] == '/')))

The sequence in question:

+                 && !((input == start + 1
+                       || (input > start + 1 && input[-2] == '\0'))

Will match:

[$ORIGIN\0...]
 ^--- start
  ^--- input

input == start + 1

or

[...........\0$ORIGIN]
 ^--- start    ^--- input

input > start + 1 (not the first path element)
&& input[-2] == '\0' (first in the path element)

Does that answer your question?

However, I see your point now that start itself can also be deleted.

If we are only ever handling single paths, we can remove start from the API
and just track the start of the singular sequence.

I will clean up the code further.

I will also remove a ":" check in is_trusted_path_normalize().

>> Should all paths get tested for trusted paths for a SUID/SGID binary?
>> It seems like that's the right idea.
>>
>> @@ -365,7 +383,8 @@ _dl_dst_substitute (struct link_map *l, const char *start,
>>  
>>    /* In SUID/SGID programs, after DST expansion the normalized
>>       path must be rooted in one of the trusted directories.  */
>> -  if (__glibc_unlikely (check_for_trusted)
>> +  if (__glibc_unlikely (__libc_enable_secure)
>> +      && l->l_type == lt_executable
>>        && !is_trusted_path_normalize (result, wp - result))
>>      {
>>        *result = '\0';
>>
>> Just drop check_for_trusted, and execute the is_trusted_path_normalize
>> check for all SUID/SGID paths?
> 
> No, $ORIGIN in a trusted path is itself trusted (because you cannot
> manipulate it using hard links).  I think there are installations out
> there which depend on this.

Understood.

I think a clearer statement would be like this:

The $LIB and $PLATFORM DST cannot in any way be manipulated by the caller
because they are fixed values that are set by the dynamic loader and therefore
any paths using just $LIB or $PLATFORM need not be checked for trust, the
authors of the binaries themselves are trusted to have designed this correctly.
Only $ORIGIN is tested in this way because it may be manipulated in some ways
with hard links.

I will add something like this as a comment.

Cheers,
Carlos.