[PATCH 1/1] elf: Canonicalize $ORIGIN in an explicit ld.so invocation [BZ #25263]

[Adhemerval Zanella Netto]

On 10/03/25 15:19, Geoffrey Thomas wrote:
> On Mon, Mar 3, 2025, at 3:08 PM, Adhemerval Zanella Netto wrote:
>> On 28/02/25 22:27, Adhemerval Zanella Netto wrote:
>>> Sigh, for some reason I did not use git-pw on your patch and applied
>>> directly from mailbox it was malformated with the wrong path for the
>>> procfs. With 'git-pw patch apply 106751' it now seems to work.
> 
> Oh no, is that something I misconfigured/broke on my end? I just used
> git send-email...

No, it is totally on my side.

> 
>> I like your approach better in fact, I was trying to make it generic
>> with realpath and it pulls too many dependencies on Hurd that I would
>> need to make it Linux specific anyway.
> 
> Note that the behavior is different on Hurd so I think we should indeed
> handle this as a Linux-specific thing for now.
> 
> The generic elf/dl-origin.c looks only at $LD_ORIGIN_PATH. This is set
> by the Hurd exec server [1] to the absolute path passed to it. The
> Linux-specific code in sysdeps/unix/sysv/linux/dl-origin.c is the one
> that looks at /proc/self/exe.
> 
> [1] https://git.savannah.gnu.org/cgit/hurd/hurd.git/tree/exec/exec.c?h=v0.9.git20250304#n948
> 
> Hurd does not canonicalize the path in $LD_ORIGIN_PATH at all:
> 
> geofft@hurd:~/one$ cat main.c
> int main(void) {}
> geofft@hurd:~/one$ cc -fPIC -shared -o libfoo.so -lc
> geofft@hurd:~/one$ cc -o main main.c -Wl,--no-as-needed -lfoo -Wl,--as-needed -L. -Wl,-rpath,\$ORIGIN
> geofft@hurd:~/one$ cd ../two
> geofft@hurd:~/two$ ln -s ../one/main
> geofft@hurd:~/two$ ./main
> ./main: error while loading shared libraries: libfoo.so: cannot open
> shared object file: No such file or directory
> 
> So, for consistency, we should not canonicalize the path either. On the
> other hand, as a consequence of $LD_ORIGIN_PATH being set (I think?), we
> have a more serious misbehavior with ldd on Hurd:
> 
> geofft@hurd:~/two$ cd ../one
> geofft@hurd:~/one$ ./main
> geofft@hurd:~/one$ ldd ./main | grep libfoo
>         libfoo.so => not found
> geofft@hurd:~/one$ LD_TRACE_LOADED_OBJECTS=1 ./main | grep libfoo
>         libfoo.so => /home/geofft/one/./libfoo.so (0x0103f000)
> geofft@hurd:~/one$ /lib/ld.so ./main
> ./main: error while loading shared libraries: libfoo.so: cannot open
> shared object file: No such file or directory
> geofft@hurd:~/one$ env | grep LD_ORIGIN_PATH
> LD_ORIGIN_PATH=/usr/bin
> geofft@hurd:~/one$ /lib/ld.so /usr/bin/env | grep LD_ORIGIN_PATH
> LD_ORIGIN_PATH=/lib
> 
> Probably the correct thing to do is to a) have the argument to ld.so
> take precedence over $LD_ORIGIN_PATH and b) canonicalize (ourselves, via
> realpath) $LD_ORIGIN_PATH in all circumstances. But that seems like a
> bigger change. (On the other hand, now that I've installed a Hurd
> VM, maybe you could get me to do it....)
> 
> This seems to have been reported previously at
> https://sourceware.org/bugzilla/show_bug.cgi?id=24260

Hurd setting LD_ORIGIN_PATH on the exec server seems like a hack solution
imho, but maybe this the cleanest way on Hurd to have this information
passed to the loader.  I though about check if we can remove this from
Linux, but it seems that there is the corner case where if procfs is
not accessible, LD_ORIGIN_PATH is the only way to get a binary with
$ORIGIN to work correctly in all cases.

And I think your suggestion to fix on Hurd seems correct, although to
pull realpath on the loader it would require a lot of changes.

> 
>> However I think we should make the _dl_canonicalize simpler and not
>> mess with the input argument since all the logic in the caller 
>> (_dl_map_object_from_fd).  Also, limit the maximum path to PATH_MAX
>> (and not PATH_MAX + 1) similar to _dl_get_origin and optimize the
>> memory allocation a bit with __strdup:
>>
>> char *
>> _dl_canonicalize (int fd)
>> {
>>   struct fd_to_filename fdfilename;
>>   char canonical[PATH_MAX];
>>   char *path = __fd_to_filename (fd, &fdfilename);
>>   int size = INTERNAL_SYSCALL_CALL (readlinkat, AT_FDCWD, path,
>>                                     canonical, PATH_MAX - 1);
>>   if (size >= 0)
>>     {
>>       canonical[size] = '\0';
>>       return __strdup (canonical);
>>     }
>>   return NULL;
>> }
>>
>> I added this suggestion along with a testcase [1], what do you
>> think?
>>
>> [1] 
>> https://sourceware.org/git/?p=glibc.git;a=commit;h=372c632ce7c78471dbda69ca33625d1ecb6fb2f7
> 
> On rereading the readlink man page, I think we should fail to
> canonicalize if the return value from readlink is equal to the passed
> buffer size, because then we don't know if the path was truncated or
> not, and I am worried about a case where the path is truncated and an
> attacker controls the truncated path name. Specifically, I think just
> changing to
> 
>   if (size >= 0 && size < PATH_MAX - 1)
> 
> would do the trick. (I do see a reference to PATH_MAX in the kernel
> implementation of readlink on /proc/$pid/fd, but I'm not sure if that's
> an ABI promise or if the kernel can return longer results in the future.)

In fact I had something similar while I was working (not sure why I
removed it) and it does make sense to check for truncation.  

And for PATH_MAX on readlink on /proc/$pid/fd, I think it is because it 
would depend of the underlying filesystem support where the binary is
being placed.  So I think PATH_MAX is the minimum denominator here and 
it should be safe to assume that kernel always support it.

> 
> I like the change to return a pointer and not mess with calling free()
> in the subroutine, and thanks for mentioning __fd_to_filename. The test
> looks good to me other than that I think the commented non-ldd test
> ought to be uncommented - I think both cases are expected to work (on
> Linux) and the test should fail if either case fails. Thanks for the
> reworking and the review! Feel free to add my signoff.
> 

Oops, I will fix it.  I will send a new version with these fixed and
if you can add your reviewed-by I will install it.

Thanks for working on this.