Skip to content

Commit 149cae9

Browse files
atorralbaatorralba
authored
Merge pull request #10971 from joefarebrother/android-certificate-pinning
Java: Add Android missing certificate pinning query (CWE-295)
2 parents cbe330e + 3e7a819 commit 149cae9

File tree

86 files changed

+1868
-2618
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

86 files changed

+1868
-2618
lines changed

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

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
/** Definitions for the Android Missing Certificate Pinning query. */
2+
3+
import java
4+
import semmle.code.xml.AndroidManifest
5+
import semmle.code.java.dataflow.TaintTracking
6+
import semmle.code.java.frameworks.Networking
7+
import semmle.code.java.security.Encryption
8+
import semmle.code.java.security.HttpsUrls
9+
10+
/** An Android Network Security Configuration XML file. */
11+
class AndroidNetworkSecurityConfigFile extends XmlFile {
12+
AndroidNetworkSecurityConfigFile() {
13+
exists(AndroidApplicationXmlElement app, AndroidXmlAttribute confAttr, string confName |
14+
confAttr.getElement() = app and
15+
confAttr.getValue() = "@xml/" + confName and
16+
this.getRelativePath().matches("%res/xml/" + confName + ".xml") and
17+
this.getARootElement().getName() = "network-security-config"
18+
)
19+
}
20+
}
21+
22+
/** Holds if this database is of an Android application. */
23+
predicate isAndroid() { exists(AndroidManifestXmlFile m) }
24+
25+
/** Holds if the given domain name is trusted by the Network Security Configuration XML file. */
26+
private predicate trustedDomainViaXml(string domainName) {
27+
exists(
28+
AndroidNetworkSecurityConfigFile confFile, XmlElement domConf, XmlElement domain,
29+
XmlElement trust
30+
|
31+
domConf.getFile() = confFile and
32+
domConf.getName() = "domain-config" and
33+
domain.getParent() = domConf and
34+
domain.getName() = "domain" and
35+
domain.getACharactersSet().getCharacters() = domainName and
36+
trust.getParent() = domConf and
37+
trust.getName() = ["trust-anchors", "pin-set"]
38+
)
39+
}
40+
41+
/** Holds if the given domain name is trusted by an OkHttp `CertificatePinner`. */
42+
private predicate trustedDomainViaOkHttp(string domainName) {
43+
exists(CompileTimeConstantExpr domainExpr, MethodAccess certPinnerAdd |
44+
domainExpr.getStringValue().replaceAll("*.", "") = domainName and // strip wildcard patterns like *.example.com
45+
certPinnerAdd.getMethod().hasQualifiedName("okhttp3", "CertificatePinner$Builder", "add") and
46+
DataFlow::localExprFlow(domainExpr, certPinnerAdd.getArgument(0))
47+
)
48+
}
49+
50+
/** Holds if the given domain name is trusted by some certificate pinning implementation. */
51+
predicate trustedDomain(string domainName) {
52+
trustedDomainViaXml(domainName)
53+
or
54+
trustedDomainViaOkHttp(domainName)
55+
}
56+
57+
/**
58+
* Holds if `setSocketFactory` is a call to `HttpsURLConnection.setSSLSocketFactory` or `HttpsURLConnection.setDefaultSSLSocketFactory`
59+
* that uses a socket factory derived from a `TrustManager`.
60+
* `default` is true if the default SSL socket factory for all URLs is being set.
61+
*/
62+
private predicate trustedSocketFactory(MethodAccess setSocketFactory, boolean default) {
63+
exists(MethodAccess getSocketFactory, MethodAccess initSslContext |
64+
exists(Method m | setSocketFactory.getMethod() = m |
65+
default = true and m instanceof SetDefaultConnectionFactoryMethod
66+
or
67+
default = false and m instanceof SetConnectionFactoryMethod
68+
) and
69+
initSslContext.getMethod().getDeclaringType() instanceof SslContext and
70+
initSslContext.getMethod().hasName("init") and
71+
getSocketFactory.getMethod().getASourceOverriddenMethod*() instanceof GetSocketFactory and
72+
not initSslContext.getArgument(1) instanceof NullLiteral and
73+
DataFlow::localExprFlow(initSslContext.getQualifier(), getSocketFactory.getQualifier()) and
74+
DataFlow::localExprFlow(getSocketFactory, setSocketFactory.getArgument(0))
75+
)
76+
}
77+
78+
/**
79+
* Holds if the given expression is an qualifier to a `URL.openConnection` or `URL.openStream` call
80+
* that is trusted due to its SSL socket factory being set.
81+
*/
82+
private predicate trustedUrlConnection(Expr url) {
83+
exists(MethodAccess openCon |
84+
openCon.getMethod().getASourceOverriddenMethod*() instanceof UrlOpenConnectionMethod and
85+
url = openCon.getQualifier() and
86+
exists(MethodAccess setSocketFactory |
87+
trustedSocketFactory(setSocketFactory, false) and
88+
TaintTracking::localExprTaint(openCon, setSocketFactory.getQualifier())
89+
)
90+
)
91+
or
92+
trustedSocketFactory(_, true) and
93+
exists(MethodAccess open, Method m |
94+
m instanceof UrlOpenConnectionMethod or m instanceof UrlOpenStreamMethod
95+
|
96+
open.getMethod().getASourceOverriddenMethod*() = m and
97+
url = open.getQualifier()
98+
)
99+
}
100+
101+
private class MissingPinningSink extends DataFlow::Node {
102+
MissingPinningSink() {
103+
this instanceof UrlOpenSink and
104+
not trustedUrlConnection(this.asExpr())
105+
}
106+
}
107+
108+
/** Configuration for finding uses of non trusted URLs. */
109+
private class UntrustedUrlConfig extends TaintTracking::Configuration {
110+
UntrustedUrlConfig() { this = "UntrustedUrlConfig" }
111+
112+
override predicate isSource(DataFlow::Node node) {
113+
trustedDomain(_) and
114+
exists(string lit | lit = node.asExpr().(CompileTimeConstantExpr).getStringValue() |
115+
lit.matches("%://%") and // it's a URL
116+
not exists(string dom | trustedDomain(dom) and lit.matches("%" + dom + "%"))
117+
)
118+
}
119+
120+
override predicate isSink(DataFlow::Node node) { node instanceof MissingPinningSink }
121+
}
122+
123+
/** Holds if `node` is a network communication call for which certificate pinning is not implemented. */
124+
predicate missingPinning(DataFlow::Node node, string domain) {
125+
isAndroid() and
126+
node instanceof MissingPinningSink and
127+
(
128+
not trustedDomain(_) and domain = ""
129+
or
130+
exists(UntrustedUrlConfig conf, DataFlow::Node src |
131+
conf.hasFlow(src, node) and
132+
domain = getDomain(src.asExpr())
133+
)
134+
)
135+
}
136+
137+
/** Gets the domain name from the given string literal */
138+
private string getDomain(CompileTimeConstantExpr expr) {
139+
result = expr.getStringValue().regexpCapture("(https?://)?([^/]*)(/.*)?", 2)
140+
}

‎java/ql/lib/semmle/code/java/security/Encryption.qll

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,13 +143,22 @@ class CreateSslEngineMethod extends Method {
143143
}
144144
}
145145

146+
/** The `setConnectionFactory` method of the class `javax.net.ssl.HttpsURLConnection`. */
146147
class SetConnectionFactoryMethod extends Method {
147148
SetConnectionFactoryMethod() {
148149
this.hasName("setSSLSocketFactory") and
149150
this.getDeclaringType().getAnAncestor() instanceof HttpsUrlConnection
150151
}
151152
}
152153

154+
/** The `setDefaultConnectionFactory` method of the class `javax.net.ssl.HttpsURLConnection`. */
155+
class SetDefaultConnectionFactoryMethod extends Method {
156+
SetDefaultConnectionFactoryMethod() {
157+
this.hasName("setDefaultSSLSocketFactory") and
158+
this.getDeclaringType().getAnAncestor() instanceof HttpsUrlConnection
159+
}
160+
}
161+
153162
class SetHostnameVerifierMethod extends Method {
154163
SetHostnameVerifierMethod() {
155164
this.hasName("setHostnameVerifier") and

‎java/ql/src/Security/CWE/CWE-295/AndroidMissingCertificatePinning.qhelp

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
Certificate pinning is the practice of only trusting a specific set of SSL certificates, rather than those that the device trusts by default.
8+
In Android applications, it is reccomended to use certificate pinning when communicating over the network,
9+
in order to minimize the risk of machine-in-the-middle attacks from a compromised CA.
10+
</p>
11+
</overview>
12+
13+
<recommendation>
14+
<p>
15+
The easiest way to implement certificate pinning is to declare your pins in a <code>network-security-config</code> XML file.
16+
This will automatically provide certificate pinning for any network connection made by the app.
17+
</p>
18+
<p>
19+
Another way to implement certificate pinning is to use the `CertificatePinner` class from the `okhttp` library.
20+
</p>
21+
<p>
22+
A final way to implement certificate pinning is to use a <code>TrustManager</code>, initialized from a <code>KeyStore</code> loaded with only the necessary certificates.
23+
</p>
24+
25+
</recommendation>
26+
27+
<example>
28+
<p>
29+
In the first (bad) case below, a network call is performed with no certificate pinning implemented.
30+
The other (good) cases demonstrate the different ways to implement certificate pinning.
31+
</p>
32+
<sample src="AndroidMissingCertificatePinning1.java" />
33+
<sample src="AndroidMissingCertificatePinning2.xml" />
34+
<sample src="AndroidMissingCertificatePinning3.java" />
35+
</example>
36+
37+
<references>
38+
<li>
39+
OWASP Mobile Security: <a href="https://mobile-security.gitbook.io/mobile-security-testing-guide/android-testing-guide/0x05g-testing-network-communication#testing-custom-certificate-stores-and-certificate-pinning-mstg-network-4">Testing Custom Certificate Stores and Certificate Pinning (MSTG-NETWORK-4)</a>.
40+
</li>
41+
<li>
42+
Android Developers: <a href="https://developer.android.com/training/articles/security-config">Network security configuration</a>.
43+
</li>
44+
<li>
45+
OkHttp: <a href="https://square.github.io/okhttp/4.x/okhttp/okhttp3/-certificate-pinner/">CertificatePinner</a>.
46+
</li>
47+
</references>
48+
</qhelp>

‎java/ql/src/Security/CWE/CWE-295/AndroidMissingCertificatePinning.ql

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* @name Android missing certificate pinning
3+
* @description Network connections that do not use certificate pinning may allow attackers to eavesdrop on communications.
4+
* @kind problem
5+
* @problem.severity warning
6+
* @security-severity 5.9
7+
* @precision medium
8+
* @id java/android/missing-certificate-pinning
9+
* @tags security
10+
* external/cwe/cwe-295
11+
*/
12+
13+
import java
14+
import semmle.code.java.security.AndroidCertificatePinningQuery
15+
16+
from DataFlow::Node node, string domain, string msg
17+
where
18+
missingPinning(node, domain) and
19+
if domain = ""
20+
then msg = "(no explicitly trusted domains)"
21+
else msg = "(" + domain + " is not trusted by a pin)"
22+
select node, "This network call does not implement certificate pinning. " + msg

‎java/ql/src/Security/CWE/CWE-295/AndroidMissingCertificatePinning1.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
// BAD - By default, this network call does not use certificate pinning
2+
URLConnection conn = new URL("https://example.com").openConnection();

‎java/ql/src/Security/CWE/CWE-295/AndroidMissingCertificatePinning2.xml

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
<!-- GOOD: Certificate pinning implemented via a Network Security Config file -->
2+
3+
<!-- In AndroidManifest.xml -->
4+
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
5+
package="com.example.app">
6+
7+
<application android:networkSecurityConfig="@xml/NetworkSecurityConfig">
8+
...
9+
</application>
10+
11+
</manifest>
12+
13+
<!-- In res/xml/NetworkSecurityConfig.xml -->
14+
<network-security-config>
15+
<domain-config>
16+
<domain>good.example.com</domain>
17+
<pin-set expiration="2038/1/19">
18+
<pin digest="SHA-256">...</pin>
19+
</pin-set>
20+
</domain-config>
21+
</network-security-config>

‎java/ql/src/Security/CWE/CWE-295/AndroidMissingCertificatePinning3.java

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
// GOOD: Certificate pinning implemented via okhttp3.CertificatePinner
2+
CertificatePinner certificatePinner = new CertificatePinner.Builder()
3+
.add("example.com", "sha256/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=")
4+
.build();
5+
OkHttpClient client = new OkHttpClient.Builder()
6+
.certificatePinner(certificatePinner)
7+
.build();
8+
9+
client.newCall(new Request.Builder().url("https://example.com").build()).execute();
10+
11+
12+
13+
// GOOD: Certificate pinning implemented via a TrustManager
14+
KeyStore keyStore = KeyStore.getInstance("BKS");
15+
keyStore.load(resources.openRawResource(R.raw.cert), null);
16+
17+
TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
18+
tmf.init(keyStore);
19+
20+
SSLContext sslContext = SSLContext.getInstance("TLS");
21+
sslContext.init(null, tmf.getTrustManagers(), null);
22+
23+
URL url = new URL("http://www.example.com/");
24+
HttpsURLConnection urlConnection = (HttpsURLConnection) url.openConnection();
25+
26+
urlConnection.setSSLSocketFactory(sslContext.getSocketFactory());

‎java/ql/src/change-notes/2022-11-30-android-certificate-pinning.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query, `java/android/missing-certificate-pinning`, to find network calls where certificate pinning is not implemented.

‎java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test1/AndroidManifest.xml

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
2+
package="com.example.app"
3+
android:installLocation="auto"
4+
android:versionCode="1"
5+
android:versionName="0.1" >
6+
7+
<application android:networkSecurityConfig="@xml/NetworkSecurityConfig">
8+
</application>
9+
10+
</manifest>

‎java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test1/Test.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import java.net.URL;
2+
import java.net.URLConnection;
3+
4+
class Test{
5+
URLConnection test1() throws Exception {
6+
return new URL("https://good.example.com").openConnection();
7+
}
8+
9+
URLConnection test2() throws Exception {
10+
return new URL("https://bad.example.com").openConnection(); // $hasUntrustedResult
11+
}
12+
}

0 commit comments

Comments
 (0)