verifying file ownership with dlopen()
in [Unix Programming]
|
From: Ersek, Laszlo on 5 May 2010 12:37 On Tue, 4 May 2010, William Ahern wrote: > #include <stdlib.h> > #include <stdio.h> > > #include <fcntl.h> > > #include <dlfcn.h> > > #include <err.h> > > int main(void) { > char path[256] = "/usr/local/lib/libyaml.so.0.0"; > char name[64] = "yaml_get_version_string"; > int fd; > void *lib; > const char *(*getver)(void); > > if (-1 == (fd = open(path, O_RDONLY))) > err(EXIT_FAILURE, "%s", path); > > snprintf(path, sizeof path, "/dev/fd/%d", fd); > > if (!(lib = dlopen(path, RTLD_NOW))) > errx(EXIT_FAILURE, "%s: %s", path, dlerror()); > > if (!(getver = dlsym(lib, name))) I think it might be better (for some definition of "better") to write this as if (!(*(void **)&getver = dlsym(lib, name))) See [0], [1], and [2]. > errx(EXIT_FAILURE, "%s: %s", name, dlerror()); > > printf("%s\n", getver()); > > return 0; > } > Cheers, lacos [0] http://www.opengroup.org/onlinepubs/9699919799/functions/dlsym.html#tag_16_96_06 [1] http://www.opengroup.org/onlinepubs/9699919799/functions/dlsym.html#tag_16_96_08 [2] http://www.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_12_03
From: Ersek, Laszlo on 5 May 2010 13:01 On Wed, 5 May 2010, Nicolas George wrote: > David Schwartz wrote in message > <f37d8057-9e6b-4e9a-a6fd-4371dadd683e(a)k25g2000prh.googlegroups.com>: >> I think it's likely a better idea to check the ownership and >> permissions of the directory the library is in before calling 'dlopen'. > > He needs to check every path element that leads to the library, because > either one could be a symlink. > > And the check must be aware of the local system's rules for permissions. > For example, just checking the file mode may not be enough if there are > ACLs. Once the full pathname has been tokenized, the way to do that might be something like this. (Note - I didn't even try to compile this. It's here only to invite criticism and/or better ideas.) int safeopen(const char * const * elem, size_t nelem) { if (0u < nelem) { int fd; size_t cur; fd = open("/", O_SEARCH | O_DIRECTORY | O_NOFOLLOW); cur = 0u; loop: if (-1 != fd) { struct stat s; if (0 == fstat(fd, &s)) { if (cur == nelem) { if (S_ISREG(s.st_mode) && /* other checks on the executable */) { return fd; } } else { if (/* checks on the directory */) { int tmp; tmp = fd; fd = openat(fd, elem[cur], O_NOFOLLOW | (cur < nelem - 1u ? O_SEARCH | O_DIRECTORY : O_EXEC)); if (0 == close(tmp)) { ++cur; goto loop; } } } } if (-1 != fd) { (void)close(fd); } } } return -1; } The returned int (if not -1) could be used with fexcve(). Unfortunately (or not), there is no "dlfdopen()". The code could hang indefinitely if the last component identified a FIFO. I'd OR O_NONBLOCK with O_EXEC, if not for O_NONBLOCK being unspecified for regular files. (I can't see anything in the openat() spec that would require O_EXEC to fail on a FIFO (and immediately).) > He also needs to check that the library has no runtime dependency > towards untrusted paths. For example, with modern automagic tools, if > someone compiles with --prefix=/tmp/install and then moves the > installation (he'd rather use DESTDIR, of course), he can get > RPATH=/tmp/install/lib in some of his libraries. > > Even if the library is in a completely trusted path and does not have > structural dependencies towards untrusted paths, it could be a > completely unrelated library unsuited to run in a SUID environment. Some > libraries, for example, parse some specific environment variable in > their _init function. We've been at the same spot in the near past: a process asking the kernel (or whomever) for protection from code it executes. Cheers, lacos
From: William Ahern on 5 May 2010 15:24 Ersek, Laszlo <lacos(a)caesar.elte.hu> wrote: > On Tue, 4 May 2010, William Ahern wrote: <snip> > > if (!(getver = dlsym(lib, name))) > > I think it might be better (for some definition of "better") to write this > as > > if (!(*(void **)&getver = dlsym(lib, name))) Mea culpa. Recent versions of GCC enforce strict aliasing so rigorously that I instinctively refrain from such tricks. Indeed, GCC 4.5 complains at -O2. Being forced to type pun to get a valid lvalue for such common, unobjectionable usage is ridiculous. The standard should make conversions from void pointers to function pointers unspecified so GCC, et al wouldn't have to emit a diagnostic when building in a POSIX environment. But there are no changes along these lines in C1X AFAICT.
From: Geoff Clare on 6 May 2010 08:33 William Ahern wrote: > Ersek, Laszlo <lacos(a)caesar.elte.hu> wrote: >> On Tue, 4 May 2010, William Ahern wrote: > <snip> >> > if (!(getver = dlsym(lib, name))) >> >> I think it might be better (for some definition of "better") to write this >> as >> >> if (!(*(void **)&getver = dlsym(lib, name))) > > Mea culpa. > > Recent versions of GCC enforce strict aliasing so rigorously that I > instinctively refrain from such tricks. Indeed, GCC 4.5 complains at -O2. It is right to complain. That code would not work on an implementation where pointer-to-void and pointer-to-function have different representations. The dlsym() example in POSIX that uses similar code is a bug in POSIX, and is being corrected by this interpretation: http://austingroupbugs.net/view.php?id=74#c205 which (among many other things) changes the relevant line in the example from *(void **)(&fptr) = dlsym(handle, "my_function"); back to fptr = (int (*)(int))dlsym(handle, "my_function"); (which is how it was in SUSv2). This might also produce a warning in some compilers, but implementations are required to accept it and "do the right thing". -- Geoff Clare <netnews(a)gclare.org.uk>
From: William Ahern on 6 May 2010 11:28
Geoff Clare <geoff(a)clare.see-my-signature.invalid> wrote: > William Ahern wrote: > > > Ersek, Laszlo <lacos(a)caesar.elte.hu> wrote: > >> On Tue, 4 May 2010, William Ahern wrote: > > <snip> > >> > if (!(getver = dlsym(lib, name))) > >> > >> I think it might be better (for some definition of "better") to write this > >> as > >> > >> if (!(*(void **)&getver = dlsym(lib, name))) > > > > Mea culpa. > > > > Recent versions of GCC enforce strict aliasing so rigorously that I > > instinctively refrain from such tricks. Indeed, GCC 4.5 complains at -O2. > It is right to complain. That code would not work on an > implementation where pointer-to-void and pointer-to-function have > different representations. Except on POSIX systems they cannot--2.12.3--and this sort of type-punning is the only way to get around the C standard. > The dlsym() example in POSIX that uses similar code is a bug in POSIX, > and is being corrected by this interpretation: > http://austingroupbugs.net/view.php?id=74#c205 > which (among many other things) changes the relevant line in the > example from > *(void **)(&fptr) = dlsym(handle, "my_function"); > back to > fptr = (int (*)(int))dlsym(handle, "my_function"); > (which is how it was in SUSv2). This might also produce a warning > in some compilers, but implementations are required to accept it > and "do the right thing". Required by which standard? All conversions from a void pointer to a function pointer are undefined behavior in C, and all implementations are required to emit a diagnostic, and allowed to bail. If a function pointer is wider than a void pointer, for instance, how does an explicit conversion change anything? That's rhetorical, because it in fact doesn't, as far as the C standard is concerned. The problem with the C standard should be fixed. Even if POSIX/SUSv2 mandates certain behavior for an explicit conversion from void to function pointer, the C standard requires at least a diagnostic, and on GCC and others you can't use the highest level of warning and still build the file. POSIX's use of type-punning, and the 2.12.3 requirement that void pointers and functions pointers have the same representation, are ugly, yet the only way to make a function like dlsym() available to a strictly conforming C program. I don't understand why the Austin Group would entertain returning to the casting form. If the intention is to be able to drop the 2.12.3 requirement, then they've just shot themselves in the foot. |