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

libressl: build libcrypto with noexecstack #66454

Merged
merged 4 commits into from Aug 21, 2019

Conversation

@ruuda
Copy link
Contributor

commented Aug 10, 2019

For some reason, libcrypto would be built with the executable stack flag set. I found out about this when Nginx failed to load the shared library, because I was running it with MemoryDenyWriteExecute=true which does not permit executable stacks.

I am not sure why the stack ends up executable; the other shared libraries which are part of LibreSSL do not have this flag set. You can verify this with execstack -q. Non-executable stacks should be the
default, and from checking some other files, that does appear to be the case. The LibreSSL sources do not contain the string "execstack", so I am not sure what causes the default to be overridden.

Adding -z noexecstack to the linker flags makes the linker unset the flag. Now my Nginx can load the library, and so far I have not run into other issues.

I would like to understand where the shared stack requirement came from though. Without knowing where it originates from, it is hard to say what the impact of disabling it is. I did find these threads, which suggest that it is safe to use noexecstack:

Motivation for this change

Nginx built against LibreSSL cannot use MemoryDenyWriteExecute=true at the moment, because libcrypto.so.45 requires an executable stack. This is a weird requirement, especially for a crypto library.

Still, I would like to understand where the requirement came from before merging this.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/). Also tested Nginx built against this LibreSSL.
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @thoughtpolice @fpletz @globin

For some reasons, libcrypto would be built with the executable stack
flag set. I found out about this when Nginx failed to load the shared
library, because I was running it with MemoryDenyWriteExecute=true,
which does not permit executable stacks.

I am not sure why the stack ends up executable; the other shared
libraries which are part of LibreSSL do not have this flag set. You can
verify this with 'execstack -q'. Non-executable stacks should be the
default, and from checking some other files, that does appear to be the
case. The LibreSSL sources do not contain the string "execstack", so
I am not sure what causes the default to be overridden.

Adding '-z noexecstack' to the linker flags makes the linker unset the
flag. Now my Nginx can load the library, and so far I have not run into
other issues.
@ruuda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

The libcrypto in pkgs.openssl does already not require an executable stack:

$ execstack -q /nix/store/rngzs8bxnajzpwnkk4896gy9d5s525ic-openssl-1.0.2s/lib/*                             
execstack: cannot open ELF file: invalid file descriptor
- /nix/store/rngzs8bxnajzpwnkk4896gy9d5s525ic-openssl-1.0.2s/lib/libcrypto.so
- /nix/store/rngzs8bxnajzpwnkk4896gy9d5s525ic-openssl-1.0.2s/lib/libcrypto.so.1.0.0
- /nix/store/rngzs8bxnajzpwnkk4896gy9d5s525ic-openssl-1.0.2s/lib/libssl.so
- /nix/store/rngzs8bxnajzpwnkk4896gy9d5s525ic-openssl-1.0.2s/lib/libssl.so.1.0.0

The - means that no executable stack is required, an X would mean that it is.

@mmahut

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

@GrahamcOfBorg build libressl

@risicle

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

This doesn't work on darwin:

/nix/store/wmb4c95cyqryqs6adv9hw8gdbnbcffgj-clang-wrapper-7.1.0/bin/clang   -arch x86_64 -Wl,-search_paths_first -Wl,-headerpad_max_install_names   CMakeFiles/cmTC_df578.dir/testCCompiler.c.o  -o cmTC_df578
    ld: unknown option: -z
@ruuda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

Does Darwin have the concept of an executable stack, or should the flag it be omitted entirely there?

Edit: also, the flag is called --noexecstack for Clang, perhaps Clang will accept it regardless of whether the platform supports it.

Edit 2: I've changed the flag for Darwin, could you check if this works @risicle?

The flags to disable executable stacks are different for Clang and GCC,
and Clang is used on Darwin.
@matthewbauer

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

I don't believe this is necessary on Darwin. There is a MH_ALLOW_STACK_EXECUTION flag but it is only valid on executables (via -allow_stack_execute), not libraries

It is not needed on Darwin. [1] Thanks Matthew for explaining this.

[1]: #66454 (comment)
@ruuda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Thanks for clarifying that, I've removed the flags entirely for Darwin.

@mmahut

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@GrahamcOfBorg build libressl

@thoughtpolice

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

FWIW, my best guess for the reason this happens is that there is some object file that the linker is bringing in which is, for some reason, NOT marked to include the section .note.GNU-stack. This section directive tells the linker this object file does not need an executable stack. Provided every object has this section set, it will be removed from the final result, and the ELF header indicating noexecstack will be set. But on the flip side, if no such section exists -- in any object file -- the linker assumes the object is old, and therefore turns on executable stacks for the final result. It's all-or-nothing. (More details are available from Ian Lance Taylor.) Most importantly, this section is always emitted by modern compilers (except in very rare cases), so the most likely culprit is that a hand-written assembly file, for instance, that forgot to include this section, causing the whole thing to go belly up.

Some initial guesswork inside the source code didn't reveal anything on that note, unfortunately. Probably the best way to verify this theory is to simply run the buildPhase of libressl and then scan all object files for .note.GNU-section, and report any that don't exist. While I can't prove this is the issue, my gut feeling is that this might reveal a lead.

(In the mean time I am tentatively approving these changes, since they seem to be correct anyway, and work around a clear deficiency.)

@ruuda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

Some initial guesswork inside the source code didn't reveal anything on that note, unfortunately. Probably the best way to verify this theory is to simply run the buildPhase of libressl and then scan all object files for .note.GNU-section, and report any that don't exist. While I can't prove this is the issue, my gut feeling is that this might reveal a lead.

Thanks for the elaborate reply, that clarifies a lot. Your suspicion is spot on, there are a number of .S.o files in the build directory that are missing .note.GNU-stack.

List of those
# objdump exits with 0 if the section exists and with 1 if it does not.
$ find build/crypto/CMakeFiles/crypto.dir/ -name '*.o' -exec sh -c "if ! objdump -h --section=.note.GNU-stack {} > /dev/null 2>&1; then echo {}; fi" \;
build/crypto/CMakeFiles/crypto.dir/cpuid-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/whrlpool/wp-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/sha/sha512-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/sha/sha256-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/sha/sha1-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/rc4/rc4-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/rc4/rc4-md5-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/modes/ghash-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/md5/md5-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/camellia/cmll-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/bn/gf2m-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/bn/mont5-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/bn/mont-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/bn/modexp512-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/aes/aes-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/aes/bsaes-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/aes/aesni-sha1-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/aes/aesni-elf-x86_64.S.o
build/crypto/CMakeFiles/crypto.dir/aes/vpaes-elf-x86_64.S.o

The .S files for these are present in the source tarball. It looks like they are generated from scripts like these (cvs), but I don’t know exactly how. I will open an issue at libressl-portable/portable to ask if anything can be done about it upstream.

In the mean time, now that we know where the executable stack flag comes from, I think -z noexecstack is safe to apply.

Edit: while inspecting the .S files, they do contain this:

#if defined(HAVE_GNU_STACK)
.section .note.GNU-stack,"",%progbits
#endif

So perhaps we can get it working without -z noexecstack by providing the right defines. I will give that a try, as it feels a bit cleaner to me.

Copy link
Member

left a comment

Looks good for now. Ideally we can find the root cause, but for now this change along with the comments are okay.

It turns out that libcrypto had an exectuable stack, because it linked
some objects without a .note.GNU-stack section. Compilers add this
section by default, but the objects produced from .S files did not
contain it. The .S files do include a directive to add the section, but
guarded behind an #ifdef HAVE_GNU_STACK. So define HAVE_GNU_STACK, to
ensure that all objects have a .note.GNU-stack section.
@ruuda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

I’ve added a new commit that uses CMAKE_C_FLAGS=-DHAVE_GNU_STACK instead of NIX_LDFLAGS=-z noexecstack, and I have confirmed that this also clears the executable stack flag. I don’t know how this affects Darwin. If the extra define is not harmful, I think this is a bit nicer than the conditional LDFLAG, but taking c02b4a1 is fine with me too.

@ofborg ofborg bot requested a review from thoughtpolice Aug 20, 2019
@matthewbauer

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

@GrahamcOfBorg build libressl

@matthewbauer matthewbauer merged commit 856d10a into NixOS:master Aug 21, 2019
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
libressl on aarch64-linux Success
Details
libressl on x86_64-darwin Success
Details
libressl on x86_64-linux Success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.