Merge remote-tracking branch 'upstream/master' into UselessCat

javascript/ql/src/Security/CWE-020/HostnameRegexpShared.qll

@@ -2,6 +2,7 @@
  * Provides predicates for reasoning about regular expressions
  * that match URLs and hostname patterns.
  */
+
 import javascript
 
 /**
@@ -39,9 +40,7 @@ predicate isDotLike(RegExpTerm term) {
 predicate matchesBeginningOfString(RegExpTerm term) {
   term.isRootTerm()
   or
-  exists(RegExpTerm parent |
-    matchesBeginningOfString(parent)
-  |
+  exists(RegExpTerm parent | matchesBeginningOfString(parent) |
     term = parent.(RegExpSequence).getChild(0)
     or
     parent.(RegExpSequence).getChild(0) instanceof RegExpCaret and
@@ -60,7 +59,11 @@ predicate matchesBeginningOfString(RegExpTerm term) {
  * `i` is bound to the index of the last child in the top-level domain part.
  */
 predicate hasTopLevelDomainEnding(RegExpSequence seq, int i) {
-  seq.getChild(i).(RegExpConstant).getValue().regexpMatch("(?i)" + RegExpPatterns::commonTLD() + "(:\\d+)?([/?#].*)?") and
+  seq
+      .getChild(i)
+      .(RegExpConstant)
+      .getValue()
+      .regexpMatch("(?i)" + RegExpPatterns::commonTLD() + "(:\\d+)?([/?#].*)?") and
   isDotLike(seq.getChild(i - 1)) and
   not (i = 1 and matchesBeginningOfString(seq))
 }
@@ -69,9 +72,7 @@ predicate hasTopLevelDomainEnding(RegExpSequence seq, int i) {
  * Holds if the given regular expression term contains top-level domain preceded by a dot,
  * such as `.com`.
  */
-predicate hasTopLevelDomainEnding(RegExpSequence seq) {
-  hasTopLevelDomainEnding(seq, _)
-}
+predicate hasTopLevelDomainEnding(RegExpSequence seq) { hasTopLevelDomainEnding(seq, _) }
 
 /**
  * Holds if `term` will always match a hostname, that is, all disjunctions contain

javascript/ql/src/Security/CWE-020/IncompleteHostnameRegExp.ql

@@ -61,9 +61,12 @@ predicate isIncompleteHostNameRegExpPattern(RegExpTerm regexp, RegExpSequence se
     unescapedDot = seq.getChild([0 .. i - 1]).getAChild*() and
     unescapedDot != seq.getChild(i - 1) and // Should not be the '.' immediately before the TLD
     not hasConsecutiveDots(unescapedDot.getParent()) and
-    hostname = seq.getChild(i - 2).getRawValue() + seq.getChild(i - 1).getRawValue() + seq.getChild(i).getRawValue()
+    hostname =
+      seq.getChild(i - 2).getRawValue() + seq.getChild(i - 1).getRawValue() +
+        seq.getChild(i).getRawValue()
   |
-    if unescapedDot.getParent() instanceof RegExpQuantifier then (
+    if unescapedDot.getParent() instanceof RegExpQuantifier
+    then
       // `.*\.example.com` can match `evil.com/?x=.example.com`
       //
       // This problem only occurs when the pattern is applied against a full URL, not just a hostname/origin.
@@ -73,16 +76,20 @@ predicate isIncompleteHostNameRegExpPattern(RegExpTerm regexp, RegExpSequence se
       seq.getChild(0) instanceof RegExpCaret and
       not seq.getAChild() instanceof RegExpDollar and
       seq.getChild([i .. i + 1]).(RegExpConstant).getValue().regexpMatch(".*[/?#].*") and
-      msg = "has an unrestricted wildcard '" +
-          unescapedDot.getParent().(RegExpQuantifier).getRawValue() +
-          "' which may cause '" + hostname + "' to be matched anywhere in the URL, outside the hostname."
-    ) else (
-      msg = "has an unescaped '.' before '" + hostname + "', so it might match more hosts than expected."
-    )
+      msg =
+        "has an unrestricted wildcard '" + unescapedDot.getParent().(RegExpQuantifier).getRawValue()
+          + "' which may cause '" + hostname +
+          "' to be matched anywhere in the URL, outside the hostname."
+    else
+      msg =
+        "has an unescaped '.' before '" + hostname +
+          "', so it might match more hosts than expected."
   )
 }
 
-from RegExpPatternSource re, RegExpTerm regexp, RegExpSequence hostSequence, string msg, string kind, DataFlow::Node aux
+from
+  RegExpPatternSource re, RegExpTerm regexp, RegExpSequence hostSequence, string msg, string kind,
+  DataFlow::Node aux
 where
   regexp = re.getRegExpTerm() and
   isIncompleteHostNameRegExpPattern(regexp, hostSequence, msg) and
@@ -96,5 +103,4 @@ where
     )
   ) and
   not CharacterEscapes::hasALikelyRegExpPatternMistake(re)
-select hostSequence,
-  "This " + kind + " " + msg, aux, "here"
+select hostSequence, "This " + kind + " " + msg, aux, "here"

javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql

@@ -20,9 +20,7 @@ class DangerousScheme extends string {
 }
 
 /** Gets a data-flow node that checks `nd` against the given `scheme`. */
-DataFlow::Node schemeCheck(
-  DataFlow::Node nd, DangerousScheme scheme
-) {
+DataFlow::Node schemeCheck(DataFlow::Node nd, DangerousScheme scheme) {
   // check of the form `nd.startsWith(scheme)`
   exists(StringOps::StartsWith sw | sw = result |
     sw.getBaseString() = nd and

javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql

@@ -46,9 +46,7 @@ predicate isInteriorAnchor(RegExpAnchor term) {
  * Holds if `term` contains an anchor that is not the first or last node
  * in its tree, such as `(foo|bar$|baz)`.
  */
-predicate containsInteriorAnchor(RegExpTerm term) {
-  isInteriorAnchor(term.getAChild*())
-}
+predicate containsInteriorAnchor(RegExpTerm term) { isInteriorAnchor(term.getAChild*()) }
 
 /**
  * Holds if `term` starts with a word boundary or lookbehind assertion,
@@ -78,9 +76,7 @@ predicate containsTrailingPseudoAnchor(RegExpSequence term) {
  * Holds if `term` is an empty sequence, usually arising from
  * literals with a trailing alternative such as `foo|`.
  */
-predicate isEmpty(RegExpSequence term) {
-  term.getNumChild() = 0
-}
+predicate isEmpty(RegExpSequence term) { term.getNumChild() = 0 }
 
 /**
  * Holds if `term` contains a letter constant.
@@ -131,14 +127,14 @@ predicate hasMisleadingAnchorPrecedence(RegExpPatternSource src, string msg) {
     (
       anchoredTerm = root.getChild(0) and
       anchoredTerm.getChild(0) instanceof RegExpCaret and
-      not containsLeadingPseudoAnchor(root.getChild([ 1 .. root.getNumChild() - 1 ])) and
-      containsLetters(root.getChild([ 1 .. root.getNumChild() - 1 ])) and
+      not containsLeadingPseudoAnchor(root.getChild([1 .. root.getNumChild() - 1])) and
+      containsLetters(root.getChild([1 .. root.getNumChild() - 1])) and
       direction = "beginning"
       or
       anchoredTerm = root.getLastChild() and
       anchoredTerm.getLastChild() instanceof RegExpDollar and
-      not containsTrailingPseudoAnchor(root.getChild([ 0 .. root.getNumChild() - 2 ])) and
-      containsLetters(root.getChild([ 0 .. root.getNumChild() - 2 ])) and
+      not containsTrailingPseudoAnchor(root.getChild([0 .. root.getNumChild() - 2])) and
+      containsLetters(root.getChild([0 .. root.getNumChild() - 2])) and
       direction = "end"
     ) and
     // is not used for replace
@@ -146,8 +142,10 @@ predicate hasMisleadingAnchorPrecedence(RegExpPatternSource src, string msg) {
       replace.getMethodName() = "replace" and
       src.getARegExpObject().flowsTo(replace.getArgument(0))
     ) and
-    msg = "Misleading operator precedence. The subexpression '" + anchoredTerm.getRawValue() +
-        "' is anchored at the " + direction + ", but the other parts of this regular expression are not"
+    msg =
+      "Misleading operator precedence. The subexpression '" + anchoredTerm.getRawValue() +
+        "' is anchored at the " + direction +
+        ", but the other parts of this regular expression are not"
   )
 }
 
@@ -181,7 +179,8 @@ predicate isSemiAnchoredHostnameRegExp(RegExpPatternSource src, string msg) {
     hasTopLevelDomainEnding(tld, i) and
     isFinalRegExpTerm(tld.getChild(i)) and // nothing is matched after the TLD
     tld.getChild(0) instanceof RegExpCaret and
-    msg = "This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end."
+    msg =
+      "This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end."
   )
 }
 
@@ -214,7 +213,8 @@ predicate isUnanchoredHostnameRegExp(RegExpPatternSource src, string msg) {
         name = "match" and exists(mcn.getAPropertyRead())
       )
     ) and
-    msg = "When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it."
+    msg =
+      "When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it."
   )
 }
 

javascript/ql/src/Security/CWE-020/UselessRegExpCharacterEscape.ql

@@ -105,13 +105,9 @@ class RegExpPatternMistake extends TRegExpPatternMistake {
  */
 class IdentityEscapeInStringMistake extends RegExpPatternMistake, TIdentityEscapeInStringMistake {
   RegExpPatternSource src;
-
   string char;
-
   string mistake;
-
   int index;
-
   ASTNode rawStringNode;
 
   IdentityEscapeInStringMistake() {
@@ -130,19 +126,19 @@ class IdentityEscapeInStringMistake extends RegExpPatternMistake, TIdentityEscap
 }
 
 /**
-  * A backspace as '\b' in a regular expression string, indicating
-  * programmer intent to use the word-boundary assertion '\b'.
-  */
+ * A backspace as '\b' in a regular expression string, indicating
+ * programmer intent to use the word-boundary assertion '\b'.
+ */
 class BackspaceInStringMistake extends RegExpPatternMistake, TBackspaceInStringMistake {
   RegExpPatternSource src;
-
   int index;
-
   ASTNode rawStringNode;
 
   BackspaceInStringMistake() { this = TBackspaceInStringMistake(src, rawStringNode, index) }
 
-  override string getMessage() { result = "'\\b' is a backspace, and not a word-boundary assertion" }
+  override string getMessage() {
+    result = "'\\b' is a backspace, and not a word-boundary assertion"
+  }
 
   override int getIndex() { result = index }
 

javascript/ql/src/Security/CWE-078/CommandInjection.ql

@@ -16,11 +16,16 @@ import javascript
 import semmle.javascript.security.dataflow.CommandInjection::CommandInjection
 import DataFlow::PathGraph
 
-from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node highlight, Source sourceNode
+from
+  Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node highlight,
+  Source sourceNode
 where
   cfg.hasFlowPath(source, sink) and
-  if cfg.isSinkWithHighlight(sink.getNode(), _)
-  then cfg.isSinkWithHighlight(sink.getNode(), highlight)
-  else highlight = sink.getNode() and
+  (
+    if cfg.isSinkWithHighlight(sink.getNode(), _)
+    then cfg.isSinkWithHighlight(sink.getNode(), highlight)
+    else highlight = sink.getNode()
+  ) and
   sourceNode = source.getNode()
-select highlight, source, sink, "This command depends on $@.", sourceNode, sourceNode.getSourceType()
+select highlight, source, sink, "This command depends on $@.", sourceNode,
+  sourceNode.getSourceType()

javascript/ql/src/Security/CWE-079/ExceptionXss.ql

@@ -15,10 +15,8 @@ import javascript
 import semmle.javascript.security.dataflow.ExceptionXss::ExceptionXss
 import DataFlow::PathGraph
 
-from
-  Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
-where
-  cfg.hasFlowPath(source, sink)
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
 select sink.getNode(), source, sink,
-  sink.getNode().(Xss::Shared::Sink).getVulnerabilityKind() + " vulnerability due to $@.", source.getNode(),
-  "user-provided value"
+  sink.getNode().(Xss::Shared::Sink).getVulnerabilityKind() + " vulnerability due to $@.",
+  source.getNode(), "user-provided value"

javascript/ql/src/Security/CWE-116/DoubleEscaping.ql

@@ -44,23 +44,17 @@ predicate escapingScheme(string metachar, string regex) {
  * A call to `String.prototype.replace` that replaces all instances of a pattern.
  */
 class Replacement extends StringReplaceCall {
-  Replacement() {
-    isGlobal()
-  }
+  Replacement() { isGlobal() }
 
   /**
    * Gets the input of this replacement.
    */
-  DataFlow::Node getInput() {
-    result = this.getReceiver()
-  }
+  DataFlow::Node getInput() { result = this.getReceiver() }
 
   /**
    * Gets the output of this replacement.
    */
-  DataFlow::SourceNode getOutput() {
-    result = this
-  }
+  DataFlow::SourceNode getOutput() { result = this }
 
   /**
    * Holds if this replacement escapes `char` using `metachar`.
@@ -93,9 +87,7 @@ class Replacement extends StringReplaceCall {
   /**
    * Gets the previous replacement in this chain of replacements.
    */
-  Replacement getPreviousReplacement() {
-    result.getOutput() = getASimplePredecessor*(getInput())
-  }
+  Replacement getPreviousReplacement() { result.getOutput() = getASimplePredecessor*(getInput()) }
 
   /**
    * Gets an earlier replacement in this chain of replacements that

javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql

@@ -44,9 +44,7 @@ predicate isSimpleCharacterClass(RegExpCharacterClass t) {
 }
 
 /** Holds if `t` is an alternation of simple terms. */
-predicate isSimpleAlt(RegExpAlt t) {
-  forall(RegExpTerm ch | ch = t.getAChild() | isSimple(ch))
-}
+predicate isSimpleAlt(RegExpAlt t) { forall(RegExpTerm ch | ch = t.getAChild() | isSimple(ch)) }
 
 /**
  * Holds if `mce` is of the form `x.replace(re, new)`, where `re` is a global

javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql

@@ -13,26 +13,20 @@
 import javascript
 
 /** Gets a property name of `req` which refers to data usually derived from cookie data. */
-string cookieProperty() {
-  result = "session" or result = "cookies" or result = "user"
-}
+string cookieProperty() { result = "session" or result = "cookies" or result = "user" }
 
 /** Gets a data flow node that flows to the base of an access to `cookies`, `session`, or `user`. */
 private DataFlow::SourceNode nodeLeadingToCookieAccess(DataFlow::TypeBackTracker t) {
   t.start() and
   exists(DataFlow::PropRead value |
     value = result.getAPropertyRead(cookieProperty()).getAPropertyRead() and
-
     // Ignore accesses to values that are part of a CSRF or captcha check
     not value.getPropertyName().regexpMatch("(?i).*(csrf|xsrf|captcha).*") and
-
     // Ignore calls like `req.session.save()`
     not value = any(DataFlow::InvokeNode call).getCalleeNode()
   )
   or
-  exists(DataFlow::TypeBackTracker t2 |
-    result = nodeLeadingToCookieAccess(t2).backtrack(t2, t)
-  )
+  exists(DataFlow::TypeBackTracker t2 | result = nodeLeadingToCookieAccess(t2).backtrack(t2, t))
 }
 
 /** Gets a data flow node that flows to the base of an access to `cookies`, `session`, or `user`. */
@@ -52,9 +46,7 @@ private DataFlow::SourceNode getARouteUsingCookies(DataFlow::TypeTracker t) {
   t.start() and
   isRouteHandlerUsingCookies(result)
   or
-  exists(DataFlow::TypeTracker t2 |
-    result = getARouteUsingCookies(t2).track(t2, t)
-  )
+  exists(DataFlow::TypeTracker t2 | result = getARouteUsingCookies(t2).track(t2, t))
 }
 
 /** Gets a data flow node referring to a route handler that uses cookies. */
@@ -113,7 +105,6 @@ from
 where
   router = setup.getRouter() and
   handler = setup.getARouteHandlerExpr() and
-
   // Require that the handler uses cookies and has cookie middleware.
   //
   // In practice, handlers that use cookies always have the cookie middleware or
@@ -122,10 +113,8 @@ where
   // don't trust it to detect the presence of CSRF middleware either.
   getARouteUsingCookies().flowsToExpr(handler) and
   hasCookieMiddleware(handler, cookie) and
-
   // Only flag the cookie parser registered first.
   not hasCookieMiddleware(cookie, _) and
-
   not hasCsrfMiddleware(handler) and
   // Only warn for the last handler in a chain.
   handler.isLastHandler() and

javascript/ql/src/Security/CWE-400/PrototypePollutionUtility.ql

@@ -1,7 +1,7 @@
 /**
  * @name Prototype pollution in utility function
  * @description Recursively copying properties between objects may cause
-                accidental modification of a built-in prototype object.
+ *                accidental modification of a built-in prototype object.
  * @kind path-problem
  * @problem.severity warning
  * @precision high
@@ -30,9 +30,7 @@ predicate arePropertiesEnumerated(DataFlow::SourceNode node) {
 predicate isEnumeratedPropName(Node node) {
   node instanceof EnumeratedPropName
   or
-  exists(Node pred |
-    isEnumeratedPropName(pred)
-  |
+  exists(Node pred | isEnumeratedPropName(pred) |
     node = pred.getASuccessor()
     or
     argumentPassingStep(_, pred, _, node)
@@ -54,7 +52,6 @@ predicate isPotentiallyObjectPrototype(SourceNode node) {
   exists(Node base, Node key |
     dynamicPropReadStep(base, key, node) and
     isEnumeratedPropName(key) and
-
     // Ignore cases where the properties of `base` are enumerated, to avoid FPs
     // where the key came from that enumeration (and thus will not return Object.prototype).
     // For example, `src[key]` in `for (let key in src) { ... src[key] ... }` will generally
@@ -62,9 +59,7 @@ predicate isPotentiallyObjectPrototype(SourceNode node) {
     not arePropertiesEnumerated(base.getALocalSource())
   )
   or
-  exists(Node use |
-    isPotentiallyObjectPrototype(use.getALocalSource())
-  |
+  exists(Node use | isPotentiallyObjectPrototype(use.getALocalSource()) |
     argumentPassingStep(_, use, _, node)
   )
 }
@@ -87,7 +82,6 @@ predicate dynamicPropWrite(DataFlow::Node base, DataFlow::Node prop, DataFlow::N
     rhs = write.getRhs().flow() and
     not exists(prop.getStringValue()) and
     not arePropertiesEnumerated(base.getALocalSource()) and
-
     // Prune writes that are unlikely to modify Object.prototype.
     // This is mainly for performance, but may block certain results due to
     // not tracking out of function returns and into callbacks.
@@ -188,12 +182,7 @@ class PropNameTracking extends DataFlow::Configuration {
   override predicate isBarrier(DataFlow::Node node) {
     super.isBarrier(node)
     or
-    exists(ConditionGuardNode guard, SsaRefinementNode refinement |
-      node = DataFlow::ssaDefinitionNode(refinement) and
-      refinement.getGuard() = guard and
-      guard.getTest() instanceof VarAccess and
-      guard.getOutcome() = false
-    )
+    node instanceof DataFlow::VarAccessBarrier
   }
 
   override predicate isBarrierGuard(DataFlow::BarrierGuardNode node) {
@@ -204,7 +193,8 @@ class PropNameTracking extends DataFlow::Configuration {
     node instanceof InstanceOfGuard or
     node instanceof TypeofGuard or
     node instanceof BlacklistInclusionGuard or
-    node instanceof WhitelistInclusionGuard
+    node instanceof WhitelistInclusionGuard or
+    node instanceof IsPlainObjectGuard
   }
 }
 
@@ -379,6 +369,25 @@ class WhitelistInclusionGuard extends DataFlow::LabeledBarrierGuardNode {
   }
 }
 
+/**
+ * A check of form `isPlainObject(e)` or similar, which sanitizes the `constructor`
+ * payload in the true case, since it rejects objects with a non-standard `constructor`
+ * property.
+ */
+class IsPlainObjectGuard extends DataFlow::LabeledBarrierGuardNode, DataFlow::CallNode {
+  IsPlainObjectGuard() {
+    exists(string name | name = "is-plain-object" or name = "is-extendable" |
+      this = moduleImport(name).getACall()
+    )
+  }
+
+  override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel lbl) {
+    e = getArgument(0).asExpr() and
+    outcome = true and
+    lbl = "constructor"
+  }
+}
+
 /**
  * Gets a meaningful name for `node` if possible.
  */
@@ -420,9 +429,7 @@ private DataFlow::SourceNode getANodeLeadingToBase(DataFlow::TypeBackTracker t,
   isPrototypePollutingAssignment(base, _, _, _) and
   result = base.getALocalSource()
   or
-  exists(DataFlow::TypeBackTracker t2 |
-    result = getANodeLeadingToBase(t2, base).backtrack(t2, t)
-  )
+  exists(DataFlow::TypeBackTracker t2 | result = getANodeLeadingToBase(t2, base).backtrack(t2, t))
 }
 
 /**

javascript/ql/src/Security/Summaries/ExtractFlowStepSummaries.ql

@@ -14,8 +14,9 @@ import PortalExitSource
 import PortalEntrySink
 
 from
-  TaintTracking::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink, Portal p1,
-  Portal p2, DataFlow::FlowLabel lbl1, DataFlow::FlowLabel lbl2, DataFlow::MidPathNode last
+  TaintTracking::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink,
+  Portal p1, Portal p2, DataFlow::FlowLabel lbl1, DataFlow::FlowLabel lbl2,
+  DataFlow::MidPathNode last
 where
   cfg = source.getConfiguration() and
   last = source.getASuccessor*() and

javascript/ql/src/Security/Summaries/ExtractSinkSummaries.ql

@@ -11,7 +11,8 @@ import Configurations
 import PortalExitSource
 import SinkFromAnnotation
 
-from DataFlow::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink,
+from
+  DataFlow::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink,
   Portal p, DataFlow::MidPathNode last
 where
   cfg = source.getConfiguration() and

javascript/ql/src/Security/Summaries/ExtractSourceSummaries.ql

@@ -11,7 +11,8 @@ import Configurations
 import PortalEntrySink
 import SourceFromAnnotation
 
-from DataFlow::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink,
+from
+  DataFlow::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink,
   Portal p, DataFlow::MidPathNode last
 where
   cfg = source.getConfiguration() and

javascript/ql/src/Security/Summaries/ImportFromCsv.qll

@@ -127,7 +127,8 @@ class AdditionalStepSpec extends ExternalData {
     exists(string config |
       if getField(4) = "" then config = "any configuration" else config = getConfiguration()
     |
-      result = "edge from " + getStartPortal() + " to " + getEndPortal() + ", transforming " +
+      result =
+        "edge from " + getStartPortal() + " to " + getEndPortal() + ", transforming " +
           getStartFlowLabel() + " into " + getEndFlowLabel() + " for " + config
     )
   }