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

[Carlos O'Donell]

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;
            }

Cheers,
Carlos.