Merge remote-tracking branch 'upstream/main' into work

change-notes/1.26/analysis-cpp.md

@@ -26,5 +26,6 @@ The following changes in version 1.26 affect C/C++ analysis in all applications.
 * The models library now models many taint flows through `std::istream` and `std::ostream`.
 * The models library now models some taint flows through `std::shared_ptr`, `std::unique_ptr`, `std::make_shared` and `std::make_unique`.
 * The models library now models many taint flows through `std::pair`, `std::map`, `std::unordered_map`, `std::set` and `std::unordered_set`.
+* The models library now models `bcopy`.
 * The `SimpleRangeAnalysis` library now supports multiplications of the form
   `e1 * e2` and `x *= e2` when `e1` and `e2` are unsigned or constant.

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

@@ -4,47 +4,25 @@
 
 import cpp
 import Nullness
+import semmle.code.cpp.models.interfaces.ArrayFunction
 
 /**
  * Holds if the call `fc` will dereference argument `i`.
  */
 predicate callDereferences(FunctionCall fc, int i) {
-  exists(string name |
-    fc.getTarget().hasGlobalOrStdName(name) and
+  exists(ArrayFunction af |
+    fc.getTarget() = af and
     (
-      name = "bcopy" and i in [0 .. 1]
-      or
-      name = "memcpy" and i in [0 .. 1]
-      or
-      name = "memmove" and i in [0 .. 1]
-      or
-      name = "strcpy" and i in [0 .. 1]
-      or
-      name = "strncpy" and i in [0 .. 1]
-      or
-      name = "strdup" and i = 0
-      or
-      name = "strndup" and i = 0
-      or
-      name = "strlen" and i = 0
-      or
-      name = "printf" and fc.getArgument(i).getType() instanceof PointerType
-      or
-      name = "fprintf" and fc.getArgument(i).getType() instanceof PointerType
-      or
-      name = "sprintf" and fc.getArgument(i).getType() instanceof PointerType
-      or
-      name = "snprintf" and fc.getArgument(i).getType() instanceof PointerType
-      or
-      name = "vprintf" and fc.getArgument(i).getType() instanceof PointerType
-      or
-      name = "vfprintf" and fc.getArgument(i).getType() instanceof PointerType
-      or
-      name = "vsprintf" and fc.getArgument(i).getType() instanceof PointerType
-      or
-      name = "vsnprintf" and fc.getArgument(i).getType() instanceof PointerType
+      af.hasArrayInput(i) or
+      af.hasArrayOutput(i)
     )
   )
+  or
+  exists(FormattingFunction ff |
+    fc.getTarget() = ff and
+    i >= ff.getFirstFormatArgumentIndex() and
+    fc.getArgument(i).getType() instanceof PointerType
+  )
 }
 
 /**

cpp/ql/src/semmle/code/cpp/models/implementations/Memcpy.qll

@@ -10,45 +10,57 @@ import semmle.code.cpp.models.interfaces.SideEffect
 import semmle.code.cpp.models.interfaces.Taint
 
 /**
- * The standard functions `memcpy` and `memmove`, and the gcc variant
- * `__builtin___memcpy_chk`
+ * The standard functions `memcpy`, `memmove` and `bcopy`; and the gcc variant
+ * `__builtin___memcpy_chk`.
  */
-class MemcpyFunction extends ArrayFunction, DataFlowFunction, SideEffectFunction, TaintFunction {
+class MemcpyFunction extends ArrayFunction, DataFlowFunction, SideEffectFunction {
   MemcpyFunction() {
-    this.hasName("memcpy") or
-    this.hasName("memmove") or
-    this.hasName("__builtin___memcpy_chk")
+    // memcpy(dest, src, num)
+    // memmove(dest, src, num)
+    // memmove(dest, src, num, remaining)
+    this.hasName(["memcpy", "memmove", "__builtin___memcpy_chk"])
+    or
+    // bcopy(src, dest, num)
+    this.hasGlobalOrStdName("bcopy")
   }
 
-  override predicate hasArrayInput(int bufParam) { bufParam = 1 }
+  /**
+   * Gets the index of the parameter that is the source buffer for the copy.
+   */
+  int getParamSrc() { if this.hasGlobalOrStdName("bcopy") then result = 0 else result = 1 }
 
-  override predicate hasArrayOutput(int bufParam) { bufParam = 0 }
+  /**
+   * Gets the index of the parameter that is the destination buffer for the
+   * copy.
+   */
+  int getParamDest() { if this.hasGlobalOrStdName("bcopy") then result = 1 else result = 0 }
+
+  /**
+   * Gets the index of the parameter that is the size of the copy (in bytes).
+   */
+  int getParamSize() { result = 2 }
+
+  override predicate hasArrayInput(int bufParam) { bufParam = getParamSrc() }
+
+  override predicate hasArrayOutput(int bufParam) { bufParam = getParamDest() }
 
   override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
-    input.isParameterDeref(1) and
-    output.isParameterDeref(0)
+    input.isParameterDeref(getParamSrc()) and
+    output.isParameterDeref(getParamDest())
     or
-    input.isParameterDeref(1) and
+    input.isParameterDeref(getParamSrc()) and
     output.isReturnValueDeref()
     or
-    input.isParameter(0) and
+    input.isParameter(getParamDest()) and
     output.isReturnValue()
   }
 
-  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
-    input.isParameter(2) and
-    output.isParameterDeref(0)
-    or
-    input.isParameter(2) and
-    output.isReturnValueDeref()
-  }
-
   override predicate hasArrayWithVariableSize(int bufParam, int countParam) {
     (
-      bufParam = 0 or
-      bufParam = 1
+      bufParam = getParamDest() or
+      bufParam = getParamSrc()
     ) and
-    countParam = 2
+    countParam = getParamSize()
   }
 
   override predicate hasOnlySpecificReadSideEffects() { any() }
@@ -56,18 +68,18 @@ class MemcpyFunction extends ArrayFunction, DataFlowFunction, SideEffectFunction
   override predicate hasOnlySpecificWriteSideEffects() { any() }
 
   override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) {
-    i = 0 and buffer = true and mustWrite = true
+    i = getParamDest() and buffer = true and mustWrite = true
   }
 
   override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
-    i = 1 and buffer = true
+    i = getParamSrc() and buffer = true
   }
 
   override ParameterIndex getParameterSizeIndex(ParameterIndex i) {
-    result = 2 and
+    result = getParamSize() and
     (
-      i = 0 or
-      i = 1
+      i = getParamDest() or
+      i = getParamSrc()
     )
   }
 }

cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected

@@ -5481,8 +5481,6 @@
 | taint.cpp:194:10:194:10 | x | taint.cpp:194:9:194:10 | & ... |  |
 | taint.cpp:194:13:194:18 | source | taint.cpp:194:2:194:7 | call to memcpy | TAINT |
 | taint.cpp:194:13:194:18 | source | taint.cpp:194:9:194:10 | ref arg & ... | TAINT |
-| taint.cpp:194:21:194:31 | sizeof(int) | taint.cpp:194:2:194:7 | call to memcpy | TAINT |
-| taint.cpp:194:21:194:31 | sizeof(int) | taint.cpp:194:9:194:10 | ref arg & ... | TAINT |
 | taint.cpp:207:6:207:11 | call to source | taint.cpp:207:2:207:13 | ... = ... |  |
 | taint.cpp:207:6:207:11 | call to source | taint.cpp:210:7:210:7 | x |  |
 | taint.cpp:207:6:207:11 | call to source | taint.cpp:213:12:213:12 | x |  |

cpp/ql/test/query-tests/Critical/MissingNullTest/MissingNullTest.expected

@@ -0,0 +1,6 @@
+| test.cpp:23:8:23:8 | p | Value may be null; it should be checked before dereferencing. |
+| test.cpp:35:10:35:10 | q | Value may be null; it should be checked before dereferencing. |
+| test.cpp:43:13:43:13 | q | Value may be null; it should be checked before dereferencing. |
+| test.cpp:51:17:51:17 | q | Value may be null; it should be checked before dereferencing. |
+| test.cpp:58:8:58:8 | p | Value may be null; it should be checked before dereferencing. |
+| test.cpp:67:8:67:8 | p | Value may be null; it should be checked before dereferencing. |

cpp/ql/test/query-tests/Critical/MissingNullTest/MissingNullTest.qlref

@@ -0,0 +1 @@
+Critical/MissingNullTest.ql

cpp/ql/test/query-tests/Critical/MissingNullTest/test.cpp

@@ -0,0 +1,71 @@
+
+#define NULL (0)
+
+typedef unsigned long size_t;
+
+void *memcpy(void *s1, const void *s2, size_t n);
+void bcopy(const void *source, void *dest, size_t amount);
+
+void mycopyint(const int *source, int *dest)
+{
+	*dest = *source;
+}
+
+void test1(bool cond)
+{
+	int x, y;
+	
+	{
+		int *p, *q;
+
+		y = *p; // BAD (p is uninitialized and could be 0) [NOT DETECTED]
+		p = NULL;
+		y = *p; // BAD (p is 0)
+		p = &x;
+		y = *p; // GOOD (p points to x)
+		p = q;
+		y = *p; // BAD (p is uninitialized and could be 0) [NOT DETECTED]
+	}
+
+	{
+		int *p = &x;
+		int *q = 0;
+
+		memcpy(p, &y, sizeof(int)); // GOOD (p points to x)
+		memcpy(q, &y, sizeof(int)); // BAD (p is 0)
+	}
+
+	{
+		int *p = &x;
+		int *q = 0;
+
+		bcopy(&y, p, sizeof(int)); // GOOD (p points to x)
+		bcopy(&y, q, sizeof(int)); // BAD (p is 0)
+	}
+
+	{
+		int *p = &x;
+		int *q = 0;
+
+		mycopyint(&y, p); // GOOD (p points to x)
+		mycopyint(&y, q); // BAD (p is 0)
+	}
+
+	{
+		int *p = 0;
+		int *q = &x;
+
+		y = *p; // BAD (p is 0)
+		memcpy(&p, &q, sizeof(p));
+		y = *p; // GOOD (p points to x)
+	}
+
+	{
+		int *p = 0;
+		int *q = &x;
+
+		y = *p; // BAD (p is 0)
+		bcopy(&q, &p, sizeof(p));
+		y = *p; // GOOD (p points to x)
+	}
+}

javascript/ql/src/LanguageFeatures/TemplateSyntaxInStringLiteral.ql

@@ -62,8 +62,8 @@ class CandidateStringLiteral extends StringLiteral {
 }
 
 /**
- * Holds if `obj` has a property for each template variable in `lit` and they occur as arguments
- * to the same call.
+ * Holds if there exists an object that has a property for each template variable in `lit` and
+ * they occur as arguments to the same call.
  *
  * This recognises a typical pattern in which template arguments are passed along with a string,
  * for example:
@@ -73,14 +73,14 @@ class CandidateStringLiteral extends StringLiteral {
  *        { url: url, name: name } );
  * ```
  */
-predicate providesTemplateVariablesFor(ObjectExpr obj, CandidateStringLiteral lit) {
-  exists(CallExpr call | call.getAnArgument() = obj and call.getAnArgument() = lit) and
-  forex(string name | lit.getAReferencedVariable() = name | hasProperty(obj, name))
+predicate hasObjectProvidingTemplateVariables(CandidateStringLiteral lit) {
+  exists(DataFlow::CallNode call, DataFlow::ObjectLiteralNode obj |
+    call.getAnArgument().getALocalSource() = obj and
+    call.getAnArgument().asExpr() = lit and
+    forex(string name | name = lit.getAReferencedVariable() | exists(obj.getAPropertyWrite(name)))
+  )
 }
 
-/** Holds if `object` has a property with the given `name`. */
-predicate hasProperty(ObjectExpr object, string name) { name = object.getAProperty().getName() }
-
 /**
  * Gets a declaration of variable `v` in `tl`, where `v` has the given `name` and
  * belongs to `scope`.
@@ -97,7 +97,7 @@ where
   decl = getDeclIn(v, s, name, lit.getTopLevel()) and
   lit.getAReferencedVariable() = name and
   lit.isInScope(s) and
-  not exists(ObjectExpr obj | providesTemplateVariablesFor(obj, lit)) and
+  not hasObjectProvidingTemplateVariables(lit) and
   not lit.getStringValue() = "${" + name + "}"
 select lit, "This string is not a template literal, but appears to reference the variable $@.",
   decl, v.getName()

javascript/ql/src/experimental/Security/CWE-090/LdapInjection.qll

@@ -14,5 +14,12 @@ module LdapInjection {
     override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
     override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+
+    override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+      exists(LdapjsParseFilter filter |
+        pred = filter.getArgument(0) and
+        succ = filter
+      )
+    }
   }
 }

javascript/ql/src/experimental/Security/CWE-090/LdapInjectionCustomizations.qll

@@ -5,9 +5,10 @@
  */
 
 import javascript
-import Ldapjs::Ldapjs
 
 module LdapInjection {
+  import Ldapjs::Ldapjs
+
   /**
    * A data flow source for LDAP injection vulnerabilities.
    */
@@ -70,16 +71,4 @@ module LdapInjection {
       )
     }
   }
-
-  /**
-   * A step through the parseFilter API (https://github.com/ldapjs/node-ldapjs/issues/181).
-   */
-  class StepThroughParseFilter extends TaintTracking::AdditionalTaintStep, DataFlow::CallNode {
-    StepThroughParseFilter() { this instanceof LdapjsParseFilter }
-
-    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      pred = this.getArgument(0) and
-      succ = this
-    }
-  }
 }

javascript/ql/test/query-tests/LanguageFeatures/TemplateSyntaxInStringLiteral/TemplateSyntaxInStringLiteral.expected

@@ -2,3 +2,4 @@
 | TemplateSyntaxInStringLiteral.js:17:15:17:40 | 'global ... alVar}' | This string is not a template literal, but appears to reference the variable $@. | TemplateSyntaxInStringLiteral.js:14:5:14:13 | globalVar | globalVar |
 | TemplateSyntaxInStringLiteral.js:19:11:19:36 | 'global ... alVar}' | This string is not a template literal, but appears to reference the variable $@. | TemplateSyntaxInStringLiteral.js:14:5:14:13 | globalVar | globalVar |
 | TemplateSyntaxInStringLiteral.js:28:15:28:21 | "${x} " | This string is not a template literal, but appears to reference the variable $@. | TemplateSyntaxInStringLiteral.js:25:14:25:14 | x | x |
+| TemplateSyntaxInStringLiteral.js:42:17:42:57 | "Name:  ... oobar}" | This string is not a template literal, but appears to reference the variable $@. | TemplateSyntaxInStringLiteral.js:37:11:37:16 | foobar | foobar |

javascript/ql/test/query-tests/LanguageFeatures/TemplateSyntaxInStringLiteral/TemplateSyntaxInStringLiteral.js

@@ -28,3 +28,16 @@ function baz(x){
     log.error("${x} ");
     log.error("${y} ");
 }
+
+
+function foo1() {
+    const aTemplateLitInScope = `Connecting to ${id}`;
+    const name = 2;
+    const date = 3;
+    const foobar = 4;
+
+    const data = {name: name, date: date};
+    writer.emit("Name: ${name}, Date: ${date}.", data); // OK 
+
+    writer.emit("Name: ${name}, Date: ${date}, ${foobar}", data); // NOT OK - `foobar` is not in `data`.     
+}