Merge branch 'main' into repeatedWord

java/ql/src/CHANGELOG.md

@@ -1,3 +1,17 @@
+## 0.3.1
+
+## 0.3.0
+
+### Breaking Changes
+
+* Contextual queries and the query libraries they depend on have been moved to the `codeql/java-all` package.
+
+### New Queries
+
+* A new query "Improper verification of intent by broadcast receiver" (`java/improper-intent-verification`) has been added. 
+  This query finds instances of Android `BroadcastReceiver`s that don't verify the action string of received intents when registered
+  to receive system intents.
+
 ## 0.2.0
 
 ### Minor Analysis Improvements

java/ql/src/Likely Bugs/Collections/ContainsTypeMismatch.ql

@@ -118,7 +118,7 @@ class MismatchedContainerAccess extends MethodAccess {
       containerAccess(package, type, p, this.getCallee().getSignature(), i)
     |
       t = this.getCallee().getDeclaringType() and
-      t.getAnAncestor().getSourceDeclaration() = g and
+      t.getASourceSupertype*().getSourceDeclaration() = g and
       g.hasQualifiedName(package, type) and
       indirectlyInstantiates(t, g, p, result)
     )
@@ -139,7 +139,7 @@ from MismatchedContainerAccess ma, RefType typearg, RefType argtype, int idx
 where
   typearg = ma.getReceiverElementType(idx).getSourceDeclaration() and
   argtype = ma.getArgumentType(idx) and
-  not haveIntersection(typearg, argtype)
+  notHaveIntersection(typearg, argtype)
 select ma.getArgument(idx),
   "Actual argument type '" + argtype.getName() + "'" +
     " is incompatible with expected argument type '" + typearg.getName() + "'."

java/ql/src/Likely Bugs/Collections/RemoveTypeMismatch.ql

@@ -88,7 +88,7 @@ class MismatchedContainerModification extends MethodAccess {
       containerModification(package, type, p, this.getCallee().getSignature(), i)
     |
       t = this.getCallee().getDeclaringType() and
-      t.getAnAncestor().getSourceDeclaration() = g and
+      t.getASourceSupertype*().getSourceDeclaration() = g and
       g.hasQualifiedName(package, type) and
       indirectlyInstantiates(t, g, p, result)
     )
@@ -109,7 +109,7 @@ from MismatchedContainerModification ma, RefType elementtype, RefType argtype, i
 where
   elementtype = ma.getReceiverElementType(idx).getSourceDeclaration() and
   argtype = ma.getArgumentType(idx) and
-  not haveIntersection(elementtype, argtype)
+  notHaveIntersection(elementtype, argtype)
 select ma.getArgument(idx),
   "Actual argument type '" + argtype.getName() + "'" +
     " is incompatible with expected argument type '" + elementtype.getName() + "'."

java/ql/src/Likely Bugs/Comparison/IncomparableEquals.ql

@@ -57,7 +57,7 @@ where
     else recvtp = ma.getMethod().getDeclaringType()
   ) and
   argtp = ma.getArgumentType() and
-  not haveIntersection(recvtp, argtp)
+  notHaveIntersection(recvtp, argtp)
 select ma,
   "Call to equals() comparing incomparable types " + recvtp.getName() + " and " + argtp.getName() +
     "."

java/ql/src/Security/CWE/CWE-022/TaintedPath.ql

@@ -15,6 +15,7 @@
 
 import java
 import semmle.code.java.dataflow.FlowSources
+private import semmle.code.java.dataflow.ExternalFlow
 import semmle.code.java.security.PathCreation
 import DataFlow::PathGraph
 import TaintedPathCommon
@@ -34,7 +35,12 @@ class TaintedPathConfig extends TaintTracking::Configuration {
   override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) {
-    exists(Expr e | e = sink.asExpr() | e = any(PathCreation p).getAnInput() and not guarded(e))
+    (
+      sink.asExpr() = any(PathCreation p).getAnInput()
+      or
+      sinkNode(sink, "create-file")
+    ) and
+    not guarded(sink.asExpr())
   }
 
   override predicate isSanitizer(DataFlow::Node node) {
@@ -44,9 +50,21 @@ class TaintedPathConfig extends TaintTracking::Configuration {
   }
 }
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, PathCreation p, TaintedPathConfig conf
-where
-  sink.getNode().asExpr() = p.getAnInput() and
-  conf.hasFlowPath(source, sink)
-select p, source, sink, "$@ flows to here and is used in a path.", source.getNode(),
-  "User-provided value"
+/**
+ * Gets the data-flow node at which to report a path ending at `sink`.
+ *
+ * Previously this query flagged alerts exclusively at `PathCreation` sites,
+ * so to avoid perturbing existing alerts, where a `PathCreation` exists we
+ * continue to report there; otherwise we report directly at `sink`.
+ */
+DataFlow::Node getReportingNode(DataFlow::Node sink) {
+  any(TaintedPathConfig c).hasFlowTo(sink) and
+  if exists(PathCreation pc | pc.getAnInput() = sink.asExpr())
+  then result.asExpr() = any(PathCreation pc | pc.getAnInput() = sink.asExpr())
+  else result = sink
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, TaintedPathConfig conf
+where conf.hasFlowPath(source, sink)
+select getReportingNode(sink.getNode()), source, sink, "$@ flows to here and is used in a path.",
+  source.getNode(), "User-provided value"

java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertificateValidation.java

@@ -0,0 +1,22 @@
+class Bad extends WebViewClient {
+    // BAD: All certificates are trusted.
+    public void onReceivedSslError (WebView view, SslErrorHandler handler, SslError error) { // $hasResult
+        handler.proceed(); 
+    }
+}
+
+class Good extends WebViewClient {
+    PublicKey myPubKey = ...;
+
+    // GOOD: Only certificates signed by a certain public key are trusted.
+    public void onReceivedSslError (WebView view, SslErrorHandler handler, SslError error) { // $hasResult
+        try {
+            X509Certificate cert = error.getCertificate().getX509Certificate();
+            cert.verify(this.myPubKey);
+            handler.proceed();
+        }
+        catch (CertificateException|NoSuchAlgorithmException|InvalidKeyException|NoSuchProviderException|SignatureException e) {
+            handler.cancel();
+        }
+    }    
+}

java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertificateValidation.qhelp

@@ -0,0 +1,46 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+If the <code>onReceivedSslError</code> method of an Android <code>WebViewClient</code> always calls <code>proceed</code> on the given <code>SslErrorHandler</code>, it trusts any certificate. 
+This allows an attacker to perform a machine-in-the-middle attack against the application, therefore breaking any security Transport Layer Security (TLS) gives.
+</p>
+
+<p>
+An attack might look like this:
+</p>
+
+<ol>
+  <li>The vulnerable application connects to <code>https://example.com</code>.</li>
+  <li>The attacker intercepts this connection and presents a valid, self-signed certificate for <code>https://example.com</code>.</li>
+  <li>The vulnerable application calls the <code>onReceivedSslError</code> method to check whether it should trust the certificate.</li>
+  <li>The <code>onReceivedSslError</code> method of your <code>WebViewClient</code> calls <code>SslErrorHandler.proceed</code>.</li>
+  <li>The vulnerable application accepts the certificate and proceeds with the connection since your <code>WevViewClient</code> trusted it by proceeding.</li>
+  <li>The attacker can now read the data your application sends to <code>https://example.com</code> and/or alter its replies while the application thinks the connection is secure.</li>
+</ol>
+</overview>
+
+<recommendation>
+<p>
+Do not use a call <code>SslerrorHandler.proceed</code> unconditionally. 
+If you have to use a self-signed certificate, only accept that certificate, not all certificates. 
+</p>
+
+</recommendation>
+
+<example>
+<p>
+In the first (bad) example, the <code>WebViewClient</code> trusts all certificates by always calling <code>SslErrorHandler.proceed</code>. 
+In the second (good) example, only certificates signed by a certain public key are accepted.
+</p>
+<sample src="ImproperWebViewCertificateValidation.java" />
+</example>
+
+<references>
+<li>
+<a href="https://developer.android.com/reference/android/webkit/WebViewClient?hl=en#onReceivedSslError(android.webkit.WebView,%20android.webkit.SslErrorHandler,%20android.net.http.SslError)">WebViewClient.onReceivedSslError documentation</a>.
+</li>
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertificateValidation.ql

@@ -0,0 +1,18 @@
+/**
+ * @name Android `WebView` that accepts all certificates
+ * @description Trusting all certificates allows an attacker to perform a machine-in-the-middle attack.
+ * @kind problem
+ * @problem.severity error
+ * @security-severity 7.5
+ * @precision high
+ * @id java/improper-webview-certificate-validation
+ * @tags security
+ *       external/cwe/cwe-295
+ */
+
+import java
+import semmle.code.java.security.AndroidWebViewCertificateValidationQuery
+
+from OnReceivedSslErrorMethod m
+where trustsAllCerts(m)
+select m, "This handler accepts all SSL certificates."

java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql

@@ -60,13 +60,43 @@ private predicate candidateMethod(RefType t, Method m, string name, int numParam
   not whitelist(name)
 }
 
-pragma[inline]
-private predicate potentiallyConfusingTypes(Type a, Type b) {
-  exists(RefType commonSubtype | hasSubtypeOrInstantiation*(a, commonSubtype) |
-    hasSubtypeOrInstantiation*(b, commonSubtype)
+predicate paramTypePair(Type t1, Type t2) {
+  exists(Method n, Method m, int i |
+    overloadedMethodsMostSpecific(n, m) and
+    t1 = n.getParameterType(i) and
+    t2 = m.getParameterType(i)
   )
+}
+
+// handle simple cases separately
+predicate potentiallyConfusingTypesSimple(Type t1, Type t2) {
+  paramTypePair(t1, t2) and
+  (
+    t1 = t2
+    or
+    t1 instanceof TypeObject and t2 instanceof RefType
+    or
+    t2 instanceof TypeObject and t1 instanceof RefType
+    or
+    confusingPrimitiveBoxedTypes(t1, t2)
+  )
+}
+
+// check erased types first
+predicate potentiallyConfusingTypesRefTypes(RefType t1, RefType t2) {
+  paramTypePair(t1, t2) and
+  not potentiallyConfusingTypesSimple(t1, t2) and
+  haveIntersection(t1, t2)
+}
+
+// then check hasSubtypeOrInstantiation
+predicate potentiallyConfusingTypes(Type t1, Type t2) {
+  potentiallyConfusingTypesSimple(t1, t2)
   or
-  confusingPrimitiveBoxedTypes(a, b)
+  potentiallyConfusingTypesRefTypes(t1, t2) and
+  exists(RefType commonSubtype | hasSubtypeOrInstantiation*(t1, commonSubtype) |
+    hasSubtypeOrInstantiation*(t2, commonSubtype)
+  )
 }
 
 private predicate hasSubtypeOrInstantiation(RefType t, RefType sub) {

java/ql/src/Violations of Best Practice/Naming Conventions/SameNameAsSuper.ql

@@ -16,5 +16,5 @@ from RefType sub, RefType sup
 where
   sub.fromSource() and
   sup = sub.getASupertype() and
-  sub.getName() = sup.getName()
+  pragma[only_bind_out](sub.getName()) = pragma[only_bind_out](sup.getName())
 select sub, sub.getName() + " has the same name as its supertype $@.", sup, sup.getQualifiedName()

java/ql/src/change-notes/2022-04-27-intent-verification.md

@@ -1,6 +0,0 @@
----
-category: newQuery
----
-* A new query "Improper verification of intent by broadcast receiver" (`java/improper-intent-verification`) has been added. 
-This query finds instances of Android `BroadcastReceiver`s that don't verify the action string of received intents when registered
-to receive system intents.

java/ql/src/change-notes/2022-06-22-improper-webview-certificate-validation.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* A new query "Android `WebView` that accepts all certificates" (`java/improper-webview-certificate-validation`) has been added. This query finds implementations of `WebViewClient`s that accept all certificates in the case of an SSL error.

java/ql/src/change-notes/2022-06-29-move-contextual-queries.md

@@ -1,4 +0,0 @@
----
-category: breaking
----
-* Contextual queries and the query libraries they depend on have been moved to the `codeql/java-all` package.

java/ql/src/change-notes/2022-08-03-tainted-path-mad.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The query `java/path-injection` now recognises vulnerable APIs defined using the `SinkModelCsv` class with the `create-file` type. Out of the box this includes Apache Commons-IO functions, as well as any user-defined sinks.

java/ql/src/change-notes/released/0.3.0.md

@@ -0,0 +1,11 @@
+## 0.3.0
+
+### Breaking Changes
+
+* Contextual queries and the query libraries they depend on have been moved to the `codeql/java-all` package.
+
+### New Queries
+
+* A new query "Improper verification of intent by broadcast receiver" (`java/improper-intent-verification`) has been added. 
+  This query finds instances of Android `BroadcastReceiver`s that don't verify the action string of received intents when registered
+  to receive system intents.

java/ql/src/change-notes/released/0.3.1.md

@@ -0,0 +1 @@
+## 0.3.1

java/ql/src/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.2.0
+lastReleaseVersion: 0.3.1

java/ql/src/experimental/Security/CWE/CWE-327/Azure/UnsafeUsageOfClientSideEncryptionVersion.java

@@ -0,0 +1,46 @@
+
+// BAD: Using an outdated SDK that does not support client side encryption version V2_0
+new EncryptedBlobClientBuilder()
+        .blobClient(blobClient)
+        .key(resolver.buildAsyncKeyEncryptionKey(keyid).block(), keyWrapAlgorithm)
+        .buildEncryptedBlobClient()
+        .uploadWithResponse(new BlobParallelUploadOptions(data)
+                        .setMetadata(metadata)
+                        .setHeaders(headers)
+                        .setTags(tags)
+                        .setTier(tier)
+                        .setRequestConditions(requestConditions)
+                        .setComputeMd5(computeMd5)
+                        .setParallelTransferOptions(parallelTransferOptions),
+                timeout, context);
+
+// BAD: Using the deprecatedd client side encryption version V1_0
+new EncryptedBlobClientBuilder(EncryptionVersion.V1)
+        .blobClient(blobClient)
+        .key(resolver.buildAsyncKeyEncryptionKey(keyid).block(), keyWrapAlgorithm)
+        .buildEncryptedBlobClient()
+        .uploadWithResponse(new BlobParallelUploadOptions(data)
+                        .setMetadata(metadata)
+                        .setHeaders(headers)
+                        .setTags(tags)
+                        .setTier(tier)
+                        .setRequestConditions(requestConditions)
+                        .setComputeMd5(computeMd5)
+                        .setParallelTransferOptions(parallelTransferOptions),
+                timeout, context);
+
+
+// GOOD: Using client side encryption version V2_0
+new EncryptedBlobClientBuilder(EncryptionVersion.V2)
+        .blobClient(blobClient)
+        .key(resolver.buildAsyncKeyEncryptionKey(keyid).block(), keyWrapAlgorithm)
+        .buildEncryptedBlobClient()
+        .uploadWithResponse(new BlobParallelUploadOptions(data)
+                        .setMetadata(metadata)
+                        .setHeaders(headers)
+                        .setTags(tags)
+                        .setTier(tier)
+                        .setRequestConditions(requestConditions)
+                        .setComputeMd5(computeMd5)
+                        .setParallelTransferOptions(parallelTransferOptions),
+                timeout, context);

java/ql/src/experimental/Security/CWE/CWE-327/Azure/UnsafeUsageOfClientSideEncryptionVersion.qhelp

@@ -0,0 +1,29 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+
+  <overview>
+    <p>Azure Storage .NET, Java, and Python SDKs support encryption on the client with a customer-managed key that is maintained in Azure Key Vault or another key store.</p>
+    <p>The Azure Storage SDK version 12.18.0 or later supports version <code>V2</code> for client-side encryption. All previous versions of Azure Storage SDK only support client-side encryption <code>V1</code> which is unsafe.</p>
+
+  </overview>
+  <recommendation>
+
+    <p>Consider switching to <code>V2</code> client-side encryption.</p>
+
+  </recommendation>
+  <example>
+
+    <sample src="UnsafeUsageOfClientSideEncryptionVersion.java" />
+
+  </example>
+  <references>
+    <li>
+      <a href="http://aka.ms/azstorageclientencryptionblog">Azure Storage Client Encryption Blog.</a>
+    </li>
+    <li>
+      <a href="https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-30187">CVE-2022-30187</a>
+    </li>
+
+  </references>
+</qhelp>

java/ql/src/experimental/Security/CWE/CWE-327/Azure/UnsafeUsageOfClientSideEncryptionVersion.ql

@@ -0,0 +1,92 @@
+/**
+ * @name Unsafe usage of v1 version of Azure Storage client-side encryption (CVE-2022-30187).
+ * @description Unsafe usage of v1 version of Azure Storage client-side encryption, please refer to http://aka.ms/azstorageclientencryptionblog
+ * @kind problem
+ * @tags security
+ *       cryptography
+ *       external/cwe/cwe-327
+ * @id java/azure-storage/unsafe-client-side-encryption-in-use
+ * @problem.severity error
+ * @precision high
+ */
+
+import java
+import semmle.code.java.dataflow.DataFlow
+
+/**
+ * Holds if `call` is an object creation for a class `EncryptedBlobClientBuilder`
+ * that takes no arguments, which means that it is using V1 encryption.
+ */
+predicate isCreatingOutdatedAzureClientSideEncryptionObject(Call call, Class c) {
+  exists(string package, string type, Constructor constructor |
+    c.hasQualifiedName(package, type) and
+    c.getAConstructor() = constructor and
+    call.getCallee() = constructor and
+    (
+      type = "EncryptedBlobClientBuilder" and
+      package = "com.azure.storage.blob.specialized.cryptography" and
+      constructor.hasNoParameters()
+      or
+      type = "BlobEncryptionPolicy" and package = "com.microsoft.azure.storage.blob"
+    )
+  )
+}
+
+/**
+ * Holds if `call` is an object creation for a class `EncryptedBlobClientBuilder`
+ * that takes `versionArg` as the argument specifying the encryption version.
+ */
+predicate isCreatingAzureClientSideEncryptionObjectNewVersion(Call call, Class c, Expr versionArg) {
+  exists(string package, string type, Constructor constructor |
+    c.hasQualifiedName(package, type) and
+    c.getAConstructor() = constructor and
+    call.getCallee() = constructor and
+    type = "EncryptedBlobClientBuilder" and
+    package = "com.azure.storage.blob.specialized.cryptography" and
+    versionArg = call.getArgument(0)
+  )
+}
+
+/**
+ * A dataflow config that tracks `EncryptedBlobClientBuilder.version` argument initialization.
+ */
+private class EncryptedBlobClientBuilderSafeEncryptionVersionConfig extends DataFlow::Configuration {
+  EncryptedBlobClientBuilderSafeEncryptionVersionConfig() {
+    this = "EncryptedBlobClientBuilderSafeEncryptionVersionConfig"
+  }
+
+  override predicate isSource(DataFlow::Node source) {
+    exists(FieldRead fr, Field f | fr = source.asExpr() |
+      f.getAnAccess() = fr and
+      f.hasQualifiedName("com.azure.storage.blob.specialized.cryptography", "EncryptionVersion",
+        "V2")
+    )
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    isCreatingAzureClientSideEncryptionObjectNewVersion(_, _, sink.asExpr())
+  }
+}
+
+/**
+ * Holds if `call` is an object creation for a class `EncryptedBlobClientBuilder`
+ * that takes `versionArg` as the argument specifying the encryption version, and that version is safe.
+ */
+predicate isCreatingSafeAzureClientSideEncryptionObject(Call call, Class c, Expr versionArg) {
+  isCreatingAzureClientSideEncryptionObjectNewVersion(call, c, versionArg) and
+  exists(EncryptedBlobClientBuilderSafeEncryptionVersionConfig config, DataFlow::Node sink |
+    sink.asExpr() = versionArg
+  |
+    config.hasFlow(_, sink)
+  )
+}
+
+from Expr e, Class c
+where
+  exists(Expr argVersion |
+    isCreatingAzureClientSideEncryptionObjectNewVersion(e, c, argVersion) and
+    not isCreatingSafeAzureClientSideEncryptionObject(e, c, argVersion)
+  )
+  or
+  isCreatingOutdatedAzureClientSideEncryptionObject(e, c)
+select e, "Unsafe usage of v1 version of Azure Storage client-side encryption."

java/ql/src/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/java-queries
-version: 0.2.1-dev
+version: 0.3.2-dev
 groups: 
   - java
   - queries