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/08/2018 01:46 AM, Carlos O'Donell wrote:
> On 06/08/2018 01:21 AM, Florian Weimer wrote:
>> On 06/08/2018 06:14 AM, Carlos O'Donell wrote:
>>> The only time the code you quote is executed, this code:
>>>
>>>   338           else if (len != 0)
>>>   339             {
>>>   340               /* We cannot use this path element, the value of the
>>>   341                  replacement is unknown.  */
>>>   342               check_for_trusted = false;
>>>   343               wp = last_elem;
>>>   344               break;
>>>   345             }
>>>
>>> Is when we find a DST we know, say $LIB, but DL_DST_LIB is invalid
>>> e.g. set to -1, indicating that $LIB's value is unknown, in which case
>>> [$ORIGIN/../$LIB] is entirely considered unknown, and*discarded*  (which
>>> is what 'wp = last_elem' does).
>>>
>>> For v4 I'm going to clean up _dl_dst_substitute to point out that we
>>> only take individual path elements of a multi-path sequence.
>>>
>>> I believe this answers your question. Please clarify if I have not.
>>
>> Yes, it does.  What the quoted code actually does is something like this, right?
>>
>>   /* Return an empty string to tell the caller to drop the element.  */
>>   *result = '\0';
>>   return;
> 
> Exactly, and you'll see I do just that in v4 patch to make things clearer.
> 
>             {
> -             /* We cannot use this path element, the value of the
> -                replacement is unknown.  */
> -             wp = last_elem;
> -             break;
> +             /* We found a valid DST that we know about, but we could
> +                not find a replacement value for it, therefore we
> +                cannot use this path element and discard it.  */
> +             *begin = '\0';
> +             return result;
>             }

... and as soon as I see this I realize that we can just use result
and should remove begin.

Cheers,
Carlos.