| Message ID | 20171218204213.26583-1-aurelien@aurel32.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] elf: Check for empty tokens before dynamic string token expansion [BZ #22625] | expand |
On Mon, Dec 18, 2017 at 09:42:13PM +0100, Aurelien Jarno wrote: > The fillin_rpath function in elf/dl-load.c loops over each RPATH or > RUNPATH tokens and intepret empty tokens as the current directory > ("./"). In practice the check for empty token is done *after* the > dynamic string token expansion. The expansion process can return an > empty string for the $ORIGIN token if __libc_enable_secure is set > or if the path of the binary can not be determined (/proc not mounted). > > To fix that, move the check for empty tokens before the dynamic string > token expansion. Change expand_dynamic_string_token to return NULL > instead of an empty string, and check for NULL pointer returned by > expand_dynamic_string_token. > > The above changed highlighted a bug in decompose_rpath, an empty array > is represented by the first element being NULL at the fillin_rpath path > level, but by using a -1 pointer in decompose_rpath and other functions. > > Changelog: > [BZ #22625] > * elf/dl-load.c (expand_dynamic_string_token): Return NULL instead > of an empty string. > (fillin_rpath): Check for empty tokens before dynamic string token > expansion. Check for NULL pointer possibly returned by > expand_dynamic_string_token. > (decompose_rpath): Check for empty path after dynamic string > token expansion. > --- > ChangeLog | 12 ++++++++++++ > NEWS | 4 ++++ > elf/dl-load.c | 43 ++++++++++++++++++++++++++++++++++++------- > 3 files changed, 52 insertions(+), 7 deletions(-) > > > This new version includes the suggestions done by Dmitry. It would be > nice if it could be reviewed by at least one more person, especially > the part modifying expand_dynamic_string_token. > > diff --git a/ChangeLog b/ChangeLog > index 4a7164354f..24a5223485 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,15 @@ > +2017-12-17 Aurelien Jarno <aurelien@aurel32.net> > + Dmitry V. Levin <ldv@altlinux.org> > + > + [BZ #22625] > + * elf/dl-load.c (expand_dynamic_string_token): Return NULL instead > + of an empty string. > + (fillin_rpath): Check for empty tokens before dynamic string token > + expansion. Check for NULL pointer possibly returned by > + expand_dynamic_string_token. > + (decompose_rpath): Check for empty path after dynamic string > + token expansion. > + > 2017-12-18 Sergei Trofimovich <slyfox@gentoo.org> > > [BZ #22624] > diff --git a/NEWS b/NEWS > index a43ff26e83..0d43e93a17 100644 > --- a/NEWS > +++ b/NEWS > @@ -149,6 +149,10 @@ Security related changes: > CVE-2017-1000366 has been applied, but it is mentioned here only because > of the CVE assignment.) Reported by Qualys. > > + CVE-2017-16997: Incorrect handling of RPATH or RUNPATH containing $ORIGIN > + for AT_SECURE or SUID binaries could be used to load libraries from the > + current directory. > + > The following bugs are resolved with this release: > > [The release manager will add the list generated by > diff --git a/elf/dl-load.c b/elf/dl-load.c > index e7d97dcc56..827ec1c491 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -384,7 +384,17 @@ expand_dynamic_string_token (struct link_map *l, const char *s, int is_path) > if (result == NULL) > return NULL; > > - return _dl_dst_substitute (l, s, result, is_path); > + char *retval = _dl_dst_substitute (l, s, result, is_path); > + > + /* If substitution of dynamic string tokens resulted to an empty string, > + return NULL as in case of insufficient memory. */ > + if (__glibc_unlikely (*retval == '\0')) > + { > + free (result); > + return NULL; > + } > + > + return retval; > } > > > @@ -433,22 +443,33 @@ fillin_rpath (char *rpath, struct r_search_path_elem **result, const char *sep, > { > char *cp; > size_t nelems = 0; > - char *to_free; > > while ((cp = __strsep (&rpath, sep)) != NULL) > { > struct r_search_path_elem *dirp; > - > - to_free = cp = expand_dynamic_string_token (l, cp, 1); > - > - size_t len = strlen (cp); > + char *to_free = NULL; > + size_t len; > > /* `strsep' can pass an empty string. This has to be > interpreted as `use the current directory'. */ > - if (len == 0) > + if (*cp == '\0') > { > static const char curwd[] = "./"; > cp = (char *) curwd; > + len = 0; > + } > + else > + { > + to_free = cp = expand_dynamic_string_token (l, cp, 1); > + > + /* expand_dynamic_string_token can return NULL in case of empty > + path or memory allocation failure. */ > + if (cp == NULL) > + continue; > + > + /* Recompute the length after dynamic string token expansion > + and ignore empty paths. */ Just /* Compute the length after dynamic string token expansion. */ > + len = strlen (cp); > } > > /* Remove trailing slashes (except for "/"). */ > @@ -620,6 +641,14 @@ decompose_rpath (struct r_search_path_struct *sps, > necessary. */ > free (copy); > > + /* There is no path after expansion. */ > + if (result[0] == NULL) > + { > + free (result); > + sps->dirs = (struct r_search_path_elem **) -1; > + return false; > + } > + > sps->dirs = result; > /* The caller will change this value if we haven't used a real malloc. */ > sps->malloced = 1; I think the patch is fine.
On 2017-12-19 00:16, Dmitry V. Levin wrote: > On Mon, Dec 18, 2017 at 09:42:13PM +0100, Aurelien Jarno wrote: > > The fillin_rpath function in elf/dl-load.c loops over each RPATH or > > RUNPATH tokens and intepret empty tokens as the current directory > > ("./"). In practice the check for empty token is done *after* the > > dynamic string token expansion. The expansion process can return an > > empty string for the $ORIGIN token if __libc_enable_secure is set > > or if the path of the binary can not be determined (/proc not mounted). > > > > To fix that, move the check for empty tokens before the dynamic string > > token expansion. Change expand_dynamic_string_token to return NULL > > instead of an empty string, and check for NULL pointer returned by > > expand_dynamic_string_token. > > > > The above changed highlighted a bug in decompose_rpath, an empty array > > is represented by the first element being NULL at the fillin_rpath path > > level, but by using a -1 pointer in decompose_rpath and other functions. > > > > Changelog: > > [BZ #22625] > > * elf/dl-load.c (expand_dynamic_string_token): Return NULL instead > > of an empty string. > > (fillin_rpath): Check for empty tokens before dynamic string token > > expansion. Check for NULL pointer possibly returned by > > expand_dynamic_string_token. > > (decompose_rpath): Check for empty path after dynamic string > > token expansion. > > --- > > ChangeLog | 12 ++++++++++++ > > NEWS | 4 ++++ > > elf/dl-load.c | 43 ++++++++++++++++++++++++++++++++++++------- > > 3 files changed, 52 insertions(+), 7 deletions(-) > > > > > > This new version includes the suggestions done by Dmitry. It would be > > nice if it could be reviewed by at least one more person, especially > > the part modifying expand_dynamic_string_token. > > > > diff --git a/ChangeLog b/ChangeLog > > index 4a7164354f..24a5223485 100644 > > --- a/ChangeLog > > +++ b/ChangeLog > > @@ -1,3 +1,15 @@ > > +2017-12-17 Aurelien Jarno <aurelien@aurel32.net> > > + Dmitry V. Levin <ldv@altlinux.org> > > + > > + [BZ #22625] > > + * elf/dl-load.c (expand_dynamic_string_token): Return NULL instead > > + of an empty string. > > + (fillin_rpath): Check for empty tokens before dynamic string token > > + expansion. Check for NULL pointer possibly returned by > > + expand_dynamic_string_token. > > + (decompose_rpath): Check for empty path after dynamic string > > + token expansion. > > + > > 2017-12-18 Sergei Trofimovich <slyfox@gentoo.org> > > > > [BZ #22624] > > diff --git a/NEWS b/NEWS > > index a43ff26e83..0d43e93a17 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -149,6 +149,10 @@ Security related changes: > > CVE-2017-1000366 has been applied, but it is mentioned here only because > > of the CVE assignment.) Reported by Qualys. > > > > + CVE-2017-16997: Incorrect handling of RPATH or RUNPATH containing $ORIGIN > > + for AT_SECURE or SUID binaries could be used to load libraries from the > > + current directory. > > + > > The following bugs are resolved with this release: > > > > [The release manager will add the list generated by > > diff --git a/elf/dl-load.c b/elf/dl-load.c > > index e7d97dcc56..827ec1c491 100644 > > --- a/elf/dl-load.c > > +++ b/elf/dl-load.c > > @@ -384,7 +384,17 @@ expand_dynamic_string_token (struct link_map *l, const char *s, int is_path) > > if (result == NULL) > > return NULL; > > > > - return _dl_dst_substitute (l, s, result, is_path); > > + char *retval = _dl_dst_substitute (l, s, result, is_path); > > + > > + /* If substitution of dynamic string tokens resulted to an empty string, > > + return NULL as in case of insufficient memory. */ > > + if (__glibc_unlikely (*retval == '\0')) > > + { > > + free (result); > > + return NULL; > > + } > > + > > + return retval; > > } > > > > > > @@ -433,22 +443,33 @@ fillin_rpath (char *rpath, struct r_search_path_elem **result, const char *sep, > > { > > char *cp; > > size_t nelems = 0; > > - char *to_free; > > > > while ((cp = __strsep (&rpath, sep)) != NULL) > > { > > struct r_search_path_elem *dirp; > > - > > - to_free = cp = expand_dynamic_string_token (l, cp, 1); > > - > > - size_t len = strlen (cp); > > + char *to_free = NULL; > > + size_t len; > > > > /* `strsep' can pass an empty string. This has to be > > interpreted as `use the current directory'. */ > > - if (len == 0) > > + if (*cp == '\0') > > { > > static const char curwd[] = "./"; > > cp = (char *) curwd; > > + len = 0; > > + } > > + else > > + { > > + to_free = cp = expand_dynamic_string_token (l, cp, 1); > > + > > + /* expand_dynamic_string_token can return NULL in case of empty > > + path or memory allocation failure. */ > > + if (cp == NULL) > > + continue; > > + > > + /* Recompute the length after dynamic string token expansion > > + and ignore empty paths. */ > > Just /* Compute the length after dynamic string token expansion. */ Good catch, I have fixed that locally. > > > + len = strlen (cp); > > } > > > > /* Remove trailing slashes (except for "/"). */ > > @@ -620,6 +641,14 @@ decompose_rpath (struct r_search_path_struct *sps, > > necessary. */ > > free (copy); > > > > + /* There is no path after expansion. */ > > + if (result[0] == NULL) > > + { > > + free (result); > > + sps->dirs = (struct r_search_path_elem **) -1; > > + return false; > > + } > > + > > sps->dirs = result; > > /* The caller will change this value if we haven't used a real malloc. */ > > sps->malloced = 1; > > I think the patch is fine. > Thanks for the review, let's see if someone else has comments on it.
On Dez 18 2017, Aurelien Jarno <aurelien@aurel32.net> wrote: > @@ -433,22 +443,33 @@ fillin_rpath (char *rpath, struct r_search_path_elem **result, const char *sep, > { > char *cp; > size_t nelems = 0; > - char *to_free; > > while ((cp = __strsep (&rpath, sep)) != NULL) > { > struct r_search_path_elem *dirp; > - > - to_free = cp = expand_dynamic_string_token (l, cp, 1); > - > - size_t len = strlen (cp); > + char *to_free = NULL; > + size_t len; > > /* `strsep' can pass an empty string. This has to be > interpreted as `use the current directory'. */ > - if (len == 0) > + if (*cp == '\0') > { > static const char curwd[] = "./"; > cp = (char *) curwd; > + len = 0; > + } I wonder why we need curwd at all. Nothing below this reads past cp[len] anyway. Andreas.
On Tue, Dec 19, 2017 at 11:59:45AM +0100, Andreas Schwab wrote: > On Dez 18 2017, Aurelien Jarno <aurelien@aurel32.net> wrote: > > > @@ -433,22 +443,33 @@ fillin_rpath (char *rpath, struct r_search_path_elem **result, const char *sep, > > { > > char *cp; > > size_t nelems = 0; > > - char *to_free; > > > > while ((cp = __strsep (&rpath, sep)) != NULL) > > { > > struct r_search_path_elem *dirp; > > - > > - to_free = cp = expand_dynamic_string_token (l, cp, 1); > > - > > - size_t len = strlen (cp); > > + char *to_free = NULL; > > + size_t len; > > > > /* `strsep' can pass an empty string. This has to be > > interpreted as `use the current directory'. */ > > - if (len == 0) > > + if (*cp == '\0') > > { > > static const char curwd[] = "./"; > > cp = (char *) curwd; > > + len = 0; > > + } > > I wonder why we need curwd at all. Nothing below this reads past > cp[len] anyway. Yes, I think `cp' could be left unchanged; the code below may access cp[0], so it cannot be set to NULL, though. The code could be simplified further, e.g. size_t len = 0; if (*cp != '\0') { to_free = cp = expand_dynamic_string_token (l, cp, 1); ... }
On 12/18/2017 09:42 PM, Aurelien Jarno wrote: > - return _dl_dst_substitute (l, s, result, is_path); > + char *retval = _dl_dst_substitute (l, s, result, is_path); > + > + /* If substitution of dynamic string tokens resulted to an empty string, > + return NULL as in case of insufficient memory. */ > + if (__glibc_unlikely (*retval == '\0')) > + { > + free (result); > + return NULL; > + } > + > + return retval; I'm not really happy with this. OOM and a zero-string expansion are very different things. We seem to have other abuses in the existing code, but I don't think we should add further instances. Furthermore, I'm not sure if the fix is robust enough. There is another bug in the $ORIGIN DST component skipping: $ORIGIN eats the trailing colon. This is because this code: else { *wp++ = *name++; if (is_path && *name == ':') happens when name is at the ':' after $ORIGIN processing, so only the next ':' after that (or the end of the string) terminates this component, and the call to the is_trusted_path_normalize covers two components mushed together instead of one. So the check likely fails, and both path elements are skipped. In some cases, this results in an empty string where it would not usually be non-empty. Thanks, Florian
On 2017-12-19 14:16, Florian Weimer wrote: > On 12/18/2017 09:42 PM, Aurelien Jarno wrote: > > - return _dl_dst_substitute (l, s, result, is_path); > > + char *retval = _dl_dst_substitute (l, s, result, is_path); > > + > > + /* If substitution of dynamic string tokens resulted to an empty string, > > + return NULL as in case of insufficient memory. */ > > + if (__glibc_unlikely (*retval == '\0')) > > + { > > + free (result); > > + return NULL; > > + } > > + > > + return retval; > > I'm not really happy with this. OOM and a zero-string expansion are very > different things. We seem to have other abuses in the existing code, but I > don't think we should add further instances. That's something changed from the first version. I can rollback that change, and add back the check or empty string in fillin_rpath. > Furthermore, I'm not sure if the fix is robust enough. There is another bug > in the $ORIGIN DST component skipping: $ORIGIN eats the trailing colon. > This is because this code: > > else > { > *wp++ = *name++; > if (is_path && *name == ':') > > happens when name is at the ':' after $ORIGIN processing, so only the next > ':' after that (or the end of the string) terminates this component, and the > call to the is_trusted_path_normalize covers two components mushed together > instead of one. So the check likely fails, and both path elements are > skipped. In some cases, this results in an empty string where it would not > usually be non-empty. While this is indeed a problem in _dl_dst_substitute, it's not an issue in this case as expand_dynamic_string_token and thus _dl_dst_substitute do not get called from fillin_rpath with a colon. Indeed since commit b75891075bec (which introduced this bug), expand_dynamic_string_token is called after the split of the path by colons instead of before. Aurelien
On Tue, Dec 19, 2017 at 06:19:06PM +0100, Aurelien Jarno wrote: > On 2017-12-19 14:16, Florian Weimer wrote: > > On 12/18/2017 09:42 PM, Aurelien Jarno wrote: > > > - return _dl_dst_substitute (l, s, result, is_path); > > > + char *retval = _dl_dst_substitute (l, s, result, is_path); > > > + > > > + /* If substitution of dynamic string tokens resulted to an empty string, > > > + return NULL as in case of insufficient memory. */ > > > + if (__glibc_unlikely (*retval == '\0')) > > > + { > > > + free (result); > > > + return NULL; > > > + } > > > + > > > + return retval; > > > > I'm not really happy with this. OOM and a zero-string expansion are very > > different things. We seem to have other abuses in the existing code, but I > > don't think we should add further instances. > > That's something changed from the first version. I can rollback that > change, and add back the check or empty string in fillin_rpath. That was my idea to move the check into expand_dynamic_string_token, feel free to rollback if you like. > > Furthermore, I'm not sure if the fix is robust enough. There is another bug > > in the $ORIGIN DST component skipping: $ORIGIN eats the trailing colon. > > This is because this code: > > > > else > > { > > *wp++ = *name++; > > if (is_path && *name == ':') > > > > happens when name is at the ':' after $ORIGIN processing, so only the next > > ':' after that (or the end of the string) terminates this component, and the > > call to the is_trusted_path_normalize covers two components mushed together > > instead of one. So the check likely fails, and both path elements are > > skipped. In some cases, this results in an empty string where it would not > > usually be non-empty. > > While this is indeed a problem in _dl_dst_substitute, it's not an issue > in this case as expand_dynamic_string_token and thus _dl_dst_substitute > do not get called from fillin_rpath with a colon. Indeed since commit > b75891075bec (which introduced this bug), expand_dynamic_string_token > is called after the split of the path by colons instead of before. Yes. In fact, the check is now redundant and could be omitted if desired: --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -326,16 +326,8 @@ _dl_dst_substitute (struct link_map *l, const char *name, char *result, *wp++ = *name++; if (is_path && *name == ':') { - /* In SUID/SGID programs, after $ORIGIN expansion the - normalized path must be rooted in one of the trusted - directories. */ - if (__glibc_unlikely (check_for_trusted) - && !is_trusted_path_normalize (last_elem, wp - last_elem)) - wp = last_elem; - else - last_elem = wp; - - check_for_trusted = false; + /* check_for_trusted == false */ + last_elem = wp; } } }
diff --git a/ChangeLog b/ChangeLog index 4a7164354f..24a5223485 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,15 @@ +2017-12-17 Aurelien Jarno <aurelien@aurel32.net> + Dmitry V. Levin <ldv@altlinux.org> + + [BZ #22625] + * elf/dl-load.c (expand_dynamic_string_token): Return NULL instead + of an empty string. + (fillin_rpath): Check for empty tokens before dynamic string token + expansion. Check for NULL pointer possibly returned by + expand_dynamic_string_token. + (decompose_rpath): Check for empty path after dynamic string + token expansion. + 2017-12-18 Sergei Trofimovich <slyfox@gentoo.org> [BZ #22624] diff --git a/NEWS b/NEWS index a43ff26e83..0d43e93a17 100644 --- a/NEWS +++ b/NEWS @@ -149,6 +149,10 @@ Security related changes: CVE-2017-1000366 has been applied, but it is mentioned here only because of the CVE assignment.) Reported by Qualys. + CVE-2017-16997: Incorrect handling of RPATH or RUNPATH containing $ORIGIN + for AT_SECURE or SUID binaries could be used to load libraries from the + current directory. + The following bugs are resolved with this release: [The release manager will add the list generated by diff --git a/elf/dl-load.c b/elf/dl-load.c index e7d97dcc56..827ec1c491 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -384,7 +384,17 @@ expand_dynamic_string_token (struct link_map *l, const char *s, int is_path) if (result == NULL) return NULL; - return _dl_dst_substitute (l, s, result, is_path); + char *retval = _dl_dst_substitute (l, s, result, is_path); + + /* If substitution of dynamic string tokens resulted to an empty string, + return NULL as in case of insufficient memory. */ + if (__glibc_unlikely (*retval == '\0')) + { + free (result); + return NULL; + } + + return retval; } @@ -433,22 +443,33 @@ fillin_rpath (char *rpath, struct r_search_path_elem **result, const char *sep, { char *cp; size_t nelems = 0; - char *to_free; while ((cp = __strsep (&rpath, sep)) != NULL) { struct r_search_path_elem *dirp; - - to_free = cp = expand_dynamic_string_token (l, cp, 1); - - size_t len = strlen (cp); + char *to_free = NULL; + size_t len; /* `strsep' can pass an empty string. This has to be interpreted as `use the current directory'. */ - if (len == 0) + if (*cp == '\0') { static const char curwd[] = "./"; cp = (char *) curwd; + len = 0; + } + else + { + to_free = cp = expand_dynamic_string_token (l, cp, 1); + + /* expand_dynamic_string_token can return NULL in case of empty + path or memory allocation failure. */ + if (cp == NULL) + continue; + + /* Recompute the length after dynamic string token expansion + and ignore empty paths. */ + len = strlen (cp); } /* Remove trailing slashes (except for "/"). */ @@ -620,6 +641,14 @@ decompose_rpath (struct r_search_path_struct *sps, necessary. */ free (copy); + /* There is no path after expansion. */ + if (result[0] == NULL) + { + free (result); + sps->dirs = (struct r_search_path_elem **) -1; + return false; + } + sps->dirs = result; /* The caller will change this value if we haven't used a real malloc. */ sps->malloced = 1;
The fillin_rpath function in elf/dl-load.c loops over each RPATH or RUNPATH tokens and intepret empty tokens as the current directory ("./"). In practice the check for empty token is done *after* the dynamic string token expansion. The expansion process can return an empty string for the $ORIGIN token if __libc_enable_secure is set or if the path of the binary can not be determined (/proc not mounted). To fix that, move the check for empty tokens before the dynamic string token expansion. Change expand_dynamic_string_token to return NULL instead of an empty string, and check for NULL pointer returned by expand_dynamic_string_token. The above changed highlighted a bug in decompose_rpath, an empty array is represented by the first element being NULL at the fillin_rpath path level, but by using a -1 pointer in decompose_rpath and other functions. Changelog: [BZ #22625] * elf/dl-load.c (expand_dynamic_string_token): Return NULL instead of an empty string. (fillin_rpath): Check for empty tokens before dynamic string token expansion. Check for NULL pointer possibly returned by expand_dynamic_string_token. (decompose_rpath): Check for empty path after dynamic string token expansion. --- ChangeLog | 12 ++++++++++++ NEWS | 4 ++++ elf/dl-load.c | 43 ++++++++++++++++++++++++++++++++++++------- 3 files changed, 52 insertions(+), 7 deletions(-) This new version includes the suggestions done by Dmitry. It would be nice if it could be reviewed by at least one more person, especially the part modifying expand_dynamic_string_token.