[Pre-RFC] Cargo Safety Rails

[alevy]

but it seems to be what is being encouraged here.

It’s not what’s being encouraged. What’s being encouraged (by me anyway), is a mechanism to surface dependencies (importantly, they may not be direct) where if you choose to incorporate them, you may want either audit them for soundness violations or just choose to trust the author (which may be totally reasonable too, in the same way that most people don’t audit the Rust compiler, but just Trust The Process). Thankfully that’s fairly easy in the cases you pointed out—small, self-contained, innocuous.

If you care so much about safety/quality that you would check every dependent crate for unsafe code, you should almost certainly be checking them all for logic bugs too.

This is a false dichotomy. While it’s of course true that soundndess/type violations are not the only source of bugs, but a caller can guarantee very strong properties about isolation if they know the callee cannot violate type safety (given the right software architecture of course). This is true in plenty of cases where the caller either doesn’t care if functionality is correct, or they can verify functionality enough with testing (while we can test expected cases, testing for adversarial inputs is much harder, which is one reason type-safety is so useful in the first place).

There is a long history of research using precisely this kind of safety to isolate extensions (Spin, Singularity, Jif, all the IFC work in Haskell, etc) as well as practical examples in your exact area of expertise—browser-based JavaScript applications (the JavaScript API exposed through the DOM simply doesn’t have an unsafe equivalent, but that’s just a different mechanism).

As I mentioned in a previous response, I think a totally reasonable response to this is “get your hands out of my package manager, make your own if that’s what you want” (though I’ll be offended if you actually say it like that). Maybe you think that as well, but I think it’s a very different argument than dismissing the utility of enforcing type-safety at the package level all together.

[nrc]

It’s not what’s being encouraged.

I can see that it is not what is intended to be encouraged, but I worry that it might be encouraged anyway

This is a false dichotomy…

I think you are assuming this facility will only be used by experts, or at least people who understand these issues as well as you do. I worry about the ‘gamification effect’ where people want a ‘no unsafe’ ‘badge’ for the sake of it.

OK, so let me suggest an alternative since I believe the goal here is reasonable, but I have been negative about the suggested solution: I would like to see a tool implemented as a cargo plugin (i.e., you run it with cargo unsafe-audit or something) which looks at every crate (including deps) and gives you a nice JSON output of every unsafe block, function, and impl, with the crate, file, and line it occurs (based on the AST, not grep so we’d see through procedural macros, etc). So with some nice frontend you can see which crates have no unsafe code and which have a little or a lot. So when you do an audit of your code, it makes it easy to find the unsafe code, but it makes it harder to make a naive ‘unsafe bad, no unsafe good’ judgement about a crate.

[phaylon]

I would like to see a tool implemented as a cargo plugin (i.e., you run it with cargo unsafe-audit or something) which looks at every crate (including deps) and gives you a nice JSON output of every unsafe block, function, and impl, with the crate, file, and line it occurs (based on the AST, not grep so we’d see through procedural macros, etc).

To me this feels more likely to have negative side effects. It’s hard to judge unsafe code by itself. You’d usually have to see it in its wider context to make sure it is safe. Usually when it depends on some value or conditional that is outside of the direct scope of the unsafe.

I do find the original proposal appealing, because I’m still feeling a lot more comfortable grasping safe Rust than unsafe Rust, and thus auditing it. There is (and probably will always be) a difference about the dangers sleeping in the safe and unsafe parts. I can understand things when it’s just about using unsafe functions to achieve a goal that is aligned with the functions definitions. But I wouldn’t dare yet judging something dealing with direct memory access, inline assembler, and things on that level.

So for me the big plus would be noticing the switch from “safe” to “unsafe” in a dependency. Because right now (and I assume others will be at this point in the future as well) it is equal to “auditable for me” switching to “possibly need to defer judgement to others”.

But I’ll admit I would be more likely to trust a safe crate due to the above than an unsafe one.

[est31]

If you care so much about safety/quality that you would check every dependent crate for unsafe code, you should almost certainly be checking them all for logic bugs too.

I’d say its dependent on the use case.

If you have a security library like maybe openssh or openssl then almost every logic bug is automatically also a security bug. Similar for operating system kernels. And for web services. If you write a web server in Rust, you can expose the entire user database with logic bugs or allow people to log in as admin. There are many examples of logic bugs in websites affecting security, like this one. Safe Rust doesn’t prevent that.

However there are other use cases for Rust. Think of media players or image viewers. These tools need to be able to handle untrusted data, and display it to the user. Here there is a big difference between logic bugs, which in the worst case give you a black/corrupted image, and memory safety bugs, which may give you a “your files have been encrypted” message. The only places where memory safety bugs can be introduced are inside the unsafe blocks, so it does make sense to minimize them, and if possible, provide an option to get rid of them completely.

As an example, think of the nautilus thumbnail bug which gained code execution privileges by exploiting memory unsafety, only from viewing a directoy with a prepared file in the file explorer. There are definitely parts of a file explorer where you might have security relevant logic bugs, but for displaying thumbnails, there is no real logic bug which is of relevance for security, at least not directly (you can use a wrong thumbnail to convince people that the file is something else than it really is, but that is a far less severe security issue than having an attacker user execution privileges on your machine), while all memory safety bugs are relevant for security. If it had been all safe Rust, it wouldn’t have happened. If it had been mostly safe Rust with unsafe components, it may have happened.

Sure, a malicious crate author might also add malicious code to an image decoding library but that assumes actual malicious intent from the author which is much less prevalent and can be recognized far more easily during review than susceptibility to doing mistakes when writing unsafe code (which affects most people, sadly). If I wanted to hide a security vulnerability, I would do it in unsafe code, not in safe code.

Unsafe code also makes logic bugs worse. Sometimes unsafe code has conditions under which it works safely, and if those are violated, safety is as well.

To summarize, being cautious which libraries get allowed to use unsafe is for many use cases no way “security theater” but a legitimate strategy to minimize attack surface.

[kornel]

Stigmatizing unsafe is based on flawed premises that code using unsafe is not trustworthy, and that code without unsafe is safe.

The last critical security vulnerability in ImageMagick was caused by passing unsanitized arguments to command-line helpers, and such vulnerability is also possible in Rust. Rust is not a sandbox.

[phaylon]

Then why have the unsafe keyword at all? If it has no safety implications above normal code, and if there’s no need to review unsafe code differently than normal one.

Code using unsafe is less trustworthy to me. Because I cannot trust it, because I don’t have the experience to trust it.

I feel like there’s a bit of a rift in the community about the safety/unsafety aspect of Rust. I’ve been reading statements similar to “as long as you don’t use unsafe, you should not run into memory safety issues in Rust” multiple times in the past, together with statements about the awfulness of memory safety errors.

[jimuazu]

There are different levels of unsafe use. After reading about all the ways you can get burned using unsafe, it’s quite clear to me that I don’t want to use any crate written by a novice and using unsafe. But standard library unsafe use we hope is sound (although there have been cases in the past where that wasn’t true). At the other extreme we have C code automatically converted to unsafe Rust which is probably as bad as the original (or worse) in safety terms. So a “uses unsafe” label doesn’t give enough information. It requires a human to evaluate how unsafe is used. A simple label doesn’t really help.

[phaylon]

@jimuazu But, wouldn’t the first part of your comment imply that a “doesn’t use unsafe” mark would be helpful?

[jimuazu]

@phaylon, yes along with a heuristic for detecting novices! (or automatically translated code).

Almost every crate uses standard library unsafe code directly or indirectly. So all crates should have a “uses unsafe” label. So to avoid that means ‘blessing’ certain blocks of unsafe code as trusted, e.g. all standard library code. So we’re back to different levels of unsafe. If someone wants to go around auditing unsafe blocks and ‘blessing’ them, then we could have a “doesn’t use any unaudited unsafe blocks”. I think that’s the best you could do. I would also like to avoid people getting an ‘unsafe’ allergy and ending up avoiding really good external crates that make very careful use of unsafe, and whose use has been well-audited.

[phaylon]

But, as you said, the trust towards the standard library is different from the trust one has in any random crate. For example: I’m fine with std’s Vec using unsafe. I’m fine with crates using std’s Vec, but I would be weary of a crate implementing their own Vec using unsafe.

[jimuazu]

So, Rayon’s an external crate. It uses plenty of unsafe. But it’s written by someone who knows his stuff. Are you going to bless that too? And then what else?

In then end we end up needing some authority to go around auditing things to make a simple label actually meaningful. Otherwise we need something much more detailed than a label, like a report of all the unsafe use in a crate and dependencies that the developer needs to read over and assess for themselves, like @nrc suggested.

[phaylon]

So, Rayon’s an external crate. It uses plenty of unsafe. But it’s written by someone who knows his stuff. Are you going to bless that too? And then what else?

Yes, I’d make the decision specifically for the rayon crate that I have heightened trust in it and am fine with it doing unsafe things. This is based on: the fact that I have heightened trust in the developers; that rayon is a well-known crate with many eyes on it; that it is wide-spread enough that if there is an issue with unsafe that I run into, there’s a good chance I’ll find someone who’ll be able to help. This things aren’t true for me for any random crate, and it’s an individual per-crate decision.

I actually do check out code from crates I’m interested in to see if there’s any unsafety going on. So in a sense, I’m already doing what a flag would be helping me with. I’m just doing it manually all the time, probably missing things, and likely to not notice if a crate that previously did mundane things suddenly starts using unsafe.

[kornel]

Then why have the unsafe keyword at all? If it has no safety implications above normal code

I did not say that. I mean that code using unsafe may be trustworthy. Mere presence of unsafe doesn’t mean a crate is unsafe, and absence of unsafe doesn’t mean a crate is safe, so banning of unsafe keyword is a very blunt auditing tool that will have false positives and false negatives.

[Eh2406]

Ok, so I am confused hear. What is the threat model this would protect us from? Who is doing what to whom, and how would this help?

[kornel]

I think this feature is a blunt tool. It’s going create a lot of wasted effort, and overall it will be ineffective method of ensuring safety of Rust dependencies.

• Making “is it safe?” decisions individually by each user for each Cargo.toml creates a lot of fragmented and duplicated work (it confuses trust with safety; one is subjective and distributed; the other — in Rust terms — is an objective property of the code itself).

• Auditing of someone else’s code is hard, and a “drive-by” check is going to have low accuracy (not enough time and familiarity to spot hidden errors, and OTOH safe blocks of code may be unnecessarily rejected if they look scary to an outsider).

• In code reviews usually 1-line changes are picked apart, but 500-line changes get “LGTM”. The same effect is likely to make users just accept large crates or deep dependency trees and hope for the best.

So in the end it may just create a lot of work spent on micromanaging a moat in Cargo.toml, with shallow peeking at code and going with a hunch.

The effort could be better spent on shared community audits, like Libz Blitz, so that code is more carefully audited, with more in-depth look, and that effort is done once, not duplicated by every crate user.

And instead of blunt "has/doesn’t have unsafe" heuristic it may be better to use more specific heuristics like checking test coverage, static analysis tools, fuzzing, runtime sanitizers, etc.

[withoutboats]

I think I’m interpreting this very differently from other commenters. I would assume (since it would be a breaking change otherwise), that every crate is trusted by default. This would allow users to specify that they only trust certain crates to contain unsafe code, and don’t want other crates to introduce unsafety.

This seems eminently reasonable to me, since unsafe code is required to uphold complex invariants to guarantee that you don’t get undefined behavior. While there is obviously still the potential for logic bugs, a key goal of Rust is to eliminate a category of very high risk bugs, and that relies on all unsafe code in your dep graph upholding all of these invariants.

I could easily see projects deciding that they only trust std & rayon, for example. And I could see people wanting to be notified when unsafe code is introduced into a dependency when they upgrade, so that they can audit it and see if they actually want to upgrade.

Obviously its a blunt instrument, but how is that worse than no instrument? I don’t see where anyone has suggested we make all crates untrusted by default. This would be something users would opt into.

[est31]

I would assume (since it would be a breaking change otherwise), that every crate is trusted by default.

That matches my reading of the original proposal:

I’ve tried to come up with a design that doesn’t change the experience for Cargo users who wouldn’t use this feature (e.g. dependencies should be trusted by default).

And I very much agree with it, as much as I agree with the feature proposed here. Having deps untrusted by default would not just be a breaking change, such a check is also only useful for a subset of use cases. In many (most?) use cases it is not useful, and as many point out correctly, even harmful, so not even future epochs should make all dependencies untrusted by default. There is only a small subset of the cases Rust gets used in where there is a strong bidirectional “unsafe” <=> “may have vulnerabilities” relation, and such crates can opt into the feature.

However, it would be cool if leaf crates could opt in to treating all their dependencies as unsafe per default, except for the trusted exceptions they list in Cargo.toml. This would mean having to attach “untrusted” to each and every untrusted dependency.

[est31]

The last critical security vulnerability in ImageMagick was caused by passing unsanitized arguments to command-line helpers, and such vulnerability is also possible in Rust.

You mean this one? It is a vulnerability in binary tool components of ImageMagick, and the tool code of course is a different use case than a decoding/encoding library, which is supposed to be neutral of the means it gets data from (whether its an https download or a read from a file or whatever).

[russdamerell]

Ye, I see unsafe as merely a statement “caution - safeguards removed”. It merely highlights an area where handholding stops, not indicating it as bad.

[leocavalcante]

I think this is an awesome idea.

Using unsafe keywords doen’t automatically affirms that the code is not trustworthy, but IMHO the keyword exists exactly to allow and warn about some unsafe code and would be nice to have this track over dependency tree as well.

+1 for @konstin solution, but I would replace [security] for [safety] to avoid confusion.

[safety]
allowed_unsafe = ["foo"]

Also, maybe people are disagreeing just because this confusion. Unsafe code does’t mean it’s insecure. Marking your crate as unsafe doesn’t mean it is insecure.