Merge pull request #9663 from joefarebrother/android-certificate-validation

Java: Add query for improper webview certificate validation

Author: joefarebrother

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

@@ -0,0 +1,29 @@
+/** Definitions for the web view certificate validation query */
+
+import java
+
+/** A method that overrides `WebViewClient.onReceivedSslError` */
+class OnReceivedSslErrorMethod extends Method {
+  OnReceivedSslErrorMethod() {
+    this.overrides*(any(Method m |
+        m.hasQualifiedName("android.webkit", "WebViewClient", "onReceivedSslError")
+      ))
+  }
+
+  /** Gets the `SslErrorHandler` argument to this method. */
+  Parameter handlerArg() { result = this.getParameter(1) }
+}
+
+/** A call to `SslErrorHandler.proceed` */
+private class SslProceedCall extends MethodAccess {
+  SslProceedCall() {
+    this.getMethod().hasQualifiedName("android.webkit", "SslErrorHandler", "proceed")
+  }
+}
+
+/** Holds if `m` trusts all certificates by calling `SslErrorHandler.proceed` unconditionally. */
+predicate trustsAllCerts(OnReceivedSslErrorMethod m) {
+  exists(SslProceedCall pr | pr.getQualifier().(VarAccess).getVariable() = m.handlerArg() |
+    pr.getBasicBlock().bbPostDominates(m.getBody().getBasicBlock())
+  )
+}

java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertificateValidation.java

@@ -0,0 +1,22 @@
+class Bad extends WebViewClient {
+    // BAD: All certificates are trusted.
+    public void onReceivedSslError (WebView view, SslErrorHandler handler, SslError error) { // $hasResult
+        handler.proceed(); 
+    }
+}
+
+class Good extends WebViewClient {
+    PublicKey myPubKey = ...;
+
+    // GOOD: Only certificates signed by a certain public key are trusted.
+    public void onReceivedSslError (WebView view, SslErrorHandler handler, SslError error) { // $hasResult
+        try {
+            X509Certificate cert = error.getCertificate().getX509Certificate();
+            cert.verify(this.myPubKey);
+            handler.proceed();
+        }
+        catch (CertificateException|NoSuchAlgorithmException|InvalidKeyException|NoSuchProviderException|SignatureException e) {
+            handler.cancel();
+        }
+    }    
+}

java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertificateValidation.qhelp

@@ -0,0 +1,46 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+If the <code>onReceivedSslError</code> method of an Android <code>WebViewClient</code> always calls <code>proceed</code> on the given <code>SslErrorHandler</code>, it trusts any certificate. 
+This allows an attacker to perform a machine-in-the-middle attack against the application, therefore breaking any security Transport Layer Security (TLS) gives.
+</p>
+
+<p>
+An attack might look like this:
+</p>
+
+<ol>
+  <li>The vulnerable application connects to <code>https://example.com</code>.</li>
+  <li>The attacker intercepts this connection and presents a valid, self-signed certificate for <code>https://example.com</code>.</li>
+  <li>The vulnerable application calls the <code>onReceivedSslError</code> method to check whether it should trust the certificate.</li>
+  <li>The <code>onReceivedSslError</code> method of your <code>WebViewClient</code> calls <code>SslErrorHandler.proceed</code>.</li>
+  <li>The vulnerable application accepts the certificate and proceeds with the connection since your <code>WevViewClient</code> trusted it by proceeding.</li>
+  <li>The attacker can now read the data your application sends to <code>https://example.com</code> and/or alter its replies while the application thinks the connection is secure.</li>
+</ol>
+</overview>
+
+<recommendation>
+<p>
+Do not use a call <code>SslerrorHandler.proceed</code> unconditionally. 
+If you have to use a self-signed certificate, only accept that certificate, not all certificates. 
+</p>
+
+</recommendation>
+
+<example>
+<p>
+In the first (bad) example, the <code>WebViewClient</code> trusts all certificates by always calling <code>SslErrorHandler.proceed</code>. 
+In the second (good) example, only certificates signed by a certain public key are accepted.
+</p>
+<sample src="ImproperWebViewCertificateValidation.java" />
+</example>
+
+<references>
+<li>
+<a href="https://developer.android.com/reference/android/webkit/WebViewClient?hl=en#onReceivedSslError(android.webkit.WebView,%20android.webkit.SslErrorHandler,%20android.net.http.SslError)">WebViewClient.onReceivedSslError documentation</a>.
+</li>
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertificateValidation.ql

@@ -0,0 +1,18 @@
+/**
+ * @name Android `WebView` that accepts all certificates
+ * @description Trusting all certificates allows an attacker to perform a machine-in-the-middle attack.
+ * @kind problem
+ * @problem.severity error
+ * @security-severity 7.5
+ * @precision high
+ * @id java/improper-webview-certificate-validation
+ * @tags security
+ *       external/cwe/cwe-295
+ */
+
+import java
+import semmle.code.java.security.AndroidWebViewCertificateValidationQuery
+
+from OnReceivedSslErrorMethod m
+where trustsAllCerts(m)
+select m, "This handler accepts all SSL certificates."

java/ql/src/change-notes/2022-06-22-improper-webview-certificate-validation.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* A new query "Android `WebView` that accepts all certificates" (`java/improper-webview-certificate-validation`) has been added. This query finds implementations of `WebViewClient`s that accept all certificates in the case of an SSL error.

java/ql/test/query-tests/security/CWE-295/ImproperWebVeiwCertificateValidation/Test.java

@@ -0,0 +1,54 @@
+import android.webkit.WebViewClient;
+import android.webkit.WebView;
+import android.webkit.SslErrorHandler;
+import android.net.http.SslError;
+import android.net.http.SslCertificate;
+import android.app.AlertDialog;
+import android.content.DialogInterface;
+import android.app.Activity;
+
+class Test {
+    class A extends WebViewClient {
+        public void onReceivedSslError (WebView view, SslErrorHandler handler, SslError error) { // $hasResult
+            handler.proceed(); 
+        }
+    }
+
+    interface Validator {
+        boolean isValid(SslCertificate cert);
+    }
+
+    class B extends WebViewClient {
+        Validator v;
+
+        public void onReceivedSslError (WebView view, SslErrorHandler handler, SslError error) {
+            if (this.v.isValid(error.getCertificate())) {
+                handler.proceed();
+            }
+            else {
+                handler.cancel();
+            }
+        } 
+    }
+
+    class C extends WebViewClient {
+        Activity activity;
+
+        public void onReceivedSslError (WebView view, SslErrorHandler handler, SslError error) {
+            new AlertDialog.Builder(activity).
+                        setTitle("SSL error").
+                        setMessage("SSL error. Connect anyway?").
+                        setPositiveButton("Yes", new DialogInterface.OnClickListener() {
+                            @Override
+                            public void onClick(DialogInterface dialog, int which) {
+                                handler.proceed();
+                            }
+                        }).setNegativeButton("No", new DialogInterface.OnClickListener() {
+                            @Override
+                            public void onClick(DialogInterface dialog, int which) {
+                                handler.cancel();
+                            }
+                        }).show();
+        }
+    }
+}

java/ql/test/query-tests/security/CWE-295/ImproperWebVeiwCertificateValidation/options

@@ -0,0 +1 @@
+// semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/google-android-9.0.0

java/ql/test/query-tests/security/CWE-295/ImproperWebVeiwCertificateValidation/test.expected

@@ -0,0 +1,19 @@
+import java
+import semmle.code.java.security.AndroidWebViewCertificateValidationQuery
+import TestUtilities.InlineExpectationsTest
+
+class WebViewTest extends InlineExpectationsTest {
+  WebViewTest() { this = "WebViewTest" }
+
+  override string getARelevantTag() { result = "hasResult" }
+
+  override predicate hasActualResult(Location location, string element, string tag, string value) {
+    exists(OnReceivedSslErrorMethod m |
+      trustsAllCerts(m) and
+      location = m.getLocation() and
+      element = m.toString() and
+      tag = "hasResult" and
+      value = ""
+    )
+  }
+}