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..27258b56fd52 --- /dev/null +++ b/csharp/ql/src/experimental/CWE-295/InsecureCertificateValidation.ql @@ -0,0 +1,22 @@ +/** + * @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 path-problem + * @problem.severity error + * @precision high + * @id cs/unsafe-certificate-validation + * @tags security + * external/cwe/cwe-295 + * external/cwe/cwe-297 + */ + +import csharp +import DataFlow +import InsecureCertificateValidationQuery +import InsecureCertificateValidation::PathGraph + +from InsecureCertificateValidation::PathNode source, InsecureCertificateValidation::PathNode sink +where InsecureCertificateValidation::flowPath(source, sink) +select sink.getNode(), source, sink, + "$@ that is defined $@ and accepts any certificate as valid, is used here.", sink, + "This certificate callback", source, "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/InsecureCertificateValidationQuery.qll b/csharp/ql/src/experimental/CWE-295/InsecureCertificateValidationQuery.qll new file mode 100644 index 000000000000..08f27cfa3ec0 --- /dev/null +++ b/csharp/ql/src/experimental/CWE-295/InsecureCertificateValidationQuery.qll @@ -0,0 +1,152 @@ +/** + * Provides a taint-tracking configuration for reasoning + * about insecure certificate validation vulnerabilities. + */ + +private import csharp +private import DataFlow +private import semmle.code.csharp.controlflow.Guards + +/** + * A source specific to unsafe certificate validation vulnerabilities. + */ +abstract private class Source extends DataFlow::Node { } + +/** + * A sink specific to unsafe certificate validation vulnerabilities. + */ +abstract private class Sink extends DataFlow::Node { } + +/** + * A sanitizer specific to unsafe certificate validation vulnerabilities. + */ +abstract private class Sanitizer extends DataFlow::ExprNode { } + +/** + * 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 `ServerCertificateValidationCallback` property of the `HttpWebRequest` class. + */ +private class HttpWebRequestServerCertificateValidationCallback extends CertificateValidationProperty +{ + HttpWebRequestServerCertificateValidationCallback() { + this.getDeclaringType().hasFullyQualifiedName("System.Net", "HttpWebRequest") and + this.hasName("ServerCertificateValidationCallback") + } +} + +/** + * 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) } +} + +/** + * 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. + */ +private Callable getActualCallable(Expr e) { + exists(Expr dcArg | dcArg = e.(DelegateCreation).getArgument() | + result = dcArg.(CallableAccess).getTarget() or + result = dcArg.(AnonymousFunctionExpr) + ) + or + result = e +} + +private predicate ignoreCertificateValidation(Guard guard, AbstractValue v) { + guard = + any(PropertyAccess access | + access.getProperty().hasFullyQualifiedName("", "Settings", "IgnoreCertificateValidation") and + v.(AbstractValues::BooleanValue).getValue() = true + ) +} + +private class AddIgnoreCheck extends Sanitizer { + AddIgnoreCheck() { + exists(Guard g, AbstractValues::BooleanValue v | ignoreCertificateValidation(g, v) | + g.controlsNode(this.getControlFlowNode(), v) + ) + } +} + +private class CallableAlwaysReturnsTrue extends Callable { + CallableAlwaysReturnsTrue() { alwaysReturnsTrue(this) } +} + +private class AddCallableAlwaysReturnsTrue extends Source { + AddCallableAlwaysReturnsTrue() { + getActualCallable(this.asExpr()) instanceof CallableAlwaysReturnsTrue + } +} + +private class AddSinks extends Sink { + AddSinks() { + exists(Expr e | e = this.asExpr() | + e = + [ + any(CertificateValidationProperty p).getAnAssignedValue(), + any(SslStreamCreation yy).getServerCertificateValidationCallback() + ] and + not e.getFile().getAbsolutePath().matches("example") + ) + } +} + +private module InsecureCertificateValidationConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { node instanceof Source } + + predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer } + + predicate isSink(DataFlow::Node node) { node instanceof Sink } +} + +/** + * A taint-tracking module for insecure certificate validation vulnerabilities. + */ +module InsecureCertificateValidation = TaintTracking::Global; 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..d7739d3a69c4 --- /dev/null +++ b/csharp/ql/test/experimental/CWE-295/InsecureCertificateValidation.expected @@ -0,0 +1,28 @@ +edges +| Program.cs:80:58:80:65 | callback : ValidateServerCertificate | Program.cs:82:67:82:74 | access to parameter callback | provenance | | +| Program.cs:87:45:87:52 | access to local variable callback : ValidateServerCertificate | Program.cs:89:25:89:32 | access to local variable callback : ValidateServerCertificate | provenance | | +| Program.cs:87:56:87:80 | delegate creation of type RemoteCertificateValidationCallback : ValidateServerCertificate | Program.cs:87:45:87:52 | access to local variable callback : ValidateServerCertificate | provenance | | +| Program.cs:89:25:89:32 | access to local variable callback : ValidateServerCertificate | Program.cs:80:58:80:65 | callback : ValidateServerCertificate | provenance | | +nodes +| Program.cs:18:13:18:78 | delegate creation of type RemoteCertificateValidationCallback | semmle.label | delegate creation of type RemoteCertificateValidationCallback | +| Program.cs:59:61:59:106 | (...) => ... | semmle.label | (...) => ... | +| Program.cs:66:67:66:132 | delegate creation of type RemoteCertificateValidationCallback | semmle.label | delegate creation of type RemoteCertificateValidationCallback | +| Program.cs:67:67:67:112 | (...) => ... | semmle.label | (...) => ... | +| Program.cs:68:67:68:91 | delegate creation of type RemoteCertificateValidationCallback | semmle.label | delegate creation of type RemoteCertificateValidationCallback | +| Program.cs:75:55:75:79 | delegate creation of type RemoteCertificateValidationCallback | semmle.label | delegate creation of type RemoteCertificateValidationCallback | +| Program.cs:80:58:80:65 | callback : ValidateServerCertificate | semmle.label | callback : ValidateServerCertificate | +| Program.cs:82:67:82:74 | access to parameter callback | semmle.label | access to parameter callback | +| Program.cs:87:45:87:52 | access to local variable callback : ValidateServerCertificate | semmle.label | access to local variable callback : ValidateServerCertificate | +| Program.cs:87:56:87:80 | delegate creation of type RemoteCertificateValidationCallback : ValidateServerCertificate | semmle.label | delegate creation of type RemoteCertificateValidationCallback : ValidateServerCertificate | +| Program.cs:89:25:89:32 | access to local variable callback : ValidateServerCertificate | semmle.label | access to local variable callback : ValidateServerCertificate | +| Program.cs:114:71:114:95 | delegate creation of type RemoteCertificateValidationCallback | semmle.label | delegate creation of type RemoteCertificateValidationCallback | +subpaths +#select +| Program.cs:18:13:18:78 | delegate creation of type RemoteCertificateValidationCallback | Program.cs:18:13:18:78 | delegate creation of type RemoteCertificateValidationCallback | Program.cs:18:13:18:78 | delegate creation of type RemoteCertificateValidationCallback | $@ that is defined $@ and accepts any certificate as valid, is used here. | Program.cs:18:13:18:78 | delegate creation of type RemoteCertificateValidationCallback | This certificate callback | Program.cs:18:13:18:78 | delegate creation of type RemoteCertificateValidationCallback | here | +| Program.cs:59:61:59:106 | (...) => ... | Program.cs:59:61:59:106 | (...) => ... | Program.cs:59:61:59:106 | (...) => ... | $@ that is defined $@ and accepts any certificate as valid, is used here. | Program.cs:59:61:59:106 | (...) => ... | This certificate callback | Program.cs:59:61:59:106 | (...) => ... | here | +| Program.cs:66:67:66:132 | delegate creation of type RemoteCertificateValidationCallback | Program.cs:66:67:66:132 | delegate creation of type RemoteCertificateValidationCallback | Program.cs:66:67:66:132 | delegate creation of type RemoteCertificateValidationCallback | $@ that is defined $@ and accepts any certificate as valid, is used here. | Program.cs:66:67:66:132 | delegate creation of type RemoteCertificateValidationCallback | This certificate callback | Program.cs:66:67:66:132 | delegate creation of type RemoteCertificateValidationCallback | here | +| Program.cs:67:67:67:112 | (...) => ... | Program.cs:67:67:67:112 | (...) => ... | Program.cs:67:67:67:112 | (...) => ... | $@ that is defined $@ and accepts any certificate as valid, is used here. | Program.cs:67:67:67:112 | (...) => ... | This certificate callback | Program.cs:67:67:67:112 | (...) => ... | here | +| Program.cs:68:67:68:91 | delegate creation of type RemoteCertificateValidationCallback | Program.cs:68:67:68:91 | delegate creation of type RemoteCertificateValidationCallback | Program.cs:68:67:68:91 | delegate creation of type RemoteCertificateValidationCallback | $@ that is defined $@ and accepts any certificate as valid, is used here. | Program.cs:68:67:68:91 | delegate creation of type RemoteCertificateValidationCallback | This certificate callback | Program.cs:68:67:68:91 | delegate creation of type RemoteCertificateValidationCallback | here | +| Program.cs:75:55:75:79 | delegate creation of type RemoteCertificateValidationCallback | Program.cs:75:55:75:79 | delegate creation of type RemoteCertificateValidationCallback | 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:75:55:75:79 | delegate creation of type RemoteCertificateValidationCallback | here | +| Program.cs:82:67:82:74 | access to parameter callback | Program.cs:87:56:87:80 | delegate creation of type RemoteCertificateValidationCallback : ValidateServerCertificate | Program.cs:82:67:82:74 | access to parameter callback | $@ that is defined $@ and accepts any certificate as valid, is used here. | Program.cs:82:67:82:74 | access to parameter callback | This certificate callback | Program.cs:87:56:87:80 | delegate creation of type RemoteCertificateValidationCallback : ValidateServerCertificate | here | +| Program.cs:114:71:114:95 | delegate creation of type RemoteCertificateValidationCallback | Program.cs:114:71:114:95 | delegate creation of type RemoteCertificateValidationCallback | Program.cs:114:71:114:95 | delegate creation of type RemoteCertificateValidationCallback | $@ that is defined $@ and accepts any certificate as valid, is used here. | Program.cs:114:71:114:95 | delegate creation of type RemoteCertificateValidationCallback | This certificate callback | Program.cs:114:71:114:95 | delegate creation of type RemoteCertificateValidationCallback | 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..b2a050128323 --- /dev/null +++ b/csharp/ql/test/experimental/CWE-295/Program.cs @@ -0,0 +1,122 @@ +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 +{ + + void M1() + { + 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; + } + + void M2() + { + 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); + } + + void M3() + { + 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 + } + + void M4() + { + HttpWebRequest request = (HttpWebRequest)WebRequest.Create("https://www.example.com"); + request.ServerCertificateValidationCallback = ValidateServerCertificate; // BAD: unsafe callback used + request.ServerCertificateValidationCallback = SafeValidateServerCertificate; // GOOD: safe callback used + + } + + void SetCallback(RemoteCertificateValidationCallback callback) + { + ServicePointManager.ServerCertificateValidationCallback = callback; // BAD: unsafe callback used + } + + void M5(bool b) + { + RemoteCertificateValidationCallback callback = ValidateServerCertificate; + if (b) { + SetCallback(callback); // BAD: unsafe callback used + } + } + + void M6(Settings settings) + { + RemoteCertificateValidationCallback callback = ValidateServerCertificate; + if (settings.IgnoreCertificateValidation) + { + SetCallback(callback); // GOOD: We don't do validation. + } + } + + void M7(Settings settings) + { + if (settings.IgnoreCertificateValidation) + { + ServicePointManager.ServerCertificateValidationCallback = ValidateServerCertificate; // GOOD: We don't do validation. + } + } + + void M8(Settings settings) + { + if (!settings.IgnoreCertificateValidation) + { + ServicePointManager.ServerCertificateValidationCallback = ValidateServerCertificate; // BAD: unsafe callback used + } + } +} + +public class Settings { + + public bool IgnoreCertificateValidation { get; set; } +} diff --git a/csharp/ql/test/experimental/CWE-295/options b/csharp/ql/test/experimental/CWE-295/options new file mode 100644 index 000000000000..77b22963f5c8 --- /dev/null +++ b/csharp/ql/test/experimental/CWE-295/options @@ -0,0 +1,2 @@ +semmle-extractor-options: /nostdlib /noconfig +semmle-extractor-options: --load-sources-from-project:${testdir}/../../resources/stubs/_frameworks/Microsoft.NETCore.App/Microsoft.NETCore.App.csproj