Create rule S6377: XML signatures should be verified securely

[gaetan-ferry-sonarsource]

Why

As part of MMF-3716, we want to close the gap between C# and other languages regarding cryptography related rules support. S6377 is one of the rules that is not currently supported by this analyzer.

What

S6377 aims to detect when XML signatures are insecurely validated. We want to add support for this behavior for both .NET core and .NET framework. Note that XML related cryptographic features are implemented as part of system.security.cryptography.xml a .NET platform extension.

Detection logic

This rule should raise any time code validates a signature without relying on a trusted public key. In that case, the could would use the signature-embedded public key to perform the validation and would be open to forgery attacks.

We want to raise when:

• System.Security.Cryptography.Xml.SignedXml.CheckSignature() is called (without a parameter).
• System.Security.Cryptography.Xml.SignedXml.CheckSignatureReturningKey is called.

Example code

XmlDocument xmlDoc = new()
{
    PreserveWhitespace = true
};
xmlDoc.Load("/data/login.xml");
SignedXml signedXml = new(xmlDoc);
XmlNodeList nodeList = xmlDoc.GetElementsByTagName("Signature");
signedXml.LoadXml((XmlElement?)nodeList[0]);
if (signedXml.CheckSignature()) { // Noncompliant
    // Process the XML content
} else {
    // Raise an error
}

CspParameters cspParams = new()
{
    KeyContainerName = "MY_RSA_KEY"
};
RSACryptoServiceProvider rsaKey = new(cspParams);

XmlDocument xmlDoc = new()
{
    PreserveWhitespace = true
};
xmlDoc.Load("/data/login.xml");
SignedXml signedXml = new(xmlDoc);
XmlNodeList nodeList = xmlDoc.GetElementsByTagName("Signature");
signedXml.LoadXml((XmlElement?)nodeList[0]);
if (signedXml.CheckSignature(rsaKey)) { // Compliant
    // Process the XML content
} else {
    // Raise an error
}

RSPEC

This rule's RSPEC (from this PR SonarSource/rspec#3814) contains information regarding messages and highlighting.

Message

Change this code to only accept signatures computed from a trusted party.

Highlight

The call to the signature verification function:

• System.Security.Cryptography.Xml.SignedXml.CheckSignature
• System.Security.Cryptography.Xml.SignedXml.CheckSignatureReturningKey

[costin-zaharia-sonarsource]

Hi, @gaetan-ferry-sonarsource!

The specification mentions that we need to raise when System.Security.Cryptography.Xml.SignedXml.CheckSignature() is called without a parameter.

The following noncompliant code example verifies an XML signature without providing a trusted public key.

However, according to the Microsoft documentation:

If an XML document was signed with an X.509 signature, the CheckSignature method will search the "AddressBook" store for certificates suitable for the verification. For example, if the certificate is referenced by a Subject Key Identifier (SKI), the CheckSignature method will select certificates with this SKI and try them one after another until it can verify the certificate.

My understanding is that the call will look in the "AddressBook" store for a trusted certificate that can validate the document. The rule description does not mention that and does not explain why using these certificates is not considered a safe approach.

[gaetan-ferry-sonarsource]

Hi @costin-zaharia-sonarsource !

When a document is signed with an X.509 signature, the application might indeed check some system keystore for a matching key in order to validate the signature. That said, I think it still causes multiple issues:

• The typical trust store of a Windows machine will typically contain all currently trusted root CA certificates. This means the application might accept any signature computed using a valid, trusted, certificate that has nothing to do with the application itself. For example, one could request a Let's Encrypt server certificate for any domain and then use the corresponding key and certificate to compute a signature for the application. There might be additional checks on the certificate to prevent this from happening, I did not check this, but that's the general attack idea.
• When the document is not signed using a X.509 signature, the application will simply use the key that is embedded in the signature element. This is totally insecure. The documentation you linked also mentions this:

Determines whether the Signature property verifies using the public key in the signature.

For information, you can see what and how we tested in the appsec-poc repository.

Regarding the rule description, we would probably need to improve it a bit. I created a hardening ticket for this. Still, I am unsure if it would work to explain all the corner cases in the RSPEC. I feel it might affect the rule readability, but we'll look into this.

I hope this helps.