mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] $ORIGIN rpath expansion without /proc: code looks wrong
@ 2021-11-17 14:04 Alexander Sosedkin
  2021-11-17 17:00 ` Érico Nogueira
  0 siblings, 1 reply; 9+ messages in thread
From: Alexander Sosedkin @ 2021-11-17 14:04 UTC (permalink / raw)
  To: musl

Hello, I've encountered a case of a binary with an rpath of
/some/meaningful/lib:$ORIGIN/../lib
not starting up due to not finding /some/meaningful/lib/libxyz.so.
ldd'ing said it's there though.
And the library was found alright when I patchelf'd rpath to just
/some/meaningful/lib

I dug into musl code and came across that bit that checks /proc.
Sure enough, when I tried mounting /proc, it started working fine.
Yet the error handling from accessing /proc puzzles me:

ldso/dynlink.c, fixup_rpath():
                 l = readlink("/proc/self/exe", buf, buf_size);
                 if (l == -1) switch (errno) {
                 case ENOENT:
                 case ENOTDIR:
                 case EACCES:
                         break;
                 default:
                         return -1;
                 }
                 if (l >= buf_size)
                         return 0;
                 buf[l] = 0;
                 origin = buf;

hitting that break like I had means zeroing buf[-1], right?
Could somebody take a look at this and double-check that
this codepath makes sense?
My attempts at comprehending it fail irrecoverably at this line.

(CC me on replies, please.
  No nice context to provide, building my own toolchain at
  https://github.com/t184256/bootstrap-from-tcc)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] $ORIGIN rpath expansion without /proc: code looks wrong
  2021-11-17 14:04 [musl] $ORIGIN rpath expansion without /proc: code looks wrong Alexander Sosedkin
@ 2021-11-17 17:00 ` Érico Nogueira
  2021-11-17 19:25   ` Alexander Sosedkin
  2021-11-17 20:01   ` Jeffrey Walton
  0 siblings, 2 replies; 9+ messages in thread
From: Érico Nogueira @ 2021-11-17 17:00 UTC (permalink / raw)
  To: musl; +Cc: monk

On Wed Nov 17, 2021 at 11:04 AM -03, Alexander Sosedkin wrote:
> Hello, I've encountered a case of a binary with an rpath of
> /some/meaningful/lib:$ORIGIN/../lib
> not starting up due to not finding /some/meaningful/lib/libxyz.so.
> ldd'ing said it's there though.
> And the library was found alright when I patchelf'd rpath to just
> /some/meaningful/lib
>
> I dug into musl code and came across that bit that checks /proc.
> Sure enough, when I tried mounting /proc, it started working fine.
> Yet the error handling from accessing /proc puzzles me:
>
> ldso/dynlink.c, fixup_rpath():
> l = readlink("/proc/self/exe", buf, buf_size);
> if (l == -1) switch (errno) {
> case ENOENT:
> case ENOTDIR:
> case EACCES:
> break;
> default:
> return -1;
> }
> if (l >= buf_size)
> return 0;
> buf[l] = 0;
> origin = buf;
>
> hitting that break like I had means zeroing buf[-1], right?

No. Because `l` is size_t (unsigned long), it's the biggest possible
value for size_t, and `l >= buf_size` will be true, which means the
function returns 0. This conditional also catches the case where
truncation happens in readlink(3).

Documenting this in a comment or changing `break;` for `return 0;` might
make sense, though.

> Could somebody take a look at this and double-check that
> this codepath makes sense?

It does, but it might not be as robust as you wish. fixup_rpath() treats
the RPATH entry as a single string, and does all $ORIGIN substitutions
in one go (what splits the string by ":" is open_path()). This means
that the entire RPATH entry containing $ORIGIN will be ignored if
/proc/self/exe can't be accessed, despite one or more of them not
depending on $ORIGIN.

> My attempts at comprehending it fail irrecoverably at this line.
>
> (CC me on replies, please.
> No nice context to provide, building my own toolchain at
> https://github.com/t184256/bootstrap-from-tcc)


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] $ORIGIN rpath expansion without /proc: code looks wrong
  2021-11-17 17:00 ` Érico Nogueira
@ 2021-11-17 19:25   ` Alexander Sosedkin
  2021-11-18 19:15     ` Érico Nogueira
  2021-11-17 20:01   ` Jeffrey Walton
  1 sibling, 1 reply; 9+ messages in thread
From: Alexander Sosedkin @ 2021-11-17 19:25 UTC (permalink / raw)
  To: Érico Nogueira, musl

On 11/17/21 18:00, Érico Nogueira wrote:
> On Wed Nov 17, 2021 at 11:04 AM -03, Alexander Sosedkin wrote:
>> Hello, I've encountered a case of a binary with an rpath of
>> /some/meaningful/lib:$ORIGIN/../lib
>> not starting up due to not finding /some/meaningful/lib/libxyz.so.
>> ldd'ing said it's there though.
>> And the library was found alright when I patchelf'd rpath to just
>> /some/meaningful/lib
>>
>> I dug into musl code and came across that bit that checks /proc.
>> Sure enough, when I tried mounting /proc, it started working fine.
>> Yet the error handling from accessing /proc puzzles me:
>>
>> ldso/dynlink.c, fixup_rpath():
>> l = readlink("/proc/self/exe", buf, buf_size);
>> if (l == -1) switch (errno) {
>> case ENOENT:
>> case ENOTDIR:
>> case EACCES:
>> break;
>> default:
>> return -1;
>> }
>> if (l >= buf_size)
>> return 0;
>> buf[l] = 0;
>> origin = buf;
>>
>> hitting that break like I had means zeroing buf[-1], right?
> 
> No. Because `l` is size_t (unsigned long), it's the biggest possible
> value for size_t, and `l >= buf_size` will be true,

Oh! Thanks a lot, that's what confused me. Sorry for the noise then.

> which means the
> function returns 0. This conditional also catches the case where
> truncation happens in readlink(3).
> Documenting this in a comment or changing `break;` for `return 0;` might
> make sense, though.

Yeah, I'd say a `return 0;` there would've been easier to comprehend.
I don't think there's much need for a comment...

>> Could somebody take a look at this and double-check that
>> this codepath makes sense?
> 
> It does, but it might not be as robust as you wish. fixup_rpath() treats
> the RPATH entry as a single string, and does all $ORIGIN substitutions
> in one go (what splits the string by ":" is open_path()). This means
> that the entire RPATH entry containing $ORIGIN will be ignored if
> /proc/self/exe can't be accessed, despite one or more of them not
> depending on $ORIGIN.

... because if running /proc-less isn't supported in general,
then such separate expansions are probably not worth the effort.
And it's far from being the only mention of /proc in the code,
so I'm going to presume it's not.

>> My attempts at comprehending it fail irrecoverably at this line.
>>
>> (CC me on replies, please.
>> No nice context to provide, building my own toolchain at
>> https://github.com/t184256/bootstrap-from-tcc)

Thanks for the answer, appreciated.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] $ORIGIN rpath expansion without /proc: code looks wrong
  2021-11-17 17:00 ` Érico Nogueira
  2021-11-17 19:25   ` Alexander Sosedkin
@ 2021-11-17 20:01   ` Jeffrey Walton
  2021-11-18 19:21     ` Érico Nogueira
  1 sibling, 1 reply; 9+ messages in thread
From: Jeffrey Walton @ 2021-11-17 20:01 UTC (permalink / raw)
  To: musl; +Cc: monk

On Wed, Nov 17, 2021 at 12:09 PM Érico Nogueira <ericonr@disroot.org> wrote:
>
> On Wed Nov 17, 2021 at 11:04 AM -03, Alexander Sosedkin wrote:
> > ...
> > Could somebody take a look at this and double-check that
> > this codepath makes sense?
>
> It does, but it might not be as robust as you wish. fixup_rpath() treats
> the RPATH entry as a single string, and does all $ORIGIN substitutions
> in one go (what splits the string by ":" is open_path()). This means
> that the entire RPATH entry containing $ORIGIN will be ignored if
> /proc/self/exe can't be accessed, despite one or more of them not
> depending on $ORIGIN.

This has come up before on the list. It is different behavior from
libc, and it may be CVE worthy if a down-level library is used when an
updated library is available but lost because the RPATH/RUNPATH is
discarded.

Jeff

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] $ORIGIN rpath expansion without /proc: code looks wrong
  2021-11-17 19:25   ` Alexander Sosedkin
@ 2021-11-18 19:15     ` Érico Nogueira
  0 siblings, 0 replies; 9+ messages in thread
From: Érico Nogueira @ 2021-11-18 19:15 UTC (permalink / raw)
  To: Alexander Sosedkin, musl; +Cc: monk

On Wed Nov 17, 2021 at 4:25 PM -03, Alexander Sosedkin wrote:
> On 11/17/21 18:00, Érico Nogueira wrote:
> > On Wed Nov 17, 2021 at 11:04 AM -03, Alexander Sosedkin wrote:
> >> Hello, I've encountered a case of a binary with an rpath of
> >> /some/meaningful/lib:$ORIGIN/../lib
> >> not starting up due to not finding /some/meaningful/lib/libxyz.so.
> >> ldd'ing said it's there though.
> >> And the library was found alright when I patchelf'd rpath to just
> >> /some/meaningful/lib
> >>
> >> I dug into musl code and came across that bit that checks /proc.
> >> Sure enough, when I tried mounting /proc, it started working fine.
> >> Yet the error handling from accessing /proc puzzles me:
> >>
> >> ldso/dynlink.c, fixup_rpath():
> >> l = readlink("/proc/self/exe", buf, buf_size);
> >> if (l == -1) switch (errno) {
> >> case ENOENT:
> >> case ENOTDIR:
> >> case EACCES:
> >> break;
> >> default:
> >> return -1;
> >> }
> >> if (l >= buf_size)
> >> return 0;
> >> buf[l] = 0;
> >> origin = buf;
> >>
> >> hitting that break like I had means zeroing buf[-1], right?
> > 
> > No. Because `l` is size_t (unsigned long), it's the biggest possible
> > value for size_t, and `l >= buf_size` will be true,
>
> Oh! Thanks a lot, that's what confused me. Sorry for the noise then.
>
> > which means the
> > function returns 0. This conditional also catches the case where
> > truncation happens in readlink(3).
> > Documenting this in a comment or changing `break;` for `return 0;` might
> > make sense, though.
>
> Yeah, I'd say a `return 0;` there would've been easier to comprehend.
> I don't think there's much need for a comment...

That sounds reasonable to me. I can make a patch for it.

>
> >> Could somebody take a look at this and double-check that
> >> this codepath makes sense?
> > 
> > It does, but it might not be as robust as you wish. fixup_rpath() treats
> > the RPATH entry as a single string, and does all $ORIGIN substitutions
> > in one go (what splits the string by ":" is open_path()). This means
> > that the entire RPATH entry containing $ORIGIN will be ignored if
> > /proc/self/exe can't be accessed, despite one or more of them not
> > depending on $ORIGIN.
>
> ... because if running /proc-less isn't supported in general,
> then such separate expansions are probably not worth the effort.
> And it's far from being the only mention of /proc in the code,
> so I'm going to presume it's not.

https://musl.libc.org/doc/1.1.24/manual.html does document the
requirement that /proc exist, and at a an arbitrary guess, complex
binaries which rely on RPATH are unlikely to run before the boot process
has mounted /proc.

I can think of some ways to try and change the order the operations are
done in, but in the end, it's probably not worth it to make changes to
this part of the code.

>
> >> My attempts at comprehending it fail irrecoverably at this line.
> >>
> >> (CC me on replies, please.
> >> No nice context to provide, building my own toolchain at
> >> https://github.com/t184256/bootstrap-from-tcc)
>
> Thanks for the answer, appreciated.

Glad to help!


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] $ORIGIN rpath expansion without /proc: code looks wrong
  2021-11-17 20:01   ` Jeffrey Walton
@ 2021-11-18 19:21     ` Érico Nogueira
  2021-11-18 19:41       ` Alexander Sosedkin
  2021-11-18 19:42       ` Jeffrey Walton
  0 siblings, 2 replies; 9+ messages in thread
From: Érico Nogueira @ 2021-11-18 19:21 UTC (permalink / raw)
  To: musl; +Cc: monk

On Wed Nov 17, 2021 at 5:01 PM -03, Jeffrey Walton wrote:
> On Wed, Nov 17, 2021 at 12:09 PM Érico Nogueira <ericonr@disroot.org>
> wrote:
> >
> > On Wed Nov 17, 2021 at 11:04 AM -03, Alexander Sosedkin wrote:
> > > ...
> > > Could somebody take a look at this and double-check that
> > > this codepath makes sense?
> >
> > It does, but it might not be as robust as you wish. fixup_rpath() treats
> > the RPATH entry as a single string, and does all $ORIGIN substitutions
> > in one go (what splits the string by ":" is open_path()). This means
> > that the entire RPATH entry containing $ORIGIN will be ignored if
> > /proc/self/exe can't be accessed, despite one or more of them not
> > depending on $ORIGIN.
>
> This has come up before on the list. It is different behavior from
> libc, and it may be CVE worthy if a down-level library is used when an
> updated library is available but lost because the RPATH/RUNPATH is
> discarded.

I would file such a CVE on the distro packaging or system administration
rather than musl. The binaries you need to run so /proc is mounted
shouldn't be the sort that depend on dynamic RPATH using ${ORIGIN}
(rather than a static one or no RPATH at all), and any security fix
should be confirmed to actually work before being deployed...

Furthermore, I don't think an unprivileged user should be able to
unmount /proc unless they have called prctl(PR_SET_NO_NEW_PRIVS, 1), no?
Which would make any "attacks" be directed at themselves.

>
> Jeff


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] $ORIGIN rpath expansion without /proc: code looks wrong
  2021-11-18 19:21     ` Érico Nogueira
@ 2021-11-18 19:41       ` Alexander Sosedkin
  2021-11-18 19:42       ` Jeffrey Walton
  1 sibling, 0 replies; 9+ messages in thread
From: Alexander Sosedkin @ 2021-11-18 19:41 UTC (permalink / raw)
  To: Érico Nogueira, musl

On 11/18/21 20:21, Érico Nogueira wrote:
> On Wed Nov 17, 2021 at 5:01 PM -03, Jeffrey Walton wrote:
>> On Wed, Nov 17, 2021 at 12:09 PM Érico Nogueira <ericonr@disroot.org>
>> wrote:
>>>
>>> On Wed Nov 17, 2021 at 11:04 AM -03, Alexander Sosedkin wrote:
>>>> ...
>>>> Could somebody take a look at this and double-check that
>>>> this codepath makes sense?
>>>
>>> It does, but it might not be as robust as you wish. fixup_rpath() treats
>>> the RPATH entry as a single string, and does all $ORIGIN substitutions
>>> in one go (what splits the string by ":" is open_path()). This means
>>> that the entire RPATH entry containing $ORIGIN will be ignored if
>>> /proc/self/exe can't be accessed, despite one or more of them not
>>> depending on $ORIGIN.
>>
>> This has come up before on the list. It is different behavior from
>> libc, and it may be CVE worthy if a down-level library is used when an
>> updated library is available but lost because the RPATH/RUNPATH is
>> discarded.
> 
> I would file such a CVE on the distro packaging or system administration
> rather than musl. The binaries you need to run so /proc is mounted
> shouldn't be the sort that depend on dynamic RPATH using ${ORIGIN}
> (rather than a static one or no RPATH at all), and any security fix
> should be confirmed to actually work before being deployed...

> Furthermore, I don't think an unprivileged user should be able to
> unmount /proc unless they have called prctl(PR_SET_NO_NEW_PRIVS, 1), no?
> Which would make any "attacks" be directed at themselves.

Well, an unprivileged user can bind-mount anything they want over /proc
with user mount namespaces, no problem.

The question would rather be "what gives", because
for non-suid case they seem to gain nothing they can't gain
by supplying their own dynamic loader;
and for suid case we don't resolve $ORIGIN anyway.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] $ORIGIN rpath expansion without /proc: code looks wrong
  2021-11-18 19:21     ` Érico Nogueira
  2021-11-18 19:41       ` Alexander Sosedkin
@ 2021-11-18 19:42       ` Jeffrey Walton
  2021-11-18 20:30         ` Rich Felker
  1 sibling, 1 reply; 9+ messages in thread
From: Jeffrey Walton @ 2021-11-18 19:42 UTC (permalink / raw)
  To: musl; +Cc: monk

On Thu, Nov 18, 2021 at 2:30 PM Érico Nogueira <ericonr@disroot.org> wrote:
>
> On Wed Nov 17, 2021 at 5:01 PM -03, Jeffrey Walton wrote:
> > On Wed, Nov 17, 2021 at 12:09 PM Érico Nogueira <ericonr@disroot.org>
> > wrote:
> > >
> > > On Wed Nov 17, 2021 at 11:04 AM -03, Alexander Sosedkin wrote:
> > > > ...
> > > > Could somebody take a look at this and double-check that
> > > > this codepath makes sense?
> > >
> > > It does, but it might not be as robust as you wish. fixup_rpath() treats
> > > the RPATH entry as a single string, and does all $ORIGIN substitutions
> > > in one go (what splits the string by ":" is open_path()). This means
> > > that the entire RPATH entry containing $ORIGIN will be ignored if
> > > /proc/self/exe can't be accessed, despite one or more of them not
> > > depending on $ORIGIN.
> >
> > This has come up before on the list. It is different behavior from
> > libc, and it may be CVE worthy if a down-level library is used when an
> > updated library is available but lost because the RPATH/RUNPATH is
> > discarded.
>
> I would file such a CVE on the distro packaging or system administration
> rather than musl. The binaries you need to run so /proc is mounted
> shouldn't be the sort that depend on dynamic RPATH using ${ORIGIN}
> (rather than a static one or no RPATH at all), and any security fix
> should be confirmed to actually work before being deployed...

If I had an actual failure for Musl on Alpine, I would post it to the
Musl list and possibly OSSecurity. I don't use Alpine regularly, so I
won't manufacture the problem just to complain.

But I have encountered the problem with Perl. Perl configuration does
not handle CFLAGS and CXXFLAGS correctly, so it screwed up RPATH
during build time. At runtime, Perl used a down-level bzip2 with an
outsatnding CVE.

In Perl's case, Perl simply dropped '$ORIGIN', and '$ORIGIN/../lib'
became just '/../lib', which became '/lib'. And the vulnerable bzip2
library resided in /lib.

Also keep in mind the ELF Format specification does not provide
authority to drop all RPATHs like Musl is doing. The ELF specification
is very clear how the paths are evaluated.

The ELF specification says to try to find the shared object at the
RPATH location. If the shared object does not exist, then move to the
next location. If the shared object does not exist in any of the
RPATHs, then move to LD_LIBRARY_PATHs. If the shared object does not
exist in any of the LD_LIBRARY_PATHs, then move to /lib. The ELF
specification does not say to discard all paths if the library is not
found at a location. Also see the section "Shared Object Dependencies"
in the ELF specification.

Jeff

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] $ORIGIN rpath expansion without /proc: code looks wrong
  2021-11-18 19:42       ` Jeffrey Walton
@ 2021-11-18 20:30         ` Rich Felker
  0 siblings, 0 replies; 9+ messages in thread
From: Rich Felker @ 2021-11-18 20:30 UTC (permalink / raw)
  To: Jeffrey Walton; +Cc: musl, monk

On Thu, Nov 18, 2021 at 02:42:22PM -0500, Jeffrey Walton wrote:
> On Thu, Nov 18, 2021 at 2:30 PM Érico Nogueira <ericonr@disroot.org> wrote:
> >
> > On Wed Nov 17, 2021 at 5:01 PM -03, Jeffrey Walton wrote:
> > > On Wed, Nov 17, 2021 at 12:09 PM Érico Nogueira <ericonr@disroot.org>
> > > wrote:
> > > >
> > > > On Wed Nov 17, 2021 at 11:04 AM -03, Alexander Sosedkin wrote:
> > > > > ...
> > > > > Could somebody take a look at this and double-check that
> > > > > this codepath makes sense?
> > > >
> > > > It does, but it might not be as robust as you wish. fixup_rpath() treats
> > > > the RPATH entry as a single string, and does all $ORIGIN substitutions
> > > > in one go (what splits the string by ":" is open_path()). This means
> > > > that the entire RPATH entry containing $ORIGIN will be ignored if
> > > > /proc/self/exe can't be accessed, despite one or more of them not
> > > > depending on $ORIGIN.
> > >
> > > This has come up before on the list. It is different behavior from
> > > libc, and it may be CVE worthy if a down-level library is used when an
> > > updated library is available but lost because the RPATH/RUNPATH is
> > > discarded.
> >
> > I would file such a CVE on the distro packaging or system administration
> > rather than musl. The binaries you need to run so /proc is mounted
> > shouldn't be the sort that depend on dynamic RPATH using ${ORIGIN}
> > (rather than a static one or no RPATH at all), and any security fix
> > should be confirmed to actually work before being deployed...
> 
> If I had an actual failure for Musl on Alpine, I would post it to the
> Musl list and possibly OSSecurity. I don't use Alpine regularly, so I
> won't manufacture the problem just to complain.
> 
> But I have encountered the problem with Perl. Perl configuration does
> not handle CFLAGS and CXXFLAGS correctly, so it screwed up RPATH
> during build time. At runtime, Perl used a down-level bzip2 with an
> outsatnding CVE.
> 
> In Perl's case, Perl simply dropped '$ORIGIN', and '$ORIGIN/../lib'
> became just '/../lib', which became '/lib'. And the vulnerable bzip2
> library resided in /lib.
> 
> Also keep in mind the ELF Format specification does not provide
> authority to drop all RPATHs like Musl is doing. The ELF specification
> is very clear how the paths are evaluated.
> 
> The ELF specification says to try to find the shared object at the
> RPATH location. If the shared object does not exist, then move to the
> next location. If the shared object does not exist in any of the
> RPATHs, then move to LD_LIBRARY_PATHs. If the shared object does not
> exist in any of the LD_LIBRARY_PATHs, then move to /lib. The ELF
> specification does not say to discard all paths if the library is not
> found at a location. Also see the section "Shared Object Dependencies"
> in the ELF specification.

With the caveat that we do not claim conformance to these behaviors
(for example, RPATH intentionally always behaves with the new RUNPATH
semantics because the original RPATH ones were wrong), I agree that
this is a bug. Failure to resolve $ORIGIN should not cause the rest of
the RPATH to be discarded. It should either prevent *all* search (due
to inability to evaluate the intended path accurately), preventing the
library from loading at all if it has any dependencies that aren't
already loaded, or should cause just the affected components to be
skipped. I'm not sure which behavior is better. The former is in some
sense more correct/fail-safe (never load wrong thing), but since this
error condition can only happen when you don't have /proc available,
the friendlier behavior might be just skipping these components. This
could probably be achieved just by setting origin="/dev/null"
(pathname guaranteed to exist and not be a directory) in the error
path.

Rich

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-11-18 20:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-17 14:04 [musl] $ORIGIN rpath expansion without /proc: code looks wrong Alexander Sosedkin
2021-11-17 17:00 ` Érico Nogueira
2021-11-17 19:25   ` Alexander Sosedkin
2021-11-18 19:15     ` Érico Nogueira
2021-11-17 20:01   ` Jeffrey Walton
2021-11-18 19:21     ` Érico Nogueira
2021-11-18 19:41       ` Alexander Sosedkin
2021-11-18 19:42       ` Jeffrey Walton
2021-11-18 20:30         ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).