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

unstable-book: split sanitizers into testing and production ones #108942

Closed
wants to merge 5 commits into from

Conversation

disconnect3d
Copy link

The Rust Unstable Book lists compiler sanitizers that can be used in Rust programs. However, it does not say whether it is okay to use certain sanitizers on production or not, which may suggest that its okay to do so.

I believe that none of ASAN/TSAN/MSAN/LeakSAN should be used on production.

There was an old thread on oss-security that provided more details here: https://www.openwall.com/lists/oss-security/2016/02/17/9 but one example to not use those sanitizers on production is the fact that some of them use environment variables that control things like "path to the binary that will be used to get the symbol names for stack traces". As a result, if a sanitized binary has suid, an attacker can use the specific environment variable to run their own program and escalate their privileges this way.

The Rust Unstable Book lists compiler sanitizers that can be used in Rust programs. However, it does not say whether it is okay to use certain sanitizers on production or not, which may suggest that its okay to do so.

I believe that none of ASAN/TSAN/MSAN/LeakSAN should be used on production.

There was an old thread on oss-security that provided more details here: https://www.openwall.com/lists/oss-security/2016/02/17/9 but one example to not use those sanitizers on production is the fact that some of them use environment variables that control things like "path to the binary that will be used to get the symbol names for stack traces". As a result, if a sanitized binary has suid, an attacker can use the specific environment variable to run their own program and escalate their privileges this way.
@rustbot
Copy link
Collaborator

rustbot commented Mar 9, 2023

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

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

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

r? rust-lang/compiler

@rustbot rustbot assigned Noratrieb and unassigned GuillaumeGomez Mar 9, 2023
@Noratrieb
Copy link
Member

Thanks, adding some disambiguation is great! I think this is correct but I'm not entirely sure, I'd rather let someone who knows more about this approve it.
r? compiler

@rustbot rustbot assigned eholk and unassigned Noratrieb Mar 9, 2023
@eholk
Copy link
Contributor

eholk commented Mar 22, 2023

This change is probably fine, although I think it might be a little out of scope for the Rust Unstable Book. It seems reasonable for us to say "these are sanitizers you can use" and direct people to further documentation on those sanitizers to evaluate whether it makes sense to use it in production or just testing.

@disconnect3d
Copy link
Author

Maybe we should have then a sentence or two at the beginning explaining what a sanitizer is, that would explain something along the lines that they are compiler plugins which enable additional instrumentation of the code that may be useful for testing/security testing and sometimes for production, depending on the sanitizer.

@apiraino
Copy link
Contributor

I am personally in favor of a little bit or verbiage (maybe with a link?) explaining what a sanitizer is.

@disconnect3d can you update this patch with a proposal?

@eholk
Copy link
Contributor

eholk commented Jul 6, 2023

Marking this as waiting on author so @disconnect3d can resolve conflicts and add a small note about what a sanitizer is.

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2023
@Dylan-DPC
Copy link
Member

@disconnect3d any updates on this? thanks

@disconnect3d
Copy link
Author

@apiraino @eholk @Dylan-DPC Updated!

@rust-log-analyzer

This comment has been minimized.

@apiraino
Copy link
Contributor

apiraino commented Aug 9, 2023

thanks @disconnect3d . As mentioned in a previous comment, I'll switch to waiting on a review to signal that this PR is ready for a review.

(For the record, merge commits are not allowed in PRs -- see https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy)

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 9, 2023
@Dylan-DPC Dylan-DPC 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 20, 2023
@Dylan-DPC
Copy link
Member

@disconnect3d can you remove the merge commit and rebase instead? After that we will get it reviewed

@JohnCSimon
Copy link
Member

@disconnect3d
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Feb 11, 2024
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Feb 11, 2024
saethlin added a commit to saethlin/rust that referenced this pull request Feb 20, 2024
unstable-book: Separate testing and production sanitizers

This is a redo of [this PR](rust-lang#108942). Left the commit as before (except for reflowing to 80-width), since it already got approved.
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Feb 20, 2024
unstable-book: Separate testing and production sanitizers

This is a redo of [this PR](rust-lang#108942). Left the commit as before (except for reflowing to 80-width), since it already got approved.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2024
Rollup merge of rust-lang#121195 - D0liphin:master, r=ehuss

unstable-book: Separate testing and production sanitizers

This is a redo of [this PR](rust-lang#108942). Left the commit as before (except for reflowing to 80-width), since it already got approved.
@disconnect3d
Copy link
Author

@JohnCSimon just to be sure, how do I reopen this?

lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
unstable-book: Separate testing and production sanitizers

This is a redo of [this PR](rust-lang/rust#108942). Left the commit as before (except for reflowing to 80-width), since it already got approved.
@JohnCSimon JohnCSimon reopened this Apr 21, 2024
@JohnCSimon JohnCSimon closed this Apr 21, 2024
@JohnCSimon JohnCSimon reopened this Apr 21, 2024
@JohnCSimon
Copy link
Member

@disconnect3d
image

I clicked "reopen pull request"

RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
unstable-book: Separate testing and production sanitizers

This is a redo of [this PR](rust-lang/rust#108942). Left the commit as before (except for reflowing to 80-width), since it already got approved.
@JohnCSimon JohnCSimon removed the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label May 26, 2024
@oskgo
Copy link
Contributor

oskgo commented Aug 9, 2024

It seems like these changes were merged in #121195, so I'll close this.
Thanks for your contribution.

@oskgo oskgo closed this Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.