Rust: Add cleartext transmission query

[paldepind]

Adds a cleartext transmission query. Large parts of this is heavily inspired or copy/pasted from the corresponding Swift query.

For now one kind of sink is modeled: URLs to requests created with the rekwest library.

[github-actions]

QHelp previews:

rust/ql/src/queries/security/CWE-311/CleartextTransmission.qhelp

Cleartext transmission of sensitive information

Sensitive information that is transmitted without encryption may be accessible to an attacker.

Recommendation

Ensure that sensitive information is always encrypted before being transmitted over the network. In general, decrypt sensitive information only at the point where it is necessary for it to be used in cleartext. Avoid transmitting sensitive information when it is not necessary to.

Example

The following example shows three cases of transmitting information. In the 'BAD' case, the transmitted data is sensitive (a credit card number) and is included as cleartext in the URL. URLs are often logged or otherwise visible in cleartext, and should not contain sensitive information.

In the 'GOOD' cases, the data is either not sensitive, or is protected with encryption. When encryption is used, ensure that you select a secure modern encryption algorithm, and put suitable key management practices into place.

func getData() {
	// ...

	// GOOD: not sensitive information
	let body = reqwest::get("https://example.com/song/{faveSong}").await?.text().await?;

	// BAD: sensitive information sent in cleartext in the URL
	let body = reqwest::get(format!("https://example.com/card/{creditCardNo}")).await?.text().await?;

	// GOOD: encrypted sensitive information sent in the URL
	let encryptedPassword = encrypt(password, encryptionKey);
	let body = reqwest::get(format!("https://example.com/card/{creditCardNo}")).await?.text().await?;

	// ...
}

References

• OWASP Top 10:2021: A02:2021 - Cryptographic Failures.
• OWASP: Key Management Cheat Sheet.
• Common Weakness Enumeration: CWE-319.

[geoffw0]

Query looks good to me. I've made some comments about specifics of the .qhelp that I think need to be tweaked a little, before we get a docs review.

I'd also like to see a greater diversity of sinks for this query, i.e MaD transmission sink models beyond just reqwest - though this can 100% be planned follow-up work rather than part of this initial PR.

[geoffw0]

Strictly speaking I think HTTPS URL parameters are encrypted as part of the HTTPS protocol - though there's a significant danger they get logged outside of that encrypted tunnel, e.g. in a cache or log, or possibly the connection isn't as secure as it seems (as described in the first reference linked from the .qhelp). So I think it is indeed BAD but we might want to rephrase our claim as to why? Alternatively, make the example http:// to avoid introducing confusion on this point?

[paldepind]

Oh, I actually thought the whole URL would be sent unencrypted. TIL.

Whether to change the description or use http://: I'm not actually entirely sure about the scope of CWE-311. Is sending a password in the URL of an HTTPS request within the scope given that is encrypted in transmission? Personally, I would think the query would be more useful if we just changed the description, as it definitely catches a real problem.

[geoffw0]

Oh I'm definitely happy that we catch these results, but in the .qhelp it's important we try to be as clear as possible about what the problem is and to recommend good practices. Passwords are a bit funny because for several reasons they usually require even more care than other kinds of sensitive information, which complicates recommending good practices if we use that as as example.

[geoffw0]

I'm happy with the changes you've made already, by the way. If you're happy to you can ask for a docs review.

[geoffw0]

A question I've been meaning to ask - when the model generator creates a model that we previously manually modelled, how do we know it's deducing it without reference to the existing manual model? i.e. how do we know it could still generate it without the manual model?

[paldepind]

That is a question that I've been asking myself and definitely something we should be aware of.

I think that in the specific case where we have a flow model for a function foo, then that wont be used when we derive models for foo, because the model applies when calling foo and the model generator tries to find a path inside the body of foo. So in that case the model for foo should be safe to delete.

But there might still be other siturations where there's many manual models, and where it's not clear from the generated output whether a manual model can be deleted or or. In that case I think the safest think to do is 1/ delete the manual model, 2/ delete the generated models and 3/ re-run the model generator and confirm that step 1 didn't change the generated models.

I also know that the model generation library has functionality where it's aware of manual models. For instance it's possible to write a manual model that "suppress" generation of models (for instance if it creates bogus models for a specific function). So perhaps there are other aspects that I'm not aware of. This is something that I'll have to look more at, and maybe ask Michael about.

[geoffw0]

DCA 👍

[mchammer01]

Approving on behalf of Docs ✨
2 minor comments/suggestions for your consideration.

[paldepind]

Thanks. I've applied the suggestions.

[mchammer01]

Thanks, re-approving for Docs

[hvitved]

I think this ought to extend QuerySink::Range instead (cc @geoffw0 ).

[geoffw0]

Yes it should, but that class is new. I'll make the change and check all the other merged queries do this now.

[geoffw0]

Here: #19103