Bugfix: try multiple keys to validate signature (#51)

* Add unit test * Remove unused use-statement * Clone document before manipulating it
simplesamlphp · Jul 25, 2024 · efda700 · efda700
1 parent 01128b2
commit efda700
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 5 deletions.
diff --git a/src/XML/SignedElementTrait.php b/src/XML/SignedElementTrait.php
@@ -27,7 +27,6 @@
 use SimpleSAML\XMLSecurity\XML\ds\X509Certificate;
 use SimpleSAML\XMLSecurity\XML\ds\X509Data;
 
-use function array_pop;
 use function base64_decode;
 use function hash;
 use function hash_equals;
@@ -137,13 +136,18 @@ private function validateReference(SignedInfo $signedInfo): SignedElementInterfa
             $this->validateReferenceUri($reference, $xml);
         }
 
-        $xp = XPath::getXPath($xml->ownerDocument);
-        $sigNode = XPath::xpQuery($xml, 'child::ds:Signature', $xp);
+        // Clone the document so we don't mess up the original DOMDocument
+        $doc = DOMDocumentFactory::create();
+        $node = $doc->importNode($xml->ownerDocument->documentElement, true);
+        $doc->appendChild($node);
+
+        $xp = XPath::getXPath($doc);
+        $sigNode = XPath::xpQuery($doc->documentElement, 'child::ds:Signature', $xp);
         Assert::minCount($sigNode, 1, NoSignatureFoundException::class);
         Assert::maxCount($sigNode, 1, 'More than one signature found in object.', TooManyElementsException::class);
-        $xml->removeChild($sigNode[0]);
 
-        $data = XML::processTransforms($reference->getTransforms(), $xml);
+        $doc->documentElement->removeChild($sigNode[0]);
+        $data = XML::processTransforms($reference->getTransforms(), $doc->documentElement);
         $algo = $reference->getDigestMethod()->getAlgorithm();
         Assert::keyExists(
             C::$DIGEST_ALGORITHMS,

diff --git a/tests/XML/SignedElementTest.php b/tests/XML/SignedElementTest.php
@@ -34,6 +34,9 @@ final class SignedElementTest extends TestCase
     /** @var \SimpleSAML\XMLSecurity\CryptoEncoding\PEM */
     private PEM $certificate;
 
+    /** @var \SimpleSAML\XMLSecurity\CryptoEncoding\PEM */
+    private PEM $wrong_certificate;
+
     /** @var \DOMElement */
     private DOMElement $signedDocumentWithComments;
 
@@ -63,6 +66,10 @@ public function setUp(): void
         $this->certificate = PEM::fromString(
             PEMCertificatesMock::getPlainCertificate(PEMCertificatesMock::SELFSIGNED_CERTIFICATE),
         );
+
+        $this->wrong_certificate = PEM::fromString(
+            PEMCertificatesMock::getPlainCertificate(PEMCertificatesMock::OTHER_CERTIFICATE),
+        );
     }
 
 
@@ -108,6 +115,44 @@ public function testSuccessfulVerifyingWithGivenKey(): void
     }
 
 
+    /**
+     * Test the verification of a signature with the wrong key first, and the right one second.
+     */
+    public function testSuccessfulVerifyingWithWrongKeyFirstRightOneSecond(): void
+    {
+        $customSigned = CustomSignable::fromXML($this->signedDocument);
+
+        $this->assertTrue($customSigned->isSigned());
+        $signature = $customSigned->getSignature();
+        $this->assertInstanceOf(Signature::class, $signature);
+        $sigAlg = $signature->getSignedInfo()->getSignatureMethod()->getAlgorithm();
+        $this->assertEquals(C::SIG_RSA_SHA256, $sigAlg);
+
+        $verified = null;
+        foreach ([$this->wrong_certificate, $this->certificate] as $i => $key) {
+            $factory = new SignatureAlgorithmFactory();
+            $certificate = new X509Certificate($key);
+            $verifier = $factory->getAlgorithm($sigAlg, $certificate->getPublicKey());
+
+            try {
+                $verified = $customSigned->verify($verifier);
+                break 1;
+            } catch (\SimpleSAML\XMLSecurity\Exception\SignatureVerificationFailedException $e) {
+                continue;
+            }
+        }
+
+        $this->assertInstanceOf(CustomSignable::class, $verified);
+        $this->assertFalse($verified->isSigned());
+        $this->assertEquals(
+            '<ssp:CustomSignable xmlns:ssp="urn:x-simplesamlphp:namespace"><ssp:Chunk>Some' .
+            '</ssp:Chunk></ssp:CustomSignable>',
+            strval($verified),
+        );
+        $this->assertEquals($certificate->getPublicKey(), $verified->getVerifyingKey());
+    }
+
+
     /**
      * Test the verification of a signature without passing a key, just what's in KeyInfo
      */