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

* Adhemerval Zanella:

> diff --git a/elf/dl-path-normalize.h b/elf/dl-path-normalize.h
> new file mode 100644
> index 00000000000..96e04f5d14e
> --- /dev/null
> +++ b/elf/dl-path-normalize.h
> @@ -0,0 +1,114 @@

> +static inline size_t
> +_dl_normalize_path (char *path)
> +{
> +  /* The root '/' of an absolute path is not removed.  */
> +  char *pstart = path + (path[0] == '/');
> +  const char *rnp = pstart;
> +  char *wnp = pstart;
> +  /* End of the prefix a ".." may not remove, the root '/' plus any preserved
> +     leading ".." components of a relative path.  */
> +  char *limit = pstart;

Suggestion: 

+  /* End of the prefix a ".." may not remove.  Either the root '/', or,
+     for relative paths, the original start of the string or any ascending
+     leading ".." components.  */

Or something like that.

> +
> +  while (*rnp != '\0')
> +    {
> +      /* Collapse consecutive separators.  */
> +      if (*rnp == '/')
> +	{
> +	  ++rnp;
> +	  continue;
> +	}
> +
> +      /* [RNP, REND) is the next input component.  */
> +      const char *rend = rnp;
> +      while (*rend != '\0' && *rend != '/')
> +	++rend;
> +      size_t clen = rend - rnp;

I think that's just

  const char *rend = __strchrnul (rnp, '/');

?

> +
> +      /* Drop '.' component.  */
> +      if (clen == 1 && rnp[0] == '.')
> +	;
> +      else if (clen == 2 && rnp[0] == '.' && rnp[1] == '.')
> +	{
> +	  if (wnp > limit)
> +	    {
> +	      /* Remove the last component along with the '/' separating it
> +		 from its predecessor (the root '/' of an absolute path is
> +		 retained).  */
> +	      while (wnp > limit && wnp[-1] != '/')
> +		--wnp;

I believe this can be expressed in terms of memrchr, but memrchr is not
available in the dynamic linker.

> +	      if (wnp > pstart)
> +		--wnp;
> +	    }
> +	  else if (pstart == path)
> +	    {

“pstart == path” checks for a relative orignal path.  Maybe mention
this here?

> +	      /* No component is left: keep the unresolvable ".." for a
> +		 relative path (it becomes part of the preserved prefix),
> +		 drop it at the root of an absolute one.  */
> +	      if (wnp > pstart)
> +		*wnp++ = '/';
> +	      *wnp++ = '.';
> +	      *wnp++ = '.';
> +	      limit = wnp;
> +	    }

The missing empty else branch here drops the the ../.  Maybe put that
part of the comment here.

> +	}
> +      else
> +	{
> +	  if (wnp > pstart)
> +	    *wnp++ = '/';
> +	  while (rnp < rend)
> +	    *wnp++ = *rnp++;
> +	}

The while loop is just mempcpy?  clen has already been computed above.
The update of rnp is redundant with the assigned below.

> +
> +      rnp = rend;
> +    }
> +
> +  *wnp = '\0';
> +  return wnp - path;
> +}

All these are just suggestions.  Maybe leave the code as-is and only
theck the comments because changing the code would invalidate the
testing below.

I reviewed the logic and it is sound.  The specification also matches
what we need.

Furthermore, I auto-generated a completely different implementation
(using recursion).  I ran both implementations against each other in a
fuzzer, and found no differences.  I also made sure that your
implementation would not crash on the fuzzing corpus when built with
Address Sanitizer.

Together with my manual review, I'm pretty confident that the
implementation is correct.

Rest of the patch looks okay, too.

Reviewed-by: Florian Weimer <[email protected]>

I still believe this needs CVE assignment, for either the TOCTOU race
condition fix or the buffer overflow fix.

Thanks,
Florian