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

Author: erik-krogh

CODEOWNERS

@@ -1,7 +1,8 @@
+/cpp/ @Semmle/cpp-analysis
 /csharp/ @Semmle/cs
 /java/ @Semmle/java
 /javascript/ @Semmle/js
-/cpp/ @Semmle/cpp-analysis
+/python/ @Semmle/python
 /cpp/**/*.qhelp @hubwriter
 /csharp/**/*.qhelp @jf205
 /java/**/*.qhelp @felicitymay

change-notes/1.23/analysis-cpp.md

@@ -9,7 +9,8 @@ The following changes in version 1.23 affect C/C++ analysis in all applications.
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
 | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) | reliability, japanese-era | This query is a combination of two old queries that were identical in purpose but separate as an implementation detail.  This new query replaces Hard-coded Japanese era start date in call (`cpp/japanese-era/constructor-or-method-with-exact-era-date`) and Hard-coded Japanese era start date in struct (`cpp/japanese-era/struct-with-exact-era-date`). |
-| Signed overflow check (`cpp/signed-overflow-check`) | correctness, reliability | Finds overflow checks that rely on signed integer addition to overflow, which has undefined behavior. Example: `a + b < a`. |
+| Signed overflow check (`cpp/signed-overflow-check`) | correctness, security | Finds overflow checks that rely on signed integer addition to overflow, which has undefined behavior. Example: `a + b < a`. |
+| Pointer overflow check (`cpp/pointer-overflow-check`) | correctness, security | Finds overflow checks that rely on pointer addition to overflow, which has undefined behavior. Example: `ptr + a < ptr`. |
 
 ## Changes to existing queries
 

change-notes/1.23/analysis-javascript.md

@@ -8,9 +8,14 @@
 
 * Support for the following frameworks and libraries has been improved:
   - [firebase](https://www.npmjs.com/package/firebase)
+  - [get-them-args](https://www.npmjs.com/package/get-them-args)
+  - [minimist](https://www.npmjs.com/package/minimist)
   - [mongodb](https://www.npmjs.com/package/mongodb)
   - [mongoose](https://www.npmjs.com/package/mongoose)
+  - [optimist](https://www.npmjs.com/package/optimist)
+  - [parse-torrent](https://www.npmjs.com/package/parse-torrent)
   - [rate-limiter-flexible](https://www.npmjs.com/package/rate-limiter-flexible)
+  - [yargs](https://www.npmjs.com/package/yargs) 
 
 * The call graph has been improved to resolve method calls in more cases. This may produce more security alerts.
 
@@ -20,14 +25,15 @@
 
 | **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. |
+| Ignoring result from pure array method (`js/ignore-array-result`)         | maintainability, correctness                                      | Highlights calls to array methods without side effects where the return value is ignored. Results are shown on LGTM by default. |
+| Incomplete URL scheme check (`js/incomplete-url-scheme-check`)            | security, correctness, external/cwe/cwe-020                       | Highlights checks for `javascript:` URLs that do not take `data:` or `vbscript:` URLs into account. 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 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.|
+| 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. |
+| Unreachable method overloads (`js/unreachable-method-overloads`)          | correctness, typescript                                           | Highlights method overloads that are impossible to use from client code. Results are shown on LGTM by default. |
+| 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. |
 | 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. |
 | Useless regular expression character escape (`js/useless-regexp-character-escape`) | correctness, security, external/cwe/cwe-20 | Highlights regular expression strings with useless character escapes, indicating a possible violation of [CWE-20](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default. |
-| Unreachable method overloads (`js/unreachable-method-overloads`)          | correctness, typescript                                           | Highlights method overloads that are impossible to use from client code. Results are shown on LGTM by default. |
-| Ignoring result from pure array method (`js/ignore-array-result`)         | maintainability, correctness                                      | Highlights calls to array methods without side effects where the return value is ignored. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 
@@ -51,6 +57,11 @@
 ## Changes to libraries
 
 * `Expr.getDocumentation()` now handles chain assignments.
+* String literals are now parsed as regular expressions.
+  Consequently, a `RegExpTerm` may occur as part of a string literal or
+  as a regular expression literal. Queries that search for regular expressions may need to
+  use `RegExpTerm.isPartOfRegExpLiteral` or `RegExpTerm.isUsedAsRegExp` to restrict the search.
+  A regular expression AST can be obtained from a string literal using `StringLiteral.asRegExp`.
 
 ## Removal of deprecated queries
 

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

@@ -13,18 +13,13 @@
 
 import cpp
 import PointlessSelfComparison
+import semmle.code.cpp.commons.Exclusions
 
 from ComparisonOperation cmp
 where
   pointlessSelfComparison(cmp) and
   not nanTest(cmp) and
   not overflowTest(cmp) and
   not cmp.isFromTemplateInstantiation(_) and
-  not exists(MacroInvocation mi |
-    // cmp is in mi
-    mi.getAnExpandedElement() = cmp and
-    // and cmp was apparently not passed in as a macro parameter
-    cmp.getLocation().getStartLine() = mi.getLocation().getStartLine() and
-    cmp.getLocation().getStartColumn() = mi.getLocation().getStartColumn()
-  )
+  not isFromMacroDefinition(cmp)
 select cmp, "Self comparison."

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

@@ -1,13 +1,13 @@
 /**
- * @name Undefined result of signed test for overflow
+ * @name Signed overflow check
  * @description Testing for overflow by adding a value to a variable
  *              to see if it "wraps around" works only for
  *              unsigned integer values.
  * @kind problem
  * @problem.severity warning
  * @precision high
  * @id cpp/signed-overflow-check
- * @tags reliability
+ * @tags correctness
  *       security
  */
 

cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow-bad.cpp

@@ -0,0 +1,3 @@
+bool not_in_range(T *ptr, T *ptr_end, size_t a) {
+    return ptr + a >= ptr_end || ptr + a < ptr; // BAD
+}

cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow-good.cpp

@@ -0,0 +1,3 @@
+bool not_in_range(T *ptr, T *ptr_end, size_t a) {
+    return a >= ptr_end - ptr; // GOOD
+}

cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow.qhelp

@@ -0,0 +1,69 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+The expression <code>ptr + a &lt; ptr</code> is equivalent to <code>a &lt;
+0</code>, and an optimizing compiler is likely to make that replacement,
+thereby removing a range check that might have been necessary for security.
+If <code>a</code> is known to be non-negative, the compiler can even replace <code>ptr +
+a &lt; ptr</code> with <code>false</code>.
+</p>
+
+<p>
+The reason is that pointer arithmetic overflow in C/C++ is undefined
+behavior. The optimizing compiler can assume that the program has no
+undefined behavior, which means that adding a positive number to <code>ptr</code> cannot
+produce a pointer less than  <code>ptr</code>.
+</p>
+</overview>
+<recommendation>
+<p>
+To check whether an index <code>a</code> is less than the length of an array, 
+simply compare these two numbers as unsigned integers: <code>a &lt; ARRAY_LENGTH</code>. 
+If the length of the array is defined as the difference between two pointers 
+<code>ptr</code> and <code>p_end</code>, write <code>a &lt; p_end - ptr</code>.
+If a is <code>signed</code>, cast it to <code>unsigned</code> 
+in order to guard against negative <code>a</code>. For example, write 
+<code>(size_t)a &lt; p_end - ptr</code>.
+</p>
+</recommendation>
+<example>
+<p>
+An invalid check for pointer overflow is most often seen as part of checking
+whether a number <code>a</code> is too large by checking first if adding the
+number to <code>ptr</code> goes past the end of an allocation and then
+checking if adding it to <code>ptr</code> creates a pointer so large that it
+overflows and wraps around.
+</p>
+
+<sample src="PointerOverflow-bad.cpp" />
+
+<p>
+In both of these checks, the operations are performed in the wrong order.
+First, an expression that may cause undefined behavior is evaluated
+(<code>ptr + a</code>), and then the result is checked for being in range.
+But once undefined behavior has happened in the pointer addition, it cannot
+be recovered from: it's too late to perform the range check after a possible
+pointer overflow.
+</p>
+
+<p>
+While it's not the subject of this query, the expression <code>ptr + a &lt;
+ptr_end</code> is also an invalid range check. It's undefined behavor in
+C/C++ to create a pointer that points more than one past the end of an
+allocation.
+</p>
+
+<p>
+The next example shows how to portably check whether an unsigned number is outside the
+range of an allocation between <code>ptr</code> and <code>ptr_end</code>.
+</p>
+<sample src="PointerOverflow-good.cpp" />
+</example>
+<references>
+<li>Embedded in Academia: <a href="https://blog.regehr.org/archives/1395">Pointer Overflow Checking</a>.</li>
+<li>LWN: <a href="https://lwn.net/Articles/278137/">GCC and pointer overflows</a>.</li>
+</references>
+</qhelp>

cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow.ql

@@ -0,0 +1,31 @@
+/**
+ * @name Pointer overflow check
+ * @description Adding a value to a pointer to check if it overflows relies
+ *              on undefined behavior and may lead to memory corruption.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id cpp/pointer-overflow-check
+ * @tags reliability
+ *       security
+ */
+
+import cpp
+private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+private import semmle.code.cpp.commons.Exclusions
+
+from RelationalOperation ro, PointerAddExpr add, Expr expr1, Expr expr2
+where
+  ro.getAnOperand() = add and
+  add.getAnOperand() = expr1 and
+  ro.getAnOperand() = expr2 and
+  globalValueNumber(expr1) = globalValueNumber(expr2) and
+  // Exclude macros but not their arguments
+  not isFromMacroDefinition(ro) and
+  // There must be a compilation of this file without a flag that makes pointer
+  // overflow well defined.
+  exists(Compilation c | c.getAFileCompiled() = ro.getFile() |
+    not c.getAnArgument() = "-fwrapv-pointer" and
+    not c.getAnArgument() = "-fno-strict-overflow"
+  )
+select ro, "Range check relying on pointer overflow."

cpp/ql/src/semmle/code/cpp/Declaration.qll

@@ -223,7 +223,7 @@ abstract class Declaration extends Locatable, @declaration {
    * Will have `getTemplateArgument(0)` return `T`, and
    * `getTemplateArgument(1)` return `X`.
    *
-   * `Foo<int, 1> bar;
+   * `Foo<int, 1> bar;`
    *
    * Will have `getTemplateArgument())` return `int`, and
    * `getTemplateArgument(1)` return `1`.