From f8ccbcba708c7f4d0a759b9b25eb5e3209aa7b60 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Wed, 22 Jun 2022 14:01:52 +0100
Subject: [PATCH] Add qhelp

---
 ...ImproperWebViewCertifiacteValidation.qhelp | 43 +++++++++++++++++++
 .../ImproperWebViewCertificateValidation.java | 22 ++++++++++
 2 files changed, 65 insertions(+)
 create mode 100644 java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertifiacteValidation.qhelp
 create mode 100644 java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertificateValidation.java

diff --git a/java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertifiacteValidation.qhelp b/java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertifiacteValidation.qhelp
new file mode 100644
index 00000000000..e00108ea51e
--- /dev/null
+++ b/java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertifiacteValidation.qhelp
@@ -0,0 +1,43 @@
+<!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> unconditonally. 
+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>
+</references>
+</qhelp>
diff --git a/java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertificateValidation.java b/java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertificateValidation.java
new file mode 100644
index 00000000000..358800c5746
--- /dev/null
+++ b/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();
+        }
+    }    
+}
\ No newline at end of file