[WIP] Java: Improve CWE 295 #4536
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| public static void main(String[] args) { | ||
| { | ||
| X509TrustManager trustAllCertManager = new X509TrustManager() { | ||
| @Override | ||
| public void checkClientTrusted(final X509Certificate[] chain, final String authType) | ||
| throws CertificateException { | ||
| } | ||
|
|
||
| @Override | ||
| public void checkServerTrusted(final X509Certificate[] chain, final String authType) | ||
| throws CertificateException { | ||
| // BAD: trust any server cert | ||
| } | ||
|
|
||
| @Override | ||
| public X509Certificate[] getAcceptedIssuers() { | ||
| return null; //BAD: doesn't check cert issuer | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| { | ||
| X509TrustManager trustCertManager = new X509TrustManager() { | ||
| @Override | ||
| public void checkClientTrusted(final X509Certificate[] chain, final String authType) | ||
| throws CertificateException { | ||
| } | ||
|
|
||
| @Override | ||
| public void checkServerTrusted(final X509Certificate[] chain, final String authType) | ||
| throws CertificateException { | ||
| pkixTrustManager.checkServerTrusted(chain, authType); //GOOD: validate the server cert | ||
| } | ||
|
|
||
| @Override | ||
| public X509Certificate[] getAcceptedIssuers() { | ||
| return new X509Certificate[0]; //GOOD: Validate the cert issuer | ||
| } | ||
|
Comment on lines
+36
to
+38
There was a problem hiding this comment. Surely this rejects everything? |
||
| }; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,31 @@ | ||||||
| <!DOCTYPE qhelp PUBLIC | ||||||
| "-//Semmle//qhelp//EN" | ||||||
| "qhelp.dtd"> | ||||||
| <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> | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| <p>And when SSLSocket or SSLEngine is created without a valid parameter of setEndpointIdentificationAlgorithm, hostname verification is disabled by default.</p> | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| <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> | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| <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> | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| </overview> | ||||||
|
|
||||||
| <recommendation> | ||||||
| <p>Validate SSL certificate in SSL authentication.</p> | ||||||
| </recommendation> | ||||||
|
|
||||||
| <example> | ||||||
| <p>The following two examples show two ways of configuring X509 trust cert manager and hostname verifier. In the 'BAD' case, | ||||||
| no validation is performed thus any certificate is trusted. In the 'GOOD' case, the proper validation is performed.</p> | ||||||
| <sample src="UnsafeCertTrust.java" /> | ||||||
| </example> | ||||||
|
|
||||||
| <references> | ||||||
| <li> | ||||||
| <a href="https://cwe.mitre.org/data/definitions/295.html">CWE-295</a> | ||||||
| </li> | ||||||
| <li> | ||||||
| <a href="https://support.google.com/faqs/answer/6346016?hl=en">How to fix apps containing an unsafe implementation of TrustManager</a> | ||||||
| </li> | ||||||
| </references> | ||||||
| </qhelp> | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| /** | ||
| * @name Unsafe certificate trust | ||
| * @description Unsafe implementation of a `X509TrustManager`, thereby making the app vulnerable to man-in-the-middle attacks. | ||
| * @kind problem | ||
| * @id java/unsafe-cert-trust | ||
| * @tags security | ||
| * external/cwe-295 | ||
| */ | ||
|
|
||
| import java | ||
| import semmle.code.java.security.Encryption | ||
|
|
||
| /** | ||
| * A `X509TrustManager` class that blindly trusts all certificates in server SSL authentication. | ||
| */ | ||
| class X509TrustAllManager extends RefType { | ||
| X509TrustAllManager() { | ||
| this.getASupertype*() instanceof X509TrustManager and | ||
| exists(Method m1 | | ||
| m1.getDeclaringType() = this and | ||
| m1.hasName("checkServerTrusted") and | ||
| m1.getBody().getNumStmt() = 0 | ||
| ) and | ||
| exists(Method m2, ReturnStmt rt2 | | ||
| m2.getDeclaringType() = this and | ||
| m2.hasName("getAcceptedIssuers") and | ||
| rt2.getEnclosingCallable() = m2 and | ||
| rt2.getResult() instanceof NullLiteral | ||
|
Comment on lines
+25
to
+28
There was a problem hiding this comment. Comment from previous review still applies: this doesn't seem to be a problem |
||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * The init method of SSLContext with the trust all manager, which is sslContext.init(..., serverTMs, ...) | ||
| */ | ||
| class X509TrustAllManagerInit extends MethodAccess { | ||
| X509TrustAllManagerInit() { | ||
| this.getMethod().hasName("init") and | ||
| this.getMethod().getDeclaringType() instanceof SSLContext and //init method of SSLContext | ||
| ( | ||
| 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 on lines
+41
to
+51
There was a problem hiding this comment. 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. |
||
| ) | ||
| } | ||
| } | ||
|
|
||
| from MethodAccess ma | ||
| where ma instanceof X509TrustAllManagerInit | ||
| select ma, "Unsafe configuration of trusted certificates" | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| /** | ||
| * @name Unsafe certificate trust and improper hostname verification | ||
| * @description 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. | ||
| * @kind problem | ||
| * @id java/unsafe-cert-trust | ||
| * @tags security | ||
| * external/cwe-273 | ||
| */ | ||
|
|
||
| import java | ||
| import semmle.code.java.security.Encryption | ||
| import semmle.code.java.dataflow.DataFlow | ||
|
|
||
| /** | ||
| * Holds if `m` always returns `true` ignoring any exceptional flow. | ||
| */ | ||
| private predicate alwaysReturnsTrue(HostnameVerifierVerify m) { | ||
| forex(ReturnStmt rs | rs.getEnclosingCallable() = m | | ||
| rs.getResult().(CompileTimeConstantExpr).getBooleanValue() = true | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * A class that overrides the `javax.net.ssl.HostnameVerifier.verify` method and **always** returns `true`, thus | ||
| * accepting any certificate despite a hostname mismatch. | ||
| */ | ||
| class TrustAllHostnameVerifier extends RefType { | ||
| TrustAllHostnameVerifier() { | ||
| this.getASupertype*() instanceof HostnameVerifier and | ||
| exists(HostnameVerifierVerify m | | ||
| m.getDeclaringType() = this and | ||
| alwaysReturnsTrue(m) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| class SSLEngine extends RefType { | ||
| SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") } | ||
| } | ||
|
|
||
| class Socket extends RefType { | ||
| Socket() { this.hasQualifiedName("java.net", "Socket") } | ||
| } | ||
|
|
||
| class SocketFactory extends RefType { | ||
| SocketFactory() { this.hasQualifiedName("javax.net", "SocketFactory") } | ||
| } | ||
|
|
||
| class SSLSocket extends RefType { | ||
| SSLSocket() { this.hasQualifiedName("javax.net.ssl", "SSLSocket") } | ||
| } | ||
|
|
||
| /** | ||
| * A configuration to model the flow of a `TrustAllHostnameVerifier` to a `set(Default)HostnameVerifier` call. | ||
| */ | ||
| class TrustAllHostnameVerifierConfiguration extends DataFlow::Configuration { | ||
| TrustAllHostnameVerifierConfiguration() { this = "TrustAllHostnameVerifierConfiguration" } | ||
|
|
||
| override predicate isSource(DataFlow::Node source) { | ||
| source.asExpr().(ClassInstanceExpr).getConstructedType() instanceof TrustAllHostnameVerifier | ||
| } | ||
|
|
||
| override predicate isSink(DataFlow::Node sink) { | ||
| exists(MethodAccess ma, Method m | | ||
| (m instanceof SetDefaultHostnameVerifierMethod or m instanceof SetHostnameVerifierMethod) and | ||
| ma.getMethod() = m | ||
| | | ||
| ma.getArgument(0) = sink.asExpr() | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| from DataFlow::Node source, DataFlow::Node sink, TrustAllHostnameVerifierConfiguration cfg | ||
| where cfg.hasFlow(source, sink) | ||
| select sink, "Unsafe configuration of trusted certificates" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| | UnsafeCertificateTrust.java:27:4:27:74 | init(...) | Unsafe configuration of trusted certificates | | ||
| | UnsafeCertificateTrust.java:42:4:42:38 | init(...) | Unsafe configuration of trusted certificates | |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).