search

AzureAD / azure-activedirectory-identitymodel-extensions-for-dotnet

IdentityModel extensions for .Net
MIT License
1.06k stars 401 forks source link

Feature request: Allow nested Signatures #1012

Closed Tarig0 closed 5 years ago

Tarig0 commented 6 years ago

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 commented 6 years ago

Looks like this feature was removed at commit 8cd95c84f1e833fcdfbcb54b56bd8dadb858e1c1

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

brentschmaltz commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

Tarig0 commented 6 years ago

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 commented 6 years ago

Did my last comment clear up what I meant?

brentschmaltz commented 6 years ago

@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 commented 6 years ago

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

AndersAbel commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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

Does that mean that you cant have the following?

AndersAbel commented 6 years ago

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

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

AndersAbel commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 5 years ago

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

brentschmaltz commented 5 years ago

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

Ghostbird commented 7 months ago

Sorry to resurrect such an old issue, but this seemed like the most appropriate place, considering this is what you find when you search for the IDX30019 error message.

I've received such a Saml2 token, but when I call ReadSaml2Token on the xml string, it throws IDX30019. How do I actually indicate that I do not want it to throw on nested signatures?

My structure of my SAML is:

<saml:Assertion xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">
  <saml:Issuer>…</saml:Issuer>
  <dsig:Signature xmlns:dsig="http://www.w3.org/2000/09/xmldsig#">
    <dsig:SignedInfo>
      <dsig:CanonicalizationMethod/>
      <dsig:SignatureMethod/>
      <dsig:Reference>
        <dsig:Transforms>
          <dsig:Transform/>
          <dsig:Transform/>
        </dsig:Transforms>
        <dsig:DigestMethod/>
        <dsig:DigestValue>…</dsig:DigestValue>
      </dsig:Reference>
    </dsig:SignedInfo>
    <dsig:SignatureValue>…</dsig:SignatureValue>
    <dsig:KeyInfo>
      <dsig:KeyName>…</dsig:KeyName>
    </dsig:KeyInfo>
  </dsig:Signature>
  <saml:Subject>
    <saml:NameID>…</saml:NameID>
    <saml:SubjectConfirmation>
      <saml:SubjectConfirmationData/>
    </saml:SubjectConfirmation>
  </saml:Subject>
  <saml:Conditions>
    <saml:AudienceRestriction>
      <saml:Audience>…</saml:Audience>
      <saml:Audience>…</saml:Audience>
    </saml:AudienceRestriction>
  </saml:Conditions>
  <saml:Advice>
    <saml2:Assertion xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion">
      <saml2:Issuer>…</saml2:Issuer>
      <ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
        <ds:SignedInfo>
          <ds:CanonicalizationMethod/>
          <ds:SignatureMethod/>
          <ds:Reference>
            <ds:Transforms>
              <ds:Transform/>
              <ds:Transform/>
            </ds:Transforms>
            <ds:DigestMethod/>
            <ds:DigestValue>…</ds:DigestValue>
          </ds:Reference>
        </ds:SignedInfo>
        <ds:SignatureValue>…</ds:SignatureValue>
        <ds:KeyInfo>
          <ds:KeyName>…</ds:KeyName>
        </ds:KeyInfo>
      </ds:Signature>
      <saml2:Subject>
        <saml2:NameID>…</saml2:NameID>
        <saml2:SubjectConfirmation>
          <saml2:SubjectConfirmationData/>
        </saml2:SubjectConfirmation>
      </saml2:Subject>
      <saml2:Conditions>
        <saml2:AudienceRestriction>
          <saml2:Audience>…</saml2:Audience>
        </saml2:AudienceRestriction>
      </saml2:Conditions>
      <saml2:AuthnStatement>
        <saml2:AuthnContext>
          <saml2:AuthnContextClassRef>…</saml2:AuthnContextClassRef>
        </saml2:AuthnContext>
      </saml2:AuthnStatement>
      <saml2:AttributeStatement>
        <saml2:Attribute>
          <saml2:AttributeValue>…</saml2:AttributeValue>
        </saml2:Attribute>
        <saml2:Attribute>
          <saml2:AttributeValue>
            <saml2:EncryptedID>
              <xenc:EncryptedData xmlns:xenc="http://www.w3.org/2001/04/xmlenc#">
                <xenc:EncryptionMethod/>
                <ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
                  <ds:RetrievalMethod/>
                </ds:KeyInfo>
                <xenc:CipherData>
                  <xenc:CipherValue>…</xenc:CipherValue>
                </xenc:CipherData>
              </xenc:EncryptedData>
              <xenc:EncryptedKey xmlns:xenc="http://www.w3.org/2001/04/xmlenc#">
                <xenc:EncryptionMethod>
                  <ds:DigestMethod xmlns:ds="http://www.w3.org/2000/09/xmldsig#"/>
                </xenc:EncryptionMethod>
                <ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
                  <ds:KeyName>…</ds:KeyName>
                </ds:KeyInfo>
                <xenc:CipherData>
                  <xenc:CipherValue>…</xenc:CipherValue>
                </xenc:CipherData>
                <xenc:ReferenceList>
                  <xenc:DataReference/>
                </xenc:ReferenceList>
              </xenc:EncryptedKey>
            </saml2:EncryptedID>
          </saml2:AttributeValue>
        </saml2:Attribute>
      </saml2:AttributeStatement>
    </saml2:Assertion>
  </saml:Advice>
  <saml:AuthnStatement>
    <saml:AuthnContext>
      <saml:AuthnContextClassRef>…</saml:AuthnContextClassRef>
      <saml:AuthenticatingAuthority>…</saml:AuthenticatingAuthority>
    </saml:AuthnContext>
  </saml:AuthnStatement>
  <saml:AttributeStatement xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xmlns:xs="http://www.w3.org/2001/XMLSchema">
    <saml:Attribute>
      <saml:AttributeValue>
        <saml2:EncryptedID xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion">
          <xenc:EncryptedData xmlns:xenc="http://www.w3.org/2001/04/xmlenc#">
            <xenc:EncryptionMethod/>
            <ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
              <ds:RetrievalMethod/>
            </ds:KeyInfo>
            <xenc:CipherData>
              <xenc:CipherValue>…</xenc:CipherValue>
            </xenc:CipherData>
          </xenc:EncryptedData>
          <xenc:EncryptedKey xmlns:xenc="http://www.w3.org/2001/04/xmlenc#">
            <xenc:EncryptionMethod>
              <ds:DigestMethod xmlns:ds="http://www.w3.org/2000/09/xmldsig#"/>
            </xenc:EncryptionMethod>
            <ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
              <ds:KeyName>…</ds:KeyName>
            </ds:KeyInfo>
            <xenc:CipherData>
              <xenc:CipherValue>…</xenc:CipherValue>
            </xenc:CipherData>
            <xenc:ReferenceList>
              <xenc:DataReference/>
            </xenc:ReferenceList>
          </xenc:EncryptedKey>
        </saml2:EncryptedID>
      </saml:AttributeValue>
    </saml:Attribute>
    <saml:Attribute>
      <saml:AttributeValue xsi:type="xs:string">…</saml:AttributeValue>
    </saml:Attribute>
  </saml:AttributeStatement>
</saml:Assertion>