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

Jump to bottom

libressl: build libcrypto with noexecstack #66454

Merged
merged 4 commits into from Aug 21, 2019
Merged

libressl: build libcrypto with noexecstack #66454

merged 4 commits into from Aug 21, 2019

Conversation

ruuda
Copy link
Contributor

@ruuda ruuda 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

@ruuda
libressl: build libcrypto with noexecstack
8b6a920 
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
Copy link
Contributor Author

ruuda 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
Copy link
Member

mmahut commented Aug 11, 2019

@GrahamcOfBorg build libressl

@risicle
Copy link
Contributor

risicle 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
Copy link
Contributor Author

ruuda commented Aug 11, 2019 •
edited

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?

@ruuda
libressl: fix noexecstack on Darwin
b3c613b 
The flags to disable executable stacks are different for Clang and GCC,
and Clang is used on Darwin.
@matthewbauer
Copy link
Member

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

@ruuda
libressl: do not set noexecstack on Darwin at all
c02b4a1 
It is not needed on Darwin. [1] Thanks Matthew for explaining this.

[1]: NixOS#66454 (comment)
@ruuda
Copy link
Contributor Author

ruuda commented Aug 13, 2019

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

@mmahut
Copy link
Member

mmahut commented Aug 13, 2019

@GrahamcOfBorg build libressl

@thoughtpolice
Copy link
Member

thoughtpolice commented Aug 20, 2019 •
edited

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
Copy link
Contributor Author

ruuda commented Aug 20, 2019 •
edited

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.

matthewbauer
matthewbauer approved these changes Aug 20, 2019
View reviewed changes
Copy link
Member

@matthewbauer matthewbauer left a comment

Choose a reason for hiding this comment

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

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

@ruuda
libressl: use CFLAGS to avoid exectuable stack
fdd78a5 
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
Copy link
Contributor Author

ruuda 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 August 20, 2019 22:49
@matthewbauer
Copy link
Member

@GrahamcOfBorg build libressl

@matthewbauer matthewbauer merged commit 856d10a into NixOS:master Aug 21, 2019
@ruuda ruuda deleted the libcrypto-noexecstack branch October 6, 2022 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer approved these changes

@globin globin Awaiting requested review from globin

@fpletz fpletz Awaiting requested review from fpletz

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

Assignees
No one assigned
Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ruuda @mmahut @risicle @matthewbauer @thoughtpolice