From bd8f35bef7020510abd26d28df4816843613dece Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 5 Dec 2023 16:49:36 +0100 Subject: [PATCH] Java: Fix FPs in Missing certificate pinning Local URIs should never require pinning --- .../AndroidCertificatePinningQuery.qll | 14 +++------- ...2-android-certificate-pinning-precision.md | 4 +++ .../Test1/Test.java | 16 ++++++++++-- .../Test2/Test.java | 16 ++++++++++-- .../Test3/Test.java | 25 +++++++++++------- .../Test4/Test.java | 9 ++++--- .../Test5/Test.java | 26 +++++++++++++++---- 7 files changed, 77 insertions(+), 33 deletions(-) create mode 100644 java/ql/src/change-notes/2023-12-12-android-certificate-pinning-precision.md diff --git a/java/ql/lib/semmle/code/java/security/AndroidCertificatePinningQuery.qll b/java/ql/lib/semmle/code/java/security/AndroidCertificatePinningQuery.qll index 4c42e5a3903..423df068544 100644 --- a/java/ql/lib/semmle/code/java/security/AndroidCertificatePinningQuery.qll +++ b/java/ql/lib/semmle/code/java/security/AndroidCertificatePinningQuery.qll @@ -108,9 +108,9 @@ private class MissingPinningSink extends DataFlow::Node { /** Configuration for finding uses of non trusted URLs. */ private module UntrustedUrlConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node node) { - trustedDomain(_) and exists(string lit | lit = node.asExpr().(CompileTimeConstantExpr).getStringValue() | lit.matches("%://%") and // it's a URL + not lit.regexpMatch("^(classpath|file|jar):.*") and // discard non-network URIs not exists(string dom | trustedDomain(dom) and lit.matches("%" + dom + "%")) ) } @@ -121,16 +121,10 @@ private module UntrustedUrlConfig implements DataFlow::ConfigSig { private module UntrustedUrlFlow = TaintTracking::Global; /** Holds if `node` is a network communication call for which certificate pinning is not implemented. */ -predicate missingPinning(DataFlow::Node node, string domain) { +predicate missingPinning(MissingPinningSink node, string domain) { isAndroid() and - node instanceof MissingPinningSink and - ( - not trustedDomain(_) and domain = "" - or - exists(DataFlow::Node src | - UntrustedUrlFlow::flow(src, node) and - domain = getDomain(src.asExpr()) - ) + exists(DataFlow::Node src | UntrustedUrlFlow::flow(src, node) | + if trustedDomain(_) then domain = getDomain(src.asExpr()) else domain = "" ) } diff --git a/java/ql/src/change-notes/2023-12-12-android-certificate-pinning-precision.md b/java/ql/src/change-notes/2023-12-12-android-certificate-pinning-precision.md new file mode 100644 index 00000000000..ae3742e9f83 --- /dev/null +++ b/java/ql/src/change-notes/2023-12-12-android-certificate-pinning-precision.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The query `java/android/missing-certificate-pinning` should no longer alert about requests pointing to the local filesystem. diff --git a/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test1/Test.java b/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test1/Test.java index ed141d80521..ac40be7fb39 100644 --- a/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test1/Test.java +++ b/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test1/Test.java @@ -1,7 +1,7 @@ import java.net.URL; import java.net.URLConnection; -class Test{ +class Test { URLConnection test1() throws Exception { return new URL("https://good.example.com").openConnection(); } @@ -9,4 +9,16 @@ class Test{ URLConnection test2() throws Exception { return new URL("https://bad.example.com").openConnection(); // $hasUntrustedResult } -} \ No newline at end of file + + URLConnection test3() throws Exception { + return new URL("classpath:example/directory/test.class").openConnection(); + } + + URLConnection test4() throws Exception { + return new URL("file:///example/file").openConnection(); + } + + URLConnection test5() throws Exception { + return new URL("jar:file:///C:/example/test.jar!/test.xml").openConnection(); + } +} diff --git a/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test2/Test.java b/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test2/Test.java index 9f68c503b46..05d58137b9a 100644 --- a/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test2/Test.java +++ b/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test2/Test.java @@ -1,8 +1,20 @@ import java.net.URL; import java.net.URLConnection; -class Test{ +class Test { URLConnection test2() throws Exception { return new URL("https://example.com").openConnection(); // $hasNoTrustedResult } -} \ No newline at end of file + + URLConnection test3() throws Exception { + return new URL("classpath:example/directory/test.class").openConnection(); + } + + URLConnection test4() throws Exception { + return new URL("file:///example/file").openConnection(); + } + + URLConnection test5() throws Exception { + return new URL("jar:file:///C:/example/test.jar!/test.xml").openConnection(); + } +} diff --git a/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test3/Test.java b/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test3/Test.java index 6a8ff8ed9d8..22f8d68919e 100644 --- a/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test3/Test.java +++ b/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test3/Test.java @@ -2,16 +2,21 @@ import okhttp3.OkHttpClient; import okhttp3.CertificatePinner; import okhttp3.Request; -class Test{ +class Test { void test1() throws Exception { - CertificatePinner certificatePinner = new CertificatePinner.Builder() - .add("good.example.com", "sha256/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=") - .build(); - OkHttpClient client = new OkHttpClient.Builder() - .certificatePinner(certificatePinner) - .build(); + CertificatePinner certificatePinner = new CertificatePinner.Builder() + .add("good.example.com", "sha256/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=") + .build(); + OkHttpClient client = + new OkHttpClient.Builder().certificatePinner(certificatePinner).build(); - client.newCall(new Request.Builder().url("https://good.example.com").build()).execute(); - client.newCall(new Request.Builder().url("https://bad.example.com").build()).execute(); // $hasUntrustedResult + client.newCall(new Request.Builder().url("https://good.example.com").build()).execute(); + client.newCall(new Request.Builder().url("https://bad.example.com").build()).execute(); // $hasUntrustedResult + client.newCall(new Request.Builder().url("classpath:example/directory/test.class").build()) + .execute(); + client.newCall(new Request.Builder().url("file:///example/file").build()).execute(); + client.newCall( + new Request.Builder().url("jar:file:///C:/example/test.jar!/test.xml").build()) + .execute(); } -} \ No newline at end of file +} diff --git a/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test4/Test.java b/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test4/Test.java index fd745a0ca1c..b3c6ef44786 100644 --- a/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test4/Test.java +++ b/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test4/Test.java @@ -8,19 +8,20 @@ import javax.net.ssl.HttpsURLConnection; import javax.net.ssl.SSLContext; import android.content.res.Resources; -class Test{ +class Test { void test1(Resources resources) throws Exception { KeyStore keyStore = KeyStore.getInstance("BKS"); keyStore.load(resources.openRawResource(R.raw.cert), null); - TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); + TrustManagerFactory tmf = + TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); tmf.init(keyStore); SSLContext sslContext = SSLContext.getInstance("TLS"); sslContext.init(null, tmf.getTrustManagers(), null); URL url = new URL("http://www.example.com/"); - HttpsURLConnection urlConnection = (HttpsURLConnection) url.openConnection(); + HttpsURLConnection urlConnection = (HttpsURLConnection) url.openConnection(); urlConnection.setSSLSocketFactory(sslContext.getSocketFactory()); } @@ -29,4 +30,4 @@ class Test{ URL url = new URL("http://www.example.com/"); HttpsURLConnection urlConnection = (HttpsURLConnection) url.openConnection(); // $hasNoTrustedResult } -} \ No newline at end of file +} diff --git a/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test5/Test.java b/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test5/Test.java index 00aa99775c1..159296e3196 100644 --- a/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test5/Test.java +++ b/java/ql/test/query-tests/security/CWE-295/AndroidMissingCertificatePinning/Test5/Test.java @@ -9,12 +9,13 @@ import javax.net.ssl.HttpsURLConnection; import javax.net.ssl.SSLContext; import android.content.res.Resources; -class Test{ +class Test { void init(Resources resources) throws Exception { KeyStore keyStore = KeyStore.getInstance("BKS"); keyStore.load(resources.openRawResource(R.raw.cert), null); - TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); + TrustManagerFactory tmf = + TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); tmf.init(keyStore); SSLContext sslContext = SSLContext.getInstance("TLS"); @@ -25,11 +26,26 @@ class Test{ URLConnection test1() throws Exception { URL url = new URL("http://www.example.com/"); - return url.openConnection(); + return url.openConnection(); } InputStream test2() throws Exception { URL url = new URL("http://www.example.com/"); - return url.openStream(); + return url.openStream(); } -} \ No newline at end of file + + InputStream test3() throws Exception { + URL url = new URL("classpath:example/directory/test.class"); + return url.openStream(); + } + + InputStream test4() throws Exception { + URL url = new URL("file:///example/file"); + return url.openStream(); + } + + InputStream test5() throws Exception { + URL url = new URL("jar:file:///C:/example/test.jar!/test.xml"); + return url.openStream(); + } +}