Merge branch 'main' into refacReDoS

java/ql/lib/change-notes/2022-08-03-properties.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Added a data-flow model for the `setProperty` method of `java.util.Properties`. Additional results may be found where relevant data is stored in and then retrieved from a `Properties` instance.

java/ql/lib/change-notes/2022-08-05-asynctask-improvements.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Improved analysis of the Android class `AsyncTask` so that data can properly flow through its methods according to the life-cycle steps described here: https://developer.android.com/reference/android/os/AsyncTask#the-4-steps.

java/ql/lib/semmle/code/java/dataflow/internal/ContainerFlow.qll

@@ -244,6 +244,9 @@ private class ContainerFlowSummaries extends SummaryModelCsv {
         "java.util;Properties;true;getProperty;(String);;Argument[-1].MapValue;ReturnValue;value;manual",
         "java.util;Properties;true;getProperty;(String,String);;Argument[-1].MapValue;ReturnValue;value;manual",
         "java.util;Properties;true;getProperty;(String,String);;Argument[1];ReturnValue;value;manual",
+        "java.util;Properties;true;setProperty;(String,String);;Argument[-1].MapValue;ReturnValue;value;manual",
+        "java.util;Properties;true;setProperty;(String,String);;Argument[0];Argument[-1].MapKey;value;manual",
+        "java.util;Properties;true;setProperty;(String,String);;Argument[1];Argument[-1].MapValue;value;manual",
         "java.util;Scanner;true;Scanner;;;Argument[0];Argument[-1];taint;manual",
         "java.util;Scanner;true;findInLine;;;Argument[-1];ReturnValue;taint;manual",
         "java.util;Scanner;true;findWithinHorizon;;;Argument[-1];ReturnValue;taint;manual",

java/ql/lib/semmle/code/java/frameworks/android/AsyncTask.qll

@@ -4,19 +4,62 @@ import java
 private import semmle.code.java.dataflow.DataFlow
 private import semmle.code.java.dataflow.FlowSteps
 
-/**
- * Models the value-preserving step from `asyncTask.execute(params)` to `AsyncTask::doInBackground(params)`.
+/*
+ * The following flow steps aim to model the life-cycle of `AsyncTask`s described here:
+ * https://developer.android.com/reference/android/os/AsyncTask#the-4-steps
  */
-private class AsyncTaskAdditionalValueStep extends AdditionalValueStep {
+
+/**
+ * A taint step from the vararg arguments of `AsyncTask::execute` and `AsyncTask::executeOnExecutor`
+ * to the parameter of `AsyncTask::doInBackground`.
+ */
+private class AsyncTaskExecuteAdditionalValueStep extends AdditionalTaintStep {
   override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
     exists(ExecuteAsyncTaskMethodAccess ma, AsyncTaskRunInBackgroundMethod m |
-      DataFlow::getInstanceArgument(ma).getType() = m.getDeclaringType() and
+      DataFlow::getInstanceArgument(ma).getType() = m.getDeclaringType()
+    |
       node1.asExpr() = ma.getParamsArgument() and
       node2.asParameter() = m.getParameter(0)
     )
   }
 }
 
+/**
+ * A value-preserving step from the return value of `AsyncTask::doInBackground`
+ * to the parameter of `AsyncTask::onPostExecute`.
+ */
+private class AsyncTaskOnPostExecuteAdditionalValueStep extends AdditionalValueStep {
+  override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(
+      AsyncTaskRunInBackgroundMethod runInBackground, AsyncTaskOnPostExecuteMethod onPostExecute
+    |
+      onPostExecute.getDeclaringType() = runInBackground.getDeclaringType()
+    |
+      node1.asExpr() = any(ReturnStmt r | r.getEnclosingCallable() = runInBackground).getResult() and
+      node2.asParameter() = onPostExecute.getParameter(0)
+    )
+  }
+}
+
+/**
+ * A value-preserving step from field initializers in `AsyncTask`'s constructor or initializer method
+ * to the instance parameter of `AsyncTask::runInBackground` and `AsyncTask::onPostExecute`.
+ */
+private class AsyncTaskFieldInitQualifierToInstanceParameterStep extends AdditionalValueStep {
+  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
+    exists(AsyncTaskInit init, Callable receiver |
+      n1.(DataFlow::PostUpdateNode).getPreUpdateNode() =
+        DataFlow::getFieldQualifier(any(FieldWrite f | f.getEnclosingCallable() = init)) and
+      n2.(DataFlow::InstanceParameterNode).getCallable() = receiver and
+      receiver.getDeclaringType() = init.getDeclaringType() and
+      (
+        receiver instanceof AsyncTaskRunInBackgroundMethod or
+        receiver instanceof AsyncTaskOnPostExecuteMethod
+      )
+    )
+  }
+}
+
 /**
  * The Android class `android.os.AsyncTask`.
  */
@@ -24,29 +67,38 @@ private class AsyncTask extends RefType {
   AsyncTask() { this.hasQualifiedName("android.os", "AsyncTask") }
 }
 
+/** The constructor or initializer method of the `android.os.AsyncTask` class. */
+private class AsyncTaskInit extends Callable {
+  AsyncTaskInit() {
+    this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and
+    (this instanceof Constructor or this instanceof InitializerMethod)
+  }
+}
+
 /** A call to the `execute` or `executeOnExecutor` methods of the `android.os.AsyncTask` class. */
 private class ExecuteAsyncTaskMethodAccess extends MethodAccess {
-  Argument paramsArgument;
-
   ExecuteAsyncTaskMethodAccess() {
-    exists(Method m |
-      this.getMethod() = m and
-      m.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask
-    |
-      m.getName() = "execute" and not m.isStatic() and paramsArgument = this.getArgument(0)
-      or
-      m.getName() = "executeOnExecutor" and paramsArgument = this.getArgument(1)
-    )
+    this.getMethod().hasName(["execute", "executeOnExecutor"]) and
+    this.getMethod().getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof
+      AsyncTask
   }
 
   /** Returns the `params` argument of this call. */
-  Argument getParamsArgument() { result = paramsArgument }
+  Argument getParamsArgument() { result = this.getAnArgument() and result.isVararg() }
 }
 
 /** The `doInBackground` method of the `android.os.AsyncTask` class. */
 private class AsyncTaskRunInBackgroundMethod extends Method {
   AsyncTaskRunInBackgroundMethod() {
     this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and
-    this.getName() = "doInBackground"
+    this.hasName("doInBackground")
+  }
+}
+
+/** The `onPostExecute` method of the `android.os.AsyncTask` class. */
+private class AsyncTaskOnPostExecuteMethod extends Method {
+  AsyncTaskOnPostExecuteMethod() {
+    this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof AsyncTask and
+    this.hasName("onPostExecute")
   }
 }

java/ql/lib/semmle/code/java/frameworks/android/Intent.qll

@@ -283,7 +283,7 @@ private class IntentBundleFlowSteps extends SummaryModelCsv {
         "android.os;Bundle;true;putStringArrayList;;;Argument[1];Argument[-1].MapValue;value;manual",
         "android.os;Bundle;true;readFromParcel;;;Argument[0];Argument[-1].MapKey;taint;manual",
         "android.os;Bundle;true;readFromParcel;;;Argument[0];Argument[-1].MapValue;taint;manual",
-        // currently only the Extras part of the intent and the data field are fully modelled
+        // currently only the Extras part of the intent and the data field are fully modeled
         "android.content;Intent;false;Intent;(Intent);;Argument[0].SyntheticField[android.content.Intent.extras].MapKey;Argument[-1].SyntheticField[android.content.Intent.extras].MapKey;value;manual",
         "android.content;Intent;false;Intent;(Intent);;Argument[0].SyntheticField[android.content.Intent.extras].MapValue;Argument[-1].SyntheticField[android.content.Intent.extras].MapValue;value;manual",
         "android.content;Intent;false;Intent;(String,Uri);;Argument[1];Argument[-1].SyntheticField[android.content.Intent.data];value;manual",

java/ql/lib/semmle/code/java/frameworks/guava/Cache.qll

@@ -13,7 +13,7 @@ private class GuavaBaseCsv extends SummaryModelCsv {
         // lambda flow from Argument[1] not implemented
         "com.google.common.cache;Cache;true;get;(Object,Callable);;Argument[-1].MapValue;ReturnValue;value;manual",
         "com.google.common.cache;Cache;true;getIfPresent;(Object);;Argument[-1].MapValue;ReturnValue;value;manual",
-        // the true flow to MapKey of ReturnValue for getAllPresent is the intersection of the these inputs, but intersections cannot be modelled fully accurately.
+        // the true flow to MapKey of ReturnValue for getAllPresent is the intersection of the these inputs, but intersections cannot be modeled fully accurately.
         "com.google.common.cache;Cache;true;getAllPresent;(Iterable);;Argument[-1].MapKey;ReturnValue.MapKey;value;manual",
         "com.google.common.cache;Cache;true;getAllPresent;(Iterable);;Argument[0].Element;ReturnValue.MapKey;value;manual",
         "com.google.common.cache;Cache;true;getAllPresent;(Iterable);;Argument[-1].MapValue;ReturnValue.MapValue;value;manual",

java/ql/lib/semmle/code/java/frameworks/guava/Collections.qll

@@ -13,8 +13,8 @@ private class GuavaCollectCsv extends SummaryModelCsv {
     row =
       [
         //"package;type;overrides;name;signature;ext;inputspec;outputspec;kind",
-        // Methods depending on lambda flow are not currently modelled
-        // Methods depending on stronger aliasing properties than we support are also not modelled.
+        // Methods depending on lambda flow are not currently modeled
+        // Methods depending on stronger aliasing properties than we support are also not modeled.
         "com.google.common.collect;ArrayListMultimap;true;create;(Multimap);;Argument[0].MapKey;ReturnValue.MapKey;value;manual",
         "com.google.common.collect;ArrayListMultimap;true;create;(Multimap);;Argument[0].MapValue;ReturnValue.MapValue;value;manual",
         "com.google.common.collect;ArrayTable;true;create;(Iterable,Iterable);;Argument[0].Element;ReturnValue.SyntheticField[com.google.common.collect.Table.rowKey];value;manual",

java/ql/lib/semmle/code/java/frameworks/spring/SpringHttp.qll

@@ -160,7 +160,7 @@ private class SpringXssSink extends XSS::XssSink {
     |
       // If a Spring request mapping method is either annotated with @ResponseBody (or equivalent),
       // or returns a HttpEntity or sub-type, then the return value of the method is converted into
-      // a HTTP reponse using a HttpMessageConverter implementation. The implementation is chosen
+      // a HTTP response using a HttpMessageConverter implementation. The implementation is chosen
       // based on the return type of the method, and the Accept header of the request.
       //
       // By default, the only message converter which produces a response which is vulnerable to

java/ql/lib/semmle/code/java/regex/RegexTreeView.qll

@@ -6,7 +6,7 @@ private import semmle.code.java.regex.regex
 /**
  * An element containing a regular expression term, that is, either
  * a string literal (parsed as a regular expression; the root of the parse tree)
- * or another regular expression term (a decendent of the root).
+ * or another regular expression term (a descendant of the root).
  *
  * For sequences and alternations, we require at least two children.
  * Otherwise, we wish to represent the term differently.
@@ -52,7 +52,7 @@ private newtype TRegExpParent =
 /**
  * An element containing a regular expression term, that is, either
  * a string literal (parsed as a regular expression; the root of the parse tree)
- * or another regular expression term (a decendent of the root).
+ * or another regular expression term (a descendant of the root).
  */
 class RegExpParent extends TRegExpParent {
   /** Gets a textual representation of this element. */

java/ql/lib/semmle/code/java/regex/regex.qll

@@ -62,7 +62,7 @@ abstract class RegexString extends StringLiteral {
 
   /**
    * Helper predicate for `quote`.
-   * Holds if the char at `pos` is the one-based `index`th occurence of a quote delimiter (`\Q` or `\E`)
+   * Holds if the char at `pos` is the one-based `index`th occurrence of a quote delimiter (`\Q` or `\E`)
    * Result is `true` for `\Q` and `false` for `\E`.
    */
   private boolean quoteDelimiter(int index, int pos) {

java/ql/lib/semmle/code/java/security/SensitiveLoggingQuery.qll

@@ -28,4 +28,6 @@ class SensitiveLoggerConfiguration extends TaintTracking::Configuration {
   override predicate isSanitizer(DataFlow::Node sanitizer) {
     sanitizer.asExpr() instanceof LiveLiteral
   }
+
+  override predicate isSanitizerIn(Node node) { isSource(node) }
 }