From 9967c7cda79cf4b3d6d253fb63e7da669e020773 Mon Sep 17 00:00:00 2001 From: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> Date: Tue, 25 Jun 2024 01:10:41 +0000 Subject: [PATCH] C#: Add query for insecure certificate validation --- .../InsecureCertificateValidation.qhelp | 29 +++++ .../CWE-295/InsecureCertificateValidation.ql | 100 ++++++++++++++++++ .../InsecureCertificateValidationExample.cs | 2 + .../SecureCertificateValidationExample.cs | 6 ++ .../InsecureCertificateValidation.expected | 6 ++ .../InsecureCertificateValidation.qlref | 1 + .../ql/test/experimental/CWE-295/Program.cs | 80 ++++++++++++++ 7 files changed, 224 insertions(+) create mode 100644 csharp/ql/src/experimental/CWE-295/InsecureCertificateValidation.qhelp create mode 100644 csharp/ql/src/experimental/CWE-295/InsecureCertificateValidation.ql create mode 100644 csharp/ql/src/experimental/CWE-295/InsecureCertificateValidationExample.cs create mode 100644 csharp/ql/src/experimental/CWE-295/SecureCertificateValidationExample.cs create mode 100644 csharp/ql/test/experimental/CWE-295/InsecureCertificateValidation.expected create mode 100644 csharp/ql/test/experimental/CWE-295/InsecureCertificateValidation.qlref create mode 100644 csharp/ql/test/experimental/CWE-295/Program.cs diff --git a/csharp/ql/src/experimental/CWE-295/InsecureCertificateValidation.qhelp b/csharp/ql/src/experimental/CWE-295/InsecureCertificateValidation.qhelp new file mode 100644 index 000000000000..05fcdc6c80df --- /dev/null +++ b/csharp/ql/src/experimental/CWE-295/InsecureCertificateValidation.qhelp @@ -0,0 +1,29 @@ + + + +

+ Using a RemoteCertificateValidationCallback that always returns true is insecure because it allows any certificate to be accepted as valid. This can lead to a variety of security issues, including man-in-the-middle attacks. +

+ + +

+ Ensure that RemoteCertificateValidationCallback implementations properly verify the certificate before returning true. Avoid implementing callbacks that unconditionally accept all certificates. +

+ + +

+ The following example demonstrates an insecure use of RemoteCertificateValidationCallback that always returns true: +

+ +

+ A secure approach would involve proper verification of the certificate before returning true: +

+ + + +
  • + CA5359: Do not disable certificate validation + CA5359 +
    • +     +     \ No newline at end of file diff --git a/csharp/ql/src/experimental/CWE-295/InsecureCertificateValidation.ql b/csharp/ql/src/experimental/CWE-295/InsecureCertificateValidation.ql new file mode 100644 index 000000000000..76b0ce86fb59 --- /dev/null +++ b/csharp/ql/src/experimental/CWE-295/InsecureCertificateValidation.ql @@ -0,0 +1,100 @@ +/** + * @name Unsafe `CertificateValidationCallback` use. + * @description Using a `CertificateValidationCallback` that always returns `true` is insecure, as it allows any certificate to be accepted as valid. + * @kind alert + * @problem.severity error + * @precision high + * @id cs/unsafe-certificate-validation + * @tags security + * external/cwe/cwe-295 + * external/cwe/cwe-297 + */ + +import csharp + +/** + * Provides an abstract base class for properties related to certificate validation. + */ +abstract private class CertificateValidationProperty extends Property { } + +/** + * Represents the `ServerCertificateValidationCallback` property of the `ServicePointManager` class. + */ +private class ServicePointManagerServerCertificateValidationCallback extends CertificateValidationProperty +{ + ServicePointManagerServerCertificateValidationCallback() { + this.getDeclaringType().hasFullyQualifiedName("System.Net", "ServicePointManager") and + this.hasName("ServerCertificateValidationCallback") + } +} + +/** + * Represents the `ServerCertificateCustomValidationCallback` property of the `HttpClientHandler` class. + */ +private class HttpClientHandlerServerCertificateCustomValidationCallback extends CertificateValidationProperty +{ + HttpClientHandlerServerCertificateCustomValidationCallback() { + this.getDeclaringType().hasFullyQualifiedName("System.Net.Http", "HttpClientHandler") and + this.hasName("ServerCertificateCustomValidationCallback") + } +} + +/** + * Represents the creation of an `SslStream` object. + */ +private class SslStreamCreation extends ObjectCreation { + SslStreamCreation() { + this.getObjectType().hasFullyQualifiedName("System.Net.Security", "SslStream") + } + + /** + * Gets the expression used as the server certificate validation callback argument + * in the creation of the `SslStream` object. + */ + Expr getServerCertificateValidationCallback() { result = this.getArgument(2) } +} + +/** + * Represents the `ServerCertificateValidationCallback` property of the `HttpWebRequest` class. + */ +private class HttpWebRequestServerCertificateValidationCallback extends CertificateValidationProperty +{ + HttpWebRequestServerCertificateValidationCallback() { + this.getDeclaringType().hasFullyQualifiedName("System.Net", "HttpWebRequest") and + this.hasName("ServerCertificateValidationCallback") + } +} + +/** + * Holds if `c` always returns `true`. + */ +private predicate alwaysReturnsTrue(Callable c) { + forex(ReturnStmt rs | rs.getEnclosingCallable() = c | + rs.getExpr().(BoolLiteral).getBoolValue() = true + ) + or + c.getBody().(BoolLiteral).getBoolValue() = true +} + +/** + * Gets the actual callable object referred to by expression `e`. + * This can be a direct reference to a callable, a delegate creation, or an anonymous function. + */ +Callable getActualCallable(Expr e) { + exists(Expr dcArg | dcArg = e.(DelegateCreation).getArgument() | + result = dcArg.(CallableAccess).getTarget() or result = dcArg.(AnonymousFunctionExpr) + ) + or + result = e.(Callable) +} + +from Expr e, Callable c +where + [ + any(SslStreamCreation yy).getServerCertificateValidationCallback(), + any(CertificateValidationProperty xx).getAnAssignedValue() + ] = e and + getActualCallable(e) = c and + alwaysReturnsTrue(c) +select e, "$@ that is defined $@ and accepts any certificate as valid, is used here.", e, + "This certificate callback", c, "here" diff --git a/csharp/ql/src/experimental/CWE-295/InsecureCertificateValidationExample.cs b/csharp/ql/src/experimental/CWE-295/InsecureCertificateValidationExample.cs new file mode 100644 index 000000000000..76cddcce2915 --- /dev/null +++ b/csharp/ql/src/experimental/CWE-295/InsecureCertificateValidationExample.cs @@ -0,0 +1,2 @@ +ServicePointManager.ServerCertificateValidationCallback = + (sender, cert, chain, sslPolicyErrors) => true; \ No newline at end of file diff --git a/csharp/ql/src/experimental/CWE-295/SecureCertificateValidationExample.cs b/csharp/ql/src/experimental/CWE-295/SecureCertificateValidationExample.cs new file mode 100644 index 000000000000..cc4c82b8412b --- /dev/null +++ b/csharp/ql/src/experimental/CWE-295/SecureCertificateValidationExample.cs @@ -0,0 +1,6 @@ +ServicePointManager.ServerCertificateValidationCallback += + (sender, cert, chain, sslPolicyErrors) => { + if (cert.Issuer == "TrustedIssuer" /* && other conditions */) + return true; + return false; + }; \ No newline at end of file diff --git a/csharp/ql/test/experimental/CWE-295/InsecureCertificateValidation.expected b/csharp/ql/test/experimental/CWE-295/InsecureCertificateValidation.expected new file mode 100644 index 000000000000..55746006b7e8 --- /dev/null +++ b/csharp/ql/test/experimental/CWE-295/InsecureCertificateValidation.expected @@ -0,0 +1,6 @@ +| Program.cs:19:13:19:78 | delegate creation of type RemoteCertificateValidationCallback | $@ that is defined $@ and accepts any certificate as valid, is used here. | Program.cs:19:13:19:78 | delegate creation of type RemoteCertificateValidationCallback | This certificate callback | Program.cs:39:24:39:48 | ValidateServerCertificate | here | +| Program.cs:60:61:60:106 | (...) => ... | $@ that is defined $@ and accepts any certificate as valid, is used here. | Program.cs:60:61:60:106 | (...) => ... | This certificate callback | Program.cs:60:61:60:106 | (...) => ... | here | +| Program.cs:67:67:67:132 | delegate creation of type RemoteCertificateValidationCallback | $@ that is defined $@ and accepts any certificate as valid, is used here. | Program.cs:67:67:67:132 | delegate creation of type RemoteCertificateValidationCallback | This certificate callback | Program.cs:39:24:39:48 | ValidateServerCertificate | here | +| Program.cs:68:67:68:112 | (...) => ... | $@ that is defined $@ and accepts any certificate as valid, is used here. | Program.cs:68:67:68:112 | (...) => ... | This certificate callback | Program.cs:68:67:68:112 | (...) => ... | here | +| Program.cs:69:67:69:91 | delegate creation of type RemoteCertificateValidationCallback | $@ that is defined $@ and accepts any certificate as valid, is used here. | Program.cs:69:67:69:91 | delegate creation of type RemoteCertificateValidationCallback | This certificate callback | Program.cs:39:24:39:48 | ValidateServerCertificate | here | +| Program.cs:75:55:75:79 | delegate creation of type RemoteCertificateValidationCallback | $@ that is defined $@ and accepts any certificate as valid, is used here. | Program.cs:75:55:75:79 | delegate creation of type RemoteCertificateValidationCallback | This certificate callback | Program.cs:39:24:39:48 | ValidateServerCertificate | here | diff --git a/csharp/ql/test/experimental/CWE-295/InsecureCertificateValidation.qlref b/csharp/ql/test/experimental/CWE-295/InsecureCertificateValidation.qlref new file mode 100644 index 000000000000..60638a0a6f9c --- /dev/null +++ b/csharp/ql/test/experimental/CWE-295/InsecureCertificateValidation.qlref @@ -0,0 +1 @@ +experimental/CWE-295/InsecureCertificateValidation.ql \ No newline at end of file diff --git a/csharp/ql/test/experimental/CWE-295/Program.cs b/csharp/ql/test/experimental/CWE-295/Program.cs new file mode 100644 index 000000000000..bd76d4f2879f --- /dev/null +++ b/csharp/ql/test/experimental/CWE-295/Program.cs @@ -0,0 +1,80 @@ +// semmle-extractor-options: /r:System.Net.Sockets.dll /r:System.Net.Security.dll /r:System.Security.Cryptography.Algorithms.dll /r:System.Net.Http.dll /r:System.Net.ServicePoint.dll /r:System.Security.Cryptography.dll /r:System.Net.Primitives.dll /r:System.Net.Requests.dll /r:System.Private.Uri.dll +using System; +using System.Net; +using System.Net.Security; +using System.Net.Sockets; +using System.Security.Cryptography.X509Certificates; +using System.IO; +using System.Net.Http; + +class Program +{ + + static void First() + { + TcpClient client = new TcpClient("www.example.com", 443); + SslStream sslStream = new SslStream( + client.GetStream(), + false, + new RemoteCertificateValidationCallback(ValidateServerCertificate), // BAD: unsafe callback used + null + ); + sslStream = new SslStream( + client.GetStream(), + false, + new RemoteCertificateValidationCallback(SafeValidateServerCertificate), // GOOD: safe callback used + null + ); + + try + { + sslStream.AuthenticateAsClient("www.example.com"); + } + catch (Exception e) + { + Console.WriteLine(e.Message); + } + } + + public static bool ValidateServerCertificate( + object sender, + X509Certificate certificate, + X509Chain chain, + SslPolicyErrors sslPolicyErrors) + { + return true; + } + + public static bool SafeValidateServerCertificate( + object sender, + X509Certificate certificate, + X509Chain chain, + SslPolicyErrors sslPolicyErrors) + { + return sslPolicyErrors == SslPolicyErrors.None; + } + + static void Second() + { + HttpClientHandler handler = new HttpClientHandler(); + handler.ServerCertificateCustomValidationCallback = (sender, cert, chain, sslPolicyErrors) => true; // BAD: unsafe callback used + handler.ServerCertificateCustomValidationCallback = SafeValidateServerCertificate; // GOOD: safe callback used + HttpClient client = new HttpClient(handler); + } + + static void Third() + { + ServicePointManager.ServerCertificateValidationCallback = new RemoteCertificateValidationCallback(ValidateServerCertificate); // BAD: unsafe callback used + ServicePointManager.ServerCertificateValidationCallback = (sender, cert, chain, sslPolicyErrors) => true; // BAD: unsafe callback used + ServicePointManager.ServerCertificateValidationCallback = ValidateServerCertificate; // BAD: unsafe callback used + ServicePointManager.ServerCertificateValidationCallback = SafeValidateServerCertificate; // GOOD: safe callback used + } + static void Fourth() + { + HttpWebRequest request = (HttpWebRequest)WebRequest.Create("https://www.example.com"); + request.ServerCertificateValidationCallback = ValidateServerCertificate; // BAD: unsafe callback used + request.ServerCertificateValidationCallback = SafeValidateServerCertificate; // GOOD: safe callback used + + } + +}