Skip to content

Commit c8aca06

Browse files
Implement pinning through a TrustManager
+ Fix that the query was accidentally placed in experimental
1 parent 4afecf5 commit c8aca06

File tree

2 files changed

+62
-2
lines changed

2 files changed

+62
-2
lines changed

java/ql/lib/semmle/code/java/security/AndroidCertificatePinningQuery.qll

Lines changed: 62 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,66 @@ predicate trustedDomain(string domainName) {
5252
trustedDomainViaOkHttp(domainName)
5353
}
5454

55+
/**
56+
* Holds if `setSocketFactory` is a call to `HttpsURLConnection.setSSLSocketFactory` or `HttpsURLConnection.setDefaultSSLSocketFactory`
57+
* that uses a socket factory derrived from a `TrustManager`.
58+
* `default` is true if the default SSL socket factory for all URLs is being set.
59+
*/
60+
private predicate trustedSocketFactory(MethodAccess setSocketFactory, boolean default) {
61+
exists(MethodAccess getSocketFactory, MethodAccess initSslContext, string methodName |
62+
setSocketFactory
63+
.getMethod()
64+
.getASourceOverriddenMethod*()
65+
.hasQualifiedName("javax.net.ssl", "HttpsURLConnection", methodName) and
66+
(
67+
default = true and methodName = "setDefaultSSLSocketFactory"
68+
or
69+
default = false and methodName = "setSSLSocketFactory"
70+
) and
71+
initSslContext.getMethod().hasQualifiedName("javax.net.ssl", "SSLContext", "init") and
72+
getSocketFactory
73+
.getMethod()
74+
.getASourceOverriddenMethod*()
75+
.hasQualifiedName("javax.net.ssl", "SSLContext", "getSocketFactory") and
76+
not initSslContext.getArgument(1) instanceof NullLiteral and
77+
DataFlow::localExprFlow(initSslContext.getQualifier(), getSocketFactory.getQualifier()) and
78+
DataFlow::localExprFlow(getSocketFactory, setSocketFactory.getArgument(0))
79+
)
80+
}
81+
82+
/**
83+
* Holds if the given expression is an qualifier to a `URL.openConnection` or `URL.openStream` call
84+
* that is trusted due to its SSL socket factory being set.
85+
*/
86+
private predicate trustedUrlConnection(Expr url) {
87+
exists(MethodAccess openCon |
88+
openCon
89+
.getMethod()
90+
.getASourceOverriddenMethod*()
91+
.hasQualifiedName("java.net", "URL", "openConnection") and
92+
url = openCon.getQualifier() and
93+
exists(MethodAccess setSocketFactory |
94+
trustedSocketFactory(setSocketFactory, false) and
95+
TaintTracking::localExprTaint(openCon, setSocketFactory.getQualifier())
96+
)
97+
)
98+
or
99+
trustedSocketFactory(_, true) and
100+
exists(MethodAccess open |
101+
open.getMethod()
102+
.getASourceOverriddenMethod*()
103+
.hasQualifiedName("java.net", "URL", ["openConnection", "openStream"]) and
104+
url = open.getQualifier()
105+
)
106+
}
107+
108+
private class MissingPinningSink extends DataFlow::Node {
109+
MissingPinningSink() {
110+
this instanceof UrlOpenSink and
111+
not trustedUrlConnection(this.asExpr())
112+
}
113+
}
114+
55115
/** Configuration for finding uses of non trusted URLs. */
56116
private class UntrustedUrlConfig extends TaintTracking::Configuration {
57117
UntrustedUrlConfig() { this = "UntrustedUrlConfig" }
@@ -64,13 +124,13 @@ private class UntrustedUrlConfig extends TaintTracking::Configuration {
64124
)
65125
}
66126

67-
override predicate isSink(DataFlow::Node node) { node instanceof UrlOpenSink }
127+
override predicate isSink(DataFlow::Node node) { node instanceof MissingPinningSink }
68128
}
69129

70130
/** Holds if `node` is a network communication call for which certificate pinning is not implemented. */
71131
predicate missingPinning(DataFlow::Node node) {
72132
isAndroid() and
73-
node instanceof UrlOpenSink and
133+
node instanceof MissingPinningSink and
74134
(
75135
not exists(string s | trustedDomain(s))
76136
or

0 commit comments

Comments
 (0)