Improve clippy security lints

[tarcieri]

clippy is a Rust linting tool designed to detect common Rust mistakes and provide helpful suggestions for how to improve code. A list of the lints it presently supports is here:

https://rust-lang.github.io/rust-clippy/master/

Among these are "restriction" (opt-in) security lints! These are not (upon cursory inspection) presently leveraged for security, but clippy's original author @manishearth has suggested they could be and patches for security-related lints are welcome (potentially including lints for popular ecosystem crates!). There's ample precedent for detecting similar unsafe patterns through AST analysis in other languages, so extending clippy with restriction lints for security seems worth pursuing.

Below is a table of lints taken from a similar tool, the gosec project, which operates using clippy-like AST analysis. These are intended as food for thought, and some may not make sense as clippy lints or for Rust in general (I've removed ones which audit unsafe and things like #![must_use]), but seem like a reasonably good starting point for figuring out which ones would be most valuable for clippy:

General

	Name	Description
⬜	Hard-coded credentials	Hardcoded passwords, crypto keys, or other secrets
⬜	Restrict net binding	Network listeners which bind to all interfaces
⬜	Bignum usage patterns	Unsafe bignum usage patterns
⬜	Command execution	Unsafe command execution patterns (e.g. shell escaping)
⬜	Directory permissions	Avoid creating directories with overly broad permissions
⬜	File permissions	Avoid creating or setting overly broad file permissions
⬜	Temporary files	Enforce safe temporary file patterns (via tempfile crate?)
⬜	Attacker-controlled paths	Avoid opening files based on attacker-controlled paths

Crate-specific

	Crate	Name	Description
⬜	askama / tera ?	HTML escaping	Disallow attacker-controlled unescaped data
⬜	diesel	SQL query construction	Disallow unescaped attacker-controlled parameters in SQL queries
⬜	hyper?	Attacker-controlled URLs	Disallow attacker-controlled URLs
⬜	md5, md-5, sha1, sha-1, rust-crypto	Broken crypto	Detect use of broken cryptographic primitives (and/or unmaintained crypto crates?)
⬜	openssl / rustls	TLS Settings	Detect bad TLS connection settings
⬜	ssh2?	Host key verification	Ensure SSH host keys are verified
⬜	tar / zip?	Directory traversal	Avoid unsafe usage patterns that could result in directory traversal attacks

Please let me know if you think some of these should definitely be added to clippy, or if ones don't make sense or are otherwise out-of-scope and I will update the table accordingly.

If there's agreement on some high priority ones to work on, I think the next step is to create specific issues about them on https://github.com/rust-lang/rust-clippy i.e. this issue is just for discussion and to get the ball rolling, and after that we should move things over to the clippy repo proper.

[tarcieri]

@manishearth any advice on what constitutes a crate significant enough to be worthy of a clippy lint?

The above list of crate-specific lints is my attempt at mapping some of the gosec lints to the Rust ecosystem. In Go the corresponding functionality for these is part of the standard library.

As a quick case study: ssh2. It's a commonly used security protocol, with a crate that's a wrapper for the de facto standard C library, and authored by a Rust core team member. That said, it only has 20,000 downloads. Is it important enough to warrant clippy lints for insecure usage patterns?

--

Another thing worth noting is several gosec lints are based on taint analysis. In discussing this with @manishearth we agreed that there might be some easier ways of accomplishing the goal of avoiding attacker-controlled parameters without complicated clippy lints that require a global analysis, like linting that values are either literals or computed via const fn. I'm curious to see if this is sufficient for most use cases, or if a more complicated analysis is required.

[Manishearth]

Uh, to be clear, y'all probably don't want restriction lints, restriction lints are for things which aren't necessarily "bad" but you may have a reason to turn them off for a subset of code.

You want correctness and pedantic lints (the former is opt-out). We could also add a new opt-out "security" category (and use pedantic for the opt in ones), since correctness lints have a high bar.

Not really sure what the bar for crates should be, but these all seem plausible crates to lint about. One way to prevent dependence on crates is to make the lint about attributes (which any crate can use to mark their functions)

If you're going to be filing issues, be sure to provide low level explanations of what the lint should be doing.

Cc @oli-obk @phansch @llogiq

[tarcieri]

We could also add a new opt-out "security" category

That sounds great to me

[Shnatsel]

We should also lint about code that tends to be slow and get refactored into unsafe code for performance reasons, even though a safe alternative is available.

See rust-lang/rust-clippy#3237 for an example. This lint was created in the wake of RUSTSEC-2018-0004. #19 should be a good source of more anti-patterns to warn about.

[Shnatsel]

Also, a static analyzer for Rust that's focused on taint analysis is in development: https://github.com/facebookexperimental/MIRAI

[tomtau]

btw, to support this effort, a bounty was put up here: https://www.bountysource.com/issues/68733714-improve-clippy-security-lints

As this will be rolled out across many small issues on https://github.com/rust-lang/rust-clippy by different people, they could all possibly come back to this issue and claim their part:

https://github.com/bountysource/core/wiki/Frequently-Asked-Questions#can-several-people-claim-the-same-bounty

but if it is preferred to move the bounty somewhere else, we could possibly contact the BountySource support to do so.

[oli-obk]

Not technically security lints, but some of the MISRA-C lints apply to Rust, too. This is tracked in https://github.com/rust-lang/rust-clippy/issues/2227

[Shnatsel]

Here's an actual bug that could potentially be discovered by a static analyzer: https://github.com/crepererum/rdxsort-rs/pull/2

Any thoughts on whether linting for this is viable?

[Manishearth]

Yep, please file an issue on clippy listing what you need there.

Though there are plenty of reasons why that may be useful (e.g. relocating a buffer) so it may have to go under the pedantic category. As it stands for most clippy lints we want them to not have too many false positives.

[Shnatsel]

https://github.com/rust-secure-code/safety-dance/issues/21 is working in that direction

[Shnatsel]

First big work item filed: https://github.com/rust-lang/rust-clippy/issues/4483

I expect much more requests to come out of safety-dance effort.

[Shnatsel]

Also just filed https://github.com/rust-lang/rust-clippy/issues/4484

[Shnatsel]

I'll keep any further lint requests I file to https://github.com/rust-secure-code/safety-dance/issues/21 to avoid spamming people.

[alex]

Another one, that came out of seeing rustsec reports: https://github.com/rust-lang/rust-clippy/issues/6638