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

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

On 06/07/2018 08:16 AM, Florian Weimer wrote:
> On 06/06/2018 10:18 PM, Carlos O'Donell wrote:
>> +/* Process INPUT for DSTs and store in RESULT using the information from
>> +   link map L to resolve the DSTs.  The value of START must equal the
>> +   start of the parent string if INPUT is a substring sequence being
>> +   parsed with path separators e.g. $ORIGIN:$PLATFORM.  */
>>   char *
>> -_dl_dst_substitute (struct link_map *l, const char *name, char *result)
>> +_dl_dst_substitute (struct link_map *l, const char *start,
>> +            const char *input, char *result)
> 
> The comment should describe the storage requirements for RESULT.

Done, I'll add that to v4.

The answer is: As much as DL_DST_REQUIRED tells you you need.

We use DL_DST_REQUIRED in both expand_dst (used for NEEDED, AUX, and FILTER),
and in expand_dynamic_string_token (used for RPATH from fillin_rpath).

> I'm a bit worried about this:
> 
>       else if (len != 0)
>         {
>           /* We cannot use this path element, the value of the
>          replacement is unknown.  */
>           check_for_trusted = false;
>           wp = last_elem;
>           break;
>         }
> 
> Does this really do the right thing for $ORIGIN/../$LIB:/foo/$ORIGIN?
> I would have expected a trusted path check for the first component in
> this case.

That depends from which input this came from.

There are two ways to this code:

(1) _dl_map_object -> DT_RPATH/RUNPATH -> fillin_rpath -> _dl_dst_substitute

(2) _dl_map_object_deps -> DT_NEEDED/AUXILIARY/FILTER -> expand_dst -> _dl_dst_substitute

I assume that you intend (1), and that ':' is to be interpreted as the
path separator in a valid list-of-paths sequence.

I also assume we are talking about SUID/SGID.

The alternative is that you mean (2), and that you have a really odd
name for the dependency of the binary, or it's a mistake made by the
developer which might be exploited by an attacker. We can talk about
this in your other email regarding ':'-splitting in fillin_rpath.

Given that you mean (1), and in fillin_rpath we split on ':' then the
sequence arriving at _dl_dst_substitute is [$ORIGIN/../$LIB], and we
can substitute for $ORIGIN and $LIB without error, both of them return
a valid repl value and length, and we never set check_for_trusted to
false. Therefore the first component *does* get a trusted path check.

Verified here:

Test 18  [SUID]: Verify RPATH $ORIGIN/../$LIB:/foo/$ORIGIN loads from /lib64.
      6277:	decompose_rpath: Call fillin_rpath
      6277:	fillin_rpath (begin)
      6277:	expand_dynamic_string_token input=$ORIGIN/../$LIB, start=$ORIGIN/../$LIB
      6277:	_dl_dst_count $ORIGIN/../$LIB (7fa91c1d4d20)
      6277:	is_dst input=ORIGIN/../$LIB (begin)
      6277:	is_dst ilen=6 rlen=6
      6277:	is_dst ilen=6 input=ORIGIN/../$LIB (7fa91c1d4d21) (ref=ORIGIN)
      6277:	_dl_dst_count next /../$LIB
      6277:	is_dst input=LIB (begin)
      6277:	is_dst ilen=3 rlen=6
      6277:	is_dst input=LIB (begin)
      6277:	is_dst ilen=3 rlen=8
      6277:	is_dst input=LIB (begin)
      6277:	is_dst ilen=3 rlen=3
      6277:	is_dst ilen=3 input=LIB (7fa91c1d4d2c) (ref=LIB)
      6277:	_dl_dst_count next 
      6277:	_dl_dst_count cnt 2
      6277:	DL_DST_REQUIRED l_origin 7fa91c1d4d60
      6277:	_dl_dst_substitute input=$ORIGIN/../$LIB start=$ORIGIN/../$LIB
      6277:	is_dst input=ORIGIN/../$LIB (begin)
      6277:	is_dst ilen=6 rlen=6
      6277:	is_dst ilen=6 input=ORIGIN/../$LIB (7fa91c1d4d21) (ref=ORIGIN)
      6277:	_dl_dst_substitute 7fa91c1d4d60
      6277:	is_dst input=LIB (begin)
      6277:	is_dst ilen=3 rlen=6
      6277:	is_dst input=LIB (begin)
      6277:	is_dst ilen=3 rlen=8
      6277:	is_dst input=LIB (begin)
      6277:	is_dst ilen=3 rlen=3
      6277:	is_dst ilen=3 input=LIB (7fa91c1d4d2c) (ref=LIB)
      6277:	_dl_dst_substitute 7fa91bfcc457
      6277:	is_trusted_path_normalize: /root/../lib64 14
      6277:	is_trusted_path_normalize: Transformed path /lib64/
      6277:	is_trusted_path_normalize: Path IS trusted.
      6277:	expand_dynamic_string_token input=/foo/$ORIGIN, start=$ORIGIN/../$LIB
      6277:	_dl_dst_count $ORIGIN (7fa91c1d4d35)
      6277:	is_dst input=ORIGIN (begin)
      6277:	is_dst ilen=6 rlen=6
      6277:	is_dst ilen=6 input=ORIGIN (7fa91c1d4d36) (ref=ORIGIN)
      6277:	_dl_dst_count next 
      6277:	_dl_dst_count cnt 1
      6277:	_dl_dst_substitute input=/foo/$ORIGIN start=$ORIGIN/../$LIB
      6277:	is_dst input=ORIGIN (begin)
      6277:	is_dst ilen=6 rlen=6
      6277:	is_dst ilen=6 input=ORIGIN (7fa91c1d4d36) (ref=ORIGIN)
      6277:	_dl_dst_substitute Invalid DST!
      6277:	_dl_dst_substitute ffffffffffffffff
      6277:	security = 1
      6277:	security = 1
      6277:	security = 1
      6277:	security = 1
PASS: Correct primary library.

The 'Invalid DST!' comes from a debug in the security checking code which
notices '/foo/$ORIGIN' doesn't meet the rules for SUID/SGID (that $ORIGIN
is first and it's rooted in a trusted direcotry) and sets repl to -1.

Therefore this works as expected for the case you cite.

What about [$ORIGIN/../$FOO], which is one valid, and one invalid DST?

This might be what you intended to write above, but I'd rather just cover
both cases to save the round-trip on the review.

     12187:	decompose_rpath: Call fillin_rpath
     12187:	fillin_rpath (begin)
     12187:	expand_dynamic_string_token input=$ORIGIN/../$FOO, start=$ORIGIN/../$FOO
     12187:	_dl_dst_count $ORIGIN/../$FOO (7fbf5ff90d20)
     12187:	is_dst input=ORIGIN/../$FOO (begin)
     12187:	is_dst ilen=6 rlen=6
     12187:	is_dst ilen=6 input=ORIGIN/../$FOO (7fbf5ff90d21) (ref=ORIGIN)
     12187:	_dl_dst_count next /../$FOO
     12187:	is_dst input=FOO (begin)
     12187:	is_dst ilen=3 rlen=6
     12187:	is_dst input=FOO (begin)
     12187:	is_dst ilen=3 rlen=8
     12187:	is_dst input=FOO (begin)
     12187:	is_dst ilen=3 rlen=3
     12187:	is_dst memcmp failed.
     12187:	_dl_dst_count next FOO
     12187:	_dl_dst_count cnt 1
     12187:	DL_DST_REQUIRED l_origin 7fbf5ff90d60
     12187:	_dl_dst_substitute input=$ORIGIN/../$FOO start=$ORIGIN/../$FOO
     12187:	is_dst input=ORIGIN/../$FOO (begin)
     12187:	is_dst ilen=6 rlen=6
     12187:	is_dst ilen=6 input=ORIGIN/../$FOO (7fbf5ff90d21) (ref=ORIGIN)
     12187:	_dl_dst_substitute 7fbf5ff90d60
     12187:	is_dst input=FOO (begin)
     12187:	is_dst ilen=3 rlen=6
     12187:	is_dst input=FOO (begin)
     12187:	is_dst ilen=3 rlen=8
     12187:	is_dst input=FOO (begin)
     12187:	is_dst ilen=3 rlen=3
     12187:	is_dst memcmp failed.
     12187:	_dl_dst_substitute 0
     12187:	is_trusted_path_normalize: /root/../$FOO 13
     12187:	is_trusted_path_normalize: Transformed path /$FOO/
     12187:	is_trusted_path_normalize: Path is NOT trusted.
     12187:	expand_dynamic_string_token input=/foo/$ORIGIN, start=$ORIGIN/../$FOO
     12187:	_dl_dst_count $ORIGIN (7fbf5ff90d35)
     12187:	is_dst input=ORIGIN (begin)
     12187:	is_dst ilen=6 rlen=6
     12187:	is_dst ilen=6 input=ORIGIN (7fbf5ff90d36) (ref=ORIGIN)
     12187:	_dl_dst_count next 
     12187:	_dl_dst_count cnt 1
     12187:	_dl_dst_substitute input=/foo/$ORIGIN start=$ORIGIN/../$FOO
     12187:	is_dst input=ORIGIN (begin)
     12187:	is_dst ilen=6 rlen=6
     12187:	is_dst ilen=6 input=ORIGIN (7fbf5ff90d36) (ref=ORIGIN)
     12187:	_dl_dst_substitute Invalid DST!
     12187:	_dl_dst_substitute ffffffffffffffff
     12187:	security = 1
     12187:	security = 1
     12187:	security = 1
     12187:	security = 1

Here, the $FOO is left unexpanded, we don't have a matching DST,
and so it's copied directly to the output, and has to go through
a trusted directory check which it fails.

The only time the code you quote is executed, this code:

 338           else if (len != 0)
 339             {
 340               /* We cannot use this path element, the value of the
 341                  replacement is unknown.  */
 342               check_for_trusted = false;
 343               wp = last_elem;
 344               break;
 345             }

Is when we find a DST we know, say $LIB, but DL_DST_LIB is invalid
e.g. set to -1, indicating that $LIB's value is unknown, in which case
[$ORIGIN/../$LIB] is entirely considered unknown, and *discarded* (which
is what 'wp = last_elem' does).

For v4 I'm going to clean up _dl_dst_substitute to point out that we
only take individual path elements of a multi-path sequence.

I believe this answers your question. Please clarify if I have not.

Cheers,
Carlos.