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

Add exploit mitigations chapter to the rustc book #76858

Merged
merged 1 commit into from
Nov 24, 2020
Merged

Add exploit mitigations chapter to the rustc book #76858

merged 1 commit into from
Nov 24, 2020

Conversation

rcvalle
Copy link
Member

@rcvalle rcvalle commented Sep 18, 2020

This section documents the exploit mitigations applicable to the Rust compiler when building programs for the Linux operating system on the AMD64 architecture and equivalent. This was done for a project I'm currently working on, and I hope it'll also be helpful to others.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2020
src/doc/rustc/src/exploit-mitigations.md Outdated Show resolved Hide resolved
The Rust compiler supports stack clashing protection via stack probing, and
enables it by default since version 1.20.0 (2017-08-31)[26]–[29].

![Screenshot listing cross references to __rust_probestack in hello-rust.](images/image1.png "Cross references to __rust_probestack in hello-rust.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use posix tools output like objdump, readelf instead of ghidra or ida screenshot ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know of any common Linux tool that can list cross references to addresses/symbols. I could perhaps use Radare2, but it also isn't a common Linux tool, and it would be the same as using Ghidra or IDA Pro.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree there is no such a general CLI tool could do that.
Could you list the toolname in the image description ?
Or have a small section about tools used.

Comment on lines 164 to 168
The Rust compiler supports integer overflow checks, and enables it when
debug assertions are enabled since version 1.0.0 (2015-05-15)[14]–[17], but
support for it was not completed until version 1.1.0 (2015-06-25)[16]. An
option to control integer overflow checks was later stabilized in version
1.17.0 (2017-04-27)[18]–[20].
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the history, but I don't think it is needed to including this.
We want to follow what current nightly do.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, in general, all docs are written "in the present", and we don't include specific version numbers.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can remove these. However, this entire document is written as a historical account of these exploit mitigations' major milestones for Rust, and removing these will make this part inconsistent with the rest of the document. Let me know if you still prefer I remove these.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we don't write docs in that style. That doesn't mean it's not useful to have, it's just not the way that the documentation is organized.

@@ -0,0 +1,583 @@
# Exploit Mitigations

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a target audience section ? Or it applies to everybody interested.

Copy link
Member Author

Choose a reason for hiding this comment

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

The original document was initially targeted at security and software engineers working on the project I'm currently working on, but later was opened to everyone interested. I think having it open to everyone interested would make sense here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can also add the following target audience and prior knowledge similarly to how it is in the original document:

This chapter describes exploit mitigations supported by the Rust compiler, and is by no means an extensive survey of the Rust programming language’s security features.

This chapter is for software engineers working with the Rust programming language, and assumes prior knowledge of the Rust programming language and its toolchain.

I removed it because I didn't see any other chapters containing a target audience and prior knowledge. What do you think?

The Rust compiler supports stack clashing protection via stack probing, and
enables it by default since version 1.20.0 (2017-08-31)[26]–[29].

![Screenshot listing cross references to __rust_probestack in hello-rust.](images/image1.png "Cross references to __rust_probestack in hello-rust.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree there is no such a general CLI tool could do that.
Could you list the toolname in the image description ?
Or have a small section about tools used.

@steveklabnik
Copy link
Member

I'd like someone from @rust-lang/compiler to approve this PR. I don't have any more feedback, but ultimately, this is about their part of the project, and it's a big addition.

Sorry for this taking so long, @rcvalle .

Copy link
Member

@alex alex left a comment

Choose a reason for hiding this comment

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

I think this is a great survey of what exists... however I'm a bit concerned a reader might take teh wrong conclusions away from this. Specifically, the document doesn't really describe how effective any of these mitigations are, and indeed the history of C is a history of people claiming a mitigation will solve memory unsafety and then it failing to do so.

To address this, I'd suggest that the introduction flag this, rather than try to document how effective each mitigation is.

src/doc/rustc/src/exploit-mitigations.md Outdated Show resolved Hide resolved
@rcvalle
Copy link
Member Author

rcvalle commented Oct 15, 2020

I'd like someone from @rust-lang/compiler to approve this PR. I don't have any more feedback, but ultimately, this is about their part of the project, and it's a big addition.

Sorry for this taking so long, @rcvalle .

Np at all. Thank you @steveklabnik and others for their time!

@rcvalle
Copy link
Member Author

rcvalle commented Oct 15, 2020

I think this is a great survey of what exists... however I'm a bit concerned a reader might take teh wrong conclusions away from this. Specifically, the document doesn't really describe how effective any of these mitigations are, and indeed the history of C is a history of people claiming a mitigation will solve memory unsafety and then it failing to do so.

To address this, I'd suggest that the introduction flag this, rather than try to document how effective each mitigation is.

What do you think about the following:

This document does not discuss the effectiveness of these exploit mitigations as they vary greatly depending on several factors besides their design and implementation, but rather describe what they do, so their effectiveness can be understood within a given context.

Which is why this document does not discuss the effectiveness of these exploit mitigations.

@alex
Copy link
Member

alex commented Oct 15, 2020 via email

@jyn514 jyn514 added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Oct 30, 2020
@camelid camelid added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 30, 2020
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Is this waiting on anything? It looks good to me :)

src/doc/rustc/src/exploit-mitigations.md Outdated Show resolved Hide resolved
@Dylan-DPC-zz
Copy link

waiting for another review from @steveklabnik after recent changes

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 20, 2020
Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

Sorry, I did not realize this was blocked on me!

@steveklabnik
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Nov 20, 2020

📌 Commit 9999616 has been approved by steveklabnik

@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 Nov 20, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 21, 2020
…ns, r=steveklabnik

Add exploit mitigations chapter to the rustc book

This section documents the exploit mitigations applicable to the Rust compiler when building programs for the Linux operating system on the AMD64 architecture and equivalent. This was done for a project I'm currently working on, and I hope it'll also be helpful to others.
@m-ou-se
Copy link
Member

m-ou-se commented Nov 21, 2020

This failed in #79252 on x86_64-apple: https://github.com/rust-lang-ci/rust/runs/1434709746

doc tests for: /Users/runner/work/rust/rust/src/doc/rustc/src/exploit-mitigations.md

running 3 tests
test /Users/runner/work/rust/rust/src/doc/rustc/src/exploit-mitigations.md - Exploit_Mitigations::Exploit_mitigations::Integer_overflow_checks (line 180) ... ok
test /Users/runner/work/rust/rust/src/doc/rustc/src/exploit-mitigations.md - Exploit_Mitigations::Exploit_mitigations::Stack_clashing_protection (line 280) ... ok
test /Users/runner/work/rust/rust/src/doc/rustc/src/exploit-mitigations.md - Exploit_Mitigations::Exploit_mitigations::Heap_corruption_protection (line 381) ... FAILED

failures:

---- /Users/runner/work/rust/rust/src/doc/rustc/src/exploit-mitigations.md - Exploit_Mitigations::Exploit_mitigations::Heap_corruption_protection (line 381) stdout ----
Test executable succeeded, but it's marked `should_panic`.

failures:
    /Users/runner/work/rust/rust/src/doc/rustc/src/exploit-mitigations.md - Exploit_Mitigations::Exploit_mitigations::Heap_corruption_protection (line 381)

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 21, 2020
@jyn514
Copy link
Member

jyn514 commented Nov 24, 2020

@rcvalle do you mind squashing the commits a little? There are instructions at https://rustc-dev-guide.rust-lang.org/git.html#rebasing.

@rcvalle
Copy link
Member Author

rcvalle commented Nov 24, 2020

@rcvalle do you mind squashing the commits a little? There are instructions at https://rustc-dev-guide.rust-lang.org/git.html#rebasing.

Not at all. Do you prefer me to squash them all into a single commit?

@jyn514
Copy link
Member

jyn514 commented Nov 24, 2020

However many you think makes sense - for a PR this big, I think up to 5 is fine. I just think changes like 'Remove deadwood from sentence' don't need to be their own commit, since they aren't particularly valuable for telling you why the code is a certain way.

This section documents the exploit mitigations applicable to the Rust
compiler when building programs for the Linux operating system on the
AMD64 architecture and equivalent.
@rcvalle
Copy link
Member Author

rcvalle commented Nov 24, 2020

However many you think makes sense - for a PR this big, I think up to 5 is fine. I just think changes like 'Remove deadwood from sentence' don't need to be their own commit, since they aren't particularly valuable for telling you why the code is a certain way.

I think most of them didn't need a separate commit, so I just squashed them all.

@jyn514
Copy link
Member

jyn514 commented Nov 24, 2020

@bors r=steveklabnik rollup

@bors
Copy link
Contributor

bors commented Nov 24, 2020

📌 Commit 5b1cb0e has been approved by steveklabnik

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 24, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 24, 2020
…as-schievink

Rollup of 10 pull requests

Successful merges:

 - rust-lang#76858 (Add exploit mitigations chapter to the rustc book)
 - rust-lang#79310 (Make `fold_item_recur` non-nullable)
 - rust-lang#79312 (Get rid of `doctree::Impl`)
 - rust-lang#79321 (Accept '!' in intra-doc links)
 - rust-lang#79346 (Allow using `-Z fewer-names=no` to retain value names)
 - rust-lang#79351 (Fix typo in `keyword` docs for traits)
 - rust-lang#79354 (BTreeMap: cut out the ceremony around BoxedNode)
 - rust-lang#79358 (BTreeMap/BTreeSet: make public doc more consistent)
 - rust-lang#79367 (Allow disabling TrapUnreachable via -Ztrap-unreachable=no)
 - rust-lang#79374 (Add note to use nightly when using expr in const generics)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8fde4be into rust-lang:master Nov 24, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet