Merge pull request #15012 from atorralba/atorralba/java/fix-missing-pinning-fp

Java: Fix FPs in Missing certificate pinning

Author: atorralba

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<UntrustedUrlConfig>;
 
 /** 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 = ""
   )
 }
 

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.

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

@@ -1,12 +1,24 @@
 import java.net.URL;
 import java.net.URLConnection;
 
-class Test{
+class Test {
     URLConnection test1() throws Exception {
         return new URL("https://good.example.com").openConnection();
     }
 
     URLConnection test2() throws Exception {
         return new URL("https://bad.example.com").openConnection(); // $hasUntrustedResult
     }
-}
+
+    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();
+    }
+}

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
     }
-}
+
+    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();
+    }
+}

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

@@ -2,16 +2,21 @@
 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();
     }
-}
+}

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

@@ -8,19 +8,20 @@
 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 @@ void test2() throws Exception {
         URL url = new URL("http://www.example.com/");
         HttpsURLConnection urlConnection = (HttpsURLConnection) url.openConnection(); // $hasNoTrustedResult
     }
-}
+}

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

@@ -9,12 +9,13 @@
 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 @@ void init(Resources resources) throws Exception {
 
     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();
     }
-}
+
+    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();
+    }
+}