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

login
register
mail settings
Submitter Carlos O'Donell
Date June 6, 2018, 4:10 p.m.
Message ID <cce5064a-c6c8-6676-5088-d737b9a7c259@redhat.com>
Download mbox | patch
Permalink /patch/27669/
State New
Headers show

Comments

Carlos O'Donell - June 6, 2018, 4:10 p.m.
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.

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 49 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              | 174 +++++++++++++++++++++++++++++++--------------
 sysdeps/generic/ldsodefs.h |   5 +-
 6 files changed, 155 insertions(+), 71 deletions(-)
Andreas Schwab - June 6, 2018, 4:30 p.m.
On Jun 06 2018, Carlos O'Donell <carlos@redhat.com> wrote:

> +  /* Find longest valid input sequence.  */
> +  ilen = 0;
> +  while ((input[ilen] >= 'A' && input[ilen] <= 'Z')
> +	 || (input[ilen] >= 'a' && input[ilen] <= 'z')
> +	 || (input[ilen] >= '0' && input[ilen] <= '9')
> +	 || (input[ilen] == '_'))
> +    ++ilen;
> +
> +  rlen = strlen (ref);
> +
> +  /* Can't be the DST we are looking for.  */
> +  if (rlen != ilen)
> +    return 0;

Why do you need that?  Just compare, then check the next character.

Andreas.
Carlos O'Donell - June 6, 2018, 5:10 p.m.
On 06/06/2018 12:30 PM, Andreas Schwab wrote:
> On Jun 06 2018, Carlos O'Donell <carlos@redhat.com> wrote:
> 
>> +  /* Find longest valid input sequence.  */
>> +  ilen = 0;
>> +  while ((input[ilen] >= 'A' && input[ilen] <= 'Z')
>> +	 || (input[ilen] >= 'a' && input[ilen] <= 'z')
>> +	 || (input[ilen] >= '0' && input[ilen] <= '9')
>> +	 || (input[ilen] == '_'))
>> +    ++ilen;
>> +
>> +  rlen = strlen (ref);
>> +
>> +  /* Can't be the DST we are looking for.  */
>> +  if (rlen != ilen)
>> +    return 0;
> 
> Why do you need that?  Just compare, then check the next character.

Are you suggesting that:
~~~
rlen = strlen (ref);

/* Can't be the DST we are looking for.  */
if (rlen != ilen)
  return 0;
~~~
Can be dropped because we are going to compare the strings anyway?

I can do that.

Cheers,
Carlos.
Florian Weimer - June 6, 2018, 5:28 p.m.
On 06/06/2018 07:10 PM, Carlos O'Donell wrote:
> On 06/06/2018 12:30 PM, Andreas Schwab wrote:
>> On Jun 06 2018, Carlos O'Donell <carlos@redhat.com> wrote:
>>
>>> +  /* Find longest valid input sequence.  */
>>> +  ilen = 0;
>>> +  while ((input[ilen] >= 'A' && input[ilen] <= 'Z')
>>> +	 || (input[ilen] >= 'a' && input[ilen] <= 'z')
>>> +	 || (input[ilen] >= '0' && input[ilen] <= '9')
>>> +	 || (input[ilen] == '_'))
>>> +    ++ilen;
>>> +
>>> +  rlen = strlen (ref);
>>> +
>>> +  /* Can't be the DST we are looking for.  */
>>> +  if (rlen != ilen)
>>> +    return 0;
>>
>> Why do you need that?  Just compare, then check the next character.
> 
> Are you suggesting that:
> ~~~
> rlen = strlen (ref);
> 
> /* Can't be the DST we are looking for.  */
> if (rlen != ilen)
>    return 0;
> ~~~
> Can be dropped because we are going to compare the strings anyway?
> 
> I can do that.

If you compare the lengths first, yo ucan use memcmp.

You could compute the length of ref in an inline wrapper function, so 
that GCC will turn it into a compile-time constant.

Thanks,
Florian
Carlos O'Donell - June 6, 2018, 6:49 p.m.
On 06/06/2018 01:28 PM, Florian Weimer wrote:
> On 06/06/2018 07:10 PM, Carlos O'Donell wrote:
>> On 06/06/2018 12:30 PM, Andreas Schwab wrote:
>>> On Jun 06 2018, Carlos O'Donell <carlos@redhat.com> wrote:
>>>
>>>> +  /* Find longest valid input sequence.  */
>>>> +  ilen = 0;
>>>> +  while ((input[ilen] >= 'A' && input[ilen] <= 'Z')
>>>> +     || (input[ilen] >= 'a' && input[ilen] <= 'z')
>>>> +     || (input[ilen] >= '0' && input[ilen] <= '9')
>>>> +     || (input[ilen] == '_'))
>>>> +    ++ilen;
>>>> +
>>>> +  rlen = strlen (ref);
>>>> +
>>>> +  /* Can't be the DST we are looking for.  */
>>>> +  if (rlen != ilen)
>>>> +    return 0;
>>>
>>> Why do you need that?  Just compare, then check the next character.
>>
>> Are you suggesting that:
>> ~~~
>> rlen = strlen (ref);
>>
>> /* Can't be the DST we are looking for.  */
>> if (rlen != ilen)
>>    return 0;
>> ~~~
>> Can be dropped because we are going to compare the strings anyway?
>>
>> I can do that.
> 
> If you compare the lengths first, yo ucan use memcmp.
> 
> You could compute the length of ref in an inline wrapper function, so that GCC will turn it into a compile-time constant.

The memcmp is a good suggestion.

Like so?

 
-  len = 0;
-  while (name[len] == str[len] && name[len] != '\0')
-    ++len;
+  /* Find longest valid input sequence.  */
+  ilen = 0;
+  while ((input[ilen] >= 'A' && input[ilen] <= 'Z')
+        || (input[ilen] >= 'a' && input[ilen] <= 'z')
+        || (input[ilen] >= '0' && input[ilen] <= '9')
+        || (input[ilen] == '_'))
+    ++ilen;
+
+  rlen = strlen (ref);
+
+  /* Can't be the DST we are looking for.  */
+  if (rlen != ilen)
+    return 0;
+
+  /* Compare the DST (no strncmp this early in startup).  */
+  if (memcmp (input, ref, ilen) != 0)
+    return 0;


The inline wrapper seems overkill for the rare is_dst() case
continuing past the strchr for '$'.

Cheers,
Carlos.
Florian Weimer - June 6, 2018, 6:56 p.m.
On 06/06/2018 08:49 PM, Carlos O'Donell wrote:
> On 06/06/2018 01:28 PM, Florian Weimer wrote:
>> On 06/06/2018 07:10 PM, Carlos O'Donell wrote:
>>> On 06/06/2018 12:30 PM, Andreas Schwab wrote:
>>>> On Jun 06 2018, Carlos O'Donell <carlos@redhat.com> wrote:
>>>>
>>>>> +  /* Find longest valid input sequence.  */
>>>>> +  ilen = 0;
>>>>> +  while ((input[ilen] >= 'A' && input[ilen] <= 'Z')
>>>>> +     || (input[ilen] >= 'a' && input[ilen] <= 'z')
>>>>> +     || (input[ilen] >= '0' && input[ilen] <= '9')
>>>>> +     || (input[ilen] == '_'))
>>>>> +    ++ilen;
>>>>> +
>>>>> +  rlen = strlen (ref);
>>>>> +
>>>>> +  /* Can't be the DST we are looking for.  */
>>>>> +  if (rlen != ilen)
>>>>> +    return 0;
>>>>
>>>> Why do you need that?  Just compare, then check the next character.
>>>
>>> Are you suggesting that:
>>> ~~~
>>> rlen = strlen (ref);
>>>
>>> /* Can't be the DST we are looking for.  */
>>> if (rlen != ilen)
>>>     return 0;
>>> ~~~
>>> Can be dropped because we are going to compare the strings anyway?
>>>
>>> I can do that.
>>
>> If you compare the lengths first, yo ucan use memcmp.
>>
>> You could compute the length of ref in an inline wrapper function, so that GCC will turn it into a compile-time constant.
> 
> The memcmp is a good suggestion.
> 
> Like so?
> 
>   
> -  len = 0;
> -  while (name[len] == str[len] && name[len] != '\0')
> -    ++len;
> +  /* Find longest valid input sequence.  */
> +  ilen = 0;
> +  while ((input[ilen] >= 'A' && input[ilen] <= 'Z')
> +        || (input[ilen] >= 'a' && input[ilen] <= 'z')
> +        || (input[ilen] >= '0' && input[ilen] <= '9')
> +        || (input[ilen] == '_'))
> +    ++ilen;
> +
> +  rlen = strlen (ref);
> +
> +  /* Can't be the DST we are looking for.  */
> +  if (rlen != ilen)
> +    return 0;
> +
> +  /* Compare the DST (no strncmp this early in startup).  */
> +  if (memcmp (input, ref, ilen) != 0)
> +    return 0;

Yes, that is what I had in mind.

> The inline wrapper seems overkill for the rare is_dst() case
> continuing past the strchr for '$'.

I have no strong preference either way.

Thanks,
Florian
Carlos O'Donell - June 6, 2018, 8:03 p.m.
On 06/06/2018 02:56 PM, Florian Weimer wrote:
> On 06/06/2018 08:49 PM, Carlos O'Donell wrote:
>> On 06/06/2018 01:28 PM, Florian Weimer wrote:
>>> On 06/06/2018 07:10 PM, Carlos O'Donell wrote:
>>>> On 06/06/2018 12:30 PM, Andreas Schwab wrote:
>>>>> On Jun 06 2018, Carlos O'Donell <carlos@redhat.com> wrote:
>>>>>
>>>>>> +  /* Find longest valid input sequence.  */
>>>>>> +  ilen = 0;
>>>>>> +  while ((input[ilen] >= 'A' && input[ilen] <= 'Z')
>>>>>> +     || (input[ilen] >= 'a' && input[ilen] <= 'z')
>>>>>> +     || (input[ilen] >= '0' && input[ilen] <= '9')
>>>>>> +     || (input[ilen] == '_'))
>>>>>> +    ++ilen;
>>>>>> +
>>>>>> +  rlen = strlen (ref);
>>>>>> +
>>>>>> +  /* Can't be the DST we are looking for.  */
>>>>>> +  if (rlen != ilen)
>>>>>> +    return 0;
>>>>>
>>>>> Why do you need that?  Just compare, then check the next character.
>>>>
>>>> Are you suggesting that:
>>>> ~~~
>>>> rlen = strlen (ref);
>>>>
>>>> /* Can't be the DST we are looking for.  */
>>>> if (rlen != ilen)
>>>>     return 0;
>>>> ~~~
>>>> Can be dropped because we are going to compare the strings anyway?
>>>>
>>>> I can do that.
>>>
>>> If you compare the lengths first, yo ucan use memcmp.
>>>
>>> You could compute the length of ref in an inline wrapper function, so that GCC will turn it into a compile-time constant.
>>
>> The memcmp is a good suggestion.
>>
>> Like so?
>>
>>   -  len = 0;
>> -  while (name[len] == str[len] && name[len] != '\0')
>> -    ++len;
>> +  /* Find longest valid input sequence.  */
>> +  ilen = 0;
>> +  while ((input[ilen] >= 'A' && input[ilen] <= 'Z')
>> +        || (input[ilen] >= 'a' && input[ilen] <= 'z')
>> +        || (input[ilen] >= '0' && input[ilen] <= '9')
>> +        || (input[ilen] == '_'))
>> +    ++ilen;
>> +
>> +  rlen = strlen (ref);
>> +
>> +  /* Can't be the DST we are looking for.  */
>> +  if (rlen != ilen)
>> +    return 0;
>> +
>> +  /* Compare the DST (no strncmp this early in startup).  */
>> +  if (memcmp (input, ref, ilen) != 0)
>> +    return 0;
> 
> Yes, that is what I had in mind.
> 
>> The inline wrapper seems overkill for the rare is_dst() case
>> continuing past the strchr for '$'.
> 
> I have no strong preference either way.

Well, this turned up another issue to consider.

Test 17  [SUID]: Verify ${} (invalid) in DT_NEEDED discards the DT_NEEDED.
     13496:     _dl_dst_count ${}liborigin.so (4003a9)
     13496:     is_dst input={}liborigin.so (begin)
     13496:     is_dst ilen=0 rlen=6
     13496:     is_dst input={}liborigin.so (begin)
     13496:     is_dst ilen=0 rlen=8
     13496:     is_dst input={}liborigin.so (begin)
     13496:     is_dst ilen=0 rlen=3
     13496:     _dl_dst_count next {}liborigin.so
     13496:     _dl_dst_count cnt 0
     13496:     security = 1
     13496:     security = 1
     13496:     security = 1
     13496:     security = 1
origin: Function called.
FAIL: Incorrectly allowed DT_NEEDED with ${}.

For ilen == 0 cases like ${} or '$' the DST is not counted, since
it's invalid, has length 0, and doesn't meet the ELF gABI for being
considered a DST, and we invoke:
~~~
If a dollar sign is not immediately followed by a name or a 
brace-enclosed name, the behavior of the dynamic linker is unspecified. 
~~~

In theory ${} or '$' is an invalid name sequence.

Similar invalid name sequences like '${libnoendcurly.so' are possible and
they will appear unsubstituted in the output for AT_SECURE.

The question is:

There is a difference between a invalid DST sequence (one
which doesn't follow the ELF gABI), and one which does conform but
for which there is no substitution (unknown name).

So currently *valid* DST sequences that have no substitution are
counted and removed entirely (skipped) or can cause load errors.

However, invalid sequences, as above, are allowed unsubstituted
to pass into the search paths as literals.

Should, once we detect $ as the start of the DST sequences, cause
the whole sequence to be skipped even if it's an invalid sequence?

I think we need to be conservative here and just let these invalid
sequences pass through.

Cheers,
Carlos.
Andreas Schwab - June 7, 2018, 6:48 a.m.
On Jun 06 2018, Carlos O'Donell <carlos@redhat.com> wrote:

> On 06/06/2018 12:30 PM, Andreas Schwab wrote:
>> On Jun 06 2018, Carlos O'Donell <carlos@redhat.com> wrote:
>> 
>>> +  /* Find longest valid input sequence.  */
>>> +  ilen = 0;
>>> +  while ((input[ilen] >= 'A' && input[ilen] <= 'Z')
>>> +	 || (input[ilen] >= 'a' && input[ilen] <= 'z')
>>> +	 || (input[ilen] >= '0' && input[ilen] <= '9')
>>> +	 || (input[ilen] == '_'))
>>> +    ++ilen;
>>> +
>>> +  rlen = strlen (ref);
>>> +
>>> +  /* Can't be the DST we are looking for.  */
>>> +  if (rlen != ilen)
>>> +    return 0;
>> 
>> Why do you need that?  Just compare, then check the next character.
>
> Are you suggesting that:
> ~~~
> rlen = strlen (ref);
>
> /* Can't be the DST we are looking for.  */
> if (rlen != ilen)
>   return 0;
> ~~~
> Can be dropped because we are going to compare the strings anyway?

Drop the whole part.  When you have compared the string you know that it
is valid so far, so what's the value of validating it twice?

Andreas.
Carlos O'Donell - June 8, 2018, 2:08 a.m.
On 06/07/2018 02:48 AM, Andreas Schwab wrote:
> On Jun 06 2018, Carlos O'Donell <carlos@redhat.com> wrote:
> 
>> On 06/06/2018 12:30 PM, Andreas Schwab wrote:
>>> On Jun 06 2018, Carlos O'Donell <carlos@redhat.com> wrote:
>>>
>>>> +  /* Find longest valid input sequence.  */
>>>> +  ilen = 0;
>>>> +  while ((input[ilen] >= 'A' && input[ilen] <= 'Z')
>>>> +	 || (input[ilen] >= 'a' && input[ilen] <= 'z')
>>>> +	 || (input[ilen] >= '0' && input[ilen] <= '9')
>>>> +	 || (input[ilen] == '_'))
>>>> +    ++ilen;
>>>> +
>>>> +  rlen = strlen (ref);
>>>> +
>>>> +  /* Can't be the DST we are looking for.  */
>>>> +  if (rlen != ilen)
>>>> +    return 0;
>>>
>>> Why do you need that?  Just compare, then check the next character.
>>
>> Are you suggesting that:
>> ~~~
>> rlen = strlen (ref);
>>
>> /* Can't be the DST we are looking for.  */
>> if (rlen != ilen)
>>   return 0;
>> ~~~
>> Can be dropped because we are going to compare the strings anyway?
> 
> Drop the whole part.  When you have compared the string you know that it
> is valid so far, so what's the value of validating it twice?

Careful, is_dst () takes as input the start of a DST sequence,
but that sequence is not validated yet.

We must compare the longest legal sequence with that of the DST
comparison. This is different from comparing *both* sequences.
The old glibc code was wrong in this regard.

In the old code we had:

  len = 0;
  while (name[len] == str[len] && name[len] != '\0')
    ++len;

What if name was '$ORIGIN-' but str was '$ORIGIN'?

With the above code we reject '$ORIGIN-' != '$ORIGIN', but this is
not true.

I have added a test for this in my test case:

With system glibc:
Test 1e: Test that RPATH of $ORIGIN- loads from /root- as expected.
FAIL: Wrong primary library.

With patched glibc:
Test 1e: Test that RPATH of $ORIGIN- loads from /root- as expected.
     24538:	expand_dynamic_string_token input=$ORIGIN-, start=$ORIGIN-
     24538:	_dl_dst_count $ORIGIN- (7f7934f38d20)
     24538:	is_dst input=ORIGIN- (begin)
     24538:	is_dst ilen=6 rlen=6
     24538:	is_dst ilen=6 input=ORIGIN- (7f7934f38d21) (ref=ORIGIN)
     24538:	_dl_dst_count next -
     24538:	_dl_dst_count cnt 1
     24538:	DL_DST_REQUIRED l_origin 7f7934f38d40
     24538:	_dl_dst_substitute input=$ORIGIN- start=$ORIGIN-
     24538:	is_dst input=ORIGIN- (begin)
     24538:	is_dst ilen=6 rlen=6
     24538:	is_dst ilen=6 input=ORIGIN- (7f7934f38d21) (ref=ORIGIN)
     24538:	_dl_dst_substitute 7f7934f38d40
     24538:	security = 0
     24538:	security = 0
     24538:	security = 0
     24538:	security = 0
PASS: Correct primary library.

The above code also makes is_dst () robust against mistakes if the caller
violates the ELF gABI specification by trying to use a DST which
doesn't conform to the [A-Za-z0-9_] requirement.

To do that the code does:

* Follow the ELF gABI rules to get the longest name.

* Compare the longest name to the DST we are looking to find.

If we didn't do the "longest name" computation we might allow both
glibc to use invalid DST names, and applications to get away with using
them without error and have them work.

With my patch the developer would have to knowingly adjust the allowed
longest name sequence, and that's a conscious change which is good
to avoid mistakes.

There is a bit of a performance cost to pay here, but I've not seen
any argument that this code is highly performance sensitive. If it
is really performance sensitive then we can talk about many other
ways to make it faster.

Again, this is a change from previous behaviour, but brings us more
sane parsing of the DSTs IMO.

Therefore I don't think this code has to change.

Cheers,
Carlos.
Andreas Schwab - June 11, 2018, 8:04 a.m.
On Jun 07 2018, Carlos O'Donell <carlos@redhat.com> wrote:

> Careful, is_dst () takes as input the start of a DST sequence,
> but that sequence is not validated yet.

You have already validated the prefix by comparing it with the string,
and you only have to check the next character.

> What if name was '$ORIGIN-' but str was '$ORIGIN'?

That would match of course, since `-' is not part of the DST (with the
relaxed rules).

Andreas.
Carlos O'Donell - June 12, 2018, 3:08 a.m.
On 06/11/2018 04:04 AM, Andreas Schwab wrote:
> On Jun 07 2018, Carlos O'Donell <carlos@redhat.com> wrote:
> 
>> Careful, is_dst () takes as input the start of a DST sequence,
>> but that sequence is not validated yet.
> 
> You have already validated the prefix by comparing it with the string,
> and you only have to check the next character.

By validating it against the abstract definition of the ELF gABI spec 
for a DST name it prevents any future caller from accidentally straying
outside of that contract without having to make two changes: (1) change 
the comparison DST and (2) change what we accept in is_dst().

Also I find the code ends up being simpler. Right now we compute the
longest DST name possible and then the checks are much easier after
that.

Is there a strong performance argument to be made for not doing the
validation?

Cheers,
Carlos.
Andreas Schwab - June 12, 2018, 7:31 a.m.
On Jun 11 2018, Carlos O'Donell <carlos@redhat.com> wrote:

> By validating it against the abstract definition of the ELF gABI spec 
> for a DST name it prevents any future caller from accidentally straying
> outside of that contract without having to make two changes: (1) change 
> the comparison DST and (2) change what we accept in is_dst().

Why would we add any use of is_dst with an invalid DST?  That doesn't
make sense.

> Also I find the code ends up being simpler.

I don't agree.  It is overly complex for no reason.

Andreas.
Carlos O'Donell - June 12, 2018, 12:45 p.m.
On 06/12/2018 03:31 AM, Andreas Schwab wrote:
> On Jun 11 2018, Carlos O'Donell <carlos@redhat.com> wrote:
> 
>> By validating it against the abstract definition of the ELF gABI spec 
>> for a DST name it prevents any future caller from accidentally straying
>> outside of that contract without having to make two changes: (1) change 
>> the comparison DST and (2) change what we accept in is_dst().
> 
> Why would we add any use of is_dst with an invalid DST?  That doesn't
> make sense.

An invalid DST would be a mistake. The point is to catch mistakes by
enforcing the rules for valid DSTs.

>> Also I find the code ends up being simpler.
> 
> I don't agree.  It is overly complex for no reason.

There is a reason. You don't think the reason has any value though,
and that's different. That's also OK.

I'll post a v6 with the validation removed, I appreciate your point
of view. I'll work with Florian to get strncmp usage in place.

Would you be OK with something like this?

  /* Is a ${...} input sequence?  */
  if (input[0] == '{')
    {
      is_curly = true;
      ++input;
    }

  /* Check for matching name, following closing curly brace (if
     required), or trailing characters which are part of an
     identifier.  */
  size_t rlen = strlen (ref);
  if (strncmp (input, ref, rlen) != 0
      || (is_curly && input[rlen] != '}')
      || ((input[rlen] >= 'A' && input[rlen] <= 'Z')
          || (input[rlen] >= 'a' && input[rlen] <= 'z')
          || (input[rlen] >= '0' && input[rlen] <= '9')
          || (input[rlen] == '_')))
    return 0;
  
  if (is_curly)
    /* Count the two curly braces.  */
    return ilen + 2;
  else
    return ilen;

Cheers,
Carlos.g
Andreas Schwab - June 12, 2018, 1:02 p.m.
On Jun 12 2018, Carlos O'Donell <carlos@redhat.com> wrote:

> An invalid DST would be a mistake. The point is to catch mistakes by
> enforcing the rules for valid DSTs.

The chance for that is minuscule.

Andreas.
Carlos O'Donell - June 12, 2018, 1:03 p.m.
On 06/12/2018 09:02 AM, Andreas Schwab wrote:
> On Jun 12 2018, Carlos O'Donell <carlos@redhat.com> wrote:
> 
>> An invalid DST would be a mistake. The point is to catch mistakes by
>> enforcing the rules for valid DSTs.
> 
> The chance for that is minuscule.

Agreed, which is why I appreciate your input.

What did you think of the suggested final version?

Cheers,
Carlos.

Patch

diff --git a/ChangeLog b/ChangeLog
index a3bc2bf31e..f6dbc961e2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,23 @@ 
+2018-06-06  Carlos O'Donell  <carlos@redhat.com>
+	    Andreas Schwab  <schwab@suse.de>
+	    Dmitry V. Levin  <ldv@altlinux.org>
+
+	[BZ #23102]
+	[BZ #21942]
+	[BZ #18018]
+	[BZ #23259]
+	CVE-2011-0536
+	* elf/dl-load.c (is_dst): Comment.  Support ELF gABI.
+	(_dl_dst_count): Comment.  Simplify and count DSTs.
+	(_dl_dst_substitute): Comment.  Support __libc_enable_secure handling.
+	Add string start to arguments.
+	(expand_dybamic_string_token): Comment. Accept path start.
+	(fillin_rpath): Pass string start to expand_dynamic_string_token.
+	* sysdeps/generic/ldsodefs.h: _dl_dst_substitute takes additiional
+	string start argument.
+	* elf/dl-deps.c (expand_dst): Adjust call to _dl_dst_substitute.
+	* elf/dl-dst.h: Remove DL_DST_COUNT.
+
 2018-06-05  Joseph Myers  <joseph@codesourcery.com>
 
 	* sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h (HWCAP_DIT): New
diff --git a/NEWS b/NEWS
index e2a6f45121..0d0bc9ad4c 100644
--- a/NEWS
+++ b/NEWS
@@ -41,6 +41,16 @@  Major new features:
   NI_IDN_ALLOW_UNASSIGNED, NI_IDN_USE_STD3_ASCII_RULES) have been
   deprecated.  They no longer have any effect.
 
+* Parsing of dynamic string tokens in DT_RPATH, DT_RUNPATH, and DT_NEEDED
+  has been expanded to support the full range of ELF gABI expressions
+  including such constructs as '$ORIGIN$ORIGIN' (if valid).  For SUID/GUID
+  applications the rules have been further restricted, and where in the
+  past a dynamic string token sequence may have been interpreted as a
+  literal string it will now cause a load failure.  These load failures
+  were always considered unspecified behaviour from the perspective of the
+  dynamic loader, and for safety are now load errors e.g. /foo/${ORIGIN}.so
+  in DT_NEEDED results in a load failure now.
+
 Deprecated and removed features, and other changes affecting compatibility:
 
 * The nonstandard header files <libio.h> and <_G_config.h> are no longer
diff --git a/elf/dl-deps.c b/elf/dl-deps.c
index c975fcffd7..07b0859456 100644
--- a/elf/dl-deps.c
+++ b/elf/dl-deps.c
@@ -100,7 +100,7 @@  struct list
   ({									      \
     const char *__str = (str);						      \
     const char *__result = __str;					      \
-    size_t __dst_cnt = DL_DST_COUNT (__str);				      \
+    size_t __dst_cnt = _dl_dst_count (__str);				      \
 									      \
     if (__dst_cnt != 0)							      \
       {									      \
@@ -114,7 +114,7 @@  DST not allowed in SUID/SGID programs"));				      \
 	__newp = (char *) alloca (DL_DST_REQUIRED (l, __str, strlen (__str),  \
 						   __dst_cnt));		      \
 									      \
-	__result = _dl_dst_substitute (l, __str, __newp);		      \
+	__result = _dl_dst_substitute (l, __str, __str, __newp);	      \
 									      \
 	if (*__result == '\0')						      \
 	  {								      \
diff --git a/elf/dl-dst.h b/elf/dl-dst.h
index 32de5d225a..859032be0d 100644
--- a/elf/dl-dst.h
+++ b/elf/dl-dst.h
@@ -18,19 +18,6 @@ 
 
 #include "trusted-dirs.h"
 
-/* Determine the number of DST elements in the name.  Only if IS_PATH is
-   nonzero paths are recognized (i.e., multiple, ':' separated filenames).  */
-#define DL_DST_COUNT(name) \
-  ({									      \
-    size_t __cnt = 0;							      \
-    const char *__sf = strchr (name, '$');				      \
-									      \
-    if (__glibc_unlikely (__sf != NULL))				      \
-      __cnt = _dl_dst_count (__sf);					      \
-									      \
-    __cnt; })
-
-
 #ifdef SHARED
 # define IS_RTLD(l) (l) == &GL(dl_rtld_map)
 #else
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 431236920f..a3f0b3f435 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -177,76 +177,115 @@  is_trusted_path_normalize (const char *path, size_t len)
   return false;
 }
 
+/* Given a substring starting at INPUT, just after the DST '$' start
+   token, determine if INPUT contains DST token REF, following the
+   ELF gABI rules for DSTs:
 
+   * Longest possible sequence using the rules (greedy).
+
+   * Must start with a $ (enforced by caller).
+
+   * Must follow $ with one underscore or ASCII [A-Za-z] (caller
+     follows these rules for REF and we enforce the comparison)
+     or '{' (start curly quoted name).
+
+   * Must follow first two characters with zero or more [A-Za-z0-9_]
+     (enforced by caller) or '}' (end curly quoted name).
+
+   If the sequence is a DST matching REF then the length of the DST is
+   returned, otherwise 0.  */
 static size_t
-is_dst (const char *start, const char *name, const char *str, int secure)
+is_dst (const char *input, const char *ref)
 {
-  size_t len;
+  size_t len, ilen, rlen;
   bool is_curly = false;
 
-  if (name[0] == '{')
+  /* Is a ${...} input sequence?  */
+  if (input[0] == '{')
     {
       is_curly = true;
-      ++name;
+      ++input;
     }
 
+  /* Find longest valid input sequence.  */
+  ilen = 0;
+  while ((input[ilen] >= 'A' && input[ilen] <= 'Z')
+	 || (input[ilen] >= 'a' && input[ilen] <= 'z')
+	 || (input[ilen] >= '0' && input[ilen] <= '9')
+	 || (input[ilen] == '_'))
+    ++ilen;
+
+  rlen = strlen (ref);
+
+  /* Can't be the DST we are looking for.  */
+  if (rlen != ilen)
+    return 0;
+
+  /* Compare the DST (no strncmp this early in startup).  */
   len = 0;
-  while (name[len] == str[len] && name[len] != '\0')
+  while (input[len] == ref[len] && len < ilen)
     ++len;
 
+  /* Characters of input don't match DST.  */
+  if (len != ilen)
+    return 0;
+
   if (is_curly)
     {
-      if (name[len] != '}')
+      /* Invalid curly sequence!  */
+      if (input[ilen] != '}')
 	return 0;
 
-      /* Point again at the beginning of the name.  */
-      --name;
-      /* Skip over closing curly brace and adjust for the --name.  */
-      len += 2;
+      /* Count the two curly braces.  */
+      ilen += 2;
     }
-  else if (name[len] != '\0' && name[len] != '/')
-    return 0;
 
-  if (__glibc_unlikely (secure)
-      && ((name[len] != '\0' && name[len] != '/')
-	  || (name != start + 1)))
-    return 0;
-
-  return len;
+  return ilen;
 }
 
-
+/* INPUT is the start of a DST sequence at the first '$' occurrence.
+   If there is a DST we call into _dl_dst_count to count the number of
+   DSTs.  We count all known DSTs regardless of __libc_enable_secure;
+   the caller is responsible for enforcing the security of the
+   substitution rules (usually _dl_dst_substitute).  */
 size_t
-_dl_dst_count (const char *name)
+_dl_dst_count (const char *input)
 {
-  const char *const start = name;
   size_t cnt = 0;
 
+  input = strchr (input, '$');
+
+  /* Most likely there is no DST.  */
+  if (__glibc_likely (input == NULL))
+    return 0;
+
   do
     {
       size_t len;
 
-      /* $ORIGIN is not expanded for SUID/GUID programs (except if it
-	 is $ORIGIN alone) and it must always appear first in path.  */
-      ++name;
-      if ((len = is_dst (start, name, "ORIGIN", __libc_enable_secure)) != 0
-	  || (len = is_dst (start, name, "PLATFORM", 0)) != 0
-	  || (len = is_dst (start, name, "LIB", 0)) != 0)
+      ++input;
+      /* All DSTs must follow ELF gABI rules, see is_dst ().  */
+      if ((len = is_dst (input, "ORIGIN")) != 0
+	  || (len = is_dst (input, "PLATFORM")) != 0
+	  || (len = is_dst (input, "LIB")) != 0)
 	++cnt;
 
-      name = strchr (name + len, '$');
+      /* There may be more than one DST in the input.  */
+      input = strchr (input + len, '$');
     }
-  while (name != NULL);
+  while (input != NULL);
 
   return cnt;
 }
 
-
+/* 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)
 {
-  const char *const start = name;
-
   /* Now fill the result path.  While copying over the string we keep
      track of the start of the last path element.  When we come across
      a DST we copy over the value or (if the value is not available)
@@ -257,32 +296,54 @@  _dl_dst_substitute (struct link_map *l, const char *name, char *result)
 
   do
     {
-      if (__glibc_unlikely (*name == '$'))
+      if (__glibc_unlikely (*input == '$'))
 	{
 	  const char *repl = NULL;
 	  size_t len;
 
-	  ++name;
-	  if ((len = is_dst (start, name, "ORIGIN", __libc_enable_secure)) != 0)
+	  ++input;
+	  if ((len = is_dst (input, "ORIGIN")) != 0)
 	    {
-	      repl = l->l_origin;
+	      /* 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.
+		   * 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[len] != '\0' && input[len] != '/'
+		       && input[len] != ':')
+		      || (input != start + 1
+			  || (input > start + 2 && input[-2] != ':'))))
+		repl = (const char *) -1;
+	      else
+	        repl = l->l_origin;
+
 	      check_for_trusted = (__libc_enable_secure
 				   && l->l_type == lt_executable);
 	    }
-	  else if ((len = is_dst (start, name, "PLATFORM", 0)) != 0)
+	  else if ((len = is_dst (input, "PLATFORM")) != 0)
 	    repl = GLRO(dl_platform);
-	  else if ((len = is_dst (start, name, "LIB", 0)) != 0)
+	  else if ((len = is_dst (input, "LIB")) != 0)
 	    repl = DL_DST_LIB;
 
 	  if (repl != NULL && repl != (const char *) -1)
 	    {
 	      wp = __stpcpy (wp, repl);
-	      name += len;
+	      input += len;
 	    }
 	  else if (len > 1)
 	    {
 	      /* We cannot use this path element, the value of the
 		 replacement is unknown.  */
+	      check_for_trusted = false;
 	      wp = last_elem;
 	      break;
 	    }
@@ -292,10 +353,10 @@  _dl_dst_substitute (struct link_map *l, const char *name, char *result)
 	}
       else
 	{
-	  *wp++ = *name++;
+	  *wp++ = *input++;
 	}
     }
-  while (*name != '\0');
+  while (*input != '\0');
 
   /* In SUID/SGID programs, after $ORIGIN expansion the normalized
      path must be rooted in one of the trusted directories.  */
@@ -309,13 +370,17 @@  _dl_dst_substitute (struct link_map *l, const char *name, char *result)
 }
 
 
-/* Return copy of argument with all recognized dynamic string tokens
-   ($ORIGIN and $PLATFORM for now) replaced.  On some platforms it
-   might not be possible to determine the path from which the object
-   belonging to the map is loaded.  In this case the path element
-   containing $ORIGIN is left out.  */
+/* Return a malloc allocated copy of INPUT with all recognized DSTs
+   ($ORIGIN and $PLATFORM for now) replaced.  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.
+   On some platforms it might not be possible to determine the path
+   from which the object belonging to the map is loaded.  In this case
+   the path element containing $ORIGIN is left out.  On error NULL is
+   returned. */
 static char *
-expand_dynamic_string_token (struct link_map *l, const char *s)
+expand_dynamic_string_token (struct link_map *l, const char *start,
+			     const char *input)
 {
   /* We make two runs over the string.  First we determine how large the
      resulting string is and then we copy it over.  Since this is no
@@ -326,21 +391,21 @@  expand_dynamic_string_token (struct link_map *l, const char *s)
   char *result;
 
   /* Determine the number of DST elements.  */
-  cnt = DL_DST_COUNT (s);
+  cnt = _dl_dst_count (input);
 
   /* If we do not have to replace anything simply copy the string.  */
   if (__glibc_likely (cnt == 0))
-    return __strdup (s);
+    return __strdup (input);
 
   /* Determine the length of the substituted string.  */
-  total = DL_DST_REQUIRED (l, s, strlen (s), cnt);
+  total = DL_DST_REQUIRED (l, input, strlen (input), cnt);
 
   /* Allocate the necessary memory.  */
   result = (char *) malloc (total + 1);
   if (result == NULL)
     return NULL;
 
-  return _dl_dst_substitute (l, s, result);
+  return _dl_dst_substitute (l, start, input, result);
 }
 
 
@@ -387,6 +452,7 @@  fillin_rpath (char *rpath, struct r_search_path_elem **result, const char *sep,
 	      const char *what, const char *where, struct link_map *l)
 {
   char *cp;
+  char *start = rpath;
   size_t nelems = 0;
 
   while ((cp = __strsep (&rpath, sep)) != NULL)
@@ -398,7 +464,7 @@  fillin_rpath (char *rpath, struct r_search_path_elem **result, const char *sep,
       /* `strsep' can pass an empty string.  */
       if (*cp != '\0')
 	{
-	  to_free = cp = expand_dynamic_string_token (l, cp);
+	  to_free = cp = expand_dynamic_string_token (l, start, cp);
 
 	  /* expand_dynamic_string_token can return NULL in case of empty
 	     path or memory allocation failure.  */
@@ -2091,7 +2157,7 @@  _dl_map_object (struct link_map *loader, const char *name,
     {
       /* The path may contain dynamic string tokens.  */
       realname = (loader
-		  ? expand_dynamic_string_token (loader, name)
+		  ? expand_dynamic_string_token (loader, name, name)
 		  : __strdup (name));
       if (realname == NULL)
 	fd = -1;
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index 95dc87519b..688ff60785 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -1108,8 +1108,9 @@  extern const char *_dl_get_origin (void) attribute_hidden;
 extern size_t _dl_dst_count (const char *name) attribute_hidden;
 
 /* Substitute DST values.  */
-extern char *_dl_dst_substitute (struct link_map *l, const char *name,
-				 char *result) attribute_hidden;
+extern char *_dl_dst_substitute (struct link_map *l, const char *start,
+				 const char *name, char *result)
+     attribute_hidden;
 
 /* Open the shared object NAME, relocate it, and run its initializer if it
    hasn't already been run.  MODE is as for `dlopen' (see <dlfcn.h>).  If