Merge pull request #4879 from intrigus-lgtm/java/improve-trustmanager

Java: Add/improve insecure trustmanager query
This commit is contained in:
Chris Smowton
2021-06-25 18:15:55 +01:00
committed by GitHub
13 changed files with 856 additions and 222 deletions

View File

@@ -15,6 +15,7 @@ import semmle.code.java.controlflow.Guards
import semmle.code.java.dataflow.DataFlow
import semmle.code.java.dataflow.FlowSources
import semmle.code.java.security.Encryption
import semmle.code.java.security.SecurityFlag
import DataFlow::PathGraph
private import semmle.code.java.dataflow.ExternalFlow
@@ -86,71 +87,30 @@ private class HostnameVerifierSink extends DataFlow::Node {
HostnameVerifierSink() { sinkNode(this, "set-hostname-verifier") }
}
bindingset[result]
private string getAFlagName() {
result
.regexpMatch("(?i).*(secure|disable|selfCert|selfSign|validat|verif|trust|ignore|nocertificatecheck).*")
}
/**
* A flag has to either be of type `String`, `boolean` or `Boolean`.
* Flags suggesting a deliberately unsafe `HostnameVerifier` usage.
*/
private class FlagType extends Type {
FlagType() {
this instanceof TypeString
or
this instanceof BooleanType
private class UnsafeHostnameVerificationFlag extends FlagKind {
UnsafeHostnameVerificationFlag() { this = "UnsafeHostnameVerificationFlag" }
bindingset[result]
override string getAFlagName() {
result
.regexpMatch("(?i).*(secure|disable|selfCert|selfSign|validat|verif|trust|ignore|nocertificatecheck).*") and
result != "equalsIgnoreCase"
}
}
private predicate isEqualsIgnoreCaseMethodAccess(MethodAccess ma) {
ma.getMethod().hasName("equalsIgnoreCase") and
ma.getMethod().getDeclaringType() instanceof TypeString
/** Gets a guard that represents a (likely) flag controlling an unsafe `HostnameVerifier` use. */
private Guard getAnUnsafeHostnameVerifierFlagGuard() {
result = any(UnsafeHostnameVerificationFlag flag).getAFlag().asExpr()
}
/** Holds if `source` should is considered a flag. */
private predicate isFlag(DataFlow::Node source) {
exists(VarAccess v | v.getVariable().getName() = getAFlagName() |
source.asExpr() = v and v.getType() instanceof FlagType
)
or
exists(StringLiteral s | s.getRepresentedString() = getAFlagName() | source.asExpr() = s)
or
exists(MethodAccess ma | ma.getMethod().getName() = getAFlagName() |
source.asExpr() = ma and
ma.getType() instanceof FlagType and
not isEqualsIgnoreCaseMethodAccess(ma)
)
}
/** Holds if there is flow from `node1` to `node2` either due to local flow or due to custom flow steps. */
private predicate flagFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
DataFlow::localFlowStep(node1, node2)
or
exists(MethodAccess ma | ma.getMethod() = any(EnvReadMethod m) |
ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr()
)
or
exists(MethodAccess ma |
ma.getMethod().hasName("parseBoolean") and
ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "Boolean")
|
ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr()
)
}
/** Gets a guard that depends on a flag. */
private Guard getAGuard() {
exists(DataFlow::Node source, DataFlow::Node sink |
isFlag(source) and
flagFlowStep*(source, sink) and
sink.asExpr() = result
)
}
/** Holds if `node` is guarded by a flag that suggests an intentionally insecure feature. */
/** Holds if `node` is guarded by a flag that suggests an intentionally insecure use. */
private predicate isNodeGuardedByFlag(DataFlow::Node node) {
exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) | g = getAGuard())
exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) |
g = getASecurityFeatureFlagGuard() or g = getAnUnsafeHostnameVerifierFlagGuard()
)
}
from

View File

@@ -1,45 +1,5 @@
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
}
};
}
{
SSLContext sslContext = SSLContext.getInstance("TLS");
SSLEngine sslEngine = sslContext.createSSLEngine();

View File

@@ -4,10 +4,9 @@
<qhelp>
<overview>
<p>Java offers two mechanisms for SSL authentication - trust manager and hostname verifier (checked by the <code>java/insecure-hostname-verifier</code> query). 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>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 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 or setEndpointIdentificationAlgorithm is missing. The query also covers a special implementation com.rabbitmq.client.ConnectionFactory.</p>
<p>This query checks whether setEndpointIdentificationAlgorithm is missing. The query also covers a special implementation com.rabbitmq.client.ConnectionFactory.</p>
</overview>
<recommendation>
@@ -15,8 +14,8 @@
</recommendation>
<example>
<p>The following two examples show two ways of configuring X509 trust cert manager. In the 'BAD' case,
no validation is performed thus any certificate is trusted. In the 'GOOD' case, the proper validation is performed.</p>
<p>The following two examples show two ways of configuring SSLSocket/SSLEngine. In the 'BAD' case,
setEndpointIdentificationAlgorithm is not called, thus no hostname verification takes place. In the 'GOOD' case, setEndpointIdentificationAlgorithm is called.</p>
<sample src="UnsafeCertTrust.java" />
</example>
@@ -25,9 +24,6 @@ no validation is performed thus any certificate is trusted. In the 'GOOD' case,
<a href="https://cwe.mitre.org/data/definitions/273.html">CWE-273</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>
<li>
<a href="https://github.com/OWASP/owasp-mstg/blob/master/Document/0x05g-Testing-Network-Communication.md">Testing Endpoint Identify Verification (MSTG-NETWORK-3)</a>
</li>
<li>

View File

@@ -1,7 +1,6 @@
/**
* @name Unsafe certificate trust
* @description Unsafe implementation of the interface X509TrustManager and
* SSLSocket/SSLEngine ignores all SSL certificate validation
* @description 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
@@ -15,49 +14,6 @@
import java
import semmle.code.java.security.Encryption
/**
* 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
)
}
}
/**
* 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);
)
)
}
}
class SSLEngine extends RefType {
SSLEngine() { this.hasQualifiedName("javax.net.ssl", "SSLEngine") }
}
@@ -208,7 +164,6 @@ class RabbitMQEnableHostnameVerificationNotSet extends MethodAccess {
from MethodAccess aa
where
aa instanceof X509TrustAllManagerInit or
aa instanceof SSLEndpointIdentificationNotSet or
aa instanceof RabbitMQEnableHostnameVerificationNotSet
select aa, "Unsafe configuration of trusted certificates"

View File

@@ -0,0 +1,52 @@
public static void main(String[] args) throws Exception {
{
class InsecureTrustManager implements X509TrustManager {
@Override
public X509Certificate[] getAcceptedIssuers() {
return null;
}
@Override
public void checkServerTrusted(X509Certificate[] chain, String authType) throws CertificateException {
// BAD: Does not verify the certificate chain, allowing any certificate.
}
@Override
public void checkClientTrusted(X509Certificate[] chain, String authType) throws CertificateException {
}
}
SSLContext context = SSLContext.getInstance("TLS");
TrustManager[] trustManager = new TrustManager[] { new InsecureTrustManager() };
context.init(null, trustManager, null);
}
{
SSLContext context = SSLContext.getInstance("TLS");
File certificateFile = new File("path/to/self-signed-certificate");
// Create a `KeyStore` with default type
KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
// `keyStore` is initially empty
keyStore.load(null, null);
X509Certificate generatedCertificate;
try (InputStream cert = new FileInputStream(certificateFile)) {
generatedCertificate = (X509Certificate) CertificateFactory.getInstance("X509")
.generateCertificate(cert);
}
// Add the self-signed certificate to the key store
keyStore.setCertificateEntry(certificateFile.getName(), generatedCertificate);
// Get default `TrustManagerFactory`
TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
// Use it with our key store that trusts our self-signed certificate
tmf.init(keyStore);
TrustManager[] trustManagers = tmf.getTrustManagers();
context.init(null, trustManagers, null);
// GOOD, we are not using a custom `TrustManager` but instead have
// added the self-signed certificate we want to trust to the key
// store. Note, the `trustManagers` will **only** trust this one
// certificate.
URL url = new URL("https://self-signed.badssl.com/");
HttpsURLConnection conn = (HttpsURLConnection) url.openConnection();
conn.setSSLSocketFactory(context.getSocketFactory());
}
}

View File

@@ -0,0 +1,47 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>
If the <code>checkServerTrusted</code> method of a <code>TrustManager</code> never throws a <code>CertificateException</code> it trusts every 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 program 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 program calls the <code>checkServerTrusted</code> method to check whether it should trust the certificate.</li>
<li>The <code>checkServerTrusted</code> method of your <code>TrustManager</code> does not throw a <code>CertificateException</code>.</li>
<li>The vulnerable program accepts the certificate and proceeds with the connection since your <code>TrustManager</code> implicitly trusted it by not throwing an exception.</li>
<li>The attacker can now read the data your program sends to <code>https://example.com</code> and/or alter its replies while the program thinks the connection is secure.</li>
</ol>
</overview>
<recommendation>
<p>
Do not use a custom <code>TrustManager</code> that trusts any certificate.
If you have to use a self-signed certificate, don't trust every certificate, but instead only trust this specific certificate.
See below for an example of how to do this.
</p>
</recommendation>
<example>
<p>
In the first (bad) example, the <code>TrustManager</code> never throws a <code>CertificateException</code> and therefore implicitly trusts any certificate.
This allows an attacker to perform a machine-in-the-middle attack.
In the second (good) example, the self-signed certificate that should be trusted
is loaded into a <code>KeyStore</code>. This explicitly defines the certificate as trusted and there is no need to create a custom <code>TrustManager</code>.
</p>
<sample src="InsecureTrustManager.java" />
</example>
<references>
<li>Android Develoers:<a href="https://developer.android.com/training/articles/security-ssl">Security with HTTPS and SSL</a>.</li>
</references>
</qhelp>

View File

@@ -0,0 +1,117 @@
/**
* @name `TrustManager` that accepts all certificates
* @description Trusting all certificates allows an attacker to perform a machine-in-the-middle attack.
* @kind path-problem
* @problem.severity error
* @precision high
* @id java/insecure-trustmanager
* @tags security
* external/cwe/cwe-295
*/
import java
import semmle.code.java.controlflow.Guards
import semmle.code.java.dataflow.DataFlow
import semmle.code.java.dataflow.FlowSources
import semmle.code.java.security.Encryption
import semmle.code.java.security.SecurityFlag
import DataFlow::PathGraph
/**
* An insecure `X509TrustManager`.
* An `X509TrustManager` is considered insecure if it never throws a `CertificateException`
* and therefore implicitly trusts any certificate as valid.
*/
class InsecureX509TrustManager extends RefType {
InsecureX509TrustManager() {
this.getASupertype*() instanceof X509TrustManager and
exists(Method m |
m.getDeclaringType() = this and
m.hasName("checkServerTrusted") and
not mayThrowCertificateException(m)
)
}
}
/** The `java.security.cert.CertificateException` class. */
private class CertificateException extends RefType {
CertificateException() { this.hasQualifiedName("java.security.cert", "CertificateException") }
}
/**
* Holds if:
* - `m` may `throw` a `CertificateException`, or
* - `m` calls another method that may throw, or
* - `m` calls a method declared to throw a `CertificateException`, but for which no source is available
*/
private predicate mayThrowCertificateException(Method m) {
exists(ThrowStmt throwStmt |
throwStmt.getThrownExceptionType().getASupertype*() instanceof CertificateException
|
throwStmt.getEnclosingCallable() = m
)
or
exists(Method otherMethod | m.polyCalls(otherMethod) |
mayThrowCertificateException(otherMethod)
or
not otherMethod.fromSource() and
otherMethod.getAnException().getType().getASupertype*() instanceof CertificateException
)
}
/**
* A configuration to model the flow of an `InsecureX509TrustManager` to an `SSLContext.init` call.
*/
class InsecureTrustManagerConfiguration extends TaintTracking::Configuration {
InsecureTrustManagerConfiguration() { this = "InsecureTrustManagerConfiguration" }
override predicate isSource(DataFlow::Node source) {
source.asExpr().(ClassInstanceExpr).getConstructedType() instanceof InsecureX509TrustManager
}
override predicate isSink(DataFlow::Node sink) {
exists(MethodAccess ma, Method m |
m.hasName("init") and
m.getDeclaringType() instanceof SSLContext and
ma.getMethod() = m
|
ma.getArgument(1) = sink.asExpr()
)
}
}
/**
* Flags suggesting a deliberately insecure `TrustManager` usage.
*/
private class InsecureTrustManagerFlag extends FlagKind {
InsecureTrustManagerFlag() { this = "InsecureTrustManagerFlag" }
bindingset[result]
override string getAFlagName() {
result
.regexpMatch("(?i).*(secure|disable|selfCert|selfSign|validat|verif|trust|ignore|nocertificatecheck).*") and
result != "equalsIgnoreCase"
}
}
/** Gets a guard that represents a (likely) flag controlling an insecure `TrustManager` use. */
private Guard getAnInsecureTrustManagerFlagGuard() {
result = any(InsecureTrustManagerFlag flag).getAFlag().asExpr()
}
/** Holds if `node` is guarded by a flag that suggests an intentionally insecure use. */
private predicate isNodeGuardedByFlag(DataFlow::Node node) {
exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) |
g = getASecurityFeatureFlagGuard() or g = getAnInsecureTrustManagerFlagGuard()
)
}
from
DataFlow::PathNode source, DataFlow::PathNode sink, InsecureTrustManagerConfiguration cfg,
RefType trustManager
where
cfg.hasFlowPath(source, sink) and
not isNodeGuardedByFlag(sink.getNode()) and
trustManager = source.getNode().asExpr().(ClassInstanceExpr).getConstructedType()
select sink, source, sink, "$@ that is defined $@ and trusts any certificate, is used here.",
source, "This trustmanager", trustManager, "here"

View File

@@ -0,0 +1,86 @@
/**
* Provides utility predicates to spot variable names, parameter names, and string literals that suggest deliberately insecure settings.
*/
import java
import semmle.code.java.controlflow.Guards
import semmle.code.java.dataflow.DataFlow
import semmle.code.java.dataflow.FlowSources
/**
* A kind of flag that may indicate security expectations regarding the code it guards.
*/
abstract class FlagKind extends string {
bindingset[this]
FlagKind() { any() }
/**
* Gets a flag name of this type.
*/
bindingset[result]
abstract string getAFlagName();
/** Gets a node representing a (likely) security flag. */
DataFlow::Node getAFlag() {
exists(DataFlow::Node flag |
exists(VarAccess v | v.getVariable().getName() = getAFlagName() |
flag.asExpr() = v and v.getType() instanceof FlagType
)
or
exists(StringLiteral s | s.getRepresentedString() = getAFlagName() | flag.asExpr() = s)
or
exists(MethodAccess ma | ma.getMethod().getName() = getAFlagName() |
flag.asExpr() = ma and
ma.getType() instanceof FlagType
)
|
flagFlowStep*(flag, result)
)
}
}
/**
* Flags suggesting an optional feature, perhaps deliberately insecure.
*/
private class SecurityFeatureFlag extends FlagKind {
SecurityFeatureFlag() { this = "SecurityFeatureFlag" }
bindingset[result]
override string getAFlagName() { result.regexpMatch("(?i).*(secure|(en|dis)able).*") }
}
/**
* A flag has to either be of type `String`, `boolean` or `Boolean`.
*/
private class FlagType extends Type {
FlagType() {
this instanceof TypeString
or
this instanceof BooleanType
}
}
/**
* Holds if there is local flow from `node1` to `node2` either due to standard data-flow steps or the
* following custom flow steps:
* 1. `Boolean.parseBoolean(taintedValue)` taints the return value of `parseBoolean`.
* 2. A call to an `EnvReadMethod` such as `System.getProperty` where a tainted value is used as an argument.
* The return value of such a method is then tainted.
*/
private predicate flagFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
DataFlow::localFlowStep(node1, node2)
or
exists(MethodAccess ma | ma.getMethod() = any(EnvReadMethod m) |
ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr()
)
or
exists(MethodAccess ma |
ma.getMethod().hasName("parseBoolean") and
ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "Boolean")
|
ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr()
)
}
/** Gets a guard that represents a (likely) security feature-flag check. */
Guard getASecurityFeatureFlagGuard() { result = any(SecurityFeatureFlag flag).getAFlag().asExpr() }