Explicitly disable stack execution on linux and bsd

[aliclark]

This explicitly adds an option telling the linker on these platforms to make the stack and heap non-executable (should already be the case for Windows, and likely OS X).

Without this option there is some risk of accidentally losing NX protection, as the linker will not enable NX if any of the binary's constituent objects don't contain the .note.GNU-stack header.

We're not aware of any users who would want a binary with executable stack or heap, but in future this could be made possible by passing a flag to disable the protection, which would also help document the fact to the crate's users.

Edit: older discussion of previous quickfix to add a .note.GNU-stack header to libunwind's assembly:

Short term solution for issue #30824 to ensure that object files generated from assembler contain the .note.GNU-stack header.

When this header is not present in any constituent object files, the linker refrains from making the stack NX in the final executable.

Further actions:

I'll try to get this change merged in with upstream too, and then update these instructions to just compile the fixed version.

It seems a good idea to use issue #30824 or some other issue to add a test that similar security regressions can be automatically caught in future.

[alexcrichton]

This seems somewhat odd recommending modifying the musl source, is this typically done via an assembly file? I figured this'd be some random flag to the linker or something like that.

[alexcrichton]

Also note that our script for building MUSL is located at https://github.com/rust-lang/rust-buildbot/blob/master/slaves/linux/build-musl.sh which we use in the docker container that builds the MUSL nightlies.

[aliclark]

To clarify the modification is to llvm's libunwind, which generates two objects from assembly files. The immediate issue is with libunwind, and I would propose a similar change to them (but inside an ifdef linux && elf). I think this article has a good background on the topic: https://wiki.gentoo.org/wiki/Hardened/GNU_stack_quickstart#How_to_fix_the_stack_.28in_theory.29

Thanks for pointing out the build-musl.sh script, that would indeed require a similar change.

I do think your suggestion of using linker argument is a good one, as this should robustly ensure a similar issue won't crop up again in future. I've stopped short of adding it to all *_base.rs files in case the argument is not present on some platform, but it would be a good idea to others too. Thanks

[alexcrichton]

Hm if this is possible to be done so via a linker flag, should this become the default for all compilations? It seems that it may be best to leave the defaults as is?

[aliclark]

You mean of rlib instead of just exe? Sure, I don't think rlibs would need this flag, though it's completely harmless so I personally wouldn't go out of my way to not specify it for rlib, unless it was easy and safe to do so.

If you mean all as in musl vs. others, then I'd say that this is already the default implicitly, and it's good in my opinion to make this configuration explicit so it can't implicitly change underneath us in future like has happened here.

[alexcrichton]

I'd be curious to drill more into what's going on here as modifying the linker seems like it's the wrong place to deal with this if it's already happening for Linux elsewhere. This seems like it's somewhat of an opinionated decision as others may want to not flip this bit, so I'd just want to be sure that we're not cornering ourselves too much.

It seems that whatever is causing normal executables to have this bit set should likely be usable by MUSL as well? Is that the assembly files that's going on? If so, that would mean that naturally anything linking against the musl we distribute would pick that up, which at this point seems like the better solution to me.

[aliclark]

Executables normally have NX set because by default gcc will compile C objects with a ".note.GNU-stack" header included, and when linking an executable only from objects containing this note the resulting binary will be NX.

However gcc does not currently do this when compiling objects from assembly, so unless care is taken these objects disable NX when linked into any binary. Unfortunately that is exactly what happened for this issue.

Regarding the use of NX, there are approximately no cases where this would be bad for a program (which is why it's a default for gcc compiling C code), but at the same time it is quite an effective extra layer of security to prevent exploitation.

However you raise a good point about not cornering in or preventing use-cases, and I'm certainly not suggesting that Rust never allow users to compile their own programs with executable stacks, eg. with an option such as --executable-stack. What I would say though is that it's a very hypothetical use case and would not outweigh the benefit to most users for making NX explicitly the default.

For example, supposing libunwind does agree to change the building of these assembly files and Rust uses the amended version, Rust would still be exposed to great risk because:

1. the same issue could pop up in future for a different target
2. a Rust user might include some assembly in their program compiled with "/bin/cc -c", and not realise that this would disable NX protection in the resulting binary.

I do not have extensive knowledge of the code base but could look into creating an explicit "--executable-stack" type option if it allays concerns.

[aliclark]

In addition to the above a useful thought experiment is to imagine what would happen if gcc changed its default for compiling C objects to not automatically add .note.GNU-stack, or similarly if ld did not automatically compile these objects into an NX executable.

In that case the only way to achieve NX would be to supply the linker flag explicitly as in this patch. A very very similar situation exists for the PIE protection, which is explicitly requested for pretty much all compilation, as it is not used by default in gcc.

rust/src/librustc_trans/back/link.rs

Line 977 in c12c42d

if !dylib && t.options.position_independent_executables {

I'll try and hop on IRC at some point to chat as it might be more fun than a high latency discussion :)

[alexcrichton]

Hm ok, this does seem convincing to me in terms of being a reasonable default, I guess at that point I'd just want to make sure that there's some method of turning it off (if for whatever reason). I'd want to double check that this argument to the linker is pretty old as well so it's reasonably expected to work across platforms.

cc @rust-lang/tools, do others have opinions on setting the NX bit in executables by default?

[brson]

I don't really recall what sort of 'service-level' we provide for NX-ness. Do we set NX on windows and bsd's? Is it something we are trying to guarantee? Were our executables intentionally NX?

It seems like we've set things up to not prevent NX but not guarantee it. This patch I think is trying to guarantee it, but only on linux. If we want to guarantee NX then we should have a solution for all platforms.

I'm not convinced we should change the musl docs to manually frob the assembly - particularly if NX is not a guaranteed property of Rust code - but fix the bug that prevents NX. If we do change these docs, it should probably include a comment explaining why, maybe even linking to the appropriate issue so somebody might remember to remove it when it's fixed.

As comparison, stack overflow protection is more important than NX, Rust is supposed to guarantee it, but doesn't. Not sure what that says about this issue.

[aliclark]

I agree that if this discussion has not already happened it should happen first. Given that Rust programs in the wild are very much expected to contain unsafe code and C libraries it would be prudent in my opinion for Rust binaries to be secure against the vulnerabilities that will inevitably exist in these types of code.

In terms of impact, Fedora indicates that its packages are all built with NX (I think the same is true for Ubuntu), so no dependencies of these programs require an executable stack or heap. Therefore if such use-cases exist, they're niche and likely to remain so.

https://fedoraproject.org/wiki/Security_Features_Matrix
https://wiki.ubuntu.com/Security/Features

The patch is open to discussion, I was merely wary that for example the argument will be different for MSVC. link.rs or linker.rs might be a better location for the change. Agreed on the docs, that was always intended as only a short term frobulation.

A grep bisection indicates the '-z noexecstack' option appearing in binutils-2.15 which I think is May 2004.

Can't comment on the stack overflow protection, but maybe I can have a look at that one later on.

[alexcrichton]

Interestingly it looks like both windows-gnu and windows-msvc have the NX bit set already (or at least to the extent the linker allows it I believe).

I would personally be fine trying this out passing the ld flag, although I agree with @brson that we should likely try to add it everywhere if we're intentionally adding it. It looks like the OSX linker has a -allow_stack_execute argument, indicating that it's likely NX by default, and Wikipedia at least corroborates this. I believe all other linkers we have are GNU-like and would respect this flag.

[aliclark]

Awesome, good catch. Ignore the commit above then, I'll try adding the missing ld flags tomorrow.

[aliclark]

Slightly better than my original, I've added the config for BSDs as well as Linux. Windows I think is covered as per your comment and possibly Apple if it already occurs. Aside from that I've avoided changing Bitrig and NaCl due to uncertainty about their options...

[alexcrichton]

Thanks! This looks good to me, but could you update the PR title/description as well?

r? @brson, you ok with this as well?

[aliclark]

Ta, have done

[brson]

@bors r+

[bors]

📌 Commit 8e36b3a has been approved by brson

[bors]

⌛ Testing commit 8e36b3a with merge a186eb2...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt