Handle encrypt-then-sign with encrypted Assertions

[h-bragg]

Problem

We had an issue where a Response with a signature and EncryptedAssertion's was failing in the change to the verifySignature method.

The Response did not contain any normal Assertion nodes and thus would fail with ERR_ZERO_SIGNATURE from these lines:

https://github.com/tngan/samlify/blob/master/src/libsaml.ts#L496-L497

however the assertionNode in the response from verifySignature is only used if the decryptRequired flag is not set so it looks like it should be ok to ignore node in this part of the response from:

https://github.com/tngan/samlify/blob/master/src/flow.ts#L223-L227

Changes

• Returns assertionNode: null from verifySignature when entitySetting.isAssertionEncrypted is set.

[ahacker1-securesaml]

Hey, thanks for looking into this and spotting the edge case:

a) Can you try to give a test case for this, so I can ensure my patch works:

With regards to the change:

• We need to make sure that the assertion is extracted from solely the signed contents.
  This means:
• The return value for checkSignature() needs to depend on the getSignedReferences() API
• It cannot be null

We need to significantly refactor the logic for encrypted signed assertions.

b) In the mean time to mitigate the vulnerability apply the patch from old v.2.9.1:

diff --git a/src/libsaml.ts b/src/libsaml.ts
index 3ea5231..95ed42d 100644
--- a/src/libsaml.ts
+++ b/src/libsaml.ts
@@ -469,6 +469,27 @@ const libSaml = () => {
       if (messageSignatureNode.length === 1) {
         const node = select("/*[contains(local-name(), 'Response') or contains(local-name(), 'Request')]/*[local-name(.)='Assertion']", doc);
         if (node.length === 1) {
+          const verifiedMessageSignatureRef = extract(messageSignatureNode[0].toString(), [{
+            key: 'refURI',
+            localPath: ['Signature', 'SignedInfo', 'Reference'],
+            attributes: ['URI']
+          }]);
+
+          const desiredResponseInfo = extract(doc.toString(), [{
+            key: 'id',
+            localPath: ['~Response'],
+            attributes: ['ID']
+          }]);
+          const desiredAssertionInfo = extract(doc.toString(), [{
+            key: 'id',
+            localPath: ['~Response', 'Assertion'],
+            attributes: ['ID']
+          }]);
+
+          if (verifiedMessageSignatureRef.refURI !== `#${desiredResponseInfo.id}` && verifiedMessageSignatureRef.refURI !== `#${desiredAssertionInfo.id}`) {
+            throw new Error('ERR_POTENTIAL_WRAPPING_ATTACK');
+          }
+
           assertionNode = node[0].toString();
         }
       }

[h-bragg]

a proper fix is being applied in #571