Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions csharp/ql/src/Security Features/CWE-295/AcceptAnyCertificate.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
using System.Net.Http;
using System.Net.Security;
using System.Security.Cryptography.X509Certificates;

public class CertificateValidation
{
public void Bad()
{
var handler = new HttpClientHandler();
// BAD: the callback always returns true, so every certificate is trusted.
handler.ServerCertificateCustomValidationCallback =
(request, certificate, chain, errors) => true;
}

public void Good()
{
var handler = new HttpClientHandler();
// GOOD: the certificate is only trusted when there are no validation errors.
handler.ServerCertificateCustomValidationCallback =
(request, certificate, chain, errors) => errors == SslPolicyErrors.None;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>
A TLS/SSL certificate validation callback that always returns <code>true</code> trusts every certificate,
regardless of any validation errors that were detected. This allows an attacker to perform a machine-in-the-middle
attack against the application, therefore breaking any security that Transport Layer Security (TLS) provides.
</p>

<p>
An attack might look like this:
</p>

<ol>
<li>The vulnerable program 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 program calls the certificate validation callback to check whether it should trust the certificate.</li>
<li>The callback ignores the <code>SslPolicyErrors</code> argument and returns <code>true</code>.</li>
<li>The vulnerable program accepts the certificate and proceeds with the connection, since the callback indicated that the certificate is trusted.</li>
<li>The attacker can now read the data the program sends to <code>https://example.com</code> and/or alter its replies while the program thinks the connection is secure.</li>
</ol>
</overview>

<recommendation>
<p>
Do not use a certificate validation callback that unconditionally returns <code>true</code>.
Either rely on the default certificate validation, or implement a callback that inspects the
<code>SslPolicyErrors</code> argument and only trusts a specific, known certificate (for example, when
using a self-signed certificate that has been explicitly pinned).
</p>
</recommendation>

<example>
<p>
In the first (bad) example, the callback always returns <code>true</code> and therefore trusts any certificate,
which allows an attacker to perform a machine-in-the-middle attack. In the second (good) example, the callback
returns <code>true</code> only when there are no validation errors.
</p>
<sample src="AcceptAnyCertificate.cs" />
</example>

<references>
<li>Microsoft Learn:
<a href="https://learn.microsoft.com/en-us/dotnet/api/system.net.security.remotecertificatevalidationcallback">RemoteCertificateValidationCallback Delegate</a>.</li>
<li>Microsoft Learn:
<a href="https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca5359">CA5359: Do not disable certificate validation</a>.</li>
<li>OWASP:
<a href="https://owasp.org/www-community/attacks/Manipulator-in-the-middle_attack">Manipulator-in-the-middle attack</a>.</li>
</references>
</qhelp>
116 changes: 116 additions & 0 deletions csharp/ql/src/Security Features/CWE-295/AcceptAnyCertificate.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/**
* @name Accepting any TLS certificate during validation
* @description A certificate validation callback that always accepts any certificate
* allows an attacker to perform a machine-in-the-middle attack.
* @kind path-problem
* @problem.severity error
* @security-severity 7.5
* @precision high
* @id cs/accept-any-certificate
* @tags security
* external/cwe/cwe-295
*/

import csharp
import semmle.code.csharp.dataflow.DataFlow::DataFlow
import AcceptAnyCertificate::PathGraph

/**
* Holds if `c` always returns `true` and never returns `false`, i.e. it accepts
* every input it is given.
*/
predicate alwaysReturnsTrue(Callable c) {
c.getReturnType() instanceof BoolType and
// 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")
}

/**
* A delegate type used as a TLS/SSL certificate validation callback. Such a
* delegate returns a `bool` (whether the certificate is trusted) and takes a
* `System.Net.Security.SslPolicyErrors` parameter describing any validation
* errors that were found. This covers `RemoteCertificateValidationCallback` as
* well as the `Func<..., SslPolicyErrors, bool>` callbacks used by, for example,
* `HttpClientHandler.ServerCertificateCustomValidationCallback`.
*/
class CertificateValidationCallbackType extends DelegateType {
CertificateValidationCallbackType() {
this.getReturnType() instanceof BoolType and
this.getAParameter().getType().hasFullyQualifiedName("System.Net.Security", "SslPolicyErrors")
}
}

/**
* Gets a callable that always accepts any certificate, referenced by the
* delegate-producing expression `e`.
*/
Callable getAcceptingCallable(Expr e) {
// A lambda or anonymous method, e.g. `(sender, cert, chain, errors) => true`.
result = e and
alwaysReturnsTrue(e)
or
// A method group, e.g. `AcceptAllCertificates`, possibly wrapped in an
// (implicit or explicit) delegate creation.
result = e.(DelegateCreation).getArgument().(CallableAccess).getTarget() and
alwaysReturnsTrue(result)
or
result = e.(CallableAccess).getTarget() and
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()))
or
// `HttpClientHandler.DangerousAcceptAnyServerCertificateValidator` is a
// built-in callback that accepts every certificate.
source
.asExpr()
.(PropertyAccess)
.getTarget()
.hasName("DangerousAcceptAnyServerCertificateValidator")
}

predicate isSink(DataFlow::Node sink) {
// The value assigned to a property, field or local of certificate
// validation callback type.
exists(Assignable a |
a.getType() instanceof CertificateValidationCallbackType and
sink.asExpr() = getAssignedDelegate(a)
)
or
// The value passed as a certificate validation callback argument, e.g. to
// the `SslStream` constructor.
exists(Call call, Parameter p |
p = call.getTarget().getAParameter() and
p.getType() instanceof CertificateValidationCallbackType and
sink.asExpr() = call.getArgumentForParameter(p)
)
}

predicate observeDiffInformedIncrementalMode() { any() }
}

module AcceptAnyCertificate = DataFlow::Global<AcceptAnyCertificateConfig>;

from AcceptAnyCertificate::PathNode source, AcceptAnyCertificate::PathNode sink
where AcceptAnyCertificate::flowPath(source, sink)
select sink.getNode(), source, sink,
"This TLS certificate validation $@, which trusts any certificate.", source.getNode(),
"uses a callback"
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: newQuery
---
* Added a new query, `cs/accept-any-certificate`, to detect TLS/SSL certificate validation callbacks that always accept any certificate (CWE-295).
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
edges
| 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: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: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 |
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Security Features/CWE-295/AcceptAnyCertificate.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
using System.IO;
using System.Net;
using System.Net.Http;
using System.Net.Security;
using System.Security.Cryptography.X509Certificates;

public class CertificateValidationTests
{
public void HttpClientHandlerBad()
{
var handler = new HttpClientHandler();
// BAD: always trusts any certificate.
handler.ServerCertificateCustomValidationCallback =
(request, certificate, chain, errors) => true;
}

public void HttpClientHandlerBlockBodyBad()
{
var handler = new HttpClientHandler();
// BAD: always trusts any certificate.
handler.ServerCertificateCustomValidationCallback =
(request, certificate, chain, errors) =>
{
return true;
};
}

public void HttpClientHandlerDangerousBad()
{
var handler = new HttpClientHandler();
// BAD: built-in callback that accepts any certificate.
handler.ServerCertificateCustomValidationCallback =
HttpClientHandler.DangerousAcceptAnyServerCertificateValidator;
}

public void ServicePointManagerBad()
{
// BAD: always trusts any certificate.
ServicePointManager.ServerCertificateValidationCallback =
(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 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.
var ssl = new SslStream(stream, false,
(sender, certificate, chain, errors) => true);
}

public void IndirectBad(Stream stream)
{
RemoteCertificateValidationCallback callback =
(sender, certificate, chain, errors) => true;
// BAD: the callback flowing here always returns true.
var ssl = new SslStream(stream, false, callback);
}

public void HttpClientHandlerGood()
{
var handler = new HttpClientHandler();
// GOOD: the certificate is only trusted when there are no validation errors.
handler.ServerCertificateCustomValidationCallback =
(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)
{
return errors == SslPolicyErrors.None;
}

public void MethodGroupGood()
{
// GOOD: the referenced method performs real validation.
ServicePointManager.ServerCertificateValidationCallback = Validate;
}
}
Original file line number Diff line number Diff line change
@@ -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