Merge branch 'main' into restify_improvements

javascript/ql/lib/CHANGELOG.md

@@ -1,3 +1,10 @@
+## 0.3.1
+
+### Minor Analysis Improvements
+
+- Several of the SQL and NoSQL library models have improved, leading to more results for the `js/sql-injection` query,
+  and in some cases the `js/missing-rate-limiting` query.
+
 ## 0.3.0
 
 ### Breaking Changes

javascript/ql/lib/change-notes/released/0.3.1.md

@@ -1,6 +1,6 @@
----
-category: minorAnalysis
----
+## 0.3.1
+
+### Minor Analysis Improvements
 
 - Several of the SQL and NoSQL library models have improved, leading to more results for the `js/sql-injection` query,
   and in some cases the `js/missing-rate-limiting` query.

javascript/ql/lib/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.3.0
+lastReleaseVersion: 0.3.1

javascript/ql/lib/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/javascript-all
-version: 0.3.1-dev
+version: 0.3.2-dev
 groups: javascript
 dbscheme: semmlecode.javascript.dbscheme
 extractor: javascript

javascript/ql/lib/semmle/javascript/PrintAst.qll

@@ -161,7 +161,7 @@ private module PrintJavaScript {
   /**
    * A print node representing an `ASTNode`.
    *
-   * Provides a default implemention that works for some (but not all) ASTNode's.
+   * Provides a default implementation that works for some (but not all) ASTNode's.
    * More specific subclasses can override this class to get more specific behavior.
    *
    * The more specific subclasses are mostly used aggregate the children of the `ASTNode`.

javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll

@@ -711,13 +711,31 @@ module TaintTracking {
     }
   }
 
+  /**
+   * Gets a local source of any part of the input to the given stringification `call`.
+   */
+  pragma[nomagic]
+  private DataFlow::Node getAJsonLocalInput(JsonStringifyCall call) {
+    result = call.getInput()
+    or
+    exists(DataFlow::SourceNode source |
+      source = pragma[only_bind_out](getAJsonLocalInput(call)).getALocalSource()
+    |
+      result = source.getAPropertyWrite().getRhs()
+      or
+      result = source.(DataFlow::ObjectLiteralNode).getASpreadProperty()
+      or
+      result = source.(DataFlow::ArrayCreationNode).getASpreadArgument()
+    )
+  }
+
   /**
    * A taint propagating data flow edge arising from JSON unparsing.
    */
   private class JsonStringifyTaintStep extends SharedTaintStep {
     override predicate serializeStep(DataFlow::Node pred, DataFlow::Node succ) {
       exists(JsonStringifyCall call |
-        pred = call.getArgument(0) and
+        pred = getAJsonLocalInput(call) and
         succ = call
       )
     }

javascript/ql/lib/semmle/javascript/frameworks/ClientRequests.qll

@@ -671,7 +671,7 @@ module ClientRequest {
     }
 
     /**
-     * Gets the response type corresponding to `getReponse()` but not
+     * Gets the response type corresponding to `getResponse()` but not
      * for explicitly typed calls like `getResponseJson()`.
      */
     string getAssignedResponseType() {

javascript/ql/lib/semmle/javascript/frameworks/Immutable.qll

@@ -7,7 +7,7 @@ import javascript
 /**
  * Provides classes implementing data-flow for Immutable.
  *
- * The implemention rely on the flowsteps implemented in `Collections.qll`.
+ * The implementation rely on the flowsteps implemented in `Collections.qll`.
  */
 private module Immutable {
   /**

javascript/ql/lib/semmle/javascript/security/BadTagFilterQuery.qll

@@ -1,5 +1,5 @@
 /**
- * Provides precicates for reasoning about bad tag filter vulnerabilities.
+ * Provides predicates for reasoning about bad tag filter vulnerabilities.
  */
 
 import regexp.RegexpMatching
@@ -65,7 +65,7 @@ predicate isBadRegexpFilter(HtmlMatchingRegExp regexp, string msg) {
   regexp.matches("<!-- foo --!>") and
   exists(int a, int b | a != b |
     regexp.fillsCaptureGroup("<!-- foo -->", a) and
-    // <!-- foo --> might be ambigously parsed (matching both capture groups), and that is ok here.
+    // <!-- foo --> might be ambiguously parsed (matching both capture groups), and that is ok here.
     regexp.fillsCaptureGroup("<!-- foo --!>", b) and
     not regexp.fillsCaptureGroup("<!-- foo --!>", a) and
     msg =

javascript/ql/lib/semmle/javascript/security/dataflow/ReflectedXssCustomizations.qll

@@ -25,13 +25,13 @@ module ReflectedXss {
    * is to prevent us from flagging plain-text or JSON responses as vulnerable.
    */
   class HttpResponseSink extends Sink instanceof Http::ResponseSendArgument {
-    HttpResponseSink() { not exists(getANonHtmlHeaderDefinition(this)) }
+    HttpResponseSink() { not exists(getAXssSafeHeaderDefinition(this)) }
   }
 
   /**
-   * Gets a HeaderDefinition that defines a non-html content-type for `send`.
+   * DEPRECATED: Gets a HeaderDefinition that defines a non-html content-type for `send`.
    */
-  Http::HeaderDefinition getANonHtmlHeaderDefinition(Http::ResponseSendArgument send) {
+  deprecated Http::HeaderDefinition getANonHtmlHeaderDefinition(Http::ResponseSendArgument send) {
     exists(Http::RouteHandler h |
       send.getRouteHandler() = h and
       result = nonHtmlContentTypeHeader(h)
@@ -42,13 +42,49 @@ module ReflectedXss {
   }
 
   /**
-   * Holds if `h` may send a response with a content type other than HTML.
+   * DEPRECATED: Holds if `h` may send a response with a content type other than HTML.
    */
-  Http::HeaderDefinition nonHtmlContentTypeHeader(Http::RouteHandler h) {
+  deprecated Http::HeaderDefinition nonHtmlContentTypeHeader(Http::RouteHandler h) {
     result = h.getAResponseHeader("content-type") and
     not exists(string tp | result.defines("content-type", tp) | tp.regexpMatch("(?i).*html.*"))
   }
 
+  /**
+   * Gets a HeaderDefinition that defines a XSS safe content-type for `send`.
+   */
+  Http::HeaderDefinition getAXssSafeHeaderDefinition(Http::ResponseSendArgument send) {
+    exists(Http::RouteHandler h |
+      send.getRouteHandler() = h and
+      result = xssSafeContentTypeHeader(h)
+    |
+      // The HeaderDefinition affects a response sent at `send`.
+      headerAffects(result, send)
+    )
+  }
+
+  /**
+   * Gets a content-type that may lead to javascript code being executed in the browser.
+   * ref: https://portswigger.net/web-security/cross-site-scripting/cheat-sheet#content-types
+   */
+  string xssUnsafeContentType() {
+    result =
+      [
+        "text/html", "application/xhtml+xml", "application/xml", "text/xml", "image/svg+xml",
+        "text/xsl", "application/vnd.wap.xhtml+xml", "text/rdf", "application/rdf+xml",
+        "application/mathml+xml", "text/vtt", "text/cache-manifest"
+      ]
+  }
+
+  /**
+   * Holds if `h` may send a response with a content type that is safe for XSS.
+   */
+  Http::HeaderDefinition xssSafeContentTypeHeader(Http::RouteHandler h) {
+    result = h.getAResponseHeader("content-type") and
+    not exists(string tp | result.defines("content-type", tp) |
+      tp.toLowerCase().matches(xssUnsafeContentType() + "%")
+    )
+  }
+
   /**
    * Holds if a header set in `header` is likely to affect a response sent at `sender`.
    */
@@ -61,6 +97,7 @@ module ReflectedXss {
       // There is no dominating header, and `header` is non-local.
       not isLocalHeaderDefinition(header) and
       not exists(Http::HeaderDefinition dominatingHeader |
+        dominatingHeader.getAHeaderName() = "content-type" and
         dominatingHeader.getBasicBlock().(ReachableBasicBlock).dominates(sender.getBasicBlock())
       )
     )

javascript/ql/lib/semmle/javascript/security/regexp/ExponentialBackTracking.qll

@@ -202,7 +202,7 @@ private predicate isFork(State q, InputSymbol s1, InputSymbol s2, State r1, Stat
     //
     // We additionally require that the there exists another InfiniteRepetitionQuantifier `mid` on the path from `q` to itself.
     // This is done to avoid flagging regular expressions such as `/(a?)*b/` - that only has polynomial runtime, and is detected by `js/polynomial-redos`.
-    // The below code is therefore a heuritic, that only flags regular expressions such as `/(a*)*b/`,
+    // The below code is therefore a heuristic, that only flags regular expressions such as `/(a*)*b/`,
     // and does not flag regular expressions such as `/(a?b?)c/`, but the latter pattern is not used frequently.
     r1 = r2 and
     q1 = q2 and

javascript/ql/lib/semmle/javascript/security/regexp/NfaUtils.qll

@@ -59,8 +59,8 @@ predicate matchesEpsilon(RegExpTerm t) {
 /**
  * A lookahead/lookbehind that matches the empty string.
  */
-class EmptyPositiveSubPatttern extends RegExpSubPattern {
-  EmptyPositiveSubPatttern() {
+class EmptyPositiveSubPattern extends RegExpSubPattern {
+  EmptyPositiveSubPattern() {
     (
       this instanceof RegExpPositiveLookahead
       or
@@ -70,6 +70,9 @@ class EmptyPositiveSubPatttern extends RegExpSubPattern {
   }
 }
 
+/** DEPRECATED: Use `EmptyPositiveSubPattern` instead. */
+deprecated class EmptyPositiveSubPatttern = EmptyPositiveSubPattern;
+
 /**
  * A branch in a disjunction that is the root node in a literal, or a literal
  * whose root node is not a disjunction.
@@ -133,7 +136,7 @@ private predicate isCanonicalTerm(RelevantRegExpTerm term, string str) {
 }
 
 /**
- * Gets a string reperesentation of the flags used with the regular expression.
+ * Gets a string representation of the flags used with the regular expression.
  * Only the flags that are relevant for the canonicalization are included.
  */
 string getCanonicalizationFlags(RegExpTerm root) {
@@ -334,7 +337,7 @@ private module CharacterClasses {
     )
   }
 
-  private string lowercaseLetter() { result = "abdcefghijklmnopqrstuvwxyz".charAt(_) }
+  private string lowercaseLetter() { result = "abcdefghijklmnopqrstuvwxyz".charAt(_) }
 
   private string upperCaseLetter() { result = "ABCDEFGHIJKLMNOPQRSTUVWXYZ".charAt(_) }
 
@@ -697,9 +700,7 @@ predicate delta(State q1, EdgeLabel lbl, State q2) {
     lbl = Epsilon() and q2 = Accept(getRoot(dollar))
   )
   or
-  exists(EmptyPositiveSubPatttern empty | q1 = before(empty) |
-    lbl = Epsilon() and q2 = after(empty)
-  )
+  exists(EmptyPositiveSubPattern empty | q1 = before(empty) | lbl = Epsilon() and q2 = after(empty))
 }
 
 /**
@@ -1028,7 +1029,7 @@ module ReDoSPruning<isCandidateSig/2 isCandidate> {
    * as the suffix "X" will cause both the regular expressions to be rejected.
    *
    * The string `w` is repeated any number of times because it needs to be
-   * infinitely repeatedable for the attack to work.
+   * infinitely repeatable for the attack to work.
    * For the regular expression `/((ab)+)*abab/` the accepting state is not reachable from the fork
    * using epsilon transitions. But any attempt at repeating `w` will end in a state that accepts all suffixes.
    */

javascript/ql/lib/semmle/javascript/security/regexp/NfaUtilsSpecific.qll

@@ -5,7 +5,7 @@
 import javascript
 
 /**
- * Holds if `term` is an ecape class representing e.g. `\d`.
+ * Holds if `term` is an escape class representing e.g. `\d`.
  * `clazz` is which character class it represents, e.g. "d" for `\d`.
  */
 predicate isEscapeClass(RegExpTerm term, string clazz) {
@@ -20,13 +20,13 @@ predicate isPossessive(RegExpQuantifier term) { none() }
 
 /**
  * Holds if the regex that `term` is part of is used in a way that ignores any leading prefix of the input it's matched against.
- * Not yet implemented for Javascript.
+ * Not yet implemented for JavaScript.
  */
 predicate matchesAnyPrefix(RegExpTerm term) { any() }
 
 /**
  * Holds if the regex that `term` is part of is used in a way that ignores any trailing suffix of the input it's matched against.
- * Not yet implemented for Javascript.
+ * Not yet implemented for JavaScript.
  */
 predicate matchesAnySuffix(RegExpTerm term) { any() }
 

javascript/ql/lib/semmle/javascript/security/regexp/RegexpMatching.qll

@@ -1,5 +1,5 @@
 /**
- * Provides precicates for reasoning about which strings are matched by a regular expression,
+ * Provides predicates for reasoning about which strings are matched by a regular expression,
  * and for testing which capture groups are filled when a particular regexp matches a string.
  */
 

javascript/ql/lib/semmle/javascript/security/regexp/SuperlinearBackTracking.qll

@@ -76,7 +76,7 @@ class StateTuple extends TStateTuple {
   StateTuple() { this = MkStateTuple(q1, q2, q3) }
 
   /**
-   * Gest a string repesentation of this tuple.
+   * Gest a string representation of this tuple.
    */
   string toString() { result = "(" + q1 + ", " + q2 + ", " + q3 + ")" }