Merge remote-tracking branch 'upstream/master' into ExceptionalPromise

Author: erik-krogh

change-notes/1.24/analysis-cpp.md

@@ -35,3 +35,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
     about the _name or scope_ of variables should remain unchanged.
   * The `LocalScopeVariableReachability` library is deprecated in favor of
     `StackVariableReachability`. The functionality is the same.
+* The taint tracking library (`semmle.code.cpp.dataflow.TaintTracking`) has had
+  the following improvements:
+  * The library now models data flow through `strdup` and similar functions.
+  

change-notes/1.24/analysis-java.md

@@ -10,6 +10,7 @@ The following changes in version 1.24 affect Java analysis in all applications.
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
+| Failure to use HTTPS or SFTP URL in Maven artifact upload/download (`java/maven/non-https-url`) | security, external/cwe/cwe-300, external/cwe/cwe-319, external/cwe/cwe-494, external/cwe/cwe-829 | Finds use of insecure protocols during Maven dependency resolution. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 

change-notes/1.24/analysis-javascript.md

@@ -20,6 +20,8 @@
 |---------------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | Cross-site scripting through exception (`js/xss-through-exception`) | security, external/cwe/cwe-079, external/cwe/cwe-116              | Highlights potential XSS vulnerabilities where an exception is written to the DOM. Results are not shown on LGTM by default. |
 | Regular expression always matches (`js/regex/always-matches`) | correctness, regular-expressions | Highlights regular expression checks that trivially succeed by matching an empty substring. Results are shown on LGTM by default. |
+| Missing await (`js/missing-await`) | correctness | Highlights expressions that operate directly on a promise object in a nonsensical way, instead of awaiting its result. Results are shown on LGTM by default. |
+| Prototype pollution in utility function (`js/prototype-pollution-utility`) | security, external/cwe/cwe-400, external/cwe/cwe-471 | Highlights recursive copying operations that are susceptible to prototype pollution. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 
@@ -29,7 +31,8 @@
 | Duplicate parameter names (`js/duplicate-parameter-name`) | Fewer results | This query now recognizes additional parameters that reasonably can have duplicated names. |
 | Incomplete string escaping or encoding (`js/incomplete-sanitization`) | Fewer false positive results | This query now recognizes additional cases where a single replacement is likely to be intentional. |
 | Unbound event handler receiver (`js/unbound-event-handler-receiver`) | Fewer false positive results | This query now recognizes additional ways event handler receivers can be bound. | 
-| Expression has no effect (`js/useless-expression`) | Fewer false positive results | The query now recognizes block-level flow type annotations. |
+| Expression has no effect (`js/useless-expression`) | Fewer false positive results | The query now recognizes block-level flow type annotations and ignores the first statement of a try block. |
+| Use of call stack introspection in strict mode (`js/strict-mode-call-stack-introspection`) | Fewer false positive results | The query no longer flags expression statements. |
 
 ## Changes to libraries
 

cpp/ql/src/Likely Bugs/InconsistentCheckReturnNull.ql

@@ -25,10 +25,16 @@ predicate assertInvocation(File f, int line) {
   )
 }
 
-predicate nullCheckAssert(Expr e, Variable v, Declaration qualifier) {
-  nullCheckInCondition(e, v, qualifier) and
+class InterestingExpr extends Expr {
+  InterestingExpr() { nullCheckInCondition(this, _, _) }
+}
+
+predicate nullCheckAssert(InterestingExpr e, Variable v, Declaration qualifier) {
   exists(File f, int i |
-    e.getLocation().getStartLine() = i and e.getFile() = f and assertInvocation(f, i)
+    e.getLocation().getStartLine() = i and
+    e.getFile() = f and
+    assertInvocation(f, i) and
+    nullCheckInCondition(e, v, qualifier)
   )
 }
 

cpp/ql/src/Likely Bugs/Likely Typos/AssignWhereCompareMeant.ql

@@ -38,14 +38,31 @@ abstract class BooleanControllingAssignment extends AssignExpr {
   abstract predicate isWhitelisted();
 }
 
+/**
+ * Gets an operand of a logical operation expression (we need the restriction
+ * to BinaryLogicalOperation expressions to get the correct transitive closure).
+ */
+Expr getComparisonOperand(BinaryLogicalOperation op) { result = op.getAnOperand() }
+
 class BooleanControllingAssignmentInExpr extends BooleanControllingAssignment {
   BooleanControllingAssignmentInExpr() {
     this.getParent() instanceof UnaryLogicalOperation or
     this.getParent() instanceof BinaryLogicalOperation or
     exists(ConditionalExpr c | c.getCondition() = this)
   }
 
-  override predicate isWhitelisted() { this.getConversion().(ParenthesisExpr).isParenthesised() }
+  override predicate isWhitelisted() {
+    this.getConversion().(ParenthesisExpr).isParenthesised()
+    or
+    // whitelist this assignment if all comparison operations in the expression that this
+    // assignment is part of, are not parenthesized. In that case it seems like programmer
+    // is fine with unparenthesized comparison operands to binary logical operators, and
+    // the parenthesis around this assignment was used to call it out as an assignment.
+    this.isParenthesised() and
+    forex(ComparisonOperation op | op = getComparisonOperand*(this.getParent+()) |
+      not op.isParenthesised()
+    )
+  }
 }
 
 class BooleanControllingAssignmentInStmt extends BooleanControllingAssignment {
@@ -65,7 +82,8 @@ class BooleanControllingAssignmentInStmt extends BooleanControllingAssignment {
  */
 predicate candidateResult(BooleanControllingAssignment ae) {
   ae.getRValue().isConstant() and
-  not ae.isWhitelisted()
+  not ae.isWhitelisted() and
+  not ae.getRValue() instanceof StringLiteral
 }
 
 /**
@@ -81,5 +99,6 @@ predicate candidateVariable(Variable v) {
 from BooleanControllingAssignment ae, UndefReachability undef
 where
   candidateResult(ae) and
+  not ae.isFromUninstantiatedTemplate(_) and
   not undef.reaches(_, ae.getLValue().(VariableAccess).getTarget(), ae.getLValue())
 select ae, "Use of '=' where '==' may have been intended."

cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql

@@ -16,12 +16,19 @@ import semmle.code.cpp.security.Overflow
 import semmle.code.cpp.security.Security
 import semmle.code.cpp.security.TaintTracking
 
+predicate isRandCall(FunctionCall fc) { fc.getTarget().getName() = "rand" }
+
+predicate isRandCallOrParent(Expr e) {
+  isRandCall(e) or
+  isRandCallOrParent(e.getAChild())
+}
+
 predicate isRandValue(Expr e) {
-  e.(FunctionCall).getTarget().getName() = "rand"
+  isRandCall(e)
   or
   exists(MacroInvocation mi |
     e = mi.getExpr() and
-    e.getAChild*().(FunctionCall).getTarget().getName() = "rand"
+    isRandCallOrParent(e)
   )
 }