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

[Florian Weimer]

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