diff --git a/javascript/ql/lib/semmle/javascript/security/IncompleteMultiCharacterSanitization.qll b/javascript/ql/lib/semmle/javascript/security/IncompleteMultiCharacterSanitization.qll index 293a36dc36b..03efbc7ca07 100644 --- a/javascript/ql/lib/semmle/javascript/security/IncompleteMultiCharacterSanitization.qll +++ b/javascript/ql/lib/semmle/javascript/security/IncompleteMultiCharacterSanitization.qll @@ -157,3 +157,46 @@ private predicate isCommonWordMatcher(RegExpTerm t) { .getValue() = "w" ) } + +/** + * Holds if `replace` has a pattern argument containing a regular expression + * `dangerous` which matches a dangerous string beginning with `prefix`, in an + * attempt to avoid a vulnerability of kind `kind`. + */ +predicate isResult( + StringSubstitutionCall replace, EmptyReplaceRegExpTerm dangerous, string prefix, string kind +) { + exists(EmptyReplaceRegExpTerm regexp | + replace = regexp.getCall() and + dangerous.getRootTerm() = regexp and + // skip leading optional elements + not dangerous.isNullable() and + // only warn about the longest match + prefix = max(string m | matchesDangerousPrefix(dangerous, m, kind) | m order by m.length(), m) and + // only warn once per kind + not exists(EmptyReplaceRegExpTerm other | + other = dangerous.getAChild+() or other = dangerous.getPredecessor+() + | + matchesDangerousPrefix(other, _, kind) and + not other.isNullable() + ) and + // avoid anchored terms + not exists(RegExpAnchor a | regexp = a.getRootTerm()) and + // Don't flag replace operations that are called repeatedly in a loop, as they can actually work correctly. + not replace.flowsTo(replace.getReceiver+()) + ) +} + +/** + * Holds if `replace` has a pattern argument containing a regular expression + * `dangerous` which matches a dangerous string beginning with `prefix`. `msg` + * is the alert we report. + */ +query predicate problems( + StringSubstitutionCall replace, string msg, EmptyReplaceRegExpTerm dangerous, string prefix +) { + exists(string kind | + isResult(replace, dangerous, prefix, kind) and + msg = "This string may still contain $@, which may cause a " + kind + " vulnerability." + ) +} diff --git a/javascript/ql/lib/semmle/javascript/security/IncompleteMultiCharacterSanitizationSpecific.qll b/javascript/ql/lib/semmle/javascript/security/IncompleteMultiCharacterSanitizationSpecific.qll index f82d1539411..eb9f0593ac9 100644 --- a/javascript/ql/lib/semmle/javascript/security/IncompleteMultiCharacterSanitizationSpecific.qll +++ b/javascript/ql/lib/semmle/javascript/security/IncompleteMultiCharacterSanitizationSpecific.qll @@ -5,6 +5,8 @@ import javascript import semmle.javascript.security.performance.ReDoSUtil as ReDoSUtil +class StringSubstitutionCall = StringReplaceCall; + /** * A regexp term that matches substrings that should be replaced with the empty string. */ @@ -15,4 +17,9 @@ class EmptyReplaceRegExpTerm extends RegExpTerm { this = replace.getRegExp().getRoot().getAChild*() ) } + + /** + * Get the substitution call that uses this regexp term. + */ + StringSubstitutionCall getCall() { this = result.getRegExp().getRoot() } } diff --git a/javascript/ql/src/Security/CWE-116/IncompleteMultiCharacterSanitization.ql b/javascript/ql/src/Security/CWE-116/IncompleteMultiCharacterSanitization.ql index 484dd0cb2ca..0b47e9a8fd7 100644 --- a/javascript/ql/src/Security/CWE-116/IncompleteMultiCharacterSanitization.ql +++ b/javascript/ql/src/Security/CWE-116/IncompleteMultiCharacterSanitization.ql @@ -13,29 +13,4 @@ * external/cwe/cwe-116 */ -import javascript -private import semmle.javascript.security.IncompleteMultiCharacterSanitization - -from - StringReplaceCall replace, EmptyReplaceRegExpTerm regexp, EmptyReplaceRegExpTerm dangerous, - string prefix, string kind -where - regexp = replace.getRegExp().getRoot() and - dangerous.getRootTerm() = regexp and - // skip leading optional elements - not dangerous.isNullable() and - // only warn about the longest match (presumably the most descriptive) - prefix = max(string m | matchesDangerousPrefix(dangerous, m, kind) | m order by m.length(), m) and - // only warn once per kind - not exists(EmptyReplaceRegExpTerm other | - other = dangerous.getAChild+() or other = dangerous.getPredecessor+() - | - matchesDangerousPrefix(other, _, kind) and - not other.isNullable() - ) and - // don't flag replace operations in a loop - not replace.getAMethodCall*().flowsTo(replace.getReceiver()) and - // avoid anchored terms - not exists(RegExpAnchor a | regexp = a.getRootTerm()) -select replace, "This string may still contain $@, which may cause a " + kind + " vulnerability.", - dangerous, prefix +import semmle.javascript.security.IncompleteMultiCharacterSanitization