Fix null-pointer dereference in rtld

[theraven]

Summary

As reported in PR253081, when a library is opened via fdlopen, it has a null pointer for its path and so _rtld_bind can crash as a result of passing the null pointer to basename (which passes it to strrchr, which doesn't do a null check).

I've been carrying a local patch for this for a while. It affects capsicum programs, possibly only ones that open libraries indirectly via LD_LIBRARY_PATH_FDS. In my case, I am opening a library via fdlopen, which depends on some libraries that are opened via directory descriptors inherited from the parent and passed in with LD_LIBRARY_PATH_FDS. It may not happen with just the single fdlopen call (it appears that it's the basename(defobject->path) call that's the problem, not the basename(obj->path) It may be that the correct fix is to initialise the path field, but nothing apart from this debug line appears to worry if it is null.

[kib]

I suspect that PATH_FDS is simply not tested enough if such issue popped up. For instance some combination of rpath in the loaded library and unsuccessful load from pathfds could make rtld to try to use refobj path.

So IMO it would be more useful to put some safety plug into obj->path when opened with fdlopen, perhaps nothing fancy but a pointer to malloced string e.g. "/unknown".

libexec/rtld-elf/rtld.c

881

obj->path != NULL according to style

[theraven]

Thanks. I can confirm that this change also fixes this problem:

diff
diff --git a/libexec/rtld-elf/rtld.c b/libexec/rtld-elf/rtld.c
index 73177fc0c931..60f453a89478 100644
--- a/libexec/rtld-elf/rtld.c
+++ b/libexec/rtld-elf/rtld.c
@@ -3377,7 +3377,7 @@ void *
 fdlopen(int fd, int mode)
 {

-       return (rtld_dlopen(NULL, fd, mode));
+       return (rtld_dlopen("/unknown", fd, mode));
 }

 static void *

I did not malloc the string here because the string argument from dlopen is passed in the same way and so I presume that there is a copy somewhere. It might be nice to make this a #{fd number} string for consistency, what do you think?

[kib]

Yes it should be strdup'ed somewhere but I am surprised that it works. Look at the start of load_object(): if name != NULL, it searches for existing loaded object with the specified name. I believe that the right patch would set path somewhere in load_object() in the 'then' case for fd >= 0 (see below).

Instead of fd number I suggest to encode (dev, ino) or something similar. fd is very well can become invalid or reopened so it is not very useful thing to put into object description/path. load_object() conveniently does fstat() on fd, but you need to plug uses of path before that moment.

[theraven]

Yes it should be strdup'ed somewhere but I am surprised that it works. Look at the start of load_object(): if name != NULL, it searches for existing loaded object with the specified name.

My failing program only fdlopens one thing, so wouldn't hit that failure case (things that are loaded indirectly wouldn't be affected). Though my change did manage to break sshd's ability to load PAM modules, which was somewhat embarrassing, but showed exactly the failure mode that you expected (multiple libraries were opened with the same name).

I believe that the right patch would set path somewhere in load_object() in the 'then' case for fd >= 0 (see below).

Instead of fd number I suggest to encode (dev, ino) or something similar. fd is very well can become invalid or reopened so it is not very useful thing to put into object description/path. load_object() conveniently does fstat() on fd, but you need to plug uses of path before that moment.

I have tried setting path in load_object in the proposed code path (and also unconditionally if it is null), but I hit my original failure in _rtld_bind. This debugging message appears to be the only thing that dereferences ->path without checking if it is null.

[kib]

Ok, go ahead with the proposed patch, I do not think it is worth the time to try to make it more advanced now.

[theraven]

Thanks, I'll see if I can chase this down later. My commit bit lapsed, please can you land it?