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 10:00 AM, Florian Weimer wrote:
> On 06/06/2018 07:02 AM, Carlos O'Donell wrote:
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index 431236920f..13263212d5 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
>> @@ -177,63 +177,89 @@ is_trusted_path_normalize (const char *path, size_t len)
>>     return false;
>>   }
>>   +/* Given a substring starting at NAME, just after the DST '$' start
>> +   token, determine if NAME contains dynamic string token STR,
>> +   following the ELF gABI rules for dynamic string tokens:
>>   +   * Longest possible sequence using the rules (greedy).
>> +
>> +   * Must start with a $ (enforced by caller).
>> +
>> +   * Must follow $ with one underscore or ASCII [A-Za-z] (enforced by
>> +     caller via STR comparison) or '{' (start curly quoted name).
> 
> “enforced by caller via STR comparison”: I don't see this in the remaining code.  So $ORIGIN and $PRIGIN are currently treated the same?

This is a mistake when I moved the code from my test harness into is_dst.

The clarified comment is this:

   * Must follow $ with one underscore or ASCII [A-Za-z] (caller
     follows these rules for STR and we enforce the compariso)
     or '{' (start curly quoted name).

The updated code is this:

  /* Compare the DST (no strncmp this early in startup).  */
  len = 0;
  while (name[len] == str[len] && len < nlen)
    ++len;

  /* Characters of name don't match DST.  */
  if (len != nlen)
    return 0;

Very similar to the original code, but using the computed length
nlen (greedy length of 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 dynamic string token matching STR 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 *name, const char *str)
> 
> I'm not entirely happy about the choice of parameter names. name/reference or input/name would work better IMHO.

Yeah, me neither, I tried to standardize across the board as name/str.

I will change everything over to use:

input - Incoming input byte stream with possible DSTs.
ref - Reference DST for comparison.

>>         /* Point again at the beginning of the name.  */
>>         --name;
> 
> This assignment is now dead, and the comment is wrong.

Good catch. Removed.

> (Still need to look at the rest of the patch.)

Thanks.

Cheers,
Carlos.