From 3d9e8a2886ebf54126b415e940f62890a44c2517 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 2 Jul 2026 19:44:46 +0000 Subject: [PATCH] Combine best of PR #1643: handle += compound assignment and ignore dead-code returns --- .../CWE-295/AcceptAnyCertificate.ql | 23 ++++++++-- .../AcceptAnyCertificate.expected | 32 ++++++++----- .../CWE-295/AcceptAnyCertificate/Test.cs | 46 ++++++++++++++++++- 3 files changed, 84 insertions(+), 17 deletions(-) diff --git a/csharp/ql/src/Security Features/CWE-295/AcceptAnyCertificate.ql b/csharp/ql/src/Security Features/CWE-295/AcceptAnyCertificate.ql index 6c1df334b92..eec9b91f352 100644 --- a/csharp/ql/src/Security Features/CWE-295/AcceptAnyCertificate.ql +++ b/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 diff --git a/csharp/ql/test/query-tests/Security Features/CWE-295/AcceptAnyCertificate/AcceptAnyCertificate.expected b/csharp/ql/test/query-tests/Security Features/CWE-295/AcceptAnyCertificate/AcceptAnyCertificate.expected index 8e9becce109..001adcdcaa9 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-295/AcceptAnyCertificate/AcceptAnyCertificate.expected +++ b/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 | diff --git a/csharp/ql/test/query-tests/Security Features/CWE-295/AcceptAnyCertificate/Test.cs b/csharp/ql/test/query-tests/Security Features/CWE-295/AcceptAnyCertificate/Test.cs index 5509abc5a22..c02478caa2f 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-295/AcceptAnyCertificate/Test.cs +++ b/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) {