Merge branch 'main' into refacReDoS

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/Security/CWE/CWE-925/AndroidManifest.xml

@@ -0,0 +1,9 @@
+<manifest xmlns:android="http://schemas.android.com/apk/res/android" package="test">
+    <application>
+        <receiver android:name=".BootReceiverXml">
+            <intent-filter>
+                <action android:name="android.intent.action.BOOT_COMPLETED" />
+            </intent-filter>
+        </receiver>
+    </application>
+</manifest>

java/ql/src/Security/CWE/CWE-925/Bad.java

@@ -0,0 +1,7 @@
+public class ShutdownReceiver extends BroadcastReceiver {
+    @Override
+    public void onReceive(final Context context, final Intent intent) {
+        mainActivity.saveLocalData();
+        mainActivity.stopActivity();
+    }
+}

java/ql/src/Security/CWE/CWE-925/Good.java

@@ -0,0 +1,10 @@
+public class ShutdownReceiver extends BroadcastReceiver {
+    @Override
+    public void onReceive(final Context context, final Intent intent) {
+        if (!intent.getAction().equals(Intent.ACTION_SHUTDOWN)) {
+            return;
+        }
+        mainActivity.saveLocalData();
+        mainActivity.stopActivity();
+    }
+}

java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.qhelp

@@ -0,0 +1,40 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+
+<qhelp>
+
+<overview>
+<p>
+When an Android application uses a <code>BroadcastReceiver</code> to receive intents, 
+it is also able to receive explicit intents that are sent directly to it, regardless of its filter.
+
+Certain intent actions are only able to be sent by the operating system, not third-party applications. 
+However, a <code>BroadcastReceiver</code> that is registered to receive system intents is still able to receive 
+intents from a third-party application, so it should check that the intent received has the expected action.
+Otherwise, a third-party application could impersonate the system this way to cause unintended behavior, such as a denial of service.
+</p>
+</overview>
+
+<example>
+  <p>In the following code, the <code>ShutdownReceiver</code> initiates a shutdown procedure upon receiving an intent, 
+  without checking that the received action is indeed <code>ACTION_SHUTDOWN</code>. This allows third-party applications to 
+  send explicit intents to this receiver to cause a denial of service.</p>
+<sample src="Bad.java" />
+<sample src="AndroidManifest.xml" />
+</example>
+
+<recommendation>
+<p>
+In the <code>onReceive</code> method of a <code>BroadcastReciever</code>, the action of the received Intent should be checked. The following code demonstrates this.
+</p>
+<sample src="Good.java" />
+</recommendation>
+
+
+
+<references>
+
+</references>
+
+</qhelp>

java/ql/src/Security/CWE/CWE-925/ImproperIntentVerification.ql

@@ -0,0 +1,19 @@
+/**
+ * @name Improper verification of intent by broadcast receiver
+ * @description A broadcast receiver that does not verify intents it receives may be susceptible to unintended behavior by third party applications sending it explicit intents.
+ * @kind problem
+ * @problem.severity warning
+ * @security-severity 8.2
+ * @precision high
+ * @id java/improper-intent-verification
+ * @tags security
+ *       external/cwe/cwe-925
+ */
+
+import java
+import semmle.code.java.security.ImproperIntentVerificationQuery
+
+from AndroidReceiverXmlElement reg, Method orm, SystemActionName sa
+where unverifiedSystemReceiver(reg, orm, sa)
+select orm, "This reciever doesn't verify intents it receives, and is registered $@ to receive $@.",
+  reg, "here", sa, "the system action " + sa.getName()