Merge branch 'main' of github.com:github/codeql into python-port-sql-injection

Author: yoff

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.

change-notes/1.26/analysis-javascript.md

@@ -2,10 +2,15 @@
 
 ## General improvements
 
+* Angular-specific taint sources and sinks are now recognized by the security queries.
+
 * Support for the following frameworks and libraries has been improved:
+  - [@angular/*](https://www.npmjs.com/package/@angular/core)
   - [AWS Serverless](https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/sam-resource-function.html)
   - [Alibaba Serverless](https://www.alibabacloud.com/help/doc-detail/156876.htm)
+  - [debounce](https://www.npmjs.com/package/debounce)
   - [bluebird](https://www.npmjs.com/package/bluebird)
+  - [call-limit](https://www.npmjs.com/package/call-limit)
   - [express](https://www.npmjs.com/package/express)
   - [fast-json-stable-stringify](https://www.npmjs.com/package/fast-json-stable-stringify)
   - [fast-safe-stringify](https://www.npmjs.com/package/fast-safe-stringify)
@@ -15,11 +20,15 @@
   - [json-stable-stringify](https://www.npmjs.com/package/json-stable-stringify)
   - [json-stringify-safe](https://www.npmjs.com/package/json-stringify-safe)
   - [json3](https://www.npmjs.com/package/json3)
+  - [jQuery throttle / debounce](https://github.com/cowboy/jquery-throttle-debounce)
   - [lodash](https://www.npmjs.com/package/lodash)
+  - [lodash.debounce](https://www.npmjs.com/package/lodash.debounce)
+  - [lodash.throttle](https://www.npmjs.com/package/lodash.throttle)
   - [needle](https://www.npmjs.com/package/needle)
   - [object-inspect](https://www.npmjs.com/package/object-inspect)
   - [pretty-format](https://www.npmjs.com/package/pretty-format)
   - [stringify-object](https://www.npmjs.com/package/stringify-object)
+  - [throttle-debounce](https://www.npmjs.com/package/throttle-debounce)
   - [underscore](https://www.npmjs.com/package/underscore)
 
 * Analyzing files with the ".cjs" extension is now supported.
@@ -43,7 +52,10 @@
 | Unsafe jQuery plugin (`js/unsafe-jquery-plugin`) | More results | This query now detects more unsafe uses of nested option properties. |
 | Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | More results | This query now recognizes some unsafe uses of `importScripts()` inside WebWorkers. |
 | Missing CSRF middleware (`js/missing-token-validation`) | More results | This query now recognizes writes to cookie and session variables as potentially vulnerable to CSRF attacks. |
+| Missing CSRF middleware (`js/missing-token-validation`) | Fewer results | This query now recognizes more ways of protecting against CSRF attacks. |
+| Client-side cross-site scripting (`js/xss`) | More results | This query now tracks data flow from `location.hash` more precisely. |
 
 
 ## Changes to libraries
 * The predicate `TypeAnnotation.hasQualifiedName` now works in more cases when the imported library was not present during extraction.
+* The class `DomBasedXss::Configuration` has been deprecated, as it has been split into `DomBasedXss::HtmlInjectionConfiguration` and `DomBasedXss::JQueryHtmlOrSelectorInjectionConfiguration`. Unless specifically working with jQuery sinks, subclasses should instead be based on `HtmlInjectionConfiguration`. To use both configurations in a query, see [Xss.ql](https://github.com/github/codeql/blob/main/javascript/ql/src/Security/CWE-079/Xss.ql) for an example.

cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.cpp

@@ -4,3 +4,5 @@ long j = i * i; //Wrong: due to overflow on the multiplication between ints,
 
 long k = (long) i * i; //Correct: the multiplication is done on longs instead of ints, 
                        //and will not overflow
+
+long l = static_cast<long>(i) * i; //Correct: modern C++

cpp/ql/src/semmle/code/cpp/Parameter.qll

@@ -36,7 +36,7 @@ class Parameter extends LocalScopeVariable, @parameter {
    *  1. The name given to the parameter at the function's definition or
    *     (for catch block parameters) at the catch block.
    *  2. A name given to the parameter at a function declaration.
-   *  3. The name "p#i" where i is the index of the parameter.
+   *  3. The name "(unnamed parameter i)" where i is the index of the parameter.
    */
   override string getName() {
     exists(VariableDeclarationEntry vde |
@@ -46,7 +46,7 @@ class Parameter extends LocalScopeVariable, @parameter {
     )
     or
     not exists(getANamedDeclarationEntry()) and
-    result = "p#" + this.getIndex().toString()
+    result = "(unnamed parameter " + this.getIndex().toString() + ")"
   }
 
   override string getAPrimaryQlClass() { result = "Parameter" }
@@ -111,7 +111,8 @@ class Parameter extends LocalScopeVariable, @parameter {
    * Holds if this parameter has a name.
    *
    * In other words, this predicate holds precisely when the result of
-   * `getName()` is not "p#i" (where `i` is the index of the parameter).
+   * `getName()` is not "(unnamed parameter i)" (where `i` is the index
+   * of the parameter).
    */
   predicate isNamed() { exists(getANamedDeclarationEntry()) }
 

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/ir/implementation/aliased_ssa/Instruction.qll

@@ -804,12 +804,26 @@ class CopyValueInstruction extends CopyInstruction, UnaryInstruction {
   final override UnaryOperand getSourceValueOperand() { result = getAnOperand() }
 }
 
+/**
+ * Gets a string describing the location pointed to by the specified address operand.
+ */
+private string getAddressOperandDescription(AddressOperand operand) {
+  result = operand.getDef().(VariableAddressInstruction).getIRVariable().toString()
+  or
+  not operand.getDef() instanceof VariableAddressInstruction and
+  result = "?"
+}
+
 /**
  * An instruction that returns a register result containing a copy of its memory operand.
  */
 class LoadInstruction extends CopyInstruction {
   LoadInstruction() { getOpcode() instanceof Opcode::Load }
 
+  final override string getImmediateString() {
+    result = getAddressOperandDescription(getSourceAddressOperand())
+  }
+
   /**
    * Gets the operand that provides the address of the value being loaded.
    */
@@ -829,6 +843,10 @@ class LoadInstruction extends CopyInstruction {
 class StoreInstruction extends CopyInstruction {
   StoreInstruction() { getOpcode() instanceof Opcode::Store }
 
+  final override string getImmediateString() {
+    result = getAddressOperandDescription(getDestinationAddressOperand())
+  }
+
   /**
    * Gets the operand that provides the address of the location to which the value will be stored.
    */

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll

@@ -804,12 +804,26 @@ class CopyValueInstruction extends CopyInstruction, UnaryInstruction {
   final override UnaryOperand getSourceValueOperand() { result = getAnOperand() }
 }
 
+/**
+ * Gets a string describing the location pointed to by the specified address operand.
+ */
+private string getAddressOperandDescription(AddressOperand operand) {
+  result = operand.getDef().(VariableAddressInstruction).getIRVariable().toString()
+  or
+  not operand.getDef() instanceof VariableAddressInstruction and
+  result = "?"
+}
+
 /**
  * An instruction that returns a register result containing a copy of its memory operand.
  */
 class LoadInstruction extends CopyInstruction {
   LoadInstruction() { getOpcode() instanceof Opcode::Load }
 
+  final override string getImmediateString() {
+    result = getAddressOperandDescription(getSourceAddressOperand())
+  }
+
   /**
    * Gets the operand that provides the address of the value being loaded.
    */
@@ -829,6 +843,10 @@ class LoadInstruction extends CopyInstruction {
 class StoreInstruction extends CopyInstruction {
   StoreInstruction() { getOpcode() instanceof Opcode::Store }
 
+  final override string getImmediateString() {
+    result = getAddressOperandDescription(getDestinationAddressOperand())
+  }
+
   /**
    * Gets the operand that provides the address of the location to which the value will be stored.
    */

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll

@@ -804,12 +804,26 @@ class CopyValueInstruction extends CopyInstruction, UnaryInstruction {
   final override UnaryOperand getSourceValueOperand() { result = getAnOperand() }
 }
 
+/**
+ * Gets a string describing the location pointed to by the specified address operand.
+ */
+private string getAddressOperandDescription(AddressOperand operand) {
+  result = operand.getDef().(VariableAddressInstruction).getIRVariable().toString()
+  or
+  not operand.getDef() instanceof VariableAddressInstruction and
+  result = "?"
+}
+
 /**
  * An instruction that returns a register result containing a copy of its memory operand.
  */
 class LoadInstruction extends CopyInstruction {
   LoadInstruction() { getOpcode() instanceof Opcode::Load }
 
+  final override string getImmediateString() {
+    result = getAddressOperandDescription(getSourceAddressOperand())
+  }
+
   /**
    * Gets the operand that provides the address of the value being loaded.
    */
@@ -829,6 +843,10 @@ class LoadInstruction extends CopyInstruction {
 class StoreInstruction extends CopyInstruction {
   StoreInstruction() { getOpcode() instanceof Opcode::Store }
 
+  final override string getImmediateString() {
+    result = getAddressOperandDescription(getDestinationAddressOperand())
+  }
+
   /**
    * Gets the operand that provides the address of the location to which the value will be stored.
    */

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

@@ -10,64 +10,76 @@ 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 }
+
+  /**
+   * 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 = 0 }
+  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() }
 
   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()
     )
   }
 }