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

Enable stack protection (CI release executables) #2801

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

nicowilliams
Copy link
Contributor

@nicowilliams nicowilliams commented Jul 31, 2023

Resolves #1514.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@nicowilliams nicowilliams force-pushed the fix_1514 branch 2 times, most recently from e80829a to dc49d0d Compare August 1, 2023 00:56
@nicowilliams nicowilliams marked this pull request as draft August 1, 2023 00:57
@nicowilliams
Copy link
Contributor Author

-mshstk didn't work, though I'm sure it would work if we only use clang. I suppose we could switch to clang for this. Also, I should enable this for OS X and for Windows too.

@@ -158,7 +159,8 @@ jobs:
--disable-valgrind \
--with-oniguruma=builtin \
--enable-static \
--enable-all-static
--enable-all-static \
CFLAGS="-O2 -pthread -fstack-protector-all"
Copy link
Member

@wader wader Aug 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh i somehow assumed -fsanitize=safe-stack was clang's versions of -fstack-protector-all but seems to be two different stack protection techniques.

I still haven't managed to get -fsanitize=safe-stack to work for darwin x86, but do seems to work on linux 🤷

@wader
Copy link
Member

wader commented Aug 1, 2023

I think this is good enough for 1.7. It would be great post 1.7 to provide these things using configure options somehow.

@nicowilliams nicowilliams marked this pull request as ready for review August 1, 2023 15:52
@nicowilliams nicowilliams added this to the 1.7 release milestone Aug 1, 2023
@itchyny
Copy link
Contributor

itchyny commented Aug 2, 2023

Did you check the executables built on CI to make sure hardening-check?

@nicowilliams
Copy link
Contributor Author

Did you check the executables built on CI to make sure hardening-check?

Not yet. Though, hmm, how would I? Use a debugger to try to force a buffer overflow?

@wader
Copy link
Member

wader commented Aug 2, 2023

Did you check the executables built on CI to make sure hardening-check?

Not yet. Though, hmm, how would I? Use a debugger to try to force a buffer overflow?

Seems like hardened-check should be able to detect it heuristically for ELF at least:

Stack Protected

This indicates that there is evidence that the ELF was compiled with the [gcc]>(https://manpages.ubuntu.com/manpages/trusty/man1/gcc.1.html)(1)
option -fstack-protector (e.g. uses __stack_chk_fail). The program will be
resistant to having its stack overflowed.

When an executable was built without any character arrays being allocated on the
stack, this check will lead to false alarms (since there is no use of
__stack_chk_fail), even though it was compiled with the correct options.

@wader
Copy link
Member

wader commented Aug 2, 2023

@nicowilliams noticed a fixup commit slipped in

@nicowilliams
Copy link
Contributor Author

image

What's stripping the executable?

@wader
Copy link
Member

wader commented Aug 2, 2023

@nicowilliams
Copy link
Contributor Author

Think this https://github.com/jqlang/jq/blob/master/.github/workflows/ci.yml#L74 probably?

Ay, yes. Well, I suppose I could build jq twice, once w/o stripping, to check if it has stack protection. Or maybe just punt. I'm doing a build without -s and if that works I'll put it back and drop the nm check and call it a day.

@nicowilliams
Copy link
Contributor Author

Without stripping we see:

image

So I'm going to undo the nm check and restore stripping.

Copy link
Member

@wader wader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I think we can makes this nicer after 1.7

@nicowilliams
Copy link
Contributor Author

I wonder if we could have a command-line option to indicate whether jq was built with any stack protection features. Or maybe we could have a command-line option that shows the build options used to build jq, just like many programs have.

@nicowilliams nicowilliams merged commit ff4bf68 into jqlang:master Aug 2, 2023
28 checks passed
@wader
Copy link
Member

wader commented Aug 2, 2023

I wonder if we could have a command-line option to indicate whether jq was built with any stack protection features. Or maybe we could have a command-line option that shows the build options used to build jq, just like many programs have.

Yeap that feels like quite neat solution. I poked around a bit with harden-checker and noticed that with static builds the __stack_chk_fail symbol seem to be included even when -fstack-protector is not used (so it part of glibc?), feels a bit shaky, so your proposal seems safer.

@nicowilliams
Copy link
Contributor Author

Yeap that feels like quite neat solution. I poked around a bit with harden-checker and noticed that with static builds the __stack_chk_fail symbol seem to be included even when -fstack-protector is not used (so it part of glibc?), feels a bit shaky, so your proposal seems safer.

With --run-tests we could spawn a thread to run a bit of vulnerable code and test it. Not sure that's a great idea or a terrible idea. Anyways, I opened #2817 to have jq print it's build configuration -- it captures only ./configure options, but maybe we could add to it a way to record the versions of all the relevant tools (autotools, compiler, bison, Oniguruma, etc.), and we could always do that later.

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.

Enable stack protection in released binary
3 participants