Merge remote-tracking branch 'upstream/master' into rc/1.20-merge-master

Conflict in `javascript/extractor/src/com/semmle/js/extractor/Main.java` resolved
in favour of `master`.

Author: Max Schaefer

.gitattributes

@@ -47,3 +47,4 @@
 *.jpeg -text
 *.gif -text
 *.dll -text
+*.pdb -text

change-notes/1.21/analysis-javascript.md

@@ -0,0 +1,24 @@
+# Improvements to JavaScript analysis
+
+## General improvements
+
+* Support for the following frameworks and libraries has been improved:
+  - [socket.io](http://socket.io)
+
+* The security queries now track data flow through Base64 decoders such as the Node.js `Buffer` class, the DOM function `atob`, and a number of npm packages intcluding [`abab`](https://www.npmjs.com/package/abab), [`atob`](https://www.npmjs.com/package/atob), [`btoa`](https://www.npmjs.com/package/btoa), [`base-64`](https://www.npmjs.com/package/base-64), [`js-base64`](https://www.npmjs.com/package/js-base64), [`Base64.js`](https://www.npmjs.com/package/Base64) and [`base64-js`](https://www.npmjs.com/package/base64-js).
+
+
+## New queries
+
+| **Query**                                     | **Tags**                                             | **Purpose**                                                                                                                                                                 |
+|-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+
+## Changes to existing queries
+
+| **Query**                      | **Expected impact**          | **Change**                                                                |
+|--------------------------------|------------------------------|---------------------------------------------------------------------------|
+| Expression has no effect       | Fewer false-positive results | This rule now treats uses of `Object.defineProperty` more conservatively. |
+| Useless assignment to property | Fewer false-positive results | This rule now ignore reads of additional getters. |
+| Arbitrary file write during zip extraction ("Zip Slip") | More results | This rule now considers more libraries, including tar as well as zip. |
+
+## Changes to QL libraries

cpp/ql/src/Architecture/Refactoring Opportunities/ClassesWithManyFields.qhelp

@@ -6,7 +6,7 @@
 
 <overview>
 <p>This rule finds classes with more than 15 instance (i.e., non-<code>static</code>) fields. Library classes
-are not shown. Having too many fields in one class is a sign that the class lacks cohesion (i.e. lacks a single purpose).
+are not shown. Having too many fields in one class is a sign that the class may lack cohesion (i.e. lack a single purpose).
 These classes can be split into smaller, more cohesive classes. Alternatively, the related fields can be grouped 
 into <code>struct</code>s.</p>
 

cpp/ql/src/Architecture/Refactoring Opportunities/ClassesWithManyFields.ql

@@ -110,5 +110,5 @@ where n = strictcount(string fieldName
       not c.isConstructedFrom(_) and
       c = vdg.getClass() and
       if c.hasOneVariableGroup() then suffix = "" else suffix = " - see $@"
-select c, kindstr(c) + " " + c.getName() + " has " + n + " fields, which is too many" + suffix + ".",
+select c, kindstr(c) + " " + c.getName() + " has " + n + " fields; we suggest refactoring to 15 fields or fewer" + suffix + ".",
        vdg, vdg.describeGroup()

cpp/ql/src/Likely Bugs/Memory Management/AllocaInLoop.ql

@@ -8,6 +8,7 @@
  *       correctness
  *       external/cwe/cwe-770
  */
+
 import cpp
 
 Loop getAnEnclosingLoopOfExpr(Expr e) {
@@ -21,7 +22,15 @@ Loop getAnEnclosingLoopOfStmt(Stmt s) {
 }
 
 from Loop l, FunctionCall fc
-where getAnEnclosingLoopOfExpr(fc) = l
-  and fc.getTarget().getName() = "__builtin_alloca"
-  and not l.(DoStmt).getCondition().getValue() = "0"
-select fc, "Stack allocation is inside a $@ and could lead to overflow.", l, l.toString()
+where
+  getAnEnclosingLoopOfExpr(fc) = l and
+  (
+    fc.getTarget().getName() = "__builtin_alloca"
+    or
+    (
+      (fc.getTarget().getName() = "_alloca" or fc.getTarget().getName() = "_malloca") and
+      fc.getTarget().getADeclarationEntry().getFile().getBaseName() = "malloc.h"
+    )
+  ) and
+  not l.(DoStmt).getCondition().getValue() = "0"
+select fc, "Stack allocation is inside a $@ and could lead to stack overflow.", l, l.toString()

cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql

@@ -56,7 +56,7 @@ where
     or
     // The data flow library doesn't support conversions, so here we check that
     // the address escapes into some expression `pointerToLocal`, which flows
-    // in a one or more steps to a returned expression.
+    // in one or more steps to a returned expression.
     exists(Expr pointerToLocal |
       variableAddressEscapesTree(va, pointerToLocal.getFullyConverted()) and
       not hasNontrivialConversion(pointerToLocal) and

cpp/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql

@@ -16,9 +16,14 @@ abstract class InsecureCryptoSpec extends Locatable {
   abstract string description();
 }
 
+Function getAnInsecureFunction() {
+  result.getName().regexpMatch(algorithmBlacklistRegex()) and
+  exists(result.getACallToThisFunction())
+}
+
 class InsecureFunctionCall extends InsecureCryptoSpec, FunctionCall {
   InsecureFunctionCall() {
-    this.getTarget().getName().regexpMatch(algorithmBlacklistRegex())
+    this.getTarget() = getAnInsecureFunction()
   }
 
   override string description() { result = "function call" }
@@ -27,9 +32,14 @@ class InsecureFunctionCall extends InsecureCryptoSpec, FunctionCall {
   override Location getLocation() { result = FunctionCall.super.getLocation() }
 }
 
+Macro getAnInsecureMacro() {
+  result.getName().regexpMatch(algorithmBlacklistRegex()) and
+  exists(result.getAnInvocation())
+}
+
 class InsecureMacroSpec extends InsecureCryptoSpec, MacroInvocation {
   InsecureMacroSpec() {
-    this.getMacro().getName().regexpMatch(algorithmBlacklistRegex())
+    this.getMacro() = getAnInsecureMacro()
   }
 
   override string description() { result = "macro invocation" }

cpp/ql/src/semmle/code/cpp/commons/Printf.qll

@@ -215,8 +215,9 @@ class FormatLiteral extends Literal {
   /**
    * Holds if the default meaning of `%s` is a `wchar_t *`, rather than
    * a `char *` (either way, `%S` will have the opposite meaning).
+   * DEPRECATED: Use getDefaultCharType() instead.
    */
-  predicate isWideCharDefault() {
+  deprecated predicate isWideCharDefault() {
     getUse().getTarget().(FormattingFunction).isWideCharDefault()
   }
 
@@ -671,37 +672,34 @@ class FormatLiteral extends Literal {
   }
 
   /**
-   * Gets the 'effective' char type character, that is, 'c' (meaning a `char`) or
-   * 'C' (meaning a `wchar_t`).
-   *  - in the base case this is the same as the format type character.
-   *  - for a `wprintf` or similar function call, the meanings are reversed.
-   *  - the size prefixes 'l'/'w' (long) and 'h' (short) override the
-   *    type character to effectively 'C' or 'c' respectively.
+   * Gets the char type required by the nth conversion specifier.
+   *  - in the base case this is the default for the formatting function
+   *    (e.g. `char` for `printf`, `wchar_t` for `wprintf`).
+   *  - the `%S` format character reverses wideness.
+   *  - the size prefixes 'l'/'w' and 'h' override the type character
+   *    to wide or single-byte characters respectively.
    */
-  private string getEffectiveCharConversionChar(int n) {
-    exists(string len, string conv | this.parseConvSpec(n, _, _, _, _, _, len, conv) and (conv = "c" or conv = "C") |
-      (len = "l" and result = "C") or
-      (len = "w" and result = "C") or
-      (len = "h" and result = "c") or
-      (len != "l" and len != "w" and len != "h" and (result = "c" or result = "C") and (if isWideCharDefault() then result != conv else result = conv))
-    )
-  }
-
   private Type getConversionType1b(int n) {
-    exists(string cnv | cnv = this.getEffectiveCharConversionChar(n) |
+    exists(string len, string conv |
+      this.parseConvSpec(n, _, _, _, _, _, len, conv) and
       (
-        cnv = "c" and
-        result instanceof CharType and
-        not result.(CharType).isExplicitlySigned() and
-        not result.(CharType).isExplicitlyUnsigned()
-      ) or (
-        cnv = "C" and
-        isMicrosoft() and
-        result instanceof WideCharType
-      ) or (
-        cnv = "C" and
-        not isMicrosoft() and
-        result.hasName("wint_t")
+        (
+          (conv = "c" or conv = "C") and
+          len = "h" and
+          result instanceof PlainCharType
+        ) or (
+          (conv = "c" or conv = "C") and
+          (len = "l" or len = "w") and
+          result = getWideCharType()
+        ) or (
+          conv = "c" and
+          (len != "l" and len != "w" and len != "h") and
+          result = getDefaultCharType()
+        ) or (
+          conv = "C" and
+          (len != "l" and len != "w" and len != "h") and
+          result = getNonDefaultCharType()
+        )
       )
     )
   }
@@ -846,15 +844,7 @@ class FormatLiteral extends Literal {
          len = 1
       or (
         this.getConversionChar(n).toLowerCase()="c" and
-        if (this.getEffectiveCharConversionChar(n)="C" and
-            not isMicrosoft() and
-            not isWideCharDefault()) then (
-          len = 6 // MB_LEN_MAX
-            // the wint_t (wide character) argument is converted
-            // to a multibyte sequence by a call to the wcrtomb(3) 
-        ) else (
-          len = 1 // e.g. 'a'
-        )
+        len = 1 // e.g. 'a'
       ) or this.getConversionChar(n).toLowerCase()="f" and
          exists(int dot, int afterdot |
            (if this.getPrecision(n) = 0 then dot = 0 else dot = 1)

cpp/ql/src/semmle/code/cpp/controlflow/SSAUtils.qll

@@ -4,10 +4,19 @@ import semmle.code.cpp.controlflow.SSA // must be imported for proper caching of
 import semmle.code.cpp.rangeanalysis.RangeSSA // must be imported for proper caching of SSAHelper
 
 /* The dominance frontier of a block `x` is the set of all blocks `w` such that
- * `x` dominates a predecessor of `w` but does not strictly dominate `w`. */
-pragma[noinline]
+ * `x` dominates a predecessor of `w` but does not strictly dominate `w`.
+ *
+ * This implementation is equivalent to:
+ *
+ *     bbDominates(x, w.getAPredecessor()) and not bbStrictlyDominates(x, w)
+ */
 private predicate dominanceFrontier(BasicBlock x, BasicBlock w) {
-    bbDominates(x, w.getAPredecessor()) and not bbStrictlyDominates(x, w)
+    x = w.getAPredecessor() and not bbIDominates(x, w)
+    or
+    exists(BasicBlock prev | dominanceFrontier(prev, w) |
+        bbIDominates(x, prev) and
+        not bbIDominates(x, w)
+    )
 }
 
 /**

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowDispatch.qll

@@ -2,11 +2,59 @@ private import cpp
 private import DataFlowPrivate
 
 Function viableImpl(MethodAccess ma) {
-  result = ma.getTarget()
+  result = viableCallable(ma)
 }
 
+/**
+ * Gets a function that might be called by `call`.
+ */
 Function viableCallable(Call call) {
   result = call.getTarget()
+  or
+  // If the target of the call does not have a body in the snapshot, it might
+  // be because the target is just a header declaration, and the real target
+  // will be determined at run time when the caller and callee are linked
+  // together by the operating system's dynamic linker. In case a _unique_
+  // function with the right signature is present in the database, we return
+  // that as a potential callee.
+  exists(string qualifiedName, int nparams |
+    callSignatureWithoutBody(qualifiedName, nparams, call) and
+    functionSignatureWithBody(qualifiedName, nparams, result) and
+    strictcount(Function other | functionSignatureWithBody(qualifiedName, nparams, other)) = 1
+  )
+}
+
+/**
+ * Holds if `f` is a function with a body that has name `qualifiedName` and
+ * `nparams` parameter count. See `functionSignature`.
+ */
+private predicate functionSignatureWithBody(string qualifiedName, int nparams, Function f) {
+  functionSignature(f, qualifiedName, nparams) and
+  exists(f.getBlock())
+}
+
+/**
+ * Holds if the target of `call` is a function _with no definition_ that has
+ * name `qualifiedName` and `nparams` parameter count. See `functionSignature`.
+ */
+pragma[noinline]
+private predicate callSignatureWithoutBody(string qualifiedName, int nparams, Call call) {
+  exists(Function target |
+    target = call.getTarget() and
+    not exists(target.getBlock()) and
+    functionSignature(target, qualifiedName, nparams)
+  )
+}
+
+/**
+ * Holds if `f` has name `qualifiedName` and `nparams` parameter count. This is
+ * an approximation of its signature for the purpose of matching functions that
+ * might be the same across link targets.
+ */
+private predicate functionSignature(Function f, string qualifiedName, int nparams) {
+  qualifiedName = f.getQualifiedName() and
+  nparams = f.getNumberOfParameters() and
+  not f.isStatic()
 }
 
 /**