Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed building on Linux, against musl. #1

Closed
wants to merge 2 commits into from
Closed

Fixed building on Linux, against musl. #1

wants to merge 2 commits into from

Conversation

dimkr
Copy link

@dimkr dimkr commented Jul 11, 2014

No description provided.

@bob-beck
Copy link
Contributor

Sorry, no this introduces a rather massive reduction in security. No. You
need that fuction.

On Fri, Jul 11, 2014 at 2:01 PM, Dima Krasner notifications@github.com
wrote:


You can merge this Pull Request by running

git pull https://github.com/iguleder/portable master

Or view, comment on, or merge it at:

#1
Commit Summary

  • Fixed building on Linux, against musl.

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#1.

@dimkr
Copy link
Author

dimkr commented Jul 11, 2014

No reduction - getauxval() is called, but without the glibc version check.

@busterb
Copy link
Contributor

busterb commented Jul 11, 2014

Because historical implementations of getauxval() in glibc did not set errno on failure, we currently whitelist this specific version currently.

While it appears that musl's implementation (http://git.musl-libc.org/cgit/musl/tree/src/misc/getauxval.c) has not released a vulnerable version that returns '0' on failure, it does not seem practical to whitelist every other embedded linux libc individually (bionic, uclibc, etc.)

A nicer patch would be one that implements a portable (between different libcs and their versions) way of reading the aux vector without relying on a particular libc getting it right.

@bob-beck
Copy link
Contributor

On Fri, Jul 11, 2014 at 3:32 PM, busterb notifications@github.com wrote:

A nicer patch would be one that implements a portable (between different
libcs and their versions) way of reading the aux vector without relying on
a particular libc getting it right.

Indeed.

@dimkr
Copy link
Author

dimkr commented Jul 11, 2014

Makes sense. It seems uClibc doesn't have getauxval() and Bionic's one doesn't change errno (https://android.googlesource.com/platform/bionic/+/android-4.4.4_r1/libc/bionic/getauxval.cpp). Since Bionic seems to be the only problematic implementation except the old glibc one, I think adding #ifdef BIONIC and #error is the right thing to do. What do you think?

@busterb
Copy link
Contributor

busterb commented Jul 11, 2014

Another concern would be building on a non-vulnerable libc, then running against a vulnerable one. This is why this is currently a run-time check than a compile-time check.

Is the ABI differences between all of these libcs's so great such that a this is not a concern? I believe we still would prefer a more bullet-proof solution.

@dimkr
Copy link
Author

dimkr commented Jul 11, 2014

musl's getauxval() isn't vulnerable and hasn't changed since its introduction, so it's fine. All versions Bionic or uClibc are problematic. I don't see a need for a runtime check for these C libraries, because the only problematic scenario I can think of is building against musl and running against a vulnerable glibc. In this case, musl doesn't provide a version querying function similar to glibc's.

@busterb
Copy link
Contributor

busterb commented Jul 12, 2014

Please implement issetugid(), and the musl libc should work properly without issues.

@bob-beck
Copy link
Contributor

Yeah, if you guys provided a version of that, you'd be one up on glibc, and
we wouldn't have to use that file at all when linking with you.

Just make sure you do it right - I think it was completely busted in glibc
;)

On Sat, Jul 12, 2014 at 10:26 AM, busterb notifications@github.com wrote:

Please implement issetugid(), and the musl libc should work properly
without issues.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@busterb
Copy link
Contributor

busterb commented Jul 12, 2014

I forwarded a patch to the musl mailing list and built against the latest HEAD for musl.

I had to copy over some include/linux and include/asm headers to /usr/local/musl/include, but it worked. Let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants