mirror of
https://github.com/github/codeql.git
synced 2026-06-10 23:41:09 +02:00
Add CWE-295 C# query for accepting any TLS certificate
This commit is contained in:
copilot-swe-agent[bot]
committed by
GitHub
GitHub
parent
f5919875b7
commit
677afff7af
@@ -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;
|
||||
}
|
||||
}
|
||||
@@ -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>
|
||||
101
csharp/ql/src/Security Features/CWE-295/AcceptAnyCertificate.ql
Normal file
101
csharp/ql/src/Security Features/CWE-295/AcceptAnyCertificate.ql
Normal file
@@ -0,0 +1,101 @@
|
||||
/**
|
||||
* @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 returned value, and every returned value is the
|
||||
// constant `true`.
|
||||
forex(Expr ret | c.canReturn(ret) | 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)
|
||||
}
|
||||
|
||||
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() = a.getAnAssignedValue()
|
||||
)
|
||||
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"
|
||||
@@ -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).
|
||||
@@ -0,0 +1,24 @@
|
||||
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 | |
|
||||
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 |
|
||||
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 |
|
||||
@@ -0,0 +1 @@
|
||||
Security Features/CWE-295/AcceptAnyCertificate.ql
|
||||
@@ -0,0 +1,89 @@
|
||||
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;
|
||||
}
|
||||
|
||||
private static bool AcceptAll(object sender, X509Certificate certificate, X509Chain chain,
|
||||
SslPolicyErrors errors)
|
||||
{
|
||||
return true;
|
||||
}
|
||||
|
||||
public void MethodGroupBad()
|
||||
{
|
||||
// BAD: the referenced method always returns true.
|
||||
ServicePointManager.ServerCertificateValidationCallback = AcceptAll;
|
||||
}
|
||||
|
||||
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;
|
||||
}
|
||||
|
||||
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;
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user