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 in released binary #1514

Closed
berlic opened this issue Oct 26, 2017 · 12 comments · Fixed by #2801
Closed

Enable stack protection in released binary #1514

berlic opened this issue Oct 26, 2017 · 12 comments · Fixed by #2801

Comments

@berlic
Copy link

berlic commented Oct 26, 2017

I've recently launched AWS Inspector scan across our systems, and one of the findings was:

Finding: The following executable files on instance i-xxxxxx do not support stack cookies: /usr/bin/jq.
Severity: Medium
Description: This rule detects the presence of third-party software that is compiled without support for stack cookies. Stack cookies increase system security by defending against stack-based buffer overflow and other memory corruption attacks.
Recommendation: It is recommended that you uninstall this software from your assessment target if you are not using it, or contact the vendor to get an updated version of this software with stack cookies enabled.

I've then checked latest available release with hardening-includes package:

# ./jq-linux64 --version
jq-1.5
# sha256sum ./jq-linux64
c6b3a7d7d3e7b70c6f51b706a3b90bd01833846c54d32ca32f0027f00226ff6d  ./jq-linux64
# hardening-check ./jq-linux64
./jq-linux64:
 Position Independent Executable: no, normal executable!
 Stack protected: no, not found!
 Fortify Source functions: unknown, no protectable libc functions used
 Read-only relocations: yes
 Immediate binding: no, not found!

Then I've downloaded corresponding source package jq-1.5.tar.gz and compiled it with just ./configure and make. That gave me this result:

# ./jq --version
jq-1.5-1-g940132e-dirty
# hardening-check ./jq
./jq:
 Position Independent Executable: no, normal executable!
 Stack protected: yes
 Fortify Source functions: yes (some protected functions found)
 Read-only relocations: yes
 Immediate binding: no, not found!

After deploying this binary to my hosts, subsequent AWS Inspector scans raised no security issues about jq.

Please consider enabling security features for released binaries by default.

@itchyny itchyny added this to the 1.7 release milestone Jun 25, 2023
@itchyny
Copy link
Contributor

itchyny commented Jul 23, 2023

Does anyone successfully build with both stack protection and -all-static enabled?

@wader
Copy link
Member

wader commented Jul 23, 2023

Seems like "static PIE" with glibc should work now, might be nice also

@wader
Copy link
Member

wader commented Jul 26, 2023

Did some digging:

I failed to get clang on macOS to compile with stack protection:

$ clang --version
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: x86_64-apple-darwin22.5.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin

$ clang -fsanitize=safe-stack test.c
clang: error: unsupported option '-fsanitize=safe-stack' for target 'x86_64-apple-darwin22.5.0'

But reading https://releases.llvm.org/14.0.0/tools/clang/docs/SafeStack.html it seems like it should be supported? something special with Apples clang build or do i misunderstand something?

@wader
Copy link
Member

wader commented Jul 26, 2023

Some progress trying to add static pie and hardening options to configure. Seems libtool is not very happy about static pie :( but i'm not very familiar with it, currently i think i kind of bypass it by using -Wl and use gcc specific things.

I was thinking we could do something like this: (NOTE: does not work, i get link error)

diff --git a/Makefile.am b/Makefile.am
index ad28407..4e0e446 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -132,6 +132,15 @@ if ENABLE_ALL_STATIC
 jq_LDFLAGS += -all-static
 endif

+if ENABLE_STATIC_PIE
+AM_CFLAGS += -fPIC -static-libgcc
+jq_LDFLAGS += -Wl,-static,-pie,-z,relro,-z,now
+endif
+
+if ENABLE_HARDENING
+AM_CFLAGS += -fstack-protector-all
+endif
+
 ### Tests (make check)

 TESTS = tests/optionaltest tests/mantest tests/jqtest tests/shtest tests/utf8test tests/base64test
diff --git a/configure.ac b/configure.ac
index 9e8830b..2c785f6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -82,6 +82,14 @@ dnl Enable building all static
 AC_ARG_ENABLE([all-static],
    AS_HELP_STRING([--enable-all-static],[link jq with static libraries only]))

+dnl Enable static PIE
+AC_ARG_ENABLE([static-pie],
+   AS_HELP_STRING([--enable-static-pie],[link as static-pie]))
+
+dnl Enable hardening
+AC_ARG_ENABLE([hardening],
+   AS_HELP_STRING([--enable-hardening],[compile with hardening options]))
+
 dnl find pipenv
 AC_ARG_VAR([PIPENV], [pipenv command])
 AC_CHECK_PROGS([PIPENV], pipenv)
@@ -122,6 +130,8 @@ AM_CONDITIONAL([ENABLE_GCOV], [test "x$enable_gcov" = xyes])
 AM_CONDITIONAL([ENABLE_DOCS], [test "x$enable_docs" != xno])
 AM_CONDITIONAL([ENABLE_ERROR_INJECTION], [test "x$enable_error_injection" = xyes])
 AM_CONDITIONAL([ENABLE_ALL_STATIC], [test "x$enable_all_static" = xyes])
+AM_CONDITIONAL([ENABLE_STATIC_PIE], [test "x$enable_static_pie" = xyes])
+AM_CONDITIONAL([ENABLE_HARDENING], [test "x$enable_hardening" = xyes])

 dnl Find pthread, if we have it. We do this first because we may set -pthread on CFLAGS
 dnl which can cause various macros to be defined (__REENTRANT on Darwin, for example)

@nicowilliams
Copy link
Contributor

nicowilliams commented Jul 30, 2023

Is this as easy as adding -fstack-protector-all to CFLAGS in the ./configure invocation in the release workflow(s)?

Well, there's other options too.

@nicowilliams
Copy link
Contributor

CFLAGS="-g -ggdb3 -gdwarf-4 -Og -pthread -fstack-protector-all -mshstk -fsanitize=safe-stack -fcf-protection" seems to work with clang. On some HW we could -fsanitize=cfi with clang too.

@nicowilliams
Copy link
Contributor

Ah, but @wader is right, the issue is --enable-all-static.

@nicowilliams
Copy link
Contributor

nicowilliams commented Jul 30, 2023

  • '--enable-all-static' '--disable-shared' '--prefix=/tmp/jq' '--with-oniguruma=builtin' 'CFLAGS=-O2 -pthread -fstack-protector-all' 'CC=clang' builds statically and passes all the tests
    (Just not with valgrind. Some tests have weird failures with valgrind, such as 394 of 421 tests passed (37 malformed, 0 skipped) for tests/jqtest! And there's lots of noise that requires a suppressions file for the all-static executable. But that's all ok, as using valgrind with statically linked executables has issues.)

  • '--enable-all-static' '--disable-shared' '--prefix=/tmp/jq' '--with-oniguruma=builtin' 'CFLAGS=-O2 -pthread -fstack-protector-all -mshstk -fcf-protection' 'CC=clang' also works (again, except for valgrind)

  • '--enable-all-static' '--disable-shared' '--prefix=/tmp/jq' '--with-oniguruma=builtin' 'CFLAGS=-O2 -pthread -fstack-protector-all -mshstk -fsanitize=safe-stack -fcf-protection' 'CC=clang' does not work (for me).

So of all the options listed above, -fsanitize=safe-stack does not work.

@nicowilliams
Copy link
Contributor

I do like the static-PIE idea (where the executable is a PIE, dynamically linked as far as system libraries go, but statically linked as far as libjq and Oniguruma (if in-tree) go) BTW -- that would allow valgrind to work w/o issues.

@wader
Copy link
Member

wader commented Jul 30, 2023

I do like the static-PIE idea (where the executable is a PIE, dynamically linked as far as system libraries go, but statically linked as far as libjq and Oniguruma (if in-tree) go) BTW -- that would allow valgrind to work w/o issues.

Yeap as i understand it static-PIE is essentially you ship a linker with the binary. For us it would be good to even link glibc "staticallty" i think so that the binaries will run on non-glibc based system also, shouldn't be a problem.

For macOS i think our current "all-static" build is already a PIE.

Windows i don't think know.

@nicowilliams
Copy link
Contributor

@wader I don't think we need to make the release executables by PIEs for 1.7, though PIE building would be nice indeed. The only downside to the all-static builds is the interoperability problems with valgrind, which is not a big deal.

@nicowilliams
Copy link
Contributor

Let's see in #2801 does it.

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

Successfully merging a pull request may close this issue.

4 participants