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