Merge branch 'master' of https://github.com/github/codeql into pr/erik-krogh/3478

Author: erik-krogh

change-notes/1.25/analysis-javascript.md

@@ -4,6 +4,7 @@
 
 * Support for the following frameworks and libraries has been improved:
   - [bluebird](http://bluebirdjs.com/)
+  - [express](https://www.npmjs.com/package/express)
   - [fstream](https://www.npmjs.com/package/fstream)
   - [jGrowl](https://github.com/stanlemon/jGrowl)
   - [jQuery](https://jquery.com/)
@@ -17,6 +18,7 @@
 |---------------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | Cross-site scripting through DOM (`js/xss-through-dom`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities where existing text from the DOM is used as HTML. Results are not shown on LGTM by default. |
 | Incomplete HTML attribute sanitization (`js/incomplete-html-attribute-sanitization`) | security, external/cwe/cwe-20, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities due to incomplete sanitization of HTML meta-characters. Results are shown on LGTM by default. |
+| Unsafe expansion of self-closing HTML tag (`js/unsafe-html-expansion`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities caused by unsafe expansion of self-closing HTML tags. |
 
 ## Changes to existing queries
 
@@ -25,10 +27,16 @@
 | Misspelled variable name (`js/misspelled-variable-name`) | Message changed | The message for this query now correctly identifies the misspelled variable in additional cases. |
 | Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. |
 | Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. |
+| Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | Less results | This query now recognizes additional safe patterns of doing URL redirects. |
+| Client-side cross-site scripting (`js/xss`) | Less results | This query now recognizes additional safe strings based on URLs. |
+| Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes additional url scheme checks. |
+| Prototype pollution in utility function (`js/prototype-pollution-utility`) | More results | This query now recognizes additional utility functions as vulnerable to prototype polution. |
 | Expression has no effect (`js/useless-expression`) | Less results | This query no longer flags an expression when that expression is the only content of the containing file. |
 | Unknown directive (`js/unknown-directive`) | Less results | This query no longer flags directives generated by the Babel compiler. |
 | Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving NoSQL code operators are now recognized. |
+| Zip Slip (`js/zipslip`) | More results | This query now recognizes additional vulnerabilities. |
 
 ## Changes to libraries
 
+* A library `semmle.javascript.explore.CallGraph` has been added to help write queries for exploring the call graph.
 * Added data flow for `Map` and `Set`, and added matching type-tracking steps that can accessed using the `CollectionsTypeTracking` module.

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

@@ -184,17 +184,8 @@ class Function extends Declaration, ControlFlowNode, AccessHolder, @function {
    * For example: for a function `int Foo(int p1, int p2)` this would
    * return `int p1, int p2`.
    */
-  string getParameterString() { result = getParameterStringFrom(0) }
-
-  private string getParameterStringFrom(int index) {
-    index = getNumberOfParameters() and
-    result = ""
-    or
-    index = getNumberOfParameters() - 1 and
-    result = getParameter(index).getTypedName()
-    or
-    index < getNumberOfParameters() - 1 and
-    result = getParameter(index).getTypedName() + ", " + getParameterStringFrom(index + 1)
+  string getParameterString() {
+    result = concat(int i | | min(getParameter(i).getTypedName()), ", " order by i)
   }
 
   /** Gets a call to this function. */
@@ -616,18 +607,8 @@ class FunctionDeclarationEntry extends DeclarationEntry, @fun_decl {
    * For example: for a function 'int Foo(int p1, int p2)' this would
    * return 'int p1, int p2'.
    */
-  string getParameterString() { result = getParameterStringFrom(0) }
-
-  private string getParameterStringFrom(int index) {
-    index = getNumberOfParameters() and
-    result = ""
-    or
-    index = getNumberOfParameters() - 1 and
-    result = getParameterDeclarationEntry(index).getTypedName()
-    or
-    index < getNumberOfParameters() - 1 and
-    result =
-      getParameterDeclarationEntry(index).getTypedName() + ", " + getParameterStringFrom(index + 1)
+  string getParameterString() {
+    result = concat(int i | | min(getParameterDeclarationEntry(i).getTypedName()), ", " order by i)
   }
 
   /**

cpp/ql/src/semmle/code/cpp/exprs/Access.qll

@@ -6,9 +6,13 @@ private import semmle.code.cpp.dataflow.EscapesTree
 /**
  * A C/C++ access expression. This refers to a function, variable, or enum constant.
  */
-abstract class Access extends Expr, NameQualifiableElement {
+class Access extends Expr, NameQualifiableElement, @access {
+  // As `@access` is a union type containing `@routineexpr` (which describes function accesses
+  // that are called), we need to exclude function calls.
+  Access() { this instanceof @routineexpr implies not iscall(underlyingElement(this), _) }
+
   /** Gets the accessed function, variable, or enum constant. */
-  abstract Declaration getTarget();
+  Declaration getTarget() { none() } // overridden in subclasses
 
   override predicate mayBeImpure() { none() }
 

cpp/ql/src/semmle/code/cpp/exprs/Cast.qll

@@ -7,7 +7,7 @@ private import semmle.code.cpp.internal.ResolveClass
  * Instances of this class are not present in the main AST which is navigated by parent/child links. Instead,
  * instances of this class are attached to nodes in the main AST via special conversion links.
  */
-abstract class Conversion extends Expr {
+class Conversion extends Expr, @conversion {
   /** Gets the expression being converted. */
   Expr getExpr() { result.getConversion() = this }
 

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll

@@ -14,19 +14,66 @@ private newtype TOperand =
     not Construction::isInCycle(useInstr) and
     strictcount(Construction::getRegisterOperandDefinition(useInstr, tag)) = 1
   } or
-  TNonPhiMemoryOperand(
-    Instruction useInstr, MemoryOperandTag tag, Instruction defInstr, Overlap overlap
-  ) {
-    defInstr = Construction::getMemoryOperandDefinition(useInstr, tag, overlap) and
-    not Construction::isInCycle(useInstr) and
-    strictcount(Construction::getMemoryOperandDefinition(useInstr, tag, _)) = 1
+  TNonPhiMemoryOperand(Instruction useInstr, MemoryOperandTag tag) {
+    useInstr.getOpcode().hasOperand(tag)
   } or
   TPhiOperand(
     PhiInstruction useInstr, Instruction defInstr, IRBlock predecessorBlock, Overlap overlap
   ) {
     defInstr = Construction::getPhiOperandDefinition(useInstr, predecessorBlock, overlap)
   }
 
+/**
+ * Base class for all register operands. This is a placeholder for the IPA union type that we will
+ * eventually use for this purpose.
+ */
+private class RegisterOperandBase extends TRegisterOperand {
+  /** Gets a textual representation of this element. */
+  abstract string toString();
+}
+
+/**
+ * Returns the register operand with the specified parameters.
+ */
+private RegisterOperandBase registerOperand(
+  Instruction useInstr, RegisterOperandTag tag, Instruction defInstr
+) {
+  result = TRegisterOperand(useInstr, tag, defInstr)
+}
+
+/**
+ * Base class for all non-Phi memory operands. This is a placeholder for the IPA union type that we
+ * will eventually use for this purpose.
+ */
+private class NonPhiMemoryOperandBase extends TNonPhiMemoryOperand {
+  /** Gets a textual representation of this element. */
+  abstract string toString();
+}
+
+/**
+ * Returns the non-Phi memory operand with the specified parameters.
+ */
+private NonPhiMemoryOperandBase nonPhiMemoryOperand(Instruction useInstr, MemoryOperandTag tag) {
+  result = TNonPhiMemoryOperand(useInstr, tag)
+}
+
+/**
+ * Base class for all Phi operands. This is a placeholder for the IPA union type that we will
+ * eventually use for this purpose.
+ */
+private class PhiOperandBase extends TPhiOperand {
+  abstract string toString();
+}
+
+/**
+ * Returns the Phi operand with the specified parameters.
+ */
+private PhiOperandBase phiOperand(
+  Instruction useInstr, Instruction defInstr, IRBlock predecessorBlock, Overlap overlap
+) {
+  result = TPhiOperand(useInstr, defInstr, predecessorBlock, overlap)
+}
+
 /**
  * A source operand of an `Instruction`. The operand represents a value consumed by the instruction.
  */
@@ -165,8 +212,8 @@ class Operand extends TOperand {
  */
 class MemoryOperand extends Operand {
   MemoryOperand() {
-    this = TNonPhiMemoryOperand(_, _, _, _) or
-    this = TPhiOperand(_, _, _, _)
+    this instanceof NonPhiMemoryOperandBase or
+    this instanceof PhiOperandBase
   }
 
   /**
@@ -200,18 +247,15 @@ class MemoryOperand extends Operand {
  */
 class NonPhiOperand extends Operand {
   Instruction useInstr;
-  Instruction defInstr;
   OperandTag tag;
 
   NonPhiOperand() {
-    this = TRegisterOperand(useInstr, tag, defInstr) or
-    this = TNonPhiMemoryOperand(useInstr, tag, defInstr, _)
+    this = registerOperand(useInstr, tag, _) or
+    this = nonPhiMemoryOperand(useInstr, tag)
   }
 
   final override Instruction getUse() { result = useInstr }
 
-  final override Instruction getAnyDef() { result = defInstr }
-
   final override string getDumpLabel() { result = tag.getLabel() }
 
   final override int getDumpSortOrder() { result = tag.getSortOrder() }
@@ -222,22 +266,41 @@ class NonPhiOperand extends Operand {
 /**
  * An operand that consumes a register (non-memory) result.
  */
-class RegisterOperand extends NonPhiOperand, TRegisterOperand {
+class RegisterOperand extends NonPhiOperand, RegisterOperandBase {
   override RegisterOperandTag tag;
+  Instruction defInstr;
+
+  RegisterOperand() { this = registerOperand(useInstr, tag, defInstr) }
+
+  final override string toString() { result = tag.toString() }
+
+  final override Instruction getAnyDef() { result = defInstr }
 
   final override Overlap getDefinitionOverlap() {
     // All register results overlap exactly with their uses.
     result instanceof MustExactlyOverlap
   }
 }
 
-class NonPhiMemoryOperand extends NonPhiOperand, MemoryOperand, TNonPhiMemoryOperand {
+class NonPhiMemoryOperand extends NonPhiOperand, MemoryOperand, NonPhiMemoryOperandBase {
   override MemoryOperandTag tag;
-  Overlap overlap;
 
-  NonPhiMemoryOperand() { this = TNonPhiMemoryOperand(useInstr, tag, defInstr, overlap) }
+  NonPhiMemoryOperand() { this = nonPhiMemoryOperand(useInstr, tag) }
 
-  final override Overlap getDefinitionOverlap() { result = overlap }
+  final override string toString() { result = tag.toString() }
+
+  final override Instruction getAnyDef() {
+    result = unique(Instruction defInstr | hasDefinition(defInstr, _))
+  }
+
+  final override Overlap getDefinitionOverlap() { hasDefinition(_, result) }
+
+  pragma[noinline]
+  private predicate hasDefinition(Instruction defInstr, Overlap overlap) {
+    defInstr = Construction::getMemoryOperandDefinition(useInstr, tag, overlap) and
+    not Construction::isInCycle(useInstr) and
+    strictcount(Construction::getMemoryOperandDefinition(useInstr, tag, _)) = 1
+  }
 }
 
 class TypedOperand extends NonPhiMemoryOperand {
@@ -254,8 +317,6 @@ class TypedOperand extends NonPhiMemoryOperand {
  */
 class AddressOperand extends RegisterOperand {
   override AddressOperandTag tag;
-
-  override string toString() { result = "Address" }
 }
 
 /**
@@ -264,8 +325,6 @@ class AddressOperand extends RegisterOperand {
  */
 class BufferSizeOperand extends RegisterOperand {
   override BufferSizeOperandTag tag;
-
-  override string toString() { result = "BufferSize" }
 }
 
 /**
@@ -274,62 +333,48 @@ class BufferSizeOperand extends RegisterOperand {
  */
 class LoadOperand extends TypedOperand {
   override LoadOperandTag tag;
-
-  override string toString() { result = "Load" }
 }
 
 /**
  * The source value operand of a `Store` instruction.
  */
 class StoreValueOperand extends RegisterOperand {
   override StoreValueOperandTag tag;
-
-  override string toString() { result = "StoreValue" }
 }
 
 /**
  * The sole operand of a unary instruction (e.g. `Convert`, `Negate`, `Copy`).
  */
 class UnaryOperand extends RegisterOperand {
   override UnaryOperandTag tag;
-
-  override string toString() { result = "Unary" }
 }
 
 /**
  * The left operand of a binary instruction (e.g. `Add`, `CompareEQ`).
  */
 class LeftOperand extends RegisterOperand {
   override LeftOperandTag tag;
-
-  override string toString() { result = "Left" }
 }
 
 /**
  * The right operand of a binary instruction (e.g. `Add`, `CompareEQ`).
  */
 class RightOperand extends RegisterOperand {
   override RightOperandTag tag;
-
-  override string toString() { result = "Right" }
 }
 
 /**
  * The condition operand of a `ConditionalBranch` or `Switch` instruction.
  */
 class ConditionOperand extends RegisterOperand {
   override ConditionOperandTag tag;
-
-  override string toString() { result = "Condition" }
 }
 
 /**
  * The operand representing the target function of an `Call` instruction.
  */
 class CallTargetOperand extends RegisterOperand {
   override CallTargetOperandTag tag;
-
-  override string toString() { result = "CallTarget" }
 }
 
 /**
@@ -347,43 +392,34 @@ class ArgumentOperand extends RegisterOperand {
  */
 class ThisArgumentOperand extends ArgumentOperand {
   override ThisArgumentOperandTag tag;
-
-  override string toString() { result = "ThisArgument" }
 }
 
 /**
  * An operand representing an argument to a function call.
  */
 class PositionalArgumentOperand extends ArgumentOperand {
   override PositionalArgumentOperandTag tag;
-  int argIndex;
-
-  PositionalArgumentOperand() { argIndex = tag.getArgIndex() }
-
-  override string toString() { result = "Arg(" + argIndex + ")" }
 
   /**
    * Gets the zero-based index of the argument.
    */
-  final int getIndex() { result = argIndex }
+  final int getIndex() { result = tag.getArgIndex() }
 }
 
 class SideEffectOperand extends TypedOperand {
   override SideEffectOperandTag tag;
-
-  override string toString() { result = "SideEffect" }
 }
 
 /**
  * An operand of a `PhiInstruction`.
  */
-class PhiInputOperand extends MemoryOperand, TPhiOperand {
+class PhiInputOperand extends MemoryOperand, PhiOperandBase {
   PhiInstruction useInstr;
   Instruction defInstr;
   IRBlock predecessorBlock;
   Overlap overlap;
 
-  PhiInputOperand() { this = TPhiOperand(useInstr, defInstr, predecessorBlock, overlap) }
+  PhiInputOperand() { this = phiOperand(useInstr, defInstr, predecessorBlock, overlap) }
 
   override string toString() { result = "Phi" }
 
@@ -413,8 +449,6 @@ class PhiInputOperand extends MemoryOperand, TPhiOperand {
 class ChiTotalOperand extends NonPhiMemoryOperand {
   override ChiTotalOperandTag tag;
 
-  override string toString() { result = "ChiTotal" }
-
   final override MemoryAccessKind getMemoryAccess() { result instanceof ChiTotalMemoryAccess }
 }
 
@@ -424,7 +458,5 @@ class ChiTotalOperand extends NonPhiMemoryOperand {
 class ChiPartialOperand extends NonPhiMemoryOperand {
   override ChiPartialOperandTag tag;
 
-  override string toString() { result = "ChiPartial" }
-
   final override MemoryAccessKind getMemoryAccess() { result instanceof ChiPartialMemoryAccess }
 }