Merge branch 'master' into overflowcalc

Author: jbj

.github/workflows/labeler.yml

@@ -27,4 +27,3 @@
 ## Changes to libraries
 
 * The predicates `RegExpTerm.getSuccessor` and `RegExpTerm.getPredecessor` have been changed to reflect textual, not operational, matching order. This only makes a difference in lookbehind assertions, which are operationally matched backwards. Previously, `getSuccessor` would mimick this, so in an assertion `(?<=ab)` the term `b` would be considered the predecessor, not the successor, of `a`. Textually, however, `a` is still matched before `b`, and this is the order we now follow.
-

change-notes/1.24/analysis-javascript.md

@@ -0,0 +1,7 @@
+[[ condition: enterprise-only ]]
+
+# Improvements to JavaScript analysis
+
+## Changes to code extraction
+
+* `import.meta` expressions no longer result in a syntax error in JavaScript files.

change-notes/1.24/extractor-javascript.md

@@ -265,5 +265,11 @@
   "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"
+  ],
+  "XML": [
+    "cpp/ql/src/semmle/code/cpp/XML.qll",
+    "java/ql/src/semmle/code/xml/XML.qll",
+    "javascript/ql/src/semmle/javascript/XML.qll",
+    "python/ql/src/semmle/python/xml/XML.qll"
   ]
 }

config/identical-files.json

@@ -10,12 +10,25 @@ import cpp
 /**
  * An alert suppression comment.
  */
-class SuppressionComment extends CppStyleComment {
+class SuppressionComment extends Comment {
   string annotation;
   string text;
 
   SuppressionComment() {
-    text = getContents().suffix(2) and
+    (
+      this instanceof CppStyleComment and
+      // strip the beginning slashes
+      text = getContents().suffix(2)
+      or
+      this instanceof CStyleComment and
+      // strip both the beginning /* and the end */ the comment
+      exists(string text0 |
+        text0 = getContents().suffix(2) and
+        text = text0.prefix(text0.length() - 2)
+      ) and
+      // The /* */ comment must be a single-line comment
+      not text.matches("%\n%")
+    ) and
     (
       // match `lgtm[...]` anywhere in the comment
       annotation = text.regexpFind("(?i)\\blgtm\\s*\\[[^\\]]*\\]", _, _)

cpp/ql/src/AlertSuppression.ql

@@ -157,4 +157,6 @@ library class SpecialNameQualifyingElement extends NameQualifyingElement,
   @specialnamequalifyingelement {
   /** Gets the name of this special qualifying element. */
   override string getName() { specialnamequalifyingelements(underlyingElement(this), result) }
+
+  override string toString() { result = getName() }
 }

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

@@ -214,7 +214,9 @@ class PreprocessorUndef extends PreprocessorDirective, @ppd_undef {
  * A C/C++ preprocessor `#pragma` directive.
  */
 class PreprocessorPragma extends PreprocessorDirective, @ppd_pragma {
-  override string toString() { result = "#pragma " + this.getHead() }
+  override string toString() {
+    if exists(this.getHead()) then result = "#pragma " + this.getHead() else result = "#pragma"
+  }
 }
 
 /**

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

@@ -2,27 +2,30 @@
  * Provides classes and predicates for working with XML files and their content.
  */
 
-import semmle.code.cpp.Location
+import semmle.files.FileSystem
 
 /** An XML element that has a location. */
 abstract class XMLLocatable extends @xmllocatable {
   /** Gets the source location for this element. */
   Location getLocation() { xmllocations(this, result) }
 
   /**
-   * Holds if this element has the specified location information,
-   * including file path, start line, start column, end line and end column.
+   * Holds if this element is at the specified location.
+   * The location spans column `startcolumn` of line `startline` to
+   * column `endcolumn` of line `endline` in file `filepath`.
+   * For more information, see
+   * [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
    */
   predicate hasLocationInfo(
     string filepath, int startline, int startcolumn, int endline, int endcolumn
   ) {
     exists(File f, Location l | l = this.getLocation() |
-      locations_default(l, unresolveElement(f), startline, startcolumn, endline, endcolumn) and
+      locations_default(l, f, startline, startcolumn, endline, endcolumn) and
       filepath = f.getAbsolutePath()
     )
   }
 
-  /** Gets a printable representation of this element. */
+  /** Gets a textual representation of this element. */
   abstract string toString();
 }
 
@@ -31,6 +34,12 @@ abstract class XMLLocatable extends @xmllocatable {
  * both of which can contain other elements.
  */
 class XMLParent extends @xmlparent {
+  XMLParent() {
+    // explicitly restrict `this` to be either an `XMLElement` or an `XMLFile`;
+    // the type `@xmlparent` currently also includes non-XML files
+    this instanceof @xmlelement or xmlEncoding(this, _)
+  }
+
   /**
    * Gets a printable representation of this XML parent.
    * (Intended to be overridden in subclasses.)
@@ -106,15 +115,21 @@ class XMLFile extends XMLParent, File {
   override string toString() { result = XMLParent.super.toString() }
 
   /** Gets the name of this XML file. */
-  override string getName() { files(this, result, _, _, _) }
+  override string getName() { result = File.super.getAbsolutePath() }
 
-  /** Gets the path of this XML file. */
-  string getPath() { files(this, _, result, _, _) }
+  /**
+   * DEPRECATED: Use `getAbsolutePath()` instead.
+   *
+   * Gets the path of this XML file.
+   */
+  deprecated string getPath() { result = getAbsolutePath() }
 
-  /** Gets the path of the folder that contains this XML file. */
-  string getFolder() {
-    result = this.getPath().substring(0, this.getPath().length() - this.getName().length())
-  }
+  /**
+   * DEPRECATED: Use `getParentContainer().getAbsolutePath()` instead.
+   *
+   * Gets the path of the folder that contains this XML file.
+   */
+  deprecated string getFolder() { result = getParentContainer().getAbsolutePath() }
 
   /** Gets the encoding of this XML file. */
   string getEncoding() { xmlEncoding(this, result) }
@@ -129,7 +144,17 @@ class XMLFile extends XMLParent, File {
   XMLDTD getADTD() { xmlDTDs(result, _, _, _, this) }
 }
 
-/** A "Document Type Definition" of an XML file. */
+/**
+ * An XML document type definition (DTD).
+ *
+ * Example:
+ *
+ * ```
+ * <!ELEMENT person (firstName, lastName?)>
+ * <!ELEMENT firstName (#PCDATA)>
+ * <!ELEMENT lastName (#PCDATA)>
+ * ```
+ */
 class XMLDTD extends @xmldtd {
   /** Gets the name of the root element of this DTD. */
   string getRoot() { xmlDTDs(this, result, _, _, _) }
@@ -156,7 +181,17 @@ class XMLDTD extends @xmldtd {
   }
 }
 
-/** An XML tag in an XML file. */
+/**
+ * An XML element in an XML file.
+ *
+ * Example:
+ *
+ * ```
+ * <manifest xmlns:android="http://schemas.android.com/apk/res/android"
+ *           package="com.example.exampleapp" android:versionCode="1">
+ * </manifest>
+ * ```
+ */
 class XMLElement extends @xmlelement, XMLParent, XMLLocatable {
   /** Holds if this XML element has the given `name`. */
   predicate hasName(string name) { name = getName() }
@@ -201,7 +236,16 @@ class XMLElement extends @xmlelement, XMLParent, XMLLocatable {
   override string toString() { result = XMLParent.super.toString() }
 }
 
-/** An attribute that occurs inside an XML element. */
+/**
+ * An attribute that occurs inside an XML element.
+ *
+ * Examples:
+ *
+ * ```
+ * package="com.example.exampleapp"
+ * android:versionCode="1"
+ * ```
+ */
 class XMLAttribute extends @xmlattribute, XMLLocatable {
   /** Gets the name of this attribute. */
   string getName() { xmlAttrs(this, _, result, _, _, _) }
@@ -222,7 +266,15 @@ class XMLAttribute extends @xmlattribute, XMLLocatable {
   override string toString() { result = this.getName() + "=" + this.getValue() }
 }
 
-/** A namespace used in an XML file */
+/**
+ * A namespace used in an XML file.
+ *
+ * Example:
+ *
+ * ```
+ * xmlns:android="http://schemas.android.com/apk/res/android"
+ * ```
+ */
 class XMLNamespace extends @xmlnamespace {
   /** Gets the prefix of this namespace. */
   string getPrefix() { xmlNs(this, result, _, _) }
@@ -241,7 +293,15 @@ class XMLNamespace extends @xmlnamespace {
   }
 }
 
-/** A comment of the form `<!-- ... -->` is an XML comment. */
+/**
+ * A comment in an XML file.
+ *
+ * Example:
+ *
+ * ```
+ * <!-- This is a comment. -->
+ * ```
+ */
 class XMLComment extends @xmlcomment, XMLLocatable {
   /** Gets the text content of this XML comment. */
   string getText() { xmlComments(this, result, _, _) }
@@ -256,6 +316,12 @@ class XMLComment extends @xmlcomment, XMLLocatable {
 /**
  * A sequence of characters that occurs between opening and
  * closing tags of an XML element, excluding other elements.
+ *
+ * Example:
+ *
+ * ```
+ * <content>This is a sequence of characters.</content>
+ * ```
  */
 class XMLCharacters extends @xmlcharacters, XMLLocatable {
   /** Gets the content of this character sequence. */

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

@@ -19,10 +19,32 @@ private predicate predictableInstruction(Instruction instr) {
   predictableInstruction(instr.(UnaryInstruction).getUnary())
 }
 
+private predicate userInputInstruction(Instruction instr) {
+  exists(CallInstruction ci, WriteSideEffectInstruction wsei |
+    userInputArgument(ci.getConvertedResultExpression(), wsei.getIndex()) and
+    instr = wsei and
+    wsei.getPrimaryInstruction() = ci
+  )
+  or
+  userInputReturned(instr.getConvertedResultExpression())
+  or
+  instr.getConvertedResultExpression() instanceof EnvironmentRead
+  or
+  instr
+      .(LoadInstruction)
+      .getSourceAddress()
+      .(VariableAddressInstruction)
+      .getASTVariable()
+      .hasName("argv") and
+  instr.getEnclosingFunction().hasGlobalName("main")
+}
+
 private class DefaultTaintTrackingCfg extends DataFlow::Configuration {
   DefaultTaintTrackingCfg() { this = "DefaultTaintTrackingCfg" }
 
-  override predicate isSource(DataFlow::Node source) { isUserInput(source.asExpr(), _) }
+  override predicate isSource(DataFlow::Node source) {
+    userInputInstruction(source.asInstruction())
+  }
 
   override predicate isSink(DataFlow::Node sink) { any() }
 
@@ -135,6 +157,8 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
   // This is part of the translation of `a[i]`, where we want taint to flow
   // from `a`.
   i2.(PointerAddInstruction).getLeft() = i1
+  // TODO: robust Chi handling
+  //
   // TODO: Flow from argument to return of known functions: Port missing parts
   // of `returnArgument` to the `interfaces.Taint` and `interfaces.DataFlow`
   // libraries.
@@ -176,11 +200,30 @@ private Element adjustedSink(DataFlow::Node sink) {
   // For compatibility, send flow into a `NotExpr` even if it's part of a
   // short-circuiting condition and thus might get skipped.
   result.(NotExpr).getOperand() = sink.asExpr()
+  or
+  // For compatibility, send flow from argument read side effects to their
+  // corresponding argument expression
+  exists(IndirectReadSideEffectInstruction read |
+    read.getAnOperand().(SideEffectOperand).getAnyDef() = sink.asInstruction() and
+    read.getArgumentDef().getUnconvertedResultExpression() = result
+  )
+  or
+  exists(BufferReadSideEffectInstruction read |
+    read.getAnOperand().(SideEffectOperand).getAnyDef() = sink.asInstruction() and
+    read.getArgumentDef().getUnconvertedResultExpression() = result
+  )
+  or
+  exists(SizedBufferReadSideEffectInstruction read |
+    read.getAnOperand().(SideEffectOperand).getAnyDef() = sink.asInstruction() and
+    read.getArgumentDef().getUnconvertedResultExpression() = result
+  )
 }
 
 predicate tainted(Expr source, Element tainted) {
   exists(DefaultTaintTrackingCfg cfg, DataFlow::Node sink |
     cfg.hasFlow(DataFlow::exprNode(source), sink)
+    or
+    cfg.hasFlow(DataFlow::definitionByReferenceNode(source), sink)
   |
     tainted = adjustedSink(sink)
   )

cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -173,11 +173,48 @@ abstract class PostUpdateNode extends Node {
   abstract Node getPreUpdateNode();
 }
 
+/**
+ * A node that represents the value of a variable after a function call that
+ * may have changed the variable because it's passed by reference.
+ *
+ * A typical example would be a call `f(&x)`. Firstly, there will be flow into
+ * `x` from previous definitions of `x`. Secondly, there will be a
+ * `DefinitionByReferenceNode` to represent the value of `x` after the call has
+ * returned. This node will have its `getArgument()` equal to `&x` and its
+ * `getVariableAccess()` equal to `x`.
+ */
+class DefinitionByReferenceNode extends Node {
+  override WriteSideEffectInstruction instr;
+
+  /** Gets the argument corresponding to this node. */
+  Expr getArgument() {
+    result = instr
+          .getPrimaryInstruction()
+          .(CallInstruction)
+          .getPositionalArgument(instr.getIndex())
+          .getUnconvertedResultExpression()
+    or
+    result = instr
+          .getPrimaryInstruction()
+          .(CallInstruction)
+          .getThisArgument()
+          .getUnconvertedResultExpression() and
+    instr.getIndex() = -1
+  }
+
+  /** Gets the parameter through which this value is assigned. */
+  Parameter getParameter() {
+    exists(CallInstruction ci | result = ci.getStaticCallTarget().getParameter(instr.getIndex()))
+  }
+}
+
 /**
  * Gets the node corresponding to `instr`.
  */
 Node instructionNode(Instruction instr) { result.asInstruction() = instr }
 
+DefinitionByReferenceNode definitionByReferenceNode(Expr e) { result.getArgument() = e }
+
 /**
  * Gets a `Node` corresponding to `e` or any of its conversions. There is no
  * result if `e` is a `Conversion`.