Merge branch 'master' of git.semmle.com:Semmle/ql into UrlSearch

Author: erik-krogh

change-notes/1.24/analysis-javascript.md

@@ -42,11 +42,14 @@
   - [ncp](https://www.npmjs.com/package/ncp)
   - [node-dir](https://www.npmjs.com/package/node-dir)
   - [path-exists](https://www.npmjs.com/package/path-exists)
+  - [pg](https://www.npmjs.com/package/pg)
   - [react](https://www.npmjs.com/package/react)
   - [recursive-readdir](https://www.npmjs.com/package/recursive-readdir)
   - [request](https://www.npmjs.com/package/request)
   - [rimraf](https://www.npmjs.com/package/rimraf)
   - [send](https://www.npmjs.com/package/send)
+  - [SockJS](https://www.npmjs.com/package/sockjs)
+  - [SockJS-client](https://www.npmjs.com/package/sockjs-client)
   - [typeahead.js](https://www.npmjs.com/package/typeahead.js)
   - [vinyl-fs](https://www.npmjs.com/package/vinyl-fs)
   - [write-file-atomic](https://www.npmjs.com/package/write-file-atomic)
@@ -82,6 +85,7 @@
 | Use of password hash with insufficient computational effort (`js/insufficient-password-hash`) | Fewer false positive results | This query now recognizes additional cases that do not require secure hashing. |
 | Useless regular-expression character escape (`js/useless-regexp-character-escape`) | Fewer false positive results | This query now distinguishes escapes in strings and regular expression literals. |
 | Identical operands (`js/redundant-operation`) | Fewer results | This query now recognizes cases where the operands change a value using ++/-- expressions. |
+| Superfluous trailing arguments (`js/superfluous-trailing-arguments`) | Fewer results | This query now recognizes cases where a function uses the `Function.arguments` value to process a variable number of parameters. |
 
 ## Changes to libraries
 

change-notes/1.24/analysis-python.md

@@ -15,6 +15,7 @@ Support for Django version 2.x and 3.x
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
+| Uncontrolled command line (`py/command-line-injection`) | More results | We now model the `fabric` and `invoke` pacakges for command execution. |
 
 ### Web framework support
 

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

@@ -133,10 +133,16 @@ class Function extends Declaration, ControlFlowNode, AccessHolder, @function {
    */
   Type getUnspecifiedType() { result = getType().getUnspecifiedType() }
 
-  /** Gets the nth parameter of this function. */
+  /**
+   * Gets the nth parameter of this function. There is no result for the
+   * implicit `this` parameter, and there is no `...` varargs pseudo-parameter.
+   */
   Parameter getParameter(int n) { params(unresolveElement(result), underlyingElement(this), n, _) }
 
-  /** Gets a parameter of this function. */
+  /**
+   * Gets a parameter of this function. There is no result for the implicit
+   * `this` parameter, and there is no `...` varargs pseudo-parameter.
+   */
   Parameter getAParameter() { params(unresolveElement(result), underlyingElement(this), _, _) }
 
   /**

cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -60,7 +60,14 @@ private DataFlow::Node getNodeForSource(Expr source) {
   (
     result = DataFlow::exprNode(source)
     or
-    result = DataFlow::definitionByReferenceNode(source)
+    // Some of the sources in `isUserInput` are intended to match the value of
+    // an expression, while others (those modeled below) are intended to match
+    // the taint that propagates out of an argument, like the `char *` argument
+    // to `gets`. It's impossible here to tell which is which, but the "access
+    // to argv" source is definitely not intended to match an output argument,
+    // and it causes false positives if we let it.
+    result = DataFlow::definitionByReferenceNode(source) and
+    not argv(source.(VariableAccess).getTarget())
   )
 }
 
@@ -183,7 +190,7 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
   i2.(UnaryInstruction).getUnary() = i1
   or
   i2.(ChiInstruction).getPartial() = i1 and
-  not isChiForAllAliasedMemory(i2)
+  not i2.isResultConflated()
   or
   exists(BinaryInstruction bin |
     bin = i2 and
@@ -276,19 +283,6 @@ private predicate modelTaintToParameter(Function f, int parameterIn, int paramet
   )
 }
 
-/**
- * Holds if `chi` is on the chain of chi-instructions for all aliased memory.
- * Taint should not pass through these instructions since they tend to mix up
- * unrelated objects.
- */
-private predicate isChiForAllAliasedMemory(Instruction instr) {
-  instr.(ChiInstruction).getTotal() instanceof AliasedDefinitionInstruction
-  or
-  isChiForAllAliasedMemory(instr.(ChiInstruction).getTotal())
-  or
-  isChiForAllAliasedMemory(instr.(PhiInstruction).getAnInputOperand().getAnyDef())
-}
-
 private predicate modelTaintToReturnValue(Function f, int parameterIn) {
   // Taint flow from parameter to return value
   exists(FunctionInput modelIn, FunctionOutput modelOut |

cpp/ql/src/semmle/code/cpp/ir/implementation/IRConfiguration.qll

@@ -16,6 +16,8 @@ class IRConfiguration extends TIRConfiguration {
    * Holds if IR should be created for function `func`. By default, holds for all functions.
    */
   predicate shouldCreateIRForFunction(Language::Function func) { any() }
+
+  predicate shouldEvaluateDebugStringsForFunction(Language::Function func) { any() }
 }
 
 private newtype TIREscapeAnalysisConfiguration = MkIREscapeAnalysisConfiguration()

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

@@ -27,6 +27,9 @@ class IRBlockBase extends TIRBlock {
    * by debugging and printing code only.
    */
   int getDisplayIndex() {
+    exists(IRConfiguration::IRConfiguration config |
+      config.shouldEvaluateDebugStringsForFunction(this.getEnclosingFunction())
+    ) and
     this =
       rank[result + 1](IRBlock funcBlock |
         funcBlock.getEnclosingFunction() = getEnclosingFunction()

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

@@ -5,6 +5,7 @@ import IRTypeSanity // module is in IRType.qll
 module InstructionSanity {
   private import internal.InstructionImports as Imports
   private import Imports::OperandTag
+  private import Imports::Overlap
   private import internal.IRInternal
 
   /**
@@ -272,4 +273,48 @@ module InstructionSanity {
     func = switchInstr.getEnclosingIRFunction() and
     funcText = Language::getIdentityString(func.getFunction())
   }
+
+  /**
+   * Holds if `instr` is on the chain of chi/phi instructions for all aliased
+   * memory.
+   */
+  private predicate isOnAliasedDefinitionChain(Instruction instr) {
+    instr instanceof AliasedDefinitionInstruction
+    or
+    isOnAliasedDefinitionChain(instr.(ChiInstruction).getTotal())
+    or
+    isOnAliasedDefinitionChain(instr.(PhiInstruction).getAnInputOperand().getAnyDef())
+  }
+
+  private predicate shouldBeConflated(Instruction instr) {
+    isOnAliasedDefinitionChain(instr)
+    or
+    instr instanceof UnmodeledDefinitionInstruction
+    or
+    instr.getOpcode() instanceof Opcode::InitializeNonLocal
+  }
+
+  query predicate notMarkedAsConflated(Instruction instr) {
+    shouldBeConflated(instr) and
+    not instr.isResultConflated()
+  }
+
+  query predicate wronglyMarkedAsConflated(Instruction instr) {
+    instr.isResultConflated() and
+    not shouldBeConflated(instr)
+  }
+
+  query predicate invalidOverlap(
+    MemoryOperand useOperand, string message, IRFunction func, string funcText
+  ) {
+    exists(Overlap overlap |
+      overlap = useOperand.getDefinitionOverlap() and
+      overlap instanceof MayPartiallyOverlap and
+      message =
+        "MemoryOperand '" + useOperand.toString() + "' has a `getDefinitionOverlap()` of '" +
+          overlap.toString() + "'." and
+      func = useOperand.getEnclosingIRFunction() and
+      funcText = Language::getIdentityString(func.getFunction())
+    )
+  }
 }

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

@@ -15,6 +15,9 @@ private import Imports::OperandTag
  * `File` and line number. Used for assigning register names when printing IR.
  */
 private Instruction getAnInstructionAtLine(IRFunction irFunc, Language::File file, int line) {
+  exists(IRConfiguration::IRConfiguration config |
+    config.shouldEvaluateDebugStringsForFunction(irFunc.getFunction())
+  ) and
   exists(Language::Location ___location |
     irFunc = result.getEnclosingIRFunction() and
     ___location = result.getLocation() and
@@ -39,13 +42,20 @@ class Instruction extends Construction::TInstruction {
     result = getResultString() + " = " + getOperationString() + " " + getOperandsString()
   }
 
+  private predicate shouldGenerateDumpStrings() {
+    exists(IRConfiguration::IRConfiguration config |
+      config.shouldEvaluateDebugStringsForFunction(this.getEnclosingFunction())
+    )
+  }
+
   /**
    * Gets a string describing the operation of this instruction. This includes
    * the opcode and the immediate value, if any. For example:
    *
    * VariableAddress[x]
    */
   final string getOperationString() {
+    shouldGenerateDumpStrings() and
     if exists(getImmediateString())
     then result = getOperationPrefix() + getOpcode().toString() + "[" + getImmediateString() + "]"
     else result = getOperationPrefix() + getOpcode().toString()
@@ -57,10 +67,12 @@ class Instruction extends Construction::TInstruction {
   string getImmediateString() { none() }
 
   private string getOperationPrefix() {
+    shouldGenerateDumpStrings() and
     if this instanceof SideEffectInstruction then result = "^" else result = ""
   }
 
   private string getResultPrefix() {
+    shouldGenerateDumpStrings() and
     if getResultIRType() instanceof IRVoidType
     then result = "v"
     else
@@ -74,6 +86,7 @@ class Instruction extends Construction::TInstruction {
    * used by debugging and printing code only.
    */
   int getDisplayIndexInBlock() {
+    shouldGenerateDumpStrings() and
     exists(IRBlock block |
       this = block.getInstruction(result)
       or
@@ -87,6 +100,7 @@ class Instruction extends Construction::TInstruction {
   }
 
   private int getLineRank() {
+    shouldGenerateDumpStrings() and
     this =
       rank[result](Instruction instr |
         instr =
@@ -105,6 +119,7 @@ class Instruction extends Construction::TInstruction {
    * Example: `r1_1`
    */
   string getResultId() {
+    shouldGenerateDumpStrings() and
     result = getResultPrefix() + getAST().getLocation().getStartLine() + "_" + getLineRank()
   }
 
@@ -116,6 +131,7 @@ class Instruction extends Construction::TInstruction {
    * Example: `r1_1(int*)`
    */
   final string getResultString() {
+    shouldGenerateDumpStrings() and
     result = getResultId() + "(" + getResultLanguageType().getDumpString() + ")"
   }
 
@@ -126,6 +142,7 @@ class Instruction extends Construction::TInstruction {
    * Example: `func:r3_4, this:r3_5`
    */
   string getOperandsString() {
+    shouldGenerateDumpStrings() and
     result =
       concat(Operand operand |
         operand = getAnOperand()
@@ -321,6 +338,17 @@ class Instruction extends Construction::TInstruction {
     Construction::hasModeledMemoryResult(this)
   }
 
+  /**
+   * Holds if this is an instruction with a memory result that represents a
+   * conflation of more than one memory allocation.
+   *
+   * This happens in practice when dereferencing a pointer that cannot be
+   * tracked back to a single local allocation. Such memory is instead modeled
+   * as originating on the `AliasedDefinitionInstruction` at the entry of the
+   * function.
+   */
+  final predicate isResultConflated() { Construction::hasConflatedMemoryResult(this) }
+
   /**
    * Gets the successor of this instruction along the control flow edge
    * specified by `kind`.

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

@@ -384,6 +384,8 @@ class PositionalArgumentOperand extends ArgumentOperand {
 
 class SideEffectOperand extends TypedOperand {
   override SideEffectOperandTag tag;
+
+  override string toString() { result = "SideEffect" }
 }
 
 /**

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

@@ -18,19 +18,19 @@ class PrintIRConfiguration extends TPrintIRConfiguration {
   predicate shouldPrintFunction(Language::Function func) { any() }
 }
 
-private predicate shouldPrintFunction(Language::Function func) {
-  exists(PrintIRConfiguration config | config.shouldPrintFunction(func))
-}
-
 /**
- * Override of `IRConfiguration` to only create IR for the functions that are to be dumped.
+ * Override of `IRConfiguration` to only evaluate debug strings for the functions that are to be dumped.
  */
 private class FilteredIRConfiguration extends IRConfiguration {
-  override predicate shouldCreateIRForFunction(Language::Function func) {
+  override predicate shouldEvaluateDebugStringsForFunction(Language::Function func) {
     shouldPrintFunction(func)
   }
 }
 
+private predicate shouldPrintFunction(Language::Function func) {
+  exists(PrintIRConfiguration config | config.shouldPrintFunction(func))
+}
+
 private string getAdditionalInstructionProperty(Instruction instr, string key) {
   exists(IRPropertyProvider provider | result = provider.getInstructionProperty(instr, key))
 }