Rust: Add regular expression injection query

[paldepind]

Note:

• I've put this query under CWE-730 as that's where the Swift one was. But given that the Rust regex implementation is robust regarding DOS attacks, I think we should find another directory for it?

[github-actions]

QHelp previews:

rust/ql/src/queries/security/CWE-020/RegexInjection.qhelp

Regular expression injection

Constructing a regular expression with unsanitized user input can be dangerous. A malicious user may be able to modify the meaning of the expression, causing it to match unexpected strings and construct large regular expressions by using counted repetitions.

Recommendation

Before embedding user input into a regular expression, escape the input string using a function such as regex::escape to escape meta-characters that have special meaning.

If purposefully supporting user supplied regular expressions, then use RegexBuilder::size_limit to limit the pattern size so that it is no larger than necessary.

Example

The following example constructs a regular expressions from the user input key without escaping it first.

use regex::Regex;

fn get_value<'h>(key: &str, property: &'h str) -> Option<&'h str> {
    // BAD: User provided `key` is interpolated into the regular expression.
    let pattern = format!(r"^property:{key}=(.*)$");
    let re = Regex::new(&pattern).unwrap();
    re.captures(property)?.get(1).map(|m| m.as_str())
}

The regular expression is intended to match strings starting with "property" such as "property:foo=bar". However, a malicious user might inject the regular expression ".*^|key" and unexpectedly cause strings such as "key=secret" to match.

If user input is used to construct a regular expression, it should be escaped first. This ensures that malicious users cannot insert characters that have special meanings in regular expressions.

use regex::{escape, Regex};

fn get_value<'h>(key: &str, property: &'h str) -> option<&'h str> {
    // GOOD: User input is escaped before being used in the regular expression.
    let escaped_key = escape(key);
    let pattern = format!(r"^property:{escaped_key}=(.*)$");
    let re = regex::new(&pattern).unwrap();
    re.captures(property)?.get(1).map(|m| m.as_str())
}

References

• regex crate documentation: Untrusted patterns.
• Common Weakness Enumeration: CWE-20.
• Common Weakness Enumeration: CWE-74.

[paldepind]

This model is accurate, but I mostly added it to make sure that the barrier actually worked.

[geoffw0]

Looks great! 🎉

We will need to check if there are any results in a DCA run, and I'm very interested to get an idea of how many sinks there are (regardless of results) as an indication of how much regex usage we are seeing.

[geoffw0]

Great example! I've added the ready-for-doc-review tag so this PR to attract a member of the docs team for further review of the .qhelp.

[geoffw0]

Is there any benefit to defining this sink in QL rather than models-as-data?

[paldepind]

This is to not consider cases where Regex::new is directly applied to a literal as sinks. I think most real-world uses of Regex::new is of that form, so it's simply as an optimization to not consider these clearly safe cases.

[geoffw0]

OK. Note that we do not currently do this in the cleartext logging query, which has far more sinks the vast majority of which just take a literal. If you think it makes a difference we might consider doing it there as well.

[paldepind]

I was told it could make a difference. But it probably doesn't matter much and let's double check before we spend time rewriting a bunch of stuff.

[mchammer01]

Hmm, I thought I had already submitted a review.
Approving this on behalf of Docs ✨
Left a few comments and suggestions for your consideration @paldepind

[mchammer01]

Minor - separating the clauses with a comma

Suggested change

	A malicious user may be able to modify the meaning of the expression causing it
	A malicious user may be able to modify the meaning of the expression, causing it

[mchammer01]

We can possibly simplify here

Suggested change

	to match unexpected strings and to construct large regular expressions by using
	to match unexpected strings and construct large regular expressions by using

[mchammer01]

Typo

Suggested change

	The following example construct a regular expressions from the user input
	The following example constructs a regular expressions from the user input

[mchammer01]

Typo / simplifying

Suggested change

	to limit the pattern size such that it is no larger than necessary.
	to limit the pattern size so that it is no larger than necessary.

[mchammer01]

Items in the References section need a full stop

Suggested change

	<code>regex</code> crate documentation: <a href="https://docs.rs/regex/latest/regex/index.html#untrusted-patterns">Untrusted patterns</a>
	<code>regex</code> crate documentation: <a href="https://docs.rs/regex/latest/regex/index.html#untrusted-patterns">Untrusted patterns</a>.

[mchammer01]

Adding a comma to separate the clauses

Suggested change

	If user input is used to construct a regular expression it should be escaped
	If user input is used to construct a regular expression, it should be escaped

[mchammer01]

Perhaps?

Suggested change

	first. This ensures that the user cannot insert characters that have special
	first. This ensures that malicious users cannot insert characters that have special

[mchammer01]

The description, which is mandatory, is missing. For guidance about writing query descriptions, see https://github.com/github/codeql/blob/main/docs/query-metadata-style-guide.md#query-descriptions-description.

[mchammer01]

I think more commits have been added since I started reviewing this, as all my comments are outdated 😞

[paldepind]

I think more commits have been added since I started reviewing this, as all my comments are outdated 😞

I moved the files to a different directory. So the comments are outdated but the actual suggestions still apply 👍

[paldepind]

Thanks for the review @mchammer01. I've applied your suggestions and added the missing description property.

[geoffw0]

Otherwise LGTM.

DCA run also LGTM.