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

[Adhemerval Zanella Netto]

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

Indeed this approach is not fully correct, and I will move it to a different
header and add an internal testcase.

> 
> 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.
Indeed expand_dynamic_string_token uses buffer without a NULL terminator,
and it does seems a security issue. I will reserve a CVE for this one and 
user for a v2.