Merge branch 'master' into connect-ir-dataflow-models

Author: Robert Marsh

change-notes/1.24/analysis-java.md

@@ -36,3 +36,6 @@ The following changes in version 1.24 affect Java analysis in all applications.
   general file classification mechanism and thus suppression of alerts, and
   also any security queries using taint tracking, as test classes act as
   default barriers stopping taint flow.
+* Parentheses are now no longer modelled directly in the AST, that is, the
+  `ParExpr` class is empty. Instead, a parenthesized expression can be
+  identified with the `Expr.isParenthesized()` member predicate.

change-notes/1.24/analysis-javascript.md

@@ -7,7 +7,9 @@
 * Imports with the `.js` extension can now be resolved to a TypeScript file,
   when the import refers to a file generated by TypeScript.
 
-- The analysis of sanitizer guards has improved, leading to fewer false-positive results from the security queries.
+* Imports that rely on path-mappings from a `tsconfig.json` file can now be resolved.
+
+* The analysis of sanitizer guards has improved, leading to fewer false-positive results from the security queries.
 
 * Support for the following frameworks and libraries has been improved:
   - [react](https://www.npmjs.com/package/react)
@@ -18,6 +20,7 @@
   - [Socket.IO](https://socket.io/)
   - [ws](https://github.com/websockets/ws)
   - [WebSocket](https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API)
+  - [Koa](https://www.npmjs.com/package/koa)
 
 ## New queries
 

cpp/ql/src/semmle/code/cpp/commons/Printf.qll

@@ -258,14 +258,7 @@ class FormatLiteral extends Literal {
    * Gets the position in the string at which the nth conversion specifier
    * starts.
    */
-  int getConvSpecOffset(int n) {
-    n = 0 and result = this.getFormat().indexOf("%", 0, 0)
-    or
-    n > 0 and
-    exists(int p |
-      n = p + 1 and result = this.getFormat().indexOf("%", 0, this.getConvSpecOffset(p) + 2)
-    )
-  }
+  int getConvSpecOffset(int n) { result = this.getFormat().indexOf("%", n, 0) }
 
   /*
    * Each of these predicates gets a regular expressions to match each individual

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

@@ -21,31 +21,19 @@ private predicate predictableInstruction(Instruction instr) {
   predictableInstruction(instr.(UnaryInstruction).getUnary())
 }
 
+private DataFlow::Node getNodeForSource(Expr source) {
+  isUserInput(source, _) and
+  (
+    result = DataFlow::exprNode(source)
+    or
+    result = DataFlow::definitionByReferenceNode(source)
+  )
+}
+
 private class DefaultTaintTrackingCfg extends DataFlow::Configuration {
   DefaultTaintTrackingCfg() { this = "DefaultTaintTrackingCfg" }
 
-  override predicate isSource(DataFlow::Node source) {
-    exists(CallInstruction ci, WriteSideEffectInstruction wsei |
-      userInputArgument(ci.getConvertedResultExpression(), wsei.getIndex()) and
-      source.asInstruction() = wsei and
-      wsei.getPrimaryInstruction() = ci
-    )
-    or
-    userInputReturned(source.asExpr())
-    or
-    isUserInput(source.asExpr(), _)
-    or
-    source.asExpr() instanceof EnvironmentRead
-    or
-    source
-        .asInstruction()
-        .(LoadInstruction)
-        .getSourceAddress()
-        .(VariableAddressInstruction)
-        .getASTVariable()
-        .hasName("argv") and
-    source.asInstruction().getEnclosingFunction().hasGlobalName("main")
-  }
+  override predicate isSource(DataFlow::Node source) { source = getNodeForSource(_) }
 
   override predicate isSink(DataFlow::Node sink) { any() }
 
@@ -59,7 +47,7 @@ private class DefaultTaintTrackingCfg extends DataFlow::Configuration {
 private class ToGlobalVarTaintTrackingCfg extends DataFlow::Configuration {
   ToGlobalVarTaintTrackingCfg() { this = "GlobalVarTaintTrackingCfg" }
 
-  override predicate isSource(DataFlow::Node source) { isUserInput(source.asExpr(), _) }
+  override predicate isSource(DataFlow::Node source) { source = getNodeForSource(_) }
 
   override predicate isSink(DataFlow::Node sink) {
     exists(GlobalOrNamespaceVariable gv | writesVariable(sink.asInstruction(), gv))
@@ -163,6 +151,22 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
   // from `a`.
   i2.(PointerAddInstruction).getLeft() = i1
   or
+  // Until we have from through indirections across calls, we'll take flow out
+  // of the parameter and into its indirection.
+  exists(IRFunction f, Parameter parameter |
+    i1 = getInitializeParameter(f, parameter) and
+    i2 = getInitializeIndirection(f, parameter)
+  )
+  or
+  // Until we have flow through indirections across calls, we'll take flow out
+  // of the indirection and into the argument.
+  // When we get proper flow through indirections across calls, this code can be
+  // moved to `adjusedSink` or possibly into the `DataFlow::ExprNode` class.
+  exists(ReadSideEffectInstruction read |
+    read.getAnOperand().(SideEffectOperand).getAnyDef() = i1 and
+    read.getArgumentDef() = i2
+  )
+  or
   // Flow from argument to return value
   i2 =
     any(CallInstruction call |
@@ -188,6 +192,33 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
     )
 }
 
+pragma[noinline]
+private InitializeIndirectionInstruction getInitializeIndirection(IRFunction f, Parameter p) {
+  result.getParameter() = p and
+  result.getEnclosingIRFunction() = f
+}
+
+pragma[noinline]
+private InitializeParameterInstruction getInitializeParameter(IRFunction f, Parameter p) {
+  result.getParameter() = p and
+  result.getEnclosingIRFunction() = f
+}
+
+/**
+ * Get an instruction that goes into argument `argumentIndex` of `call`. This
+ * can be either directly or through one pointer indirection.
+ */
+private Instruction getACallArgumentOrIndirection(CallInstruction call, int argumentIndex) {
+  result = call.getPositionalArgument(argumentIndex)
+  or
+  exists(ReadSideEffectInstruction readSE |
+    // TODO: why are read side effect operands imprecise?
+    result = readSE.getSideEffectOperand().getAnyDef() and
+    readSE.getPrimaryInstruction() = call and
+    readSE.getIndex() = argumentIndex
+  )
+}
+
 private predicate modelTaintToParameter(Function f, int parameterIn, int parameterOut) {
   exists(FunctionInput modelIn, FunctionOutput modelOut |
     (
@@ -270,31 +301,11 @@ private Element adjustedSink(DataFlow::Node sink) {
   // For compatibility, send flow into a `NotExpr` even if it's part of a
   // short-circuiting condition and thus might get skipped.
   result.(NotExpr).getOperand() = sink.asExpr()
-  or
-  // For compatibility, send flow from argument read side effects to their
-  // corresponding argument expression
-  exists(IndirectReadSideEffectInstruction read |
-    read.getAnOperand().(SideEffectOperand).getAnyDef() = sink.asInstruction() and
-    read.getArgumentDef().getUnconvertedResultExpression() = result
-  )
-  or
-  exists(BufferReadSideEffectInstruction read |
-    read.getAnOperand().(SideEffectOperand).getAnyDef() = sink.asInstruction() and
-    read.getArgumentDef().getUnconvertedResultExpression() = result
-  )
-  or
-  exists(SizedBufferReadSideEffectInstruction read |
-    read.getAnOperand().(SideEffectOperand).getAnyDef() = sink.asInstruction() and
-    read.getArgumentDef().getUnconvertedResultExpression() = result
-  )
 }
 
 predicate tainted(Expr source, Element tainted) {
   exists(DefaultTaintTrackingCfg cfg, DataFlow::Node sink |
-    cfg.hasFlow(DataFlow::exprNode(source), sink)
-    or
-    cfg.hasFlow(DataFlow::definitionByReferenceNode(source), sink)
-  |
+    cfg.hasFlow(getNodeForSource(source), sink) and
     tainted = adjustedSink(sink)
   )
 }
@@ -307,7 +318,7 @@ predicate taintedIncludingGlobalVars(Expr source, Element tainted, string global
     ToGlobalVarTaintTrackingCfg toCfg, FromGlobalVarTaintTrackingCfg fromCfg, DataFlow::Node store,
     GlobalOrNamespaceVariable global, DataFlow::Node load, DataFlow::Node sink
   |
-    toCfg.hasFlow(DataFlow::exprNode(source), store) and
+    toCfg.hasFlow(getNodeForSource(source), store) and
     store
         .asInstruction()
         .(StoreInstruction)

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected

@@ -21,14 +21,18 @@
 | defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:22:8:22:33 | (const char *)... |
 | defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:22:20:22:25 | call to getenv |
 | defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:22:20:22:32 | (const char *)... |
+| defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:24:8:24:10 | (const char *)... |
+| defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:24:8:24:10 | array to pointer conversion |
 | defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:24:8:24:10 | buf |
 | defaulttainttracking.cpp:22:20:22:25 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 |
+| defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:31:40:31:53 | dotted_address |
 | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:32:11:32:26 | p#0 |
 | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:38:11:38:21 | env_pointer |
 | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:38:25:38:30 | call to getenv |
 | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:38:25:38:37 | (void *)... |
 | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:22:39:22 | a |
 | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:26:39:34 | call to inet_addr |
+| defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:36:39:61 | (const char *)... |
 | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:50:39:61 | & ... |
 | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:40:10:40:10 | a |
 | defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 |

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected

@@ -5,8 +5,8 @@
 | defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:3:21:3:22 | s1 | AST only |
 | defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:21:8:21:10 | buf | AST only |
 | defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:22:15:22:17 | buf | AST only |
-| defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:31:40:31:53 | dotted_address | AST only |
-| defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:36:39:61 | (const char *)... | AST only |
+| defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:24:8:24:10 | (const char *)... | IR only |
+| defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:24:8:24:10 | array to pointer conversion | IR only |
 | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:51:39:61 | env_pointer | AST only |
 | defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | IR only |
 | defaulttainttracking.cpp:88:18:88:23 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 | IR only |

java/ql/src/Language Abuse/UselessUpcast.ql

@@ -15,7 +15,7 @@ import java
 predicate usefulUpcast(CastExpr e) {
   // Upcasts that may be performed to affect resolution of methods or constructors.
   exists(Call c, int i, Callable target |
-    c.getArgument(i).getProperExpr() = e and
+    c.getArgument(i) = e and
     target = c.getCallee() and
     // An upcast to the type of the corresponding parameter.
     e.getType() = target.getParameterType(i)
@@ -31,27 +31,25 @@ predicate usefulUpcast(CastExpr e) {
   )
   or
   // Upcasts of a varargs argument.
-  exists(Call c, int iArg, int iParam | c.getArgument(iArg).getProperExpr() = e |
+  exists(Call c, int iArg, int iParam | c.getArgument(iArg) = e |
     c.getCallee().getParameter(iParam).isVarargs() and iArg >= iParam
   )
   or
   // Upcasts that are performed on an operand of a ternary expression.
-  exists(ConditionalExpr ce |
-    e = ce.getTrueExpr().getProperExpr() or e = ce.getFalseExpr().getProperExpr()
-  )
+  exists(ConditionalExpr ce | e = ce.getTrueExpr() or e = ce.getFalseExpr())
   or
   // Upcasts to raw types.
   e.getType() instanceof RawType
   or
   e.getType().(Array).getElementType() instanceof RawType
   or
   // Upcasts that are performed to affect field, private method, or static method resolution.
-  exists(FieldAccess fa | e = fa.getQualifier().getProperExpr() |
+  exists(FieldAccess fa | e = fa.getQualifier() |
     not e.getExpr().getType().(RefType).inherits(fa.getField())
   )
   or
   exists(MethodAccess ma, Method m |
-    e = ma.getQualifier().getProperExpr() and
+    e = ma.getQualifier() and
     m = ma.getMethod() and
     (m.isStatic() or m.isPrivate())
   |

java/ql/src/Likely Bugs/Arithmetic/BadCheckOdd.ql

@@ -15,7 +15,7 @@ import java
 import semmle.code.java.Collections
 
 predicate isDefinitelyPositive(Expr e) {
-  isDefinitelyPositive(e.getProperExpr()) or
+  isDefinitelyPositive(e) or
   e.(IntegerLiteral).getIntValue() >= 0 or
   e.(MethodAccess).getMethod() instanceof CollectionSizeMethod or
   e.(MethodAccess).getMethod() instanceof StringLengthMethod or
@@ -24,10 +24,10 @@ predicate isDefinitelyPositive(Expr e) {
 
 from BinaryExpr t, RemExpr lhs, IntegerLiteral rhs, string parity
 where
-  t.getLeftOperand().getProperExpr() = lhs and
-  t.getRightOperand().getProperExpr() = rhs and
+  t.getLeftOperand() = lhs and
+  t.getRightOperand() = rhs and
   not isDefinitelyPositive(lhs.getLeftOperand()) and
-  lhs.getRightOperand().getProperExpr().(IntegerLiteral).getIntValue() = 2 and
+  lhs.getRightOperand().(IntegerLiteral).getIntValue() = 2 and
   (
     t instanceof EQExpr and rhs.getIntValue() = 1 and parity = "oddness"
     or

java/ql/src/Likely Bugs/Arithmetic/ConstantExpAppearsNonConstant.ql

@@ -17,9 +17,6 @@ predicate isConstantExp(Expr e) {
   // A literal is constant.
   e instanceof Literal
   or
-  // A parenthesized expression is constant if its proper expression is.
-  isConstantExp(e.(ParExpr).getProperExpr())
-  or
   e instanceof TypeAccess
   or
   e instanceof ArrayTypeAccess
@@ -33,26 +30,22 @@ predicate isConstantExp(Expr e) {
   )
   or
   // A cast expression is constant if its expression is.
-  exists(CastExpr c | c = e | isConstantExp(c.getExpr().getProperExpr()))
+  exists(CastExpr c | c = e | isConstantExp(c.getExpr()))
   or
   // Multiplication by 0 is constant.
-  exists(MulExpr m | m = e | eval(m.getAnOperand().getProperExpr()) = 0)
+  exists(MulExpr m | m = e | eval(m.getAnOperand()) = 0)
   or
   // Integer remainder by 1 is constant.
   exists(RemExpr r | r = e |
     r.getLeftOperand().getType() instanceof IntegralType and
-    eval(r.getRightOperand().getProperExpr()) = 1
+    eval(r.getRightOperand()) = 1
   )
   or
-  exists(AndBitwiseExpr a | a = e | eval(a.getAnOperand().getProperExpr()) = 0)
+  exists(AndBitwiseExpr a | a = e | eval(a.getAnOperand()) = 0)
   or
-  exists(AndLogicalExpr a | a = e |
-    a.getAnOperand().getProperExpr().(BooleanLiteral).getBooleanValue() = false
-  )
+  exists(AndLogicalExpr a | a = e | a.getAnOperand().(BooleanLiteral).getBooleanValue() = false)
   or
-  exists(OrLogicalExpr o | o = e |
-    o.getAnOperand().getProperExpr().(BooleanLiteral).getBooleanValue() = true
-  )
+  exists(OrLogicalExpr o | o = e | o.getAnOperand().(BooleanLiteral).getBooleanValue() = true)
 }
 
 from Expr e

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

@@ -35,8 +35,8 @@ float exprBound(Expr e) {
 /** A multiplication that does not overflow. */
 predicate small(MulExpr e) {
   exists(NumType t, float lhs, float rhs, float res | t = e.getType() |
-    lhs = exprBound(e.getLeftOperand().getProperExpr()) and
-    rhs = exprBound(e.getRightOperand().getProperExpr()) and
+    lhs = exprBound(e.getLeftOperand()) and
+    rhs = exprBound(e.getRightOperand()) and
     lhs * rhs = res and
     res <= t.getOrdPrimitiveType().getMaxValue()
   )
@@ -47,7 +47,7 @@ predicate small(MulExpr e) {
  */
 Expr getRestrictedParent(Expr e) {
   result = e.getParent() and
-  (result instanceof ArithExpr or result instanceof ConditionalExpr or result instanceof ParExpr)
+  (result instanceof ArithExpr or result instanceof ConditionalExpr)
 }
 
 from ConversionSite c, MulExpr e, NumType sourceType, NumType destType