Java: Insecure JXBrowser

java/ql/src/experimental/Security/CWE/CWE-295/JxBrowserWithoutCertValidation.java

@@ -0,0 +1,23 @@
+public static void main(String[] args) {
+	{
+		Browser browser = new Browser();
+		browser.loadURL("https://example.com");
+		// no further calls
+		// BAD: The browser ignores any certificate error by default!
+	}
+
+	{
+		Browser browser = new Browser();
+		browser.setLoadHandler(new LoadHandler() {
+			public boolean onLoad(LoadParams params) {
+				return true;
+			}
+
+			public boolean onCertificateError(CertificateErrorParams params){
+				return true; // GOOD: This means that loading will be cancelled on certificate errors
+			}
+		}); // GOOD: A secure `LoadHandler` is used.
+		browser.loadURL("https://example.com");
+
+	}
+}

java/ql/src/experimental/Security/CWE/CWE-295/JxBrowserWithoutCertValidation.qhelp

@@ -0,0 +1,31 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>JxBrowser is a Java library that allows to embed the Chromium browser inside Java applications.
+Versions smaller than 6.24 by default ignore any HTTPS certificate errors thereby allowing man-in-the-middle attacks.
+</p>
+</overview>
+
+<recommendation>
+<p>Do either of these:
+<li>Update to version 6.24 or 7.x.x as these correctly reject certificate errors by default.</li>
+<li>Add a custom implementation of the <code>LoadHandler</code> interface whose <code>onCertificateError</code> method always returns <b>true</b> indicating that loading should be cancelled.
+Then use the <code>setLoadHandler</code> method with your custom <code>LoadHandler</code> on every <code>Browser</code> you use.</li>
+</p>
+</recommendation>
+
+<example>
+<p>The following two examples show two ways of using a <code>Browser</code>. In the 'BAD' case,
+all certificate errors are ignored. In the 'GOOD' case, certificate errors are rejected.</p>
+<sample src="JxBrowserWithoutCertValidation.java" />
+</example>
+
+<references>
+<li>Teamdev:
+<a href="https://jxbrowser.support.teamdev.com/support/discussions/topics/9000051708">
+Changelog of JxBrowser 6.24</a>.</li>
+</references>
+</qhelp>

java/ql/src/experimental/Security/CWE/CWE-295/JxBrowserWithoutCertValidation.ql

@@ -0,0 +1,86 @@
+/**
+ * @name JxBrowser with disabled certificate validation
+ * @description Insecure configuration of JxBrowser disables certificate validation making the app vulnerable to man-in-the-middle attacks.
+ * @kind problem
+ * @id java/jxbrowser/disabled-certificate-validation
+ * @tags security
+ *       external/cwe-295
+ */
+
+import java
+import semmle.code.java.security.Encryption
+import semmle.code.java.dataflow.DataFlow
+
+/*
+ * This query is version specific to JxBrowser < 6.24. The version is indirectly detected.
+ * In version 6.x.x the `Browser` class is in a different package compared to version 7.x.x.
+ */
+
+/**
+ * Holds if a safe JxBrowser 6.x.x version is used, such as version 6.24.
+ * This is detected by the the presence of the `addBoundsListener` in the `Browser` class.
+ */
+private predicate isSafeJxBrowserVersion() {
+  exists(Method m | m.getDeclaringType() instanceof JxBrowser | m.hasName("addBoundsListener"))
+}
+
+/** The `com.teamdev.jxbrowser.chromium.Browser` class. */
+private class JxBrowser extends RefType {
+  JxBrowser() { this.hasQualifiedName("com.teamdev.jxbrowser.chromium", "Browser") }
+}
+
+/** The `setLoadHandler` method on the `com.teamdev.jxbrowser.chromium.Browser` class. */
+private class JxBrowserSetLoadHandler extends Method {
+  JxBrowserSetLoadHandler() {
+    this.hasName("setLoadHandler") and this.getDeclaringType() instanceof JxBrowser
+  }
+}
+
+/** The `com.teamdev.jxbrowser.chromium.LoadHandler` interface. */
+private class JxBrowserLoadHandler extends RefType {
+  JxBrowserLoadHandler() { this.hasQualifiedName("com.teamdev.jxbrowser.chromium", "LoadHandler") }
+}
+
+private predicate isOnCertificateErrorMethodSafe(Method m) {
+  forex(ReturnStmt rs | rs.getEnclosingCallable() = m |
+    rs.getResult().(CompileTimeConstantExpr).getBooleanValue() = true
+  )
+}
+
+/** A class that securely implements the `com.teamdev.jxbrowser.chromium.LoadHandler` interface. */
+private class JxBrowserSafeLoadHandler extends RefType {
+  JxBrowserSafeLoadHandler() {
+    this.getASupertype() instanceof JxBrowserLoadHandler and
+    exists(Method m | m.hasName("onCertificateError") and m.getDeclaringType() = this |
+      isOnCertificateErrorMethodSafe(m)
+    )
+  }
+}
+
+/**
+ * Models flow from the source `new Browser()` to a sink `browser.setLoadHandler(loadHandler)` where `loadHandler`
+ * has been determined to be safe.
+ */
+private class JxBrowserFlowConfiguration extends DataFlow::Configuration {
+  JxBrowserFlowConfiguration() { this = "JxBrowserFlowConfiguration" }
+
+  override predicate isSource(DataFlow::Node src) {
+    exists(ClassInstanceExpr newJxBrowser | newJxBrowser.getConstructedType() instanceof JxBrowser |
+      newJxBrowser = src.asExpr()
+    )
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess ma | ma.getMethod() instanceof JxBrowserSetLoadHandler |
+      ma.getArgument(0).getType() instanceof JxBrowserSafeLoadHandler and
+      ma.getQualifier() = sink.asExpr()
+    )
+  }
+}
+
+from JxBrowserFlowConfiguration cfg, DataFlow::Node src
+where
+  cfg.isSource(src) and
+  not cfg.hasFlow(src, _) and
+  not isSafeJxBrowserVersion()
+select src, "This JxBrowser instance may not check HTTPS certificates."

java/ql/test/experimental/query-tests/security/CWE-295/jxbrowser-6.23.1/JxBrowserWithoutCertValidation.expected

@@ -0,0 +1 @@
+| JxBrowserWithoutCertValidationV6_23_1.java:17:27:17:39 | new Browser(...) | This JxBrowser instance allows man-in-the-middle attacks. |

java/ql/test/experimental/query-tests/security/CWE-295/jxbrowser-6.23.1/JxBrowserWithoutCertValidation.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-295/JxBrowserWithoutCertValidation.ql

java/ql/test/experimental/query-tests/security/CWE-295/jxbrowser-6.23.1/JxBrowserWithoutCertValidationV6_23_1.java

@@ -0,0 +1,36 @@
+import com.teamdev.jxbrowser.chromium.Browser;
+import com.teamdev.jxbrowser.chromium.LoadHandler;
+import com.teamdev.jxbrowser.chromium.LoadParams;
+import com.teamdev.jxbrowser.chromium.CertificateErrorParams;
+
+public class JxBrowserWithoutCertValidationV6_23_1 {
+
+    public static void main(String[] args) {
+
+        badUsage();
+
+        goodUsage();
+
+    }
+
+    private static void badUsage() {
+        Browser browser = new Browser();
+        browser.loadURL("https://example.com");
+        // no further calls
+        // BAD: The browser ignores any certificate error by default!
+    }
+
+    private static void goodUsage() {
+        Browser browser = new Browser();
+        browser.setLoadHandler(new LoadHandler() {
+            public boolean onLoad(LoadParams params) {
+                return true;
+            }
+
+            public boolean onCertificateError(CertificateErrorParams params) {
+                return true; // GOOD: This means that loading will be cancelled on certificate errors
+            }
+        }); // GOOD: A secure `LoadHandler` is used.
+        browser.loadURL("https://example.com");
+    }
+}

java/ql/test/experimental/query-tests/security/CWE-295/jxbrowser-6.23.1/options

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

java/ql/test/experimental/query-tests/security/CWE-295/jxbrowser-6.24/JxBrowserWithoutCertValidation.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-295/JxBrowserWithoutCertValidation.ql

java/ql/test/experimental/query-tests/security/CWE-295/jxbrowser-6.24/JxBrowserWithoutCertValidationV6_24.java

@@ -0,0 +1,36 @@
+import com.teamdev.jxbrowser.chromium.Browser;
+import com.teamdev.jxbrowser.chromium.LoadHandler;
+import com.teamdev.jxbrowser.chromium.LoadParams;
+import com.teamdev.jxbrowser.chromium.CertificateErrorParams;
+
+public class JxBrowserWithoutCertValidationV6_24 {
+
+    public static void main(String[] args) {
+
+        goodUsage();
+
+        goodUsage2();
+
+    }
+
+    private static void goodUsage() {
+        Browser browser = new Browser();
+        browser.loadURL("https://example.com");
+        // no further calls
+        // GOOD: On version 6.24 the browser properly validates certificates by default!
+    }
+
+    private static void goodUsage2() {
+        Browser browser = new Browser();
+        browser.setLoadHandler(new LoadHandler() {
+            public boolean onLoad(LoadParams params) {
+                return true;
+            }
+
+            public boolean onCertificateError(CertificateErrorParams params) {
+                return true; // GOOD: This means that loading will be cancelled on certificate errors
+            }
+        }); // GOOD: A secure `LoadHandler` is used.
+        browser.loadURL("https://example.com");
+    }
+}

java/ql/test/experimental/query-tests/security/CWE-295/jxbrowser-6.24/options

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

java/ql/test/experimental/stubs/jxbrowser-6.23.1/com/teamdev/jxbrowser/chromium/Browser.java

@@ -0,0 +1,9 @@
+package com.teamdev.jxbrowser.chromium;
+
+public class Browser extends java.lang.Object {
+    public void setLoadHandler(LoadHandler handler) {
+    }
+
+    public void loadURL(String url) {
+    }
+}

java/ql/test/experimental/stubs/jxbrowser-6.23.1/com/teamdev/jxbrowser/chromium/CertificateErrorParams.java

@@ -0,0 +1,5 @@
+package com.teamdev.jxbrowser.chromium;
+
+public final class CertificateErrorParams extends Object {
+
+}

java/ql/test/experimental/stubs/jxbrowser-6.23.1/com/teamdev/jxbrowser/chromium/LoadHandler.java

@@ -0,0 +1,7 @@
+package com.teamdev.jxbrowser.chromium;
+
+public interface LoadHandler {
+    boolean onCertificateError(CertificateErrorParams params);
+
+    boolean onLoad(LoadParams params);
+}

java/ql/test/experimental/stubs/jxbrowser-6.23.1/com/teamdev/jxbrowser/chromium/LoadParams.java

@@ -0,0 +1,5 @@
+package com.teamdev.jxbrowser.chromium;
+
+public final class LoadParams extends Object {
+
+}

java/ql/test/experimental/stubs/jxbrowser-6.24/com/teamdev/jxbrowser/chromium/BoundsListener.java

@@ -0,0 +1,5 @@
+package com.teamdev.jxbrowser.chromium;
+
+public interface BoundsListener {
+
+}

java/ql/test/experimental/stubs/jxbrowser-6.24/com/teamdev/jxbrowser/chromium/Browser.java

@@ -0,0 +1,13 @@
+package com.teamdev.jxbrowser.chromium;
+
+public class Browser extends java.lang.Object {
+    public void setLoadHandler(LoadHandler handler) {
+    }
+
+    public void loadURL(String url) {
+    }
+
+    public void addBoundsListener(BoundsListener listener) {
+
+    }
+}

java/ql/test/experimental/stubs/jxbrowser-6.24/com/teamdev/jxbrowser/chromium/CertificateErrorParams.java

@@ -0,0 +1,5 @@
+package com.teamdev.jxbrowser.chromium;
+
+public final class CertificateErrorParams extends Object {
+
+}

java/ql/test/experimental/stubs/jxbrowser-6.24/com/teamdev/jxbrowser/chromium/LoadHandler.java

@@ -0,0 +1,7 @@
+package com.teamdev.jxbrowser.chromium;
+
+public interface LoadHandler {
+    boolean onCertificateError(CertificateErrorParams params);
+
+    boolean onLoad(LoadParams params);
+}

java/ql/test/experimental/stubs/jxbrowser-6.24/com/teamdev/jxbrowser/chromium/LoadParams.java

@@ -0,0 +1,5 @@
+package com.teamdev.jxbrowser.chromium;
+
+public final class LoadParams extends Object {
+
+}