Add System.Security.Cryptography.Xml.SignedXml class

[ghost]

Execution plan

Goal: Provide APIs fully compatible with full/Desktop .NET Framework (no changes as part of this work - straight port only)

Plan:

• [x] 1. Add the source code on GH sanitized, with licenses, etc. (it won't build)
• [x] 2. Make the source code build (still excluded from overall repo build)
• [x] 3. Remove Desktop registry-compat code paths
  • Remove methods that have [RegistryPermission] (Utils and SignedXml classes) along with any owned methods
  • Address any other registry-related compile errors such as those using Registry, RegistryPermission, RegistryKey, etc (delete code as necessary)
• [x] 4. Add tests (We have to agree on expected code coverage and how much of the spec we have to cover)
  • Compare test results between Desktop and .NET Core implementations
• [x] 5. Make it part of overall repo build & ship it!

Code changes rules: Only code changes absolutely necessary to make it build and compatible with (full) .NET Framework are allowed, no additional bug fixes or changing architecture - such PRs will be rejected now. Changes like that can be considered after the initial port is done, when we have a good test bed and when we are able to validate such change.

If larger code/architecture changes are needed from some reason, we should discuss them first here, before doing the work / submitting PR.

If you work on some parts of the plan, please say so in the discussion to avoid duplicated work. We (@steveharter @karelz) will co-assign the issue to you.

---

Original

The class to digitally sign XML needs to be added.

[rschiefer]

As referenced by Tratcher, this is a blocker for adding support for WsFederation/ADFS in ASP.NET 5. We use ADFS extensively for many enterprise ASP.NET 4 applications. We are very interested in migrating to ASP.NET 5 and using WsFederation.

[bartonjs]

@rschiefer @Tratcher Well, it's... complicated.

• ADFS doesn't use System.Security.Cryptography.Xml.SignedXml; but instead its own implementation.
  • (That's largely from dusting off mental cobwebs and remembering back to being on that team for version 1 of ADFS)
• System.IdentityModel definitely doesn't use System.Security.Cryptography.Xml
  • (That's 'cuz their source code on referencesource says so :smile:)
• People don't really like System.Security.Cryptography.Xml.SignedXml because it's based on XmlDocument, which leads to some perf issues.
  • The ADFS version was based on XmlReader, IIRC.
• So, likely what people want is CoreFX to create a new implementation of SignedXml.
• But a new version of SignedXml won't compatible with the old version of SignedXml
• So some people might want us to keep the old version, too.
• So in all, people really want two versions.
• Except they don't want the old version.
• Except for when they do.

So we're left with the conundrum of:

• We can port that class that no one really wants
• Or, we can stop and design a brand new thing that probably no one will use, since they can't be compatible with anything else
• Or, to use the most effort and still not benefit anyone, do both :smile:.

@terrajobst - fyi

[bartonjs]

Apparently I was a bit rambly this morning. Sorry :).

We've definitely identified that there's work to be done here, but we don't think the right answer is to bring forward the existing System.Security.Cryptography.Xml code. Instead, this represents the item on our backlog to engineer a general-purposed implementation for XmlDSig which is fast and not tied to legacy object models (e.g. XmlDocument).

That effort isn't something that we're expecting to do for .NET Core 1.0, simply because we're focused on other things.

But, letting us know that you're impacted by the missing functionality does help with ongoing prioritization.

[AndersAbel]

I'm looking at creating an ASP.NET Core SAML2 Middleware, based on https://github.com/KentorIT/authservices. Without a SignedXml port it won't be able to run on the Core framework.

I definitely do agree that not carrying over the existing one is a good idea. Would it be possible to do anything based on an XmlReader API? That way both XDocument and XmlDocument can be supported. Also providing an implementation of the enveloped reader used in System.IdentityModel would be nice (if it was improved to support XML files with whitespace...)

[bartonjs]

@AndersAbel XmlReader is where I'd start, yeah (unless the XML guys tell me there's something better).

Since XmlReader what the System.IdentityModel variant is based on, it should be doable :).

[AndersAbel]

@bartonjs The System.IdentityModel variant is quite limited though in what transforms it can handle. For SAML2/WS-Fed work it wouldn't be a problem, but as a general API it must be considered how to deal with non-enveloped signatures and xml containing nested signatures (such as a signed saml response containing signed assertions). Also I think that the System.IdentityModel.EnvelopedSignatureReader is copying the data when doing the validation. There's a lot of fun to be done there. If I had the time I'd love to work on it.

[Icestorm0141]

I appreciate the rambling @bartonjs, helps to see whats going on behind the scenes.

Currently my company is impacted by this. We have some legacy code that we're trying to port over to .NET Core that generates signed XML license files and without this set of classes we're stuck. We're open to diverging from XML files as the base for the licenses but as of this moment we haven't found a good solution that fits our needs.

Looking forward to seeing this get added in the future.

[sandersaares]

I can attest that this (and also the slightly related XML Encryption) is relevant for us. The existing form in .NET Framework would be just fine - no need for innovation here, from my perspective. Copy & paste implementation would be very welcome!

Is there currently any workaround available?

[af0l]

Would like to know what @sandersaares asked, too. There's no built in way to sign xml in the CoreFX now?

[bartonjs]

@sandersaares / @af0l : There is not, for .NET Core 1.0, any inbuilt implementation of SignedXml/XmlDSig.

Based off of comments here (and others) we'll probably just bring over the old API, but we didn't have time to make it happen for 1.0.

[af0l]

Thank you @bartonjs, that must be the reason I could not get our project to work on Core. :) It's also a great shame, because I'd love to move ahead and can't until it's done. We have to report all payments to our tax authority using signed xml's, so it's a showstopper really.

[emailtowalter]

Any progress on this? I am stuck with the SAML token validation which requires this feature. Thanks

[af0l]

Yeah, would liek to know that too. We ended up extracting the features needing the signature and putting them into a separate web API solution...

[john182]

Already have idea on which version will be implemented or solution

[bartonjs]

At this point it seems that the most straightforward answer is to port the existing .NET Framework implementation to .NET Core. So we're bucketing this in with other "making it hard to port" API omissions.

[sandersaares]

Potentially relevant to the topic: https://connect.microsoft.com/VisualStudio/feedback/details/3002812/xmldsigc14ntransform-incorrectly-strips-whitespace-and-does-it-inconsistently and https://github.com/sandersaares/xml-c14n-whitespace-defect. It would seem to me that the .NET Framework implementation of Canonical XML 1.0 is incorrect. I hope I am wrong but if so, this might introduce some hairy compatibility questions.

[AndersAbel]

@sandersaares Looked at your sample and you need to set XmlDocument.PreserveWhiteSpace = true when reading the Xml if it contains whitespace.

[sandersaares]

@AndersAbel Thanks for the hint! That changes the situation and actually enables conforming validation if an XML Schema is present. Without an XML Schema, behavior remains invalid (in a new and exciting way). I have updated the Connect issue and GitHub repo accordingly.

[AndersAbel]

@bartonjs If you port the existing .NET framework solution over, please fix the bug that prevents verification of multiple signatures: https://connect.microsoft.com/VisualStudio/Feedback/Details/2288620

[sandersaares]

FYI if this gets to implementation stage then I have a freshly minted library here, with tests, that makes use of XML signatures (both sign and verify) and other XML functionality on .NET Framework - might be useful to get some dependency-free real-world code to try out the implementation against: https://github.com/Axinom/cpix

[henkmollema]

Is there a timeline for development of this API?

//cc @bartonjs

[bartonjs]

@henkmollema Nothing specific, no. For the 1.2 release we're working to shrink the gap between .NET Framework and .NET Core; and that effort is currently where SignedXml is coming from.

[brockallen]

I was on a customer call today that needs SAML2-P support on ASP.NET Core (which would be using KentorIT's implementation). This is a blocking issue for customers that want to move to ASP.NET Core. For now my customer will have to stay on Katana.

[brockallen]

I see the milestone was changed from 1.2 to Future (by @bartonjs). Can you comment or elaborate?

[bartonjs]

Mainly it just has to do with how we're tracking milestones. We used to do more of "we hope it'll make this one", then at the end of the milestone we'd re-assign everything that wasn't done. We're now trying really hard to say "if we marked it for this milestone it should be very rare for it to get reassigned".

This is downstream of a lot of other work for the 1.2 milestone, and it was (to me, anyways) always a bit of a stretch for it to make it for 1.2. It's still pretty high up on our "what's next" list, we're just not 'committing' to it being part of the 1.2 release (which is mainly netstandard2.0 work, bugfixing, and a couple of infrastructure projects).

Marking it as Future doesn't guarantee that it won't be part of the 1.2 release, it just means (using our current interpretation of milestone) that we won't hold the release for it. Once someone is actually working on it then there's a better understanding of when it'll actually get done, and then it'll get marked as whatever milestone it'll actually release in.

@karelz If there's anything you want to add (or correct) feel welcome to chime in.

[karelz]

We won't be able to fund the work in 1.2 timeframe (there's just too much other stuff with higher impact to finish). that's why we moved it to Future milestone, to communicate our plans. We are aware of the number of asks, that's why it is high in our Security area backlog. It is also one of the top asked corefx missing APIs in general (among DirectoryServices, SerialPort, etc.).

cc: @steveharter @danmosemsft @terrajobst

[karelz]

Our answers crossed :) More context on Milestones is in our issue guide.

The .NET Framework code is available as reference source. So technically speaking the port can be initiated even outside of .NET Core team - if folks are interested to help us here. From my earlier chats with @bartonjs I think the key "challenge" will be creating/porting tests.

[Jaedson33]

Hey, what's the actual situation about that problem?

[karelz]

@Jaedson33 what do you mean by 'problem'? If you mean when it will be fixed -- see the answers from last week.

[Jaedson33]

@karelz But I don't want to wait. Why you don't fix it now?

[karelz]

@Jaedson33 see my reply above - it explains why we cannot fund it now. It is about priorities. There is a line of people who want many features/APIs now, but we do not have infinite-size team, so we have to prioritize.

If you need it really badly ASAP, you can always contribute to the code base, we will be happy to help out with guidance, code reviews and feedback.

[Jaedson33]

Ok, so I'll wait.

[tintoy]

@karelz if the original tests are still available to verify the work, I'd be willing to put my hand up :)

(one of my coworkers also has relevant experience so we'd probably be working on it together)

[karelz]

Key part of the work is actually to create new test assets. The old tests are insufficient and have poor coverage. We will need someone to review the spec and add test for every interesting requirement -- that's where most of the cost is.

If you are still interested, we can try to drop in source code from full .NET Framework as is - the next step will be to get it built and add test coverage, before it can be released as part of .NET Core. Let me know if you're interested ...

[tintoy]

Ok, yes - we're still interested :)

[Jaedson33]

@tintoy I'm interested to help you because I really need that class.

[tintoy]

@tintoy I'm interested to help you because I really need that class.

Glad to hear it :)

[Jaedson33]

So... How can I help? Obs: It's the first time I'm using GitHub.

[tintoy]

So... How can I help?

Let me first have a chat to my coworker and we'll come up with a plan of attack. @karelz - are there any guidelines or other docs we should read before wading in? To start with, I'm guessing my coworker will probably wade into the standard, I'll probably look at working out where the code needs to go (and what's involved in getting existing tests from other parts of the framework to run before we make any changes). Does that sound reasonable?

CC: @anthonylangsworth

[AndersAbel]

To keep the scope a bit limited I'd recommend to start without the features that are disabled by MS16-035 (xpath transform, xslt transform, external references). I don't know what room there is for breaking changes, but the current fallback mechanism in DefaultGetIdElement can be exploited in signature wrapping attacks. I'd prefer a more secure default version.

It would also be good if the internal API was restructured a bit to support the EnvelopedSignatureReader used by System.IdentityModel instead of having two separate implementations of XML Signature validation.

Finally I'd also like a single dot to be added as per this bug report.

[karelz]

@tintoy I don't think we have good docs. I think we should add the sources and then we can parallelize the work - let me sync with @bartonjs @steveharter @ianhays.

[AndersAbel]

I'll try to find some time to help too. If there are any questions on the spec and how it works I'll be happy to look into that - I've already spent some time reviewing the spec.

Does anyone has anything to say about the idea to consolidate SignedXml and the EnvelopedSignatureReader used by System.IdentityModel?

[karelz]

@AndersAbel

start without the features that are disabled by MS16-035 (xpath transform, xslt transform, external references)

We should start with latest .NET Framework source code, which should be secure. If you have any concerns about .NET Framework code security, please let us know offline.

I don't know what room there is for breaking changes

No room, we should start with straightforward port from .NET Framework. Any further improvements, changes, breaking changes, etc. can be considered later. Not as part of the initial work. Otherwise it will grow over our heads.

current fallback mechanism in DefaultGetIdElement can be exploited in signature wrapping attacks

That should be treated as separate problem. @bartonjs can you please comment?

It would also be good if the internal API was restructured a bit to support the EnvelopedSignatureReader used by System.IdentityModel instead of having two separate implementations of XML Signature validation.

Again, let's take it as next step, after we have fully functional port with good test coverage.

Finally I'd also like a single dot to be added as per this bug report.

Please file it as separate issue here, on GitHub. Ideally after we have the code here ported (i.e. when the bug is truly applicable to .NET Core).

Does anyone has anything to say about the idea to consolidate SignedXml and the EnvelopedSignatureReader used by System.IdentityModel?

Just want to reiterate. We should treat it as next step, post porting.

[bartonjs]

current fallback mechanism in DefaultGetIdElement can be exploited in signature wrapping attacks

That should be treated as separate problem. @bartonjs can you please comment?

@AndersAbel If you believe there is a security concern, please follow the vulnerability reporting process at https://technet.microsoft.com/en-us/security/ff852094.aspx.

Does anyone has anything to say about the idea to consolidate SignedXml and the EnvelopedSignatureReader used by System.IdentityModel?.

It likely isn't possible. SignedXml is very heavily (and public-API-ally) based on the rich-DOM XmlDocument. IdentityModel's representation is based on XmlReader. But, once the existing stuff is brought over it can be investigated.

[tintoy]

I'll try to find some time to help too. If there are any questions on the spec and how it works I'll be happy to look into that - I've already spent some time reviewing the spec.

@AndersAbel - cheers, I'm sure we could use the help :)

[AndersAbel]

@bartonjs I've reported those issues to secure@microsoft.com, which resulted in MS16-035. IMO there are some remaining risky issues though, that MS decided to not fix (they would incur breaking changes). I've not yet published the details, but if you would like to discuss them privately, please mail me.

@karelz Thanks for clearing up that there's no room for breaking changes. That means that my ideas about consolidation are not relevant.

start without the features that are disabled by MS16-035 (xpath transform, xslt transform, external references)

We should start with latest .NET Framework source code, which should be secure. If you have any concerns about .NET Framework code security, please let us know offline.

The MS16-035 patch addressed a number of issues in SignedXml. It is possible though to use registry keys to revert to the old, unsecure behaviour. Should those options also be ported to .NET Core? My suggestion above was meant to prioritise getting the current default .NET Framework behaviour ported, leaving those parts that are deactivated by default behind for now. Or do you mean that those parts are crucial to be moved over too? Then there's the question how to handle the configuration as .NET Core AFAIK doesn't rely on the registry for configuration (as it's not available on all platforms).

[bartonjs]

It is possible though to use registry keys to revert to the old, unsecure behaviour. Should those options also be ported to .NET Core?

No. Registry-compat-only code will be deleted before the package is made available.

[Jaedson33]

Why you don't create a project on GitHub to implement that?

[karelz]

We synced with @bartonjs, @steveharter and @ianhays

EDIT: Execution plan moved to top most post.

[tintoy]

Sounds good to me :)