Re: [PATCH v4] 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 02:25 AM, Florian Weimer wrote:
> On 06/08/2018 08:03 AM, Carlos O'Donell wrote:
>> On 06/08/2018 01:51 AM, Florian Weimer wrote:
>>> On 06/08/2018 07:45 AM, Carlos O'Donell wrote:
>>>> +          /* For SUID/GUID programs we normally ignore the path with
>>>> +         a DST in DT_RUNPATH, or DT_RPATH.  However, there is
>>>> +         one exception to this rule, and it is:
>>>> +
>>>> +           * $ORIGIN appears first in the path element, and is
>>>> +             the only thing in the element or is immediately
>>>> +             followed by a path separator and the rest of the
>>>> +             path.
>>>> +
>>>> +           * The path element is rooted in a trusted directory.
>>>> +
>>>> +         This exception allows such programs to reference
>>>> +         shared libraries in subdirectories of trusted
>>>> +         directories.  The use case is one of general
>>>> +         organization and deployment flexibility.
>>>> +         Trusted directories are usually such paths as "/lib64"
>>>> +         or "/lib".  */
>>>> +          if (__glibc_unlikely (__libc_enable_secure)
>>>> +          && !((input == start + 1
>>>> +            || (input > start + 1 && input[-2] == '\0'))
>>>> +               && (input[len] == '\0' || input[len] == '/')))
>>>> +        repl = (const char *) -1;
>>>
>>> The comment does not match the code: The code checks that $ORIGIN
>>> comes first in the *path*, not *path element* (hence the need for the
>>> start variable).  I'm not sure what the right behavior is here.
>>> Going by path element seems more correct.
>> Sorry, there is a bit of confusion regarding terminology here.
>>
>> When you have multiple colon separated paths in DT_RUNPATH or
>> DT_RPATH, what is each path called? Are they path elements?
>>
>> Or are we calling path elements only those things that are
>> between slashes in a singular path?
>>
>> It seems context dependent, but I understand the confusion.
>>
>> [/path/dir:/path/dir1:/path/dir2]
>>   ^^^^^^^^^ --- What do we call this in the context of multiple paths?
>>
>> Just a 'path' ?
>>
>> I consider the following valid:
>>
>> [$ORIGIN/../$LIB]
> 
> I'm actually asking about this:
> 
> [$LIB:$ORIGIN/../$LIB]
> 
> Doesn't the current code reject this?

The current code does not reject this. In fact it accepts $LIB in RPATH
et. al. because there is no code to reject it. Only $ORIGIN has any
restrictions.

The patched code continues to accept [$LIB:$ORIGIN/../$LIB].

> (Some of the callers split at :, not /.)

The fillin_rpath caller splits at ':', generally for DT_RUNPATH and
DT_RPATH. While the other callers do no splitting. Do you see a caller
that splits at '/'?

In summary, the current patch doesn't change the handling of *just* $LIB
for SUID/SGID binaries.

Should all paths get tested for trusted paths for a SUID/SGID binary?
It seems like that's the right idea.

@@ -365,7 +383,8 @@ _dl_dst_substitute (struct link_map *l, const char *start,
 
   /* In SUID/SGID programs, after DST expansion the normalized
      path must be rooted in one of the trusted directories.  */
-  if (__glibc_unlikely (check_for_trusted)
+  if (__glibc_unlikely (__libc_enable_secure)
+      && l->l_type == lt_executable
       && !is_trusted_path_normalize (result, wp - result))
     {
       *result = '\0';

Just drop check_for_trusted, and execute the is_trusted_path_normalize
check for all SUID/SGID paths?

Cheers,
Carlos.