Azure-activedirectory-identitymodel-extensions-for-dotnet: Feature request: Allow nested Signatures

[Tarig0]

Looks like the only reason why signatures, that include another signature in its calculation, are not supported is because the XML token stream will take out ALL signatures and not just the first instance of it.

I think, with the assumption that the first signature is the one being verified, an element remove flag could be added and if true include any other signature elements.

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/6e7a53e241e4566998d3bf365f03acd0da699a31/src/Microsoft.IdentityModel.Xml/XmlTokenStreamWriter.cs#L158-L160

[Tarig0]

Looks like this feature was removed at
commit 8cd95c84f1e833fcdfbcb54b56bd8dadb858e1c1

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/commit/8cd95c84f1e833fcdfbcb54b56bd8dadb858e1c1#diff-913b807422bc2f47291aa2bdc533be7eL68

[brentschmaltz]

@Tarig0 yes, we deliberately only support a single signature as we were focused on Saml tokens. I agree that xmldsig has the ability for multiple signatures. Can you share why you need this support?

[Tarig0]

I'm trying to extend support for samlp. Currently I could use signedxml, only need it for the protocol level signature, since that is now part of core but I was hoping to keep it out.

[brentschmaltz]

@Tarig0 What are you trying to keep out? Sorry it is not clear.

[Tarig0]

Trying to avoid taking on the following dependency

https://www.nuget.org/packages/System.Security.Cryptography.Xml/

The only reason why I need it is to validate the signature of the SAMLP response

[Tarig0]

Did my last comment clear up what I meant?

[brentschmaltz]

@Tarig0 it was simply a work scoping tactic to allow a single signature. The code was developed to extend to multiple.
What is your time frame?

I think this fits with https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/759

[Tarig0]

I am in exploratory/learning phase so no formal timeline. I agree that this request is covered by the work required for #759

[AndersAbel]

@Tarig0 I'm afraid nested signatures are more complex than merely taking the first. There is a horrible class attacks on SAML2 implemenations that use XML Signature Wrapping in various creative ways. The short-cut the team has taken so far with only one signature and hard coding it to one reference which is hard coded to be the entire document is safe. But as soon as those assumptions are no longer valid, I think that steps must be taken to ensure that the right signature (exact location in XML doc must follow SAML2 spec) is validated and that it contain exactly one reference, which points to the right element (once again according to the SAML2 spec). The filtering out of the signature itself is done by the enveloped signature transform, which then should be used to ensure that the right signature block is excluded.

Sorry if I'm a bit pessimistic, but in my work with SAML2P I've found out that XML signatures are hard. Not just to implement, but even to use by calling the APIs in a secure way.

[Tarig0]

The assumptions that I have is.

1. The reference is for a node that the signature is a direct child of.
2. The xmlreader that is wrapped will be windowed to said node.
   --Added
3. There is no sibling signatures

Little more fleshed out

1. I have a signed Response with a signed Assertion
2. I open a wrappedxmlreader on the doc (this will validate the response signature and include the assertion's signature)
3. when I reach the assertion another wrappedxmlreader is created, this will validate the assertion's signature, and have no knowledge of the wrapping protocol
4. finish reading assertion validate signature for assertion.
5. finish reading response and finalize validation

[brentschmaltz]

@AndersAbel yes, when we wrote WS-Security for WCF that has multiple signatures it was tricky.

Having multiple references is not as hard a problem.
@Tarig0 yes, embedded signatures where the inner is simply data to the outer AND the reading is strict (signature follows the node) should be fine.

[Tarig0]

I'm not sure I understand the meaning of "signature follows the node".

Does that mean that you cant have the following?

• Response
  
  
  
  • Assertion
  
  
  • Signature (on assertion)
  
  
  • Signature (on response)
  
  

[AndersAbel]

@Tarig0 Yes, the structure you show is valid. Although the ordering is different, the response signature must be before the assertion:

• Response
  
  
  
  • Response issuer
  
  
  • Response signature
  
  
  • Assertion
  
  
  • Assertion issuer
  
  
  • Assertion signature
  
  

I talked to @brentschmaltz yesterday and we discussed this issue (and some others). Our suggestion is to change the EnvelopedSignatureReader so that it preserves the ID of the initial node, and then checks each Signature to see if it contains a single reference to the ID. Any Signatures not matchin will be kept as part of the data. That will solve the multiple signature issue.

[brentschmaltz]

@Tarig0 @AndersAbel We could allow for the Response signature to be after the assertions.
We would need logic to ensure ID match AND there is only one such ID.

[AndersAbel]

@brentschmaltz Yes, my bad - I'm too stuck into the SAML2 world where the signature is always right after the issuer. You said that the same isn't true for WS-Fed.

[brentschmaltz]

@AndersAbel I'll lobby to get resources to put this in the next release.

[AndersAbel]

I did some code reading on this (and checked the spec).

The first problem to solve is to identity the right Signature to set as EnvelopedSignatureReader.Signature. This can be done by checking the reference. Which can only be done after it has been read. So if the <ds:Signature> node is read and it turns out it's the wrong one, the reader must somehow be rewinded to output the <ds:Signature> node. For SAML2, it could be acceptable to filter out inner signatures in most cases, but sometimes they are relevant (such as when an assertion in a response is signed by someone else than the response creator).

The second problem is the EnvelopedSignatureTransform that right now filters out any signature node. That will obviously not work as an outer signature will also protect an inner signature. This is tricky, as there is no requirement for a <ds:Signature> element to have an ID. So somehow when a signature node is encountered in the XmlStream, it must be possible to check whether it's the right one to filter out or not. One idea is to save a reference to the XmlTokenEntry for the <ds:Signature> in the Transform and then do a reference comparison when processing/filtering.

[AndersAbel]

Looking at the job needed to extend the EnvelopedSignatureReader to allow for nested signatures, it feels more and more like reinventing a complete SignedXml implementation. And there already is one. What is the reason for having a separate implementation here, instead of relying on System.Security.Cryptography.Xml.SignedXml?

[brentschmaltz]

@AndersAbel couple of reasons for xmlreaders.

1. using xmldocuments was found to be quite a bit slower
2. validation of schema can be written into the readers and failures can have crisp messages
3. external generation and validation, using services, of signatures is natural using readers
4. we are not providing a 'general' DSig library, but a fast token specific reader

[AndersAbel]

@brentschmaltz Thanks for the explanations

we are not providing a 'general' DSig library, but a fast token specific reader

If we focus on what can actually happen in the SAML world, I've done some digging in the SAML2 specs and there's some good news: The relevant signature is always the first one in the token stream. The signature covering the root node is required to be before any contained data that can be signed. A abbreviated example:

<Response ID="R"> <Signature><Reference URI="#R"></Signature> <Assertion ID="A"> <Signature><Reference URI="A"></Signature> </Assertion> </Response>

If validating the signature of the Response, the EnvelopedSignatureReader token stream will start at the <Response> element. If validating the signature of the assertion, the EnvelopedSignatureReader token stream will start at the <Assertion> element.

So a flag in XmlTokenStreamWriter.WriteTo that is set when an element has been excluded and ensures only one element is excluded would do.

This is at least true in the SAML2/SAML2P world. But what about WS-Fed? If I recall what you've said previously there is never more than one signature in WS-Fed?

[brentschmaltz]

@AndersAbel for general WS-Security signatures, the signature could be anywhere (almost) in the SOAP envelope and contain multiple parts. That's what I mean by 'general' DSig. These XML libraries 'could' be extended to provide multiple signatures over multiple parts of the envelope.

[AndersAbel]

@brentschmaltz Trying to solve one problem at a time - would you accept a PR that implements a flag like I suggested to only filter out the first signature? It would become SAML2/SAML2P compatible, and would not break anything existing.

[brentschmaltz]

@AndersAbel in general I am not a big fan of flags. I prefer a policy strategy.
Are you thinking on the DSigSerializer or EnvelopeSignatureReader?

I feel this feature should be easily to fit into the next release, whatever form it takes.
We are shooting for 11/19 (was stated as 10/19, but that was a typo).

[AndersAbel]

It's not a flag for the user - it's an internal flag in the processing that marks when one signature has been excluded and then doesn't exclude the rest. I'll try to write up a PR including tests in the next few days so you can see what it looks like.

[mafurman]

Related issue: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1042

[brentschmaltz]

Added with: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/1181
We still have work todo to ensure the InclusivePrefixes is correct. See https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1184