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

change-notes/1.26/analysis-cpp.md

@@ -14,7 +14,10 @@ The following changes in version 1.26 affect C/C++ analysis in all applications.
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
 | Inconsistent direction of for loop (`cpp/inconsistent-loop-direction`) | Fewer false positive results | The query now accounts for intentional wrapping of an unsigned loop counter. |
+| Comparison result is always the same (`cpp/constant-comparison`) | More correct results | Bounds on expressions involving multiplication can now be determined in more cases. |
 
 ## Changes to libraries
 
 * The models library now models more taint flows through `std::string`.
+* The `SimpleRangeAnalysis` library now supports multiplications of the form
+  `e1 * e2` when `e1` and `e2` are unsigned.

cpp/ql/src/Microsoft/SAL.qll

@@ -1,5 +1,13 @@
+/**
+ * Provides classes for identifying and reasoning about Microsoft source code
+ * annotation language (SAL) macros.
+ */
+
 import cpp
 
+/**
+ * A SAL macro defined in `sal.h` or a similar header file.
+ */
 class SALMacro extends Macro {
   SALMacro() {
     exists(string filename | filename = this.getFile().getBaseName() |
@@ -20,27 +28,34 @@ class SALMacro extends Macro {
 }
 
 pragma[noinline]
-predicate isTopLevelMacroAccess(MacroAccess ma) { not exists(ma.getParentInvocation()) }
+private predicate isTopLevelMacroAccess(MacroAccess ma) { not exists(ma.getParentInvocation()) }
 
+/**
+ * An invocation of a SAL macro (excluding invocations inside other macros).
+ */
 class SALAnnotation extends MacroInvocation {
   SALAnnotation() {
     this.getMacro() instanceof SALMacro and
     isTopLevelMacroAccess(this)
   }
 
-  /** Returns the `Declaration` annotated by `this`. */
+  /** Gets the `Declaration` annotated by `this`. */
   Declaration getDeclaration() {
     annotatesAt(this, result.getADeclarationEntry(), _, _) and
     not result instanceof Type // exclude typedefs
   }
 
-  /** Returns the `DeclarationEntry` annotated by `this`. */
+  /** Gets the `DeclarationEntry` annotated by `this`. */
   DeclarationEntry getDeclarationEntry() {
     annotatesAt(this, result, _, _) and
     not result instanceof TypeDeclarationEntry // exclude typedefs
   }
 }
 
+/**
+ * A SAL macro indicating that the return value of a function should always be
+ * checked.
+ */
 class SALCheckReturn extends SALAnnotation {
   SALCheckReturn() {
     exists(SALMacro m | m = this.getMacro() |
@@ -50,6 +65,10 @@ class SALCheckReturn extends SALAnnotation {
   }
 }
 
+/**
+ * A SAL macro indicating that a pointer variable or return value should not be
+ * `NULL`.
+ */
 class SALNotNull extends SALAnnotation {
   SALNotNull() {
     exists(SALMacro m | m = this.getMacro() |
@@ -69,6 +88,9 @@ class SALNotNull extends SALAnnotation {
   }
 }
 
+/**
+ * A SAL macro indicating that a value may be `NULL`.
+ */
 class SALMaybeNull extends SALAnnotation {
   SALMaybeNull() {
     exists(SALMacro m | m = this.getMacro() |
@@ -79,13 +101,29 @@ class SALMaybeNull extends SALAnnotation {
   }
 }
 
+/**
+ * A parameter annotated by one or more SAL annotations.
+ */
+class SALParameter extends Parameter {
+  /** One of this parameter's annotations. */
+  SALAnnotation a;
+
+  SALParameter() { annotatesAt(a, this.getADeclarationEntry(), _, _) }
+
+  predicate isIn() { a.getMacroName().toLowerCase().matches("%\\_in%") }
+
+  predicate isOut() { a.getMacroName().toLowerCase().matches("%\\_out%") }
+
+  predicate isInOut() { a.getMacroName().toLowerCase().matches("%\\_inout%") }
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // Implementation details
 /**
  * Holds if `a` annotates the declaration entry `d` and
  * its start position is the `idx`th position in `file` that holds a SAL element.
  */
-predicate annotatesAt(SALAnnotation a, DeclarationEntry d, File file, int idx) {
+private predicate annotatesAt(SALAnnotation a, DeclarationEntry d, File file, int idx) {
   annotatesAtPosition(a.(SALElement).getStartPosition(), d, file, idx)
 }
 
@@ -109,22 +147,6 @@ private predicate annotatesAtPosition(SALPosition pos, DeclarationEntry d, File
   )
 }
 
-/**
- * A parameter annotated by one or more SAL annotations.
- */
-class SALParameter extends Parameter {
-  /** One of this parameter's annotations. */
-  SALAnnotation a;
-
-  SALParameter() { annotatesAt(a, this.getADeclarationEntry(), _, _) }
-
-  predicate isIn() { a.getMacroName().toLowerCase().matches("%\\_in%") }
-
-  predicate isOut() { a.getMacroName().toLowerCase().matches("%\\_out%") }
-
-  predicate isInOut() { a.getMacroName().toLowerCase().matches("%\\_inout%") }
-}
-
 /**
  * A SAL element, that is, a SAL annotation or a declaration entry
  * that may have SAL annotations.

cpp/ql/src/experimental/Security/CWE/CWE-359/PrivateCleartextWrite.qhelp

@@ -0,0 +1,32 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Private data that is stored in a file or buffer unencrypted is accessible to an attacker who gains access to the
+storage.</p>
+
+</overview>
+<recommendation>
+
+<p>Ensure that private data is always encrypted before being stored.
+It may be wise to encrypt information before it is put into a buffer that may be readable in memory.</p>
+
+<p>In general, decrypt private data only at the point where it is necessary for it to be used in
+cleartext.</p>
+
+</recommendation>
+
+<references>
+
+<li><a href="https://owasp.org/www-project-top-ten/OWASP_Top_Ten_2017/Top_10-2017_A3-Sensitive_Data_Exposure">OWASP Sensitive_Data_Exposure</a></li>
+<li>M. Dowd, J. McDonald and J. Schuhm, <i>The Art of Software Security Assessment</i>, 1st Edition, Chapter 2 - 'Common Vulnerabilities of Encryption', p. 43. Addison Wesley, 2006.</li> 
+<li>M. Howard and D. LeBlanc, <i>Writing Secure Code</i>, 2nd Edition, Chapter 9 - 'Protecting Secret Data', p. 299. Microsoft, 2002.</li>
+
+
+
+<!--  LocalWords:  CWE
+ -->
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-359/PrivateCleartextWrite.ql

@@ -0,0 +1,21 @@
+/**
+ * @name Exposure of private information
+ * @description If private information is written to an external location, it may be accessible by
+ *              unauthorized persons.
+ * @kind path-problem
+ * @problem.severity error
+ * @id cpp/private-cleartext-write
+ * @tags security
+ *       external/cwe/cwe-359
+ */
+
+import cpp
+import experimental.semmle.code.cpp.security.PrivateCleartextWrite
+import experimental.semmle.code.cpp.security.PrivateCleartextWrite::PrivateCleartextWrite
+import DataFlow::PathGraph
+
+from WriteConfig b, DataFlow::PathNode source, DataFlow::PathNode sink
+where b.hasFlowPath(source, sink)
+select sink.getNode(),
+  "This write into the external location '" + sink + "' may contain unencrypted data from $@",
+  source, "this source."

cpp/ql/src/experimental/semmle/code/cpp/security/PrivateCleartextWrite.qll

@@ -0,0 +1,63 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about private information flowing unencrypted to an external location.
+ */
+
+import cpp
+import semmle.code.cpp.dataflow.TaintTracking
+import experimental.semmle.code.cpp.security.PrivateData
+import semmle.code.cpp.security.FileWrite
+import semmle.code.cpp.security.BufferWrite
+import semmle.code.cpp.dataflow.TaintTracking
+
+module PrivateCleartextWrite {
+  /**
+   * A data flow source for private information flowing unencrypted to an external location.
+   */
+  abstract class Source extends DataFlow::ExprNode { }
+
+  /**
+   * A data flow sink for private information flowing unencrypted to an external location.
+   */
+  abstract class Sink extends DataFlow::ExprNode { }
+
+  /**
+   * A sanitizer for private information flowing unencrypted to an external location.
+   */
+  abstract class Sanitizer extends DataFlow::ExprNode { }
+
+  /** A call to any method whose name suggests that it encodes or encrypts the parameter. */
+  class ProtectSanitizer extends Sanitizer {
+    ProtectSanitizer() {
+      exists(Function m, string s |
+        this.getExpr().(FunctionCall).getTarget() = m and
+        m.getName().regexpMatch("(?i).*" + s + ".*")
+      |
+        s = "protect" or s = "encode" or s = "encrypt"
+      )
+    }
+  }
+
+  class WriteConfig extends TaintTracking::Configuration {
+    WriteConfig() { this = "Write configuration" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+    override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+  }
+
+  class PrivateDataSource extends Source {
+    PrivateDataSource() { this.getExpr() instanceof PrivateDataExpr }
+  }
+
+  class WriteSink extends Sink {
+    WriteSink() {
+      exists(FileWrite f, BufferWrite b |
+        this.asExpr() = f.getASource()
+        or
+        this.asExpr() = b.getAChild()
+      )
+    }
+  }
+}

cpp/ql/src/experimental/semmle/code/cpp/security/PrivateData.qll

@@ -0,0 +1,53 @@
+/**
+ * Provides classes and predicates for identifying private data and functions for security.
+ *
+ * 'Private' data in general is anything that would compromise user privacy if exposed. This
+ * library tries to guess where private data may either be stored in a variable or produced by a
+ * function.
+ *
+ * This library is not concerned with credentials. See `SensitiveActions` for expressions related
+ * to credentials.
+ */
+
+import cpp
+
+/** A string for `match` that identifies strings that look like they represent private data. */
+private string privateNames() {
+  // Inspired by the list on https://cwe.mitre.org/data/definitions/359.html
+  // Government identifiers, such as Social Security Numbers
+  result = "%social%security%number%" or
+  // Contact information, such as home addresses and telephone numbers
+  result = "%postcode%" or
+  result = "%zipcode%" or
+  // result = "%telephone%" or
+  // Geographic location - where the user is (or was)
+  result = "%latitude%" or
+  result = "%longitude%" or
+  // Financial data - such as credit card numbers, salary, bank accounts, and debts
+  result = "%creditcard%" or
+  result = "%salary%" or
+  result = "%bankaccount%" or
+  // Communications - e-mail addresses, private e-mail messages, SMS text messages, chat logs, etc.
+  // result = "%email%" or
+  // result = "%mobile%" or
+  result = "%employer%" or
+  // Health - medical conditions, insurance status, prescription records
+  result = "%medical%"
+}
+
+/** An expression that might contain private data. */
+abstract class PrivateDataExpr extends Expr { }
+
+/** A functiond call that might produce private data. */
+class PrivateFunctionCall extends PrivateDataExpr, FunctionCall {
+  PrivateFunctionCall() {
+    exists(string s | this.getTarget().getName().toLowerCase() = s | s.matches(privateNames()))
+  }
+}
+
+/** An access to a variable that might contain private data. */
+class PrivateVariableAccess extends PrivateDataExpr, VariableAccess {
+  PrivateVariableAccess() {
+    exists(string s | this.getTarget().getName().toLowerCase() = s | s.matches(privateNames()))
+  }
+}

cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll

@@ -156,6 +156,10 @@ float safeFloor(float v) {
   result = v
 }
 
+private class UnsignedMulExpr extends MulExpr {
+  UnsignedMulExpr() { this.getType().(IntegralType).isUnsigned() }
+}
+
 /** Set of expressions which we know how to analyze. */
 private predicate analyzableExpr(Expr e) {
   // The type of the expression must be arithmetic. We reuse the logic in
@@ -178,6 +182,8 @@ private predicate analyzableExpr(Expr e) {
     or
     e instanceof SubExpr
     or
+    e instanceof UnsignedMulExpr
+    or
     e instanceof AssignExpr
     or
     e instanceof AssignAddExpr
@@ -278,6 +284,10 @@ private predicate exprDependsOnDef(Expr e, RangeSsaDefinition srcDef, StackVaria
   or
   exists(SubExpr subExpr | e = subExpr | exprDependsOnDef(subExpr.getAnOperand(), srcDef, srcVar))
   or
+  exists(UnsignedMulExpr mulExpr | e = mulExpr |
+    exprDependsOnDef(mulExpr.getAnOperand(), srcDef, srcVar)
+  )
+  or
   exists(AssignExpr addExpr | e = addExpr | exprDependsOnDef(addExpr.getRValue(), srcDef, srcVar))
   or
   exists(AssignAddExpr addExpr | e = addExpr |
@@ -625,6 +635,13 @@ private float getLowerBoundsImpl(Expr expr) {
     result = addRoundingDown(xLow, -yHigh)
   )
   or
+  exists(UnsignedMulExpr mulExpr, float xLow, float yLow |
+    expr = mulExpr and
+    xLow = getFullyConvertedLowerBounds(mulExpr.getLeftOperand()) and
+    yLow = getFullyConvertedLowerBounds(mulExpr.getRightOperand()) and
+    result = xLow * yLow
+  )
+  or
   exists(AssignExpr assign |
     expr = assign and
     result = getFullyConvertedLowerBounds(assign.getRValue())
@@ -794,6 +811,13 @@ private float getUpperBoundsImpl(Expr expr) {
     result = addRoundingUp(xHigh, -yLow)
   )
   or
+  exists(UnsignedMulExpr mulExpr, float xHigh, float yHigh |
+    expr = mulExpr and
+    xHigh = getFullyConvertedUpperBounds(mulExpr.getLeftOperand()) and
+    yHigh = getFullyConvertedUpperBounds(mulExpr.getRightOperand()) and
+    result = xHigh * yHigh
+  )
+  or
   exists(AssignExpr assign |
     expr = assign and
     result = getFullyConvertedUpperBounds(assign.getRValue())

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-359/semmle/tests/PrivateCleartextWrite.expected

@@ -0,0 +1,21 @@
+edges
+| test.cpp:77:16:77:22 | medical | test.cpp:78:24:78:27 | temp |
+| test.cpp:81:17:81:20 | call to func | test.cpp:82:24:82:28 | buff5 |
+| test.cpp:81:22:81:28 | medical | test.cpp:81:17:81:20 | call to func |
+nodes
+| test.cpp:57:9:57:18 | theZipcode | semmle.label | theZipcode |
+| test.cpp:74:24:74:30 | medical | semmle.label | medical |
+| test.cpp:77:16:77:22 | medical | semmle.label | medical |
+| test.cpp:78:24:78:27 | temp | semmle.label | temp |
+| test.cpp:81:17:81:20 | call to func | semmle.label | call to func |
+| test.cpp:81:22:81:28 | medical | semmle.label | medical |
+| test.cpp:82:24:82:28 | buff5 | semmle.label | buff5 |
+| test.cpp:96:37:96:46 | theZipcode | semmle.label | theZipcode |
+| test.cpp:99:42:99:51 | theZipcode | semmle.label | theZipcode |
+#select
+| test.cpp:57:9:57:18 | theZipcode | This write into the external location 'theZipcode' may contain unencrypted data from $@ | test.cpp:57:9:57:18 | theZipcode | this source. |
+| test.cpp:74:24:74:30 | medical | This write into the external location 'medical' may contain unencrypted data from $@ | test.cpp:74:24:74:30 | medical | this source. |
+| test.cpp:78:24:78:27 | temp | This write into the external location 'temp' may contain unencrypted data from $@ | test.cpp:77:16:77:22 | medical | this source. |
+| test.cpp:82:24:82:28 | buff5 | This write into the external location 'buff5' may contain unencrypted data from $@ | test.cpp:81:22:81:28 | medical | this source. |
+| test.cpp:96:37:96:46 | theZipcode | This write into the external location 'theZipcode' may contain unencrypted data from $@ | test.cpp:96:37:96:46 | theZipcode | this source. |
+| test.cpp:99:42:99:51 | theZipcode | This write into the external location 'theZipcode' may contain unencrypted data from $@ | test.cpp:99:42:99:51 | theZipcode | this source. |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-359/semmle/tests/PrivateCleartextWrite.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-359/PrivateCleartextWrite.ql

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-359/semmle/tests/test.cpp

@@ -0,0 +1,105 @@
+#define FILE int
+#define wchar char
+#define size_t int
+typedef int streamsize;
+
+size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
+int fputs(const char *s, FILE *stream);
+int fputc(int c, FILE *stream);
+int fprintf(FILE *stream, const char *format, ...);
+int sprintf(char *s, const char *format, ...);
+size_t strlen(const char *s);
+
+namespace std
+{
+  template <class charT>
+  struct char_traits;
+
+  template <class charT, class traits = char_traits<charT>>
+  class basic_ostream /*: virtual public basic_ios<charT,traits> - not needed for this test */
+  {
+  public:
+    typedef charT char_type;
+    basic_ostream<charT, traits> &write(const char_type *s, streamsize n);
+  };
+
+  template <class charT, class traits = char_traits<charT>>
+  class basic_ofstream : public basic_ostream<charT, traits>
+  {
+  public:
+  };
+
+  template <class charT, class traits>
+  basic_ostream<charT, traits> &operator<<(basic_ostream<charT, traits> &, const charT *);
+
+  typedef basic_ostream<char> ostream;
+  typedef basic_ofstream<char> ofstream;
+}; // namespace std
+
+using namespace std;
+
+char *encrypt(char *buffer)
+{
+  return buffer;
+}
+char *func(char *buffer)
+{
+  return buffer;
+}
+
+// test for CleartextFileWrite
+void file()
+{
+  char *theZipcode = "cleartext zipcode!";
+  FILE *file;
+
+  // BAD: write zipcode to file in cleartext
+  fputs(theZipcode, file);
+
+  // GOOD: encrypt first
+  char *encrypted = encrypt(theZipcode);
+  fwrite(encrypted, sizeof(encrypted), 1, file);
+}
+
+// test for CleartextBufferWrite
+int main(int argc, char **argv)
+{
+  char *medical = "medical";
+  char *buff1;
+  char *buff2;
+  char *buff3;
+  char *buff4;
+
+  // BAD: write medical to buffer in cleartext
+  sprintf(buff1, "%s", medical);
+
+  // BAD: write medical to buffer in cleartext
+  char *temp = medical;
+  sprintf(buff2, "%s", temp);
+
+  // BAD: write medical to buffer in cleartext
+  char *buff5 = func(medical);
+  sprintf(buff3, "%s", buff5);
+
+  char *buff6 = encrypt(medical);
+  // GOOD: encrypt first
+  sprintf(buff4, "%s", buff6);
+}
+
+// test for CleartextFileWrite
+void stream()
+{
+  char *theZipcode = "cleartext zipcode!";
+  ofstream mystream;
+
+  // BAD: write zipcode to file in cleartext
+  mystream << "the zipcode is: " << theZipcode;
+
+  // BAD: write zipcode to file in cleartext
+  (mystream << "the zipcode is: ").write(theZipcode, strlen(theZipcode));
+
+  // GOOD: encrypt first
+  char *encrypted = encrypt(theZipcode);
+  mystream << "the zipcode is: " << encrypted;
+  (mystream << "the zipcode is: ").write(encrypted, strlen(encrypted));
+}

cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/lowerBound.ql

@@ -1,4 +1,4 @@
 import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 
 from VariableAccess expr
-select expr, lowerBound(expr)
+select expr, lowerBound(expr).toString()

cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/test.c

@@ -463,3 +463,30 @@ int test_unsigned_mult02(unsigned b) {
 
   return total;
 }
+
+unsigned long mult_rounding() {
+  unsigned long x, y, xy;
+  x = y = 1000000003UL; // 1e9 + 3
+  xy = x * y;
+  return xy; // BUG: upper bound should be >= 1000000006000000009UL
+}
+
+unsigned long mult_overflow() {
+  unsigned long x, y, xy;
+  x = 274177UL;
+  y = 67280421310721UL;
+  xy = x * y;
+  return xy; // BUG: lower bound should be <= 18446744073709551617UL
+}
+
+unsigned long mult_lower_bound(unsigned int ui, unsigned long ul) {
+  if (ui >= 10) {
+    unsigned long result = (unsigned long)ui * ui;
+    return result; // BUG: upper bound should be >= 18446744065119617025 (possibly a pretty-printing bug)
+  }
+  if (ul >= 10) {
+    unsigned long result = ul * ul;
+    return result; // lower bound is correctly 0 (overflow is possible)
+  }
+  return 0;
+}

cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/upperBound.ql

@@ -1,4 +1,4 @@
 import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 
 from VariableAccess expr
-select expr, upperBound(expr)
+select expr, upperBound(expr).toString()

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.c

@@ -385,10 +385,39 @@ void bitwise_ands()
 
 void unsigned_mult(unsigned int x, unsigned int y) {
   if(x < 13 && y < 35) {
-      if(x * y > 1024) {} // always false [NOT DETECTED]
+      if(x * y > 1024) {} // always false
       if(x * y < 204) {}
       if(x >= 3 && y >= 2) {
-        if(x * y < 5) {} // always false [NOT DETECTED]
+        if(x * y < 5) {} // always false
       }
   }
-}
+}
+
+void mult_rounding() {
+  unsigned long x, y, xy;
+  x = y = 1000000003UL; // 1e9 + 3
+  xy = 1000000006000000009UL; // x * y, precisely
+  // Even though the range analysis wrongly considers x*y to be xy - 9, there
+  // are no PointlessComparison false positives in these tests because alerts
+  // are suppressed when ulp() < 1, which roughly means that the number is
+  // larger than 2^53.
+  if (x * y < xy) {} // always false [NOT DETECTED]
+  if (x * y > xy) {} // always false [NOT DETECTED]
+}
+
+void mult_overflow() {
+  unsigned long x, y;
+  // The following two numbers multiply to 2^64 + 1, which is 1 when truncated
+  // to 64-bit unsigned.
+  x = 274177UL;
+  y = 67280421310721UL;
+  if (x * y == 1) {} // always true [BUG: reported as always false]
+
+  // This bug appears to be caused by
+  // `RangeAnalysisUtils::typeUpperBound(unsigned long)` having a result of
+  // 2**64 + 384, making the range analysis think that the multiplication can't
+  // overflow. The correct `typeUpperBound` would be 2**64 - 1, but we can't
+  // represent that with a QL float or int. We could make `typeUpperBound`
+  // exclusive instead of inclusive, but there is no exclusive upper bound for
+  // floats.
+}

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.expected

@@ -41,6 +41,9 @@
 | PointlessComparison.c:372:6:372:16 | ... >= ... | Comparison is always true because ... >> ... >= 1. |
 | PointlessComparison.c:373:6:373:16 | ... >= ... | Comparison is always false because ... >> ... <= 1. |
 | PointlessComparison.c:383:6:383:17 | ... >= ... | Comparison is always false because ... & ... <= 2. |
+| PointlessComparison.c:388:10:388:21 | ... > ... | Comparison is always false because ... * ... <= 408. |
+| PointlessComparison.c:391:12:391:20 | ... < ... | Comparison is always false because ... * ... >= 6. |
+| PointlessComparison.c:414:7:414:16 | ... == ... | Comparison is always false because ... * ... >= 18446744073709552000. |
 | PointlessComparison.cpp:36:6:36:33 | ... >= ... | Comparison is always false because ... >> ... <= 9223372036854776000. |
 | PointlessComparison.cpp:41:6:41:29 | ... >= ... | Comparison is always false because ... >> ... <= 140737488355327.5. |
 | PointlessComparison.cpp:42:6:42:29 | ... >= ... | Comparison is always false because ... >> ... <= 140737488355327.5. |

java/ql/src/Security/CWE/CWE-020/ExternalAPISinkExample.java

@@ -0,0 +1,8 @@
+public class XSS extends HttpServlet {
+	protected void doGet(HttpServletRequest request, HttpServletResponse response)
+	throws ServletException, IOException {
+		// BAD: a request parameter is written directly to an error response page
+		response.sendError(HttpServletResponse.SC_NOT_FOUND,
+				"The page \"" + request.getParameter("page") + "\" was not found.");
+	}
+}

java/ql/src/Security/CWE/CWE-020/ExternalAPITaintStepExample.java

@@ -0,0 +1,12 @@
+public class SQLInjection extends HttpServlet {
+	protected void doGet(HttpServletRequest request, HttpServletResponse response)
+	throws ServletException, IOException {
+
+		StringBuilder sqlQueryBuilder = new StringBuilder();
+		sqlQueryBuilder.append("SELECT * FROM user WHERE user_id='");
+		sqlQueryBuilder.append(request.getParameter("user_id"));
+		sqlQueryBuilder.append("'");
+
+		// ...
+	}
+}

java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp

@@ -0,0 +1,48 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Using unsanitized untrusted data in an external API can cause a variety of security issues. This query reports 
+all external APIs that are used with untrusted data, along with how frequently the API is used, and how many 
+unique sources of untrusted data flow to this API. This query is designed primarily to help identify which APIs 
+may be relevant for security analysis of this application.</p>
+
+<p>An external API is defined as a method call to a method that is not defined in the source code, not overridden 
+in the source code, and is not modeled as a taint step in the default taint library. External APIs may be from the 
+Java standard library, third party dependencies or from internal dependencies. The query will report the method 
+signature with a fully qualified name, along with either <code>[param x]</code>, where <code>x</code> indicates the 
+position of the parameter receiving the untrusted data or <code>[qualifier]</code> indicating the untrusted data is 
+used as the qualifier to the method call.</p>
+
+</overview>
+<recommendation>
+
+<p>For each result:</p>
+
+<ul>
+  <li>If the result highlights a known sink, no action is required.</li>
+  <li>If the result highlights an unknown sink for a problem, then add modeling for the sink to the relevant query.</li>
+  <li>If the result represents a call to an external API which transfers taint, add the appropriate modeling, and 
+  re-run the query to determine what new results have appeared due to this additional modeling.</li>
+</ul>
+
+<p>Otherwise, the result is likely uninteresting. Custom versions of this query can extend the <code>SafeExternalAPIMethod</code> 
+class to exclude known safe external APIs from future analysis.</p>
+
+</recommendation>
+<example>
+
+<p>If the query were to return the API <code>javax.servlet.http.HttpServletResponse.sendError(int, java.lang.String) [param 1]</code> 
+then we should first consider whether this a security relevant sink. In this case, this is writing to a HTTP response, so we should 
+consider whether this is an XSS sink. If it is, we should confirm that it is handled by the XSS query.</p>
+
+<p>If the query were to return the API <code>java.lang.StringBuilder.append(java.lang.String) [param 0]</code>, then this should be
+reviewed as a possible taint step, because tainted data would flow from the 0th argument to the qualifier of the call.</p>
+
+<p>Note that both examples are correctly handled by the standard taint tracking library and XSS query.</p>
+</example>
+<references>
+
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql

@@ -0,0 +1,18 @@
+/**
+ * @name Frequency counts for external APIs that are used with untrusted data
+ * @description This reports the external APIs that are used with untrusted data, along with how
+ *              frequently the API is called, and how many unique sources of untrusted data flow
+ *              to it.
+ * @id java/count-untrusted-data-external-api
+ * @kind table
+ * @tags security external/cwe/cwe-20
+ */
+
+import java
+import semmle.code.java.security.ExternalAPIs
+import semmle.code.java.dataflow.DataFlow
+
+from ExternalAPIUsedWithUntrustedData externalAPI
+select externalAPI, count(externalAPI.getUntrustedDataNode()) as numberOfUses,
+  externalAPI.getNumberOfUntrustedSources() as numberOfUntrustedSources order by
+    numberOfUntrustedSources desc

java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.qhelp

@@ -0,0 +1,57 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Using unsanitized untrusted data in an external API can cause a variety of security issues. This query reports 
+external APIs that use untrusted data. The results are not filtered so that you can audit all examples. The query provides data for security reviews of the application and you can also use it to identify external APIs that should be modeled as either taint steps, or sinks for specific problems.</p>
+
+<p>An external API is defined as a method call to a method that is not defined in the source code, not overridden 
+in the source code, and is not modeled as a taint step in the default taint library. External APIs may be from the
+Java standard library, third-party dependencies or from internal dependencies. The query reports uses of 
+untrusted data in either the qualifier or as one of the arguments of external APIs.</p>
+
+</overview>
+<recommendation>
+
+<p>For each result:</p>
+
+<ul>
+  <li>If the result highlights a known sink, confirm that the result is reported by the relevant query, or 
+  that the result is a false positive because this data is sanitized.</li>
+  <li>If the result highlights an unknown sink for a problem, then add modeling for the sink to the relevant query, 
+  and confirm that the result is either found, or is safe due to appropriate sanitization.</li>
+  <li>If the result represents a call to an external API that transfers taint, add the appropriate modeling, and 
+  re-run the query to determine what new results have appeared due to this additional modeling.</li>
+</ul>
+
+<p>Otherwise, the result is likely uninteresting. Custom versions of this query can extend the <code>SafeExternalAPIMethod</code> 
+class to exclude known safe external APIs from future analysis.</p>
+
+</recommendation>
+<example>
+
+<p>In this first example, a request parameter is read from <code>HttpServletRequest</code> and then ultimately used in a call to the 
+<code>HttpServletResponse.sendError</code> external API:</p>
+
+<sample src="ExternalAPISinkExample.java" />
+
+<p>This is an XSS sink. The XSS query should therefore be reviewed to confirm that this sink is appropriately modeled, 
+and if it is, to confirm that the query reports this particular result, or that the result is a false positive due to 
+some existing sanitization.</p>
+
+<p>In this second example, again a request parameter is read from <code>HttpServletRequest</code>.</p>
+
+<sample src="ExternalAPITaintStepExample.java" />
+
+<p>If the query reported the call to <code>StringBuilder.append</code> on line 7, this would suggest that this external API is 
+not currently modeled as a taint step in the taint tracking library. The next step would be to model this as a taint step, then
+re-run the query to determine what additional results might be found. In this example, it seems likely that the result of the
+<code>StringBuilder</code> will be executed as an SQL query, potentially leading to an SQL injection vulnerability.</p>
+
+<p>Note that both examples are correctly handled by the standard taint tracking library and XSS query.</p>
+</example>
+<references>
+
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql

@@ -0,0 +1,21 @@
+/**
+ * @name Untrusted data passed to external API
+ * @description Data provided remotely is used in this external API without sanitization, which could be a security risk.
+ * @id java/untrusted-data-to-external-api
+ * @kind path-problem
+ * @precision low
+ * @problem.severity error
+ * @tags security external/cwe/cwe-20
+ */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.security.ExternalAPIs
+import DataFlow::PathGraph
+
+from UntrustedDataToExternalAPIConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink, source, sink,
+  "Call to " + sink.getNode().(ExternalAPIDataNode).getMethodDescription() +
+    " with untrusted data from $@.", source, source.toString()

java/ql/src/experimental/CWE-532/SensitiveInfoLog.ql

@@ -17,7 +17,7 @@ import PathGraph
  */
 private string getACredentialRegex() {
   result = "(?i).*challenge|pass(wd|word|code|phrase)(?!.*question).*" or
-  result = "(?i)(.*username|url).*"
+  result = "(?i)(.*username|.*secret|url).*"
 }
 
 /** Variable keeps sensitive information judging by its name * */
@@ -31,8 +31,12 @@ class CredentialExpr extends Expr {
 class LoggerType extends RefType {
   LoggerType() {
     this.hasQualifiedName("org.apache.log4j", "Category") or //Log4J
+    this.hasQualifiedName("org.apache.logging.log4j", "Logger") or //Log4J 2
     this.hasQualifiedName("org.slf4j", "Logger") or //SLF4j and Gradle Logging
-    this.hasQualifiedName("org.jboss.logging", "BasicLogger") //JBoss Logging
+    this.hasQualifiedName("org.jboss.logging", "BasicLogger") or //JBoss Logging
+    this.hasQualifiedName("org.jboss.logging", "Logger") or //JBoss Logging (`org.jboss.logging.Logger` in some implementations like JBoss Application Server 4.0.4 did not implement `BasicLogger`)
+    this.hasQualifiedName("org.apache.commons.logging", "Log") or //Apache Commons Logging
+    this.hasQualifiedName("org.scijava.log", "Logger") //SciJava Logging
   }
 }
 
@@ -42,7 +46,8 @@ predicate isSensitiveLoggingSink(DataFlow::Node sink) {
     (
       ma.getMethod().hasName("debug") or
       ma.getMethod().hasName("trace") or
-      ma.getMethod().hasName("debugf")
+      ma.getMethod().hasName("debugf") or
+      ma.getMethod().hasName("debugv")
     ) and //Check low priority log levels which are more likely to be real issues to reduce false positives
     sink.asExpr() = ma.getAnArgument()
   )

java/ql/src/semmle/code/java/Expr.qll

@@ -1344,10 +1344,7 @@ class VarAccess extends Expr, @varaccess {
    */
   predicate isLValue() {
     exists(Assignment a | a.getDest() = this) or
-    exists(PreIncExpr e | e.getExpr() = this) or
-    exists(PreDecExpr e | e.getExpr() = this) or
-    exists(PostIncExpr e | e.getExpr() = this) or
-    exists(PostDecExpr e | e.getExpr() = this)
+    exists(UnaryAssignExpr e | e.getExpr() = this)
   }
 
   /**

java/ql/src/semmle/code/java/security/ExternalAPIs.qll

@@ -0,0 +1,146 @@
+/**
+ * Definitions for reasoning about untrusted data used in APIs defined outside the
+ * database.
+ */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.dataflow.TaintTracking
+
+/**
+ * A `Method` that is considered a "safe" external API from a security perspective.
+ */
+abstract class SafeExternalAPIMethod extends Method { }
+
+/** The default set of "safe" external APIs. */
+private class DefaultSafeExternalAPIMethod extends SafeExternalAPIMethod {
+  DefaultSafeExternalAPIMethod() {
+    this instanceof EqualsMethod
+    or
+    getName().regexpMatch("size|length|compareTo|getClass|lastIndexOf")
+    or
+    this.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "Validate")
+    or
+    getQualifiedName() = "Objects.equals"
+    or
+    getDeclaringType() instanceof TypeString and getName() = "equals"
+    or
+    getDeclaringType().hasQualifiedName("com.google.common.base", "Preconditions")
+    or
+    getDeclaringType().getPackage().getName().matches("org.junit%")
+    or
+    getDeclaringType().hasQualifiedName("com.google.common.base", "Strings") and
+    getName() = "isNullOrEmpty"
+    or
+    getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "StringUtils") and
+    getName() = "isNotEmpty"
+    or
+    getDeclaringType().hasQualifiedName("java.lang", "Character") and
+    getName() = "isDigit"
+    or
+    getDeclaringType().hasQualifiedName("java.lang", "String") and
+    getName().regexpMatch("equalsIgnoreCase|regionMatches")
+    or
+    getDeclaringType().hasQualifiedName("java.lang", "Boolean") and
+    getName() = "parseBoolean"
+    or
+    getDeclaringType().hasQualifiedName("org.apache.commons.io", "IOUtils") and
+    getName() = "closeQuietly"
+    or
+    getDeclaringType().hasQualifiedName("org.springframework.util", "StringUtils") and
+    getName().regexpMatch("hasText|isEmpty")
+  }
+}
+
+/** A node representing data being passed to an external API. */
+class ExternalAPIDataNode extends DataFlow::Node {
+  Call call;
+  int i;
+
+  ExternalAPIDataNode() {
+    (
+      // Argument to call to a method
+      this.asExpr() = call.getArgument(i)
+      or
+      // Qualifier to call to a method which returns non trivial value
+      this.asExpr() = call.getQualifier() and
+      i = -1 and
+      not call.getCallee().getReturnType() instanceof VoidType and
+      not call.getCallee().getReturnType() instanceof BooleanType
+    ) and
+    // Defined outside the source archive
+    not call.getCallee().fromSource() and
+    // Not a call to an method which is overridden in source
+    not exists(Method m |
+      m.getASourceOverriddenMethod() = call.getCallee().getSourceDeclaration() and
+      m.fromSource()
+    ) and
+    // Not already modeled as a taint step
+    not exists(DataFlow::Node next | TaintTracking::localTaintStep(this, next)) and
+    // Not a call to a known safe external API
+    not call.getCallee() instanceof SafeExternalAPIMethod
+  }
+
+  /** Gets the called API `Method`. */
+  Method getMethod() { result = call.getCallee() }
+
+  /** Gets the index which is passed untrusted data (where -1 indicates the qualifier). */
+  int getIndex() { result = i }
+
+  /** Gets the description of the method being called. */
+  string getMethodDescription() {
+    result = getMethod().getDeclaringType().getPackage() + "." + getMethod().getQualifiedName()
+  }
+}
+
+/** A configuration for tracking flow from `RemoteFlowSource`s to `ExternalAPIDataNode`s. */
+class UntrustedDataToExternalAPIConfig extends TaintTracking::Configuration {
+  UntrustedDataToExternalAPIConfig() { this = "UntrustedDataToExternalAPIConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof ExternalAPIDataNode }
+}
+
+/** A node representing untrusted data being passed to an external API. */
+class UntrustedExternalAPIDataNode extends ExternalAPIDataNode {
+  UntrustedExternalAPIDataNode() { any(UntrustedDataToExternalAPIConfig c).hasFlow(_, this) }
+
+  /** Gets a source of untrusted data which is passed to this external API data node. */
+  DataFlow::Node getAnUntrustedSource() {
+    any(UntrustedDataToExternalAPIConfig c).hasFlow(result, this)
+  }
+}
+
+private newtype TExternalAPI =
+  TExternalAPIParameter(Method m, int index) {
+    exists(UntrustedExternalAPIDataNode n |
+      m = n.getMethod() and
+      index = n.getIndex()
+    )
+  }
+
+/** An external API which is used with untrusted data. */
+class ExternalAPIUsedWithUntrustedData extends TExternalAPI {
+  /** Gets a possibly untrusted use of this external API. */
+  UntrustedExternalAPIDataNode getUntrustedDataNode() {
+    this = TExternalAPIParameter(result.getMethod(), result.getIndex())
+  }
+
+  /** Gets the number of untrusted sources used with this external API. */
+  int getNumberOfUntrustedSources() {
+    result = count(getUntrustedDataNode().getAnUntrustedSource())
+  }
+
+  /** Gets a textual representation of this element. */
+  string toString() {
+    exists(Method m, int index, string indexString |
+      if index = -1 then indexString = "qualifier" else indexString = "param " + index
+    |
+      this = TExternalAPIParameter(m, index) and
+      result =
+        m.getDeclaringType().(RefType).getQualifiedName() + "." + m.getSignature() + " [" +
+          indexString + "]"
+    )
+  }
+}

javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java

@@ -210,6 +210,7 @@ public class AutoBuild {
   private final String defaultEncoding;
   private ExecutorService threadPool;
   private volatile boolean seenCode = false;
+  private volatile boolean seenFiles = false;
   private boolean installDependencies = false;
   private int installDependenciesTimeout;
   private final VirtualSourceRoot virtualSourceRoot;
@@ -472,7 +473,11 @@ public class AutoBuild {
       shutdownThreadPool();
     }
     if (!seenCode) {
-      warn("No JavaScript or TypeScript code found.");
+      if (seenFiles) {
+        warn("Only found JavaScript or TypeScript files that were empty or contained syntax errors.");
+      } else {
+        warn("No JavaScript or TypeScript code found.");
+      }
       return -1;
     }
     return 0;
@@ -1201,6 +1206,7 @@ protected DependencyInstallationResult preparePackagesAndDependencies(Set<Path>
       long start = logBeginProcess("Extracting " + file);
       Integer loc = extractor.extract(f, state);
       if (!extractor.getConfig().isExterns() && (loc == null || loc != 0)) seenCode = true;
+      if (!extractor.getConfig().isExterns()) seenFiles = true;
       logEndProcess(start, "Done extracting " + file);
     } catch (Throwable t) {
       System.err.println("Exception while extracting " + file + ".");

javascript/ql/src/Declarations/ConflictingFunctions.ql

@@ -24,5 +24,6 @@ where
   // ignore ambient, abstract, and overloaded declarations in TypeScript
   f.hasBody() and
   g.hasBody()
-select f.getId(), "Declaration of " + f.describe() + " conflicts with $@ in the same scope.",
-  g.getId(), "another declaration"
+select f.getIdentifier(),
+  "Declaration of " + f.describe() + " conflicts with $@ in the same scope.", g.getIdentifier(),
+  "another declaration"

javascript/ql/src/Declarations/DeadStoreOfLocal.ql

@@ -42,11 +42,11 @@ where
     access.isAmbient()
   ) and
   // don't flag function expressions
-  not exists(FunctionExpr fe | dead = fe.getId()) and
+  not exists(FunctionExpr fe | dead = fe.getIdentifier()) and
   // don't flag function declarations nested inside blocks or other compound statements;
   // their meaning is only partially specified by the standard
   not exists(FunctionDeclStmt fd, StmtContainer outer |
-    dead = fd.getId() and outer = fd.getEnclosingContainer()
+    dead = fd.getIdentifier() and outer = fd.getEnclosingContainer()
   |
     not fd = outer.getBody().(BlockStmt).getAStmt()
   ) and

javascript/ql/src/Declarations/RedeclaredVariable.ql

@@ -22,6 +22,6 @@ where
   not decl.isAmbient() and
   not redecl.isAmbient() and
   // Redeclaring a namespace extends the previous definition.
-  not decl = any(NamespaceDeclaration ns).getId() and
-  not redecl = any(NamespaceDeclaration ns).getId()
+  not decl = any(NamespaceDeclaration ns).getIdentifier() and
+  not redecl = any(NamespaceDeclaration ns).getIdentifier()
 select redecl, "This variable has already been declared $@.", decl, "here"

javascript/ql/src/Declarations/UnusedVariable.ql

@@ -93,7 +93,7 @@ predicate isEnumMember(VarDecl decl) { decl = any(EnumMember member).getIdentifi
  * "function f", "variable v" or "class c".
  */
 string describeVarDecl(VarDecl vd) {
-  if vd = any(Function f).getId()
+  if vd = any(Function f).getIdentifier()
   then result = "function " + vd.getName()
   else
     if vd = any(ClassDefinition c).getIdentifier()
@@ -115,7 +115,7 @@ class ImportVarDeclProvider extends Stmt {
    */
   VarDecl getAVarDecl() {
     result = this.(ImportDeclaration).getASpecifier().getLocal() or
-    result = this.(ImportEqualsDeclaration).getId()
+    result = this.(ImportEqualsDeclaration).getIdentifier()
   }
 
   /**

javascript/ql/src/Expressions/SuspiciousPropAccess.ql

@@ -23,7 +23,7 @@ private import semmle.javascript.dataflow.InferredTypes
  * if it is part of a const enum access, so we conservatively silence the alert in that case.
  */
 predicate namespaceOrConstEnumAccess(VarAccess e) {
-  exists(NamespaceDeclaration decl | e.getVariable().getADeclaration() = decl.getId())
+  exists(NamespaceDeclaration decl | e.getVariable().getADeclaration() = decl.getIdentifier())
   or
   exists(EnumDeclaration decl | e.getVariable().getADeclaration() = decl.getIdentifier() |
     decl.isConst()

javascript/ql/src/semmle/javascript/DefUse.qll

@@ -30,15 +30,17 @@ private predicate defn(ControlFlowNode def, Expr lhs, AST::ValueNode rhs) {
   or
   exists(VariableDeclarator vd | def = vd | lhs = vd.getBindingPattern() and rhs = vd.getInit())
   or
-  exists(Function f | def = f.getId() | lhs = def and rhs = f)
+  exists(Function f | def = f.getIdentifier() | lhs = def and rhs = f)
   or
   exists(ClassDefinition c | lhs = c.getIdentifier() | def = c and rhs = c and not c.isAmbient())
   or
-  exists(NamespaceDeclaration n | def = n | lhs = n.getId() and rhs = n)
+  exists(NamespaceDeclaration n | def = n | lhs = n.getIdentifier() and rhs = n)
   or
   exists(EnumDeclaration ed | def = ed.getIdentifier() | lhs = def and rhs = ed)
   or
-  exists(ImportEqualsDeclaration i | def = i | lhs = i.getId() and rhs = i.getImportedEntity())
+  exists(ImportEqualsDeclaration i | def = i |
+    lhs = i.getIdentifier() and rhs = i.getImportedEntity()
+  )
   or
   exists(ImportSpecifier i | def = i | lhs = i.getLocal() and rhs = i)
   or
@@ -149,7 +151,7 @@ class RValue extends RefExpr {
     or
     this = any(UpdateExpr u).getOperand().getUnderlyingReference()
     or
-    this = any(NamespaceDeclaration decl).getId()
+    this = any(NamespaceDeclaration decl).getIdentifier()
   }
 }
 

javascript/ql/src/semmle/javascript/ES2015Modules.qll

@@ -330,7 +330,7 @@ class ExportDefaultDeclaration extends ExportDeclaration, @exportdefaultdeclarat
   /** Gets the declaration, if any, exported by this default export. */
   VarDecl getADecl() {
     exists(ExprOrStmt op | op = getOperand() |
-      result = op.(FunctionDeclStmt).getId() or
+      result = op.(FunctionDeclStmt).getIdentifier() or
       result = op.(ClassDeclStmt).getIdentifier()
     )
   }
@@ -364,13 +364,13 @@ class ExportNamedDeclaration extends ExportDeclaration, @exportnameddeclaration
   Identifier getAnExportedDecl() {
     exists(ExprOrStmt op | op = getOperand() |
       result = op.(DeclStmt).getADecl().getBindingPattern().getABindingVarRef() or
-      result = op.(FunctionDeclStmt).getId() or
+      result = op.(FunctionDeclStmt).getIdentifier() or
       result = op.(ClassDeclStmt).getIdentifier() or
-      result = op.(NamespaceDeclaration).getId() or
+      result = op.(NamespaceDeclaration).getIdentifier() or
       result = op.(EnumDeclaration).getIdentifier() or
       result = op.(InterfaceDeclaration).getIdentifier() or
       result = op.(TypeAliasDeclaration).getIdentifier() or
-      result = op.(ImportEqualsDeclaration).getId()
+      result = op.(ImportEqualsDeclaration).getIdentifier()
     )
   }
 

javascript/ql/src/semmle/javascript/Functions.qll

@@ -77,8 +77,15 @@ class Function extends @function, Parameterized, TypeParameterized, StmtContaine
     result = getDocumentation().getATagByTitle("this").getType()
   }
 
+  /**
+   * DEPRECATED: Use `getIdentifier()` instead.
+   *
+   * Gets the identifier specifying the name of this function, if any.
+   */
+  deprecated VarDecl getId() { result = getIdentifier() }
+
   /** Gets the identifier specifying the name of this function, if any. */
-  VarDecl getId() { result = getChildExpr(-1) }
+  VarDecl getIdentifier() { result = getChildExpr(-1) }
 
   /**
    * Gets the name of this function if it has one, or a name inferred from its context.
@@ -89,9 +96,9 @@ class Function extends @function, Parameterized, TypeParameterized, StmtContaine
    * can be inferred, there is no result.
    */
   string getName() {
-    result = getId().getName()
+    result = getIdentifier().getName()
     or
-    not exists(getId()) and
+    not exists(getIdentifier()) and
     (
       exists(VarDef vd | this = vd.getSource() | result = vd.getTarget().(VarRef).getName())
       or
@@ -111,7 +118,7 @@ class Function extends @function, Parameterized, TypeParameterized, StmtContaine
   }
 
   /** Gets the variable holding this function. */
-  Variable getVariable() { result = getId().getVariable() }
+  Variable getVariable() { result = getIdentifier().getVariable() }
 
   /** Gets the `arguments` variable of this function, if any. */
   ArgumentsVariable getArgumentsVariable() { result.getFunction() = this }
@@ -183,11 +190,11 @@ class Function extends @function, Parameterized, TypeParameterized, StmtContaine
     not exists(getAParameter()) and
     (
       // if the function has a name, the opening parenthesis comes right after it
-      result = getId().getLastToken().getNextToken()
+      result = getIdentifier().getLastToken().getNextToken()
       or
       // otherwise this must be an arrow function with no parameters, so the opening
       // parenthesis is the very first token of the function
-      not exists(getId()) and result = getFirstToken()
+      not exists(getIdentifier()) and result = getFirstToken()
     )
   }
 
@@ -309,8 +316,8 @@ class Function extends @function, Parameterized, TypeParameterized, StmtContaine
    */
   private string inferNameFromVarDef() {
     // in ambiguous cases like `var f = function g() {}`, prefer `g` to `f`
-    if exists(getId())
-    then result = "function " + getId().getName()
+    if exists(getIdentifier())
+    then result = "function " + getIdentifier().getName()
     else
       exists(VarDef vd | this = vd.getSource() |
         result = "function " + vd.getTarget().(VarRef).getName()

javascript/ql/src/semmle/javascript/GlobalAccessPaths.qll

@@ -267,7 +267,7 @@ module AccessPath {
     or
     exists(FunctionDeclStmt fun |
       node = DataFlow::valueNode(fun) and
-      result = fun.getId().(GlobalVarDecl).getName() and
+      result = fun.getIdentifier().(GlobalVarDecl).getName() and
       root.isGlobal()
     )
     or
@@ -285,7 +285,7 @@ module AccessPath {
     or
     exists(NamespaceDeclaration decl |
       node = DataFlow::valueNode(decl) and
-      result = decl.getId().(GlobalVarDecl).getName() and
+      result = decl.getIdentifier().(GlobalVarDecl).getName() and
       root.isGlobal()
     )
   }

javascript/ql/src/semmle/javascript/TypeScript.qll

@@ -8,13 +8,16 @@ import javascript
  */
 class NamespaceDefinition extends Stmt, @namespacedefinition, AST::ValueNode {
   /**
+   * DEPRECATED: Use `getIdentifier()` instead.
+   *
    * Gets the identifier naming the namespace.
    */
-  Identifier getId() {
-    result = this.(NamespaceDeclaration).getId()
-    or
-    result = this.(EnumDeclaration).getIdentifier()
-  }
+  deprecated Identifier getId() { result = getIdentifier() }
+
+  /**
+   * Gets the identifier naming the namespace.
+   */
+  Identifier getIdentifier() { none() } // Overridden in subtypes.
 
   /**
    * Gets unqualified name of the namespace being defined.
@@ -29,7 +32,7 @@ class NamespaceDefinition extends Stmt, @namespacedefinition, AST::ValueNode {
    * Gets the local namespace name induced by this namespace.
    */
   LocalNamespaceName getLocalNamespaceName() {
-    result = getId().(LocalNamespaceDecl).getLocalNamespaceName()
+    result = getIdentifier().(LocalNamespaceDecl).getLocalNamespaceName()
   }
 
   /**
@@ -55,10 +58,10 @@ class NamespaceDefinition extends Stmt, @namespacedefinition, AST::ValueNode {
  */
 class NamespaceDeclaration extends NamespaceDefinition, StmtContainer, @namespacedeclaration {
   /** Gets the name of this namespace. */
-  override Identifier getId() { result = getChildExpr(-1) }
+  override Identifier getIdentifier() { result = getChildExpr(-1) }
 
   /** Gets the name of this namespace as a string. */
-  override string getName() { result = getId().getName() }
+  override string getName() { result = getIdentifier().getName() }
 
   /** Gets the `i`th statement in this namespace. */
   Stmt getStmt(int i) {
@@ -83,7 +86,7 @@ class NamespaceDeclaration extends NamespaceDefinition, StmtContainer, @namespac
   predicate isInstantiated() { isInstantiated(this) }
 
   override ControlFlowNode getFirstControlFlowNode() {
-    if hasDeclareKeyword(this) then result = this else result = getId()
+    if hasDeclareKeyword(this) then result = this else result = getIdentifier()
   }
 }
 
@@ -178,13 +181,20 @@ class GlobalAugmentationDeclaration extends Stmt, StmtContainer, @globalaugmenta
 
 /** A TypeScript "import-equals" declaration. */
 class ImportEqualsDeclaration extends Stmt, @importequalsdeclaration {
+  /**
+   * DEPRECATED: Use `getIdentifier()` instead.
+   *
+   * Gets the name under which the imported entity is imported.
+   */
+  deprecated Identifier getId() { result = getIdentifier() }
+
   /** Gets the name under which the imported entity is imported. */
-  Identifier getId() { result = getChildExpr(0) }
+  Identifier getIdentifier() { result = getChildExpr(0) }
 
   /** Gets the expression specifying the imported module or entity. */
   Expr getImportedEntity() { result = getChildExpr(1) }
 
-  override ControlFlowNode getFirstControlFlowNode() { result = getId() }
+  override ControlFlowNode getFirstControlFlowNode() { result = getIdentifier() }
 }
 
 /**
@@ -348,7 +358,7 @@ class TypeDecl extends Identifier, TypeRef, LexicalDecl {
     this = any(ClassOrInterface ci).getIdentifier() or
     this = any(TypeParameter tp).getIdentifier() or
     this = any(ImportSpecifier im).getLocal() or
-    this = any(ImportEqualsDeclaration im).getId() or
+    this = any(ImportEqualsDeclaration im).getIdentifier() or
     this = any(TypeAliasDeclaration td).getIdentifier() or
     this = any(EnumDeclaration ed).getIdentifier() or
     this = any(EnumMember member).getIdentifier()
@@ -1226,8 +1236,8 @@ abstract class NamespaceRef extends ASTNode { }
  */
 class LocalNamespaceDecl extends VarDecl, NamespaceRef {
   LocalNamespaceDecl() {
-    any(NamespaceDeclaration nd).getId() = this or
-    any(ImportEqualsDeclaration im).getId() = this or
+    any(NamespaceDeclaration nd).getIdentifier() = this or
+    any(ImportEqualsDeclaration im).getIdentifier() = this or
     any(ImportSpecifier im).getLocal() = this or
     any(EnumDeclaration ed).getIdentifier() = this
   }
@@ -1325,7 +1335,7 @@ class ImportVarTypeAccess extends VarTypeAccess, ImportTypeExpr, @importvartypea
  */
 class EnumDeclaration extends NamespaceDefinition, @enumdeclaration, AST::ValueNode {
   /** Gets the name of this enum, such as `E` in `enum E { A, B }`. */
-  Identifier getIdentifier() { result = getChildExpr(0) }
+  override Identifier getIdentifier() { result = getChildExpr(0) }
 
   /** Gets the name of this enum as a string. */
   override string getName() { result = getIdentifier().getName() }

javascript/ql/src/semmle/javascript/Variables.qll

@@ -312,8 +312,11 @@ class LocalVariable extends Variable {
     this = result.getScope().getAVariable()
     or
     exists(VarDecl d | d = getADeclaration() |
-      if d = any(FunctionDeclStmt fds).getId()
-      then exists(FunctionDeclStmt fds | d = fds.getId() | result = fds.getEnclosingContainer())
+      if d = any(FunctionDeclStmt fds).getIdentifier()
+      then
+        exists(FunctionDeclStmt fds | d = fds.getIdentifier() |
+          result = fds.getEnclosingContainer()
+        )
       else result = d.getContainer()
     )
   }

javascript/ql/src/semmle/javascript/dataflow/internal/BasicExprTypeInference.qll

@@ -111,7 +111,7 @@ private class AnalyzedNamespaceDeclaration extends DataFlow::AnalyzedValueNode {
 
   AbstractValue getPreviousValue() {
     exists(AnalyzedSsaDefinition def |
-      def.getVariable().getAUse() = astNode.getId() and
+      def.getVariable().getAUse() = astNode.getIdentifier() and
       result = def.getAnRhsValue()
     )
   }

javascript/ql/src/semmle/javascript/frameworks/CryptoLibraries.qll

@@ -1,5 +1,5 @@
 /**
- * Provides classes for modelling cryptographic libraries.
+ * Provides classes for modeling cryptographic libraries.
  */
 
 import javascript

javascript/ql/test/library-tests/Functions/getId.qll

@@ -1,5 +1,5 @@
 import javascript
 
 query predicate test_getId(Function f, VarDecl res0, string res1) {
-  res0 = f.getId() and res1 = f.getName()
+  res0 = f.getIdentifier() and res1 = f.getName()
 }

javascript/ql/test/library-tests/TypeScript/Namespaces/CheckBindings.ql

@@ -10,7 +10,7 @@ class ResolveCall extends CallExpr {
   string getDeclaredValue() {
     result = getVariable().getAnAssignedExpr().getStringValue()
     or
-    exists(NamespaceDeclaration decl | decl.getId() = getVariable().getADeclaration() |
+    exists(NamespaceDeclaration decl | decl.getIdentifier() = getVariable().getADeclaration() |
       result = getNamespaceName(decl)
     )
   }
@@ -21,7 +21,8 @@ string getNamespaceName(NamespaceDeclaration decl) {
   or
   not decl.getStmt(0).(ExprStmt).getExpr() instanceof ConstantString and
   result =
-    "Namespace " + decl.getId() + " on line " + decl.getFirstToken().getLocation().getStartLine()
+    "Namespace " + decl.getIdentifier() + " on line " +
+      decl.getFirstToken().getLocation().getStartLine()
 }
 
 from ResolveCall resolve