Swift: Query for regular expression injection

[geoffw0]

Adds a query for regular expression injection, that is, constructing a regular expression from unsanitized user input. This is fairly straightforward as the vulnerability is a simple injection issue and we already have a regular expressions library that identifies points where regular expressions are evaluated. It turns out we also want to identify points where regular expressions are constructed in advance of evaluation, so as to get clean and reliable alerts, so we do that as well.

There are a handful of test results that are false positives or not detected. We have follow-up issues to address all of these, and in the case of the false positives I do not believe they will be noisy in real world projects before we address them.

TODO:

• team review
• doc review
• DCA run

[github-actions]

QHelp previews:

swift/ql/src/queries/Security/CWE-730/RegexInjection.qhelp

Regular expression injection

Constructing a regular expression with unsanitized user input is dangerous, since a malicious user may be able to modify the meaning of the expression. They may be able to cause unexpected program behaviour, or perform a denial-of-service attack. For example, they may provide a regular expression fragment that takes exponential time to evaluate in the worst case.

Recommendation

Before embedding user input into a regular expression, use a sanitization function such as NSRegularExpression::escapedPattern(for:) to escape meta-characters that have special meaning.

Example

The following examples construct regular expressions from user input without sanitizing it first:

func processRemoteInput(remoteInput: String) {
  ...

  // BAD: Unsanitized user input is used to construct a regular expression
  let regex1 = try Regex(remoteInput)

  // BAD: Unsanitized user input is used to construct a regular expression
  let regexStr = "abc|\(remoteInput)"
  let regex2 = try NSRegularExpression(pattern: regexStr)

  ...
}

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

func processRemoteInput(remoteInput: String) {
  ...

  // GOOD: Regular expression is not derived from user input
  let regex1 = try Regex(myRegex)

  // GOOD: User input is sanitized before being used to construct a regular expression
  let escapedInput = NSRegularExpression.escapedPattern(for: remoteInput)
  let regexStr = "abc|\(escapedInput)"
  let regex2 = try NSRegularExpression(pattern: regexStr)

  ...
}

References

• OWASP: Regular expression Denial of Service - ReDoS.
• Wikipedia: ReDoS.
• Swift: NSRegularExpression.escapedPattern(for:).
• Common Weakness Enumeration: CWE-730.
• Common Weakness Enumeration: CWE-400.

[geoffw0]

DCA run LGTM.

[geoffw0]

Thanks for the review @mattpollard. I've accepted both of your suggestions.

@rdmarsh2 I did your recommended change as well, are you happy to approve this now?