SignedXml - refuses valid signed XML file #21451
Comments
|
Next steps:
|
|
cc @StanislavUshakov @anthonylangsworth @tintoy @peterwurzinger - anyone interested to tackle it? |
|
I could take a look tomorrow morning (AU timezone) if nobody else wants it? |
|
I was going to put up my hand for it but the earliest I can get to it is tomorrow evening (also AU timezone). |
|
Appreciate your help guys, thanks! I guess whoever starts first should ping this issue ... thanks again! |
|
Ok - I'll have a look at this shortly. |
|
@karelz - I'll create a test for this scenario; assuming it fails, is that sufficient for a repro? |
|
Yep, that's what I had in mind. Sorry for not being clear. (updated above) |
|
NP - just bootstrapping the build on my new laptop and then I'll get right onto it. |
|
BTW: We have new docs for contributors (mostly new contribs): https://github.com/dotnet/corefx/wiki/New-contributor-Docs |
|
No worries - I have a Powershell profile that sets up environment variables for corefx / netfx dev, but building corefx itself has more subtle requirements it seems; had to drop back to using "Developer Command Prompt for VS2017" to get it to work. |
|
Hmm, interesting. After the build, I see diff --git a/src/shims/ApiCompatBaseline.netcoreapp.netfx461.txt b/src/shims/ApiCompatBaseline.netcoreapp.netfx461.txt
index 845369c..fe4e873 100644
--- a/src/shims/ApiCompatBaseline.netcoreapp.netfx461.txt
+++ b/src/shims/ApiCompatBaseline.netcoreapp.netfx461.txt
@@ -5,6 +5,11 @@ ApiCompat Error: 0 : Unable to resolve assembly 'Assembly(Name=System.ServiceMod
ApiCompat Error: 0 : Unable to resolve assembly 'Assembly(Name=System.Web.ApplicationServices, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)' referenced by the contract assembly 'Assembly(Name=System.Web, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a)'.
ApiCompat Error: 0 : Unable to resolve assembly 'Assembly(Name=System.Configuration, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a)' referenced by the contract assembly 'Assembly(Name=System.Web, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a)'.
ApiCompat Error: 0 : Unable to resolve assembly 'Assembly(Name=System.ServiceModel, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089)' referenced by the contract assembly 'Assembly(Name=System.Xml.Serialization, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089)'.
+Compat issues with assembly mscorlib:
+TypesMustExist : Type 'System.ArgIterator' does not exist in the implementation but it does exist in the contract.
+MembersMustExist : Member 'System.Console.Write(System.String, System.Object, System.Object, System.Object, System.Object, __arglist)' does not exist in the implementation but it does exist in the contract.
+MembersMustExist : Member 'System.Console.WriteLine(System.String, System.Object, System.Object, System.Object, System.Object, __arglist)' does not exist in the implementation but it does exist in the contract.
+MembersMustExist : Member 'System.String.Concat(System.Object, System.Object, System.Object, System.Object, __arglist)' does not exist in the implementation but it does exist in the contract.
Compat issues with assembly System:
MembersMustExist : Member 'System.CodeDom.Compiler.CompilerParameters.Evidence.get()' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.CodeDom.Compiler.CompilerParameters.Evidence.set(System.Security.Policy.Evidence)' does not exist in the implementation but it does exist in the contract. |
|
Urk! Ok, have run into a more serious problem. Opened @karelz I'm sure this worked on my old machine, and I have verified that required VS components are installed (read the contributor docs to be sure). Anything obvious I've missed? |
|
(interestingly, the test project builds fine, it's just the other 2 that don't) |
|
Did you compile from root first? @mellinoe @weshaggard can hopefully help troubleshoot ... |
|
Yep, |
|
Or if I build from developer command prompt, it just spits out the same wall of errors listed above. |
|
Are you able to build the csproj directly from the command line? |
|
cc @krwq |
|
@mellinoe yes! Just not the solution file. |
|
Wrong configuration selected perhaps? |
Hmm, nope - there are only 2 configurations listed. |
|
@krwq just make some configuration changes to this project so I suspect we need to regenerate the sln file. You can do that by doing https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/project-guidelines.md#updating-configurations. |
|
@weshaggard - thanks, that did the trick. Had to edit |
|
Test fails, but according to this online verifier the signature is invalid, too. Perhaps it's a whitespace issue? Was the original XML supplied in a file or perhaps mangled by form / forum page? |
|
I copied it from web page. There was also file ... digging ... |
|
Here are the files from the repro:
Code file: verify.cs XmlDocument xmlDocument = new XmlDocument();
// Format using white spaces.
xmlDocument.PreserveWhitespace = true;
// Load the passed XML file into the document.
try
{
xmlDocument.Load("signedNOT_OK_xdsNS_inSignatureNode.xml");
}
catch (Exception e)
{
throw new CryptographicException("Unable to load XML document. Error: " + e.Message);
}
// Create a new SignedXml object and pass it
// the XML document class.
SignedXml signedXml = new SignedXml(xmlDocument);
// Find the "Signature" node and create a new
// XmlNodeList object.
//XmlNodeList nodeList = xmlDocument.GetElementsByTagName("Signature", SignedXml.XmlDsigNamespaceUrl);
// Throw an exception if no signature was found.
if (nodeList.Count <= 0)
{
throw new CryptographicException("No Signature was found in the document.");
}
// This example only supports one signature for
// the entire XML document. Throw an exception
// if more than one signature was found.
if (nodeList.Count >= 2)
{
throw new CryptographicException("More that one signature was found for the document.");
}
// Load the signature node.
signedXml.LoadXml((XmlElement)nodeList[0]);
var x509data = signedXml.Signature.KeyInfo.OfType<KeyInfoX509Data>().First();
if (x509data != null)
{
signatureCert = x509data.Certificates[0] as X509Certificate2;
}
if (signatureCert == null)
{
ErrorMessage = "Signing certficate not present in the XML.";
return false;
}
AsymmetricAlgorithm key = signatureCert.PublicKey.Key;
string keyName = GetKeyName(key);
// Check the signature and return the result.
bool ok = signedXml.CheckSignature(signatureCert, true); |
|
Ok, tried that, too - the online validator says it's still not valid: |
|
They claim other XMLDSIG implementations can successfully validate that XML document, and the only online one I've found claims that the certificate is not valid (which may indicate that the signature is valid but the signing key fails X.509 certificate validation). |
|
This online validator claims that the document is valid, BTW (but also that the cert is not). So yes it does seem like there might be a bug here. |
|
@anthonylangsworth - do you have an opinion here about what constitutes correct behaviour? |
|
@tintoy thank you a lot for investigating this! I've tried the document with the validator you provided and modified a doc a little bit (outside of the signature) and can confirm that the online validator claims that unmodified doc is ok and modified is not ok. Given that - looks like a product bug and likely related to propagating namespaces. @karelz @bartonjs - despite this being a product bug I do believe this won't meet the bar for 2.0 considering following:
IMO: I believe this should be fixed for the next release though (regular priority). If anyone sends a patch we should approve it for 2.0 (assuming no API addition required). |
|
Hello, @karelz @tintoy and @krwq. First of all, great effort in porting the XmlDSig feature and also in trying to tackle this long-standing issue. I am on the same team as Bruno C Dias Ribeiro (@brunocapu), and we've found the workaround mentioned on the Connect report together. We'd like to offer our help with this issue.
Unfortunately, this is not the case. Consider the following document: https://files.lacunasoftware.com/temp/19198/false-positive.xml Notice: please let us know if you'd like a slightly different false positive case, for instance with a X509Certificate element instead of KeyValue. This document is reported as NOT VALID by the validator at aleksey.com. Validating with Java also gives NOT VALID (please let us know if you'd like our validation code in Java). However, validating with the SignedXml class may yield VALID, depending on whether the workaround is used. If it is used, the result is NOT VALID (correct). However, if it is not used, i.e. if the SignedXml constructor is called with the XmlDocument, validation yields VALID. I can't think of any way how this might be exploited in practice, but generally if there is a false positive in a signature validation it's only a matter of creativity and time to come up with a practical exploit.
The issue is with the canonicalization of the SignedInfo element. AFAIK it affects all kinds of signatures. We've been able to reproduce it with both enveloped signatures (signature over the entire XML document, i.e. Reference with URI="") and detached signatures (signature over an element of the XML document, i.e. Reference with URI="#id").
Not really, the references are taken into account. If they weren't, the validation of a correct signature would certainly fail, since the references were taken into account when producing the signature. We'd like to help with this issue in any way we can, since we believe it is a bug. |
|
The plain reasoning behind the workaround is to change the behavior of the following line: private byte[] GetC14NDigest (HashAlgorithm hash) {
// ...
CanonicalXmlNodeList namespaces = (m_context == null ? null : Utils.GetPropagatedAttributes(m_context));
// ...
}Calling the constructor of the This yields different results if namespace prefixes are added on elements between (but excluding) the root document and (including) the signature element: Any namespace prefixes added on the elements marked above would cause differences on the behavior of the The Canonical XML 1.0 algorithm definition clearly states that the correct behavior in this case is to take into account the prefixes inherited by the Signature element. Please notice that this is not the case for the exclusive canonicalization algorithm, which is currently not implemented on the .NET Framework Unfortunately this is not uncommon. A quite common case is adding the namespace prefix "ds" on the signature element itself, which is what the external tool that GregorM was using (from the Connect report) does. However, there are infinite other cases. We believe this issue comes from a poor documentation of the constructors in the We think that the desired effects for each case would be:
Again, none of that is stated on the documentation of the class, that's simply what we think is reasonable to imply from the API. We believe the problem lies on implementation of case 2a, when the code assumes that the context of the validation is the root element -- especially given that the caller will inevitably pass the actual signature element later on the Therefore, we'd like to propose a fix consisting of changing the public void LoadXml(XmlElement value) {
if (value == null)
throw new ArgumentNullException("value");
m_signature.LoadXml(value);
//if (m_context == null) {
m_context = value;
//}
bCacheValid = false;
}In other words, when the |
|
@karelz , we talked on #15603 yesterday about this thread. I'm not sure what it is that you need us to do. You yourself created this thread because of a connect report of a supposed bug on the XmlDSig implementation on .NET Framework. On the course of this thread, it seems the collaborators have come to a conclusion that it is perhaps not a bug, and that it can be addressed later since it was thought that the implementation never says an invalid document is valid (a "false positive"). We then provided evidence that that is in fact not true, that the current implementation does say invalid documents are valid, depending on the way the API is used ("not using the workaround"). We then proposed a fix. I'm sorry, this is the first time my team has tried to collaborate with .NET efforts, we are a bit lost regarding the contribution process. What do you need us to do next? |
|
@leopignataro thanks for the details, it's helpful. The fix of course has to be reviewed and thought through from all angles - incl. impact on compatibility and performance of all scenarios hitting the code path. And the fix has to have good code coverage. |
maryamariyan
added
the
untriaged
|
This issue still exists in .NET 5.0 - any update would be appreciated... Edit, okay it seems like it was too late when I wrote the original version of the Post :( Can anyone please help me, why that signature does not work when validating in .NET but it works in Java as well as with xmlsec1: |
|
@phax, we do not currently plan to fix this as it has relatively low priority comparing to other issues we have but we're open for contribution. |
|
I'm having this problem with .NET 6 and the failure of validating the SignedInfo element. Either I'm doing something wrong or this is still a live bug. These links suggest that the .NET implementation fails to propagate the namespaces correctly to the SignedInfo element during the Inclusive C14N Canonicalization process. I've also managed to enable the Debug logging of the SignedXml.CheckSignature method and it clearly shows that it is the SignedInfo validation that fails. // Enable debug logging of System.Security.Cryptography.Xml
let traceSource =
Assembly.Load("System.Security.Cryptography.Xml")
.GetType("System.Security.Cryptography.Xml.SignedXmlDebugLog")
.GetField("s_traceSource", (BindingFlags.Static ||| BindingFlags.NonPublic))
.GetValue(null) :?> TraceSource
let added = traceSource.Listeners.Add(new TextWriterTraceListener(System.Console.Out))
traceSource.Switch.Level <- SourceLevels.VerboseGiven time I'll see if it's possible to create a proper reproduction repo. However, when inspecting the Canonicalized SignedInfo i see that it does propagate the namespaces as specified. I'll just have to see if it can be replicated in a repo. |
|
I've recreated @karelz reproduction as a public reproduction repo in dotnet 6.0.203 with the debug info enabled. |
|
Tagging @bartonjs @GrabYourPitchforks who own the area now. |
|
The issue still exist in .NET 7, is there any update? |
Not sure exactly what your situation is, and I'm going to completely hack this explanation cause I'm going from memory... My situation was validating a SAML token. The token had the overall Document signed and the Assertion Element signed. The overall Document signature checked out fine prior to .NET 7, but the Assertion signature was failing due to this issue. After the 'fix' was in, my Assertion signature still wasn't passing. I eventually figured out it was because the Assertion Element was isolated from the main XmlDocument, and a new XmlDocument was created out of it. This was the way it was done on about every example you would find on the internet. When I checked the Assertion signature based off of referencing the element in the original XmlDocument, then the signature checked out. |
|
@willisd2 my issues is exactly the sames as @phax mentioned. Maybe there are two seperated issues discussed in this thread. Mine is also the same as: https://stackoverflow.com/questions/47694541/wrong-canonicalization-c14n-made-by-signedxml-it-removes-line-breaks-13 |
|
so, year 6 of this same issue, completely prevents anyone from making a secure saml implementation in dotnet |
|
I just happened to stumble the need of this too. Welcome to 2024. Subscribed. |
|
The specific problems described in this issue are varied. Some of them look like a known issue of .NET SignedXml not playing nicely with carriage returns ( I've commented on that at #99856 and is where future discussion for the carriage return problem should happen. Any issues that aren't specifically related to carriage returns should open a new issue; but be aware the .NET team is unlikely to fix any problems that aren't "SignedXml can't read a signature produced by an earlier version of the package (or .NET Framework)" |
Created based on Connect report: http://connect.microsoft.com/VisualStudio/feedback/details/1765025/verify-xml-signature-getc14ndigest-problem-in-signedxml-class (internal bug 144292)
Description: I've a problem with verifying XML signature file. The signature is done by third-party application and I am verifying the signature of that XML file in .NET framework 4.0 (check with latestes 4.6 and there is no difference).
To identify the problem, I've taken a sample.xml
When I sign the xml with the third-party app, I get signed.xml:
(Note: The original issue had no (non-required) whitespace in the XML, it was pretty-printed here to allow for better human understanding)
This signed XML is valid, according to let's say this (https://www.signatur.rtr.at/en/elsi/Pruefung.html) tool, and I've verified it with a few other tools and all confirm that it's OK, except when I verify it with SignedXml class from .NET Framework.
The code I'am using to verify the XML.
Now the problem as I've managed to identify is in the SignedXml class in function
GetC14NDigestwhereUtils.GetPropagatedAttributes(m_context)is called.m_contextis signed XML document, but the namespacexmlns:xds="http://uri.etsi.org/01903/v1.1.1#"is only in Signature node, which was propagated into theSignedInfonode, but theUtils.GetPropagatedAttributes(m_context)did not return that one, and the SignedInfo node is missing that one, so the SignedInfo node is not the same as it was when the signature was constructed.I've tried to sign an xml like this:
and this signed XML I was able to verify successfully with SignedXml class.
The text was updated successfully, but these errors were encountered: