Merge pull request #137 from github/erik-krogh/even-more-consistency

even more consistency

Author: erik-krogh

ql/src/codeql_ql/performance/InefficientStringComparisonQuery.qll

@@ -45,10 +45,10 @@ class RegexpMatchPredicate extends BuiltinPredicate {
 predicate canUseMatchInsteadOfRegexpMatch(Call c, string matchesStr) {
   c.getTarget() instanceof RegexpMatchPredicate and
   exists(string raw | raw = c.getArgument(0).(String).getValue() |
-    matchesStr = "%" + raw.regexpCapture("^\\.\\*(\\w+)$", _)
+    matchesStr = "%" + raw.regexpCapture("^\\.\\*([a-zA-Z\\d\\s-]+)$", _)
     or
-    matchesStr = raw.regexpCapture("^(\\w+)\\.\\*$", _) + "%"
+    matchesStr = raw.regexpCapture("^([a-zA-Z\\d\\s-]+)\\.\\*$", _) + "%"
     or
-    matchesStr = "%" + raw.regexpCapture("^\\.\\*(\\w+)\\.\\*$", _) + "%"
+    matchesStr = "%" + raw.regexpCapture("^\\.\\*([a-zA-Z\\d\\s-]+)\\.\\*$", _) + "%"
   )
 }

ql/src/queries/diagnostics/EmptyConsistencies.ql

@@ -33,10 +33,10 @@ where
   or
   AstConsistency::nonTotalGetParent(node) and msg = "AstConsistency::nonTotalGetParent"
   or
-  //or // has 1 result, but the CodeQL compiler also can't figure out that one. I suppoed the file is never imported.
-  //TypeConsistency::noResolve(node) and msg = "TypeConsistency::noResolve"
-  //or // has 1 result, but the CodeQL compiler also can't figure out that one. Same file as above.
-  //ModConsistency::noResolve(node) and msg = "ModConsistency::noResolve"
+  TypeConsistency::noResolve(node) and msg = "TypeConsistency::noResolve"
+  or
+  ModConsistency::noResolve(node) and msg = "ModConsistency::noResolve"
+  or
   ModConsistency::noResolveModuleExpr(node) and msg = "ModConsistency::noResolveModuleExpr"
   or
   VarConsistency::noFieldDef(node) and msg = "VarConsistency::noFieldDef"

repo-tests/codeql.txt

@@ -1 +1 @@
-6c2713dd8bf76ae1207e3123900a04d6f89b5162
+1f3f7e9ccc631177f671f3d465faec3477cbe1c5

repo-tests/codeql/cpp/ql/src/Security/CWE/CWE-313/CleartextSqliteDatabase.ql

@@ -29,7 +29,7 @@ class SqliteFunctionCall extends FunctionCall {
 }
 
 predicate sqlite_encryption_used() {
-  any(StringLiteral l).getValue().toLowerCase().regexpMatch("pragma key.*") or
+  any(StringLiteral l).getValue().toLowerCase().matches("pragma key%") or
   any(StringLiteral l).getValue().toLowerCase().matches("%attach%database%key%") or
   any(FunctionCall fc).getTarget().getName().matches("sqlite%\\_key\\_%")
 }

…ts/codeql/csharp/ql/src/Linq/Helpers.qll …ts/codeql/csharp/ql/lib/Linq/Helpers.qllrepo-tests/codeql/csharp/ql/src/Linq/Helpers.qll renamed to repo-tests/codeql/csharp/ql/lib/Linq/Helpers.qll repo-tests/codeql/csharp/ql/src/Linq/Helpers.qll renamed to repo-tests/codeql/csharp/ql/lib/Linq/Helpers.qll

@@ -268,13 +268,13 @@ module ControlFlow {
 
     /** A node for a callable exit point, annotated with the type of exit. */
     class AnnotatedExitNode extends Node, TAnnotatedExitNode {
-      private Callable c;
+      private CfgScope scope;
       private boolean normal;
 
-      AnnotatedExitNode() { this = TAnnotatedExitNode(c, normal) }
+      AnnotatedExitNode() { this = TAnnotatedExitNode(scope, normal) }
 
       /** Gets the callable that this exit applies to. */
-      Callable getCallable() { result = c }
+      CfgScope getCallable() { result = scope }
 
       /** Holds if this node represents a normal exit. */
       predicate isNormal() { normal = true }
@@ -285,31 +285,35 @@ module ControlFlow {
 
       override Callable getEnclosingCallable() { result = this.getCallable() }
 
-      override Location getLocation() { result = this.getCallable().getLocation() }
+      override Location getLocation() { result = scope.getLocation() }
 
       override string toString() {
         exists(string s |
           normal = true and s = "normal"
           or
           normal = false and s = "abnormal"
         |
-          result = "exit " + this.getCallable() + " (" + s + ")"
+          result = "exit " + scope + " (" + s + ")"
         )
       }
     }
 
     /** A node for a callable exit point. */
     class ExitNode extends Node, TExitNode {
+      private CfgScope scope;
+
+      ExitNode() { this = TExitNode(scope) }
+
       /** Gets the callable that this exit applies to. */
-      Callable getCallable() { this = TExitNode(result) }
+      Callable getCallable() { result = scope }
 
       override BasicBlocks::ExitBlock getBasicBlock() { result = Node.super.getBasicBlock() }
 
       override Callable getEnclosingCallable() { result = this.getCallable() }
 
-      override Location getLocation() { result = this.getCallable().getLocation() }
+      override Location getLocation() { result = scope.getLocation() }
 
-      override string toString() { result = "exit " + this.getCallable().toString() }
+      override string toString() { result = "exit " + scope }
     }
 
     /**

repo-tests/codeql/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowGraph.qll

@@ -97,6 +97,24 @@ module ControlFlowTree {
   predicate idOf(Range_ x, int y) = equivalenceRelation(id/2)(x, y)
 }
 
+/**
+ * The `expr_parent_top_level_adjusted()` relation restricted to exclude relations
+ * between properties and their getters' expression bodies in properties such as
+ * `int P => 0`.
+ *
+ * This is in order to only associate the expression body with one CFG scope, namely
+ * the getter (and not the declaration itself).
+ */
+private predicate expr_parent_top_level_adjusted2(
+  Expr child, int i, @top_level_exprorstmt_parent parent
+) {
+  expr_parent_top_level_adjusted(child, i, parent) and
+  not exists(Getter g |
+    g.getDeclaration() = parent and
+    i = 0
+  )
+}
+
 /** Holds if `first` is first executed when entering `scope`. */
 predicate scopeFirst(CfgScope scope, ControlFlowElement first) {
   scope =
@@ -109,17 +127,23 @@ predicate scopeFirst(CfgScope scope, ControlFlowElement first) {
         else first(c.getBody(), first)
     )
   or
-  expr_parent_top_level_adjusted(any(Expr e | first(e, first)), _, scope) and
+  expr_parent_top_level_adjusted2(any(Expr e | first(e, first)), _, scope) and
   not scope instanceof Callable
 }
 
 /** Holds if `scope` is exited when `last` finishes with completion `c`. */
-predicate scopeLast(Callable scope, ControlFlowElement last, Completion c) {
-  last(scope.getBody(), last, c) and
-  not c instanceof GotoCompletion
+predicate scopeLast(CfgScope scope, ControlFlowElement last, Completion c) {
+  scope =
+    any(Callable callable |
+      last(callable.getBody(), last, c) and
+      not c instanceof GotoCompletion
+      or
+      last(InitializerSplitting::lastConstructorInitializer(scope, _), last, c) and
+      not callable.hasBody()
+    )
   or
-  last(InitializerSplitting::lastConstructorInitializer(scope, _), last, c) and
-  not scope.hasBody()
+  expr_parent_top_level_adjusted2(any(Expr e | last(e, last, c)), _, scope) and
+  not scope instanceof Callable
 }
 
 private class ConstructorTree extends ControlFlowTree, Constructor {

repo-tests/codeql/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll

@@ -85,6 +85,9 @@ module Public {
     /** Holds if this stack contains summary component `c`. */
     predicate contains(SummaryComponent c) { c = this.drop(_).head() }
 
+    /** Gets the bottom element of this stack. */
+    SummaryComponent bottom() { result = this.drop(this.length() - 1).head() }
+
     /** Gets a textual representation of this stack. */
     string toString() {
       exists(SummaryComponent head, SummaryComponentStack tail |
@@ -197,6 +200,8 @@ module Private {
       or
       tail.(RequiredSummaryComponentStack).required(TParameterSummaryComponent(_)) and
       head = thisParam()
+      or
+      derivedFluentFlowPush(_, _, _, head, tail, _)
     }
 
   pragma[nomagic]
@@ -210,7 +215,7 @@ module Private {
     c.propagatesFlow(output, input, preservesValue) and
     preservesValue = true and
     isCallbackParameter(input) and
-    isContentOfArgument(output)
+    isContentOfArgument(output, _)
     or
     // flow from the receiver of a callback into the instance-parameter
     exists(SummaryComponentStack s, SummaryComponentStack callbackRef |
@@ -222,16 +227,81 @@ module Private {
       output = TConsSummaryComponentStack(thisParam(), input) and
       preservesValue = true
     )
+    or
+    exists(SummaryComponentStack arg, SummaryComponentStack return |
+      derivedFluentFlow(c, input, arg, return, preservesValue)
+    |
+      arg.length() = 1 and
+      output = return
+      or
+      exists(SummaryComponent head, SummaryComponentStack tail |
+        derivedFluentFlowPush(c, input, arg, head, tail, 0) and
+        output = SummaryComponentStack::push(head, tail)
+      )
+    )
+    or
+    // Chain together summaries where values get passed into callbacks along the way
+    exists(SummaryComponentStack mid, boolean preservesValue1, boolean preservesValue2 |
+      c.propagatesFlow(input, mid, preservesValue1) and
+      c.propagatesFlow(mid, output, preservesValue2) and
+      mid.drop(mid.length() - 2) =
+        SummaryComponentStack::push(TParameterSummaryComponent(_),
+          SummaryComponentStack::singleton(TArgumentSummaryComponent(_))) and
+      preservesValue = preservesValue1.booleanAnd(preservesValue2)
+    )
+  }
+
+  /**
+   * Holds if `c` has a flow summary from `input` to `arg`, where `arg`
+   * writes to (contents of) the `i`th argument, and `c` has a
+   * value-preserving flow summary from the `i`th argument to a return value
+   * (`return`).
+   *
+   * In such a case, we derive flow from `input` to (contents of) the return
+   * value.
+   *
+   * As an example, this simplifies modeling of fluent methods:
+   * for `StringBuilder.append(x)` with a specified value flow from qualifier to
+   * return value and taint flow from argument 0 to the qualifier, then this
+   * allows us to infer taint flow from argument 0 to the return value.
+   */
+  pragma[nomagic]
+  private predicate derivedFluentFlow(
+    SummarizedCallable c, SummaryComponentStack input, SummaryComponentStack arg,
+    SummaryComponentStack return, boolean preservesValue
+  ) {
+    exists(int i |
+      summary(c, input, arg, preservesValue) and
+      isContentOfArgument(arg, i) and
+      summary(c, SummaryComponentStack::singleton(TArgumentSummaryComponent(i)), return, true) and
+      return.bottom() = TReturnSummaryComponent(_)
+    )
+  }
+
+  pragma[nomagic]
+  private predicate derivedFluentFlowPush(
+    SummarizedCallable c, SummaryComponentStack input, SummaryComponentStack arg,
+    SummaryComponent head, SummaryComponentStack tail, int i
+  ) {
+    derivedFluentFlow(c, input, arg, tail, _) and
+    head = arg.drop(i).head() and
+    i = arg.length() - 2
+    or
+    exists(SummaryComponent head0, SummaryComponentStack tail0 |
+      derivedFluentFlowPush(c, input, arg, head0, tail0, i + 1) and
+      head = arg.drop(i).head() and
+      tail = SummaryComponentStack::push(head0, tail0)
+    )
   }
 
   private predicate isCallbackParameter(SummaryComponentStack s) {
     s.head() = TParameterSummaryComponent(_) and exists(s.tail())
   }
 
-  private predicate isContentOfArgument(SummaryComponentStack s) {
-    s.head() = TContentSummaryComponent(_) and isContentOfArgument(s.tail())
+  private predicate isContentOfArgument(SummaryComponentStack s, int i) {
+    s.head() = TContentSummaryComponent(_) and isContentOfArgument(s.tail(), i)
     or
-    s = TSingletonSummaryComponentStack(TArgumentSummaryComponent(_))
+    s = TSingletonSummaryComponentStack(TArgumentSummaryComponent(i))
   }
 
   private predicate outputState(SummarizedCallable c, SummaryComponentStack s) {
@@ -508,9 +578,14 @@ module Private {
    * node, and back out to `p`.
    */
   predicate summaryAllowParameterReturnInSelf(ParamNode p) {
-    exists(SummarizedCallable c, int i |
-      c.clearsContent(i, _) and
-      p.isParameterOf(c, i)
+    exists(SummarizedCallable c, int i | p.isParameterOf(c, i) |
+      c.clearsContent(i, _)
+      or
+      exists(SummaryComponentStack inputContents, SummaryComponentStack outputContents |
+        summary(c, inputContents, outputContents, _) and
+        inputContents.bottom() = pragma[only_bind_into](TArgumentSummaryComponent(i)) and
+        outputContents.bottom() = pragma[only_bind_into](TArgumentSummaryComponent(i))
+      )
     )
   }
 
@@ -534,22 +609,6 @@ module Private {
         preservesValue = false and not summary(c, inputContents, outputContents, true)
       )
       or
-      // If flow through a method updates a parameter from some input A, and that
-      // parameter also is returned through B, then we'd like a combined flow from A
-      // to B as well. As an example, this simplifies modeling of fluent methods:
-      // for `StringBuilder.append(x)` with a specified value flow from qualifier to
-      // return value and taint flow from argument 0 to the qualifier, then this
-      // allows us to infer taint flow from argument 0 to the return value.
-      succ instanceof ParamNode and
-      summaryPostUpdateNode(pred, succ) and
-      preservesValue = true
-      or
-      // Similarly we would like to chain together summaries where values get passed
-      // into callbacks along the way.
-      pred instanceof ArgNode and
-      summaryPostUpdateNode(succ, pred) and
-      preservesValue = true
-      or
       exists(SummarizedCallable c, int i |
         pred.(ParamNode).isParameterOf(c, i) and
         succ = summaryNode(c, TSummaryNodeClearsContentState(i, _)) and

repo-tests/codeql/csharp/ql/lib/semmle/code/csharp/dataflow/internal/FlowSummaryImpl.qll

@@ -174,32 +174,34 @@ module EntityFramework {
     }
   }
 
-  private class RawSqlStringSummarizedCallable extends EFSummarizedCallable {
-    private SummaryComponentStack input_;
-    private SummaryComponentStack output_;
-    private boolean preservesValue_;
-
-    RawSqlStringSummarizedCallable() {
+  private class RawSqlStringConstructorSummarizedCallable extends EFSummarizedCallable {
+    RawSqlStringConstructorSummarizedCallable() {
       exists(RawSqlStringStruct s |
         this = s.getAConstructor() and
-        input_ = SummaryComponentStack::argument(0) and
-        this.getNumberOfParameters() > 0 and
-        output_ = SummaryComponentStack::return() and
-        preservesValue_ = false
-        or
-        this = s.getAConversionTo() and
-        input_ = SummaryComponentStack::argument(0) and
-        output_ = SummaryComponentStack::return() and
-        preservesValue_ = false
+        this.getNumberOfParameters() > 0
       )
     }
 
     override predicate propagatesFlow(
       SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue
     ) {
-      input = input_ and
-      output = output_ and
-      preservesValue = preservesValue_
+      input = SummaryComponentStack::argument(0) and
+      output = SummaryComponentStack::return() and
+      preservesValue = false
+    }
+  }
+
+  private class RawSqlStringConversionSummarizedCallable extends EFSummarizedCallable {
+    RawSqlStringConversionSummarizedCallable() {
+      exists(RawSqlStringStruct s | this = s.getAConversionTo())
+    }
+
+    override predicate propagatesFlow(
+      SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue
+    ) {
+      input = SummaryComponentStack::argument(0) and
+      output = SummaryComponentStack::return() and
+      preservesValue = false
     }
   }
 

repo-tests/codeql/csharp/ql/lib/semmle/code/csharp/frameworks/EntityFramework.qll

@@ -12,7 +12,7 @@
  */
 
 import csharp
-import Helpers
+import Linq.Helpers
 
 /** The enumerable sequence is likely not to be repeatable. */
 predicate likelyNonRepeatableSequence(IEnumerableSequence seq) {