[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sat, 27 Apr 2019 13:19:07 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 3/3] crt: add dcrt1, with support for locating the
dynamic loader at runtime
On Fri, Apr 26, 2019 at 08:13:29PM -0500, Rodger Combs wrote:
> diff --git a/crt/dcrt1.c b/crt/dcrt1.c
> new file mode 100644
> index 0000000..47c6dc2
> --- /dev/null
> +++ b/crt/dcrt1.c
> @@ -0,0 +1,362 @@
> +#define SYSCALL_NO_TLS
> +
> +#include <elf.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <features.h>
> +#include <libgen.h>
> +#include <sys/mman.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include "atomic.h"
> +#include "dynlink.h"
> +#include "syscall.h"
> +
> +extern weak hidden const size_t _DYNAMIC[];
> +
> +int main();
> +weak void _init();
> +weak void _fini();
> +weak _Noreturn int __libc_start_main(int (*)(), int, char **,
> + void (*)(), void(*)(), void(*)());
> +
> +#define DLSTART_PROLOGUE __libc_start_main(main, argc, argv, _init, _fini, 0);
> +
> +#define START "_start"
> +#define _dlstart_c _start_c
> +#include "../ldso/dlstart.c"
> +
> +struct dso {
> + unsigned char *base;
> + struct fdpic_loadmap *loadmap;
> + size_t *dynv;
> + Phdr *phdr;
> + int phnum;
> + size_t phentsize;
> + unsigned char *map;
> + size_t map_len;
> +};
There is no need for making a dummy strict dso or exposing struct dso
to this file. The code to be taken/shared from map_library just needs
to be refactored so that the part that writes things into struct dso
remains in dynlink.c. Or, since we're punting on fdpic support for
this for now, a simplified version of map_library without fdpic
support (it gets quite simple then) could perhaps just be put here for
now, with the intent of refactoring to unify later.
> +#ifndef PAGE_SIZE
> +#define PAGE_SIZE SYSCALL_MMAP2_UNIT
> +#endif
> [...]
> +static inline int crt_mprotect(void *addr, size_t len, int prot)
> +{
> + size_t start, end;
> + start = (size_t)addr & -PAGE_SIZE;
> + end = (size_t)((char *)addr + len + PAGE_SIZE-1) & -PAGE_SIZE;
> + return __syscall(SYS_mprotect, start, end-start, prot);
> +}
> +#define mprotect crt_mprotect
This is wrong. mprotect does not take units of SYSCALL_MMAP2_UNIT. You
need to get the variable page size from auxv.
> +static char *crt_getenv(const char *name, char **environ)
> +{
> + size_t l = crt_strchrnul(name, '=') - name;
> + if (l && !name[l] && environ)
> + for (char **e = environ; *e; e++)
> + if (!crt_strncmp(name, *e, l) && l[*e] == '=')
> + return *e + l+1;
> + return 0;
> +}
I really question the use of environment here at all, both from a
standpoint of simplicity/least-surprise, and from a standpoint of
intended usage case. If you're on a non-musl system, the environment
is likely to contain settings that wouldn't be appropriate to musl
anyway, meaing you probably need to remove them for this to work at
all, and then if you really need to force a path other than the
builtin relative one for a particular musl-linked binary, you might as
well just invoke ldso as a command, making the override local to that
invocation rather than inherited across exec of other programs.
If you have compelling reasons you want to search the environment,
let's discuss it, but if it's just gratuitous "maybe it will be
useful", let's apply YAGNI.
> +static void get_rpaths(const char **rpath, const char **runpath, const size_t *dyn)
> +{
> + /* DT_STRTAB is pre-relocated for us by dlstart */
> + const char *strings = (void*)dyn[DT_STRTAB];
> +
> + if (dyn[0] & (1 << DT_RPATH))
> + *rpath = strings + dyn[DT_RPATH];
> + if (dyn[0] & (1 << DT_RUNPATH))
> + *runpath = strings + dyn[DT_RUNPATH];
> +}
musl does not honor the legacy difference in search order between
rpath and runpath (see dynlink.c), but if wwe don't search via
environment here it doesn't matter anyway.
> +static size_t find_linker(char *outbuf, size_t bufsize, const char *this_path, size_t thisl, const size_t *dyn, char **environ, int secure)
> +{
> + const char *paths[3] = {0}; // rpath, envpath, runpath
> + size_t i;
> + int fd;
> +
> + if (thisl)
> + thisl--;
> + while (thisl > 1 && this_path[thisl] == '/')
> + thisl--;
> + while (thisl > 0 && this_path[thisl] != '/')
> + thisl--;
> +
> + if (!secure) {
> + const char *envpath = crt_getenv("LD_LOADER_PATH", environ);
> + if (envpath) {
> + size_t envlen = crt_strlen(envpath);
> + if (envlen < bufsize) {
> + crt_memcpy(outbuf, envpath, envlen + 1);
> + return envlen + 1;
> + }
> + }
> + }
> +
> + get_rpaths(&paths[0], &paths[2], dyn);
> +
> + paths[1] = secure ? NULL : crt_getenv("LD_LIBRARY_PATH", environ);
> +
> + for (i = 0; i < 3; i++) {
> + int relative = 0;
> + const char *p = paths[i];
> + char *o = outbuf;
> + if (!p)
> + continue;
> + for (;;) {
> + if (!crt_strncmp(p, "$ORIGIN", 7) ||
> + !crt_strncmp(p, "${ORIGIN}", 9)) {
> + relative = 1;
> + if (o + thisl + 1 < outbuf + bufsize) {
> + crt_memcpy(o, this_path, thisl);
> + o += thisl;
> + } else {
> + o = outbuf + bufsize - 1;
> + }
> + p += (p[1] == '{' ? 9 : 7);
> + } else if (*p == ':' || !*p) {
> +#define LDSO_FILENAME "ld-musl-" LDSO_ARCH ".so.1"
> + relative |= outbuf[0] != '/';
> + if ((!secure || !relative) && o + sizeof(LDSO_FILENAME) + 1 < outbuf + bufsize) {
> + *o++ = '/';
> + crt_memcpy(o, LDSO_FILENAME, sizeof(LDSO_FILENAME));
> + if (!access(outbuf, R_OK | X_OK))
> + return (o + sizeof(LDSO_FILENAME)) - outbuf;
> + }
> + if (!*p)
> + break;
> + relative = 0;
> + o = outbuf;
> + p++;
> + } else {
> + if (o < outbuf + bufsize)
> + *o++ = *p;
> + p++;
> + }
> + }
> + }
> +
> + // Didn't find a usable loader anywhere, so try the hardcoded path :shrug:
> + crt_memcpy(outbuf, LDSO_PATHNAME, sizeof(LDSO_PATHNAME));
> + return sizeof(LDSO_PATHNAME);
> +}
Including fallbacks that alter behavior is probably a bad idea
(failing preferable). If the changes in your other patch to dynlink.c
are needed to make it work with dcrt1, then it won't necessarily even
work to load a (possibly older) system-wide musl ldso; however if
that's the case I Think we should fix it.
> +static void final_start_c(long *p)
> +{
> + int argc = p[0];
> + char **argv = (void *)(p+1);
> + __libc_start_main(main, argc, argv, _init, _fini, 0);
> +}
> +
> +hidden _Noreturn void __dls2(unsigned char *base, size_t *p, size_t *dyn)
> +{
> + int argc = p[0];
> + char **argv = (void *)(p+1);
> + int fd;
> + int secure;
> + int prot = PROT_READ;
> + struct dso loader;
> + Ehdr *loader_hdr;
> + Phdr *new_hdr;
> + void *entry;
> + char this_path[2*NAME_MAX+2] = {0};
> + size_t thisl;
> + char linker_path[2*NAME_MAX+2] = {0};
2*NAME_MAX is a fairly short arbitrary limit. Why not PATH_MAX, or a
VLA that adapts up to PATH_MAX (to avoid expanding stack when longer
is not needed)?
> + size_t linker_len;
> + size_t i;
> + size_t aux[AUX_CNT];
> + size_t *auxv;
> + char **environ = argv + argc + 1;
> +
> + /* Find aux vector just past environ[] and use it to initialize
> + * global data that may be needed before we can make syscalls. */
> + for (i = argc + 1; argv[i]; i++);
> + auxv = (void *)(argv + i + 1);
> + decode_vec(auxv, aux, AUX_CNT);
> + secure = ((aux[0] & 0x7800) != 0x7800 || aux[AT_UID] != aux[AT_EUID]
> + || aux[AT_GID] != aux[AT_EGID] || aux[AT_SECURE]);
At this point we can just abort if secure != 0. There is unbounded
attack surface trying to load a (possibly relative) ldso with elevated
privileges.
> + thisl = readlink("/proc/self/exe", this_path, sizeof this_path);
We might consider whether use of AT_EXECFN (possibly with additional
resolution of symlinks etc) would be better. It would avoid dependency
on /proc, making it possible to run dcrt1 binaries at early boot or
inside chroots, but does have different semantics that might be less
(or maybe more?) desirable.
> + // Copy the program headers into an anonymous mapping
> + new_hdr = mmap(0, (aux[AT_PHENT] * (aux[AT_PHNUM] + 2) + linker_len + PAGE_SIZE - 1) & -PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> + if (map_library_failed(new_hdr))
> + goto error;
Can you remind us why patched program headers are needed? I think it
was absence of PT_PHDR or something...
> + // Point it back at the original kernel-provided base
> + new_hdr->p_type = PT_PHDR;
> + new_hdr->p_vaddr = (size_t)new_hdr - (size_t)base;
> +
> + ((Phdr*)((char*)new_hdr + aux[AT_PHENT]))->p_type = PT_INTERP;
> + ((Phdr*)((char*)new_hdr + aux[AT_PHENT]))->p_vaddr = new_hdr->p_vaddr + aux[AT_PHENT] * (aux[AT_PHNUM] + 2);
But here you're also adding a PT_INTERP. What's the motivation for that?
> + crt_memcpy((char*)new_hdr + aux[AT_PHENT] * (aux[AT_PHNUM] + 2), linker_path, linker_len);
> +
> + for (i = 0; i < aux[AT_PHNUM]; i++) {
> + Phdr *hdr = (void*)((char*)aux[AT_PHDR] + aux[AT_PHENT] * i);
> + Phdr *dst = (void*)((char*)new_hdr + aux[AT_PHENT] * (i + 2));
> + if (hdr->p_type == PT_PHDR || hdr->p_type == PT_INTERP) {
Isn't it impossible to have a PT_INTERP already here?
> + // Can't have a duplicate
> + dst->p_type = PT_NULL;
Is adding PT_NULL headers in the middle valid, or do they get
interpreted as end of headers?
> + } else {
> + crt_memcpy(dst, hdr, aux[AT_PHENT]);
> + }
> + }
> +
> + if (mprotect(new_hdr, aux[AT_PHENT] * (aux[AT_PHNUM] + 2) + linker_len, PROT_READ))
> + goto error;
> +
> + for (i=0; auxv[i]; i+=2) {
> + if (auxv[i] == AT_BASE)
> + auxv[i + 1] = (size_t)loader_hdr;
> + if (auxv[i] == AT_PHDR)
> + auxv[i + 1] = (size_t)new_hdr;
> + if (auxv[i] == AT_PHNUM)
> + auxv[i + 1] += 2;
> + }
> +
> + entry = laddr(&loader, loader_hdr->e_entry);
> +
> +#ifndef LD_FDPIC
> + /* Undo the relocations performed by dlstart */
> +
> + if (NEED_MIPS_GOT_RELOCS) {
> + const size_t *dynv = _DYNAMIC;
> + size_t local_cnt = 0;
> + size_t *got = (void *)(base + dyn[DT_PLTGOT]);
> + for (i=0; dynv[i]; i+=2) if (dynv[i]==DT_MIPS_LOCAL_GOTNO)
> + local_cnt = dynv[i+1];
> + for (i=0; i<local_cnt; i++) got[i] -= (size_t)base;
> + }
> +
> + size_t *rel = (void *)((size_t)base+dyn[DT_REL]);
> + size_t rel_size = dyn[DT_RELSZ];
> + for (; rel_size; rel+=2, rel_size-=2*sizeof(size_t)) {
> + if (!IS_RELATIVE(rel[1], 0)) continue;
> + size_t *rel_addr = (void *)((size_t)base + rel[0]);
> + *rel_addr -= (size_t)base;
> + }
> +#endif
> +
> + CRTJMP(entry, argv - 1);
> +
> +error:
> + for(;;) a_crash();
> +}
> diff --git a/ldso/dlstart.c b/ldso/dlstart.c
> index 20d50f2..7ff79d6 100644
> --- a/ldso/dlstart.c
> +++ b/ldso/dlstart.c
> @@ -21,7 +21,7 @@
> hidden void _dlstart_c(size_t *sp, size_t *dynv)
> {
> size_t i, aux[AUX_CNT], dyn[DYN_CNT];
> - size_t *rel, rel_size, base;
> + size_t *rel, rel_size, base, loader_phdr;
>
> int argc = *sp;
> char **argv = (void *)(sp+1);
> @@ -41,6 +41,13 @@ hidden void _dlstart_c(size_t *sp, size_t *dynv)
> * space and moving the extra fdpic arguments to the stack
> * vector where they are easily accessible from C. */
> segs = ((struct fdpic_loadmap *)(sp[-1] ? sp[-1] : sp[-2]))->segs;
> + if (aux[AT_BASE]) {
> + Ehdr *eh = (void*)aux[AT_BASE];
> + for (i = 0; eh->e_phoff - segs[i].p_vaddr >= segs[i].p_memsz; i++);
> + loader_phdr = (eh->e_phoff - segs[i].p_vaddr + segs[i].addr);
> + } else {
> + loader_phdr = aux[AT_PHDR];
> + }
> } else {
> /* If dynv is null, the entry point was started from loader
> * that is not fdpic-aware. We can assume normal fixed-
> @@ -55,6 +62,7 @@ hidden void _dlstart_c(size_t *sp, size_t *dynv)
> segs[0].p_memsz = -1;
> Ehdr *eh = (void *)base;
> Phdr *ph = (void *)(base + eh->e_phoff);
> + loader_phdr = (size_t)ph;
> size_t phnum = eh->e_phnum;
> size_t phent = eh->e_phentsize;
> while (phnum-- && ph->p_type != PT_DYNAMIC)
> @@ -69,13 +77,38 @@ hidden void _dlstart_c(size_t *sp, size_t *dynv)
>
> #if DL_FDPIC
> for (i=0; i<DYN_CNT; i++) {
> - if (i==DT_RELASZ || i==DT_RELSZ) continue;
> + if (i==DT_RELASZ || i==DT_RELSZ || i==DT_RPATH || i==DT_RUNPATH) continue;
> if (!dyn[i]) continue;
> for (j=0; dyn[i]-segs[j].p_vaddr >= segs[j].p_memsz; j++);
> dyn[i] += segs[j].addr - segs[j].p_vaddr;
> }
> base = 0;
> +#else
> + /* If the dynamic linker is invoked as a command, its load
> + * address is not available in the aux vector. Instead, compute
> + * the load address as the difference between &_DYNAMIC and the
> + * virtual address in the PT_DYNAMIC program header. */
> + base = aux[AT_BASE];
> + if (!base) {
> + size_t phnum = aux[AT_PHNUM];
> + size_t phentsize = aux[AT_PHENT];
> + Phdr *ph = (void *)aux[AT_PHDR];
> + for (i=phnum; i--; ph = (void *)((char *)ph + phentsize)) {
> + if (ph->p_type == PT_DYNAMIC) {
> + base = (size_t)dynv - ph->p_vaddr;
> + break;
> + }
> + }
> + }
> + loader_phdr = base + ((Ehdr*)base)->e_phoff;
> +#endif
>
> +#ifdef DLSTART_PROLOGUE
> + if (aux[AT_PHDR] != loader_phdr)
> + DLSTART_PROLOGUE
> +#endif
As mentioned before, this does not work. It puts a symbol reference
into a function that is required to be pure PIC that can run prior to
relocations. Having the reference in a branch that's not taken does
not help.
Instead it should be something like (pseudocode):
if (relocs_already_done) goto done:
// ...
done:
stage2_func dls2;
GETFUNCSYM(&dls2, __dls2, base+dyn[DT_PLTGOT]);
dls2((void *)base, sp);
This does not require any DLSTART_PROLOGUE macro that's specific to
dcrt1; the exact same thing works in rcrt1 (static pie) and will also
make it safe to invoke static pie programs via ldso (seems pointless,
but useful when you don't realize the binary is static).
The dcrt1 code in dls2 (which can vary between ldso, rcrt1, and dcrt1)
then has to do its own test to tell which code path it needs to take,
but that's fine. General principle here: repeating small amounts of
work is better than increasing dependency between stages/layers. Same
applies to passing dyn into dls2 -- doing so prevents the call from
being a tail call, and keeps _dlstart_c's stack frame around despite
being done with it.
> + /* For convenience in dcrt1, we relocate STRTAB here (as with FDPIC) */
> + dyn[DT_STRTAB] += base;
Just do it where you need it.
> /* MIPS uses an ugly packed form for GOT relocations. Since we
> * can't make function calls yet and the code is tiny anyway,
> @@ -144,5 +163,5 @@ hidden void _dlstart_c(size_t *sp, size_t *dynv)
>
> stage2_func dls2;
> GETFUNCSYM(&dls2, __dls2, base+dyn[DT_PLTGOT]);
> - dls2((void *)base, sp);
> + dls2((void *)base, sp, dyn);
See above.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
