wip: flow summaries for ActionController::Parameters

Java: Timing attacks while comparing the headers value

cpp/config/suites/c/correctness

@@ -31,6 +31,7 @@
 + semmlecode-cpp-queries/Critical/NewArrayDeleteMismatch.ql: /Correctness/Common Errors
 + semmlecode-cpp-queries/Critical/NewDeleteArrayMismatch.ql: /Correctness/Common Errors
 + semmlecode-cpp-queries/Critical/NewFreeMismatch.ql: /Correctness/Common Errors
++ semmlecode-cpp-queries/Likely Bugs/Memory Management/UsingExpiredStackAddress.ql: /Correctness/Common Errors
   # Use of Libraries
 + semmlecode-cpp-queries/Likely Bugs/Memory Management/SuspiciousCallToMemset.ql: /Correctness/Use of Libraries
 + semmlecode-cpp-queries/Likely Bugs/Memory Management/SuspiciousSizeof.ql: /Correctness/Use of Libraries

cpp/config/suites/cpp/correctness

@@ -34,6 +34,7 @@
 + semmlecode-cpp-queries/Critical/NewArrayDeleteMismatch.ql: /Correctness/Common Errors
 + semmlecode-cpp-queries/Critical/NewDeleteArrayMismatch.ql: /Correctness/Common Errors
 + semmlecode-cpp-queries/Critical/NewFreeMismatch.ql: /Correctness/Common Errors
++ semmlecode-cpp-queries/Likely Bugs/Memory Management/UsingExpiredStackAddress.ql: /Correctness/Common Errors
   # Exceptions
 + semmlecode-cpp-queries/Best Practices/Exceptions/AccidentalRethrow.ql: /Correctness/Exceptions
 + semmlecode-cpp-queries/Best Practices/Exceptions/CatchingByValue.ql: /Correctness/Exceptions

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

@@ -0,0 +1,25 @@
+static const int* xptr;
+
+void localAddressEscapes() {
+  int x = 0;
+  xptr = &x;
+}
+
+void example1() {
+  localAddressEscapes();
+  const int* x = xptr; // BAD: This pointer points to expired stack allocated memory.
+}
+
+void localAddressDoesNotEscape() {
+  int x = 0;
+  xptr = &x;
+  // ...
+  // use `xptr`
+  // ...
+  xptr = nullptr;
+}
+
+void example2() {
+  localAddressDoesNotEscape();
+  const int* x = xptr; // GOOD: This pointer does not point to expired memory.
+}

cpp/ql/src/Likely Bugs/Memory Management/UsingExpiredStackAddress.qhelp

@@ -0,0 +1,49 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+This rule finds uses of pointers that likely point to local variables in
+expired stack frames. A pointer to a local variable is only valid
+until the function returns, after which it becomes a dangling pointer.
+</p>
+
+</overview>
+<recommendation>
+
+<ol>
+
+<li>
+If it is necessary to take the address of a local variable, then make
+sure that the address is only stored in memory that does not outlive
+the local variable. For example, it is safe to store the address in
+another local variable. Similarly, it is also safe to pass the address
+of a local variable to another function provided that the other
+function only uses it locally and does not store it in non-local
+memory.
+</li>
+<li>
+If it is necessary to store an address which will outlive the
+current function scope, then it should be allocated on the heap. Care
+should be taken to make sure that the memory is deallocated when it is
+no longer needed, particularly when using low-level memory management
+routines such as <tt>malloc</tt>/<tt>free</tt> or
+<tt>new</tt>/<tt>delete</tt>. Modern C++ applications often use smart
+pointers, such as <tt>std::shared_ptr</tt>, to reduce the chance of
+a memory leak.
+</li>
+</ol>
+
+</recommendation>
+<example>
+
+<sample src="UsingExpiredStackAddress.cpp" />
+
+</example>
+<references>
+
+<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Dangling_pointer">Dangling pointer</a>.</li>
+
+</references>
+</qhelp>

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

@@ -0,0 +1,247 @@
+/**
+ * @name Use of expired stack-address
+ * @description Accessing the stack-allocated memory of a function
+ *              after it has returned can lead to memory corruption.
+ * @kind problem
+ * @problem.severity error
+ * @security-severity 9.3
+ * @precision high
+ * @id cpp/using-expired-stack-address
+ * @tags reliability
+ *       security
+ *       external/cwe/cwe-825
+ */
+
+import cpp
+// We don't actually use the global value numbering library in this query, but without it we end up
+// recomputing the IR.
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+import semmle.code.cpp.ir.IR
+
+predicate instructionHasVariable(VariableAddressInstruction vai, StackVariable var, Function f) {
+  var = vai.getASTVariable() and
+  f = vai.getEnclosingFunction() and
+  // Pointer-to-member types aren't properly handled in the dbscheme.
+  not vai.getResultType() instanceof PointerToMemberType and
+  // Rule out FPs caused by extraction errors.
+  not any(ErrorExpr e).getEnclosingFunction() = f
+}
+
+/**
+ * Holds if `source` is the base address of an address computation whose
+ * result is stored in `address`.
+ */
+predicate stackPointerFlowsToUse(Instruction address, VariableAddressInstruction source) {
+  address = source and
+  instructionHasVariable(source, _, _)
+  or
+  stackPointerFlowsToUse(address.(CopyInstruction).getSourceValue(), source)
+  or
+  stackPointerFlowsToUse(address.(ConvertInstruction).getUnary(), source)
+  or
+  stackPointerFlowsToUse(address.(CheckedConvertOrNullInstruction).getUnary(), source)
+  or
+  stackPointerFlowsToUse(address.(InheritanceConversionInstruction).getUnary(), source)
+  or
+  stackPointerFlowsToUse(address.(FieldAddressInstruction).getObjectAddress(), source)
+  or
+  stackPointerFlowsToUse(address.(PointerOffsetInstruction).getLeft(), source)
+}
+
+/**
+ * A HashCons-like table for comparing addresses that are
+ * computed relative to some global variable.
+ */
+newtype TGlobalAddress =
+  TGlobalVariable(GlobalOrNamespaceVariable v) {
+    // Pointer-to-member types aren't properly handled in the dbscheme.
+    not v.getUnspecifiedType() instanceof PointerToMemberType
+  } or
+  TLoad(TGlobalAddress address) {
+    address = globalAddress(any(LoadInstruction load).getSourceAddress())
+  } or
+  TConversion(string kind, TGlobalAddress address, Type fromType, Type toType) {
+    kind = "unchecked" and
+    exists(ConvertInstruction convert |
+      uncheckedConversionTypes(convert, fromType, toType) and
+      address = globalAddress(convert.getUnary())
+    )
+    or
+    kind = "checked" and
+    exists(CheckedConvertOrNullInstruction convert |
+      checkedConversionTypes(convert, fromType, toType) and
+      address = globalAddress(convert.getUnary())
+    )
+    or
+    kind = "inheritance" and
+    exists(InheritanceConversionInstruction convert |
+      inheritanceConversionTypes(convert, fromType, toType) and
+      address = globalAddress(convert.getUnary())
+    )
+  } or
+  TFieldAddress(TGlobalAddress address, Field f) {
+    exists(FieldAddressInstruction fai |
+      fai.getField() = f and
+      address = globalAddress(fai.getObjectAddress())
+    )
+  }
+
+pragma[noinline]
+predicate uncheckedConversionTypes(ConvertInstruction convert, Type fromType, Type toType) {
+  fromType = convert.getUnary().getResultType() and
+  toType = convert.getResultType()
+}
+
+pragma[noinline]
+predicate checkedConversionTypes(CheckedConvertOrNullInstruction convert, Type fromType, Type toType) {
+  fromType = convert.getUnary().getResultType() and
+  toType = convert.getResultType()
+}
+
+pragma[noinline]
+predicate inheritanceConversionTypes(
+  InheritanceConversionInstruction convert, Type fromType, Type toType
+) {
+  fromType = convert.getUnary().getResultType() and
+  toType = convert.getResultType()
+}
+
+/** Gets the HashCons value of an address computed by `instr`, if any. */
+TGlobalAddress globalAddress(Instruction instr) {
+  result = TGlobalVariable(instr.(VariableAddressInstruction).getASTVariable())
+  or
+  not instr instanceof LoadInstruction and
+  result = globalAddress(instr.(CopyInstruction).getSourceValue())
+  or
+  exists(LoadInstruction load | instr = load |
+    result = TLoad(globalAddress(load.getSourceAddress()))
+  )
+  or
+  exists(ConvertInstruction convert, Type fromType, Type toType | instr = convert |
+    uncheckedConversionTypes(convert, fromType, toType) and
+    result = TConversion("unchecked", globalAddress(convert.getUnary()), fromType, toType)
+  )
+  or
+  exists(CheckedConvertOrNullInstruction convert, Type fromType, Type toType | instr = convert |
+    checkedConversionTypes(convert, fromType, toType) and
+    result = TConversion("checked", globalAddress(convert.getUnary()), fromType, toType)
+  )
+  or
+  exists(InheritanceConversionInstruction convert, Type fromType, Type toType | instr = convert |
+    inheritanceConversionTypes(convert, fromType, toType) and
+    result = TConversion("inheritance", globalAddress(convert.getUnary()), fromType, toType)
+  )
+  or
+  exists(FieldAddressInstruction fai | instr = fai |
+    result = TFieldAddress(globalAddress(fai.getObjectAddress()), fai.getField())
+  )
+  or
+  result = globalAddress(instr.(PointerOffsetInstruction).getLeft())
+}
+
+/** Gets a `StoreInstruction` that may be executed after executing `store`. */
+pragma[inline]
+StoreInstruction getAStoreStrictlyAfter(StoreInstruction store) {
+  exists(IRBlock block, int index1, int index2 |
+    block.getInstruction(index1) = store and
+    block.getInstruction(index2) = result and
+    index2 > index1
+  )
+  or
+  exists(IRBlock block1, IRBlock block2 |
+    store.getBlock() = block1 and
+    result.getBlock() = block2 and
+    block1.getASuccessor+() = block2
+  )
+}
+
+/**
+ * Holds if `store` copies the address of `f`'s local variable `var`
+ * into the address `globalAddress`.
+ */
+predicate stackAddressEscapes(
+  StoreInstruction store, StackVariable var, TGlobalAddress globalAddress, Function f
+) {
+  globalAddress = globalAddress(store.getDestinationAddress()) and
+  exists(VariableAddressInstruction vai |
+    instructionHasVariable(pragma[only_bind_into](vai), var, f) and
+    stackPointerFlowsToUse(store.getSourceValue(), vai)
+  ) and
+  // Ensure there's no subsequent store that overrides the global address.
+  not globalAddress = globalAddress(getAStoreStrictlyAfter(store).getDestinationAddress())
+}
+
+predicate blockStoresToAddress(
+  IRBlock block, int index, StoreInstruction store, TGlobalAddress globalAddress
+) {
+  block.getInstruction(index) = store and
+  globalAddress = globalAddress(store.getDestinationAddress())
+}
+
+predicate blockLoadsFromAddress(
+  IRBlock block, int index, LoadInstruction load, TGlobalAddress globalAddress
+) {
+  block.getInstruction(index) = load and
+  globalAddress = globalAddress(load.getSourceAddress())
+}
+
+predicate globalAddressPointsToStack(
+  StoreInstruction store, StackVariable var, CallInstruction call, IRBlock block,
+  TGlobalAddress globalAddress, boolean isCallBlock, boolean isStoreBlock
+) {
+  (
+    if blockStoresToAddress(block, _, _, globalAddress)
+    then isStoreBlock = true
+    else isStoreBlock = false
+  ) and
+  (
+    isCallBlock = true and
+    exists(Function f |
+      stackAddressEscapes(store, var, globalAddress, f) and
+      call.getStaticCallTarget() = f and
+      call.getBlock() = block
+    )
+    or
+    isCallBlock = false and
+    exists(IRBlock mid |
+      mid.immediatelyDominates(block) and
+      // Only recurse if there is no store to `globalAddress` in `mid`.
+      globalAddressPointsToStack(store, var, call, mid, globalAddress, _, false)
+    )
+  )
+}
+
+from
+  StoreInstruction store, StackVariable var, LoadInstruction load, CallInstruction call,
+  IRBlock block, boolean isCallBlock, TGlobalAddress address, boolean isStoreBlock
+where
+  globalAddressPointsToStack(store, var, call, block, address, isCallBlock, isStoreBlock) and
+  block.getAnInstruction() = load and
+  globalAddress(load.getSourceAddress()) = address and
+  (
+    // We know that we have a sequence:
+    // (1) store to `address` -> (2) return from `f` -> (3) load from `address`.
+    // But if (2) and (3) happen in the sam block we need to check the
+    // block indices to ensure that (3) happens after (2).
+    if isCallBlock = true
+    then
+      // If so, the load must happen after the call.
+      exists(int callIndex, int loadIndex |
+        blockLoadsFromAddress(_, loadIndex, load, _) and
+        block.getInstruction(callIndex) = call and
+        callIndex < loadIndex
+      )
+    else any()
+  ) and
+  // If there is a store to the address we need to make sure that the load we found was
+  // before that store (So that the load doesn't read an overwritten value).
+  if isStoreBlock = true
+  then
+    exists(int storeIndex, int loadIndex |
+      blockStoresToAddress(block, storeIndex, _, address) and
+      block.getInstruction(loadIndex) = load and
+      loadIndex < storeIndex
+    )
+  else any()
+select load, "Stack variable $@ escapes $@ and is used after it has expired.", var, var.toString(),
+  store, "here"

cpp/ql/src/Security/CWE/CWE-319/UseOfHttp.ql

@@ -3,7 +3,7 @@
  * @description Non-HTTPS connections can be intercepted by third parties.
  * @kind path-problem
  * @problem.severity warning
- * @precision medium
+ * @precision high
  * @id cpp/non-https-url
  * @tags security
  *       external/cwe/cwe-319
@@ -12,6 +12,7 @@
 
 import cpp
 import semmle.code.cpp.dataflow.TaintTracking
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import DataFlow::PathGraph
 
 /**
@@ -57,7 +58,12 @@ class HttpStringToUrlOpenConfig extends TaintTracking::Configuration {
 
   override predicate isSource(DataFlow::Node src) {
     // Sources are strings containing an HTTP URL not in a private domain.
-    src.asExpr() instanceof HttpStringLiteral
+    src.asExpr() instanceof HttpStringLiteral and
+    // block taint starting at `strstr`, which is likely testing an existing URL, rather than constructing an HTTP URL.
+    not exists(FunctionCall fc |
+      fc.getTarget().getName() = ["strstr", "strcasestr"] and
+      fc.getArgument(1) = globalValueNumber(src.asExpr()).getAnExpr()
+    )
   }
 
   override predicate isSink(DataFlow::Node sink) {

cpp/ql/src/change-notes/2022-02-22-using-expired-stack-address.md

@@ -0,0 +1,6 @@
+---
+category: newQuery
+---
+
+- A new query titled "Use of expired stack-address" (`cpp/using-expired-stack-address`) has been added.
+  This query finds accesses to expired stack-allocated memory that escaped via a global variable.

cpp/ql/src/change-notes/2022-02-24-non-https-url.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The "Failure to use HTTPS URLs" (`cpp/non-https-url`) has been improved reducing false positive results, and its precision has been increased to 'high'.

cpp/ql/test/query-tests/Likely Bugs/Memory Management/UsingExpiredStackAddress/UsingExpiredStackAddress.expected

@@ -0,0 +1,24 @@
+| test.cpp:15:16:15:16 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:9:7:9:7 | x | x | test.cpp:10:3:10:13 | Store: ... = ... | here |
+| test.cpp:58:16:58:16 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:51:36:51:36 | y | y | test.cpp:52:3:52:13 | Store: ... = ... | here |
+| test.cpp:73:16:73:16 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:62:7:62:7 | x | x | test.cpp:68:3:68:13 | Store: ... = ... | here |
+| test.cpp:98:15:98:15 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:92:8:92:8 | s | s | test.cpp:93:3:93:15 | Store: ... = ... | here |
+| test.cpp:111:16:111:16 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:102:7:102:7 | x | x | test.cpp:106:3:106:14 | Store: ... = ... | here |
+| test.cpp:161:16:161:17 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:132:7:132:8 | b1 | b1 | test.cpp:136:3:136:12 | Store: ... = ... | here |
+| test.cpp:162:16:162:17 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:132:7:132:8 | b1 | b1 | test.cpp:137:3:137:16 | Store: ... = ... | here |
+| test.cpp:164:16:164:17 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:139:3:139:12 | Store: ... = ... | here |
+| test.cpp:165:16:165:17 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:139:3:139:12 | Store: ... = ... | here |
+| test.cpp:166:17:166:18 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:140:3:140:16 | Store: ... = ... | here |
+| test.cpp:167:16:167:17 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:141:3:141:15 | Store: ... = ... | here |
+| test.cpp:168:17:168:18 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:142:3:142:19 | Store: ... = ... | here |
+| test.cpp:170:16:170:17 | Load: p3 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:144:3:144:12 | Store: ... = ... | here |
+| test.cpp:171:17:171:18 | Load: p3 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:145:3:145:16 | Store: ... = ... | here |
+| test.cpp:172:18:172:19 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:146:3:146:15 | Store: ... = ... | here |
+| test.cpp:173:18:173:19 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:147:3:147:19 | Store: ... = ... | here |
+| test.cpp:174:18:174:19 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:142:3:142:19 | Store: ... = ... | here |
+| test.cpp:175:16:175:17 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:148:3:148:18 | Store: ... = ... | here |
+| test.cpp:177:14:177:21 | Load: access to array | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:132:7:132:8 | b1 | b1 | test.cpp:151:3:151:15 | Store: ... = ... | here |
+| test.cpp:178:14:178:21 | Load: access to array | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:132:7:132:8 | b1 | b1 | test.cpp:152:3:152:19 | Store: ... = ... | here |
+| test.cpp:179:14:179:21 | Load: access to array | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:153:3:153:18 | Store: ... = ... | here |
+| test.cpp:180:14:180:19 | Load: * ... | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:154:3:154:22 | Store: ... = ... | here |
+| test.cpp:181:13:181:20 | Load: access to array | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:155:3:155:21 | Store: ... = ... | here |
+| test.cpp:182:14:182:19 | Load: * ... | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:156:3:156:25 | Store: ... = ... | here |

cpp/ql/test/query-tests/Likely Bugs/Memory Management/UsingExpiredStackAddress/UsingExpiredStackAddress.qlref

@@ -0,0 +1 @@
+Likely Bugs/Memory Management/UsingExpiredStackAddress.ql

cpp/ql/test/query-tests/Likely Bugs/Memory Management/UsingExpiredStackAddress/test.cpp

@@ -0,0 +1,199 @@
+struct S100 {
+  int i;
+  int* p;
+};
+
+static struct S100 s101;
+
+void escape1() {
+  int x;
+  s101.p = &x;
+}
+
+int simple_field_bad() {
+  escape1();
+  return *s101.p; // BAD
+}
+
+int simple_field_good() {
+  escape1();
+  return s101.i; // GOOD
+}
+
+int deref_p() {
+  return *s101.p;
+}
+
+int field_indirect_bad() {
+  escape1();
+  return deref_p(); // BAD [NOT DETECTED]
+}
+
+int deref_i() {
+  return s101.i;
+}
+
+int field_indirect_good() {
+  escape1();
+  return deref_i(); // GOOD
+}
+
+void store_argument(int *p) {
+  s101.p = p;
+}
+
+int store_argument_value() {
+  int x;
+  store_argument(&x);
+  return *s101.p; // GOOD
+}
+
+void store_address_of_argument(int y) {
+  s101.p = &y;
+}
+
+int store_argument_address() {
+  int x;
+  store_address_of_argument(x);
+  return *s101.p; // BAD
+}
+
+void address_escapes_through_pointer_arith() {
+  int x[2];
+  int* p0 = x;
+  int* p1 = p0 + 1;
+  int* p2 = p1 - 1;
+  int* p3 = 1 + p2;
+  p3++;
+  s101.p = p3;
+}
+
+int test_pointer_arith_bad() {
+  address_escapes_through_pointer_arith();
+  return *s101.p; // BAD
+}
+
+int test_pointer_arith_good_1() {
+  int x;
+  address_escapes_through_pointer_arith();
+  s101.p = &x;
+  return *s101.p; // GOOD
+}
+
+int test_pointer_arith_good_2(bool b) {
+  int x;
+  if(b) {
+    address_escapes_through_pointer_arith();
+  }
+  return *s101.p; // GOOD (we can't say for sure that this is a local address)
+}
+
+void field_address_escapes() {
+  S100 s;
+  s101.p = &s.i;
+}
+
+int test_field_address_escapes() {
+  field_address_escapes();
+  return s101.p[0]; // BAD
+}
+
+void escape_through_reference() {
+  int x = 0;
+  int& r0 = x;
+  int& r1 = r0;
+  r1++;
+  s101.p = &r1;
+}
+
+int test_escapes_through_reference() {
+  escape_through_reference();
+  return *s101.p; // BAD
+}
+
+struct S300 {
+  int a1[15];
+  int a2[14][15];
+  int a3[13][14][15];
+  int *p1;
+  int (*p2)[15];
+  int (*p3)[14][15];
+  int** pp;
+};
+
+S300 s1;
+S300 s2;
+S300 s3;
+S300 s4;
+S300 s5;
+S300 s6;
+
+void escape_through_arrays() {
+  int b1[15];
+  int b2[14][15];
+  int b3[13][14][15];
+
+  s1.p1 = b1;
+  s2.p1 = &b1[1];
+
+  s1.p2 = b2;
+  s2.p2 = &b2[1];
+  s3.p1 = b2[1];
+  s4.p1 = &b2[1][2];
+
+  s1.p3 = b3;
+  s2.p3 = &b3[1];
+  s3.p2 = b3[1];
+  s4.p2 = &b3[1][2];
+  s5.p1 = b3[1][2];
+  s6.p1 = &b3[1][2][3];
+
+  s1.pp[0] = b1;
+  s2.pp[0] = &b1[1];
+  s3.pp[0] = b2[1];
+  s4.pp[0] = &b2[1][2];
+  s5.pp[0] = b3[1][2];
+  s6.pp[0] = &b3[1][2][3];
+}
+
+void test_escape_through_arrays() {
+  escape_through_arrays();
+  int x1 = *s1.p1; // BAD
+  int x2 = *s2.p1; // BAD
+
+  int* x3 = s1.p2[1]; // BAD
+  int x4 = *s1.p2[1]; // BAD
+  int* x5 = *s2.p2; // BAD
+  int* x6 = s3.p1; // BAD
+  int x7 = *&s4.p1[1]; // BAD
+
+  int x8 = *s1.p3[1][2]; // BAD
+  int x9 = (*s2.p3[0])[0]; // BAD
+  int x10 = **s3.p2; // BAD
+  int x11 = **s4.p2; // BAD
+  int x12 = (*s4.p1); // BAD
+  int x13 = s5.p1[1]; // BAD
+
+  int* x14 = s1.pp[0]; // BAD
+  int x15 = *s2.pp[0]; // BAD
+  int x16 = *s3.pp[0]; // BAD
+  int x17 = **s4.pp; // BAD
+  int x18 = s5.pp[0][0]; // BAD
+  int x19 = (*s6.pp)[0]; // BAD
+}
+
+void not_escape_through_arrays() {
+  int x;
+
+  s1.a1[0] = x;
+  s1.a2[0][1] = s1.a1[0];
+  **s1.a3[0] = 42;
+}
+
+void test_not_escape_through_array() {
+  not_escape_through_arrays();
+  
+  int x20 = s1.a1[0]; // GOOD
+  int x21 = s1.a2[0][1]; // GOOD
+  int* x22 = s1.a3[5][2]; // GOOD
+}

cpp/ql/test/query-tests/Security/CWE/CWE-319/UseOfHttp/UseOfHttp.expected

@@ -7,6 +7,8 @@ edges
 | test.cpp:40:11:40:17 | access to array | test.cpp:11:26:11:28 | url |
 | test.cpp:46:18:46:26 | http:// | test.cpp:49:11:49:16 | buffer |
 | test.cpp:49:11:49:16 | buffer | test.cpp:11:26:11:28 | url |
+| test.cpp:110:21:110:40 | http://example.com | test.cpp:121:11:121:13 | ptr |
+| test.cpp:121:11:121:13 | ptr | test.cpp:11:26:11:28 | url |
 nodes
 | test.cpp:11:26:11:28 | url | semmle.label | url |
 | test.cpp:15:30:15:32 | url | semmle.label | url |
@@ -17,9 +19,12 @@ nodes
 | test.cpp:40:11:40:17 | access to array | semmle.label | access to array |
 | test.cpp:46:18:46:26 | http:// | semmle.label | http:// |
 | test.cpp:49:11:49:16 | buffer | semmle.label | buffer |
+| test.cpp:110:21:110:40 | http://example.com | semmle.label | http://example.com |
+| test.cpp:121:11:121:13 | ptr | semmle.label | ptr |
 subpaths
 #select
 | test.cpp:28:10:28:29 | http://example.com | test.cpp:28:10:28:29 | http://example.com | test.cpp:15:30:15:32 | url | A URL may be constructed with the HTTP protocol. |
 | test.cpp:35:23:35:42 | http://example.com | test.cpp:35:23:35:42 | http://example.com | test.cpp:15:30:15:32 | url | A URL may be constructed with the HTTP protocol. |
 | test.cpp:36:26:36:45 | http://example.com | test.cpp:36:26:36:45 | http://example.com | test.cpp:15:30:15:32 | url | A URL may be constructed with the HTTP protocol. |
 | test.cpp:46:18:46:26 | http:// | test.cpp:46:18:46:26 | http:// | test.cpp:15:30:15:32 | url | A URL may be constructed with the HTTP protocol. |
+| test.cpp:110:21:110:40 | http://example.com | test.cpp:110:21:110:40 | http://example.com | test.cpp:15:30:15:32 | url | A URL may be constructed with the HTTP protocol. |

cpp/ql/test/query-tests/Security/CWE/CWE-319/UseOfHttp/test.cpp

@@ -58,3 +58,66 @@ void test()
 		openUrl(buffer);
 	}
 }
+
+typedef unsigned long size_t;
+int strncmp(const char *s1, const char *s2, size_t n);
+char* strstr(char* s1, const char* s2);
+
+void test2(const char *url)
+{
+	if (strncmp(url, "http://", 7)) // GOOD (or at least dubious; we are not constructing the URL)
+	{
+		openUrl(url);
+	}
+}
+
+void test3(char *url)
+{
+	char *ptr;
+
+	ptr = strstr(url, "https://"); // GOOD (https)
+	if (!ptr)
+	{
+		ptr = strstr(url, "http://"); // GOOD (we are not constructing the URL)
+	}
+
+	if (ptr)
+	{
+		openUrl(ptr);
+	}
+}
+
+void test4(char *url)
+{
+	const char *https_string = "https://"; // GOOD (https)
+	const char *http_string = "http://"; // GOOD (we are not constructing the URL)
+	char *ptr;
+
+	ptr = strstr(url, https_string);
+	if (!ptr)
+	{
+		ptr = strstr(url, http_string);
+	}
+
+	if (ptr)
+	{
+		openUrl(ptr);
+	}
+}
+
+void test5()
+{
+	char *url_string = "http://example.com"; // BAD
+	char *ptr;
+
+	ptr = strstr(url_string, "https://"); // GOOD (https)
+	if (!ptr)
+	{
+		ptr = strstr(url_string, "http://"); // GOOD (we are not constructing the URL here)
+	}
+
+	if (ptr)
+	{
+		openUrl(ptr);
+	}
+}

csharp/ql/lib/semmle/code/csharp/dataflow/internal/AccessPathSyntax.qll

@@ -6,6 +6,15 @@
  * (which does not use the shared data flow libraries).
  */
 
+/**
+ * Convenience-predicate for extracting two capture groups at once.
+ */
+bindingset[input, regexp]
+private predicate regexpCaptureTwo(string input, string regexp, string capture1, string capture2) {
+  capture1 = input.regexpCapture(regexp, 1) and
+  capture2 = input.regexpCapture(regexp, 2)
+}
+
 /** Companion module to the `AccessPath` class. */
 module AccessPath {
   /** A string that should be parsed as an access path. */
@@ -13,6 +22,95 @@ module AccessPath {
     bindingset[this]
     Range() { any() }
   }
+
+  /**
+   * Parses an integer constant `n` or interval `n1..n2` (inclusive) and gets the value
+   * of the constant or any value contained in the interval.
+   */
+  bindingset[arg]
+  int parseInt(string arg) {
+    result = arg.toInt()
+    or
+    // Match "n1..n2"
+    exists(string lo, string hi |
+      regexpCaptureTwo(arg, "(-?\\d+)\\.\\.(-?\\d+)", lo, hi) and
+      result = [lo.toInt() .. hi.toInt()]
+    )
+  }
+
+  /**
+   * Parses a lower-bounded interval `n..` and gets the lower bound.
+   */
+  bindingset[arg]
+  private int parseLowerBound(string arg) {
+    result = arg.regexpCapture("(-?\\d+)\\.\\.", 1).toInt()
+  }
+
+  /**
+   * Parses an integer constant or interval (bounded or unbounded) that explicitly
+   * references the arity, such as `N-1` or `N-3..N-1`.
+   *
+   * Note that expressions of form `N-x` will never resolve to a negative index,
+   * even if `N` is zero (it will have no result in that case).
+   */
+  bindingset[arg, arity]
+  private int parseIntWithExplicitArity(string arg, int arity) {
+    result >= 0 and // do not allow N-1 to resolve to a negative index
+    exists(string lo |
+      // N-x
+      lo = arg.regexpCapture("N-(\\d+)", 1) and
+      result = arity - lo.toInt()
+      or
+      // N-x..
+      lo = arg.regexpCapture("N-(\\d+)\\.\\.", 1) and
+      result = [arity - lo.toInt(), arity - 1]
+    )
+    or
+    exists(string lo, string hi |
+      // x..N-y
+      regexpCaptureTwo(arg, "(-?\\d+)\\.\\.N-(\\d+)", lo, hi) and
+      result = [lo.toInt() .. arity - hi.toInt()]
+      or
+      // N-x..N-y
+      regexpCaptureTwo(arg, "N-(\\d+)\\.\\.N-(\\d+)", lo, hi) and
+      result = [arity - lo.toInt() .. arity - hi.toInt()] and
+      result >= 0
+      or
+      // N-x..y
+      regexpCaptureTwo(arg, "N-(\\d+)\\.\\.(\\d+)", lo, hi) and
+      result = [arity - lo.toInt() .. hi.toInt()] and
+      result >= 0
+    )
+  }
+
+  /**
+   * Parses an integer constant or interval (bounded or unbounded) and gets any
+   * of the integers contained within (of which there may be infinitely many).
+   *
+   * Has no result for arguments involving an explicit arity, such as `N-1`.
+   */
+  bindingset[arg, result]
+  int parseIntUnbounded(string arg) {
+    result = parseInt(arg)
+    or
+    result >= parseLowerBound(arg)
+  }
+
+  /**
+   * Parses an integer constant or interval (bounded or unbounded) that
+   * may reference the arity of a call, such as `N-1` or `N-3..N-1`.
+   *
+   * Note that expressions of form `N-x` will never resolve to a negative index,
+   * even if `N` is zero (it will have no result in that case).
+   */
+  bindingset[arg, arity]
+  int parseIntWithArity(string arg, int arity) {
+    result = parseInt(arg)
+    or
+    result in [parseLowerBound(arg) .. arity - 1]
+    or
+    result = parseIntWithExplicitArity(arg, arity)
+  }
 }
 
 /** Gets the `n`th token on the access path as a string. */

csharp/ql/lib/semmle/code/csharp/dataflow/internal/FlowSummaryImplSpecific.qll

@@ -124,21 +124,26 @@ predicate sinkElement(Element e, string input, string kind) {
 
 /** Gets the summary component for specification component `c`, if any. */
 bindingset[c]
-SummaryComponent interpretComponentSpecific(string c) {
+SummaryComponent interpretComponentSpecific(AccessPathToken c) {
   c = "Element" and result = SummaryComponent::content(any(ElementContent ec))
   or
+  // Qualified names may contain commas,such as in `Tuple<,>`, so get the entire argument list
+  // rather than an individual argument.
   exists(Field f |
-    c.regexpCapture("Field\\[(.+)\\]", 1) = f.getQualifiedName() and
+    c.getName() = "Field" and
+    c.getArgumentList() = f.getQualifiedName() and
     result = SummaryComponent::content(any(FieldContent fc | fc.getField() = f))
   )
   or
   exists(Property p |
-    c.regexpCapture("Property\\[(.+)\\]", 1) = p.getQualifiedName() and
+    c.getName() = "Property" and
+    c.getArgumentList() = p.getQualifiedName() and
     result = SummaryComponent::content(any(PropertyContent pc | pc.getProperty() = p))
   )
   or
   exists(SyntheticField f |
-    c.regexpCapture("SyntheticField\\[(.+)\\]", 1) = f and
+    c.getName() = "SyntheticField" and
+    c.getArgumentList() = f and
     result = SummaryComponent::content(any(SyntheticFieldContent sfc | sfc.getField() = f))
   )
 }
@@ -253,21 +258,10 @@ predicate interpretInputSpecific(string c, InterpretNode mid, InterpretNode n) {
   )
 }
 
-bindingset[s]
-private int parseIntegerPosition(string s) {
-  result = s.regexpCapture("([0-9]+)", 1).toInt()
-  or
-  exists(int n1, int n2 |
-    s.regexpCapture("([0-9]+)\\.\\.([0-9]+)", 1).toInt() = n1 and
-    s.regexpCapture("([0-9]+)\\.\\.([0-9]+)", 2).toInt() = n2 and
-    result in [n1 .. n2]
-  )
-}
-
 /** Gets the argument position obtained by parsing `X` in `Parameter[X]`. */
 bindingset[s]
 ArgumentPosition parseParamBody(string s) {
-  result.getPosition() = parseIntegerPosition(s)
+  result.getPosition() = AccessPath::parseInt(s)
   or
   s = "This" and
   result.isQualifier()
@@ -276,7 +270,7 @@ ArgumentPosition parseParamBody(string s) {
 /** Gets the parameter position obtained by parsing `X` in `Argument[X]`. */
 bindingset[s]
 ParameterPosition parseArgBody(string s) {
-  result.getPosition() = parseIntegerPosition(s)
+  result.getPosition() = AccessPath::parseInt(s)
   or
   s = "Qualifier" and
   result.isThisParameter()

docs/codeql/ql-language-reference/ql-language-specification.rst

@@ -998,10 +998,14 @@ There are several kinds of expressions:
            |   literal
            |   variable
            |   super_expr
-           |   callwithresult
            |   postfix_cast
+           |   callwithresults
            |   aggregation
+           |   expression_pragma
            |   any
+           |   range
+           |   setliteral
+
 
 Parenthesized expressions
 ~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -1303,6 +1307,26 @@ The values of an ``any`` expression are those values of the expression for which
 
 The abbreviated cases for an ``any`` expression are interpreted in the same way as for an aggregation.
 
+Expression Pragma
+~~~~~~~~~~~~~~~~~
+
+Expression pragmas can be used to guide optimization.
+
+::
+   expression_pragma ::= "pragma" "[" expression_pragma_type "]" "(" expr ")"
+
+   expression_pragma_type ::= "only_bind_out" | "only_bind_into"
+
+The values of an expression pragma are the values of the contained expression.
+
+The type `only_bind_out` hints that uses of the result of the expression pragma should not be used to guide the evaluation of the result of the contained expression.
+When checking to see that all values are bound the compiler does not assume that if the result of the expression pragma is bound then the result of the contained 
+expression is bound.
+
+The type `only_bind_into` hints that uses of the contained expression should not be used to guide the evaluation of the result of the expression pragma.
+When checking to see that all values are bound the compiler does not assume that if the result of the contained expression is bound then the result of the 
+expression pragma is bound.
+
 Ranges
 ~~~~~~
 
@@ -1506,9 +1530,10 @@ A range check has the following syntax:
 
 ::
 
-   inrange ::= expr "in" range
+   inrange ::= expr "in" (range | setliteral)
 
-The formula is equivalent to ``expr "=" range``.
+
+The formula is equivalent to ``expr "=" range`` or ``expr "=" setliteral``.
 
 Calls
 ~~~~~
@@ -2107,7 +2132,7 @@ The complete grammar for QL is as follows:
 
    instanceof ::= expr "instanceof" type
 
-   inrange ::= expr "in" range
+   inrange ::= expr "in" (range | setliteral)
 
    call ::= predicateRef (closure)? "(" (exprs)? ")"
         |   primary "." predicateName (closure)? "(" (exprs)? ")"
@@ -2128,6 +2153,7 @@ The complete grammar for QL is as follows:
            |   postfix_cast
            |   callwithresults
            |   aggregation
+           |   expression_pragma
            |   any
            |   range
            |   setliteral
@@ -2159,6 +2185,10 @@ The complete grammar for QL is as follows:
                |   aggid ("[" expr "]")? "(" as_exprs ("order" "by" aggorderbys)? ")"
                |   "unique" "(" var_decls "|" (formula)? ("|" as_exprs)? ")"
  
+   expression_pragma ::= "pragma" "[" expression_pragma_type "]" "(" expr ")"
+
+   expression_pragma_type ::= "only_bind_out" | "only_bind_into"
+
    aggid ::= "avg" | "concat" | "count" | "max" | "min" | "rank" | "strictconcat" | "strictcount" | "strictsum" | "sum"
 
    aggorderbys ::= aggorderby ("," aggorderby)*

docs/codeql/support/reusables/versions-compilers.rst

@@ -17,11 +17,11 @@
 
    .NET 5, .NET 6","``.sln``, ``.csproj``, ``.cs``, ``.cshtml``, ``.xaml``"
    Go (aka Golang), "Go up to 1.17", "Go 1.11 or more recent", ``.go``
-   Java,"Java 7 to 16 [4]_","javac (OpenJDK and Oracle JDK),
+   Java,"Java 7 to 17 [4]_","javac (OpenJDK and Oracle JDK),
 
    Eclipse compiler for Java (ECJ) [5]_",``.java``
    JavaScript,ECMAScript 2021 or lower,Not applicable,"``.js``, ``.jsx``, ``.mjs``, ``.es``, ``.es6``, ``.htm``, ``.html``, ``.xhtm``, ``.xhtml``, ``.vue``, ``.hbs``, ``.ejs``, ``.njk``, ``.json``, ``.yaml``, ``.yml``, ``.raml``, ``.xml`` [6]_"
-   Python,"2.7, 3.5, 3.6, 3.7, 3.8, 3.9",Not applicable,``.py``
+   Python,"2.7, 3.5, 3.6, 3.7, 3.8, 3.9, 3.10",Not applicable,``.py``
    Ruby [7]_,"up to 3.0.2",Not applicable,"``.rb``, ``.erb``, ``.gemspec``, ``Gemfile``"
    TypeScript [8]_,"2.6-4.5",Standard TypeScript compiler,"``.ts``, ``.tsx``"
 
@@ -30,7 +30,7 @@
     .. [1] C++20 support is currently in beta. Supported for GCC on Linux only. Modules are *not* supported.
     .. [2] Support for the clang-cl compiler is preliminary.
     .. [3] Support for the Arm Compiler (armcc) is preliminary.
-    .. [4] Builds that execute on Java 7 to 16 can be analyzed. The analysis understands Java 16 standard language features.
+    .. [4] Builds that execute on Java 7 to 17 can be analyzed. The analysis understands Java 17 standard language features.
     .. [5] ECJ is supported when the build invokes it via the Maven Compiler plugin or the Takari Lifecycle plugin.
     .. [6] JSX and Flow code, YAML, JSON, HTML, and XML files may also be analyzed with JavaScript files.
     .. [7] Requires glibc 2.17.

java/ql/lib/semmle/code/java/dataflow/internal/AccessPathSyntax.qll

@@ -6,6 +6,15 @@
  * (which does not use the shared data flow libraries).
  */
 
+/**
+ * Convenience-predicate for extracting two capture groups at once.
+ */
+bindingset[input, regexp]
+private predicate regexpCaptureTwo(string input, string regexp, string capture1, string capture2) {
+  capture1 = input.regexpCapture(regexp, 1) and
+  capture2 = input.regexpCapture(regexp, 2)
+}
+
 /** Companion module to the `AccessPath` class. */
 module AccessPath {
   /** A string that should be parsed as an access path. */
@@ -13,6 +22,95 @@ module AccessPath {
     bindingset[this]
     Range() { any() }
   }
+
+  /**
+   * Parses an integer constant `n` or interval `n1..n2` (inclusive) and gets the value
+   * of the constant or any value contained in the interval.
+   */
+  bindingset[arg]
+  int parseInt(string arg) {
+    result = arg.toInt()
+    or
+    // Match "n1..n2"
+    exists(string lo, string hi |
+      regexpCaptureTwo(arg, "(-?\\d+)\\.\\.(-?\\d+)", lo, hi) and
+      result = [lo.toInt() .. hi.toInt()]
+    )
+  }
+
+  /**
+   * Parses a lower-bounded interval `n..` and gets the lower bound.
+   */
+  bindingset[arg]
+  private int parseLowerBound(string arg) {
+    result = arg.regexpCapture("(-?\\d+)\\.\\.", 1).toInt()
+  }
+
+  /**
+   * Parses an integer constant or interval (bounded or unbounded) that explicitly
+   * references the arity, such as `N-1` or `N-3..N-1`.
+   *
+   * Note that expressions of form `N-x` will never resolve to a negative index,
+   * even if `N` is zero (it will have no result in that case).
+   */
+  bindingset[arg, arity]
+  private int parseIntWithExplicitArity(string arg, int arity) {
+    result >= 0 and // do not allow N-1 to resolve to a negative index
+    exists(string lo |
+      // N-x
+      lo = arg.regexpCapture("N-(\\d+)", 1) and
+      result = arity - lo.toInt()
+      or
+      // N-x..
+      lo = arg.regexpCapture("N-(\\d+)\\.\\.", 1) and
+      result = [arity - lo.toInt(), arity - 1]
+    )
+    or
+    exists(string lo, string hi |
+      // x..N-y
+      regexpCaptureTwo(arg, "(-?\\d+)\\.\\.N-(\\d+)", lo, hi) and
+      result = [lo.toInt() .. arity - hi.toInt()]
+      or
+      // N-x..N-y
+      regexpCaptureTwo(arg, "N-(\\d+)\\.\\.N-(\\d+)", lo, hi) and
+      result = [arity - lo.toInt() .. arity - hi.toInt()] and
+      result >= 0
+      or
+      // N-x..y
+      regexpCaptureTwo(arg, "N-(\\d+)\\.\\.(\\d+)", lo, hi) and
+      result = [arity - lo.toInt() .. hi.toInt()] and
+      result >= 0
+    )
+  }
+
+  /**
+   * Parses an integer constant or interval (bounded or unbounded) and gets any
+   * of the integers contained within (of which there may be infinitely many).
+   *
+   * Has no result for arguments involving an explicit arity, such as `N-1`.
+   */
+  bindingset[arg, result]
+  int parseIntUnbounded(string arg) {
+    result = parseInt(arg)
+    or
+    result >= parseLowerBound(arg)
+  }
+
+  /**
+   * Parses an integer constant or interval (bounded or unbounded) that
+   * may reference the arity of a call, such as `N-1` or `N-3..N-1`.
+   *
+   * Note that expressions of form `N-x` will never resolve to a negative index,
+   * even if `N` is zero (it will have no result in that case).
+   */
+  bindingset[arg, arity]
+  int parseIntWithArity(string arg, int arity) {
+    result = parseInt(arg)
+    or
+    result in [parseLowerBound(arg) .. arity - 1]
+    or
+    result = parseIntWithExplicitArity(arg, arity)
+  }
 }
 
 /** Gets the `n`th token on the access path as a string. */

java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImplSpecific.qll

@@ -200,21 +200,10 @@ predicate interpretInputSpecific(string c, InterpretNode mid, InterpretNode n) {
   )
 }
 
-bindingset[s]
-private int parsePosition(string s) {
-  result = s.regexpCapture("([-0-9]+)", 1).toInt()
-  or
-  exists(int n1, int n2 |
-    s.regexpCapture("([-0-9]+)\\.\\.([0-9]+)", 1).toInt() = n1 and
-    s.regexpCapture("([-0-9]+)\\.\\.([0-9]+)", 2).toInt() = n2 and
-    result in [n1 .. n2]
-  )
-}
-
 /** Gets the argument position obtained by parsing `X` in `Parameter[X]`. */
 bindingset[s]
-ArgumentPosition parseParamBody(string s) { result = parsePosition(s) }
+ArgumentPosition parseParamBody(string s) { result = AccessPath::parseInt(s) }
 
 /** Gets the parameter position obtained by parsing `X` in `Argument[X]`. */
 bindingset[s]
-ParameterPosition parseArgBody(string s) { result = parsePosition(s) }
+ParameterPosition parseArgBody(string s) { result = AccessPath::parseInt(s) }

java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstHeader.java

@@ -0,0 +1,20 @@
+import javax.servlet.http.HttpServletRequest;
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.lang.String;
+
+
+public class Test {
+    private boolean UnsafeComparison(HttpServletRequest request) {
+        String Key = "secret";
+        return Key.equals(request.getHeader("X-Auth-Token"));        
+    }
+
+    private boolean safeComparison(HttpServletRequest request) {
+          String token = request.getHeader("X-Auth-Token");
+          String Key = "secret"; 
+          return MessageDigest.isEqual(Key.getBytes(StandardCharsets.UTF_8), token.getBytes(StandardCharsets.UTF_8));
+    }
+    
+}
+

java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstHeader.qhelp

@@ -0,0 +1,28 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+A constant-time algorithm should be used for checking the value of sensitive headers.
+In other words, the comparison time should not depend on the content of the input. 
+Otherwise timing information could be used to infer the header's expected, secret value.
+</p>
+</overview>
+
+
+<recommendation>
+<p>
+Use <code>MessageDigest.isEqual()</code> method to check the value of headers.
+If this method is used, then the calculation time depends only on the length of input byte arrays,
+and does not depend on the contents of the arrays.
+</p>
+</recommendation>
+<example>
+<p>
+The following example uses <code>String.equals()</code> method for validating a csrf token.
+This method implements a non-constant-time algorithm. The example also demonstrates validation using a safe constant-time algorithm.
+</p>
+<sample src="TimingAttackAgainstHeader.java" />
+</example>
+</qhelp>
+

java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstHeader.ql

@@ -0,0 +1,72 @@
+/**
+ * @name Timing attack against header value
+ * @description Use of a non-constant-time verification routine to check the value of an HTTP header,
+ *              possibly allowing a timing attack to infer the header's expected value.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/timing-attack-against-headers-value
+ * @tags security
+ *       external/cwe/cwe-208
+ */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.dataflow.TaintTracking
+import DataFlow::PathGraph
+
+/** A static method that uses a non-constant-time algorithm for comparing inputs. */
+private class NonConstantTimeComparisonCall extends StaticMethodAccess {
+  NonConstantTimeComparisonCall() {
+    this.getMethod()
+        .hasQualifiedName("org.apache.commons.lang3", "StringUtils",
+          ["equals", "equalsAny", "equalsAnyIgnoreCase", "equalsIgnoreCase"])
+  }
+}
+
+/** Methods that use a non-constant-time algorithm for comparing inputs. */
+private class NonConstantTimeEqualsCall extends MethodAccess {
+  NonConstantTimeEqualsCall() {
+    this.getMethod()
+        .hasQualifiedName("java.lang", "String", ["equals", "contentEquals", "equalsIgnoreCase"])
+  }
+}
+
+private predicate isNonConstantEqualsCallArgument(Expr e) {
+  exists(NonConstantTimeEqualsCall call | e = [call.getQualifier(), call.getArgument(0)])
+}
+
+private predicate isNonConstantComparisonCallArgument(Expr p) {
+  exists(NonConstantTimeComparisonCall call | p = [call.getArgument(0), call.getArgument(1)])
+}
+
+class ClientSuppliedIpTokenCheck extends DataFlow::Node {
+  ClientSuppliedIpTokenCheck() {
+    exists(MethodAccess ma |
+      ma.getMethod().hasName("getHeader") and
+      ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() in [
+          "x-auth-token", "x-csrf-token", "http_x_csrf_token", "x-csrf-param", "x-csrf-header",
+          "http_x_csrf_token", "x-api-key", "authorization", "proxy-authorization"
+        ] and
+      ma = this.asExpr()
+    )
+  }
+}
+
+class NonConstantTimeComparisonConfig extends TaintTracking::Configuration {
+  NonConstantTimeComparisonConfig() { this = "NonConstantTimeComparisonConfig" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof ClientSuppliedIpTokenCheck
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    isNonConstantEqualsCallArgument(sink.asExpr()) or
+    isNonConstantComparisonCallArgument(sink.asExpr())
+  }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeComparisonConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Possible timing attack against $@ validation.",
+  source.getNode(), "client-supplied token"

java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstHeader/Test.java

@@ -0,0 +1,19 @@
+import javax.servlet.http.HttpServletRequest;
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.lang.String;
+
+
+public class Test {
+    private boolean UnsafeComparison(HttpServletRequest request) {
+        String Key = "secret";
+        return Key.equals(request.getHeader("X-Auth-Token"));        
+    }
+
+    private boolean safeComparison(HttpServletRequest request) {
+          String token = request.getHeader("X-Auth-Token");
+          String Key = "secret"; 
+          return MessageDigest.isEqual(Key.getBytes(StandardCharsets.UTF_8), token.getBytes(StandardCharsets.UTF_8));
+    }
+
+}

java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstHeader/TimingAttackAgainstHeader.expected

@@ -0,0 +1,6 @@
+edges
+nodes
+| Test.java:10:27:10:59 | getHeader(...) | semmle.label | getHeader(...) |
+subpaths
+#select
+| Test.java:10:27:10:59 | getHeader(...) | Test.java:10:27:10:59 | getHeader(...) | Test.java:10:27:10:59 | getHeader(...) | Possible timing attack against $@ validation. | Test.java:10:27:10:59 | getHeader(...) | client-supplied token |

java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstHeader/TimingAttackAgainstHeader.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-208/TimingAttackAgainstHeader.ql

java/ql/test/library-tests/method-signatures/Test.java

@@ -0,0 +1,13 @@
+import java.lang.annotation.*;
+
+public class Test {
+
+  @Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})
+  @interface NotNull { }
+
+  class Inner { }
+
+  public void annotations(@NotNull byte[] b1, byte @NotNull [] b2, @NotNull String s, Class<@NotNull String> c, @NotNull Test.Inner ti, Class<? extends @NotNull String> wc, Class<String>[] classes, @NotNull byte b, @NotNull String[] sArray, String @NotNull [] sArray2) { }
+
+}
+

java/ql/test/library-tests/method-signatures/signatures.expected

@@ -0,0 +1 @@
+| Test.java:10:15:10:25 | annotations | annotations(byte[],byte[],java.lang.String,java.lang.Class,Test.Inner,java.lang.Class,java.lang.Class[],byte,java.lang.String[],java.lang.String[]) |

java/ql/test/library-tests/method-signatures/signatures.ql

@@ -0,0 +1,5 @@
+import java
+
+from Method m
+where m.getFile().getBaseName() = "Test.java"
+select m, m.getSignature()

java/ql/test/query-tests/InefficientOutputStream/InefficientOutputStreamAnnotations.java

@@ -0,0 +1,24 @@
+import java.io.*;
+import java.lang.annotation.*;
+
+public class InefficientOutputStreamAnnotations {
+
+  @Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})
+  @interface NotNull { }
+
+  public static void test() {
+
+        OutputStream stream = new OutputStream() {
+          @Override
+          public void write(int b) throws IOException {
+            OutputStream otherStream = null;
+            otherStream.write(1);            
+          }
+          @Override
+          public void write(byte @NotNull [] b, int off, int len) throws IOException { // GOOD: even with the annotation @NotNull, this overrides write(byte[], int, int).
+          }
+        };
+
+  }
+
+}

javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/CountAlertsAndEndpoints.ql

@@ -13,6 +13,8 @@ import semmle.javascript.security.dataflow.NosqlInjection
 import semmle.javascript.security.dataflow.SqlInjection
 import semmle.javascript.security.dataflow.TaintedPath
 import semmle.javascript.security.dataflow.DomBasedXss
+import semmle.javascript.security.dataflow.StoredXss
+import semmle.javascript.security.dataflow.XssThroughDom
 import evaluation.EndToEndEvaluation
 
 int numAlerts(DataFlow::Configuration cfg) {
@@ -26,6 +28,8 @@ select numAlerts(any(NosqlInjection::Configuration cfg)) as numNosqlAlerts,
   numAlerts(any(SqlInjection::Configuration cfg)) as numSqlAlerts,
   numAlerts(any(TaintedPath::Configuration cfg)) as numTaintedPathAlerts,
   numAlerts(any(DomBasedXss::Configuration cfg)) as numXssAlerts,
+  numAlerts(any(StoredXss::Configuration cfg)) as numStoredXssAlerts,
+  numAlerts(any(XssThroughDom::Configuration cfg)) as numXssThroughDomAlerts,
   count(DataFlow::Node sink |
     exists(NosqlInjection::Configuration cfg | cfg.isSink(sink) or cfg.isSink(sink, _))
   ) as numNosqlSinks,
@@ -37,4 +41,10 @@ select numAlerts(any(NosqlInjection::Configuration cfg)) as numNosqlAlerts,
   ) as numTaintedPathSinks,
   count(DataFlow::Node sink |
     exists(DomBasedXss::Configuration cfg | cfg.isSink(sink) or cfg.isSink(sink, _))
-  ) as numXssSinks
+  ) as numXssSinks,
+  count(DataFlow::Node sink |
+    exists(StoredXss::Configuration cfg | cfg.isSink(sink) or cfg.isSink(sink, _))
+  ) as numStoredXssSinks,
+  count(DataFlow::Node sink |
+    exists(XssThroughDom::Configuration cfg | cfg.isSink(sink) or cfg.isSink(sink, _))
+  ) as numXssThroughDomSinks

javascript/ql/lib/semmle/javascript/frameworks/data/internal/AccessPathSyntax.qll

@@ -6,6 +6,15 @@
  * (which does not use the shared data flow libraries).
  */
 
+/**
+ * Convenience-predicate for extracting two capture groups at once.
+ */
+bindingset[input, regexp]
+private predicate regexpCaptureTwo(string input, string regexp, string capture1, string capture2) {
+  capture1 = input.regexpCapture(regexp, 1) and
+  capture2 = input.regexpCapture(regexp, 2)
+}
+
 /** Companion module to the `AccessPath` class. */
 module AccessPath {
   /** A string that should be parsed as an access path. */
@@ -13,6 +22,95 @@ module AccessPath {
     bindingset[this]
     Range() { any() }
   }
+
+  /**
+   * Parses an integer constant `n` or interval `n1..n2` (inclusive) and gets the value
+   * of the constant or any value contained in the interval.
+   */
+  bindingset[arg]
+  int parseInt(string arg) {
+    result = arg.toInt()
+    or
+    // Match "n1..n2"
+    exists(string lo, string hi |
+      regexpCaptureTwo(arg, "(-?\\d+)\\.\\.(-?\\d+)", lo, hi) and
+      result = [lo.toInt() .. hi.toInt()]
+    )
+  }
+
+  /**
+   * Parses a lower-bounded interval `n..` and gets the lower bound.
+   */
+  bindingset[arg]
+  private int parseLowerBound(string arg) {
+    result = arg.regexpCapture("(-?\\d+)\\.\\.", 1).toInt()
+  }
+
+  /**
+   * Parses an integer constant or interval (bounded or unbounded) that explicitly
+   * references the arity, such as `N-1` or `N-3..N-1`.
+   *
+   * Note that expressions of form `N-x` will never resolve to a negative index,
+   * even if `N` is zero (it will have no result in that case).
+   */
+  bindingset[arg, arity]
+  private int parseIntWithExplicitArity(string arg, int arity) {
+    result >= 0 and // do not allow N-1 to resolve to a negative index
+    exists(string lo |
+      // N-x
+      lo = arg.regexpCapture("N-(\\d+)", 1) and
+      result = arity - lo.toInt()
+      or
+      // N-x..
+      lo = arg.regexpCapture("N-(\\d+)\\.\\.", 1) and
+      result = [arity - lo.toInt(), arity - 1]
+    )
+    or
+    exists(string lo, string hi |
+      // x..N-y
+      regexpCaptureTwo(arg, "(-?\\d+)\\.\\.N-(\\d+)", lo, hi) and
+      result = [lo.toInt() .. arity - hi.toInt()]
+      or
+      // N-x..N-y
+      regexpCaptureTwo(arg, "N-(\\d+)\\.\\.N-(\\d+)", lo, hi) and
+      result = [arity - lo.toInt() .. arity - hi.toInt()] and
+      result >= 0
+      or
+      // N-x..y
+      regexpCaptureTwo(arg, "N-(\\d+)\\.\\.(\\d+)", lo, hi) and
+      result = [arity - lo.toInt() .. hi.toInt()] and
+      result >= 0
+    )
+  }
+
+  /**
+   * Parses an integer constant or interval (bounded or unbounded) and gets any
+   * of the integers contained within (of which there may be infinitely many).
+   *
+   * Has no result for arguments involving an explicit arity, such as `N-1`.
+   */
+  bindingset[arg, result]
+  int parseIntUnbounded(string arg) {
+    result = parseInt(arg)
+    or
+    result >= parseLowerBound(arg)
+  }
+
+  /**
+   * Parses an integer constant or interval (bounded or unbounded) that
+   * may reference the arity of a call, such as `N-1` or `N-3..N-1`.
+   *
+   * Note that expressions of form `N-x` will never resolve to a negative index,
+   * even if `N` is zero (it will have no result in that case).
+   */
+  bindingset[arg, arity]
+  int parseIntWithArity(string arg, int arity) {
+    result = parseInt(arg)
+    or
+    result in [parseLowerBound(arg) .. arity - 1]
+    or
+    result = parseIntWithExplicitArity(arg, arity)
+  }
 }
 
 /** Gets the `n`th token on the access path as a string. */

javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll

@@ -274,7 +274,7 @@ API::Node getSuccessorFromNode(API::Node node, AccessPathToken token) {
   // use-node represents be an argument, and an edge originating from a def-node represents a parameter.
   // We just map both to the same thing.
   token.getName() = ["Argument", "Parameter"] and
-  result = node.getParameter(getAnIntFromStringUnbounded(token.getAnArgument()))
+  result = node.getParameter(AccessPath::parseIntUnbounded(token.getAnArgument()))
   or
   token.getName() = "ReturnValue" and
   result = node.getReturn()
@@ -289,13 +289,9 @@ API::Node getSuccessorFromNode(API::Node node, AccessPathToken token) {
 bindingset[token]
 API::Node getSuccessorFromInvoke(Specific::InvokeNode invoke, AccessPathToken token) {
   token.getName() = "Argument" and
-  (
+  result =
-    result = invoke.getParameter(getAnIntFromStringUnbounded(token.getAnArgument()))
+    invoke
-    or
+        .getParameter(AccessPath::parseIntWithArity(token.getAnArgument(), invoke.getNumArgument()))
-    result =
-      invoke
-          .getParameter(getAnIntFromStringWithArity(token.getAnArgument(), invoke.getNumArgument()))
-  )
   or
   token.getName() = "ReturnValue" and
   result = invoke.getReturn()
@@ -310,7 +306,7 @@ API::Node getSuccessorFromInvoke(Specific::InvokeNode invoke, AccessPathToken to
 pragma[inline]
 private predicate invocationMatchesCallSiteFilter(Specific::InvokeNode invoke, AccessPathToken token) {
   token.getName() = "WithArity" and
-  invoke.getNumArgument() = getAnIntFromStringUnbounded(token.getAnArgument())
+  invoke.getNumArgument() = AccessPath::parseIntUnbounded(token.getAnArgument())
   or
   Specific::invocationMatchesExtraCallSiteFilter(invoke, token)
 }
@@ -361,89 +357,6 @@ Specific::InvokeNode getInvocationFromPath(string package, string type, AccessPa
   result = getInvocationFromPath(package, type, path, path.getNumToken())
 }
 
-/**
- * Convenience-predicate for extracting two capture groups at once.
- */
-bindingset[input, regexp]
-private predicate regexpCaptureTwo(string input, string regexp, string capture1, string capture2) {
-  capture1 = input.regexpCapture(regexp, 1) and
-  capture2 = input.regexpCapture(regexp, 2)
-}
-
-/**
- * Parses an integer constant `n` or interval `n1..n2` (inclusive) and gets the value
- * of the constant or any value contained in the interval.
- */
-bindingset[arg]
-private int getAnIntFromString(string arg) {
-  result = arg.toInt()
-  or
-  // Match "n1..n2"
-  exists(string lo, string hi |
-    regexpCaptureTwo(arg, "(\\d+)\\.\\.(\\d+)", lo, hi) and
-    result = [lo.toInt() .. hi.toInt()]
-  )
-}
-
-/**
- * Parses a lower-bounded interval `n..` and gets the lower bound.
- */
-bindingset[arg]
-private int getLowerBoundFromString(string arg) {
-  // Match "n.."
-  result = arg.regexpCapture("(\\d+)\\.\\.", 1).toInt()
-}
-
-/**
- * Parses an integer constant or interval (bounded or unbounded) and gets any
- * of the integers contained within (of which there may be infinitely many).
- *
- * Has no result for arguments involving an explicit arity, such as `N-1`.
- */
-bindingset[arg, result]
-private int getAnIntFromStringUnbounded(string arg) {
-  result = getAnIntFromString(arg)
-  or
-  result >= getLowerBoundFromString(arg)
-}
-
-/**
- * Parses an integer constant or interval (bounded or unbounded) that explicitly
- * references the arity, such as `N-1` or `N-3..N-1`.
- *
- * Note that such expressions will never resolve to a negative index, even if the
- * arity is zero (it will have no result in that case).
- */
-bindingset[arg, arity]
-private int getAnIntFromStringWithArity(string arg, int arity) {
-  result >= 0 and // do not allow N-1 to resolve to a negative index
-  exists(string lo |
-    // N-x
-    lo = arg.regexpCapture("N-(\\d+)", 1) and
-    result = arity - lo.toInt()
-    or
-    // N-x..
-    lo = arg.regexpCapture("N-(\\d+)\\.\\.", 1) and
-    result = [arity - lo.toInt(), arity - 1]
-  )
-  or
-  exists(string lo, string hi |
-    // x..N-y
-    regexpCaptureTwo(arg, "(\\d+)\\.\\.N-(\\d+)", lo, hi) and
-    result = [lo.toInt() .. arity - hi.toInt()]
-    or
-    // N-x..Ny
-    regexpCaptureTwo(arg, "N-(\\d+)\\.\\.N-(\\d+)", lo, hi) and
-    result = [arity - lo.toInt() .. arity - hi.toInt()] and
-    result >= 0
-    or
-    // N-x..y
-    regexpCaptureTwo(arg, "N-(\\d+)\\.\\.(\\d+)", lo, hi) and
-    result = [arity - lo.toInt() .. hi.toInt()] and
-    result >= 0
-  )
-}
-
 /**
  * Module providing access to the imported models in terms of API graph nodes.
  */

python/.vscode/ql.code-snippets

@@ -222,26 +222,6 @@
         ],
         "description": "Type tracking class (select full class path before inserting)",
     },
-    "foo": {
-        "scope": "ql",
-        "prefix": "foo",
-        "body": [
-            "    /**",
-            "     * Taint propagation for `$1`.",
-            "     */",
-            "     private class InstanceTaintSteps extends InstanceTaintStepsHelper {",
-            "        InstanceTaintSteps() { this = \"$1\" }",
-            "",
-            "        override DataFlow::Node getInstance() { result = instance() }",
-            "",
-            "        override string getAttributeName() { none() }",
-            "",
-            "        override string getMethodName() { none() }",
-            "",
-            "        override string getAsyncMethodName() { none() }",
-            "      }",
-        ],
-    },
     "API graph .getMember chain": {
         "scope": "ql",
         "prefix": "api graph .getMember chain",
@@ -250,4 +230,22 @@
         ],
         "description": "API graph .getMember chain (select full path before inserting)",
     },
+    "debug partial flow": {
+        "scope": "ql",
+        "prefix": "debug partial flow",
+        "body": [
+            "// put the line below inside the configuration",
+            "// override int explorationLimit() { result = 5 }",
+            "// and then run quick evaluation on the predicate below",
+            "// (and potentially limit the set of sources)",
+            "predicate debugPartialFlow(Location loc, DataFlow::PartialPathNode node, int dist) {",
+            "  loc = node.getNode().getLocation() and",
+            "  exists(loc.getFile().getRelativePath()) and",
+            "  exists(Configuration config, DataFlow::PartialPathNode source |",
+            "    config.hasPartialFlow(source, node, dist)",
+            "  )",
+            "}",
+        ],
+        "description": "debug partial flow",
+    },
 }

python/ql/lib/change-notes/2022-02-08-fix-attribute-taint.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Fixed taint propagation for attribute assignment. In the assignment `x.foo = tainted` we no longer treat the entire object `x` as tainted, just because the attribute `foo` contains tainted data. This leads to slightly fewer false positives.

python/ql/lib/semmle/python/dataflow/new/internal/TaintTrackingPrivate.qll

@@ -167,8 +167,25 @@ predicate stringManipulation(DataFlow::CfgNode nodeFrom, DataFlow::CfgNode nodeT
  */
 predicate containerStep(DataFlow::CfgNode nodeFrom, DataFlow::Node nodeTo) {
   // construction by literal
-  // TODO: Not limiting the content argument here feels like a BIG hack, but we currently get nothing for free :|
+  //
-  DataFlowPrivate::storeStep(nodeFrom, _, nodeTo)
+  // TODO: once we have proper flow-summary modeling, we might not need this step any
+  // longer -- but there needs to be a matching read-step for the store-step, and we
+  // don't provide that right now.
+  DataFlowPrivate::listStoreStep(nodeFrom, _, nodeTo)
+  or
+  DataFlowPrivate::setStoreStep(nodeFrom, _, nodeTo)
+  or
+  DataFlowPrivate::tupleStoreStep(nodeFrom, _, nodeTo)
+  or
+  DataFlowPrivate::dictStoreStep(nodeFrom, _, nodeTo)
+  or
+  // comprehension, so there is taint-flow from `x` in `[x for x in xs]` to the
+  // resulting list of the list-comprehension.
+  //
+  // TODO: once we have proper flow-summary modeling, we might not need this step any
+  // longer -- but there needs to be a matching read-step for the store-step, and we
+  // don't provide that right now.
+  DataFlowPrivate::comprehensionStoreStep(nodeFrom, _, nodeTo)
   or
   // constructor call
   exists(DataFlow::CallCfgNode call | call = nodeTo |

python/ql/test/experimental/dataflow/tainttracking/defaultAdditionalTaintStep/test_attr.py

@@ -0,0 +1,43 @@
+# Add taintlib to PATH so it can be imported during runtime without any hassle
+import sys; import os; sys.path.append(os.path.dirname(os.path.dirname((__file__))))
+from taintlib import *
+
+# This has no runtime impact, but allows autocomplete to work
+from typing import TYPE_CHECKING
+if TYPE_CHECKING:
+    from ..taintlib import *
+
+
+# Actual tests
+
+class Foo:
+    def __init__(self, arg):
+        self.arg = arg
+        self.other_arg = "other_arg"
+
+
+def test_tainted_attr():
+    # The following demonstrates how tainting an attribute affected the taintedness of
+    # the object.
+    #
+    # Previously we would (wrongly) treat the object as tainted if we noticed a write of
+    # a tainted value to any of its' attributes. This lead to FP, highlighted in
+    # https://github.com/github/codeql/issues/7786
+
+    f = Foo(TAINTED_STRING)
+    ensure_not_tainted(f)
+    ensure_tainted(f.arg) # $ tainted
+    ensure_not_tainted(f.other_arg)
+
+
+    x = Foo("x")
+    ensure_not_tainted(x, x.arg, x.other_arg)
+
+    x.arg = TAINTED_STRING
+    ensure_not_tainted(x)
+    ensure_tainted(x.arg) # $ tainted
+    ensure_not_tainted(f.other_arg)
+
+
+    b = Foo("bar")
+    ensure_not_tainted(b, b.arg, b.other_arg)

python/ql/test/experimental/dataflow/tainttracking/defaultAdditionalTaintStep/test_with.py

@@ -49,7 +49,7 @@ class Context_arg:
 def test_with_arg():
     ctx = Context_arg(TAINTED_STRING)
     with ctx as tainted:
-        ensure_tainted(tainted) # $ tainted
+        ensure_tainted(tainted) # $ MISSING: tainted
 
 
 

ql/ql/src/codeql_ql/ast/Ast.qll

@@ -415,7 +415,7 @@ class ClasslessPredicate extends TClasslessPredicate, Predicate, ModuleDeclarati
   ClasslessPredicate() { this = TClasslessPredicate(pred) }
 
   /**
-   * If this predicate is an alias, gets the aliased value.
+   * Gets the aliased value if this predicate is an alias
    * E.g. for `predicate foo = Module::bar/2;` gets `Module::bar/2`.
    * The result is either a `PredicateExpr` or `HigherOrderFormula`.
    */
@@ -1672,7 +1672,7 @@ class Rank extends Aggregate {
   override string getAPrimaryQlClass() { result = "Rank" }
 
   /**
-   * The `i` in `rank[i]( | | )`.
+   * Gets the `i` in `rank[i]( | | )`.
    */
   Expr getRankExpr() { toQL(result) = this.getAggregate().getChild(1) }
 

ql/ql/src/ide-contextual-queries/printAst.ql

@@ -13,7 +13,7 @@ import codeql_ql.printAstAst
 import codeql.IDEContextual
 
 /**
- * The source file to generate an AST from.
+ * Gets the source file to generate an AST from.
  */
 external string selectedSourceFile();
 

ql/ql/src/queries/style/RegexpInsteadOfPattern.ql

@@ -13,13 +13,16 @@ import ql
  * Gets a regular expression pattern that matches the syntax of likely regular expressions.
  */
 private string getALikelyRegExpPattern() {
-  result = "/.*/[gimuy]{1,5}" or // pattern with at least one flag: /foo/i
+  result =
-  result = "/\\^.*/[gimuy]{0,5}" or // pattern with anchor: /^foo/
+    [
-  result = "/.*\\$/[gimuy]{0,5}" or // pattern with anchor: /foo$/
+      "/.*/[gimuy]{1,5}", // pattern with at least one flag: /foo/i
-  result = "\\^.*\\$" or // pattern body with anchors: ^foo$
+      "/\\^.*/[gimuy]{0,5}", // pattern with anchor: /^foo/
-  result = ".*(?<!\\\\)\\\\[dDwWsSB].*" or // contains a builtin character class: \s
+      "/.*\\$/[gimuy]{0,5}", // pattern with anchor: /foo$/
-  result = ".*(?<!\\\\)\\\\[\\[\\]()*+?{}|^$.].*" or // contains an escaped meta-character: \(
+      "\\^.*\\$", // pattern body with anchors: ^foo$
-  result = ".*\\[\\^?[\\p{Alnum}\\p{Blank}_-]+\\][*+].*" // contains a quantified custom character class: [^a-zA-Z123]+
+      ".*(?<!\\\\)\\\\[dDwWsSB].*", // contains a builtin character class: \s
+      ".*(?<!\\\\)\\\\[\\[\\]()*+?{}|^$.].*", // contains an escaped meta-character: \(
+      ".*\\[\\^?[\\p{Alnum}\\p{Blank}_-]+\\][*+].*" // contains a quantified custom character class: [^a-zA-Z123]+
+    ]
 }
 
 predicate isMatchesCall(MemberCall c) { c.getMemberName() = "matches" }

ruby/ql/lib/codeql/ruby/dataflow/FlowSummary.qll

@@ -32,12 +32,18 @@ module SummaryComponent {
   /** Gets a summary component that represents an element in an array at an unknown index. */
   SummaryComponent arrayElementUnknown() { result = SC::content(TUnknownArrayElementContent()) }
 
-  /** Gets a summary component that represents an element in an array at a known index. */
+  /**
+   * Gets a summary component that represents an element in an array at a known index.
+   *
+   * Has no result for negative indices. Wrap-around interpretation of negative indices should be
+   * handled by the caller, if modeling a function that has such behavior.
+   */
   bindingset[i]
   SummaryComponent arrayElementKnown(int i) {
     result = SC::content(TKnownArrayElementContent(i))
     or
     // `i` may be out of range
+    i >= 0 and
     not exists(TKnownArrayElementContent(i)) and
     result = arrayElementUnknown()
   }

ruby/ql/lib/codeql/ruby/dataflow/internal/AccessPathSyntax.qll

@@ -6,6 +6,15 @@
  * (which does not use the shared data flow libraries).
  */
 
+/**
+ * Convenience-predicate for extracting two capture groups at once.
+ */
+bindingset[input, regexp]
+private predicate regexpCaptureTwo(string input, string regexp, string capture1, string capture2) {
+  capture1 = input.regexpCapture(regexp, 1) and
+  capture2 = input.regexpCapture(regexp, 2)
+}
+
 /** Companion module to the `AccessPath` class. */
 module AccessPath {
   /** A string that should be parsed as an access path. */
@@ -13,6 +22,95 @@ module AccessPath {
     bindingset[this]
     Range() { any() }
   }
+
+  /**
+   * Parses an integer constant `n` or interval `n1..n2` (inclusive) and gets the value
+   * of the constant or any value contained in the interval.
+   */
+  bindingset[arg]
+  int parseInt(string arg) {
+    result = arg.toInt()
+    or
+    // Match "n1..n2"
+    exists(string lo, string hi |
+      regexpCaptureTwo(arg, "(-?\\d+)\\.\\.(-?\\d+)", lo, hi) and
+      result = [lo.toInt() .. hi.toInt()]
+    )
+  }
+
+  /**
+   * Parses a lower-bounded interval `n..` and gets the lower bound.
+   */
+  bindingset[arg]
+  private int parseLowerBound(string arg) {
+    result = arg.regexpCapture("(-?\\d+)\\.\\.", 1).toInt()
+  }
+
+  /**
+   * Parses an integer constant or interval (bounded or unbounded) that explicitly
+   * references the arity, such as `N-1` or `N-3..N-1`.
+   *
+   * Note that expressions of form `N-x` will never resolve to a negative index,
+   * even if `N` is zero (it will have no result in that case).
+   */
+  bindingset[arg, arity]
+  private int parseIntWithExplicitArity(string arg, int arity) {
+    result >= 0 and // do not allow N-1 to resolve to a negative index
+    exists(string lo |
+      // N-x
+      lo = arg.regexpCapture("N-(\\d+)", 1) and
+      result = arity - lo.toInt()
+      or
+      // N-x..
+      lo = arg.regexpCapture("N-(\\d+)\\.\\.", 1) and
+      result = [arity - lo.toInt(), arity - 1]
+    )
+    or
+    exists(string lo, string hi |
+      // x..N-y
+      regexpCaptureTwo(arg, "(-?\\d+)\\.\\.N-(\\d+)", lo, hi) and
+      result = [lo.toInt() .. arity - hi.toInt()]
+      or
+      // N-x..N-y
+      regexpCaptureTwo(arg, "N-(\\d+)\\.\\.N-(\\d+)", lo, hi) and
+      result = [arity - lo.toInt() .. arity - hi.toInt()] and
+      result >= 0
+      or
+      // N-x..y
+      regexpCaptureTwo(arg, "N-(\\d+)\\.\\.(\\d+)", lo, hi) and
+      result = [arity - lo.toInt() .. hi.toInt()] and
+      result >= 0
+    )
+  }
+
+  /**
+   * Parses an integer constant or interval (bounded or unbounded) and gets any
+   * of the integers contained within (of which there may be infinitely many).
+   *
+   * Has no result for arguments involving an explicit arity, such as `N-1`.
+   */
+  bindingset[arg, result]
+  int parseIntUnbounded(string arg) {
+    result = parseInt(arg)
+    or
+    result >= parseLowerBound(arg)
+  }
+
+  /**
+   * Parses an integer constant or interval (bounded or unbounded) that
+   * may reference the arity of a call, such as `N-1` or `N-3..N-1`.
+   *
+   * Note that expressions of form `N-x` will never resolve to a negative index,
+   * even if `N` is zero (it will have no result in that case).
+   */
+  bindingset[arg, arity]
+  int parseIntWithArity(string arg, int arity) {
+    result = parseInt(arg)
+    or
+    result in [parseLowerBound(arg) .. arity - 1]
+    or
+    result = parseIntWithExplicitArity(arg, arity)
+  }
 }
 
 /** Gets the `n`th token on the access path as a string. */

ruby/ql/lib/codeql/ruby/dataflow/internal/FlowSummaryImplSpecific.qll

@@ -59,7 +59,7 @@ predicate summaryElement(DataFlowCallable c, string input, string output, string
  * is currently restricted to `"BlockArgument"`.
  */
 bindingset[c]
-SummaryComponent interpretComponentSpecific(string c) {
+SummaryComponent interpretComponentSpecific(AccessPathToken c) {
   c = "Receiver" and
   result = FlowSummary::SummaryComponent::receiver()
   or
@@ -76,15 +76,10 @@ SummaryComponent interpretComponentSpecific(string c) {
   result = FlowSummary::SummaryComponent::arrayElementUnknown()
   or
   exists(int i |
-    c.regexpCapture("ArrayElement\\[([0-9]+)\\]", 1).toInt() = i and
+    c.getName() = "ArrayElement" and
+    i = AccessPath::parseInt(c.getAnArgument()) and
     result = FlowSummary::SummaryComponent::arrayElementKnown(i)
   )
-  or
-  exists(int i1, int i2 |
-    c.regexpCapture("ArrayElement\\[([-0-9]+)\\.\\.([0-9]+)\\]", 1).toInt() = i1 and
-    c.regexpCapture("ArrayElement\\[([-0-9]+)\\.\\.([0-9]+)\\]", 2).toInt() = i2 and
-    result = FlowSummary::SummaryComponent::arrayElementKnown([i1 .. i2])
-  )
 }
 
 /** Gets the textual representation of a summary component in the format used for flow summaries. */
@@ -172,25 +167,14 @@ module ParsePositions {
     )
   }
 
-  bindingset[s]
-  private int parsePosition(string s) {
-    result = s.regexpCapture("([-0-9]+)", 1).toInt()
-    or
-    exists(int n1, int n2 |
-      s.regexpCapture("([-0-9]+)\\.\\.([0-9]+)", 1).toInt() = n1 and
-      s.regexpCapture("([-0-9]+)\\.\\.([0-9]+)", 2).toInt() = n2 and
-      result in [n1 .. n2]
-    )
-  }
-
   predicate isParsedParameterPosition(string c, int i) {
     isParamBody(c) and
-    i = parsePosition(c)
+    i = AccessPath::parseInt(c)
   }
 
   predicate isParsedArgumentPosition(string c, int i) {
     isArgBody(c) and
-    i = parsePosition(c)
+    i = AccessPath::parseInt(c)
   }
 }
 

ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll

@@ -11,6 +11,7 @@ private import codeql.ruby.ast.internal.Module
 private import codeql.ruby.ApiGraphs
 private import ActionView
 private import codeql.ruby.frameworks.ActionDispatch
+private import codeql.ruby.dataflow.FlowSummary
 
 /**
  * A `ClassDeclaration` for a class that extends `ActionController::Base`.
@@ -348,3 +349,65 @@ private class ActionControllerProtectFromForgeryCall extends CSRFProtectionSetti
     if this.getWithValueText() = "exception" then result = true else result = false
   }
 }
+
+module ActionController {
+  module Parameters {
+    /**
+     * A dataflow node which is an instance of `ActionController::Parameters`.
+     */
+    private class Params extends DataFlow::Node {
+      Params() {
+        this.asExpr().getExpr() instanceof ActionControllerParamsCall or
+        exists(Params p | p.(DataFlow::LocalSourceNode).flowsTo(this))
+      }
+    }
+
+    private class FetchSummary extends SummarizedCallable {
+      FetchSummary() { this = "ActionController::Parameters#fetch" }
+
+      override MethodCall getACall() {
+        exists(Params params, DataFlow::CallNode fetchNode |
+          fetchNode.getMethodName() = "fetch" and
+          fetchNode.getReceiver() = params and
+          result = fetchNode.asExpr().getExpr()
+        )
+      }
+
+      override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
+        input = "Receiver" and
+        output = "Result" and
+        preservesValue = false
+      }
+    }
+
+    /**
+     * A flow summary for methods that return a modified instance of `ActionController::Parameters`, with taint preserved.
+     */
+    private class ModifierSummary extends SummarizedCallable {
+      ModifierSummary() { this = "ActionController::Parameters#<various methods>" }
+
+      string getMethodName() {
+        result =
+          [
+            "compact", "compact!", "compact_blank", "compact_blank!", "deep_dup",
+            "deep_transform_keys", "deep_transform_keys!", "except", "reject", "reject!", "select",
+            "select!", "slice", "slice!", "to_h", "to_hash"
+          ]
+      }
+
+      override MethodCall getACall() {
+        exists(Params params, DataFlow::CallNode fetchNode |
+          fetchNode.getMethodName() = "fetch" and
+          fetchNode.getReceiver() = params and
+          result = fetchNode.asExpr().getExpr()
+        )
+      }
+
+      override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
+        input = "Receiver" and
+        output = "Result" and
+        preservesValue = false
+      }
+    }
+  }
+}

ruby/ql/src/queries/meta/CallGraph.ql

@@ -0,0 +1,15 @@
+/**
+ * @name Call graph
+ * @description An edge in the call graph.
+ * @kind problem
+ * @problem.severity recommendation
+ * @id rb/meta/call-graph
+ * @tags meta
+ * @precision very-low
+ */
+
+import codeql.ruby.AST
+
+from Call invoke, Callable f
+where invoke.getATarget() = f
+select invoke, "Call to $@", f, f.toString()

ruby/ql/src/queries/meta/TaintSinks.ql

@@ -0,0 +1,14 @@
+/**
+ * @name Taint sinks
+ * @description Sinks that are sensitive to untrusted data.
+ * @kind problem
+ * @problem.severity recommendation
+ * @id rb/meta/taint-sinks
+ * @tags meta
+ * @precision very-low
+ */
+
+import internal.TaintMetrics
+
+from string kind
+select relevantTaintSink(kind), kind + " sink"

ruby/ql/src/queries/meta/TaintSources.ql

@@ -0,0 +1,14 @@
+/**
+ * @name Taint sources
+ * @description Sources of untrusted input.
+ * @kind problem
+ * @problem.severity recommendation
+ * @id rb/meta/taint-sources
+ * @tags meta
+ * @precision very-low
+ */
+
+import internal.TaintMetrics
+
+from string kind
+select relevantTaintSource(kind), kind

ruby/ql/src/queries/meta/internal/TaintMetrics.qll

@@ -0,0 +1,38 @@
+private import codeql.files.FileSystem
+private import codeql.ruby.DataFlow
+private import codeql.ruby.dataflow.RemoteFlowSources
+private import codeql.ruby.security.CodeInjectionCustomizations
+private import codeql.ruby.security.CommandInjectionCustomizations
+private import codeql.ruby.security.XSS
+private import codeql.ruby.security.PathInjectionCustomizations
+private import codeql.ruby.security.ServerSideRequestForgeryCustomizations
+private import codeql.ruby.security.UnsafeDeserializationCustomizations
+private import codeql.ruby.security.UrlRedirectCustomizations
+
+class RelevantFile extends File {
+  RelevantFile() { not getRelativePath().regexpMatch(".*/test(case)?s?/.*") }
+}
+
+RemoteFlowSource relevantTaintSource(string kind) {
+  result.getLocation().getFile() instanceof RelevantFile and
+  kind = result.getSourceType()
+}
+
+DataFlow::Node relevantTaintSink(string kind) {
+  result.getLocation().getFile() instanceof RelevantFile and
+  (
+    kind = "CodeInjection" and result instanceof CodeInjection::Sink
+    or
+    kind = "CommandInjection" and result instanceof CommandInjection::Sink
+    or
+    kind = "XSS" and result instanceof ReflectedXSS::Sink
+    or
+    kind = "PathInjection" and result instanceof PathInjection::Sink
+    or
+    kind = "ServerSideRequestForgery" and result instanceof ServerSideRequestForgery::Sink
+    or
+    kind = "UnsafeDeserialization" and result instanceof UnsafeDeserialization::Sink
+    or
+    kind = "UrlRedirect" and result instanceof UrlRedirect::Sink
+  )
+}