Improve DST handling (Bug 23102, Bug 21942, Bug 18018, Bug, 23259, CVE-2011-0536 ).
Commit Message
On 06/06/2018 12:10 PM, Carlos O'Donell wrote:
> On 06/06/2018 01:02 AM, Carlos O'Donell wrote:
>> This commit improves DST handling significantly in the following
>
> v2
>
> - Fix is_dst() by adding back string comparison, and clarify comment.
> Added extra test testcases (49 now) to cover this error.
> - Renamed parameters as 'input' for data from ELF file, and 'ref' as
> reference DST to compare against.
> - Removed dead update to name in is_dst().
> - Killed DL_DST_COUNT, not needed really, and a weak optimization.
> - Did not remove len from _dl_dst_count because we use it to advance
> input more quickly to the next DST.
>
v3
- Use memcmp in is_dst().
- In _dl_dst_substitute use 'len != 0' conditional to clarify that
all we are looking to do is distinguish between a valid DST
for which we don't recognize the DST, and an invalid DST which
may just be stray characters which we will copy to the result.
- Added a few more tests: Small invalid sequence e.g. ${}, and
large valid sequence with unknown DST.
This commit improves DST handling significantly in the following
ways: firstly is_dst () is overhauled to correctly process DST
sequences that would be accepted given the ELF gABI. This means that
we actually now accept slightly more sequences than before. Now we
accept $ORIGIN$ORIGIN, but in the past we accepted only $ORIGIN\0 or
$ORIGIN/..., but this kind of behaviour results in unexpected
and uninterpreted DST sequences being used as literal search paths
leading to security defects. Therefore the first step in correcting
this defect is making is_dst () properly account for all DSTs
and making the function context free in the sense that it counts
DSTs without knowledge of path, or AT_SECURE. Next, _dl_dst_count ()
is also simplified to count all DSTs regardless of context.
Then in _dl_dst_substitute () we reintroduce context-dependent
processing for such things as AT_SECURE handling. At the level of
_dl_dst_substitute we can have access to things like the true start
of the string sequence to validate $ORIGIN-based paths rooted in
trusted directories. Lastly, callers of _dl_dst_substitute () are
adjusted to pass in the start of their string sequences, this includes
expand_dynamic_string_token () and fillin_rpath (). Lastly, after this
commit we tighten up the accepted sequences in AT_SECURE, and avoid
leaving unexpanded DSTs, this is noted in the NEWS entry.
Verified with a sequence of 56 tests on x86_64 that cover
non-AT_SECURE and AT_SECURE testing using a sysroot (requires root
to run). The tests cover cases for bug 23102, bug 21942, bug 18018,
and bug 23259. These tests are not yet appropriate for the glibc
regression testsuite, but with the upcoming test-in-container testing
framework it should be possible to include these tests upstream soon.
See the mailing list for the tests:
https://www.sourceware.org/ml/libc-alpha/2018-06/msg00073.html
---
ChangeLog | 20 ++++++
NEWS | 10 +++
elf/dl-deps.c | 4 +-
elf/dl-dst.h | 13 ----
elf/dl-load.c | 175 ++++++++++++++++++++++++++++++---------------
sysdeps/generic/ldsodefs.h | 5 +-
6 files changed, 153 insertions(+), 74 deletions(-)
Comments
On 06/06/2018 10:18 PM, Carlos O'Donell wrote:
> +/* Process INPUT for DSTs and store in RESULT using the information from
> + link map L to resolve the DSTs. The value of START must equal the
> + start of the parent string if INPUT is a substring sequence being
> + parsed with path separators e.g. $ORIGIN:$PLATFORM. */
> char *
> -_dl_dst_substitute (struct link_map *l, const char *name, char *result)
> +_dl_dst_substitute (struct link_map *l, const char *start,
> + const char *input, char *result)
The comment should describe the storage requirements for RESULT.
I'm a bit worried about this:
else if (len != 0)
{
/* We cannot use this path element, the value of the
replacement is unknown. */
check_for_trusted = false;
wp = last_elem;
break;
}
Does this really do the right thing for $ORIGIN/../$LIB:/foo/$ORIGIN? I
would have expected a trusted path check for the first component in this
case.
Thanks,
Florian
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?
Didn't we change the code so that _dl_dst_substitute is only called with
a single component as an argument?
fillin_rpath splits the string at :/:. The callers in dl-deps.c
I also suggest to use struct alloc_buffer, to make the code more
obviously correct.
Thanks,
Florian
On 06/07/2018 08:16 AM, Florian Weimer wrote:
> On 06/06/2018 10:18 PM, Carlos O'Donell wrote:
>> +/* Process INPUT for DSTs and store in RESULT using the information from
>> + link map L to resolve the DSTs. The value of START must equal the
>> + start of the parent string if INPUT is a substring sequence being
>> + parsed with path separators e.g. $ORIGIN:$PLATFORM. */
>> char *
>> -_dl_dst_substitute (struct link_map *l, const char *name, char *result)
>> +_dl_dst_substitute (struct link_map *l, const char *start,
>> + const char *input, char *result)
>
> The comment should describe the storage requirements for RESULT.
Done, I'll add that to v4.
The answer is: As much as DL_DST_REQUIRED tells you you need.
We use DL_DST_REQUIRED in both expand_dst (used for NEEDED, AUX, and FILTER),
and in expand_dynamic_string_token (used for RPATH from fillin_rpath).
> I'm a bit worried about this:
>
> else if (len != 0)
> {
> /* We cannot use this path element, the value of the
> replacement is unknown. */
> check_for_trusted = false;
> wp = last_elem;
> break;
> }
>
> Does this really do the right thing for $ORIGIN/../$LIB:/foo/$ORIGIN?
> I would have expected a trusted path check for the first component in
> this case.
That depends from which input this came from.
There are two ways to this code:
(1) _dl_map_object -> DT_RPATH/RUNPATH -> fillin_rpath -> _dl_dst_substitute
(2) _dl_map_object_deps -> DT_NEEDED/AUXILIARY/FILTER -> expand_dst -> _dl_dst_substitute
I assume that you intend (1), and that ':' is to be interpreted as the
path separator in a valid list-of-paths sequence.
I also assume we are talking about SUID/SGID.
The alternative is that you mean (2), and that you have a really odd
name for the dependency of the binary, or it's a mistake made by the
developer which might be exploited by an attacker. We can talk about
this in your other email regarding ':'-splitting in fillin_rpath.
Given that you mean (1), and in fillin_rpath we split on ':' then the
sequence arriving at _dl_dst_substitute is [$ORIGIN/../$LIB], and we
can substitute for $ORIGIN and $LIB without error, both of them return
a valid repl value and length, and we never set check_for_trusted to
false. Therefore the first component *does* get a trusted path check.
Verified here:
Test 18 [SUID]: Verify RPATH $ORIGIN/../$LIB:/foo/$ORIGIN loads from /lib64.
6277: decompose_rpath: Call fillin_rpath
6277: fillin_rpath (begin)
6277: expand_dynamic_string_token input=$ORIGIN/../$LIB, start=$ORIGIN/../$LIB
6277: _dl_dst_count $ORIGIN/../$LIB (7fa91c1d4d20)
6277: is_dst input=ORIGIN/../$LIB (begin)
6277: is_dst ilen=6 rlen=6
6277: is_dst ilen=6 input=ORIGIN/../$LIB (7fa91c1d4d21) (ref=ORIGIN)
6277: _dl_dst_count next /../$LIB
6277: is_dst input=LIB (begin)
6277: is_dst ilen=3 rlen=6
6277: is_dst input=LIB (begin)
6277: is_dst ilen=3 rlen=8
6277: is_dst input=LIB (begin)
6277: is_dst ilen=3 rlen=3
6277: is_dst ilen=3 input=LIB (7fa91c1d4d2c) (ref=LIB)
6277: _dl_dst_count next
6277: _dl_dst_count cnt 2
6277: DL_DST_REQUIRED l_origin 7fa91c1d4d60
6277: _dl_dst_substitute input=$ORIGIN/../$LIB start=$ORIGIN/../$LIB
6277: is_dst input=ORIGIN/../$LIB (begin)
6277: is_dst ilen=6 rlen=6
6277: is_dst ilen=6 input=ORIGIN/../$LIB (7fa91c1d4d21) (ref=ORIGIN)
6277: _dl_dst_substitute 7fa91c1d4d60
6277: is_dst input=LIB (begin)
6277: is_dst ilen=3 rlen=6
6277: is_dst input=LIB (begin)
6277: is_dst ilen=3 rlen=8
6277: is_dst input=LIB (begin)
6277: is_dst ilen=3 rlen=3
6277: is_dst ilen=3 input=LIB (7fa91c1d4d2c) (ref=LIB)
6277: _dl_dst_substitute 7fa91bfcc457
6277: is_trusted_path_normalize: /root/../lib64 14
6277: is_trusted_path_normalize: Transformed path /lib64/
6277: is_trusted_path_normalize: Path IS trusted.
6277: expand_dynamic_string_token input=/foo/$ORIGIN, start=$ORIGIN/../$LIB
6277: _dl_dst_count $ORIGIN (7fa91c1d4d35)
6277: is_dst input=ORIGIN (begin)
6277: is_dst ilen=6 rlen=6
6277: is_dst ilen=6 input=ORIGIN (7fa91c1d4d36) (ref=ORIGIN)
6277: _dl_dst_count next
6277: _dl_dst_count cnt 1
6277: _dl_dst_substitute input=/foo/$ORIGIN start=$ORIGIN/../$LIB
6277: is_dst input=ORIGIN (begin)
6277: is_dst ilen=6 rlen=6
6277: is_dst ilen=6 input=ORIGIN (7fa91c1d4d36) (ref=ORIGIN)
6277: _dl_dst_substitute Invalid DST!
6277: _dl_dst_substitute ffffffffffffffff
6277: security = 1
6277: security = 1
6277: security = 1
6277: security = 1
PASS: Correct primary library.
The 'Invalid DST!' comes from a debug in the security checking code which
notices '/foo/$ORIGIN' doesn't meet the rules for SUID/SGID (that $ORIGIN
is first and it's rooted in a trusted direcotry) and sets repl to -1.
Therefore this works as expected for the case you cite.
What about [$ORIGIN/../$FOO], which is one valid, and one invalid DST?
This might be what you intended to write above, but I'd rather just cover
both cases to save the round-trip on the review.
12187: decompose_rpath: Call fillin_rpath
12187: fillin_rpath (begin)
12187: expand_dynamic_string_token input=$ORIGIN/../$FOO, start=$ORIGIN/../$FOO
12187: _dl_dst_count $ORIGIN/../$FOO (7fbf5ff90d20)
12187: is_dst input=ORIGIN/../$FOO (begin)
12187: is_dst ilen=6 rlen=6
12187: is_dst ilen=6 input=ORIGIN/../$FOO (7fbf5ff90d21) (ref=ORIGIN)
12187: _dl_dst_count next /../$FOO
12187: is_dst input=FOO (begin)
12187: is_dst ilen=3 rlen=6
12187: is_dst input=FOO (begin)
12187: is_dst ilen=3 rlen=8
12187: is_dst input=FOO (begin)
12187: is_dst ilen=3 rlen=3
12187: is_dst memcmp failed.
12187: _dl_dst_count next FOO
12187: _dl_dst_count cnt 1
12187: DL_DST_REQUIRED l_origin 7fbf5ff90d60
12187: _dl_dst_substitute input=$ORIGIN/../$FOO start=$ORIGIN/../$FOO
12187: is_dst input=ORIGIN/../$FOO (begin)
12187: is_dst ilen=6 rlen=6
12187: is_dst ilen=6 input=ORIGIN/../$FOO (7fbf5ff90d21) (ref=ORIGIN)
12187: _dl_dst_substitute 7fbf5ff90d60
12187: is_dst input=FOO (begin)
12187: is_dst ilen=3 rlen=6
12187: is_dst input=FOO (begin)
12187: is_dst ilen=3 rlen=8
12187: is_dst input=FOO (begin)
12187: is_dst ilen=3 rlen=3
12187: is_dst memcmp failed.
12187: _dl_dst_substitute 0
12187: is_trusted_path_normalize: /root/../$FOO 13
12187: is_trusted_path_normalize: Transformed path /$FOO/
12187: is_trusted_path_normalize: Path is NOT trusted.
12187: expand_dynamic_string_token input=/foo/$ORIGIN, start=$ORIGIN/../$FOO
12187: _dl_dst_count $ORIGIN (7fbf5ff90d35)
12187: is_dst input=ORIGIN (begin)
12187: is_dst ilen=6 rlen=6
12187: is_dst ilen=6 input=ORIGIN (7fbf5ff90d36) (ref=ORIGIN)
12187: _dl_dst_count next
12187: _dl_dst_count cnt 1
12187: _dl_dst_substitute input=/foo/$ORIGIN start=$ORIGIN/../$FOO
12187: is_dst input=ORIGIN (begin)
12187: is_dst ilen=6 rlen=6
12187: is_dst ilen=6 input=ORIGIN (7fbf5ff90d36) (ref=ORIGIN)
12187: _dl_dst_substitute Invalid DST!
12187: _dl_dst_substitute ffffffffffffffff
12187: security = 1
12187: security = 1
12187: security = 1
12187: security = 1
Here, the $FOO is left unexpanded, we don't have a matching DST,
and so it's copied directly to the output, and has to go through
a trusted directory check which it fails.
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.
Cheers,
Carlos.
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;
Thanks,
Florian
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.
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.
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.
@@ -1,3 +1,23 @@
+2018-06-06 Carlos O'Donell <carlos@redhat.com>
+ Andreas Schwab <schwab@suse.de>
+ Dmitry V. Levin <ldv@altlinux.org>
+
+ [BZ #23102]
+ [BZ #21942]
+ [BZ #18018]
+ [BZ #23259]
+ CVE-2011-0536
+ * elf/dl-load.c (is_dst): Comment. Support ELF gABI.
+ (_dl_dst_count): Comment. Simplify and count DSTs.
+ (_dl_dst_substitute): Comment. Support __libc_enable_secure handling.
+ Add string start to arguments.
+ (expand_dybamic_string_token): Comment. Accept path start.
+ (fillin_rpath): Pass string start to expand_dynamic_string_token.
+ * sysdeps/generic/ldsodefs.h: _dl_dst_substitute takes additiional
+ string start argument.
+ * elf/dl-deps.c (expand_dst): Adjust call to _dl_dst_substitute.
+ * elf/dl-dst.h: Remove DL_DST_COUNT.
+
2018-06-05 Joseph Myers <joseph@codesourcery.com>
* sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h (HWCAP_DIT): New
@@ -41,6 +41,16 @@ Major new features:
NI_IDN_ALLOW_UNASSIGNED, NI_IDN_USE_STD3_ASCII_RULES) have been
deprecated. They no longer have any effect.
+* Parsing of dynamic string tokens in DT_RPATH, DT_RUNPATH, and DT_NEEDED
+ has been expanded to support the full range of ELF gABI expressions
+ including such constructs as '$ORIGIN$ORIGIN' (if valid). For SUID/GUID
+ applications the rules have been further restricted, and where in the
+ past a dynamic string token sequence may have been interpreted as a
+ literal string it will now cause a load failure. These load failures
+ were always considered unspecified behaviour from the perspective of the
+ dynamic loader, and for safety are now load errors e.g. /foo/${ORIGIN}.so
+ in DT_NEEDED results in a load failure now.
+
Deprecated and removed features, and other changes affecting compatibility:
* The nonstandard header files <libio.h> and <_G_config.h> are no longer
@@ -100,7 +100,7 @@ struct list
({ \
const char *__str = (str); \
const char *__result = __str; \
- size_t __dst_cnt = DL_DST_COUNT (__str); \
+ size_t __dst_cnt = _dl_dst_count (__str); \
\
if (__dst_cnt != 0) \
{ \
@@ -114,7 +114,7 @@ DST not allowed in SUID/SGID programs")); \
__newp = (char *) alloca (DL_DST_REQUIRED (l, __str, strlen (__str), \
__dst_cnt)); \
\
- __result = _dl_dst_substitute (l, __str, __newp); \
+ __result = _dl_dst_substitute (l, __str, __str, __newp); \
\
if (*__result == '\0') \
{ \
@@ -18,19 +18,6 @@
#include "trusted-dirs.h"
-/* Determine the number of DST elements in the name. Only if IS_PATH is
- nonzero paths are recognized (i.e., multiple, ':' separated filenames). */
-#define DL_DST_COUNT(name) \
- ({ \
- size_t __cnt = 0; \
- const char *__sf = strchr (name, '$'); \
- \
- if (__glibc_unlikely (__sf != NULL)) \
- __cnt = _dl_dst_count (__sf); \
- \
- __cnt; })
-
-
#ifdef SHARED
# define IS_RTLD(l) (l) == &GL(dl_rtld_map)
#else
@@ -177,76 +177,110 @@ is_trusted_path_normalize (const char *path, size_t len)
return false;
}
+/* Given a substring starting at INPUT, just after the DST '$' start
+ token, determine if INPUT contains DST token REF, following the
+ ELF gABI rules for DSTs:
+ * Longest possible sequence using the rules (greedy).
+
+ * Must start with a $ (enforced by caller).
+
+ * Must follow $ with one underscore or ASCII [A-Za-z] (caller
+ follows these rules for REF and we enforce the comparison)
+ or '{' (start curly quoted name).
+
+ * Must follow first two characters with zero or more [A-Za-z0-9_]
+ (enforced by caller) or '}' (end curly quoted name).
+
+ If the sequence is a DST matching REF then the length of the DST is
+ returned, otherwise 0. */
static size_t
-is_dst (const char *start, const char *name, const char *str, int secure)
+is_dst (const char *input, const char *ref)
{
- size_t len;
+ size_t ilen, rlen;
bool is_curly = false;
- if (name[0] == '{')
+ /* Is a ${...} input sequence? */
+ if (input[0] == '{')
{
is_curly = true;
- ++name;
+ ++input;
}
- len = 0;
- while (name[len] == str[len] && name[len] != '\0')
- ++len;
+ /* Find longest valid input sequence. */
+ ilen = 0;
+ while ((input[ilen] >= 'A' && input[ilen] <= 'Z')
+ || (input[ilen] >= 'a' && input[ilen] <= 'z')
+ || (input[ilen] >= '0' && input[ilen] <= '9')
+ || (input[ilen] == '_'))
+ ++ilen;
+
+ rlen = strlen (ref);
+
+ /* Can't be the DST we are looking for. */
+ if (rlen != ilen)
+ return 0;
+
+ /* Compare the DST (no strncmp this early in startup). */
+ if (memcmp (input, ref, ilen) != 0)
+ return 0;
if (is_curly)
{
- if (name[len] != '}')
+ /* Invalid curly sequence! */
+ if (input[ilen] != '}')
return 0;
- /* Point again at the beginning of the name. */
- --name;
- /* Skip over closing curly brace and adjust for the --name. */
- len += 2;
+ /* Count the two curly braces. */
+ ilen += 2;
}
- else if (name[len] != '\0' && name[len] != '/')
- return 0;
- if (__glibc_unlikely (secure)
- && ((name[len] != '\0' && name[len] != '/')
- || (name != start + 1)))
- return 0;
-
- return len;
+ return ilen;
}
-
+/* INPUT is the start of a DST sequence at the first '$' occurrence.
+ If there is a DST we call into _dl_dst_count to count the number of
+ DSTs. We count all known DSTs regardless of __libc_enable_secure;
+ the caller is responsible for enforcing the security of the
+ substitution rules (usually _dl_dst_substitute). */
size_t
-_dl_dst_count (const char *name)
+_dl_dst_count (const char *input)
{
- const char *const start = name;
size_t cnt = 0;
+ input = strchr (input, '$');
+
+ /* Most likely there is no DST. */
+ if (__glibc_likely (input == NULL))
+ return 0;
+
do
{
size_t len;
- /* $ORIGIN is not expanded for SUID/GUID programs (except if it
- is $ORIGIN alone) and it must always appear first in path. */
- ++name;
- if ((len = is_dst (start, name, "ORIGIN", __libc_enable_secure)) != 0
- || (len = is_dst (start, name, "PLATFORM", 0)) != 0
- || (len = is_dst (start, name, "LIB", 0)) != 0)
+ ++input;
+ /* All DSTs must follow ELF gABI rules, see is_dst (). */
+ if ((len = is_dst (input, "ORIGIN")) != 0
+ || (len = is_dst (input, "PLATFORM")) != 0
+ || (len = is_dst (input, "LIB")) != 0)
++cnt;
- name = strchr (name + len, '$');
+ /* There may be more than one DST in the input. */
+ input = strchr (input + len, '$');
}
- while (name != NULL);
+ while (input != NULL);
return cnt;
}
-
+/* Process INPUT for DSTs and store in RESULT using the information from
+ link map L to resolve the DSTs. The value of START must equal the
+ start of the parent string if INPUT is a substring sequence being
+ parsed with path separators e.g. $ORIGIN:$PLATFORM. */
char *
-_dl_dst_substitute (struct link_map *l, const char *name, char *result)
+_dl_dst_substitute (struct link_map *l, const char *start,
+ const char *input, char *result)
{
- const char *const start = name;
-
/* Now fill the result path. While copying over the string we keep
track of the start of the last path element. When we come across
a DST we copy over the value or (if the value is not available)
@@ -257,32 +291,54 @@ _dl_dst_substitute (struct link_map *l, const char *name, char *result)
do
{
- if (__glibc_unlikely (*name == '$'))
+ if (__glibc_unlikely (*input == '$'))
{
const char *repl = NULL;
size_t len;
- ++name;
- if ((len = is_dst (start, name, "ORIGIN", __libc_enable_secure)) != 0)
+ ++input;
+ if ((len = is_dst (input, "ORIGIN")) != 0)
{
- repl = l->l_origin;
+ /* 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.
+ * 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[len] != '\0' && input[len] != '/'
+ && input[len] != ':')
+ || (input != start + 1
+ || (input > start + 2 && input[-2] != ':'))))
+ repl = (const char *) -1;
+ else
+ repl = l->l_origin;
+
check_for_trusted = (__libc_enable_secure
&& l->l_type == lt_executable);
}
- else if ((len = is_dst (start, name, "PLATFORM", 0)) != 0)
+ else if ((len = is_dst (input, "PLATFORM")) != 0)
repl = GLRO(dl_platform);
- else if ((len = is_dst (start, name, "LIB", 0)) != 0)
+ else if ((len = is_dst (input, "LIB")) != 0)
repl = DL_DST_LIB;
if (repl != NULL && repl != (const char *) -1)
{
wp = __stpcpy (wp, repl);
- name += len;
+ input += len;
}
- else if (len > 1)
+ else if (len != 0)
{
/* We cannot use this path element, the value of the
replacement is unknown. */
+ check_for_trusted = false;
wp = last_elem;
break;
}
@@ -292,10 +348,10 @@ _dl_dst_substitute (struct link_map *l, const char *name, char *result)
}
else
{
- *wp++ = *name++;
+ *wp++ = *input++;
}
}
- while (*name != '\0');
+ while (*input != '\0');
/* In SUID/SGID programs, after $ORIGIN expansion the normalized
path must be rooted in one of the trusted directories. */
@@ -309,13 +365,17 @@ _dl_dst_substitute (struct link_map *l, const char *name, char *result)
}
-/* Return copy of argument with all recognized dynamic string tokens
- ($ORIGIN and $PLATFORM for now) replaced. On some platforms it
- might not be possible to determine the path from which the object
- belonging to the map is loaded. In this case the path element
- containing $ORIGIN is left out. */
+/* Return a malloc allocated copy of INPUT with all recognized DSTs
+ ($ORIGIN and $PLATFORM for now) replaced. The value of START must
+ equal the start of the parent string if INPUT is a substring
+ sequence being parsed with path separators e.g. $ORIGIN:$PLATFORM.
+ On some platforms it might not be possible to determine the path
+ from which the object belonging to the map is loaded. In this case
+ the path element containing $ORIGIN is left out. On error NULL is
+ returned. */
static char *
-expand_dynamic_string_token (struct link_map *l, const char *s)
+expand_dynamic_string_token (struct link_map *l, const char *start,
+ const char *input)
{
/* We make two runs over the string. First we determine how large the
resulting string is and then we copy it over. Since this is no
@@ -326,21 +386,21 @@ expand_dynamic_string_token (struct link_map *l, const char *s)
char *result;
/* Determine the number of DST elements. */
- cnt = DL_DST_COUNT (s);
+ cnt = _dl_dst_count (input);
/* If we do not have to replace anything simply copy the string. */
if (__glibc_likely (cnt == 0))
- return __strdup (s);
+ return __strdup (input);
/* Determine the length of the substituted string. */
- total = DL_DST_REQUIRED (l, s, strlen (s), cnt);
+ total = DL_DST_REQUIRED (l, input, strlen (input), cnt);
/* Allocate the necessary memory. */
result = (char *) malloc (total + 1);
if (result == NULL)
return NULL;
- return _dl_dst_substitute (l, s, result);
+ return _dl_dst_substitute (l, start, input, result);
}
@@ -387,6 +447,7 @@ fillin_rpath (char *rpath, struct r_search_path_elem **result, const char *sep,
const char *what, const char *where, struct link_map *l)
{
char *cp;
+ char *start = rpath;
size_t nelems = 0;
while ((cp = __strsep (&rpath, sep)) != NULL)
@@ -398,7 +459,7 @@ fillin_rpath (char *rpath, struct r_search_path_elem **result, const char *sep,
/* `strsep' can pass an empty string. */
if (*cp != '\0')
{
- to_free = cp = expand_dynamic_string_token (l, cp);
+ to_free = cp = expand_dynamic_string_token (l, start, cp);
/* expand_dynamic_string_token can return NULL in case of empty
path or memory allocation failure. */
@@ -2091,7 +2152,7 @@ _dl_map_object (struct link_map *loader, const char *name,
{
/* The path may contain dynamic string tokens. */
realname = (loader
- ? expand_dynamic_string_token (loader, name)
+ ? expand_dynamic_string_token (loader, name, name)
: __strdup (name));
if (realname == NULL)
fd = -1;
@@ -1108,8 +1108,9 @@ extern const char *_dl_get_origin (void) attribute_hidden;
extern size_t _dl_dst_count (const char *name) attribute_hidden;
/* Substitute DST values. */
-extern char *_dl_dst_substitute (struct link_map *l, const char *name,
- char *result) attribute_hidden;
+extern char *_dl_dst_substitute (struct link_map *l, const char *start,
+ const char *name, char *result)
+ attribute_hidden;
/* Open the shared object NAME, relocate it, and run its initializer if it
hasn't already been run. MODE is as for `dlopen' (see <dlfcn.h>). If