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

[Geoffrey Thomas]

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

> 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

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

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.

-- 
Geoffrey Thomas
geofft@ldpreload.com