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/11/2018 03:28 AM, Florian Weimer wrote:
> On 06/11/2018 04:54 AM, Carlos O'Donell wrote:
>>>> 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.
> 
> I meant the $ORIGN part.  As far as I understand it, start will not
> be close to input for the second part containing origin (after the
> :), so this check in _dl_dst_substitute should reject it:

The only code that splits on ':' is fillin_rpath, and it passes start
as the start of the whole DT_RUNPATH/DT_RPATH, and uses strsep so there
is no ':' to find.
 
>               || (input != start + 1
>               || (input > start + 2 && input[-2] != ':'))))
> 
> I'm not sure that this is right.

You are correct, and I fixed this for you based on earlier comments when
I sent out v4. Notice the v4 patch calls this out and I added a test
 case for it when you commented on it.

- Fix _dl_dst_substitute exception logic to check for NULL separator
  between non-first path since fillin_rpath and other callers will
  use strsep to split colon separated path elements. This fixes
  the case tested by test 19.

+             /* 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] == '/')))

The sequence in question:

+                 && !((input == start + 1
+                       || (input > start + 1 && input[-2] == '\0'))

Will match:

[$ORIGIN\0...]
 ^--- start
  ^--- input

input == start + 1

or

[...........\0$ORIGIN]
 ^--- start    ^--- input

input > start + 1 (not the first path element)
&& input[-2] == '\0' (first in the path element)

Does that answer your question?

However, I see your point now that start itself can also be deleted.

If we are only ever handling single paths, we can remove start from the API
and just track the start of the singular sequence.

I will clean up the code further.

I will also remove a ":" check in is_trusted_path_normalize().

>> 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?
> 
> No, $ORIGIN in a trusted path is itself trusted (because you cannot
> manipulate it using hard links).  I think there are installations out
> there which depend on this.

Understood.

I think a clearer statement would be like this:

The $LIB and $PLATFORM DST cannot in any way be manipulated by the caller
because they are fixed values that are set by the dynamic loader and therefore
any paths using just $LIB or $PLATFORM need not be checked for trust, the
authors of the binaries themselves are trusted to have designed this correctly.
Only $ORIGIN is tested in this way because it may be manipulated in some ways
with hard links.

I will add something like this as a comment.

Cheers,
Carlos.