Merge from master

Author: Dave Bartolomeo

change-notes/1.23/analysis-java.md

@@ -2,6 +2,12 @@
 
 The following changes in version 1.23 affect Java analysis in all applications.
 
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+| Continue statement that does not continue (`java/continue-in-false-loop`) | correctness | Finds `continue` statements in `do { ... } while (false)` loops. |
+
 ## Changes to existing queries
 
 | **Query**                    | **Expected impact**    | **Change**                        |

change-notes/1.23/analysis-javascript.md

@@ -12,12 +12,15 @@
 
 * The call graph has been improved to resolve method calls in more cases. This may produce more security alerts.
 
+* TypeScript 3.6 features are supported.
+
+
 ## New queries
 
 | **Query**                                                                 | **Tags**                                                          | **Purpose**                                                                                                                                                                            |
 |---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | Unused index variable (`js/unused-index-variable`)                        | correctness                                                       | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. Results are shown on LGTM by default. |
-| Loop bound injection (`js/loop-bound-injection`)                          | security, external/cwe/cwe-834                                      | Highlights loops where a user-controlled object with an arbitrary .length value can trick the server to loop indefinitely. Results are not shown on LGTM by default. |
+| Loop bound injection (`js/loop-bound-injection`)                          | security, external/cwe/cwe-834                                      | Highlights loops where a user-controlled object with an arbitrary .length value can trick the server to loop indefinitely. Results are shown on LGTM by default. |
 | Suspicious method name (`js/suspicious-method-name-declaration`)          | correctness, typescript, methods                                  | Highlights suspiciously named methods where the developer likely meant to write a constructor or function. Results are shown on LGTM by default. |
 | Shell command built from environment values (`js/shell-command-injection-from-environment`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights shell commands that may change behavior inadvertently depending on the execution environment, indicating a possible violation of [CWE-78](https://cwe.mitre.org/data/definitions/78.html). Results are shown on LGTM by default.|
 | Use of returnless function (`js/use-of-returnless-function`)              | maintainability, correctness                                      | Highlights calls where the return value is used, but the callee never returns a value. Results are shown on LGTM by default. |

change-notes/1.23/analysis-python.md

@@ -19,4 +19,4 @@
 | **Query**                  | **Expected impact**    | **Change** |
 |----------------------------|------------------------|------------|
 | Unreachable code | Fewer false positives | Analysis now accounts for uses of `contextlib.suppress` to suppress exceptions. |
-
+| `__iter__` method returns a non-iterator | Better alert message | Alert now highlights which class is expected to be an iterator. |

config/identical-files.json

@@ -76,7 +76,11 @@
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Operand.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll",
     "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Operand.qll",
-    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Operand.qll"
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Operand.qll" 
+  ],
+  "IR IRType": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/IRType.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/IRType.qll"
   ],
   "IR Operand Tag": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/internal/OperandTag.qll",
@@ -169,22 +173,39 @@
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/PrintIRImports.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/PrintIRImports.qll"
   ],
+  "C++ SSA SSAConstructionImports": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstructionImports.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstructionImports.qll"
+  ],
   "C++ SSA AliasAnalysis": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasAnalysis.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll"
   ],
-  "C++ SSA SSAConstruction": [
+  "C++ IR ValueNumberingImports": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/internal/ValueNumberingImports.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/internal/ValueNumberingImports.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/internal/ValueNumberingImports.qll"
+  ],
+  "IR SSA SimpleSSA": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll"
+  ],
+  "IR SSA SSAConstruction": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll",
-    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll"
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll"
   ],
-  "C++ SSA PrintSSA": [
+  "IR SSA PrintSSA": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/PrintSSA.qll",
-    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/PrintSSA.qll"
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/PrintSSA.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/PrintSSA.qll"
   ],
-  "C++ IR ValueNumber": [
+  "IR ValueNumber": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/ValueNumbering.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll",
-    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll"
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/gvn/ValueNumbering.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll"
   ],
   "C++ IR ConstantAnalysis": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/constant/ConstantAnalysis.qll",
@@ -235,5 +256,9 @@
   "C# IR PrintIRImports": [
     "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/internal/PrintIRImports.qll",
     "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/PrintIRImports.qll"
+  ],
+  "C# IR ValueNumberingImports": [
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/gvn/internal/ValueNumberingImports.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/gvn/internal/ValueNumberingImports.qll"
   ]
 }

cpp/ql/src/Likely Bugs/Arithmetic/BadAdditionOverflowCheck.qhelp

@@ -2,36 +2,39 @@
   "-//Semmle//qhelp//EN"
   "qhelp.dtd">
 <qhelp>
-  <overview>
-    <p>
-      Checking for overflow of integer addition needs to be done with
-      care, because automatic type promotion can prevent the check
-      from working correctly.
-    </p>
-  </overview>
-  <recommendation>
-    <p>
-      Use an explicit cast to make sure that the result of the addition is
-      not implicitly converted to a larger type.
-    </p>
-  </recommendation>
-  <example>
-    <sample src="BadAdditionOverflowCheckExample1.cpp" />
-    <p>
-      On a typical architecture where <tt>short</tt> is 16 bits
-      and <tt>int</tt> is 32 bits, the operands of the addition are
-      automatically promoted to <tt>int</tt>, so it cannot overflow
-      and the result of the comparison is always false.
-    </p>
-    <p>
-      The code below implements the check correctly, by using an
-      explicit cast to make sure that the result of the addition
-      is <tt>unsigned short</tt>.
-    </p>
-    <sample src="BadAdditionOverflowCheckExample2.cpp" />
-  </example>
-  <references>
-    <li><a href="http://c-faq.com/expr/preservingrules.html">Preserving Rules</a></li>
-    <li><a href="https://www.securecoding.cert.org/confluence/plugins/servlet/mobile#content/view/20086942">Understand integer conversion rules</a></li>
-  </references>
+
+<overview>
+<p>
+Checking for overflow of integer addition needs to be done with
+care, because automatic type promotion can prevent the check
+from working as intended, with the same value (<code>true</code>
+or <code>false</code>) always being returned.
+</p>
+</overview>
+<recommendation>
+<p>
+Use an explicit cast to make sure that the result of the addition is
+not implicitly converted to a larger type.
+</p>
+</recommendation>
+<example>
+<sample src="BadAdditionOverflowCheckExample1.cpp" />
+<p>
+On a typical architecture where <code>short</code> is 16 bits
+and <code>int</code> is 32 bits, the operands of the addition are
+automatically promoted to <code>int</code>, so it cannot overflow
+and the result of the comparison is always false.
+</p>
+<p>
+The code below implements the check correctly, by using an
+explicit cast to make sure that the result of the addition
+is <code>unsigned short</code> (which may overflow, in which case
+the comparison would evaluate to <code>true</code>).
+</p>
+<sample src="BadAdditionOverflowCheckExample2.cpp" />
+</example>
+<references>
+<li><a href="http://c-faq.com/expr/preservingrules.html">Preserving Rules</a></li>
+<li><a href="https://www.securecoding.cert.org/confluence/plugins/servlet/mobile#content/view/20086942">Understand integer conversion rules</a></li>
+</references>
 </qhelp>

cpp/ql/src/Likely Bugs/Arithmetic/BadAdditionOverflowCheckExample1.cpp

@@ -1,3 +1,4 @@
 bool checkOverflow(unsigned short x, unsigned short y) {
-  return (x + y < x);  // BAD: x and y are automatically promoted to int.
+  // BAD: comparison is always false due to type promotion
+  return (x + y < x);  
 }

cpp/ql/src/Likely Bugs/Arithmetic/ComparisonPrecedence.ql

@@ -18,4 +18,4 @@ where
   co.getAChild() = chco and
   not chco.isParenthesised() and
   not co.isFromUninstantiatedTemplate(_)
-select co, "Check the comparison operator precedence."
+select co, "Comparison as an operand to another comparison."

cpp/ql/src/Likely Bugs/Memory Management/Buffer Overflow/BufferAccess.qll

@@ -0,0 +1,174 @@
+import cpp
+import semmle.code.cpp.dataflow.TaintTracking
+private import semmle.code.cpp.dataflow.RecursionPrevention
+
+/**
+ * A buffer which includes an allocation size.
+ */
+abstract class BufferWithSize extends DataFlow::Node {
+  abstract Expr getSizeExpr();
+
+  BufferAccess getAnAccess() {
+    any(BufferWithSizeConfig bsc).hasFlow(this, DataFlow::exprNode(result.getPointer()))
+  }
+}
+
+/** An allocation function. */
+abstract class Alloc extends Function { }
+
+/**
+ * Allocation functions identified by the QL for C/C++ standard library.
+ */
+class DefaultAlloc extends Alloc {
+  DefaultAlloc() { allocationFunction(this) }
+}
+
+/** A buffer created through a call to an allocation function. */
+class AllocBuffer extends BufferWithSize {
+  FunctionCall call;
+
+  AllocBuffer() {
+    asExpr() = call and
+    call.getTarget() instanceof Alloc
+  }
+
+  override Expr getSizeExpr() { result = call.getArgument(0) }
+}
+
+/**
+ * Find accesses of buffers for which we have a size expression.
+ */
+private class BufferWithSizeConfig extends TaintTracking::Configuration {
+  BufferWithSizeConfig() { this = "BufferWithSize" }
+
+  override predicate isSource(DataFlow::Node n) { n = any(BufferWithSize b) }
+
+  override predicate isSink(DataFlow::Node n) { n.asExpr() = any(BufferAccess ae).getPointer() }
+
+  override predicate isSanitizer(DataFlow::Node s) {
+    s = any(BufferWithSize b) and
+    s.asExpr().getControlFlowScope() instanceof Alloc
+  }
+}
+
+/**
+ * An access (read or write) to a buffer, provided as a pair of
+ * a pointer to the buffer and the length of data to be read or written.
+ * Extend this class to support different kinds of buffer access.
+ */
+abstract class BufferAccess extends Locatable {
+  /** Gets the pointer to the buffer being accessed. */
+  abstract Expr getPointer();
+
+  /** Gets the length of the data being read or written by this buffer access. */
+  abstract Expr getAccessedLength();
+}
+
+/**
+ * A buffer access through an array expression.
+ */
+class ArrayBufferAccess extends BufferAccess, ArrayExpr {
+  override Expr getPointer() { result = this.getArrayBase() }
+
+  override Expr getAccessedLength() { result = this.getArrayOffset() }
+}
+
+/**
+ * A buffer access through an overloaded array expression.
+ */
+class OverloadedArrayBufferAccess extends BufferAccess, OverloadedArrayExpr {
+  override Expr getPointer() { result = this.getQualifier() }
+
+  override Expr getAccessedLength() { result = this.getAnArgument() }
+}
+
+/**
+ * A buffer access through pointer arithmetic.
+ */
+class PointerArithmeticAccess extends BufferAccess, Expr {
+  PointerArithmeticOperation p;
+
+  PointerArithmeticAccess() {
+    this = p and
+    p.getAnOperand().getType().getUnspecifiedType() instanceof IntegralType and
+    not p.getParent() instanceof ComparisonOperation
+  }
+
+  override Expr getPointer() {
+    result = p.getAnOperand() and
+    result.getType().getUnspecifiedType() instanceof PointerType
+  }
+
+  override Expr getAccessedLength() {
+    result = p.getAnOperand() and
+    result.getType().getUnspecifiedType() instanceof IntegralType
+  }
+}
+
+/**
+ * A pair of buffer accesses through a call to memcpy.
+ */
+class MemCpy extends BufferAccess, FunctionCall {
+  MemCpy() { getTarget().hasName("memcpy") }
+
+  override Expr getPointer() {
+    result = getArgument(0) or
+    result = getArgument(1)
+  }
+
+  override Expr getAccessedLength() { result = getArgument(2) }
+}
+
+class StrncpySizeExpr extends BufferAccess, FunctionCall {
+  StrncpySizeExpr() { getTarget().hasName("strncpy") }
+
+  override Expr getPointer() {
+    result = getArgument(0) or
+    result = getArgument(1)
+  }
+
+  override Expr getAccessedLength() { result = getArgument(2) }
+}
+
+class RecvSizeExpr extends BufferAccess, FunctionCall {
+  RecvSizeExpr() { getTarget().hasName("recv") }
+
+  override Expr getPointer() { result = getArgument(1) }
+
+  override Expr getAccessedLength() { result = getArgument(2) }
+}
+
+class SendSizeExpr extends BufferAccess, FunctionCall {
+  SendSizeExpr() { getTarget().hasName("send") }
+
+  override Expr getPointer() { result = getArgument(1) }
+
+  override Expr getAccessedLength() { result = getArgument(2) }
+}
+
+class SnprintfSizeExpr extends BufferAccess, FunctionCall {
+  SnprintfSizeExpr() { getTarget().hasName("snprintf") }
+
+  override Expr getPointer() { result = getArgument(0) }
+
+  override Expr getAccessedLength() { result = getArgument(1) }
+}
+
+class MemcmpSizeExpr extends BufferAccess, FunctionCall {
+  MemcmpSizeExpr() { getTarget().hasName("Memcmp") }
+
+  override Expr getPointer() {
+    result = getArgument(0) or
+    result = getArgument(1)
+  }
+
+  override Expr getAccessedLength() { result = getArgument(2) }
+}
+
+class MallocSizeExpr extends BufferAccess, FunctionCall {
+  MallocSizeExpr() { getTarget().hasName("malloc") }
+
+  override Expr getPointer() { none() }
+
+  override Expr getAccessedLength() { result = getArgument(1) }
+}

cpp/ql/src/Likely Bugs/Memory Management/Buffer Overflow/NtohlArrayBad.cpp

@@ -0,0 +1,6 @@
+int get_number_from_network();
+
+int process_network(int[] buff, int buffSize) {
+  int i = ntohl(get_number_from_network());
+  return buff[i];
+}

cpp/ql/src/Likely Bugs/Memory Management/Buffer Overflow/NtohlArrayGood.cpp

@@ -0,0 +1,10 @@
+uint32_t get_number_from_network();
+
+int process_network(int[] buff, uint32_t buffSize) {
+  uint32_t i = ntohl(get_number_from_network());
+  if (i < buffSize) {
+    return buff[i];
+  } else {
+    return -1;
+  }
+}