Ruby: Add IncompleteMultiCharSanitization query

[hmac]

trafficstars

This PR adds a new query, IncompleteMultiCharSanitization, which is a port of a JS query of the same name. It finds cases where a regex is used to strip a particular pattern from a string, but is done in a way that can leave instances of the pattern remaining in the string. We focus on regexes that attempt to strip path traversal (../), HTML elements and HTML attributes.

This PR also improves the sensitivity of StringSubstitutionCall, which leads to new results for the IncompleteSanitization query.

The JS changes are just so we can share most of the logic of this query between the two languages.

[github-actions[bot]]

QHelp previews:

ruby/ql/src/queries/security/cwe-116/IncompleteMultiCharacterSanitization.qhelp

Incomplete multi-character sanitization

Sanitizing untrusted input is a common technique for preventing injection attacks such as SQL injection or cross-site scripting. Usually, this is done by escaping meta-characters such as quotes in a domain-specific way so that they are treated as normal characters.

However, directly using the String#sub method to perform escaping is notoriously error-prone. Common mistakes include only replacing the first occurrence of a meta-character, or backslash-escaping various meta-characters but not the backslash itself.

In the former case, later meta-characters are left undisturbed and can be used to subvert the sanitization. In the latter case, preceding a meta-character with a backslash leads to the backslash being escaped, but the meta-character appearing un-escaped, which again makes the sanitization ineffective.

Even if the escaped string is not used in a security-critical context, incomplete escaping may still have undesirable effects, such as badly rendered or confusing output.

Recommendation

Use a (well-tested) sanitization library if at all possible. These libraries are much more likely to handle corner cases correctly than a custom implementation.

An even safer alternative is to design the application so that sanitization is not needed. Otherwise, make sure to use String#gsub rather than String#sub, to ensure that all occurrences are replaced, and remember to escape backslashes if applicable.

Note, however, that this is generally not sufficient for replacing multi-character strings: the String#gsub method performs only one pass over the input string, and will not replace further instances of the string that result from earlier replacements.

For example, consider the code snippet s.gsub /\/\.\.\//, "", which attempts to strip out all occurrences of /../ from s. This will not work as expected: for the string /./.././, for example, it will remove the single occurrence of /../ in the middle, but the remainder of the string then becomes /../, which is another instance of the substring we were trying to remove.

Example

As an example, assume that we want to embed a user-controlled string account_number into a SQL query as part of a string literal. To avoid SQL injection, we need to ensure that the string does not contain un-escaped single-quote characters. The following method attempts to ensure this by doubling single quotes, and thereby escaping them:

def escape_quotes(s)
  s.sub "'", "''"
end

As written, this sanitizer is ineffective: String#sub will replace only the first occurrence of that string.

As mentioned above, the method escape_quotes should be replaced with a purpose-built sanitizer, such as ActiveRecord::Base::sanitize_sql in Rails, or by using ORM methods that automatically sanitize parameters.

If this is not an option, escape_quotes should be rewritten to use the String#gsub method instead:

def escape_quotes(s)
  s.gsub "'", "''"
end

References

• OWASP Top 10: A1 Injection.
• Rails: ActiveRecord::Base::sanitize_sql.
• Common Weakness Enumeration: CWE-20.
• Common Weakness Enumeration: CWE-80.
• Common Weakness Enumeration: CWE-116.

[hmac]

Had to rebase it due to some conflicts related to the renaming of ReDoSUtil, sorry!