Multiple enveloped signatures #67010

AndersAbel · 2022-03-22T21:22:40Z

Handle multiple enveloped XML signatures in the same document. This is fairly common for Saml2 implementations.

ghost · 2022-03-22T21:22:48Z

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Handle multiple enveloped XML signatures in the same document. This is fairly common for Saml2 implementations.

Author:	AndersAbel
Assignees:	-
Labels:	`area-System.Security`, `community-contribution`
Milestone:	-

src/libraries/System.Security.Cryptography.Xml/tests/SignedXmlTest.cs

...libraries/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/Reference.cs

AndersAbel · 2022-03-22T21:40:35Z

@bartonjs @krwq Before doing too much work on review, please note that this is still marked as work in progress. I failed to get the complete tests running in my environment, so I pushed to get a CI build. I would be surprised if it passed the build on first try.

danmoseley · 2022-03-23T02:50:31Z

@AndersAbel What issue did you have running tests locally? It should work 🙂

AndersAbel · 2022-03-23T08:07:36Z

@AndersAbel What issue did you have running tests locally? It should work 🙂

@danmoseley I didn't read the instructions on setting up the environment well enough. Once I found my way to https://github.com/dotnet/runtime/blob/main/docs/workflow/building/libraries/README.md it got clearer and it now works.

AndersAbel · 2022-03-23T11:58:44Z

I might need some help here on the build and what is expected to pass or not.
The tests failed on the .NET Framework target, so I added some more diagnostics using reflection. That shows that the updated code with the bug fix is not run on .NET framework. Now also the entire test run fails on .NET 7, I don't know if that is due to reflection usage or a temporary issue.

On the .NET Framework target, is the .NET Framework version of SignedXml used? In that case it is expected to fail as that bug was present already in the .NET Framework version of SignedXml. Should I exclude those tests for the .NET Framework target?

I need some feedback on how to proceed.

Once the failure are sorted out I'll remove the reflection again before merge. I can leave them commented out if anyone needs them in future troubleshooting, but I don't think reflection belongs in the live test harness.

krwq · 2022-03-23T13:08:59Z

@AndersAbel the easiest way to find out which dll you're using is under the debugger (in VS open the Modules window). Simply put [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] on the test (optionally put an explanation on the attribute with perhaps issue number or some extra info). We already do that in multiple places, i.e.:

runtime/src/libraries/System.Security.Cryptography.Xml/tests/TransformTest.cs

Line 82 in 57bfe47

[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]

AndersAbel · 2022-03-23T13:58:28Z

@AndersAbel the easiest way to find out which dll you're using is under the debugger (in VS open the Modules window). Simply put [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] on the test (optionally put an explanation on the attribute with perhaps issue number or some extra info). We already do that in multiple places, i.e.:

runtime/src/libraries/System.Security.Cryptography.Xml/tests/TransformTest.cs

Line 82 in 57bfe47

[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]

Thanks. That's the reason. I'll comment out the reflection stuff and make the PR ready for final review.

AndersAbel · 2022-03-23T20:13:07Z

Looks like the build failed due to some infrastructure issues and that it is now unavailable. The contents are ready for review.

AndersAbel · 2022-03-25T08:26:19Z

/azp run

AndersAbel · 2022-03-25T08:34:08Z

@krwq @bartonjs Looks like I cannot get the build to rerun. Can you please have a look?

danmoseley · 2022-03-25T13:07:54Z

I see it running?
Btw the trick to force validation to rerun is to close and reopen the PR.

AndersAbel · 2022-03-25T14:54:54Z

@danmoseley That's a stale run that has been trying to complete for 2 days... Thanks for the trick, I'll close and reopen.

krwq · 2022-03-25T18:32:08Z

...libraries/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/Reference.cs

@@ -260,12 +260,20 @@ public void LoadXml(XmlElement value!!)
                        // let the transform read the children of the transformElement for data
                        transform.LoadInnerXml(transformElement.ChildNodes);
                        // Hack! this is done to get around the lack of here() function support in XPath
-                        if (transform is XmlDsigEnvelopedSignatureTransform)
+                        if (transform is XmlDsigEnvelopedSignatureTransform
+                            && _uri != null && (_uri.Length == 0 || _uri[0] == '#'))


should we throw if _uri somehow ends up null?

Null means "whole document"... if I recall correctly?

No, I don't think so. Null means "custom data source application or first transform should now about". In that case an enveloped signature transform makes no sense and should become a no-op. It is not illegal to include an enveloped signature transform where it is not needed. So with null, I think the reasonable thing to do is to just ignore.
@bartonjs an empty Uri means whole document, and that is taken care of in the code.

krwq · 2022-03-25T18:35:20Z

src/libraries/System.Security.Cryptography.Xml/tests/SignedXmlTest.cs

+            //void Main()
+            //{
+            //	var xml = "<root><x ID=\"a\"/><x ID=\"b\"><x ID=\"c\"><x ID=\"y\"/></x></x></root>";
+            //
+            //	var xd = new XmlDocument();
+            //	xd.LoadXml(xml);
+            //
+            //	Sign(xd, "c", "y");
+            //	Sign(xd, "b", "b");
+            //	Sign(xd, "a", "a");
+            //}
+            //
+            //void Sign(XmlDocument xd, string id, string signaturePlacement)
+            //{
+            //	var sx = new SignedXml(xd);
+            //	var key = RSA.Create();
+            //	sx.SigningKey = key;
+            //
+            //	var reference = new Reference("#" + id);
+            //	reference.AddTransform(new XmlDsigEnvelopedSignatureTransform());
+            //	reference.AddTransform(new XmlDsigExcC14NTransform());
+            //	sx.AddReference(reference);
+            //
+            //	sx.ComputeSignature();
+            //	sx.KeyInfo.AddClause(new RSAKeyValue(key));
+            //
+            //	var node = xd.SelectSingleNode("//x[@ID=\'" + signaturePlacement + "']");
+            //
+            //	var signatureElement = sx.GetXml();
+            //
+            //	node.AppendChild(sx.GetXml());
+            //}


Why not calculate it once and assign in a static field?

I think I prefer the approach of using the pregenerated value. Makes it easier to understand what's going on, and is more like a conformance test.

I agree with @bartonjs (Because he agrees with me ;) I think that the pregenerated value as it is makes it easier to read the code and understand the test. Including the generation code as a comment is a convenience to any future programmer that want to alter the test case, so they can easily reproduce the current generated data.

krwq · 2022-03-25T18:36:54Z

...libraries/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/Reference.cs

+                            XmlNode referenceTarget =
+                                _uri.Length == 0
+                                ? transformElement.OwnerDocument
+                                : SignedXml.GetIdElement(transformElement.OwnerDocument, Utils.GetIdFromLocalUri(_uri, out bool _));


we should also test for case where reference is incorrect (i.e. someone tampered with the doc)

True. While the behavior we're fixing is "we always return failure in this case", let's make sure that we haven't turned complex nesting into so now it "always succeeds, regardless of signature correctness"

This fix is about the loading of the signature data and not the actual validation. While the test coverage can certainly be improved, I think that is a different task. I do have some validation tests in https://github.com/Sustainsys/Saml2/blob/c466d3bbf12479944e350b649f5b5c825616007b/Tests/Tests.Shared/XmlHelpersTests.cs#L171 that I could carry over. Right now they are testing my wrapper around SignedXml, but essentially they are testing SignedXml.

This fix is about the loading of the signature data and not the actual validation.

Sure. But from a black-box perspective we now have a test that shows CheckSignature() can return true with complex nesting. Returning false when it should is arguably more important than returning true when it should, so we should have a test that ensures that this change (from a black box perspective) isn't creating a hole.

Essentially something like load the same document as the positive case, and via a [Theory] input change each of a/b/c/y (one per case) to add some extra attribute (or child node, whatever), and show that CheckSignature() returns false. It doesn't need to care about full matrix testing all of the algorithms, just that we haven't turned complex nesting from "never works" into "free pass".

Ok, I'll add that.

krwq · 2022-03-29T15:45:03Z

...libraries/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/Reference.cs

@@ -260,12 +260,20 @@ public void LoadXml(XmlElement value!!)
                        // let the transform read the children of the transformElement for data
                        transform.LoadInnerXml(transformElement.ChildNodes);
                        // Hack! this is done to get around the lack of here() function support in XPath
-                        if (transform is XmlDsigEnvelopedSignatureTransform)
+                        if (transform is XmlDsigEnvelopedSignatureTransform
+                            && _uri != null && (_uri.Length == 0 || _uri[0] == '#'))


make sure to cover case where _uri is null (I assume it's when Uri attribute is missing) or point to a test case covering it

The existing test LoadEnvelopedSignatureTransforms already tests the flow where _uri == null during loading.

Tests works locally, but not on build server. Temporarily using some dark reflection magic to check the internal index that the fix is about to see the values as run on the build server.

- Skip test on .NET Framework target as it will fail. - Comment out reflection-based tests. - Add comment with test data generation code for future coders to reference

- Test for empty reference URI, which implies whole document

- Positive and negative tests for tampered data in doc with multiple enveloped signatures

AndersAbel · 2022-03-31T18:08:22Z

I've added some more tests and I think I've now handled all the review comments. Please let me know if there is anything more you need to be able to merge this.

bartonjs · 2022-04-01T22:11:41Z

Thanks, @AndersAbel

krwq · 2022-04-02T10:20:49Z

src/libraries/System.Security.Cryptography.Xml/tests/SignedXmlTest.cs

+        public void CheckSignatureDetectsTamperedDataOnMultipleEnvelopedSignatures(
+            string signatureParent, string tamperNode, bool expected)
+        {
+            var tampered = multipleSignaturesXml.Replace($"ID=\"{tamperNode}\"", $"ID=\"{tamperNode}\" Hackerz=\"true\"");


I'm still missing a case like:

replace uri with some nonexisting ID

replace uri with some existing ID but different than original

Added test in #67501 and it actually caught a bug/changed behaviour that I also fixed in that PR.

krwq · 2022-04-02T10:24:33Z

src/libraries/System.Security.Cryptography.Xml/tests/SignedXmlTest.cs

+        [InlineData("a", "b", true)]
+        [InlineData("b", "b", false)]
+        [InlineData("b", "c", false)]
+        [InlineData("y", "b", true)]


why are some of these true?

Because the altered note is outside the validated content for the signature.

ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 22, 2022

dotnet-issue-labeler bot added the area-System.Security label Mar 22, 2022

bartonjs reviewed Mar 22, 2022

View reviewed changes

src/libraries/System.Security.Cryptography.Xml/tests/SignedXmlTest.cs Outdated Show resolved Hide resolved

bartonjs reviewed Mar 22, 2022

View reviewed changes

src/libraries/System.Security.Cryptography.Xml/tests/SignedXmlTest.cs Outdated Show resolved Hide resolved

bartonjs reviewed Mar 22, 2022

View reviewed changes

...libraries/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/Reference.cs Outdated Show resolved Hide resolved

bartonjs requested a review from krwq March 22, 2022 21:38

AndersAbel marked this pull request as ready for review March 23, 2022 14:18

AndersAbel closed this Mar 25, 2022

AndersAbel reopened this Mar 25, 2022

krwq reviewed Mar 25, 2022

View reviewed changes

krwq reviewed Mar 29, 2022

View reviewed changes

AndersAbel added 5 commits March 29, 2022 20:43

Test case for multiple enveloped signatures in document

47d1d5d

Fix for handling multiple enveloped xml signatures in the same document.

3c0aaee

Review fixes

ca51abf

Build server troubleshooting

4b1425f

Tests works locally, but not on build server. Temporarily using some dark reflection magic to check the internal index that the fix is about to see the values as run on the build server.

Cleanup before review

8697078

- Skip test on .NET Framework target as it will fail. - Comment out reflection-based tests. - Add comment with test data generation code for future coders to reference

AndersAbel force-pushed the MultipleEnvelopedSignatures branch from 365c6b9 to 8697078 Compare March 29, 2022 18:43

AndersAbel added 2 commits March 29, 2022 21:43

Improved tests

c0e5b64

- Test for empty reference URI, which implies whole document

Improved tests

8a6bbe8

- Positive and negative tests for tampered data in doc with multiple enveloped signatures

AndersAbel force-pushed the MultipleEnvelopedSignatures branch from 77d0289 to 8a6bbe8 Compare March 29, 2022 21:24

bartonjs approved these changes Apr 1, 2022

View reviewed changes

bartonjs merged commit 8bdc4e7 into dotnet:main Apr 1, 2022

krwq reviewed Apr 2, 2022

View reviewed changes

AndersAbel mentioned this pull request Apr 3, 2022

Tests for altered references for SignedXml #67501

Merged

ghost locked as resolved and limited conversation to collaborators May 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple enveloped signatures #67010

Multiple enveloped signatures #67010

AndersAbel commented Mar 22, 2022 •

edited

Loading

ghost commented Mar 22, 2022

AndersAbel commented Mar 22, 2022

danmoseley commented Mar 23, 2022

AndersAbel commented Mar 23, 2022

AndersAbel commented Mar 23, 2022

krwq commented Mar 23, 2022 •

edited

Loading

AndersAbel commented Mar 23, 2022

AndersAbel commented Mar 23, 2022 •

edited

Loading

AndersAbel commented Mar 25, 2022

AndersAbel commented Mar 25, 2022

danmoseley commented Mar 25, 2022

AndersAbel commented Mar 25, 2022

krwq Mar 25, 2022 •

edited

Loading

bartonjs Mar 25, 2022

AndersAbel Mar 28, 2022

krwq Mar 25, 2022

bartonjs Mar 25, 2022

AndersAbel Mar 28, 2022

krwq Mar 25, 2022

bartonjs Mar 25, 2022

AndersAbel Mar 28, 2022

bartonjs Mar 28, 2022

AndersAbel Mar 29, 2022

AndersAbel Mar 29, 2022

krwq Mar 29, 2022 •

edited

Loading

AndersAbel Mar 29, 2022

AndersAbel commented Mar 31, 2022

bartonjs commented Apr 1, 2022

krwq Apr 2, 2022 •

edited

Loading

AndersAbel Apr 3, 2022

krwq Apr 2, 2022

AndersAbel Apr 3, 2022

Multiple enveloped signatures #67010

Multiple enveloped signatures #67010

Conversation

AndersAbel commented Mar 22, 2022 • edited Loading

ghost commented Mar 22, 2022

AndersAbel commented Mar 22, 2022

danmoseley commented Mar 23, 2022

AndersAbel commented Mar 23, 2022

AndersAbel commented Mar 23, 2022

krwq commented Mar 23, 2022 • edited Loading

AndersAbel commented Mar 23, 2022

AndersAbel commented Mar 23, 2022 • edited Loading

AndersAbel commented Mar 25, 2022

AndersAbel commented Mar 25, 2022

danmoseley commented Mar 25, 2022

AndersAbel commented Mar 25, 2022

krwq Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndersAbel commented Mar 31, 2022

bartonjs commented Apr 1, 2022

krwq Apr 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndersAbel commented Mar 22, 2022 •

edited

Loading

krwq commented Mar 23, 2022 •

edited

Loading

AndersAbel commented Mar 23, 2022 •

edited

Loading

krwq Mar 25, 2022 •

edited

Loading

krwq Mar 29, 2022 •

edited

Loading

krwq Apr 2, 2022 •

edited

Loading