[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20190428162852.GV23599@brightrain.aerifal.cx>
Date: Sun, 28 Apr 2019 12:28:52 -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 Sun, Apr 28, 2019 at 01:07:14PM +0200, Szabolcs Nagy wrote:
> * Rodger Combs <rodger.combs@...il.com> [2019-04-27 19:16:30 -0500]:
> > > On Apr 27, 2019, at 18:55, Szabolcs Nagy <nsz@...t70.net> wrote:
> > > * Rodger Combs <rodger.combs@...il.com> [2019-04-27 17:51:17 -0500]:
> > >> On Apr 27, 2019, at 12:19, Rich Felker <dalias@...c.org> wrote:
> > >>> On Fri, Apr 26, 2019 at 08:13:29PM -0500, Rodger Combs wrote:
> > >>>> + 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.
> > >>
> > >> No more so than dynlink.c normally has when loading other SOs. Like there, I don't follow $ORIGIN in secure mode, and additionally here I don't handle relative-to-cwd paths in secure mode. I don't see a problem with allowing a load from an absolute rpath, or from the hardcoded path, using this mechanism, though.
> > >> Basically, I'm intending for this to be a feature that you could just turn on in your linker flags for everything you build, and get the functionality in the cases where you want it, at no significant cost in those where you don't.
> > >
> > > i think the code should be written such that it is obvious
> > > that user input cannot affect runtime behaviour in secure
> > > mode in any way (in particular the loaded code).
> >
> > This is the case (CWD, the executable path, and env vars are all ignored in secure mode); if there's something you'd like changed to make that more clear, please elaborate.
>
> the current code does not *obviously* have the right
> security properties (it does not even document the
> properties it guarantees).
>
> if an auditor has to read complex code like find_linker
> to verify important security properties then it is not
> obviously secure.
>
> the original musl ldso code is already fairly complicated
> and you created a dcrt1 that has more state and branching
> around user input.
Right, esp. re: user input. Also note that a bug here in dcrt1 is
*unfixable* in deployed binaries (without binary-level hacking)
because it's linked into every one of them. This is contrary to the
expectation of dynamic linking that bugs in library code are isolated
to the corresponding libraries and can be fixed by upgrading the
affected library only.
> i suggest refactoring at least find_linker (e.g. into a
> secure and a non-secure version, but there might be ways
> with less code duplication) and documenting assumptions
> about the secure paths (e.g. not user writable).
I really think we should just refrain from trying to support suid with
this at all. The number of binaries that "should" be suid can be
counted on one hand (one could even argue that it's a nice round
number), and having suid fail when linked DNI is probably a "feature"
in that it would catch stuff trying to install suid binaries
unexpectedly and make you fix it.
>
> > >>>> + // 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...
> > >>
> > >> Yeah, the linker doesn't add PT_PHDR when we tell it not to set a dynamic loader, and dynlink needs it.
> > >
> > > there should be a strong reason to add fake program headers.
> > > why is PT_PHDR required?
> > > who uses PT_INTERP?
> >
> > PT_PHDR is needed for the dynamic loader to find the executable's base address.
> > PT_INTERP isn't currently used by musl, but it is in glibc (to find its own path, so it knows where it's loaded from for future dlopen()s and such, and potentially for debugging?), and it seems reasonable that the linker might care about it in the future, so I'm including it for potential forwards-compatibility (and also glibc compatibility), since we already need to create an entry for PHDR anyway, so it's trivial to do this as well.
>
> i think base address can be found without PT_PHDR if
> there is a dynamic section
I don't think so. When the dynamic linker entry point is reached, the
*only* references is has to the main program it's supposed to execute
are AT_PHDR and AT_ENTRY. In order to find the load base of the main
program, it needs to be able to compare the mapped and ELF-vaddr
addresses for a single point in the program. Without knowing the load
base, you can't even find the main program's _DYNAMIC. The only single
point I know that you can do the comparison on is AT_PHDR vs PT_PHDR.
> or with a new api between
> the ldso and loader of the ldso (i'd only try the
> fake phdr mapping if other options are explored and
> turn out to be worse).
A new API between them is invention of a new interface boundary, which
is a very bad thing in itself, plus it precludes running DNI
executables with any existing/older ldso; they'd only work with new
ones that speak the new interface.
Ideally ld could/should be taught that "wasting" 9 words for an
"unnecessary" phdr is not a problem and that it should always emit
PT_PHDR. If this is something that might eventually get fixed in ld,
the rewriting could be made conditional on whether PT_PHDR was
initially absent; if we go that path, omitting PT_INTERP might be
desirable so that the behavior is unchanged when rewriting is not
necessary.
Rich
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.
