[PATCH] elf: Open the normalized $ORIGIN rpath in AT_SECURE programs (BZ 34360)

[Florian Weimer]

* Adhemerval Zanella:

> +/* Lexically normalize the NUL-terminated PATH in place, collapsing "//",
> +   "/./" and "/../" segments (a leading "/../" collapses to "/").  The
> +   normalized string is a rearrangement of a prefix of PATH: the write
> +   cursor never runs ahead of the read cursor and no trailing character is
> +   appended, so this only ever touches bytes within the original
> +   strlen (PATH) + 1 storage and can never access memory out of bounds.
> +   Returns the length of the normalized path (excluding the terminating
> +   NUL).  */
> +static size_t
> +dst_normalize_path (char *path)
>  {
> +  char *wnp = path;
> +  const char *rnp = path;
> +  while (*rnp != '\0')
>      {
> +      if (rnp[0] == '/')
>  	{
> +	  /* Collapse a run of '/' to a single one before interpreting "/."
> +	     or "/..", so that the "." or ".." is applied to the real
> +	     preceding component rather than to an empty "//" segment: skip
> +	     this '/' whenever it is immediately followed by another one.  */
> +	  if (rnp[1] == '/')
>  	    {
> +	      ++rnp;
> +	      continue;
>  	    }
>  
> +	  if (rnp[1] == '.')
>  	    {
> +	      if (rnp[2] == '.' && (rnp[3] == '/' || rnp[3] == '\0'))
> +		{
> +		  while (wnp > path && *--wnp != '/')
> +		    ;
> +		  rnp += 3;
> +		  continue;
> +		}
> +	      else if (rnp[2] == '/' || rnp[2] == '\0')
> +		{
> +		  rnp += 2;
> +		  continue;
> +		}
>  	    }
>  	}
>  
> +      *wnp++ = *rnp++;
>      }
>  
> +  *wnp = '\0';
> +  return wnp - path;
> +}

This turns "/a/.." into "", which doesn't look right to me.  "a/../"
ends up as "/", and "a/.." as "".  And of course "" remains "".  The ""
special case probably needs to be called out in the function comment.
The comment should also clarify the expected behavior regarding trailing
slashes (the caller cannot assume their presence or absence).

I suggest putting this function into a separate (header) file, so that
we can test it directly.

The integration test looks okay to me.

>  /* Given a substring starting at INPUT, just after the DST '$' start
> @@ -327,6 +342,8 @@ _dl_dst_substitute (struct link_map *l, const char *input, char *result)
>      }
>    while (*input != '\0');
>  
> +  *wp = '\0';
> +
>    /* In SUID/SGID programs, after $ORIGIN expansion the normalized
>       path must be rooted in one of the trusted directories.  The $LIB
>       and $PLATFORM DST cannot in any way be manipulated by the caller
> @@ -335,15 +352,21 @@ _dl_dst_substitute (struct link_map *l, const char *input, char *result)
>       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.  */
> -  if (__glibc_unlikely (check_for_trusted)
> -      && !is_trusted_path_normalize (result, wp - result))
> -    {
> -      *result = '\0';
> -      return result;
> -    }
> +     links.
>  
> -  *wp = '\0';
> +     Checking the normalized path but opening the raw one is not enough:
> +     "a/b/../c" only names "a/c" when "b" is not a symbolic link, so an
> +     attacker who controls a component of $ORIGIN (for example by
> +     hard-linking the program into an attacker-owned directory) could
> +     otherwise redirect the lookup outside the trusted directory.  Replace
> +     the expansion with its normalized, "../"-free form, so that the path
> +     that is opened is exactly the path that was validated.  */
> +  if (__glibc_unlikely (check_for_trusted))
> +    {
> +      size_t nlen = dst_normalize_path (result);
> +      if (!path_is_trusted (result, nlen))
> +	*result = '\0';
> +    }

It turns out this fixes a potential buffer overflow as well.
Previously, we called is_trusted_path_normalize on a potential
non-null-terminated byte array.  The old loop iteration did not stop at
the specified length, but at the first null byte.  So this could end up
writing beyond the end of the stack-allocated array.

I've attached the full report below.

I've been instructed to mention: Found by AISLE in partnership with Red Hat


I think under the current rules, these two bugs do not need separate in
CVE assignment even though they are very different in nature.  They were
introduced in the same commit, and as your patch shows, it's not really
possible to fix them separately.

Thanks,
Florian
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20260814/5dc58ad5/attachment-0001.html>