Merge branch 'master' into url-concat

Author: Max Schaefer

change-notes/1.19/analysis-cpp.md

@@ -16,6 +16,7 @@
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
 | Empty branch of conditional | Fewer false positive results | The query now recognizes commented blocks more reliably. |
+| Expression has no effect | Fewer false positive results | Expressions in template instantiations are now excluded from this query. |
 | Resource not released in destructor | Fewer false positive results | Placement new is now excluded from the query. Also fixed an issue where false positives could occur if the destructor body was not in the snapshot. |
 | Missing return statement (`cpp/missing-return`) | Visible by default | The precision of this query has been increased from 'medium' to 'high', which makes it visible by default in LGTM. It was 'medium' in release 1.17 and 1.18 because it had false positives due to an extractor bug that was fixed in 1.18. |
 | Missing return statement | Fewer false positive results | The query is now produces correct results when a function returns a template-dependent type. |

change-notes/1.19/analysis-csharp.md

@@ -3,14 +3,15 @@
 ## General improvements
 
 * Control flow graph improvements:
-* The control flow graph construction now takes simple Boolean conditions on local scope variables into account. For example, in `if (b) x = 0; if (b) x = 1;`, the control flow graph will reflect that taking the `true` (resp. `false`) branch in the first condition implies taking the same branch in the second condition. In effect, the first assignment to `x` will now be identified as being dead.
+  * The control flow graph construction now takes simple Boolean conditions on local scope variables into account. For example, in `if (b) x = 0; if (b) x = 1;`, the control flow graph will reflect that taking the `true` (resp. `false`) branch in the first condition implies taking the same branch in the second condition. In effect, the first assignment to `x` will now be identified as being dead.
   * Code that is only reachable from a constant failing assertion, such as `Debug.Assert(false)`, is considered to be unreachable.
 
 ## New queries
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
-| *@name of query (Query ID)* | *Tags*    |*Aim of the new query and whether it is enabled by default or not*  |
+| Using a package with a known vulnerability (cs/use-of-vulnerable-package) | security, external/cwe/cwe-937 | Finds project build files that import packages with known vulnerabilities. This is included by default. |
+
 
 ## Changes to existing queries
 
@@ -21,3 +22,5 @@
 
 
 ## Changes to QL libraries
+
+* `getArgument()` on `AccessorCall` has been improved so it now takes tuple assignments into account. For example, the argument for the implicit `value` parameter in the setter of property `P` is `0` in `(P, x) = (0, 1)`. Additionally, the argument for the `value` parameter in compound assignments is now only the expanded value, for example, in `P += 7` the argument is `P + 7` and not `7`.

change-notes/1.19/analysis-javascript.md

@@ -9,9 +9,7 @@
 * Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following features:
   - file system access, for example through [fs-extra](https://github.com/jprichardson/node-fs-extra) or [globby](https://www.npmjs.com/package/globby)
   - outbound network access, for example through the [fetch API](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API)
-  - the [Google Cloud Spanner](https://cloud.google.com/spanner), [lodash](https://lodash.com), [underscore](https://underscorejs.org/), [async](https://www.npmjs.com/package/async) and [async-es](https://www.npmjs.com/package/async-es) libraries
-
-* The type inference now handles nested imports (that is, imports not appearing at the toplevel). This may yield fewer false-positive results on projects that use this non-standard language feature.
+  - the [lodash](https://lodash.com), [underscore](https://underscorejs.org/), [async](https://www.npmjs.com/package/async) and [async-es](https://www.npmjs.com/package/async-es) libraries
 
 * Type inference for function calls has been improved. This may give additional results for queries that rely on type inference.
 
@@ -25,6 +23,7 @@
 | Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. |
 | Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. |
 | Unclear precedence of nested operators (`js/unclear-operator-precedence`) | maintainability, correctness, external/cwe/cwe-783 | Highlights nested binary operators whose relative precedence is easy to misunderstand. Results shown on LGTM by default. |
+| Useless assignment to property | maintainability | Highlights property assignments whose value is always overwritten. Results are shown on LGTM by default. |
 | User-controlled data in file | security, external/cwe/cwe-912 | Highlights locations where user-controlled data is written to a file. Results are not shown on LGTM by default. |
 
 ## Changes to existing queries
@@ -39,13 +38,13 @@
 | Server-side URL redirect | More results | This rule now recognizes redirection calls in more cases. |
 | Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that may be used by `eval` calls. |
 | Unused variable, import, function or class | Fewer results | This rule now flags import statements with multiple unused imports once. |
-| User-controlled bypass of security check | Fewer results | This rule no longer flags conditions that guard early returns. The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. | 
 | Whitespace contradicts operator precedence | Fewer false-positive results | This rule no longer flags operators with asymmetric whitespace. |
 | Unused import | Fewer false-positive results | This rule no longer flags imports used by the `transform-react-jsx` Babel plugin. |
 | Self assignment | Fewer false-positive results | This rule now ignores self-assignments preceded by a JSDoc comment with a `@type` tag. |
 | Client side cross-site scripting | More results | This rule now also flags HTML injection in the body of an email. |
 | Client-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. |
 | Server-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. |
+| Information exposure through a stack trace | More results | This rule now also flags cases where the entire exception object (including the stack trace) may be exposed. |
 
 ## Changes to QL libraries
 

change-notes/1.19/extractor-javascript.md

@@ -21,8 +21,6 @@
 
 ## Changes to code extraction
 
-* Destructuring assignments are now modeled more precisely, which fixes both false-negative and false-positive results for the rules
-  "Missing variable declaration" and "Useless assignment to local variable" in certain corner cases.
 * The TypeScript compiler is now bundled with the distribution, and no longer needs to be installed manually.
   Should the compiler version need to be overridden, set the `SEMMLE_TYPESCRIPT_HOME` environment variable to
   point to an installation of the `typescript` NPM package.

cpp/ql/src/Critical/InconsistentNullnessTesting.cpp

@@ -1,10 +1,10 @@
 void* f() {
-	block = malloc(BLOCK_SIZE);
+	block = (MyBlock *)malloc(sizeof(MyBlock));
 	if (block) { //correct: block is checked for nullness here
 		block->id = NORMAL_BLOCK_ID;
 	}
 	//...
 	/* make sure data-portion is null-terminated */
-	block[BLOCK_SIZE - 1] = '\0'; //wrong: block not checked for nullness here
+	block->data[BLOCK_SIZE - 1] = '\0'; //wrong: block not checked for nullness here
 	return block;
 }

cpp/ql/src/Critical/OverflowCalculated.cpp

@@ -1,5 +1,5 @@
 void f(char* string) {
-	// wrong: allocates space for characters, put not zero terminator
+	// wrong: allocates space for characters, but not zero terminator
 	char* buf = malloc(strlen(string));
 	strcpy(buf, string);
 

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

@@ -100,6 +100,7 @@ where // EQExprs are covered by CompareWhereAssignMeant.ql
       not accessInInitOfForStmt(peivc) and
       not peivc.isCompilerGenerated() and
       not exists(Macro m | peivc = m.getAnInvocation().getAnExpandedElement()) and
+      not peivc.isFromTemplateInstantiation(_) and
       parent = peivc.getParent() and
       not parent.isInMacroExpansion() and
       not parent instanceof PureExprInVoidContext and

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

@@ -0,0 +1,20 @@
+#define FLAGS   0x4004
+
+void f_warning(int i)
+{
+    // The usage of the logical not operator in this case is unlikely to be correct
+    // as the output is being used as an operator for a bit-wise and operation
+    if (i & !FLAGS) 
+    {
+        // code
+    }
+}
+
+
+void f_fixed(int i)
+{
+    if (i & ~FLAGS) // Changing the logical not operator for the bit-wise not operator would fix this logic
+    {
+        // code
+    }
+}

cpp/ql/src/Likely Bugs/Likely Typos/IncorrectNotOperatorUsage.qhelp

@@ -0,0 +1,26 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>This rule finds logical-not operator usage as an operator for in a bit-wise operation.</p>
+
+<p>Due to the nature of logical operation result value, only the lowest bit could possibly be set, and it is unlikely to be intent in bitwise opeartions. Violations are often indicative of a typo, using a logical-not (<code>!</code>) opeartor instead of the bit-wise not (<code>~</code>) operator. </p>
+<p>This rule is restricted to analyze bit-wise and (<code>&amp;</code>) and bit-wise or (<code>|</code>) operation in order to provide better precision.</p>
+<p>This rule ignores instances where a double negation (<code>!!</code>) is explicitly used as the opeartor of the bitwise operation, as this is a commonly used as a mechanism to normalize an integer value to either 1 or 0.</p>
+<p>NOTE: It is not recommended to use this rule in kernel code or older C code as it will likely find several false positive instances.</p>
+
+</overview>
+<recommendation>
+<p>Carefully inspect the flagged expressions. Consider the intent in the code logic, and decide whether it is necessary to change the not operator.</p>
+</recommendation>
+
+<example><sample src="IncorrectNotOperatorUsage.cpp" /></example>
+
+<references>
+  <li>
+    <a href="https://docs.microsoft.com/en-us/visualstudio/code-quality/c6317?view=vs-2017">warning C6317: incorrect operator: logical-not (!) is not interchangeable with ones-complement (~)</a>
+  </li>
+</references>
+</qhelp>

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

@@ -0,0 +1,35 @@
+/**
+ * @name Incorrect Not Operator Usage
+ * @description Usage of a logical-not (!) operator as an operand for a bit-wise operation.
+ *              This commonly indicates the usage of an incorrect operator instead of the bit-wise not (~) operator,
+ *              also known as ones' complement operator.
+ * @kind problem
+ * @id cpp/incorrect-not-operator-usage
+ * @problem.severity warning
+ * @precision medium
+ * @tags security
+ *       external/cwe/cwe-480
+ *       external/microsoft/c6317
+ */
+
+import cpp
+
+/**
+ * It's common in some projects to use "a double negation" to normalize the boolean
+ * result to either 1 or 0.
+ * This predciate is intended to filter explicit usage of a double negation as it typically 
+ * indicates the explicit purpose to normalize the result for bit-wise or arithmetic purposes. 
+ */
+predicate doubleNegationNormalization( NotExpr notexpr ){
+  notexpr.getAnOperand() instanceof NotExpr
+}
+
+from BinaryBitwiseOperation binbitwop
+where exists( NotExpr notexpr | 
+  binbitwop.getAnOperand() = notexpr
+  and not doubleNegationNormalization(notexpr)
+  and ( binbitwop instanceof BitwiseAndExpr
+    or binbitwop instanceof BitwiseOrExpr )
+)
+select binbitwop, "Usage of a logical not (!) expression as a bitwise operator."
+