karelz
commented
7 years ago Next steps:
- Turn the repro into test. Confirm it repros.
- Fix the bug.
Open karelz opened 7 years ago
karelz
commented
7 years ago Next steps:
karelz
commented
7 years ago cc @StanislavUshakov @anthonylangsworth @tintoy @peterwurzinger - anyone interested to tackle it?
tintoy
commented
7 years ago I could take a look tomorrow morning (AU timezone) if nobody else wants it?
anthonylangsworth
commented
7 years ago I was going to put up my hand for it but the earliest I can get to it is tomorrow evening (also AU timezone).
karelz
commented
7 years ago Appreciate your help guys, thanks! I guess whoever starts first should ping this issue ... thanks again!
tintoy
commented
7 years ago Ok - I'll have a look at this shortly.
tintoy
commented
7 years ago @karelz - I'll create a test for this scenario; assuming it fails, is that sufficient for a repro?
karelz
commented
7 years ago Yep, that's what I had in mind. Sorry for not being clear. (updated above)
tintoy
commented
7 years ago NP - just bootstrapping the build on my new laptop and then I'll get right onto it.
karelz
commented
7 years ago BTW: We have new docs for contributors (mostly new contribs): https://github.com/dotnet/corefx/wiki/New-contributor-Docs If you have gotchas, please add them - it is writeable wiki without PR system.
tintoy
commented
7 years ago 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.
tintoy
commented
7 years ago Hmm, interesting. After the build, I see src/shims/ApiCompatBaseline.netcoreapp.netfx461.txt has changed:
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 System.Security.Cryptography.Xml.sln in VS2017, hit build, and run into a wall of errors:
1>------ Build started: Project: System.Security.Cryptography.Xml (ref\System.Security.Cryptography.Xml), Configuration: netcoreapp-Debug Any CPU ------
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(3,27,3,55): error CS0234: The type or namespace name 'AllowPartiallyTrustedCallersAttribute' does not exist in the namespace 'System.Security' (are you missing an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(3,27,3,55): error CS0234: The type or namespace name 'AllowPartiallyTrustedCallers' does not exist in the namespace 'System.Security' (are you missing an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(4,18,4,25): error CS0234: The type or namespace name 'Runtime' does not exist in the namespace 'System' (are you missing an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(5,18,5,28): error CS0234: The type or namespace name 'Reflection' does not exist in the namespace 'System' (are you missing an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(6,11,6,24): error CS0246: The type or namespace name 'AssemblyTitleAttribute' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(6,11,6,24): error CS0246: The type or namespace name 'AssemblyTitle' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(7,11,7,30): error CS0246: The type or namespace name 'AssemblyDescriptionAttribute' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(7,11,7,30): error CS0246: The type or namespace name 'AssemblyDescription' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(8,11,8,31): error CS0246: The type or namespace name 'AssemblyDefaultAliasAttribute' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(8,11,8,31): error CS0246: The type or namespace name 'AssemblyDefaultAlias' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(9,11,9,26): error CS0246: The type or namespace name 'AssemblyCompanyAttribute' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(9,11,9,26): error CS0246: The type or namespace name 'AssemblyCompany' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(10,11,10,26): error CS0246: The type or namespace name 'AssemblyProductAttribute' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(10,11,10,26): error CS0246: The type or namespace name 'AssemblyProduct' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(11,11,11,28): error CS0246: The type or namespace name 'AssemblyCopyrightAttribute' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(11,11,11,28): error CS0246: The type or namespace name 'AssemblyCopyright' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(12,11,12,26): error CS0246: The type or namespace name 'AssemblyVersionAttribute' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(12,11,12,26): error CS0246: The type or namespace name 'AssemblyVersion' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(13,11,13,30): error CS0246: The type or namespace name 'AssemblyFileVersionAttribute' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(13,11,13,30): error CS0246: The type or namespace name 'AssemblyFileVersion' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(14,11,14,39): error CS0246: The type or namespace name 'AssemblyInformationalVersionAttribute' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(14,11,14,39): error CS0246: The type or namespace name 'AssemblyInformationalVersion' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(15,11,15,23): error CS0246: The type or namespace name 'CLSCompliantAttribute' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(15,11,15,23): error CS0246: The type or namespace name 'CLSCompliant' could not be found (are you missing a using directive or an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(16,18,16,28): error CS0234: The type or namespace name 'Reflection' does not exist in the namespace 'System' (are you missing an assembly reference?)
1>D:\Development\github\dotnet\corefx\bin\obj\ref\System.Security.Cryptography.Xml\4.0.0.0\netcoreapp\_AssemblyInfo.cs(17,18,17,28): error CS0234: The type or namespace name 'Reflection' does not exist
<SNIP>
3>------ Build started: Project: System.Security.Cryptography.Xml.Tests, Configuration: netcoreapp-Debug Any CPU ------
3> System.Security.Cryptography.Xml.Tests -> D:\Development\github\dotnet\corefx\bin\AnyOS.AnyCPU.Debug\System.Security.Cryptography.Xml.Tests\netcoreapp\System.Security.Cryptography.Xml.Tests.dll
========== Build: 1 succeeded, 2 failed, 0 up-to-date, 0 skipped ==========@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?
tintoy
commented
7 years ago (interestingly, the test project builds fine, it's just the other 2 that don't)
karelz
commented
7 years ago Did you compile from root first? @mellinoe @weshaggard can hopefully help troubleshoot ...
tintoy
commented
7 years ago Yep, .\build.cmd. That works fine. Interestingly, if I run msbuild src\System.Security.Cryptography.Xml\System.Security.Cryptography.Xml.sln it also fails (can't find Microsoft.CSharp.Portable.targets).
tintoy
commented
7 years ago Or if I build from developer command prompt, it just spits out the same wall of errors listed above.
mellinoe
commented
7 years ago Are you able to build the csproj directly from the command line?
karelz
commented
7 years ago cc @krwq
tintoy
commented
7 years ago @mellinoe yes! Just not the solution file.
tintoy
commented
7 years ago Wrong configuration selected perhaps?
tintoy
commented
7 years ago Wrong configuration selected perhaps?
Hmm, nope - there are only 2 configurations listed.
weshaggard
commented
7 years ago @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.
tintoy
commented
7 years ago @weshaggard - thanks, that did the trick. Had to edit build.proj though - the UpdateVSConfigurations target had a dependency on a non-existentBuildCoreFxTools target.
tintoy
commented
7 years ago 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?
karelz
commented
7 years ago I copied it from web page. There was also file ... digging ...
karelz
commented
7 years ago 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:
func=xmlSecOpenSSLX509StoreVerify:file=x509vfy.c:line=360:obj=x509-store:subj=X509_verify_cert:error=4:crypto library function failed:subj=/CN=Prvi Uporabnik/OU=i4 user/O=Test d.o.o./L=Tolmin/C=SI;err=20;msg=unable to get local issuer certificate
func=xmlSecOpenSSLX509StoreVerify:file=x509vfy.c:line=408:obj=x509-store:subj=unknown:error=71:certificate verification failed:err=20;msg=unable to get local issuer certificate
func=xmlSecOpenSSLEvpSignatureVerify:file=signatures.c:line=493:obj=rsa-sha1:subj=EVP_VerifyFinal:error=18:data do not match:signature do not match
RESULT: Signature is INVALID
---------------------------------------------------
= VERIFICATION CONTEXT
== Status: invalid
== flags: 0x00000000
== flags2: 0x00000000
== Key Info Read Ctx:
= KEY INFO READ CONTEXT
== flags: 0x00000000
== flags2: 0x00000000
== enabled key data: all
== RetrievalMethod level (cur/max): 0/1
== TRANSFORMS CTX (status=0)
== flags: 0x00000000
== flags2: 0x00000000
== enabled transforms: all
=== uri: NULL
=== uri xpointer expr: NULL
== EncryptedKey level (cur/max): 0/1
=== KeyReq:
==== keyId: rsa
==== keyType: 0x00000001
==== keyUsage: 0x00000002
==== keyBitsSize: 0
=== list size: 0
== Key Info Write Ctx:
= KEY INFO WRITE CONTEXT
== flags: 0x00000000
== flags2: 0x00000000
== enabled key data: all
== RetrievalMethod level (cur/max): 0/1
== TRANSFORMS CTX (status=0)
== flags: 0x00000000
== flags2: 0x00000000
== enabled transforms: all
=== uri: NULL
=== uri xpointer expr: NULL
== EncryptedKey level (cur/max): 0/1
=== KeyReq:
==== keyId: NULL
==== keyType: 0x00000001
==== keyUsage: 0xffffffff
==== keyBitsSize: 0
=== list size: 0
== Signature Transform Ctx:
== TRANSFORMS CTX (status=2)
== flags: 0x00000000
== flags2: 0x00000000
== enabled transforms: all
=== uri: NULL
=== uri xpointer expr: NULL
=== Transform: c14n (href=http://www.w3.org/TR/2001/REC-xml-c14n-20010315)
=== Transform: rsa-sha1 (href=http://www.w3.org/2000/09/xmldsig#rsa-sha1)
=== Transform: membuf-transform (href=NULL)
== Signature Method:
=== Transform: rsa-sha1 (href=http://www.w3.org/2000/09/xmldsig#rsa-sha1)
== Signature Key:
== KEY
=== method: RSAKeyValue
=== key type: Private
=== key name: test-rsa
=== key usage: -1
=== rsa key: size = 1024
== SignedInfo References List:
=== list size: 1
= REFERENCE VERIFICATION CONTEXT
== Status: succeeded
== URI: ""
== Reference Transform Ctx:
== TRANSFORMS CTX (status=2)
== flags: 0x00000000
== flags2: 0x00000000
== enabled transforms: all
=== uri: NULL
=== uri xpointer expr: NULL
=== Transform: enveloped-signature (href=http://www.w3.org/2000/09/xmldsig#enveloped-signature)
=== Transform: c14n (href=http://www.w3.org/TR/2001/REC-xml-c14n-20010315)
=== Transform: sha1 (href=http://www.w3.org/2000/09/xmldsig#sha1)
=== Transform: membuf-transform (href=NULL)
== Digest Method:
=== Transform: sha1 (href=http://www.w3.org/2000/09/xmldsig#sha1)
== Manifest References List:
=== list size: 0
'''I also tried the repro code supplied above, loading directly from signedNOT_OK_xdsNS_inSignatureNode.xml.txt (preserving whitespace), and validation still fails.
Will have a go to see if it works on netfx, but it looks to me like this doesn't repro on corefx.
karelz
commented
7 years ago Interesting. Either CoreFX vs. NetFX difference, or invalid bug report ...
weshaggard
commented
7 years ago @weshaggard - thanks, that did the trick. Had to edit build.proj though - the UpdateVSConfigurations target had a dependency on a non-existentBuildCoreFxTools target.
Thanks I will submit a fix for that and a PR to update all the configurations.
karelz
commented
7 years ago Actually there seems to be workaround suggested on the Connect issue which I missed (sorry!): https://connectadmin/Feedback/ConnectTab.aspx?FeedbackID=1765025
Here it is (from Bruno C Dias Ribeiro on 4/6/2016 at 5:46 PM):
We caught ourselves in this same issue here while trying to validate the Peru TSL (Trust Service List) (https://iofe.indecopi.gob.pe/TSL/tsl-pe.xml). It did not propagate the etsi namespace defined in the \<Signature> element. Thus, it got a wrong hash from canonicalization. After some code analysis in the .Net reference source, we noticed that to get the correct context for namespace propagation, we should pass the SignatureElement instead of the xmlDocument in the SignedXml constructor:
XmlNodeList nodeList = xmlDocument.GetElementsByTagName("Signature", SignedXml.XmlDsigNamespaceUrl);
SignedXml signedXml = new SignedXml((XmlElement)nodeList[0]);
signedXml.LoadXml((XmlElement)nodeList[0]);instead of
SignedXml signedXml = new SignedXml(xmlDocument);
XmlNodeList nodeList = xmlDocument.GetElementsByTagName("Signature", SignedXml.XmlDsigNamespaceUrl);
signedXml.LoadXml((XmlElement)nodeList[0]);Give it a try and tell us what you got.
karelz
commented
7 years ago I don't know enough about SignedXml to judge if it makes the original bug report invalid, or what else to think about it. @tintoy do you have opinion? (Or should I ask @krwq to take a look?)
It seems that it helped the original reporter:
thank you for reply. I have checked the Bruno C Dias Ribeiro's hint and it works like that. If you consider that it could work in either way you create SignedXml class than you can fix it, but I am ok with Bruno C Dias Ribeiro's solution. Thank you Bruno C Dias Ribeiro for the solution.
tintoy
commented
7 years ago Sure, I'll give that a go shortly and let you know.
tintoy
commented
7 years ago Ok, so I definitely think it feels weird to have to create a SignedXml from an XmlDocument and then call LoadXml against the <Signature/> element before the signature even shows up correctly.
Having said that, the following still doesn't validate correctly:
// Repro for dotnet/corefx#19198.
[Fact]
void Verify_XdsNamespace()
{
const string inputXml = @"<?xml version=""1.0"" encoding=""UTF-8"" standalone=""no"" ?><a><b>A</b><ds:Signature xmlns:ds=""http://www.w3.org/2000/09/xmldsig#"" xmlns:xds=""http://uri.etsi.org/01903/v1.1.1#""><ds:SignedInfo><ds:CanonicalizationMethod Algorithm=""http://www.w3.org/TR/2001/REC-xml-c14n-20010315""/><ds:SignatureMethod Algorithm=""http://www.w3.org/2000/09/xmldsig#rsa-sha1""/><ds:Reference URI=""""><ds:Transforms><ds:Transform Algorithm=""http://www.w3.org/2000/09/xmldsig#enveloped-signature""/><ds:Transform Algorithm=""http://www.w3.org/TR/2001/REC-xml-c14n-20010315""/></ds:Transforms><ds:DigestMethod Algorithm=""http://www.w3.org/2000/09/xmldsig#sha1""/><ds:DigestValue>/l5xchvEjxgLdPNt9RiB3TTQTcc=</ds:DigestValue></ds:Reference></ds:SignedInfo><ds:SignatureValue>cSUDo87XVNqbo+ljCZRAXvcuIXkak/10XyVWks9BNz5EdUmcVbiU84Y5qrJoAkMdyHPiIFkz8Dx5
v5psK5oZjGtRleQ67nm1BryHA8F7EW4Otxe/8hEHsqVFK0zz3P79XzKD2vl4lUaTTOLlCMD+Sbpb
ekyLkDIjXS/6IylPWbNNF3sH9MclGGhSSjREwOuJAyj8xqqibQEDvIytLN23+bZJtOGAU54ERXPn
h4rccBvzByno08DHVnkQrSQCgY4ECvVbocIFo7GGtq8v9oj6rK3KpWUQnL1V1Aqj5fXRNRC8VnxJ
yIkHAXOBWC3Wr+DQzEm4W0Xa+vZ8o0x/2Ct2cg==</ds:SignatureValue><ds:KeyInfo><ds:X509Data><ds:X509Certificate>MIIEejCCA+OgAwIBAgIIBg5jkS3DgTkwDQYJKoZIhvcNAQEFBQAwOjETMBEGA1UECxMKSW5m
cmF4STRDQTEWMBQGA1UEChMNSW5mcmF4IGQuby5vLjELMAkGA1UEBhMCU0kwHhcNMTEwNTAz
MDkxMzU2WhcNMTYwNTAxMDkxMzU2WjBfMRcwFQYDVQQDEw5QcnZpIFVwb3JhYm5pazEQMA4G
A1UECxMHaTQgdXNlcjEUMBIGA1UEChMLVGVzdCBkLm8uby4xDzANBgNVBAcTBlRvbG1pbjEL
MAkGA1UEBhMCU0kwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCkkkadn/Uapod4
N0f7GVgDNNN/vRoNUzqbdsERvUA7f/MoARF0LoXAoxpAEk2DDf24oIkGjL60uqwx4di5LQr8
3zsdA/7Wh7RHb73jCv0glfwK/YpfJcsxfLi/OajWNX5p63D9ISSDYtIgUb4k4JFElg+mbMsR
FlPQNyR6bADQTWputmTE+reWRiHLlnXLNSH3wGO008BfYuBnsuWupK8ar5UQ5I2IyafZJONO
4UQjsXsanYGqivWm147bihj7zfDjqEmVg0yYm9NS/jbhQSXRV9n8348engtu48NDg7Jsr363
Ou1nvThzsxIyRBbqSCNSJPAiEYl/jF4WRO2zGmZzAgMBAAGjggHeMIIB2jAMBgNVHRMBAf8E
AjAAMCAGA1UdJQEB/wQWMBQGCCsGAQUFBwMCBggrBgEFBQcDBDAfBgNVHSMEGDAWgBSjdHM8
IbieDPxF1rwNvMKR0T6WQDB3BgNVHR8EcDBuMGygaqBohmZodHRwOi8vY2EuaW5mcmF4LnNp
L2VqYmNhL3B1YmxpY3dlYi93ZWJkaXN0L2NlcnRkaXN0P2NtZD1jcmwmaXNzdWVyPU9VPUlu
ZnJheEk0Q0EsTz1JbmZyYXggZC5vLm8uLEM9U0kwDgYDVR0PAQH/BAQDAgSwMB0GA1UdDgQW
BBSKMErpxIQp6TQ8Jqr0ywYrYla4NzCB3gYIKwYBBQUHAQEEgdEwgc4wgYkGCCsGAQUFBzAC
hn1odHRwOi8vY2EuaW5mcmF4LnNpL2VqYmNhL3B1YmxpY3dlYi93ZWJkaXN0L2NlcnRkaXN0
P2NtZD1pZWNhY2VydCZpc3N1ZXI9T1UlM2RJbmZyYXhJNENBJTJjTyUzZEluZnJheCtkLm8u
by4lMmNDJTNkU0kmbGV2ZWw9MDBABggrBgEFBQcwAYY0aHR0cDovL2NhLmluZnJheC5zaTo4
MDgwL2VqYmNhL3B1YmxpY3dlYi9zdGF0dXMvb2NzcDANBgkqhkiG9w0BAQUFAAOBgQAms5UL
/f0AIavlVo9C5W5Ijdt12g/59PIZeAEDWzoEKi+WrE7ORaq527UHgcVGw7Gr2HhOEHmggUe6
k43edQ0fkNg5WXfJzf18hHA+foVqsxraDxBeot442A1zw9GjRnIDCl2r91tHgkneqg2EE8kf
7lkRtMRck1MUbHBnLnppKg==</ds:X509Certificate></ds:X509Data></ds:KeyInfo></ds:Signature></a>";
XmlDocument input = new XmlDocument { PreserveWhitespace = true };
input.Load(input);
XmlElement signatureElement = input
.GetElementsByTagName("Signature", SignedXml.XmlDsigNamespaceUrl)
.OfType<XmlElement>()
.First();
SignedXml signedXml = new SignedXml(input);
signedXml.LoadXml(signatureElement);
X509Certificate2 signatureCertificate = signedXml.Signature.KeyInfo
.OfType<KeyInfoX509Data>()
.SelectMany(
keyInfo => keyInfo.Certificates.OfType<X509Certificate2>()
)
.First();
bool isSignatureValid = signedXml.CheckSignature(signatureCertificate.PublicKey.Key);
Assert.True(isSignatureValid);
}From memory there are 2 or 3 kinds of XML-DSIG formats (attached, detached, etc). I wonder if this represents an edge case? As far as which is more correct, sorry, but I think that may lie a bit outside my area of expertise; I'm always happy to help out here (with anything really) but I don't think have enough domain knowledge to know what (if any) the side-effects would be from implicitly selecting the correct element. Maybe @anthonylangsworth have a better idea? Or yes, @krwq :)
tintoy
commented
7 years ago Oops! Misread their code - when I do the double-load of the signature element, it does validate correctly.
But it kinda looks to me like the signature is being loaded without the elements that it's meant to refer to, so what does "signature is valid" actually even mean in this context?
So yes, the workaround does show that it's possible to validate the signature but only in isolation of the thing that was signed (which seems kinda pointless). FWIW it's starting to sound like a bug to me (proves signature is valid, but overall validation fails because node canonicalisation is incorrectly performed).
tintoy
commented
7 years ago FWIW it's starting to sound like a bug
Although perhaps not a trivial one to fix (I can see roughly what needs to be fixed but since it's used elsewhere, risk of regressions may be increased).
krwq
commented
7 years ago @tintoy - so do I understand correctly that this document has been accidentally not signed at all? (if nothing is being hashed then it's technically not signed) If yes - is this a result of improper API usage by a customer or bug in .NET? (or perhaps not intuitively designed API promoting the bad usage)
I'm quite confused by
it kinda looks to me like the signature is being loaded without the elements
vs
node canonicalisation is incorrectly performed
unless you meant that one is the result of another - I believe we do have a test confirming that modifying a node is causing validation failure which suggest to me that we do have a workaround and it is not high priority for 2.0
tintoy
commented
7 years ago @krwq I could be missing something obvious, but here's what I see when I look at the workaround code above:
<Signature /> element.<Signature /> element contains a signature relating to the elements that contain it (I think that's how it works, but I could be wrong).SignedXml (i.e. without the elements that contain it).SignedXml know what elements to canonicalise and hash in order to compare them to the signature?tintoy
commented
7 years ago (I guess I'm just worried that I've missed something obvious, because the workaround above doesn't look to me like it does what I'm hearing people say it does; to me, it looks like it verifies the signature against the signature value, but doesn't compare the signature value to the elements that were signed)
krwq
commented
7 years ago @tintoy please correct me if I understood you incorrectly:
Repro document:
Assuming I understood you correctly:
tintoy
commented
7 years ago From what I can see, the problem does look like the namespace is not propagated correctly. Whether that's worth fixing for 2.0 I have no idea.
I guess I just don't understand what the workaround listed at the end of the repro is trying to accomplish, unless it's to prove that the signature is valid but cannot be correctly correlated with the containing document's canonicalised nodes (by showing that the only way to get their example to validate is by validating the signature by itself, standalone).
tintoy
commented
7 years ago 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).
tintoy
commented
7 years ago 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.
tintoy
commented
7 years ago @anthonylangsworth - do you have an opinion here about what constitutes correct behaviour?
krwq
commented
7 years ago @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).
karelz
commented
7 years ago @krwq agreed, thanks for summary and thanks @tintoy for your great analysis!
leopignataro
commented
7 years ago 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.
we never claim bad document is good
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.
From memory there are 2 or 3 kinds of XML-DSIG formats (attached, detached, etc). I wonder if this represents an edge case?
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").
So yes, the workaround does show that it's possible to validate the signature but only in isolation of the thing that was signed (which seems kinda pointless).
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.
leopignataro
commented
7 years ago 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 SignedXml class passing a XmlDocument causes the Utils.GetPropagatedAttributes() method to be called with the document root element, while calling the constructor passing the signature element causes the Utils.GetPropagatedAttributes() method to be called with the signature element itself.
This yields different results if namespace prefixes are added on elements between (but excluding) the root document and (including) the signature element:
A
+- B
+- C*
+- D
+- E*
+- F
+- Signature*Any namespace prefixes added on the elements marked above would cause differences on the behavior of the GetC14NDigest() method
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 SignedXml class. To make things worse, these constructors may be called both when creating and when validating signatures.
We think that the desired effects for each case would be:
1) During signature creation
a) Calling SignedXml(XmlDocument) means the signature will be inserted on the given document. Only "global" namespace prefixes (declared on the root element) will be taken into account on the canonicalization of the SignedInfo. This works well if the signature element will be inserted as a child of the root document, which is arguably the most common case.
b) Calling SignedXml(XmlElement) means the signature will be inserted on the document that owns the given element, not as a child of the root element but as a child of the given element. All namespace prefixes inherited on that "context" will be taken into account on the canonicalization of the SignedInfo.
2) During signature validation
a) Calling SignedXml(XmlDocument) means the signature is on the given document, and will later be given on the LoadXml() element
b) Calling SignedXml(XmlElement) means the signature is the given element (and perhaps calling LoadXml() is a bit redundant, but that's another matter)
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 LoadXml() method.
Therefore, we'd like to propose a fix consisting of changing the LoadXml() method by removing the lines commented out below:
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 LoadXml() method is called, override what may be initially have been considered as the "context" when the class was instantiated.
leopignataro
commented
7 years ago @karelz , we talked on dotnet/runtime#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?
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.