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

On 31/08/26 10:54, Florian Weimer wrote:
> * 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.

Ack, you suggestion sounds good.
> 
>> +
>> +  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, '/');
> 
> ?

Yes, but it would require pulling strchrnul in the loader. Maybe now it is used
in more than one place, we can consolidate it along with other usages (like
_dl_hwcaps_split, which contains this very justification).

I will send a patch for that.

> 
>> +
>> +      /* 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.

The problem here is backward scan must stop at limit, not the buffer start.

> 
>> +	      if (wnp > pstart)
>> +		--wnp;
>> +	    }
>> +	  else if (pstart == path)
>> +	    {
> 
> “pstart == path” checks for a relative orignal path.  Maybe mention
> this here?

Right, I changed to:

             /* No component is left and the original path is relative,
                keep the unresolvable ".." (it becomes part of the
                preserved prefix).  */

> 
>> +	      /* 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.

The destination may overlap the source, for e.g. "//abc" they partially
overlap (3 bytes from path+2 to path+1). We can use memmove here, ld.so
already pulls it.

> 
>> +
>> +      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.

Sounds reasonable, I will send the strchrnul and memmove changes as a
subsequent patch.

> 
> 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.
Ok, I will check with Carlos and Siddhesh about it.