Signature validation fails for Assertions with EncryptedID due to premature decryption

[ojura]

I observed this on 4.x. When a SAML Response includes a signed <saml:Assertion> containing an <saml:EncryptedID>, the signature check fails.

The problem stems from OneLogin\Saml2\Response::decryptAssertion(), which decrypts the <saml:EncryptedID> right after decrypting the <saml:EncryptedAssertion>. This looks like an optimization to cache the decrypted NameID for later use in getNameIdData(), but it breaks things.

There's two issues:

• The assertion is altered before validating the signature. Since the original signature was generated with the EncryptedID in place, any modification (like decryption) before validation invalidates the digest.

• The decryptNameId() method sends the wrong key type to Utils::decryptElement(), which causes an algorithm mismatch error. For example:

  1. The decryptAssertion() method is called. It successfully decrypts the main <saml:EncryptedAssertion> block.
  2. It then finds the <saml:EncryptedID> inside the now-decrypted assertion.
  3. It calls a new method, decryptNameId(), to handle this inner decryption.
  4. The decryptNameId() method correctly uses the private key to decrypt the session key that is wrapped inside the <saml:EncryptedID>.
  5. However, it then passes this newly-decrypted session key (which uses aes128-cbc) to the Utils::decryptElement() function.
  6. The Utils::decryptElement() function expects the private key (which uses rsa-oaep-mgf1p) to start its own process of decrypting the session key. When it sees it has been given a key of the wrong type, it throws the "Algorithm mismatch" error.

Seems like the new code is trying to do the same decryption step twice, passing the result of the first attempt as the input to the second.

Fix:

Defer the EncryptedID decryption until getNameIdData() runs, after the signature has been verified. Add memoization in getNameIdData() so the decryption happens once and is reused.

--- a/src/Saml2/Response.php
+++ b/src/Saml2/Response.php
@@ -55,6 +55,13 @@
  * @var int
  */
 private $_validSCDNotOnOrAfter;
+
+/**
+ * Cached NameID Data
+ *
+ * @var array
+ */
+private $_nameIdData = array();

@@ -500,6 +507,10 @@
  */
 public function getNameIdData()
 {
+    if (!empty($this->_nameIdData)) {
+        return $this->_nameIdData;
+    }
+
     $encryptedIdDataEntries = $this->_queryAssertion('/saml:Subject/saml:EncryptedID/xenc:EncryptedData');

     if ($encryptedIdDataEntries->length == 1) {
@@ -548,7 +559,8 @@
         }
     }

-    return $nameIdData;
+    $this->_nameIdData = $nameIdData;
+    return $this->_nameIdData;
 }

@@ -1013,10 +1025,8 @@

     if ($encryptedID) {
         // decrypt the encryptedID
         $this->encryptedNameId = true;
-        $encryptedData = $encryptedID->getElementsByTagName('EncryptedData')->item(0);
-        $nameId = $this->decryptNameId($encryptedData, $pem);
-        Utils::treeCopyReplace($encryptedID, $nameId);
+        // Decryption of NameID is handled in getNameIdData() after signature validation.
     }

     if ($encData->parentNode instanceof DOMDocument) {

[pitbulk]

That is a scenario I never tested,

What is the point of encrypting the Name ID if you already have the Assertion encrypted?
What IdP are you using that allows you to encrypt both elements at the same time?

[ojura]

There is no point, I agree. 😅

I didn't really know what I was doing, and I ticked all the checkboxes. The funny thing is, now it's summer break time and it's an administrative thing to get it changed.

But... technically... it shouldn't be broken, right?

[pitbulk]

I will mark this as a bug, but as it is not a real case scenario (never saw a real IdP allowing this) I will give it low priority to be fixed. Thanks for reporting this.

[ojura]

Thanks @pitbulk. Let me know if a test case would help.

[Daniel-KM]

There was a test that was not passed before the fix #638 and now it works.