Combine best of PR #1643: handle += compound assignment and ignore dead-code returns

csharp/ql/src/Security Features/CWE-295/AcceptAnyCertificate.ql

@@ -21,9 +21,9 @@ import AcceptAnyCertificate::PathGraph
  */
 predicate alwaysReturnsTrue(Callable c) {
   c.getReturnType() instanceof BoolType and
-  // There is at least one returned value, and every returned value is the
-  // constant `true`.
-  forex(Expr ret | c.canReturn(ret) | ret.getValue() = "true")
+  // There is at least one live returned value, and every live returned value is
+  // the constant `true`. Dead (unreachable) returns are ignored.
+  forex(Expr ret | c.canReturn(ret) and ret.isLive() | ret.getValue() = "true")
 }
 
 /**
@@ -59,6 +59,21 @@ Callable getAcceptingCallable(Expr e) {
   alwaysReturnsTrue(result)
 }
 
+/**
+ * Gets the expression that produces the delegate value assigned to `a`,
+ * handling both simple assignments (`a = ...`) and compound assignments such as
+ * `a += ...` (used to combine delegates).
+ */
+Expr getAssignedDelegate(Assignable a) {
+  exists(Expr source | source = a.getAnAssignedValue() |
+    // `a += ...` combines delegates; the delegate value is the right operand.
+    result = source.(AssignOperation).getRightOperand()
+    or
+    // `a = ...` assigns the delegate value directly.
+    result = source and not source instanceof AssignOperation
+  )
+}
+
 module AcceptAnyCertificateConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
     exists(getAcceptingCallable(source.asExpr()))
@@ -77,7 +92,7 @@ module AcceptAnyCertificateConfig implements DataFlow::ConfigSig {
     // validation callback type.
     exists(Assignable a |
       a.getType() instanceof CertificateValidationCallbackType and
-      sink.asExpr() = a.getAnAssignedValue()
+      sink.asExpr() = getAssignedDelegate(a)
     )
     or
     // The value passed as a certificate validation callback argument, e.g. to

csharp/ql/test/query-tests/Security Features/CWE-295/AcceptAnyCertificate/AcceptAnyCertificate.expected

@@ -1,24 +1,32 @@
 edges
-| Test.cs:64:45:64:52 | access to local variable callback : (...) => ... | Test.cs:67:48:67:55 | access to local variable callback | provenance |  |
-| Test.cs:65:13:65:56 | (...) => ... : (...) => ... | Test.cs:64:45:64:52 | access to local variable callback : (...) => ... | provenance |  |
+| Test.cs:88:45:88:52 | access to local variable callback : (...) => ... | Test.cs:91:48:91:55 | access to local variable callback | provenance |  |
+| Test.cs:89:13:89:56 | (...) => ... : (...) => ... | Test.cs:88:45:88:52 | access to local variable callback : (...) => ... | provenance |  |
 nodes
 | Test.cs:14:13:14:57 | (...) => ... | semmle.label | (...) => ... |
 | Test.cs:22:13:25:13 | (...) => ... | semmle.label | (...) => ... |
 | Test.cs:33:13:33:74 | access to property DangerousAcceptAnyServerCertificateValidator | semmle.label | access to property DangerousAcceptAnyServerCertificateValidator |
 | Test.cs:40:13:40:56 | (...) => ... | semmle.label | (...) => ... |
-| Test.cs:52:67:52:75 | delegate creation of type RemoteCertificateValidationCallback | semmle.label | delegate creation of type RemoteCertificateValidationCallback |
-| Test.cs:59:13:59:56 | (...) => ... | semmle.label | (...) => ... |
-| Test.cs:64:45:64:52 | access to local variable callback : (...) => ... | semmle.label | access to local variable callback : (...) => ... |
-| Test.cs:65:13:65:56 | (...) => ... | semmle.label | (...) => ... |
-| Test.cs:65:13:65:56 | (...) => ... : (...) => ... | semmle.label | (...) => ... : (...) => ... |
-| Test.cs:67:48:67:55 | access to local variable callback | semmle.label | access to local variable callback |
+| Test.cs:47:13:47:61 | (...) => ... | semmle.label | (...) => ... |
+| Test.cs:49:68:49:87 | (...) => ... | semmle.label | (...) => ... |
+| Test.cs:51:68:51:92 | delegate(...) { ... } | semmle.label | delegate(...) { ... } |
+| Test.cs:69:67:69:75 | delegate creation of type RemoteCertificateValidationCallback | semmle.label | delegate creation of type RemoteCertificateValidationCallback |
+| Test.cs:76:13:76:76 | delegate creation of type RemoteCertificateValidationCallback | semmle.label | delegate creation of type RemoteCertificateValidationCallback |
+| Test.cs:83:13:83:56 | (...) => ... | semmle.label | (...) => ... |
+| Test.cs:88:45:88:52 | access to local variable callback : (...) => ... | semmle.label | access to local variable callback : (...) => ... |
+| Test.cs:89:13:89:56 | (...) => ... | semmle.label | (...) => ... |
+| Test.cs:89:13:89:56 | (...) => ... : (...) => ... | semmle.label | (...) => ... : (...) => ... |
+| Test.cs:91:48:91:55 | access to local variable callback | semmle.label | access to local variable callback |
 subpaths
 #select
 | Test.cs:14:13:14:57 | (...) => ... | Test.cs:14:13:14:57 | (...) => ... | Test.cs:14:13:14:57 | (...) => ... | This TLS certificate validation $@, which trusts any certificate. | Test.cs:14:13:14:57 | (...) => ... | uses a callback |
 | Test.cs:22:13:25:13 | (...) => ... | Test.cs:22:13:25:13 | (...) => ... | Test.cs:22:13:25:13 | (...) => ... | This TLS certificate validation $@, which trusts any certificate. | Test.cs:22:13:25:13 | (...) => ... | uses a callback |
 | Test.cs:33:13:33:74 | access to property DangerousAcceptAnyServerCertificateValidator | Test.cs:33:13:33:74 | access to property DangerousAcceptAnyServerCertificateValidator | Test.cs:33:13:33:74 | access to property DangerousAcceptAnyServerCertificateValidator | This TLS certificate validation $@, which trusts any certificate. | Test.cs:33:13:33:74 | access to property DangerousAcceptAnyServerCertificateValidator | uses a callback |
 | Test.cs:40:13:40:56 | (...) => ... | Test.cs:40:13:40:56 | (...) => ... | Test.cs:40:13:40:56 | (...) => ... | This TLS certificate validation $@, which trusts any certificate. | Test.cs:40:13:40:56 | (...) => ... | uses a callback |
-| Test.cs:52:67:52:75 | delegate creation of type RemoteCertificateValidationCallback | Test.cs:52:67:52:75 | delegate creation of type RemoteCertificateValidationCallback | Test.cs:52:67:52:75 | delegate creation of type RemoteCertificateValidationCallback | This TLS certificate validation $@, which trusts any certificate. | Test.cs:52:67:52:75 | delegate creation of type RemoteCertificateValidationCallback | uses a callback |
-| Test.cs:59:13:59:56 | (...) => ... | Test.cs:59:13:59:56 | (...) => ... | Test.cs:59:13:59:56 | (...) => ... | This TLS certificate validation $@, which trusts any certificate. | Test.cs:59:13:59:56 | (...) => ... | uses a callback |
-| Test.cs:65:13:65:56 | (...) => ... | Test.cs:65:13:65:56 | (...) => ... | Test.cs:65:13:65:56 | (...) => ... | This TLS certificate validation $@, which trusts any certificate. | Test.cs:65:13:65:56 | (...) => ... | uses a callback |
-| Test.cs:67:48:67:55 | access to local variable callback | Test.cs:65:13:65:56 | (...) => ... : (...) => ... | Test.cs:67:48:67:55 | access to local variable callback | This TLS certificate validation $@, which trusts any certificate. | Test.cs:65:13:65:56 | (...) => ... | uses a callback |
+| Test.cs:47:13:47:61 | (...) => ... | Test.cs:47:13:47:61 | (...) => ... | Test.cs:47:13:47:61 | (...) => ... | This TLS certificate validation $@, which trusts any certificate. | Test.cs:47:13:47:61 | (...) => ... | uses a callback |
+| Test.cs:49:68:49:87 | (...) => ... | Test.cs:49:68:49:87 | (...) => ... | Test.cs:49:68:49:87 | (...) => ... | This TLS certificate validation $@, which trusts any certificate. | Test.cs:49:68:49:87 | (...) => ... | uses a callback |
+| Test.cs:51:68:51:92 | delegate(...) { ... } | Test.cs:51:68:51:92 | delegate(...) { ... } | Test.cs:51:68:51:92 | delegate(...) { ... } | This TLS certificate validation $@, which trusts any certificate. | Test.cs:51:68:51:92 | delegate(...) { ... } | uses a callback |
+| Test.cs:69:67:69:75 | delegate creation of type RemoteCertificateValidationCallback | Test.cs:69:67:69:75 | delegate creation of type RemoteCertificateValidationCallback | Test.cs:69:67:69:75 | delegate creation of type RemoteCertificateValidationCallback | This TLS certificate validation $@, which trusts any certificate. | Test.cs:69:67:69:75 | delegate creation of type RemoteCertificateValidationCallback | uses a callback |
+| Test.cs:76:13:76:76 | delegate creation of type RemoteCertificateValidationCallback | Test.cs:76:13:76:76 | delegate creation of type RemoteCertificateValidationCallback | Test.cs:76:13:76:76 | delegate creation of type RemoteCertificateValidationCallback | This TLS certificate validation $@, which trusts any certificate. | Test.cs:76:13:76:76 | delegate creation of type RemoteCertificateValidationCallback | uses a callback |
+| Test.cs:83:13:83:56 | (...) => ... | Test.cs:83:13:83:56 | (...) => ... | Test.cs:83:13:83:56 | (...) => ... | This TLS certificate validation $@, which trusts any certificate. | Test.cs:83:13:83:56 | (...) => ... | uses a callback |
+| Test.cs:89:13:89:56 | (...) => ... | Test.cs:89:13:89:56 | (...) => ... | Test.cs:89:13:89:56 | (...) => ... | This TLS certificate validation $@, which trusts any certificate. | Test.cs:89:13:89:56 | (...) => ... | uses a callback |
+| Test.cs:91:48:91:55 | access to local variable callback | Test.cs:89:13:89:56 | (...) => ... : (...) => ... | Test.cs:91:48:91:55 | access to local variable callback | This TLS certificate validation $@, which trusts any certificate. | Test.cs:89:13:89:56 | (...) => ... | uses a callback |

csharp/ql/test/query-tests/Security Features/CWE-295/AcceptAnyCertificate/Test.cs

@@ -40,18 +40,42 @@ public class CertificateValidationTests
             (sender, certificate, chain, errors) => true;
     }
 
+    public void ServicePointManagerCompoundBad()
+    {
+        // BAD: always trusts any certificate (compound assignment).
+        ServicePointManager.ServerCertificateValidationCallback +=
+            (sender, cert, chain, errors) => { return true; };
+        // BAD
+        ServicePointManager.ServerCertificateValidationCallback += (a, b, c, d) => true;
+        // BAD: parameterless anonymous method.
+        ServicePointManager.ServerCertificateValidationCallback += delegate { return true; };
+    }
+
     private static bool AcceptAll(object sender, X509Certificate certificate, X509Chain chain,
         SslPolicyErrors errors)
     {
         return true;
     }
 
+    public bool AcceptAllNonStatic(object sender, X509Certificate certificate, X509Chain chain,
+        SslPolicyErrors errors)
+    {
+        return true;
+    }
+
     public void MethodGroupBad()
     {
-        // BAD: the referenced method always returns true.
+        // BAD: the referenced static method always returns true.
         ServicePointManager.ServerCertificateValidationCallback = AcceptAll;
     }
 
+    public void MethodGroupNonStaticBad()
+    {
+        // BAD: the referenced instance method always returns true.
+        ServicePointManager.ServerCertificateValidationCallback =
+            new RemoteCertificateValidationCallback(this.AcceptAllNonStatic);
+    }
+
     public void SslStreamBad(Stream stream)
     {
         // BAD: the validation callback always returns true.
@@ -75,6 +99,26 @@ public class CertificateValidationTests
             (request, certificate, chain, errors) => errors == SslPolicyErrors.None;
     }
 
+    public void ControlFlowGood()
+    {
+        // GOOD: not every returned value is `true`.
+        ServicePointManager.ServerCertificateValidationCallback +=
+            (sender, cert, chain, errors) =>
+            {
+                if (cert == null)
+                {
+                    return false;
+                }
+
+                if (errors != SslPolicyErrors.None)
+                {
+                    return false;
+                }
+
+                return true;
+            };
+    }
+
     private static bool Validate(object sender, X509Certificate certificate, X509Chain chain,
         SslPolicyErrors errors)
     {