Skip to content

Conversation

mrcnski
Copy link
Contributor

@mrcnski mrcnski commented May 18, 2023

Only updated exploit-mitigations.md to reflect that the option exists. Removed the alternatives mentioned as they are not actually implemented yet.

As this is an unstable feature, should it be added to unstable-book also? Example. I didn't do that because I couldn't find the tracking issue for stack-protector. (There should be one to track stabilization of the feature, I think?)

cc @rcvalle

Only updated `exploit-mitigations.md` to reflect that the option exists. Removed the alternatives
mentioned as they are not actually implemented yet.

As this is an unstable feature, should it be added to `unstable-book` also? I didn't do that because
I couldn't find the tracking issue for it. (There should be one to track stabilization of the
feature.)
@rustbot
Copy link
Collaborator

rustbot commented May 18, 2023

r? @JohnTitor

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 18, 2023
@rcvalle
Copy link
Member

rcvalle commented May 19, 2023

Here is an updated "Fig. 14. IDA Pro listing cross references to __stack_chk_fail in hello-rust." (i.e., image3.png) for you to update:

image3

@rcvalle
Copy link
Member

rcvalle commented May 19, 2023

I also suggest changing

To check if stack smashing protection is enabled for a given binary, search
for cross references to `__stack_chk_fail`. The only cross references to
`__stack_chk_fail` in hello-rust are from the statically-linked libbacktrace
library (see Fig. 14).

to

To check if stack smashing protection is enabled for a given binary, search
for cross references to `__stack_chk_fail` (see Fig. 14).

@JohnTitor
Copy link
Member

r? rcvalle

@rustbot rustbot assigned rcvalle and unassigned JohnTitor May 19, 2023
@rcvalle rcvalle added the PG-exploit-mitigations Project group: Exploit mitigations label May 19, 2023
@rcvalle rcvalle linked an issue May 19, 2023 that may be closed by this pull request
@mrcnski
Copy link
Contributor Author

mrcnski commented May 19, 2023

Thanks for the review @rcvalle! Will make the changes, I just want to make sure I understand them correctly: does the presence of cross-references to __stack_chk_fail in the binary mean that stack-protector is turned on? If so, is it expected that the number of cross-references went down since the feature was implemented? (15 -> 1)

@rcvalle
Copy link
Member

rcvalle commented May 19, 2023

Thanks for the review @rcvalle! Will make the changes, I just want to make sure I understand them correctly: does the presence of cross-references to __stack_chk_fail in the binary mean that stack-protector is turned on?

Yes, the presence of cross-references to __stack_chk_fail from Rust-compiled code (e.g., hello_rust::main) indicates that the stack smashing protection is enabled.

If so, is it expected that the number of cross-references went down since the feature was implemented? (15 -> 1)
The only cross references to

Note that previously there were no cross-references to __stack_chk_fail from Rust-compiled code. The only cross references to __stack_chk_fail were from the statically-linked libbacktrace library (i.e., non Rust-compiled code).

@mrcnski
Copy link
Contributor Author

mrcnski commented May 21, 2023

Done!

@rcvalle
Copy link
Member

rcvalle commented May 22, 2023

Thank you for your time and for working on this, @mrcnski! Much appreciated.

@rcvalle
Copy link
Member

rcvalle commented May 22, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented May 22, 2023

@rcvalle: 🔑 Insufficient privileges: Not in reviewers

@cuviper
Copy link
Member

cuviper commented May 22, 2023

@bors r=rcvalle

@bors
Copy link
Collaborator

bors commented May 22, 2023

📌 Commit a4d6d9a has been approved by rcvalle

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#111427 ([rustdoc][JSON] Use exclusively externally tagged enums in the JSON representation)
 - rust-lang#111486 (Pretty-print inherent projections correctly)
 - rust-lang#111722 (Document stack-protector option)
 - rust-lang#111761 (fix(resolve): not defined `extern crate shadow_name`)
 - rust-lang#111845 (Update books)
 - rust-lang#111851 (CFI: Fix encode_region: unexpected ReEarlyBound(0, 'a))
 - rust-lang#111871 (Migrate GUI colors test to original CSS color format)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ee08dd8 into rust-lang:master May 23, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 23, 2023
arielb1 pushed a commit to arielb1/rust that referenced this pull request Sep 9, 2025
I propose stabilizing `-Cstack-protector` as `-Zstack-protector`. This PR adds a new `-Cstack-protector` flag, leaving the unstable `-Z` flag as is to ease the transition period. The `-Z` flag will be removed in the future.

No RFC/MCP, this flag was added in rust-lang#84197 and was not deemed large enough to require additional process.

The tracking issue for this feature is rust-lang#114903.

The `-Cstack-protector=strong` mode uses the same underlying heuristics as Clang's `-fstack-protector-strong`.
These heuristics weren't designed for Rust, and may be over-conservative in some cases - for example, if
Rust stores a field's data in an alloca using an LLVM array type, LLVM regard the alloca as meaning
that the function has a C array, and enable stack overflow canaries even if the function accesses
the alloca in a safe way. Some people thought we should wait on stabilization until there are better
heuristics, but I didn't hear about any concrete case where this unduly harms performance, and I think
that when a need comes, we can improve the heuristics in LLVM after stabilization.

The heuristics do seem to not be under-conservative, so this should not be a security risk.

The `-Cstack-protector=basic` mode (`-fstack-protector`) uses heuristics that are specifically designed
to catch old-C-style string manipulation. This is not a good fit to Rust, which does not perform much
unsafe C-style string manipulation. As far as I can tell, nobody has been asking for it,
and few people are using it even in today's C - modern distros (e.g. [Debian]) tend to use
`-fstack-protector-strong`.

Therefore, `-Cstack-protector=basic` has been **removed**. If anyone is interested in it, they
are welcome to add it back as an unstable option.

[Debian]: https://wiki.debian.org/Hardening#DEB_BUILD_HARDENING_STACKPROTECTOR_.28gcc.2Fg.2B-.2B-_-fstack-protector-strong.29

Most implementation was done in <rust-lang#84197>. The command-line
attribute enables the relevant LLVM attribute on all functions in
<https://github.com/rust-lang/rust/blob/68baa87ba6f03f8b6af2a368690161f1601e4040/compiler/rustc_codegen_llvm/src/attributes.rs#L267-L276>.

Each target can indicate that it does not support stack canaries - currently,
the GPU platforms `nvptx64-nvidia-cuda` and `amdgcn-amd-amdhsa`. On these
platforms, use of `-Cstack-protector` causes an error.

The feature has tests that make sure that the LLVM heuristic gives reasonable
results for several functions, by checking for `__security_check_cookie` (on Windows)
or `__stack_chk_fail` (on Linux). See
<https://github.com/rust-lang/rust/tree/68baa87ba6f03f8b6af2a368690161f1601e4040/tests/assembly-llvm/stack-protector>

No call-for-testing has been conducted, but the feature seems to be in use.

No reported bugs seem to exist.

- @bbjornse was the original implementor at rust-lang#84197
- @mrcnski documented it at rust-lang#111722
- @wesleywiser added tests for Windows at rust-lang#116037
- @davidtwco worked on the feature at rust-lang#121742
- @nikic provided support from the LLVM side (on Zulip on <https://rust-lang.zulipchat.com/#narrow/channel/233931-t-compiler.2Fmajor-changes/topic/Proposal.20for.20Adapt.20Stack.20Protector.20for.20Ru.E2.80.A6.20compiler-team.23841> and elsewhere),
  thanks @nikic!

No FIXMEs related to this feature.

This feature cannot cause undefined behavior.

No changes to reference/spec, docs added to the codegen docs as part of the stabilization PR.

No.

None.

No support needed for rustdoc, clippy, rust-analyzer, rustfmt or rustup.

Cargo could expose this as an option in build profiles but I would expect the decision as to what version should be used would
be made for the entire crate graph at build time rather than by individual package authors.

`-C stack-protector` is propagated to C compilers using cc-rs via rust-lang/cc-rs#1550
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document stack smashing protection
6 participants