Improve DST handling (Bug 23102, Bug 21942, Bug 18018, Bug, 23259, CVE-2011-0536 ).
Commit Message
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 ().
Verified with a sequence of 47 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:
[insert final URL of post containing reference to swbz23259.tar.gz (test cases)]
OK to checkin?
---
ChangeLog | 20 +++++++
NEWS | 10 ++++
elf/dl-deps.c | 2 +-
elf/dl-dst.h | 3 +-
elf/dl-load.c | 141 +++++++++++++++++++++++++++++++--------------
sysdeps/generic/ldsodefs.h | 5 +-
6 files changed, 133 insertions(+), 48 deletions(-)
Comments
On 06/06/2018 07:02 AM, Carlos O'Donell wrote:
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 431236920f..13263212d5 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -177,63 +177,89 @@ is_trusted_path_normalize (const char *path, size_t len)
> return false;
> }
>
> +/* Given a substring starting at NAME, just after the DST '$' start
> + token, determine if NAME contains dynamic string token STR,
> + following the ELF gABI rules for dynamic string tokens:
>
> + * Longest possible sequence using the rules (greedy).
> +
> + * Must start with a $ (enforced by caller).
> +
> + * Must follow $ with one underscore or ASCII [A-Za-z] (enforced by
> + caller via STR comparison) or '{' (start curly quoted name).
“enforced by caller via STR comparison”: I don't see this in the
remaining code. So $ORIGIN and $PRIGIN are currently treated the same?
> + * 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 dynamic string token matching STR 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 *name, const char *str)
I'm not entirely happy about the choice of parameter names.
name/reference or input/name would work better IMHO.
> /* Point again at the beginning of the name. */
> --name;
This assignment is now dead, and the comment is wrong.
(Still need to look at the rest of the patch.)
Thanks,
Florian
On 06/06/2018 07:02 AM, Carlos O'Donell wrote:
> +/* Passed the start of a DST sequence at the first '$' occurrence.
> + See the DL_DST_COUNT macro which inlines the strchr to find the
> + first occurrence of '$' and optimizes that likely case that there
> + is no DST. 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). */
Maybe kill DL_DST_COUNT? It doesn't look useful to me.
> + /* All DSTs must follow ELF gABI rules, see is_dst (). */
> + if ((len = is_dst (name, "ORIGIN")) != 0
> + || (len = is_dst (name, "PLATFORM")) != 0
> + || (len = is_dst (name, "LIB")) != 0)
> ++cnt;
len is never read, so you can remove the variable.
Thanks,
Florian
On 06/06/2018 10:00 AM, Florian Weimer wrote:
> On 06/06/2018 07:02 AM, Carlos O'Donell wrote:
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index 431236920f..13263212d5 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
>> @@ -177,63 +177,89 @@ is_trusted_path_normalize (const char *path, size_t len)
>> return false;
>> }
>> +/* Given a substring starting at NAME, just after the DST '$' start
>> + token, determine if NAME contains dynamic string token STR,
>> + following the ELF gABI rules for dynamic string tokens:
>> + * Longest possible sequence using the rules (greedy).
>> +
>> + * Must start with a $ (enforced by caller).
>> +
>> + * Must follow $ with one underscore or ASCII [A-Za-z] (enforced by
>> + caller via STR comparison) or '{' (start curly quoted name).
>
> “enforced by caller via STR comparison”: I don't see this in the remaining code. So $ORIGIN and $PRIGIN are currently treated the same?
This is a mistake when I moved the code from my test harness into is_dst.
The clarified comment is this:
* Must follow $ with one underscore or ASCII [A-Za-z] (caller
follows these rules for STR and we enforce the compariso)
or '{' (start curly quoted name).
The updated code is this:
/* Compare the DST (no strncmp this early in startup). */
len = 0;
while (name[len] == str[len] && len < nlen)
++len;
/* Characters of name don't match DST. */
if (len != nlen)
return 0;
Very similar to the original code, but using the computed length
nlen (greedy length of 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 dynamic string token matching STR 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 *name, const char *str)
>
> I'm not entirely happy about the choice of parameter names. name/reference or input/name would work better IMHO.
Yeah, me neither, I tried to standardize across the board as name/str.
I will change everything over to use:
input - Incoming input byte stream with possible DSTs.
ref - Reference DST for comparison.
>> /* Point again at the beginning of the name. */
>> --name;
>
> This assignment is now dead, and the comment is wrong.
Good catch. Removed.
> (Still need to look at the rest of the patch.)
Thanks.
Cheers,
Carlos.
On Jun 06 2018, Carlos O'Donell <carlos@redhat.com> wrote:
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 431236920f..13263212d5 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -177,63 +177,89 @@ is_trusted_path_normalize (const char *path, size_t len)
> return false;
> }
>
> +/* Given a substring starting at NAME, just after the DST '$' start
> + token, determine if NAME contains dynamic string token STR,
> + following the ELF gABI rules for dynamic string tokens:
>
> + * Longest possible sequence using the rules (greedy).
> +
> + * Must start with a $ (enforced by caller).
> +
> + * Must follow $ with one underscore or ASCII [A-Za-z] (enforced by
> + caller via STR 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 dynamic string token matching STR 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 *name, const char *str)
> {
> - size_t len;
> + size_t nlen, slen;
> bool is_curly = false;
>
> + /* Is a ${...} name sequence? */
> if (name[0] == '{')
> {
> is_curly = true;
> ++name;
> }
>
> - len = 0;
> - while (name[len] == str[len] && name[len] != '\0')
> - ++len;
> + /* Find longest valid name sequence. */
> + nlen = 0;
> + while ((name[nlen] >= 'A' && name[nlen] <= 'Z')
> + || (name[nlen] >= 'a' && name[nlen] <= 'z')
> + || (name[nlen] >= '0' && name[nlen] <= '9')
> + || (name[nlen] == '_'))
> + ++nlen;
> +
> + slen = strlen (str);
You are completely ignoring the contents of str now. That doesn't make
sense.
Andreas.
On 06/06/2018 10:15 AM, Florian Weimer wrote:
> On 06/06/2018 07:02 AM, Carlos O'Donell wrote:
>> +/* Passed the start of a DST sequence at the first '$' occurrence.
>> + See the DL_DST_COUNT macro which inlines the strchr to find the
>> + first occurrence of '$' and optimizes that likely case that there
>> + is no DST. 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). */
>
> Maybe kill DL_DST_COUNT? It doesn't look useful to me.
Killed.
>> + /* All DSTs must follow ELF gABI rules, see is_dst (). */
>> + if ((len = is_dst (name, "ORIGIN")) != 0
>> + || (len = is_dst (name, "PLATFORM")) != 0
>> + || (len = is_dst (name, "LIB")) != 0)
>> ++cnt;
>
> len is never read, so you can remove the variable.
Removed.
v2 patch coming up.
Cheers,
Carlos
On 06/06/2018 11:47 AM, Andreas Schwab wrote:
> You are completely ignoring the contents of str now. That doesn't make
> sense.
Correct, it was a mistake in the posted patch.
v2 coming with a fix, and a new test case to ensure I don't cut-n-paste
the wrong thing again.
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: Fix comment.
+
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
@@ -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,8 +18,7 @@
#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). */
+/* Determine the number of DST elements in the name. */
#define DL_DST_COUNT(name) \
({ \
size_t __cnt = 0; \
@@ -177,63 +177,89 @@ is_trusted_path_normalize (const char *path, size_t len)
return false;
}
+/* Given a substring starting at NAME, just after the DST '$' start
+ token, determine if NAME contains dynamic string token STR,
+ following the ELF gABI rules for dynamic string tokens:
+ * Longest possible sequence using the rules (greedy).
+
+ * Must start with a $ (enforced by caller).
+
+ * Must follow $ with one underscore or ASCII [A-Za-z] (enforced by
+ caller via STR 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 dynamic string token matching STR 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 *name, const char *str)
{
- size_t len;
+ size_t nlen, slen;
bool is_curly = false;
+ /* Is a ${...} name sequence? */
if (name[0] == '{')
{
is_curly = true;
++name;
}
- len = 0;
- while (name[len] == str[len] && name[len] != '\0')
- ++len;
+ /* Find longest valid name sequence. */
+ nlen = 0;
+ while ((name[nlen] >= 'A' && name[nlen] <= 'Z')
+ || (name[nlen] >= 'a' && name[nlen] <= 'z')
+ || (name[nlen] >= '0' && name[nlen] <= '9')
+ || (name[nlen] == '_'))
+ ++nlen;
+
+ slen = strlen (str);
+
+ /* Can't be the DST we are looking for. */
+ if (slen != nlen)
+ return 0;
if (is_curly)
{
- if (name[len] != '}')
+ /* Invalid curly sequence! */
+ if (name[nlen] != '}')
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. */
+ nlen += 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 nlen;
}
-
+/* Passed the start of a DST sequence at the first '$' occurrence.
+ See the DL_DST_COUNT macro which inlines the strchr to find the
+ first occurrence of '$' and optimizes that likely case that there
+ is no DST. 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)
{
- const char *const start = name;
size_t cnt = 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)
+ /* All DSTs must follow ELF gABI rules, see is_dst (). */
+ if ((len = is_dst (name, "ORIGIN")) != 0
+ || (len = is_dst (name, "PLATFORM")) != 0
+ || (len = is_dst (name, "LIB")) != 0)
++cnt;
+ /* There may be more than one DST in the sequence. */
name = strchr (name + len, '$');
}
while (name != NULL);
@@ -241,12 +267,14 @@ _dl_dst_count (const char *name)
return cnt;
}
-
+/* Process NAME 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 NAME 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 *name, 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)
@@ -263,15 +291,36 @@ _dl_dst_substitute (struct link_map *l, const char *name, char *result)
size_t len;
++name;
- if ((len = is_dst (start, name, "ORIGIN", __libc_enable_secure)) != 0)
+ if ((len = is_dst (name, "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)
+ && ((name[len] != '\0' && name[len] != '/'
+ && name[len] != ':')
+ || (name != start + 1
+ || (name > start + 2 && name[-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 (name, "PLATFORM")) != 0)
repl = GLRO(dl_platform);
- else if ((len = is_dst (start, name, "LIB", 0)) != 0)
+ else if ((len = is_dst (name, "LIB")) != 0)
repl = DL_DST_LIB;
if (repl != NULL && repl != (const char *) -1)
@@ -283,6 +332,7 @@ _dl_dst_substitute (struct link_map *l, const char *name, char *result)
{
/* We cannot use this path element, the value of the
replacement is unknown. */
+ check_for_trusted = false;
wp = last_elem;
break;
}
@@ -309,13 +359,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 NAME with all recognized DSTs
+ ($ORIGIN and $PLATFORM for now) replaced. The value of START must
+ equal the start of the parent string if NAME 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 *name)
{
/* 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 +380,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 (name);
/* If we do not have to replace anything simply copy the string. */
if (__glibc_likely (cnt == 0))
- return __strdup (s);
+ return __strdup (name);
/* Determine the length of the substituted string. */
- total = DL_DST_REQUIRED (l, s, strlen (s), cnt);
+ total = DL_DST_REQUIRED (l, name, strlen (name), 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, name, result);
}
@@ -387,6 +441,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 +453,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 +2146,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