Improvements to UnsafeAndroidAccess

java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll

@@ -86,6 +86,7 @@ private module Frameworks {
   private import semmle.code.java.frameworks.android.Slice
   private import semmle.code.java.frameworks.android.SQLite
   private import semmle.code.java.frameworks.android.Widget
+  private import semmle.code.java.frameworks.android.WebView
   private import semmle.code.java.frameworks.android.XssSinks
   private import semmle.code.java.frameworks.ApacheHttp
   private import semmle.code.java.frameworks.apache.Collections

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

@@ -1,17 +1,23 @@
 import java
+private import semmle.code.java.dataflow.DataFlow
+private import semmle.code.java.dataflow.ExternalFlow
 
+/** The class `android.webkit.WebView`. */
 class TypeWebView extends Class {
   TypeWebView() { this.hasQualifiedName("android.webkit", "WebView") }
 }
 
+/** The class `android.webkit.WebViewClient`. */
 class TypeWebViewClient extends Class {
   TypeWebViewClient() { this.hasQualifiedName("android.webkit", "WebViewClient") }
 }
 
+/** The class `android.webkit.WebSettings`. */
 class TypeWebSettings extends Class {
   TypeWebSettings() { this.hasQualifiedName("android.webkit", "WebSettings") }
 }
 
+/** The method `getSettings` of the class `android.webkit.WebView`. */
 class WebViewGetSettingsMethod extends Method {
   WebViewGetSettingsMethod() {
     this.hasName("getSettings") and
@@ -19,6 +25,7 @@ class WebViewGetSettingsMethod extends Method {
   }
 }
 
+/** The method `loadUrl` or `postUrl` of the class `android.webkit.WebView`. */
 class WebViewLoadUrlMethod extends Method {
   WebViewLoadUrlMethod() {
     this.getDeclaringType() instanceof TypeWebView and
@@ -26,9 +33,108 @@ class WebViewLoadUrlMethod extends Method {
   }
 }
 
+/** The method `getUrl` or `getOriginalUrl` of the class `android.webkit.WebView`. */
 class WebViewGetUrlMethod extends Method {
   WebViewGetUrlMethod() {
     this.getDeclaringType() instanceof TypeWebView and
     (this.getName() = "getUrl" or this.getName() = "getOriginalUrl")
   }
 }
+
+/**
+ * A method allowing any-local-file and cross-origin access in the class `android.webkit.WebSettings`.
+ */
+class CrossOriginAccessMethod extends Method {
+  CrossOriginAccessMethod() {
+    this.getDeclaringType() instanceof TypeWebSettings and
+    this.hasName(["setAllowUniversalAccessFromFileURLs", "setAllowFileAccessFromFileURLs"])
+  }
+}
+
+/**
+ * The method `setJavaScriptEnabled` of the class `android.webkit.WebSettings`.
+ */
+class AllowJavaScriptMethod extends Method {
+  AllowJavaScriptMethod() {
+    this.getDeclaringType() instanceof TypeWebSettings and
+    this.hasName("setJavaScriptEnabled")
+  }
+}
+
+/** The method `setWebViewClient` of the class `android.webkit.WebView`. */
+class WebViewSetWebViewClientMethod extends Method {
+  WebViewSetWebViewClientMethod() {
+    this.getDeclaringType() instanceof TypeWebView and
+    this.hasName("setWebViewClient")
+  }
+}
+
+/** The method `shouldOverrideUrlLoading` of the class `android.webkit.WebViewClient`. */
+class ShouldOverrideUrlLoading extends Method {
+  ShouldOverrideUrlLoading() {
+    this.getDeclaringType().getASupertype*() instanceof TypeWebViewClient and
+    this.hasName("shouldOverrideUrlLoading")
+  }
+}
+
+/**
+ * Holds if `webview` is a `WebView` and its option `setJavascriptEnabled`
+ * has been set to `true` via a `WebSettings` object obtained from it.
+ */
+predicate isJSEnabled(Expr webview) {
+  webview.getType().(RefType).getASupertype*() instanceof TypeWebView and
+  exists(MethodAccess allowJs |
+    allowJs.getMethod() instanceof AllowJavaScriptMethod and
+    allowJs.getArgument(0).(CompileTimeConstantExpr).getBooleanValue() = true and
+    exists(MethodAccess settings |
+      settings.getMethod() instanceof WebViewGetSettingsMethod and
+      DataFlow::localExprFlow(settings, allowJs.getQualifier()) and
+      DataFlow::localExprFlow(webview, settings.getQualifier())
+    )
+  )
+}
+
+/**
+ * Holds if `webview` is a `WebView` and its options `setAllowUniversalAccessFromFileURLs` or
+ * `setAllowFileAccessFromFileURLs` have been set to `true`.
+ */
+predicate isAllowFileAccessEnabled(Expr webview) {
+  exists(MethodAccess allowFileAccess |
+    allowFileAccess.getMethod() instanceof CrossOriginAccessMethod and
+    allowFileAccess.getArgument(0).(CompileTimeConstantExpr).getBooleanValue() = true and
+    exists(MethodAccess settings |
+      settings.getMethod() instanceof WebViewGetSettingsMethod and
+      DataFlow::localExprFlow(settings, allowFileAccess.getQualifier()) and
+      DataFlow::localExprFlow(webview, settings.getQualifier())
+    )
+  )
+}
+
+private class WebkitSourceModels extends SourceModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        "android.webkit;WebResourceRequest;true;doUpdateVisitedHistory;;;Parameter[1];remote",
+        "android.webkit;WebResourceRequest;true;onLoadResource;;;Parameter[1];remote",
+        "android.webkit;WebResourceRequest;true;onPageCommitVisible;;;Parameter[1];remote",
+        "android.webkit;WebResourceRequest;true;onPageFinished;;;Parameter[1];remote",
+        "android.webkit;WebResourceRequest;true;onPageStarted;;;Parameter[1];remote",
+        "android.webkit;WebResourceRequest;true;onReceivedError;(WebView,int,String,String);;Parameter[3];remote",
+        "android.webkit;WebResourceRequest;true;onReceivedError;(WebView,WebResourceRequest,WebResourceError);;Parameter[1];remote",
+        "android.webkit;WebResourceRequest;true;onReceivedHttpError;;;Parameter[1];remote",
+        "android.webkit;WebResourceRequest;true;onSafeBrowsingHit;;;Parameter[1];remote",
+        "android.webkit;WebResourceRequest;true;shouldInterceptRequest;;;Parameter[1];remote",
+        "android.webkit;WebResourceRequest;true;shouldOverrideUrlLoading;;;Parameter[1];remote"
+      ]
+  }
+}
+
+private class WebkitSummaryModels extends SummaryModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        "android.webkit;WebResourceRequest;true;getRequestHeaders;;;Argument[-1];ReturnValue;taint",
+        "android.webkit;WebResourceRequest;true;getUrl;;;Argument[-1];ReturnValue;taint"
+      ]
+  }
+}

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

@@ -26,11 +26,9 @@ abstract class UrlResourceSink extends DataFlow::Node {
  */
 private class CrossOriginUrlResourceSink extends JavaScriptEnabledUrlResourceSink {
   CrossOriginUrlResourceSink() {
-    exists(Variable settings, MethodAccess ma |
-      webViewLoadUrl(this.asExpr(), settings) and
-      ma.getMethod() instanceof CrossOriginAccessMethod and
-      ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true and
-      ma.getQualifier() = settings.getAnAccess()
+    exists(WebViewRef webview |
+      webViewLoadUrl(this.asExpr(), webview.getAnAccess()) and
+      isAllowFileAccessEnabled(webview.getAnAccess())
     )
   }
 
@@ -44,57 +42,75 @@ private class CrossOriginUrlResourceSink extends JavaScriptEnabledUrlResourceSin
  */
 private class JavaScriptEnabledUrlResourceSink extends UrlResourceSink {
   JavaScriptEnabledUrlResourceSink() {
-    exists(Variable settings |
-      webViewLoadUrl(this.asExpr(), settings) and
-      isJSEnabled(settings)
+    exists(WebViewRef webview |
+      isJSEnabled(webview.getAnAccess()) and
+      webViewLoadUrl(this.asExpr(), webview.getAnAccess())
     )
   }
 
   override string getSinkType() { result = "user input vulnerable to XSS attacks" }
 }
 
+private class WebViewRef extends Element {
+  WebViewRef() {
+    this.(RefType).getASourceSupertype*() instanceof TypeWebView or
+    this.(Variable).getType().(RefType).getASourceSupertype*() instanceof TypeWebView
+  }
+
+  /** Gets an access to this WebView. */
+  Expr getAnAccess() {
+    exists(ThisAccess t | t.getType() = this and result = t |
+      t.isOwnInstanceAccess() or
+      t.isEnclosingInstanceAccess(this)
+    )
+    or
+    result = this.(Variable).getAnAccess()
+  }
+}
+
+private Expr getUnderlyingExpr(Expr e) {
+  if e instanceof CastExpr or e instanceof UnaryExpr
+  then
+    result = getUnderlyingExpr(e.(CastExpr).getExpr()) or
+    result = getUnderlyingExpr(e.(UnaryExpr).getExpr())
+  else result = e
+}
+
 /**
- * Holds if a `WebViewLoadUrlMethod` method is called with the given `urlArg` on a
- * WebView with settings stored in `settings`.
+ * Holds if `WebViewLoadUrlMethod` is called on `webview`
+ * with `urlArg` as its first argument.
  */
-private predicate webViewLoadUrl(Expr urlArg, Variable settings) {
-  exists(MethodAccess loadUrl, Variable webview, MethodAccess getSettings |
+private predicate webViewLoadUrl(Argument urlArg, Expr webview) {
+  exists(MethodAccess loadUrl |
     loadUrl.getArgument(0) = urlArg and
-    loadUrl.getMethod() instanceof WebViewLoadUrlMethod and
-    loadUrl.getQualifier() = webview.getAnAccess() and
-    getSettings.getMethod() instanceof WebViewGetSettingsMethod and
-    webview.getAnAccess() = getSettings.getQualifier() and
-    settings.getAnAssignedValue() = getSettings
+    loadUrl.getMethod() instanceof WebViewLoadUrlMethod
+  |
+    getUnderlyingExpr(loadUrl.getQualifier()) = webview
+    or
+    // `webview` is received as a parameter of an event method in a custom `WebViewClient`,
+    // so we need to find WebViews that use that specific `WebViewClient`.
+    exists(WebViewClientEventMethod eventMethod, MethodAccess setWebClient |
+      setWebClient.getMethod() instanceof WebViewSetWebViewClientMethod and
+      setWebClient.getArgument(0).getType() = eventMethod.getDeclaringType() and
+      getUnderlyingExpr(setWebClient.getQualifier()) = webview and
+      getUnderlyingExpr(loadUrl.getQualifier()) = eventMethod.getWebViewParameter().getAnAccess()
+    )
   )
 }
 
-/**
- * A method allowing any-local-file and cross-origin access in the WebSettings class.
- */
-private class CrossOriginAccessMethod extends Method {
-  CrossOriginAccessMethod() {
-    this.getDeclaringType() instanceof TypeWebSettings and
-    this.hasName(["setAllowUniversalAccessFromFileURLs", "setAllowFileAccessFromFileURLs"])
+/** A method of the class `WebViewClient` that handles an event. */
+private class WebViewClientEventMethod extends Method {
+  WebViewClientEventMethod() {
+    this.getDeclaringType().getASupertype*() instanceof TypeWebViewClient and
+    this.hasName([
+        "shouldOverrideUrlLoading", "shouldInterceptRequest", "onPageStarted", "onPageFinished",
+        "onLoadResource", "onPageCommitVisible", "onTooManyRedirects"
+      ])
+  }
+
+  /** Gets a `WebView` parameter of this method. */
+  Parameter getWebViewParameter() {
+    result = this.getAParameter() and
+    result.getType() instanceof TypeWebView
   }
 }
-
-/**
- * The `setJavaScriptEnabled` method for the webview.
- */
-private class AllowJavaScriptMethod extends Method {
-  AllowJavaScriptMethod() {
-    this.getDeclaringType() instanceof TypeWebSettings and
-    this.hasName("setJavaScriptEnabled")
-  }
-}
-
-/**
- * Holds if a call to `v.setJavaScriptEnabled(true)` exists.
- */
-private predicate isJSEnabled(Variable v) {
-  exists(MethodAccess jsa |
-    v.getAnAccess() = jsa.getQualifier() and
-    jsa.getMethod() instanceof AllowJavaScriptMethod and
-    jsa.getArgument(0).(BooleanLiteral).getBooleanValue() = true
-  )
-}