Feature/padding hardening

src/Backend/OpenSSL.php

@@ -23,6 +23,7 @@
 use function openssl_verify;
 use function ord;
 use function str_repeat;
+use function strlen;
 use function substr;
 
 /**
@@ -176,6 +177,7 @@ public function decrypt(
         if ($plaintext === false) {
             throw new OpenSSLException('Cannot decrypt data');
         }
+
         return $this->useAuthTag ? $plaintext : $this->unpad($plaintext);
     }
 
@@ -299,11 +301,16 @@ public function setDigestAlg(string $digest): void
      * Pad a plaintext using ISO 10126 padding.
      *
      * @param string $plaintext The plaintext to pad.
-     *
      * @return string The padded plaintext.
+     *
+     * @throws \SimpleSAML\XMLSecurity\Exception\RuntimeException in case padding failed
      */
     public function pad(string $plaintext): string
     {
+        if ($this->blocksize > 256) {
+            throw new RuntimeException('Block size higher than 256 not allowed');
+        }
+
         $padchr = $this->blocksize - (mb_strlen($plaintext) % $this->blocksize);
         $pattern = chr($padchr);
         return $plaintext . str_repeat($pattern, $padchr);
@@ -314,11 +321,29 @@ public function pad(string $plaintext): string
      * Remove an existing ISO 10126 padding from a given plaintext.
      *
      * @param string $plaintext The padded plaintext.
-     *
      * @return string The plaintext without the padding.
+     *
+     * @throws \SimpleSAML\XMLSecurity\Exception\OpenSSLException in case unpadding failed
      */
     public function unpad(string $plaintext): string
     {
-        return substr($plaintext, 0, -ord(substr($plaintext, -1)));
+        $len = mb_strlen($plaintext);
+        if ($len === 0) {
+            /*
+             * Use a single, generic error for every decryption failure. Do not
+             * reveal whether the padding (as opposed to the ciphertext) was the
+             * cause: distinguishable padding errors turn unauthenticated CBC
+             * decryption into a padding oracle (plaintext recovery).
+             */
+            throw new OpenSSLException('Cannot decrypt data');
+        }
+
+        $padLen = ord($plaintext[$len - 1]);
+        $blocksize = $this->blocksize ?? 16;
+        if ($padLen < 1 || $padLen > $blocksize || $padLen > $len) {
+            throw new OpenSSLException('Cannot decrypt data');
+        }
+
+        return substr($plaintext, 0, -$padLen);
     }
 }

tests/XML/ds/RSAKeyValueTest.php

@@ -75,7 +75,7 @@ public function testMarshallingElementOrder(): void
         $modulus = XPath::xpQuery($RSAKeyValueElement, './ds:Modulus', $xpCache);
         $this->assertCount(1, $modulus);
 
-        /** @var \DOMElement[] $RSAKeyValueElements */
+        /** @var \Dom\Element[] $RSAKeyValueElements */
         $RSAKeyValueElements = XPath::xpQuery($RSAKeyValueElement, './ds:Modulus/following-sibling::*', $xpCache);
 
         // Test ordering of RSAKeyValue contents

tests/XML/ds/SignatureMethodTest.php

@@ -96,7 +96,7 @@ public function testMarshallingElementOrder(): void
         $hmacOutputLength = XPath::xpQuery($signatureMethodElement, './ds:HMACOutputLength', $xpCache);
         $this->assertCount(1, $hmacOutputLength);
 
-        /** @var \DOMElement[] $signatureMethodElements */
+        /** @var \Dom\Element[] $signatureMethodElements */
         $signatureMethodElements = XPath::xpQuery(
             $signatureMethodElement,
             './ds:HMACOutputLength/following-sibling::*',

tests/XML/ds/SignatureTest.php

@@ -139,7 +139,7 @@ public function testMarshallingElementOrdering(): void
         $signedInfo = XPath::xpQuery($signatureElement, './ds:SignedInfo', $xpCache);
         $this->assertCount(1, $signedInfo);
 
-        /** @var \DOMElement[] $signatureElements */
+        /** @var \Dom\Element[] $signatureElements */
         $signatureElements = XPath::xpQuery($signatureElement, './ds:SignedInfo/following-sibling::*', $xpCache);
 
         // Test ordering of Signature contents

tests/XML/ds/X509IssuerSerialTest.php

@@ -90,7 +90,7 @@ public function testMarshallingElementOrdering(): void
         $issuerName = XPath::xpQuery($x509IssuerSerialElement, './ds:X509IssuerName', $xpCache);
         $this->assertCount(1, $issuerName);
 
-        /** @var \DOMElement[] $x509IssuerSerialElements */
+        /** @var \Dom\Element[] $x509IssuerSerialElements */
         $x509IssuerSerialElements = XPath::xpQuery(
             $x509IssuerSerialElement,
             './ds:X509IssuerName/following-sibling::*',

tests/XML/xenc/AgreementMethodTest.php

@@ -207,18 +207,18 @@ public function testMarshallingElementOrdering(): void
             [$digestMethod],
         );
 
-        // Marshall it to a \DOMElement
+        // Marshall it to a \Dom\Element
         $agreementMethodElement = $agreementMethod->toXML();
 
         $xpCache = XPath::getXPath($agreementMethodElement);
 
         // Test for an KA-Nonce
-        /** @var \DOMElement[] $kaNonceElements */
+        /** @var \Dom\Element[] $kaNonceElements */
         $kaNonceElements = XPath::xpQuery($agreementMethodElement, './xenc:KA-Nonce', $xpCache);
         $this->assertCount(1, $kaNonceElements);
 
         // Test ordering of AgreementMethod contents
-        /** @var \DOMElement[] $agreementMethodElements */
+        /** @var \Dom\Element[] $agreementMethodElements */
         $agreementMethodElements = XPath::xpQuery(
             $agreementMethodElement,
             './xenc:KA-Nonce/following-sibling::*',

tests/XML/xenc/DHKeyValueTest.php

@@ -79,18 +79,18 @@ public function testMarshalling(): void
      */
     public function testMarshallingElementOrder(): void
     {
-        // Marshall it to a \DOMElement
+        // Marshall it to a \Dom\Element
         $dhKeyValueElement = self::$dhKeyValue->toXML();
 
         $xpCache = XPath::getXPath($dhKeyValueElement);
 
         // Test for an P
-        /** @var \DOMElement[] $pElements */
+        /** @var \Dom\Element[] $pElements */
         $pElements = XPath::xpQuery($dhKeyValueElement, './xenc:P', $xpCache);
         $this->assertCount(1, $pElements);
 
         // Test ordering of DHKeyValue contents
-        /** @var \DOMElement[] $dhKeyValueElements */
+        /** @var \Dom\Element[] $dhKeyValueElements */
         $dhKeyValueElements = XPath::xpQuery(
             $dhKeyValueElement,
             './xenc:P/following-sibling::*',

tests/XML/xenc/EncryptedDataTest.php

@@ -143,7 +143,7 @@ public function testMarshallingElementOrdering(): void
         $this->assertCount(1, $encryptedDataElements);
 
         // Test ordering of EncryptedData contents
-        /** @var \DOMElement[] $encryptedDataElements */
+        /** @var \Dom\Element[] $encryptedDataElements */
         $encryptedDataElements = XPath::xpQuery(
             $encryptedDataElement,
             './xenc:EncryptionMethod/following-sibling::*',

tests/XML/xenc/EncryptedKeyTest.php

@@ -181,7 +181,7 @@ public function testMarshallingElementOrdering(): void
             ]),
         );
 
-        // Marshall it to a \DOMElement
+        // Marshall it to a \Dom\Element
         $encryptedKeyElement = $encryptedKey->toXML();
 
         $xpCache = XPath::getXPath($encryptedKeyElement);
@@ -194,7 +194,7 @@ public function testMarshallingElementOrdering(): void
         $this->assertCount(1, $encryptedKeyElements);
 
         // Test ordering of EncryptedKey contents
-        /** @var \DOMElement[] $encryptedKeyElements */
+        /** @var \Dom\Element[] $encryptedKeyElements */
         $encryptedKeyElements = XPath::xpQuery(
             $encryptedKeyElement,
             './xenc:ReferenceList/following-sibling::*',

tests/XML/xenc11/DerivedKeyTest.php

@@ -150,12 +150,12 @@ public function testMarshallingElementOrder(): void
         $xpCache = XPathUtils::getXPath($dkElement);
 
         // Test for a KeyDerivationMethod
-        /** @var \DOMElement[] $keyDerivationMethodElements */
+        /** @var \Dom\Element[] $keyDerivationMethodElements */
         $keyDerivationMethodElements = XPathUtils::xpQuery($dkElement, './xenc11:KeyDerivationMethod', $xpCache);
         $this->assertCount(1, $keyDerivationMethodElements);
 
         // Test ordering of DerivedKey contents
-        /** @var \DOMElement[] $dkElements */
+        /** @var \Dom\Element[] $dkElements */
         $dkElements = XPathUtils::xpQuery($dkElement, './xenc11:KeyDerivationMethod/following-sibling::*', $xpCache);
 
         $this->assertCount(3, $dkElements);