Merge branch 'master-to-next-20180905-master' into master-to-next-20180905

2025-12-17 01:03:14 +01:00 · 2018-09-05 21:08:06 +02:00
parent 8fbc191bf0 d5e0357201
commit 69e915639d
14 changed files with 125 additions and 63 deletions
--- a/change-notes/1.19/analysis-javascript.md
+++ b/change-notes/1.19/analysis-javascript.md
@@ -0,0 +1,18 @@
+# Improvements to JavaScript analysis
+
+## General improvements
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+| *@name of query (Query ID)* | *Tags*    |*Aim of the new query and whether it is enabled by default or not*  |
+
+## Changes to existing queries
+
+| **Query**                      | **Expected impact**        | **Change**                                   |
+|--------------------------------|----------------------------|----------------------------------------------|
+| Regular expression injection | Fewer false-positive results | This rule now identifies calls to `String.prototype.search` with more precision. |
+
+
+## Changes to QL libraries
--- a/Bugs/Arithmetic/IntMultToLong.ql
+++ b/Bugs/Arithmetic/IntMultToLong.ql
@@ -103,4 +103,4 @@ where t1 = me.getType().getUnderlyingType() and
        ) and
        e.(Literal).getType().getSize() = t2.getSize()
      )
-select me, "Cast to '" + me.getFullyConverted().getType().toString() + "' before multiplication to avoid potential overflow."
+select me, "Multiplication result may overflow '" + me.getType().toString() + "' before it is converted to '" + me.getFullyConverted().getType().toString() + "'."
--- a/cpp/ql/src/semmle/code/cpp/Declaration.qll
+++ b/cpp/ql/src/semmle/code/cpp/Declaration.qll
@@ -87,11 +87,24 @@ abstract class Declaration extends Locatable, @declaration {

  override string toString() { result = this.getName() }

-  /** Gets the name of this declaration. */
+  /**
+   * Gets the name of this declaration.
+   *
+   * This name doesn't include a namespace or any argument types, so
+   * for example both functions `::open()` and `::std::ifstream::open(...)`
+   * have the same name.
+   *
+   * To get the name including the namespace, use `getQualifiedName` or
+   * `hasQualifiedName`.
+   *
+   * To test whether this declaration has a particular name in the global
+   * namespace, use `hasGlobalName`.
+   */
  abstract string getName();
+  /** Holds if this declaration has the given name. */
  predicate hasName(string name) { name = this.getName() }

-  /** Holds if this element has the given name in the global namespace. */
+  /** Holds if this declaration has the given name in the global namespace. */
  predicate hasGlobalName(string name) {
    hasName(name)
    and getNamespace() instanceof GlobalNamespace
--- a/cpp/ql/src/semmle/code/cpp/Function.qll
+++ b/cpp/ql/src/semmle/code/cpp/Function.qll
@@ -17,19 +17,6 @@ private import semmle.code.cpp.internal.ResolveClass
 * in more detail in `Declaration.qll`.
 */
 class Function extends Declaration, ControlFlowNode, AccessHolder, @function {
-
-  /**
-   * Gets the name of this function.
-   *
-   * This name doesn't include a namespace or any argument types, so both
-   * `::open()` and `::std::ifstream::open(...)` have the same name.
-   *
-   * To get the name including the namespace, use `getQualifiedName` or
-   * `hasQualifiedName`.
-   *
-   * To test whether a function has a particular name in the global
-   * namespace, use `hasGlobalName`.
-   */
  override string getName() { functions(underlyingElement(this),result,_) }

  /**
--- a/cpp/ql/src/semmle/code/cpp/Namespace.qll
+++ b/cpp/ql/src/semmle/code/cpp/Namespace.qll
@@ -190,11 +190,6 @@ class GlobalNamespace extends Namespace {

  override Declaration getADeclaration() {
    suppressWarningForUnused(this) and
-    not exists(DeclStmt d |
-      d.getADeclaration() = result and
-      not result instanceof Function
-    ) and
-    not exists(ConditionDeclExpr cde | cde.getVariable() = result) and
    result.isTopLevel() and
    not namespacembrs(_, unresolveElement(result))
  }
--- a/cpp/ql/src/semmle/code/cpp/Type.qll
+++ b/cpp/ql/src/semmle/code/cpp/Type.qll
@@ -1155,8 +1155,6 @@ class TemplateParameter extends UserType
 {
  TemplateParameter() { usertypes(underlyingElement(this), _, 7) or usertypes(underlyingElement(this), _, 8) }

-  override string getName() { usertypes(underlyingElement(this), result, _) }
-
  override predicate involvesTemplateParameter() {
    any()
  }
--- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRBlockConstruction.qll
+++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRBlockConstruction.qll
@@ -32,15 +32,29 @@ private cached module Cached {
      startsBasicBlock(firstInstr)
    }

+  /** Holds if `i2` follows `i1` in a `IRBlock`. */
+  private predicate adjacentInBlock(Instruction i1, Instruction i2) {
+    exists(GotoEdge edgeKind | i2 = i1.getSuccessor(edgeKind)) and
+    not startsBasicBlock(i2)
+  }
+
+  /** Gets the index of `i` in its `IRBlock`. */
+  private int getMemberIndex(Instruction i) {
+    startsBasicBlock(i) and
+    result = 0
+    or
+    exists(Instruction iPrev |
+      adjacentInBlock(iPrev, i) and
+      result = getMemberIndex(iPrev) + 1
+    )
+  }
+
+  /** Holds if `i` is the `index`th instruction in `block`. */
  cached Instruction getInstruction(TIRBlock block, int index) {
-    index = 0 and block = MkIRBlock(result) or
-    (
-      index > 0 and
-      not startsBasicBlock(result) and
-      exists(Instruction predecessor, GotoEdge edge |
-        predecessor = getInstruction(block, index - 1) and
-        result = predecessor.getSuccessor(edge)
-      )
+    exists(Instruction first |
+      block = MkIRBlock(first) and
+      index = getMemberIndex(result) and
+      adjacentInBlock*(first, result)
    )
  }

--- a/cpp/ql/test/library-tests/namespaces/namespaces/decls.expected
+++ b/cpp/ql/test/library-tests/namespaces/namespaces/decls.expected
@@ -1,18 +1,21 @@
-| file://:0:0:0:0 | (global namespace) | file://:0:0:0:0 | __va_list_tag | __va_list_tag |
-| file://:0:0:0:0 | (global namespace) | file://:0:0:0:0 | fp_offset | __va_list_tag::fp_offset |
-| file://:0:0:0:0 | (global namespace) | file://:0:0:0:0 | gp_offset | __va_list_tag::gp_offset |
-| file://:0:0:0:0 | (global namespace) | file://:0:0:0:0 | operator= | __va_list_tag::operator= |
-| file://:0:0:0:0 | (global namespace) | file://:0:0:0:0 | operator= | __va_list_tag::operator= |
-| file://:0:0:0:0 | (global namespace) | file://:0:0:0:0 | overflow_arg_area | __va_list_tag::overflow_arg_area |
-| file://:0:0:0:0 | (global namespace) | file://:0:0:0:0 | reg_save_area | __va_list_tag::reg_save_area |
-| file://:0:0:0:0 | B | namespaces.cpp:32:7:32:7 | x | B::x |
-| namespaces.cpp:11:13:11:13 | C::D | file://:0:0:0:0 | p#0 | <none> |
-| namespaces.cpp:11:13:11:13 | C::D | file://:0:0:0:0 | p#0 | <none> |
-| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:13:17:13:17 | f | C::D::f |
-| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:15:12:15:12 | E | C::D::E |
-| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:15:12:15:12 | operator= | C::D::E::operator= |
-| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:15:12:15:12 | operator= | C::D::E::operator= |
-| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:17:12:17:12 | g | C::D::E::g |
-| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:17:18:17:18 | p | <none> |
-| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:18:12:18:12 | a | <none> |
-| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:20:13:20:13 | b | <none> |
+| file://:0:0:0:0 | (global namespace) | file://:0:0:0:0 | __va_list_tag | __va_list_tag | true |
+| file://:0:0:0:0 | (global namespace) | file://:0:0:0:0 | fp_offset | __va_list_tag::fp_offset | false |
+| file://:0:0:0:0 | (global namespace) | file://:0:0:0:0 | gp_offset | __va_list_tag::gp_offset | false |
+| file://:0:0:0:0 | (global namespace) | file://:0:0:0:0 | operator= | __va_list_tag::operator= | false |
+| file://:0:0:0:0 | (global namespace) | file://:0:0:0:0 | operator= | __va_list_tag::operator= | false |
+| file://:0:0:0:0 | (global namespace) | file://:0:0:0:0 | overflow_arg_area | __va_list_tag::overflow_arg_area | false |
+| file://:0:0:0:0 | (global namespace) | file://:0:0:0:0 | reg_save_area | __va_list_tag::reg_save_area | false |
+| file://:0:0:0:0 | (global namespace) | namespaces.cpp:40:5:40:13 | globalInt | globalInt | true |
+| file://:0:0:0:0 | (global namespace) | namespaces.cpp:42:6:42:18 | globalIntUser | globalIntUser | true |
+| file://:0:0:0:0 | <none> | file://:0:0:0:0 | auto | <none> | false |
+| file://:0:0:0:0 | B | namespaces.cpp:32:7:32:7 | x | B::x | true |
+| namespaces.cpp:11:13:11:13 | C::D | file://:0:0:0:0 | p#0 | <none> | false |
+| namespaces.cpp:11:13:11:13 | C::D | file://:0:0:0:0 | p#0 | <none> | false |
+| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:13:17:13:17 | f | C::D::f | true |
+| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:15:12:15:12 | E | C::D::E | true |
+| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:15:12:15:12 | operator= | C::D::E::operator= | false |
+| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:15:12:15:12 | operator= | C::D::E::operator= | false |
+| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:17:12:17:12 | g | C::D::E::g | false |
+| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:17:18:17:18 | p | <none> | false |
+| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:18:12:18:12 | a | <none> | false |
+| namespaces.cpp:11:13:11:13 | C::D | namespaces.cpp:20:13:20:13 | b | <none> | false |
--- a/cpp/ql/test/library-tests/namespaces/namespaces/decls.ql
+++ b/cpp/ql/test/library-tests/namespaces/namespaces/decls.ql
@@ -6,7 +6,24 @@ string qual(Declaration d) {
  else result = "<none>"
 }

-from Namespace n, Declaration d
-where n = d.getNamespace()
-select n, d, qual(d)
+newtype TMaybeNamespace = SomeNamespace(Namespace ns) or NoNamespace()

+class MaybeNamespace extends TMaybeNamespace {
+  string toString() {
+    this = NoNamespace() and result = "<none>"
+    or
+    exists(Namespace ns | this = SomeNamespace(ns) and result = ns.toString())
+  }
+
+  Location getLocation() {
+    exists(Namespace ns |
+      this = SomeNamespace(ns) and result = ns.getLocation())
+  }
+}
+
+from MaybeNamespace n, Declaration d
+where n = SomeNamespace(d.getNamespace())
+   or n = NoNamespace() and not exists(d.getNamespace())
+select n, d,
+       qual(d),
+       any(boolean b | if d.isTopLevel() then b = true else b = false)
--- a/cpp/ql/test/library-tests/namespaces/namespaces/namespaces.cpp
+++ b/cpp/ql/test/library-tests/namespaces/namespaces/namespaces.cpp
@@ -36,3 +36,9 @@ namespace B {
 namespace std {

 }
+
+int globalInt;
+
+void globalIntUser(void) {
+    extern int globalInt;
+}
--- a/Bugs/Arithmetic/IntMultToLong/IntMultToLong.expected
+++ b/Bugs/Arithmetic/IntMultToLong/IntMultToLong.expected
@@ -1,9 +1,9 @@
-| IntMultToLong.c:4:10:4:14 | ... * ... | Cast to 'long long' before multiplication to avoid potential overflow. |
-| IntMultToLong.c:7:16:7:20 | ... * ... | Cast to 'long long' before multiplication to avoid potential overflow. |
-| IntMultToLong.c:18:19:18:23 | ... * ... | Cast to 'double' before multiplication to avoid potential overflow. |
-| IntMultToLong.c:21:19:21:29 | ... * ... | Cast to 'double' before multiplication to avoid potential overflow. |
-| IntMultToLong.c:38:19:38:23 | ... * ... | Cast to 'double' before multiplication to avoid potential overflow. |
-| IntMultToLong.c:59:20:59:31 | ... * ... | Cast to 'double' before multiplication to avoid potential overflow. |
-| IntMultToLong.c:61:23:61:33 | ... * ... | Cast to 'long long' before multiplication to avoid potential overflow. |
-| IntMultToLong.c:63:23:63:40 | ... * ... | Cast to 'long long' before multiplication to avoid potential overflow. |
-| IntMultToLong.c:75:9:75:13 | ... * ... | Cast to 'size_t' before multiplication to avoid potential overflow. |
+| IntMultToLong.c:4:10:4:14 | ... * ... | Multiplication result may overflow 'int' before it is converted to 'long long'. |
+| IntMultToLong.c:7:16:7:20 | ... * ... | Multiplication result may overflow 'int' before it is converted to 'long long'. |
+| IntMultToLong.c:18:19:18:23 | ... * ... | Multiplication result may overflow 'float' before it is converted to 'double'. |
+| IntMultToLong.c:21:19:21:29 | ... * ... | Multiplication result may overflow 'float' before it is converted to 'double'. |
+| IntMultToLong.c:38:19:38:23 | ... * ... | Multiplication result may overflow 'float' before it is converted to 'double'. |
+| IntMultToLong.c:59:20:59:31 | ... * ... | Multiplication result may overflow 'float' before it is converted to 'double'. |
+| IntMultToLong.c:61:23:61:33 | ... * ... | Multiplication result may overflow 'int' before it is converted to 'long long'. |
+| IntMultToLong.c:63:23:63:40 | ... * ... | Multiplication result may overflow 'int' before it is converted to 'long long'. |
+| IntMultToLong.c:75:9:75:13 | ... * ... | Multiplication result may overflow 'int' before it is converted to 'size_t'. |
--- a/javascript/ql/src/semmle/javascript/security/dataflow/RegExpInjection.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/RegExpInjection.qll
@@ -69,7 +69,13 @@ module RegExpInjection {
        mce.getReceiver().analyze().getAType() = TTString() and
        mce.getMethodName() = methodName |
        (methodName = "match" and this.asExpr() = mce.getArgument(0) and mce.getNumArgument() = 1) or
-        (methodName = "search" and this.asExpr() = mce.getArgument(0) and mce.getNumArgument() = 1)
+        (
+           methodName = "search" and
+           this.asExpr() = mce.getArgument(0) and
+           mce.getNumArgument() = 1 and
+           // `String.prototype.search` returns a number, so exclude chained accesses
+           not exists(PropAccess p | p.getBase() = mce)
+        )
      )
    }

--- a/javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.expected
@@ -10,4 +10,5 @@
 | RegExpInjection.js:45:20:45:24 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
 | RegExpInjection.js:46:23:46:27 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
 | RegExpInjection.js:47:22:47:26 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
+| RegExpInjection.js:50:46:50:50 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
 | tst.js:3:16:3:35 | "^"+ data.name + "$" | This regular expression is constructed from a $@. | tst.js:1:46:1:46 | e | user-provided value |
--- a/javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.js
+++ b/javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.js
@@ -1,6 +1,6 @@
 var express = require('express');
 var app = express();
-
+var URI = reuires("urijs");
 app.get('/findKey', function(req, res) {
  var key = req.param("key"), input = req.param("input");

@@ -46,4 +46,8 @@ app.get('/findKey', function(req, res) {
  likelyString.search(input); // NOT OK
  maybeString.search(input); // NOT OK
  notString.search(input); // OK
+
+  URI(`${protocol}://${host}${path}`).search(input); // OK, but still flagged
+  URI(`${protocol}://${host}${path}`).search(input).href(); // OK
+  unknown.search(input).unknown; // OK
 });