Skip to content

Commit b749112

Browse files
aschackmullaschackmull
authored
Merge pull request github#4945 from intrigus-lgtm/java/insecure-jxbrowser
Java: Insecure JXBrowser
2 parents 18225fa + 5c82ff8 commit b749112

20 files changed

+279
-0
lines changed

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
public static void main(String[] args) {
2+
{
3+
Browser browser = new Browser();
4+
browser.loadURL("https://example.com");
5+
// no further calls
6+
// BAD: The browser ignores any certificate error by default!
7+
}
8+
9+
{
10+
Browser browser = new Browser();
11+
browser.setLoadHandler(new LoadHandler() {
12+
public boolean onLoad(LoadParams params) {
13+
return true;
14+
}
15+
16+
public boolean onCertificateError(CertificateErrorParams params){
17+
return true; // GOOD: This means that loading will be cancelled on certificate errors
18+
}
19+
}); // GOOD: A secure `LoadHandler` is used.
20+
browser.loadURL("https://example.com");
21+
22+
}
23+
}

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>JxBrowser is a Java library that allows to embed the Chromium browser inside Java applications.
8+
Versions smaller than 6.24 by default ignore any HTTPS certificate errors thereby allowing man-in-the-middle attacks.
9+
</p>
10+
</overview>
11+
12+
<recommendation>
13+
<p>Do either of these:</p>
14+
<ul>
15+
<li>Update to version 6.24 or 7.x.x as these correctly reject certificate errors by default.</li>
16+
<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.
17+
Then use the <code>setLoadHandler</code> method with your custom <code>LoadHandler</code> on every <code>Browser</code> you use.</li>
18+
</ul>
19+
</recommendation>
20+
21+
<example>
22+
<p>The following two examples show two ways of using a <code>Browser</code>. In the 'BAD' case,
23+
all certificate errors are ignored. In the 'GOOD' case, certificate errors are rejected.</p>
24+
<sample src="JxBrowserWithoutCertValidation.java" />
25+
</example>
26+
27+
<references>
28+
<li>Teamdev:
29+
<a href="https://jxbrowser.support.teamdev.com/support/discussions/topics/9000051708">
30+
Changelog of JxBrowser 6.24</a>.</li>
31+
</references>
32+
</qhelp>

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

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
/**
2+
* @name JxBrowser with disabled certificate validation
3+
* @description Insecure configuration of JxBrowser disables certificate validation making the app vulnerable to man-in-the-middle attacks.
4+
* @kind problem
5+
* @id java/jxbrowser/disabled-certificate-validation
6+
* @tags security
7+
* external/cwe/cwe-295
8+
*/
9+
10+
import java
11+
import semmle.code.java.security.Encryption
12+
import semmle.code.java.dataflow.DataFlow
13+
14+
/*
15+
* This query is version specific to JxBrowser < 6.24. The version is indirectly detected.
16+
* In version 6.x.x the `Browser` class is in a different package compared to version 7.x.x.
17+
*/
18+
19+
/**
20+
* Holds if a safe JxBrowser 6.x.x version is used, such as version 6.24.
21+
* This is detected by the the presence of the `addBoundsListener` in the `Browser` class.
22+
*/
23+
private predicate isSafeJxBrowserVersion() {
24+
exists(Method m | m.getDeclaringType() instanceof JxBrowser | m.hasName("addBoundsListener"))
25+
}
26+
27+
/** The `com.teamdev.jxbrowser.chromium.Browser` class. */
28+
private class JxBrowser extends RefType {
29+
JxBrowser() { this.hasQualifiedName("com.teamdev.jxbrowser.chromium", "Browser") }
30+
}
31+
32+
/** The `setLoadHandler` method on the `com.teamdev.jxbrowser.chromium.Browser` class. */
33+
private class JxBrowserSetLoadHandler extends Method {
34+
JxBrowserSetLoadHandler() {
35+
this.hasName("setLoadHandler") and this.getDeclaringType() instanceof JxBrowser
36+
}
37+
}
38+
39+
/** The `com.teamdev.jxbrowser.chromium.LoadHandler` interface. */
40+
private class JxBrowserLoadHandler extends RefType {
41+
JxBrowserLoadHandler() { this.hasQualifiedName("com.teamdev.jxbrowser.chromium", "LoadHandler") }
42+
}
43+
44+
private predicate isOnCertificateErrorMethodSafe(Method m) {
45+
forex(ReturnStmt rs | rs.getEnclosingCallable() = m |
46+
rs.getResult().(CompileTimeConstantExpr).getBooleanValue() = true
47+
)
48+
}
49+
50+
/** A class that securely implements the `com.teamdev.jxbrowser.chromium.LoadHandler` interface. */
51+
private class JxBrowserSafeLoadHandler extends RefType {
52+
JxBrowserSafeLoadHandler() {
53+
this.getASupertype() instanceof JxBrowserLoadHandler and
54+
exists(Method m | m.hasName("onCertificateError") and m.getDeclaringType() = this |
55+
isOnCertificateErrorMethodSafe(m)
56+
)
57+
}
58+
}
59+
60+
/**
61+
* Models flow from the source `new Browser()` to a sink `browser.setLoadHandler(loadHandler)` where `loadHandler`
62+
* has been determined to be safe.
63+
*/
64+
private class JxBrowserFlowConfiguration extends DataFlow::Configuration {
65+
JxBrowserFlowConfiguration() { this = "JxBrowserFlowConfiguration" }
66+
67+
override predicate isSource(DataFlow::Node src) {
68+
exists(ClassInstanceExpr newJxBrowser | newJxBrowser.getConstructedType() instanceof JxBrowser |
69+
newJxBrowser = src.asExpr()
70+
)
71+
}
72+
73+
override predicate isSink(DataFlow::Node sink) {
74+
exists(MethodAccess ma | ma.getMethod() instanceof JxBrowserSetLoadHandler |
75+
ma.getArgument(0).getType() instanceof JxBrowserSafeLoadHandler and
76+
ma.getQualifier() = sink.asExpr()
77+
)
78+
}
79+
}
80+
81+
from JxBrowserFlowConfiguration cfg, DataFlow::Node src
82+
where
83+
cfg.isSource(src) and
84+
not cfg.hasFlow(src, _) and
85+
not isSafeJxBrowserVersion()
86+
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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
| JxBrowserWithoutCertValidationV6_23_1.java:17:27:17:39 | new Browser(...) | This JxBrowser instance may not check HTTPS certificates. |

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import com.teamdev.jxbrowser.chromium.Browser;
2+
import com.teamdev.jxbrowser.chromium.LoadHandler;
3+
import com.teamdev.jxbrowser.chromium.LoadParams;
4+
import com.teamdev.jxbrowser.chromium.CertificateErrorParams;
5+
6+
public class JxBrowserWithoutCertValidationV6_23_1 {
7+
8+
public static void main(String[] args) {
9+
10+
badUsage();
11+
12+
goodUsage();
13+
14+
}
15+
16+
private static void badUsage() {
17+
Browser browser = new Browser();
18+
browser.loadURL("https://example.com");
19+
// no further calls
20+
// BAD: The browser ignores any certificate error by default!
21+
}
22+
23+
private static void goodUsage() {
24+
Browser browser = new Browser();
25+
browser.setLoadHandler(new LoadHandler() {
26+
public boolean onLoad(LoadParams params) {
27+
return true;
28+
}
29+
30+
public boolean onCertificateError(CertificateErrorParams params) {
31+
return true; // GOOD: This means that loading will be cancelled on certificate errors
32+
}
33+
}); // GOOD: A secure `LoadHandler` is used.
34+
browser.loadURL("https://example.com");
35+
}
36+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
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.expected

Whitespace-only changes.

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE/CWE-295/JxBrowserWithoutCertValidation.ql

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import com.teamdev.jxbrowser.chromium.Browser;
2+
import com.teamdev.jxbrowser.chromium.LoadHandler;
3+
import com.teamdev.jxbrowser.chromium.LoadParams;
4+
import com.teamdev.jxbrowser.chromium.CertificateErrorParams;
5+
6+
public class JxBrowserWithoutCertValidationV6_24 {
7+
8+
public static void main(String[] args) {
9+
10+
goodUsage();
11+
12+
goodUsage2();
13+
14+
}
15+
16+
private static void goodUsage() {
17+
Browser browser = new Browser();
18+
browser.loadURL("https://example.com");
19+
// no further calls
20+
// GOOD: On version 6.24 the browser properly validates certificates by default!
21+
}
22+
23+
private static void goodUsage2() {
24+
Browser browser = new Browser();
25+
browser.setLoadHandler(new LoadHandler() {
26+
public boolean onLoad(LoadParams params) {
27+
return true;
28+
}
29+
30+
public boolean onCertificateError(CertificateErrorParams params) {
31+
return true; // GOOD: This means that loading will be cancelled on certificate errors
32+
}
33+
}); // GOOD: A secure `LoadHandler` is used.
34+
browser.loadURL("https://example.com");
35+
}
36+
}

0 commit comments

Comments
 (0)