Merge remote-tracking branch 'remotes/origin/main' into marcono1234/statement-expression

java/ql/src/experimental/Security/CWE/CWE-200/AndroidWebResourceResponse.qll

@@ -0,0 +1,81 @@
+/** Provides Android methods relating to web resource response. */
+
+import java
+private import semmle.code.java.dataflow.DataFlow
+private import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.dataflow.FlowSteps
+private import semmle.code.java.frameworks.android.WebView
+
+/**
+ * The Android class `android.webkit.WebResourceRequest` for handling web requests.
+ */
+class WebResourceRequest extends RefType {
+  WebResourceRequest() { this.hasQualifiedName("android.webkit", "WebResourceRequest") }
+}
+
+/**
+ * The Android class `android.webkit.WebResourceResponse` for rendering web responses.
+ */
+class WebResourceResponse extends RefType {
+  WebResourceResponse() { this.hasQualifiedName("android.webkit", "WebResourceResponse") }
+}
+
+/** The `shouldInterceptRequest` method of a class implementing `WebViewClient`. */
+class ShouldInterceptRequestMethod extends Method {
+  ShouldInterceptRequestMethod() {
+    this.hasName("shouldInterceptRequest") and
+    this.getDeclaringType().getASupertype*() instanceof TypeWebViewClient
+  }
+}
+
+/** A method call to `WebView.setWebViewClient`. */
+class SetWebViewClientMethodAccess extends MethodAccess {
+  SetWebViewClientMethodAccess() {
+    this.getMethod().hasName("setWebViewClient") and
+    this.getMethod().getDeclaringType().getASupertype*() instanceof TypeWebView
+  }
+}
+
+/** A sink representing the data argument of a call to the constructor of `WebResourceResponse`. */
+class WebResourceResponseSink extends DataFlow::Node {
+  WebResourceResponseSink() {
+    exists(ConstructorCall cc |
+      cc.getConstructedType() instanceof WebResourceResponse and
+      (
+        this.asExpr() = cc.getArgument(2) and cc.getNumArgument() = 3 // WebResourceResponse(String mimeType, String encoding, InputStream data)
+        or
+        this.asExpr() = cc.getArgument(5) and cc.getNumArgument() = 6 // WebResourceResponse(String mimeType, String encoding, int statusCode, String reasonPhrase, Map<String, String> responseHeaders, InputStream data)
+      )
+    )
+  }
+}
+
+/**
+ * A value step from the URL argument of `WebView::loadUrl` to the URL parameter of
+ * `WebViewClient::shouldInterceptRequest`.
+ */
+private class FetchUrlStep extends AdditionalValueStep {
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(
+      // webview.loadUrl(url) -> webview.setWebViewClient(new WebViewClient() { shouldInterceptRequest(view, url) });
+      MethodAccess lma, ShouldInterceptRequestMethod im, SetWebViewClientMethodAccess sma
+    |
+      sma.getArgument(0).getType() = im.getDeclaringType().getASupertype*() and
+      lma.getMethod() instanceof WebViewLoadUrlMethod and
+      lma.getQualifier().getType() = sma.getQualifier().getType() and
+      pred.asExpr() = lma.getArgument(0) and
+      succ.asParameter() = im.getParameter(1)
+    )
+  }
+}
+
+/** Value/taint steps relating to url loading and file reading in an Android application. */
+private class LoadUrlSummaries extends SummaryModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        "java.io;FileInputStream;true;FileInputStream;;;Argument[0];Argument[-1];taint",
+        "android.webkit;WebResourceRequest;false;getUrl;;;Argument[-1];ReturnValue;taint"
+      ]
+  }
+}

java/ql/src/experimental/Security/CWE/CWE-200/InsecureWebResourceResponse.java

@@ -0,0 +1,17 @@
+// BAD: no URI validation
+Uri uri = Uri.parse(url);
+FileInputStream inputStream = new FileInputStream(uri.getPath());
+String mimeType = getMimeTypeFromPath(uri.getPath());
+return new WebResourceResponse(mimeType, "UTF-8", inputStream);
+
+
+// GOOD: check for a trusted prefix, ensuring path traversal is not used to erase that prefix:
+// (alternatively use `WebViewAssetsLoader`)
+if (uri.getPath().startsWith("/local_cache/") && !uri.getPath().contains("..")) {
+    File cacheFile = new File(getCacheDir(), uri.getLastPathSegment());
+    FileInputStream inputStream = new FileInputStream(cacheFile);
+    String mimeType = getMimeTypeFromPath(uri.getPath());
+    return new WebResourceResponse(mimeType, "UTF-8", inputStream);
+}
+
+return assetLoader.shouldInterceptRequest(request.getUrl());

java/ql/src/experimental/Security/CWE/CWE-200/InsecureWebResourceResponse.qhelp

@@ -0,0 +1,43 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Android provides a <code>WebResourceResponse</code> class, which allows an Android application to behave
+as a web server by handling requests of popular protocols such as <code>http(s)</code>, <code>file</code>,
+as well as <code>javascript</code> and returning a response (including status code, content type, content
+encoding, headers and the response body). Improper implementation with insufficient input validation can lead
+to leakage of sensitive configuration files or user data because requests could refer to paths intended to be
+application-private.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Unsanitized user-provided URLs must not be used to serve a response directly. When handling a request,
+always validate that the requested file path is not in the receiver's protected directory. Alternatively
+the Android class <code>WebViewAssetLoader</code> can be used, which safely processes data from resources,
+assets or a predefined directory.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following examples show a bad scenario and two good scenarios respectively. In the bad scenario, a
+response is served without path validation. In the good scenario, a response is either served with path
+validation or through the safe <code>WebViewAssetLoader</code> implementation.
+</p>
+<sample src="InsecureWebResourceResponse.java" />
+</example>
+
+<references>
+<li>
+Oversecured:
+<a href="https://blog.oversecured.com/Android-Exploring-vulnerabilities-in-WebResourceResponse/">Android: Exploring vulnerabilities in WebResourceResponse</a>.
+</li>
+<li>
+CVE:
+<a href="https://cordova.apache.org/announcements/2014/08/04/android-351.html">CVE-2014-3502: Cordova apps can potentially leak data to other apps via URL loading</a>.
+</li>
+</references>
+</qhelp>

java/ql/src/experimental/Security/CWE/CWE-200/InsecureWebResourceResponse.ql

@@ -0,0 +1,33 @@
+/**
+ * @name Insecure Android WebView Resource Response
+ * @description An insecure implementation of Android `WebResourceResponse` may lead to leakage of arbitrary
+ *               sensitive content.
+ * @kind path-problem
+ * @id java/insecure-webview-resource-response
+ * @problem.severity error
+ * @tags security
+ *       external/cwe/cwe-200
+ */
+
+import java
+import semmle.code.java.controlflow.Guards
+import experimental.semmle.code.java.PathSanitizer
+import AndroidWebResourceResponse
+import DataFlow::PathGraph
+
+class InsecureWebResourceResponseConfig extends TaintTracking::Configuration {
+  InsecureWebResourceResponseConfig() { this = "InsecureWebResourceResponseConfig" }
+
+  override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof WebResourceResponseSink }
+
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof PathTraversalBarrierGuard
+  }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, InsecureWebResourceResponseConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Leaking arbitrary content in Android from $@.",
+  source.getNode(), "this user input"

java/ql/src/experimental/Security/CWE/CWE-611/XXELib.qll

@@ -204,7 +204,7 @@ private class SafeDigesterFlowConfig extends DataFlow4::Configuration {
   override int fieldFlowBranchLimit() { result = 0 }
 }
 
-/** The class `java.beans.XmlDecoder`. */
+/** The class `java.beans.XMLDecoder`. */
 class XmlDecoder extends RefType {
   XmlDecoder() { this.hasQualifiedName("java.beans", "XMLDecoder") }
 }
@@ -212,7 +212,7 @@ class XmlDecoder extends RefType {
 /** DEPRECATED: Alias for XmlDecoder */
 deprecated class XMLDecoder = XmlDecoder;
 
-/** A call to `XmlDecoder.readObject`. */
+/** A call to `XMLDecoder.readObject`. */
 class XmlDecoderReadObject extends XmlParserCall {
   XmlDecoderReadObject() {
     exists(Method m |

java/ql/src/experimental/semmle/code/java/PathSanitizer.qll

@@ -20,13 +20,24 @@ private class ExactStringPathMatchGuard extends PathTraversalBarrierGuard instan
   }
 }
 
+/**
+ * Given input `e` = `v.method1(...).method2(...)...`, returns `v` where `v` is a `VarAccess`.
+ *
+ * This is used to look through field accessors such as `uri.getPath()`.
+ */
+private Expr getUnderlyingVarAccess(Expr e) {
+  result = getUnderlyingVarAccess(e.(MethodAccess).getQualifier())
+  or
+  result = e.(VarAccess)
+}
+
 private class AllowListGuard extends Guard instanceof MethodAccess {
   AllowListGuard() {
     (isStringPartialMatch(this) or isPathPartialMatch(this)) and
     not isDisallowedWord(super.getAnArgument())
   }
 
-  Expr getCheckedExpr() { result = super.getQualifier() }
+  Expr getCheckedExpr() { result = getUnderlyingVarAccess(super.getQualifier()) }
 }
 
 /**
@@ -73,7 +84,7 @@ private class BlockListGuard extends Guard instanceof MethodAccess {
     isDisallowedWord(super.getAnArgument())
   }
 
-  Expr getCheckedExpr() { result = super.getQualifier() }
+  Expr getCheckedExpr() { result = getUnderlyingVarAccess(super.getQualifier()) }
 }
 
 /**
@@ -144,7 +155,7 @@ class PathTraversalGuard extends Guard instanceof MethodAccess {
     super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".."
   }
 
-  Expr getCheckedExpr() { result = super.getQualifier() }
+  Expr getCheckedExpr() { result = getUnderlyingVarAccess(super.getQualifier()) }
 }
 
 /** A complementary sanitizer that protects against path traversal using path normalization. */