-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Mitigation enforcement #3855
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
base: master
Are you sure you want to change the base?
Mitigation enforcement #3855
Conversation
2a83ea5
to
52f7403
Compare
FYI: Your rendered link doesn't work because you updated the filename in the URL to account for the PR number, but didn't actually update the filename in the code itself. |
52f7403
to
1305c42
Compare
Fixed |
text/3855-mitigation-enforcement.md
Outdated
For example, with `-C stack-protector`, the compatibility table will be | ||
as follows: | ||
|
||
| Base\Child | none | none-noenforce | strong | strong-noenforce | all | all-noenforce | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use the words "dependency" instead of "child" here and above? Child is not a word we generally use for crate relationships which makes it a bit confusing
and vulnerabilities. | ||
|
||
Mitigations are generally enabled by passing a flag to the compiler (for | ||
example, [`-Z harden-sls`] or [`-Z stack-protector`]). If the compilation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You always talk about stack-protector here, since that's your primary motivation of course, but I think it would be good to more explicitly list out all kinds of mitigations that Rust has it that people would like Rust to have in the future, to ensure that this makes sense for all of them (for example, the ones from https://doc.rust-lang.org/nightly/rustc/exploit-mitigations.html#exploit-mitigations-1).
I would especially be interested in whether there are existing stable mitigations that would like to make use of this, especially if it has a flag to toggle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some exploit mitigations that would benefit from it (e.g., CFI, and including most in the https://doc.rust-lang.org/nightly/rustc/exploit-mitigations.html#exploit-mitigations-1) precedes the Target Modifiers feature (which was intended to also solve this), but I don't think there are any stable exploit mitigations except maybe -C control-flow-guard
.
Some projects may already have tooling to check certain things are as expected, e.g. |
I mentioned hardening-check. If you have experience with objtool, you can add that as well. I also couldn't find any documentation for objtool used as a hardening check tool, so if you could provide me some I would try to include it. |
The docs are here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/objtool/Documentation/objtool.txt I mentioned |
So it looks like it works on a per- Is that right?
I do think that the big difference is "tool that works on .o files vs. tool that works on executables", since if the tool needs |
3af6d94
to
3f7188b
Compare
I don't think the
Anyway, none of the above really matters -- I mentioned Thanks for working on this! |
If there was a "magic" analyzer that would reliably do the sanitizer enforcement, there would be much less need for it as a compiler flag. As far as I can tell, the existing analyzers either have large holes or require significant project-specific intervention. That of course does not mean they are not useful, only that they don't automatically solve the problem for everyone. |
Not sure if there is a disagreement here, but just in case: I didn't claim there is a "magic" analyzer out there solving this. Quite the contrary -- I said that even if such a tool existed that covered everything, having an independent check at another layer like this RFC proposes would still be useful. The |
library only comes with a single set of enabled mitigations per target. | ||
|
||
Mitigation enforcement should be disableable by the end-user via a compiler | ||
flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's be cool if this was somehow per dependency, like maybe you'd add some disable-mitigation-enforcement = {std}
into the binary's Cargo.toml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's be cool if this was somehow per dependency, like maybe you'd add some disable-mitigation-enforcement = {std} into the binary's Cargo.toml.
You probably want something in the style of -C allow-partial-mitigations=stack-protector=std+alloc+core
, so you know which mitigations you are allowing.
(Of course, with also a syntax in Cargo, which should come with a separate RFC I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added that to alternatives
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying crate names from the sysroot is very problematic since std has about 10 different dependencies that would need to be specified and that are not stable.
What would we desired for this use case of allowing the sysroot and nothing else would be special syntax to allow it only in the sysroot. A list of crate names would not help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would we desired for this use case of allowing the sysroot and nothing else would be special syntax to allow it only in the sysroot. A list of crate names would not help.
Do you think that special casing core
(or @core
or something) to apply to the entire sysroot would work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's be cool if this was somehow per dependency, like maybe you'd add some disable-mitigation-enforcement = {std} into the binary's Cargo.toml.
You probably want something in the style of
-C allow-partial-mitigations=stack-protector=std+alloc+core
, so you know which mitigations you are allowing.
This would still be useful for crates outside std, for example, if a project experiences a mitigation "failure" in a non-security critical crate or feature branch.
A summary of the reasoning behind explicitly stating that it is allowed for the Rust compiler to accept a mitigation might be applied partially:
For solving this, I'm in favor of the simpler For stack smashing protection specifically (which is beyond the scope of this RFC, but relevant to choosing the right approach):
For this, I propose either:
For (2), we could perform a comprehensive set of tests for binary size, build time, and run time performance and make a decision based upon the ROI (considering the returns are different from a program written in C or C++ compared to a program written in Rust). |
Objtool is mainly designed to validate the expected construction of the (x86) kernel's functions and related metadata (e.g. "can we always unwind the stack correctly?" or "what things are reachable for indirect calls?") As far as the mitigation enforcement idea as presented, I like the idea of having this be a declared compatibility thing to check. In C it is trivial to mix and match different mitigations and the resulting binary is very hard to analyze after the fact (see |
I do think there is an interesting middle point where we ship a In any case, it should not be relevant to this RFC. |
I also much prefer this approach to having many
|
leverage an already-existing memory vulnerability into ROP execution, even | ||
if the memory vulnerability is in a completely different part of the code than | ||
the part that has the mitigation disabled | ||
4. For "local" mitigations (e.g. stack protector, or C's `-fwrapv` - which I don't think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The equivalent of -fwrapv
in Rust is -C overflow-checks=off
. And if you're depending on it rather than one of the other alternatives, you probably want to make sure it's enabled everywhere that isn't using one of those alternatives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not possible to have the Rust equivalent of -fno-wrapv
(undefined behavior on signed integer wrapping).
-C overflow-checks=on
is more equivalent to -ftrapv
. Do people feel that there is a need to set it to enforcing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand it doesn't quite fit the model here, but I've written code where we wanted to panic rather than wrapping, because wrapping meant we'd generated an incorrect value.
I've also written code where we relied on the guaranteed wrapping behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll say this (enforcing for overflow checks) is off scope for the current RFC. There is no reason to do it in the "main" enforcing pulse. Feel free to open another RFC/FCP/whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it's not really a security mitigation in the classic sense and shouldn't be enforced.
FWIW, relying on wrapping behavior is incorrect, as Rust always considers overflow to be a bug, it just doesn't always check it. Use wrapping if you need wrapping.
text/3855-mitigation-enforcement.md
Outdated
turned on, and one of the dependencies does not have that mitigation turned | ||
on (whether enforcing or not), a compilation error results. | ||
|
||
If a mitigation has multiple "levels", a lower level at a dependent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might need to be clarified. Do you mean a stricter or less strict level in the dependent crate?
It might also be helpful to use the same dependency terms as the previous paragraph (current and dependency). Edit: or change that paragraph to use base and dependent?
Made We can probably change our choice over an edition boundary if we want (can we?) |
Zulip: https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Mitigation.20enforcement.20.28.60-C.20allow-partial-mitigations.60.29/with/539293124
Rendered