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