Merge branch 'main' into android-database

java/ql/src/Metrics/Dependencies/ExternalDependencies.ql

@@ -1,4 +1,5 @@
 /**
+ * @deprecated
  * @name External dependencies
  * @description Count the number of dependencies a Java source file has on jar files.
  * @kind treemap

java/ql/src/Metrics/Dependencies/ExternalDependenciesSourceLinks.ql

@@ -1,4 +1,5 @@
 /**
+ * @deprecated
  * @name External dependency source links
  * @kind source-link
  * @metricType externalDependency

java/ql/src/Metrics/Files/FLinesOfDuplicatedCode.ql

@@ -1,4 +1,5 @@
 /**
+ * @deprecated
  * @name Duplicated lines in files
  * @description The number of lines in a file, including code, comment and whitespace lines,
  *              which are duplicated in at least one other place.

java/ql/src/Metrics/Files/FLinesOfSimilarCode.ql

@@ -1,4 +1,5 @@
 /**
+ * @deprecated
  * @name Similar lines in files
  * @description The number of lines in a file, including code, comment and whitespace lines,
  *              which are similar to lines in at least one other place.

java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp

@@ -39,7 +39,7 @@ giving them access to all the system's passwords.</p>
 
 <li>
 OWASP:
-<a href="https://www.owasp.org/index.php/Path_traversal">Path Traversal</a>.
+<a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.
 </li>
 
 </references>

java/ql/src/Security/CWE/CWE-022/ZipSlip.qhelp

@@ -63,7 +63,7 @@ Snyk:
 </li>
 <li>
 OWASP:
-<a href="https://www.owasp.org/index.php/Path_traversal">Path Traversal</a>.
+<a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.
 </li>
 
 </references>

java/ql/src/Security/CWE/CWE-209/StackTraceExposure.qhelp

@@ -38,7 +38,7 @@ information.</p>
 </example>
 
 <references>
-<li>OWASP: <a href="https://www.owasp.org/index.php/Information_Leak_(information_disclosure)">Information Leak</a>.</li>
+<li>OWASP: <a href="https://owasp.org/www-community/Improper_Error_Handling">Improper Error Handling</a>.</li>
 
 <li>CERT Java Coding Standard:
 <a href="https://www.securecoding.cert.org/confluence/display/java/ERR01-J.+Do+not+allow+exceptions+to+expose+sensitive+information">ERR01-J.

java/ql/src/codeql-suites/java-lgtm-full.qls

@@ -7,10 +7,7 @@
     tags contain:
       - ide-contextual-queries/local-definitions
       - ide-contextual-queries/local-references
-- query: Metrics/Dependencies/ExternalDependencies.ql
-- query: Metrics/Dependencies/ExternalDependenciesSourceLinks.ql
 - query: Metrics/Files/FLinesOfCode.ql
 - query: Metrics/Files/FLinesOfCommentedCode.ql
 - query: Metrics/Files/FLinesOfComment.ql
-- query: Metrics/Files/FLinesOfDuplicatedCode.ql
 - query: Metrics/Files/FNumberOfTests.ql

java/ql/src/experimental/Security/CWE/CWE-643/XPathInjection.qhelp

@@ -38,7 +38,7 @@ The fifth example is a dom4j XPath injection example.
 </example>
 
 <references>
-<li>OWASP: <a href="https://www.owasp.org/index.php?title=Testing_for_XPath_Injection_(OTG-INPVAL-010)">Testing for XPath Injection</a>.</li>
+<li>OWASP: <a href="https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/07-Input_Validation_Testing/09-Testing_for_XPath_Injection">Testing for XPath Injection</a>.</li>
 <li>OWASP: <a href="https://www.owasp.org/index.php/XPATH_Injection">XPath Injection</a>.</li>
 </references>
 </qhelp>

java/ql/src/external/DuplicateAnonymous.ql

@@ -1,4 +1,5 @@
 /**
+ * @deprecated
  * @name Duplicate anonymous class
  * @description Duplicated anonymous classes indicate that refactoring is necessary.
  * @kind problem

java/ql/src/external/DuplicateBlock.ql

@@ -1,4 +1,5 @@
 /**
+ * @deprecated
  * @name Duplicate code
  * @description This block of code is duplicated elsewhere. If possible, the shared code should be refactored so there is only one occurrence left. It may not always be possible to address these issues; other duplicate code checks (such as duplicate function, duplicate class) give subsets of the results with higher confidence.
  * @kind problem

java/ql/src/external/DuplicateMethod.ql

@@ -1,4 +1,5 @@
 /**
+ * @deprecated
  * @name Duplicate method
  * @description Duplicated methods make code more difficult to understand and introduce a risk of
  *              changes being made to only one copy.

java/ql/src/external/MostlyDuplicateClass.ql

@@ -1,4 +1,5 @@
 /**
+ * @deprecated
  * @name Mostly duplicate class
  * @description Classes in which most of the methods are duplicated in another class make code more
  *              difficult to understand and introduce a risk of changes being made to only one copy.

java/ql/src/external/MostlyDuplicateFile.ql

@@ -1,4 +1,5 @@
 /**
+ * @deprecated
  * @name Mostly duplicate file
  * @description Files in which most of the lines are duplicated in another file make code more
  *              difficult to understand and introduce a risk of changes being made to only one copy.

java/ql/src/external/MostlyDuplicateMethod.ql

@@ -1,4 +1,5 @@
 /**
+ * @deprecated
  * @name Mostly duplicate method
  * @description Methods in which most of the lines are duplicated in another method make code more
  *              difficult to understand and introduce a risk of changes being made to only one copy.

java/ql/src/external/MostlySimilarFile.ql

@@ -1,4 +1,5 @@
 /**
+ * @deprecated
  * @name Mostly similar file
  * @description Files in which most of the lines are similar to those in another file make code more
  *              difficult to understand and introduce a risk of changes being made to only one copy.

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

@@ -1912,14 +1912,10 @@ private module Qualifier {
 }
 
 /** An expression that assigns a value to a field. */
-class FieldWrite extends FieldAccess {
-  FieldWrite() { exists(Field f | f = getVariable() and isLValue()) }
-}
+class FieldWrite extends FieldAccess, LValue { }
 
 /** An expression that reads a field. */
-class FieldRead extends FieldAccess {
-  FieldRead() { exists(Field f | f = getVariable() and isRValue()) }
-}
+class FieldRead extends FieldAccess, RValue { }
 
 private predicate hasInstantiation(RefType t) {
   t instanceof TypeVariable or

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

@@ -113,6 +113,10 @@ private predicate locationSortKeys(Element ast, string file, int line, int colum
  */
 private newtype TPrintAstNode =
   TElementNode(Element el) { shouldPrint(el, _) } or
+  TForInitNode(ForStmt fs) { shouldPrint(fs, _) and exists(fs.getAnInit()) } or
+  TLocalVarDeclNode(LocalVariableDeclExpr lvde) {
+    shouldPrint(lvde, _) and lvde.getParent() instanceof SingleLocalVarDeclParent
+  } or
   TAnnotationsNode(Annotatable ann) {
     shouldPrint(ann, _) and ann.hasAnnotation() and not partOfAnnotation(ann)
   } or
@@ -221,6 +225,24 @@ abstract class ElementNode extends PrintAstNode, TElementNode {
   final Element getElement() { result = element }
 }
 
+/**
+ * A node representing an `Expr` or a `Stmt`.
+ */
+class ExprStmtNode extends ElementNode {
+  ExprStmtNode() { element instanceof ExprOrStmt }
+
+  override PrintAstNode getChild(int childIndex) {
+    exists(Element el | result.(ElementNode).getElement() = el |
+      el.(Expr).isNthChildOf(element, childIndex)
+      or
+      el.(Stmt).isNthChildOf(element, childIndex)
+    )
+  }
+}
+
+/**
+ * Holds if the given expression is part of an annotation.
+ */
 private predicate partOfAnnotation(Expr e) {
   e instanceof Annotation
   or
@@ -228,49 +250,123 @@ private predicate partOfAnnotation(Expr e) {
   partOfAnnotation(e.getParent())
 }
 
-private Expr getAnAnnotationChild(Expr e) {
-  partOfAnnotation(e) and
-  (
-    result = e.(Annotation).getValue(_)
+/**
+ * A node representing an `Expr` that is part of an annotation.
+ */
+final class AnnotationPartNode extends ExprStmtNode {
+  AnnotationPartNode() { partOfAnnotation(element) }
+
+  override ElementNode getChild(int childIndex) {
+    result.getElement() =
+      rank[childIndex](Element ch, string file, int line, int column |
+        ch = getAnAnnotationChild() and locationSortKeys(ch, file, line, column)
+      |
+        ch order by file, line, column
+      )
+  }
+
+  private Expr getAnAnnotationChild() {
+    result = element.(Annotation).getValue(_)
     or
-    result = e.(ArrayInit).getAnInit()
+    result = element.(ArrayInit).getAnInit()
     or
-    result = e.(ArrayInit).(Annotatable).getAnAnnotation()
-  )
+    result = element.(ArrayInit).(Annotatable).getAnAnnotation()
+  }
 }
 
 /**
- * An node representing an `Expr` or a `Stmt`.
+ * A node representing a `LocalVariableDeclExpr`.
  */
-final class ExprStmtNode extends ElementNode {
-  ExprStmtNode() { element instanceof ExprOrStmt }
+final class LocalVarDeclExprNode extends ExprStmtNode {
+  LocalVarDeclExprNode() { element instanceof LocalVariableDeclExpr }
 
   override PrintAstNode getChild(int childIndex) {
-    exists(Element el | result.(ElementNode).getElement() = el |
-      el.(Expr).isNthChildOf(element, childIndex) and
-      not partOfAnnotation(element)
-      or
-      el.(Stmt).isNthChildOf(element, childIndex)
-      or
-      childIndex = -4 and
-      el = element.(ClassInstanceExpr).getAnonymousClass()
-      or
-      childIndex = 0 and
-      el = element.(LocalClassDeclStmt).getLocalClass()
-      or
-      partOfAnnotation(element) and
-      el =
-        rank[childIndex](Element ch, string file, int line, int column |
-          ch = getAnAnnotationChild(element) and locationSortKeys(ch, file, line, column)
-        |
-          ch order by file, line, column
-        )
-    )
+    result = super.getChild(childIndex)
     or
-    exists(Element el | result.(AnnotationsNode).getAnnotated() = el |
-      childIndex = -2 and
-      el = element.(LocalVariableDeclExpr).getVariable()
-    )
+    childIndex = -2 and
+    result.(AnnotationsNode).getAnnotated() = element.(LocalVariableDeclExpr).getVariable()
+  }
+}
+
+/**
+ * A node representing a `ClassInstanceExpr`.
+ */
+final class ClassInstanceExprNode extends ExprStmtNode {
+  ClassInstanceExprNode() { element instanceof ClassInstanceExpr }
+
+  override ElementNode getChild(int childIndex) {
+    result = super.getChild(childIndex)
+    or
+    childIndex = -4 and
+    result.getElement() = element.(ClassInstanceExpr).getAnonymousClass()
+  }
+}
+
+/**
+ * A node representing a `LocalClassDeclStmt`.
+ */
+final class LocalClassDeclStmtNode extends ExprStmtNode {
+  LocalClassDeclStmtNode() { element instanceof LocalClassDeclStmt }
+
+  override ElementNode getChild(int childIndex) {
+    result = super.getChild(childIndex)
+    or
+    childIndex = 0 and
+    result.getElement() = element.(LocalClassDeclStmt).getLocalClass()
+  }
+}
+
+/**
+ * A node representing a `ForStmt`.
+ */
+final class ForStmtNode extends ExprStmtNode {
+  ForStmtNode() { element instanceof ForStmt }
+
+  override PrintAstNode getChild(int childIndex) {
+    childIndex >= 1 and
+    result = super.getChild(childIndex)
+    or
+    childIndex = 0 and
+    result.(ForInitNode).getForStmt() = element
+  }
+}
+
+/**
+ * An element that can be the parent of up to one `LocalVariableDeclExpr` for which we want
+ * to use a synthetic node to hold the variable declaration and its `TypeAccess`.
+ */
+private class SingleLocalVarDeclParent extends ExprOrStmt {
+  SingleLocalVarDeclParent() {
+    this instanceof EnhancedForStmt or
+    this instanceof CatchClause or
+    this.(InstanceOfExpr).isPattern()
+  }
+
+  /** Gets the variable declaration that this element contains */
+  LocalVariableDeclExpr getVariable() { result.getParent() = this }
+
+  /** Gets the type access of the variable */
+  Expr getTypeAccess() { result = getVariable().getTypeAccess() }
+}
+
+/**
+ * A node representing an element that can be the parent of up to one `LocalVariableDeclExpr` for which we
+ * want to use a synthetic node to variable declaration and its type access.
+ *
+ * Excludes `LocalVariableDeclStmt` and `ForStmt`, as they can hold multiple declarations.
+ * For these cases, either a synthetic node is not necassary or a different synthetic node is used.
+ */
+final class SingleLocalVarDeclParentNode extends ExprStmtNode {
+  SingleLocalVarDeclParent lvdp;
+
+  SingleLocalVarDeclParentNode() { lvdp = element }
+
+  override PrintAstNode getChild(int childIndex) {
+    result = super.getChild(childIndex) and
+    not result.(ElementNode).getElement() = [lvdp.getVariable(), lvdp.getTypeAccess()]
+    or
+    childIndex = lvdp.getVariable().getIndex() and
+    result.(LocalVarDeclSynthNode).getVariable() = lvdp.getVariable()
   }
 }
 
@@ -432,6 +528,51 @@ final class TypeVariableNode extends ElementNode {
   }
 }
 
+/**
+ * A node representing the initializers of a `ForStmt`.
+ */
+final class ForInitNode extends PrintAstNode, TForInitNode {
+  ForStmt fs;
+
+  ForInitNode() { this = TForInitNode(fs) }
+
+  override string toString() { result = "(For Initializers) " }
+
+  override ElementNode getChild(int childIndex) {
+    childIndex >= 0 and
+    result.getElement().(Expr).isNthChildOf(fs, -childIndex)
+  }
+
+  /**
+   * Gets the underlying `ForStmt`.
+   */
+  ForStmt getForStmt() { result = fs }
+}
+
+/**
+ * A synthetic node holding a `LocalVariableDeclExpr` and its type access.
+ */
+final class LocalVarDeclSynthNode extends PrintAstNode, TLocalVarDeclNode {
+  LocalVariableDeclExpr lvde;
+
+  LocalVarDeclSynthNode() { this = TLocalVarDeclNode(lvde) }
+
+  override string toString() { result = "(Single Local Variable Declaration)" }
+
+  override ElementNode getChild(int childIndex) {
+    childIndex = 0 and
+    result.getElement() = lvde.getTypeAccess()
+    or
+    childIndex = 1 and
+    result.getElement() = lvde
+  }
+
+  /**
+   * Gets the underlying `LocalVariableDeclExpr`
+   */
+  LocalVariableDeclExpr getVariable() { result = lvde }
+}
+
 /**
  * A node representing the annotations of an `Annotatable`.
  * Only rendered if there is at least one annotation.

java/ql/src/semmle/code/java/dataflow/FlowSources.qll

@@ -106,12 +106,31 @@ private class MessageBodyReaderParameterSource extends RemoteFlowSource {
   override string getSourceType() { result = "MessageBodyReader parameter" }
 }
 
+private class SpringMultipartRequestSource extends RemoteFlowSource {
+  SpringMultipartRequestSource() {
+    exists(MethodAccess ma, Method m |
+      ma = this.asExpr() and
+      m = ma.getMethod() and
+      m
+          .getDeclaringType()
+          .getASourceSupertype*()
+          .hasQualifiedName("org.springframework.web.multipart", "MultipartRequest") and
+      m.getName().matches("get%")
+    )
+  }
+
+  override string getSourceType() { result = "Spring MultipartRequest getter" }
+}
+
 private class SpringMultipartFileSource extends RemoteFlowSource {
   SpringMultipartFileSource() {
     exists(MethodAccess ma, Method m |
       ma = this.asExpr() and
       m = ma.getMethod() and
-      m.getDeclaringType().hasQualifiedName("org.springframework.web.multipart", "MultipartFile") and
+      m
+          .getDeclaringType()
+          .getASourceSupertype*()
+          .hasQualifiedName("org.springframework.web.multipart", "MultipartFile") and
       m.getName().matches("get%")
     )
   }

java/ql/src/semmle/code/java/dataflow/SSA.qll

@@ -770,7 +770,9 @@ private module SsaImpl {
 
     /** Holds if `v` occurs in `b` or one of `b`'s transitive successors. */
     private predicate blockPrecedesVar(TrackedVar v, BasicBlock b) {
-      varOccursInBlock(v, b.getABBSuccessor*())
+      varOccursInBlock(v, b)
+      or
+      ssaDefReachesEndOfBlock(v, _, b)
     }
 
     /**

java/ql/src/semmle/code/java/dataflow/internal/BaseSSA.qll

@@ -321,7 +321,9 @@ private module SsaImpl {
 
     /** Holds if `v` occurs in `b` or one of `b`'s transitive successors. */
     private predicate blockPrecedesVar(BaseSsaSourceVariable v, BasicBlock b) {
-      varOccursInBlock(v, b.getABBSuccessor*())
+      varOccursInBlock(v, b)
+      or
+      ssaDefReachesEndOfBlock(v, _, b)
     }
 
     /**

java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll

@@ -125,6 +125,8 @@ private predicate localAdditionalTaintExprStep(Expr src, Expr sink) {
   stringBuilderStep(src, sink)
   or
   serializationStep(src, sink)
+  or
+  formatStep(src, sink)
 }
 
 /**
@@ -389,6 +391,9 @@ private predicate taintPreservingQualifierToMethod(Method m) {
     )
   )
   or
+  m.getDeclaringType() instanceof TypeFormatter and
+  m.hasName(["format", "out"])
+  or
   m.getDeclaringType().getASourceSupertype*() instanceof TypeSQLiteQueryBuilder and
   // buildQuery(String[] projectionIn, String selection, String groupBy, String having, String sortOrder, String limit)
   // buildQuery(String[] projectionIn, String selection, String[] selectionArgs, String groupBy, String having, String sortOrder, String limit)
@@ -457,6 +462,9 @@ private predicate taintPreservingArgumentToMethod(Method method) {
   method.getDeclaringType() instanceof TypeString and
   (method.hasName("format") or method.hasName("formatted") or method.hasName("join"))
   or
+  method.getDeclaringType() instanceof TypeFormatter and
+  method.hasName("format")
+  or
   method.getDeclaringType() instanceof TypeDatabaseUtils and
   // String[] appendSelectionArgs(String[] originalValues, String[] newValues)
   // String concatenateWhere(String a, String b)
@@ -672,6 +680,20 @@ private predicate argToQualifierStep(Expr tracked, Expr sink) {
     tracked = ma.getArgument(i) and
     sink = ma.getQualifier()
   )
+  or
+  exists(MethodAccess ma |
+    taintPreservingArgumentToQualifier(ma.getMethod()) and
+    tracked = ma.getAnArgument() and
+    sink = ma.getQualifier()
+  )
+}
+
+/**
+ * Holds if `method` is a method that transfers taint from any of its arguments to its qualifier.
+ */
+private predicate taintPreservingArgumentToQualifier(Method method) {
+  method.getDeclaringType() instanceof TypeFormatter and
+  method.hasName("format")
 }
 
 /**
@@ -777,6 +799,56 @@ class ObjectOutputStreamVar extends LocalVariableDecl {
   }
 }
 
+/** Flow through string formatting. */
+private predicate formatStep(Expr tracked, Expr sink) {
+  exists(FormatterVar v, VariableAssign def |
+    def = v.getADef() and
+    exists(MethodAccess ma, RValue use |
+      ma.getAnArgument() = tracked and
+      ma = v.getAFormatMethodAccess() and
+      use = ma.getQualifier() and
+      defUsePair(def, use)
+    ) and
+    exists(RValue output, ClassInstanceExpr cie |
+      cie = def.getSource() and
+      output = cie.getArgument(0) and
+      adjacentUseUse(output, sink) and
+      exists(RefType t | output.getType().(RefType).getASourceSupertype*() = t |
+        t.hasQualifiedName("java.io", "OutputStream") or
+        t.hasQualifiedName("java.lang", "Appendable")
+      )
+    )
+  )
+}
+
+/**
+ * A local variable that is assigned a `Formatter`.
+ * Writing tainted data to such a formatter causes the underlying
+ * `OutputStream` or `Appendable` to be tainted.
+ */
+private class FormatterVar extends LocalVariableDecl {
+  FormatterVar() {
+    exists(ClassInstanceExpr cie | cie = this.getAnAssignedValue() |
+      cie.getType() instanceof TypeFormatter
+    )
+  }
+
+  VariableAssign getADef() {
+    result.getSource().(ClassInstanceExpr).getType() instanceof TypeFormatter and
+    result.getDestVar() = this
+  }
+
+  MethodAccess getAFormatMethodAccess() {
+    result.getQualifier() = getAnAccess() and
+    result.getMethod().hasName("format")
+  }
+}
+
+/** The class `java.util.Formatter`. */
+private class TypeFormatter extends Class {
+  TypeFormatter() { this.hasQualifiedName("java.util", "Formatter") }
+}
+
 private import StringBuilderVarModule
 
 module StringBuilderVarModule {

java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisCommon.qll

@@ -245,24 +245,30 @@ Sign ssaDefSign(SsaVariable v) {
 /** Gets a possible sign for `e`. */
 cached
 Sign exprSign(Expr e) {
-  result = certainExprSign(e)
-  or
-  not exists(certainExprSign(e)) and
-  (
-    unknownSign(e)
+  exists(Sign s |
+    s = certainExprSign(e)
     or
-    exists(SsaVariable v | getARead(v) = e | result = ssaVariableSign(v, e))
-    or
-    e =
-      any(VarAccess access |
-        not exists(SsaVariable v | getARead(v) = access) and
-        (
-          result = fieldSign(getField(access.(FieldAccess))) or
-          not access instanceof FieldAccess
+    not exists(certainExprSign(e)) and
+    (
+      unknownSign(e)
+      or
+      exists(SsaVariable v | getARead(v) = e | s = ssaVariableSign(v, e))
+      or
+      e =
+        any(VarAccess access |
+          not exists(SsaVariable v | getARead(v) = access) and
+          (
+            s = fieldSign(getField(access.(FieldAccess))) or
+            not access instanceof FieldAccess
+          )
         )
-      )
-    or
-    result = specificSubExprSign(e)
+      or
+      s = specificSubExprSign(e)
+    )
+  |
+    if e.getType() instanceof UnsignedNumericType and s = TNeg()
+    then result = TPos()
+    else result = s
   )
 }
 

java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll

@@ -53,6 +53,8 @@ private module Impl {
   private import SignAnalysisCommon
   private import SsaReadPositionCommon
 
+  class UnsignedNumericType = CharacterType;
+
   float getNonIntegerValue(Expr e) {
     result = e.(LongLiteral).getValue().toFloat() or
     result = e.(FloatingPointLiteral).getValue().toFloat() or