Merge remote-tracking branch 'origin/main' into nickrolfe/regex_injection

Author: nickrolfe

CONTRIBUTING.md

@@ -11,13 +11,14 @@ If you have an idea for a query that you would like to share with other CodeQL u
 
 1. **Directory structure**
 
-    There are five language-specific query directories in this repository:
+    There are six language-specific query directories in this repository:
 
       * C/C++: `cpp/ql/src`
       * C#: `csharp/ql/src`
       * Java: `java/ql/src`
       * JavaScript: `javascript/ql/src`
       * Python: `python/ql/src`
+      * Ruby: `ruby/ql/src`
 
     Each language-specific directory contains further subdirectories that group queries based on their `@tags` or purpose.
     - Experimental queries and libraries are stored in the `experimental` subdirectory within each language-specific directory in the [CodeQL repository](https://github.com/github/codeql). For example, experimental Java queries and libraries are stored in `java/ql/src/experimental` and any corresponding tests in `java/ql/test/experimental`.

cpp/change-notes/2021-11-01-isFromSystemMacroDefinition.md

@@ -0,0 +1,4 @@
+lgtm,codescanning
+* The QL library `semmle.code.cpp.commons.Exclusions` now contains a predicate
+  `isFromSystemMacroDefinition` for identifying code that originates from a
+  macro outside the project being analyzed.

cpp/ql/lib/semmle/code/cpp/Class.qll

@@ -237,7 +237,7 @@ class Class extends UserType {
     exists(ClassDerivation cd | cd.getBaseClass() = base |
       result =
         this.accessOfBaseMemberMulti(cd.getDerivedClass(),
-          fieldInBase.accessInDirectDerived(cd.getASpecifier().(AccessSpecifier)))
+          fieldInBase.accessInDirectDerived(cd.getASpecifier()))
     )
   }
 
@@ -261,8 +261,7 @@ class Class extends UserType {
    * includes the case of `base` = `this`.
    */
   AccessSpecifier accessOfBaseMember(Declaration member) {
-    result =
-      this.accessOfBaseMember(member.getDeclaringType(), member.getASpecifier().(AccessSpecifier))
+    result = this.accessOfBaseMember(member.getDeclaringType(), member.getASpecifier())
   }
 
   /**
@@ -319,7 +318,7 @@ class Class extends UserType {
     exists(Type t | t = this.getAFieldSubobjectType().getUnspecifiedType() |
       // Note: Overload resolution is not implemented -- all copy
       // constructors are considered equal.
-      this.cannotAccessCopyConstructorOnAny(t.(Class))
+      this.cannotAccessCopyConstructorOnAny(t)
     )
     or
     // - T has direct or virtual base class that cannot be copied (has deleted,
@@ -392,7 +391,7 @@ class Class extends UserType {
     exists(Type t | t = this.getAFieldSubobjectType().getUnspecifiedType() |
       // Note: Overload resolution is not implemented -- all copy assignment
       // operators are considered equal.
-      this.cannotAccessCopyAssignmentOperatorOnAny(t.(Class))
+      this.cannotAccessCopyAssignmentOperatorOnAny(t)
     )
     or
     exists(Class c | c = this.getADirectOrVirtualBase() |

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

@@ -490,8 +490,7 @@ class AccessHolder extends Declaration, TAccessHolder {
    */
   pragma[inline]
   predicate canAccessMember(Declaration member, Class derived) {
-    this.couldAccessMember(member.getDeclaringType(), member.getASpecifier().(AccessSpecifier),
-      derived)
+    this.couldAccessMember(member.getDeclaringType(), member.getASpecifier(), derived)
   }
 
   /**

cpp/ql/lib/semmle/code/cpp/commons/Dependency.qll

@@ -275,17 +275,17 @@ private predicate dependsOnDeclarationEntry(Element src, DeclarationEntry dest)
     dependsOnTransitive(src, mid) and
     not mid instanceof Type and
     not mid instanceof EnumConstant and
-    getDeclarationEntries(mid, dest.(DeclarationEntry)) and
+    getDeclarationEntries(mid, dest) and
     not dest instanceof TypeDeclarationEntry
   )
   or
   exists(Declaration mid |
     // dependency from a Type / Variable / Function use -> any (visible) definition
     dependsOnTransitive(src, mid) and
     not mid instanceof EnumConstant and
-    getDeclarationEntries(mid, dest.(DeclarationEntry)) and
+    getDeclarationEntries(mid, dest) and
     // must be definition
-    dest.(DeclarationEntry).isDefinition()
+    dest.isDefinition()
   )
 }
 
@@ -307,7 +307,7 @@ private predicate dependsOnFull(DependsSource src, Symbol dest, int category) {
     // dependency from a Variable / Function use -> non-visible definition (link time)
     dependsOnTransitive(src, mid) and
     not mid instanceof EnumConstant and
-    getDeclarationEntries(mid, dest.(DeclarationEntry)) and
+    getDeclarationEntries(mid, dest) and
     not dest instanceof TypeDeclarationEntry and
     // must be definition
     dest.(DeclarationEntry).isDefinition() and

cpp/ql/lib/semmle/code/cpp/commons/Exclusions.qll

@@ -81,8 +81,8 @@ predicate functionContainsPreprocCode(Function f) {
 }
 
 /**
- * Holds if `e` is completely or partially from a macro definition, as opposed
- * to being passed in as an argument.
+ * Holds if `e` is completely or partially from a macro invocation `mi`, as
+ * opposed to being passed in as an argument.
  *
  * In the following example, the call to `f` is from a macro definition,
  * while `y`, `+`, `1`, and `;` are not. This assumes that no identifier apart
@@ -93,8 +93,8 @@ predicate functionContainsPreprocCode(Function f) {
  *   M(y + 1);
  * ```
  */
-predicate isFromMacroDefinition(Element e) {
-  exists(MacroInvocation mi, Location eLocation, Location miLocation |
+private predicate isFromMacroInvocation(Element e, MacroInvocation mi) {
+  exists(Location eLocation, Location miLocation |
     mi.getAnExpandedElement() = e and
     eLocation = e.getLocation() and
     miLocation = mi.getLocation() and
@@ -109,3 +109,36 @@ predicate isFromMacroDefinition(Element e) {
     eLocation.getEndColumn() >= miLocation.getEndColumn()
   )
 }
+
+/**
+ * Holds if `e` is completely or partially from a macro definition, as opposed
+ * to being passed in as an argument.
+ *
+ * In the following example, the call to `f` is from a macro definition,
+ * while `y`, `+`, `1`, and `;` are not. This assumes that no identifier apart
+ * from `M` refers to a macro.
+ * ```
+ * #define M(x) f(x)
+ * ...
+ *   M(y + 1);
+ * ```
+ */
+predicate isFromMacroDefinition(Element e) { isFromMacroInvocation(e, _) }
+
+/**
+ * Holds if `e` is completely or partially from a _system macro_ definition, as
+ * opposed to being passed in as an argument. A system macro is a macro whose
+ * definition is outside the source directory of the database.
+ *
+ * If the system macro is invoked through a non-system macro, then this
+ * predicate does not hold.
+ *
+ * See also `isFromMacroDefinition`.
+ */
+predicate isFromSystemMacroDefinition(Element e) {
+  exists(MacroInvocation mi |
+    isFromMacroInvocation(e, mi) and
+    // Has no relative path in the database, meaning it's a system file.
+    not exists(mi.getMacro().getFile().getRelativePath())
+  )
+}

cpp/ql/lib/semmle/code/cpp/commons/NullTermination.qll

@@ -11,6 +11,19 @@ private predicate mayAddNullTerminatorHelper(Expr e, VariableAccess va, Expr e0)
   )
 }
 
+bindingset[n1, n2]
+private predicate controlFlowNodeSuccessorTransitive(ControlFlowNode n1, ControlFlowNode n2) {
+  exists(BasicBlock bb1, int pos1, BasicBlock bb2, int pos2 |
+    pragma[only_bind_into](bb1).getNode(pos1) = n1 and
+    pragma[only_bind_into](bb2).getNode(pos2) = n2 and
+    (
+      bb1 = bb2 and pos1 < pos2
+      or
+      bb1.getASuccessor+() = bb2
+    )
+  )
+}
+
 /**
  * Holds if the expression `e` may add a null terminator to the string in
  * variable `v`.
@@ -30,14 +43,9 @@ predicate mayAddNullTerminator(Expr e, VariableAccess va) {
   )
   or
   // Assignment to another stack variable
-  exists(Expr e0, BasicBlock bb, int pos, BasicBlock bb0, int pos0 |
-    mayAddNullTerminatorHelper(e, va, e0) and
-    bb.getNode(pos) = e and
-    bb0.getNode(pos0) = e0
-  |
-    bb = bb0 and pos < pos0
-    or
-    bb.getASuccessor+() = bb0
+  exists(Expr e0 |
+    mayAddNullTerminatorHelper(pragma[only_bind_into](e), va, pragma[only_bind_into](e0)) and
+    controlFlowNodeSuccessorTransitive(e, e0)
   )
   or
   // Assignment to non-stack variable
@@ -119,14 +127,9 @@ predicate variableMustBeNullTerminated(VariableAccess va) {
       variableMustBeNullTerminated(use) and
       // Simplified: check that `p` may not be null terminated on *any*
       // path to `use` (including the one found via `parameterUsePair`)
-      not exists(Expr e, BasicBlock bb1, int pos1, BasicBlock bb2, int pos2 |
-        mayAddNullTerminator(e, p.getAnAccess()) and
-        bb1.getNode(pos1) = e and
-        bb2.getNode(pos2) = use
-      |
-        bb1 = bb2 and pos1 < pos2
-        or
-        bb1.getASuccessor+() = bb2
+      not exists(Expr e |
+        mayAddNullTerminator(pragma[only_bind_into](e), p.getAnAccess()) and
+        controlFlowNodeSuccessorTransitive(e, use)
       )
     )
   )

cpp/ql/lib/semmle/code/cpp/commons/Printf.qll

@@ -175,9 +175,7 @@ class FormattingFunctionCall extends Expr {
   /**
    * Gets the index at which the format string occurs in the argument list.
    */
-  int getFormatParameterIndex() {
-    result = this.getTarget().(FormattingFunction).getFormatParameterIndex()
-  }
+  int getFormatParameterIndex() { result = this.getTarget().getFormatParameterIndex() }
 
   /**
    * Gets the format expression used in this call.
@@ -191,7 +189,7 @@ class FormattingFunctionCall extends Expr {
     exists(int i |
       result = this.getArgument(i) and
       n >= 0 and
-      n = i - this.getTarget().(FormattingFunction).getFirstFormatArgumentIndex()
+      n = i - this.getTarget().getFirstFormatArgumentIndex()
     )
   }
 
@@ -251,7 +249,7 @@ class FormattingFunctionCall extends Expr {
   int getNumFormatArgument() {
     result = count(this.getFormatArgument(_)) and
     // format arguments must be known
-    exists(this.getTarget().(FormattingFunction).getFirstFormatArgumentIndex())
+    exists(this.getTarget().getFirstFormatArgumentIndex())
   }
 
   /**
@@ -289,35 +287,27 @@ class FormatLiteral extends Literal {
    * a `char *` (either way, `%S` will have the opposite meaning).
    * DEPRECATED: Use getDefaultCharType() instead.
    */
-  deprecated predicate isWideCharDefault() {
-    this.getUse().getTarget().(FormattingFunction).isWideCharDefault()
-  }
+  deprecated predicate isWideCharDefault() { this.getUse().getTarget().isWideCharDefault() }
 
   /**
    * Gets the default character type expected for `%s` by this format literal.  Typically
    * `char` or `wchar_t`.
    */
-  Type getDefaultCharType() {
-    result = this.getUse().getTarget().(FormattingFunction).getDefaultCharType()
-  }
+  Type getDefaultCharType() { result = this.getUse().getTarget().getDefaultCharType() }
 
   /**
    * Gets the non-default character type expected for `%S` by this format literal.  Typically
    * `wchar_t` or `char`.  On some snapshots there may be multiple results where we can't tell
    * which is correct for a particular function.
    */
-  Type getNonDefaultCharType() {
-    result = this.getUse().getTarget().(FormattingFunction).getNonDefaultCharType()
-  }
+  Type getNonDefaultCharType() { result = this.getUse().getTarget().getNonDefaultCharType() }
 
   /**
    * Gets the wide character type for this format literal.  This is usually `wchar_t`.  On some
    * snapshots there may be multiple results where we can't tell which is correct for a
    * particular function.
    */
-  Type getWideCharType() {
-    result = this.getUse().getTarget().(FormattingFunction).getWideCharType()
-  }
+  Type getWideCharType() { result = this.getUse().getTarget().getWideCharType() }
 
   /**
    * Holds if this `FormatLiteral` is in a context that supports
@@ -896,7 +886,7 @@ class FormatLiteral extends Literal {
     exists(string len, string conv |
       this.parseConvSpec(n, _, _, _, _, _, len, conv) and
       (len != "l" and len != "w" and len != "h") and
-      this.getUse().getTarget().(FormattingFunction).getFormatCharType().getSize() > 1 and // wide function
+      this.getUse().getTarget().getFormatCharType().getSize() > 1 and // wide function
       (
         conv = "c" and
         result = this.getNonDefaultCharType()

cpp/ql/lib/semmle/code/cpp/controlflow/DefinitionsAndUses.qll

@@ -25,7 +25,7 @@ predicate definitionUsePair(SemanticStackVariable var, Expr def, Expr use) {
  * Holds if the definition `def` of some stack variable can reach `node`, which
  * is a definition or use, without crossing definitions of the same variable.
  */
-predicate definitionReaches(Expr def, Expr node) { def.(Def).reaches(true, _, node.(DefOrUse)) }
+predicate definitionReaches(Expr def, Expr node) { def.(Def).reaches(true, _, node) }
 
 private predicate hasAddressOfAccess(SemanticStackVariable var) {
   var.getAnAccess().isAddressOfAccessNonConst()

cpp/ql/lib/semmle/code/cpp/controlflow/SSA.qll

@@ -62,7 +62,7 @@ class SsaDefinition extends ControlFlowNodeBase {
   BasicBlock getBasicBlock() { result.contains(this.getDefinition()) }
 
   /** Holds if this definition is a phi node for variable `v`. */
-  predicate isPhiNode(StackVariable v) { exists(StandardSSA x | x.phi_node(v, this.(BasicBlock))) }
+  predicate isPhiNode(StackVariable v) { exists(StandardSSA x | x.phi_node(v, this)) }
 
   /** Gets the location of this definition. */
   Location getLocation() { result = this.(ControlFlowNode).getLocation() }