mirror of
https://github.com/github/codeql.git
synced 2025-12-21 19:26:31 +01:00
Merge pull request #4771 from intrigus-lgtm/split-cwe-295
Java: Add unsafe hostname verification query and remove existing overlapping query
This commit is contained in:
Anders Schack-Mulligen
@@ -0,0 +1,30 @@
|
||||
public static void main(String[] args) {
|
||||
|
||||
{
|
||||
HostnameVerifier verifier = new HostnameVerifier() {
|
||||
@Override
|
||||
public boolean verify(String hostname, SSLSession session) {
|
||||
return true; // BAD: accept even if the hostname doesn't match
|
||||
}
|
||||
};
|
||||
HttpsURLConnection.setDefaultHostnameVerifier(verifier);
|
||||
}
|
||||
|
||||
{
|
||||
HostnameVerifier verifier = new HostnameVerifier() {
|
||||
@Override
|
||||
public boolean verify(String hostname, SSLSession session) {
|
||||
try { // GOOD: verify the certificate
|
||||
Certificate[] certs = session.getPeerCertificates();
|
||||
X509Certificate x509 = (X509Certificate) certs[0];
|
||||
check(new String[]{host}, x509);
|
||||
return true;
|
||||
} catch (SSLException e) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
};
|
||||
HttpsURLConnection.setDefaultHostnameVerifier(verifier);
|
||||
}
|
||||
|
||||
}
|
||||
@@ -0,0 +1,50 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
<overview>
|
||||
<p>
|
||||
If a <code>HostnameVerifier</code> always returns <code>true</code> it will not verify the hostname at all.
|
||||
This stops Transport Layer Security (TLS) providing any security and allows an attacker to perform a man-in-the-middle attack against the application.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
An attack might look like this:
|
||||
</p>
|
||||
|
||||
<ol>
|
||||
<li>The program connects to <code>https://example.com</code>.</li>
|
||||
<li>The attacker intercepts this connection and presents an apparently-valid certificate of their choosing.</li>
|
||||
<li>The <code>TrustManager</code> of the program verifies that the certificate has been issued by a trusted certificate authority.</li>
|
||||
<li>The Java HTTPS library checks whether the certificate has been issued for the host <code>example.com</code>. This check fails because the certificate has been issued for a domain controlled by the attacker, for example: <code>malicious.domain</code>.</li>
|
||||
<li>The HTTPS library wants to reject the certificate because the hostname does not match. Before doing this it checks whether a <code>HostnameVerifier</code> exists.</li>
|
||||
<li>Your <code>HostnameVerifier</code> is called which returns <code>true</code> for any certificate so also for this one.</li>
|
||||
<li>The program proceeds with the connection since your <code>HostnameVerifier</code> accepted it.</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 an open <code>HostnameVerifier</code>.
|
||||
If you have a configuration problem with TLS/HTTPS, you should always solve the configuration problem instead of using an open verifier.
|
||||
</p>
|
||||
|
||||
</recommendation>
|
||||
|
||||
<example>
|
||||
<p>
|
||||
In the first (bad) example, the <code>HostnameVerifier</code> always returns <code>true</code>.
|
||||
This allows an attacker to perform a man-in-the-middle attack, because any certificate is accepted despite an incorrect hostname.
|
||||
In the second (good) example, the <code>HostnameVerifier</code> only returns <code>true</code> when the certificate has been correctly checked.
|
||||
</p>
|
||||
<sample src="UnsafeHostnameVerification.java" />
|
||||
</example>
|
||||
|
||||
<references>
|
||||
<li>Android developers: <a href="https://developer.android.com/training/articles/security-ssl">Security with HTTPS and SSL</a>.</li>
|
||||
<li>Terse systems blog: <a href="https://tersesystems.com/blog/2014/03/23/fixing-hostname-verification/">Fixing Hostname Verification</a>.</li>
|
||||
</references>
|
||||
</qhelp>
|
||||
163
java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql
Normal file
163
java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql
Normal file
@@ -0,0 +1,163 @@
|
||||
/**
|
||||
* @name Unsafe hostname verification
|
||||
* @description Marking a certificate as valid for a host without checking the certificate hostname allows an attacker to perform a machine-in-the-middle attack.
|
||||
* @kind path-problem
|
||||
* @problem.severity error
|
||||
* @precision high
|
||||
* @id java/unsafe-hostname-verification
|
||||
* @tags security
|
||||
* external/cwe/cwe-297
|
||||
*/
|
||||
|
||||
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 DataFlow::PathGraph
|
||||
|
||||
/**
|
||||
* 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` (though it could also exit due to an uncaught exception), 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)
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* 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()
|
||||
)
|
||||
}
|
||||
|
||||
override predicate isBarrier(DataFlow::Node barrier) {
|
||||
// ignore nodes that are in functions that intentionally disable hostname verification
|
||||
barrier
|
||||
.getEnclosingCallable()
|
||||
.getName()
|
||||
/*
|
||||
* Regex: (_)* :
|
||||
* some methods have underscores.
|
||||
* Regex: (no|ignore|disable)(strictssl|ssl|verify|verification|hostname)
|
||||
* noStrictSSL ignoreSsl
|
||||
* Regex: (set)?(accept|trust|ignore|allow)(all|every|any)
|
||||
* acceptAll trustAll ignoreAll setTrustAnyHttps
|
||||
* Regex: (use|do|enable)insecure
|
||||
* useInsecureSSL
|
||||
* Regex: (set|do|use)?no.*(check|validation|verify|verification)
|
||||
* setNoCertificateCheck
|
||||
* Regex: disable
|
||||
* disableChecks
|
||||
*/
|
||||
|
||||
.regexpMatch("^(?i)(_)*((no|ignore|disable)(strictssl|ssl|verify|verification|hostname)" +
|
||||
"|(set)?(accept|trust|ignore|allow)(all|every|any)" +
|
||||
"|(use|do|enable)insecure|(set|do|use)?no.*(check|validation|verify|verification)|disable).*$")
|
||||
}
|
||||
}
|
||||
|
||||
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`.
|
||||
*/
|
||||
private class FlagType extends Type {
|
||||
FlagType() {
|
||||
this instanceof TypeString
|
||||
or
|
||||
this instanceof BooleanType
|
||||
}
|
||||
}
|
||||
|
||||
private predicate isEqualsIgnoreCaseMethodAccess(MethodAccess ma) {
|
||||
ma.getMethod().hasName("equalsIgnoreCase") and
|
||||
ma.getMethod().getDeclaringType() instanceof TypeString
|
||||
}
|
||||
|
||||
/** 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. */
|
||||
private predicate isNodeGuardedByFlag(DataFlow::Node node) {
|
||||
exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) | g = getAGuard())
|
||||
}
|
||||
|
||||
from
|
||||
DataFlow::PathNode source, DataFlow::PathNode sink, TrustAllHostnameVerifierConfiguration cfg,
|
||||
RefType verifier
|
||||
where
|
||||
cfg.hasFlowPath(source, sink) and
|
||||
not isNodeGuardedByFlag(sink.getNode()) and
|
||||
verifier = source.getNode().asExpr().(ClassInstanceExpr).getConstructedType()
|
||||
select sink, source, sink,
|
||||
"$@ that is defined $@ and accepts any certificate as valid, is used here.", source,
|
||||
"This hostname verifier", verifier, "here"
|
||||
Reference in New Issue
Block a user