[WIP] Java: Improve CWE 295 #4536

intrigus-lgtm · 2020-10-21T21:40:09Z

Continuation of #3581
@smowton what do you think about the approach?
I.e. using data flow and splitting everything into unsafe trustmanager, trust-all hostname verifier, and missing SSL endpoint indentification?

This seperates security issues with missing hostname verification (CWE 297) and issues with trustmanagers that accept any certificate as valid (CWE 295). (This is an incomplete separation for now)

Separate trust-all hostname verification and cases that miss enabling SSL endpoint identification.

smowton

Submitting this partial review, but I think I may have partially reviewed commits that are subsequently undone. Will now switch to reviewing the whole changeset as a single unit.

smowton · 2020-10-23T09:56:06Z

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

+			public X509Certificate[] getAcceptedIssuers() {
+				return null; //BAD: doesn't check cert issuer
+			}


Are you sure? All the uses in the JDK I can find seem to treat a null return as either invalid, or equivalent to a zero-length return (i.e., trust nobody).

smowton · 2020-10-23T09:56:38Z

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

+			public X509Certificate[] getAcceptedIssuers() {
+				return new X509Certificate[0]; //GOOD: Validate the cert issuer
+			}


Surely this rejects everything?

smowton · 2020-10-23T09:59:19Z

java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.qhelp

 </li>
 <li>
-<a href="https://support.google.com/faqs/answer/6346016?hl=en">How to fix apps containing an unsafe implementation of TrustManager</a>
+<a href="https://support.google.com/faqs/answer/7188426?hl=en">How to resolve Insecure HostnameVerifier</a>


Would be better to find a link that doesn't specifically talk about Google Play, since this query is more general than that.

java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql

smowton

Some of these comments are improvements to code that's already committed to experimental, so they're optional.

Could you clarify which cases you think are now covered that were not previously covered by the CWE-273 query?

smowton · 2020-10-23T10:24:59Z

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

+<qhelp>
+
+<overview>
+<p>Java offers two mechanisms for SSL authentication - trust manager and hostname verifier. Trust manager validates the peer's certificate chain while hostname verification establishes that the hostname in the URL matches the hostname in the server's identification.</p>


Suggested change

Java offers two mechanisms for SSL authentication - trust manager and hostname verifier. Trust manager validates the peer's certificate chain while hostname verification establishes that the hostname in the URL matches the hostname in the server's identification.

Java offers two mechanisms for SSL authentication - the X509 trust manager and the hostname verifier. The X509 trust manager validates the peer's certificate chain, while hostname verification establishes that the hostname in the URL matches the hostname given by the peer's certificate.

smowton · 2020-10-23T10:28:09Z

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

+
+<overview>
+<p>Java offers two mechanisms for SSL authentication - trust manager and hostname verifier. Trust manager validates the peer's certificate chain while hostname verification establishes that the hostname in the URL matches the hostname in the server's identification.</p>
+<p>And when SSLSocket or SSLEngine is created without a valid parameter of setEndpointIdentificationAlgorithm, hostname verification is disabled by default.</p>


Suggested change

And when SSLSocket or SSLEngine is created without a valid parameter of setEndpointIdentificationAlgorithm, hostname verification is disabled by default.

Note that SSLSocket and SSLEngine do not verify hostnames by default. Verification must be enabled by calling <code>setEndpointIdentificationAlgorithm</code>.

smowton · 2020-10-23T10:30:04Z

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

+<overview>
+<p>Java offers two mechanisms for SSL authentication - trust manager and hostname verifier. Trust manager validates the peer's certificate chain while hostname verification establishes that the hostname in the URL matches the hostname in the server's identification.</p>
+<p>And when SSLSocket or SSLEngine is created without a valid parameter of setEndpointIdentificationAlgorithm, hostname verification is disabled by default.</p>
+<p>Unsafe implementation of the interface X509TrustManager, HostnameVerifier, and SSLSocket/SSLEngine ignores all SSL certificate validation errors when establishing an HTTPS connection, thereby making the app vulnerable to man-in-the-middle attacks.</p>


Suggested change

Unsafe implementation of the interface X509TrustManager, HostnameVerifier, and SSLSocket/SSLEngine ignores all SSL certificate validation errors when establishing an HTTPS connection, thereby making the app vulnerable to man-in-the-middle attacks.

Unsafe implementations of the interfaces <code>X509TrustManager</code>, <code>HostnameVerifier</code>, and <code>SSLSocket</code>/<code>SSLEngine</code> that ignore SSL certificate validation errors when establishing an HTTPS connection make the connection vulnerable to man-in-the-middle attacks.

smowton · 2020-10-23T10:30:39Z

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

+<p>Java offers two mechanisms for SSL authentication - trust manager and hostname verifier. Trust manager validates the peer's certificate chain while hostname verification establishes that the hostname in the URL matches the hostname in the server's identification.</p>
+<p>And when SSLSocket or SSLEngine is created without a valid parameter of setEndpointIdentificationAlgorithm, hostname verification is disabled by default.</p>
+<p>Unsafe implementation of the interface X509TrustManager, HostnameVerifier, and SSLSocket/SSLEngine ignores all SSL certificate validation errors when establishing an HTTPS connection, thereby making the app vulnerable to man-in-the-middle attacks.</p>
+<p>This query checks whether trust manager is set to trust all certificates, the hostname verifier is turned off, or setEndpointIdentificationAlgorithm is missing. The query also covers a special implementation com.rabbitmq.client.ConnectionFactory.</p>


Suggested change

This query checks whether trust manager is set to trust all certificates, the hostname verifier is turned off, or setEndpointIdentificationAlgorithm is missing. The query also covers a special implementation com.rabbitmq.client.ConnectionFactory.

This query checks whether trust manager is set to trust all certificates, the hostname verifier is turned off, or <code>setEndpointIdentificationAlgorithm</code> is missing. The query also covers an insecure implementation <code>com.rabbitmq.client.ConnectionFactory</code>.

smowton · 2020-10-23T10:32:19Z

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

+      m2.getDeclaringType() = this and
+      m2.hasName("getAcceptedIssuers") and
+      rt2.getEnclosingCallable() = m2 and
+      rt2.getResult() instanceof NullLiteral


Comment from previous review still applies: this doesn't seem to be a problem

smowton · 2020-10-23T10:33:24Z

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

+      exists(ArrayInit ai |
+        this.getArgument(1).(ArrayCreationExpr).getInit() = ai and
+        ai.getInit(0).(VarAccess).getVariable().getInitializer().getType().(Class).getASupertype*()
+          instanceof X509TrustAllManager //Scenario of context.init(null, new TrustManager[] { TRUST_ALL_CERTIFICATES }, null);
+      )
+      or
+      exists(Variable v, ArrayInit ai |
+        this.getArgument(1).(VarAccess).getVariable() = v and
+        ai.getParent() = v.getAnAssignedValue() and
+        ai.getInit(0).getType().(Class).getASupertype*() instanceof X509TrustAllManager //Scenario of context.init(null, serverTMs, null);
+      )


Comment from previous review still applies: avoid using brittle precise syntax like this; instead use function models to follow taint (in this case indicating an insecure config) from the instantiation of the weak checker through to use in an SSLSocket or similar.

java/ql/src/experimental/Security/CWE/CWE-297/UnsafeHostnameVerification.ql

intrigus-lgtm · 2021-01-04T14:10:31Z

Superseded by #4771

intrigus added 4 commits October 21, 2020 22:06

Separate hostname verification and trustmanger.

2a0a7ad

This seperates security issues with missing hostname verification (CWE 297) and issues with trustmanagers that accept any certificate as valid (CWE 295). (This is an incomplete separation for now)

Accept test changes and more separation.

104accf

Switch to dataflow for trust-all verifier.

4210ba8

Further separation.

2a0067e

Separate trust-all hostname verification and cases that miss enabling SSL endpoint identification.

github-actions bot added documentation Java labels Oct 21, 2020

smowton reviewed Oct 23, 2020

View reviewed changes

Inline TrueStmt class

e140fb5

smowton mentioned this pull request Jan 4, 2021

[Java]: CWE 295 - Insecure TrustManager - MiTM github/securitylab#221

Closed

1 task

intrigus-lgtm closed this Jan 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Java: Improve CWE 295 #4536

[WIP] Java: Improve CWE 295 #4536

intrigus-lgtm commented Oct 21, 2020

smowton left a comment

smowton Oct 23, 2020

smowton Oct 23, 2020

smowton Oct 23, 2020

smowton left a comment

smowton Oct 23, 2020

smowton Oct 23, 2020

smowton Oct 23, 2020

smowton Oct 23, 2020

smowton Oct 23, 2020

smowton Oct 23, 2020

intrigus-lgtm commented Jan 4, 2021

	<p>Java offers two mechanisms for SSL authentication - trust manager and hostname verifier. Trust manager validates the peer's certificate chain while hostname verification establishes that the hostname in the URL matches the hostname in the server's identification.</p>
	<p>Java offers two mechanisms for SSL authentication - the X509 trust manager and the hostname verifier. The X509 trust manager validates the peer's certificate chain, while hostname verification establishes that the hostname in the URL matches the hostname given by the peer's certificate.</p>

	<p>And when SSLSocket or SSLEngine is created without a valid parameter of setEndpointIdentificationAlgorithm, hostname verification is disabled by default.</p>
	<p>Note that SSLSocket and SSLEngine do not verify hostnames by default. Verification must be enabled by calling <code>setEndpointIdentificationAlgorithm</code>.</p>

	<p>Unsafe implementation of the interface X509TrustManager, HostnameVerifier, and SSLSocket/SSLEngine ignores all SSL certificate validation errors when establishing an HTTPS connection, thereby making the app vulnerable to man-in-the-middle attacks.</p>
	<p>Unsafe implementations of the interfaces <code>X509TrustManager</code>, <code>HostnameVerifier</code>, and <code>SSLSocket</code>/<code>SSLEngine</code> that ignore SSL certificate validation errors when establishing an HTTPS connection make the connection vulnerable to man-in-the-middle attacks.</p>

	<p>This query checks whether trust manager is set to trust all certificates, the hostname verifier is turned off, or setEndpointIdentificationAlgorithm is missing. The query also covers a special implementation com.rabbitmq.client.ConnectionFactory.</p>
	<p>This query checks whether trust manager is set to trust all certificates, the hostname verifier is turned off, or <code>setEndpointIdentificationAlgorithm</code> is missing. The query also covers an insecure implementation <code>com.rabbitmq.client.ConnectionFactory</code>.</p>

[WIP] Java: Improve CWE 295 #4536

[WIP] Java: Improve CWE 295 #4536

Conversation

intrigus-lgtm commented Oct 21, 2020

smowton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smowton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intrigus-lgtm commented Jan 4, 2021