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/07/2018 08:43 AM, Florian Weimer wrote:
> On 06/06/2018 10:18 PM, Carlos O'Donell wrote:
>> +          if (__glibc_unlikely (__libc_enable_secure)
>> +          && ((input[len] != '\0' && input[len] != '/'
>> +               && input[len] != ':')
>> +              || (input != start + 1
>> +              || (input > start + 2 && input[-2] != ':'))))
> 
> Is the ':' check really the right thing here?

No. It's superflous. We'll never see it (except with DT_NEEDED, but in that case
for SUID/SGID we already rejected the load, and !SUID/!SGID we just should interpret
as literal). However, I realized something else is wrong.

> Didn't we change the code so that _dl_dst_substitute is only called with a single component as an argument?

We did.
 
> fillin_rpath splits the string at :/:.  The callers in dl-deps.c

Correct.

I'm going to remove the ':' check *and* cleanup the code to mention explicitly
that we only accept single path components from a :-separated path.

The real problem I failed to notice is that strsep in fillin_rpath destroys the
":" in the string so "intput[-2] != ':'" is always true. Therefore a non-first
component in the multi-component list is always discarded even if it starts with
$ORIGIN and is rooted in a trusted directory.

I have removed the double-negation and fixed the code to read:

 324                    * $ORIGIN appears first in the path element, and is
 325                      the only thing in the element or is immediately
 326                      followed by a path separator and the rest of the
 327                      path.

 320               if (__glibc_unlikely (__libc_enable_secure)
 321                   && !((input == start + 1
 322                         || (input > start + 1 && input[-2] == '\0'))
 323                        && (input[len] == '\0' || input[len] == '/')))
 324                 repl = (const char *) -1;
 325               else
 326                 repl = l->l_origin;


Such that the second path e.g.

[$ORIGIN/../$LIB\0$ORIGIN/../$LIB/subdir]
 ^--- start        ^--- input

... now works.  I verified this with a test.

Test 19  [SUID]: Verify RPATH $ORIGIN/../$LIB:$ORIGIN/../$LIB/subdir loads from /lib64/subdir.
     10833:	decompose_rpath: Call fillin_rpath
     10833:	fillin_rpath (begin)
     10833:	expand_dynamic_string_token input=$ORIGIN/../$LIB, start=$ORIGIN/../$LIB
     10833:	_dl_dst_count $ORIGIN/../$LIB (7f84ae18bd20)
     10833:	is_dst input=ORIGIN/../$LIB (begin)
     10833:	is_dst ilen=6 rlen=6
     10833:	is_dst ilen=6 input=ORIGIN/../$LIB (7f84ae18bd21) (ref=ORIGIN)
     10833:	_dl_dst_count next /../$LIB
     10833:	is_dst input=LIB (begin)
     10833:	is_dst ilen=3 rlen=6
     10833:	is_dst input=LIB (begin)
     10833:	is_dst ilen=3 rlen=8
     10833:	is_dst input=LIB (begin)
     10833:	is_dst ilen=3 rlen=3
     10833:	is_dst ilen=3 input=LIB (7f84ae18bd2c) (ref=LIB)
     10833:	_dl_dst_count next 
     10833:	_dl_dst_count cnt 2
     10833:	DL_DST_REQUIRED l_origin 7f84ae18bd70
     10833:	_dl_dst_substitute input=$ORIGIN/../$LIB start=$ORIGIN/../$LIB
     10833:	is_dst input=ORIGIN/../$LIB (begin)
     10833:	is_dst ilen=6 rlen=6
     10833:	is_dst ilen=6 input=ORIGIN/../$LIB (7f84ae18bd21) (ref=ORIGIN)
     10833:	_dl_dst_substitute 7f84ae18bd70
     10833:	is_dst input=LIB (begin)
     10833:	is_dst ilen=3 rlen=6
     10833:	is_dst input=LIB (begin)
     10833:	is_dst ilen=3 rlen=8
     10833:	is_dst input=LIB (begin)
     10833:	is_dst ilen=3 rlen=3
     10833:	is_dst ilen=3 input=LIB (7f84ae18bd2c) (ref=LIB)
     10833:	_dl_dst_substitute 7f84adf83457
     10833:	is_trusted_path_normalize: /root/../lib64 14
     10833:	is_trusted_path_normalize: Transformed path /lib64/
     10833:	is_trusted_path_normalize: Path IS trusted.
     10833:	expand_dynamic_string_token input=$ORIGIN/../$LIB/subdir, start=$ORIGIN/../$LIB
     10833:	_dl_dst_count $ORIGIN/../$LIB/subdir (7f84ae18bd30)
     10833:	is_dst input=ORIGIN/../$LIB/subdir (begin)
     10833:	is_dst ilen=6 rlen=6
     10833:	is_dst ilen=6 input=ORIGIN/../$LIB/subdir (7f84ae18bd31) (ref=ORIGIN)
     10833:	_dl_dst_count next /../$LIB/subdir
     10833:	is_dst input=LIB/subdir (begin)
     10833:	is_dst ilen=3 rlen=6
     10833:	is_dst input=LIB/subdir (begin)
     10833:	is_dst ilen=3 rlen=8
     10833:	is_dst input=LIB/subdir (begin)
     10833:	is_dst ilen=3 rlen=3
     10833:	is_dst ilen=3 input=LIB/subdir (7f84ae18bd3c) (ref=LIB)
     10833:	_dl_dst_count next /subdir
     10833:	_dl_dst_count cnt 2
     10833:	_dl_dst_substitute input=$ORIGIN/../$LIB/subdir start=$ORIGIN/../$LIB
     10833:	is_dst input=ORIGIN/../$LIB/subdir (begin)
     10833:	is_dst ilen=6 rlen=6
     10833:	is_dst ilen=6 input=ORIGIN/../$LIB/subdir (7f84ae18bd31) (ref=ORIGIN)
     10833:	_dl_dst_substitute 7f84ae18bd70
     10833:	is_dst input=LIB/subdir (begin)
     10833:	is_dst ilen=3 rlen=6
     10833:	is_dst input=LIB/subdir (begin)
     10833:	is_dst ilen=3 rlen=8
     10833:	is_dst input=LIB/subdir (begin)
     10833:	is_dst ilen=3 rlen=3
     10833:	is_dst ilen=3 input=LIB/subdir (7f84ae18bd3c) (ref=LIB)
     10833:	_dl_dst_substitute 7f84adf83457
     10833:	is_trusted_path_normalize: /root/../lib64/subdir 21
     10833:	is_trusted_path_normalize: Transformed path /lib64/subdir/
     10833:	is_trusted_path_normalize: Path IS trusted.
     10833:	security = 1
     10833:	security = 1
     10833:	security = 1
     10833:	security = 1
PASS: Correct primary library.
PASS: Correctly used second valid RPATH component.

Lastly, we could get here via DT_NEEDED paths, and those could have weird SONAMES
which have ':' in them, but we should not treat those as special in any way.
It is the correct abstraction to have fillin_rpath do splitting, while also having
expand_dst() simply pass a full SONAME (which may have colons in it).

Now with this update _dl_dst_substitute doesn't care about colons.

> I also suggest to use struct alloc_buffer, to make the code more obviously correct.

I would like to leave this for a subsequent cleanup, just to get to an acceptable
set of semantics with the current code.

I'll post v4 of my patch with new tests shortly.

Cheers,
Carlos.