diff --git a/.github/workflows/test-integration.yml b/.github/workflows/test-integration.yml index f413ebbe..d125e91d 100644 --- a/.github/workflows/test-integration.yml +++ b/.github/workflows/test-integration.yml @@ -6,10 +6,11 @@ jobs: continue-on-error: ${{ matrix.experimental }} strategy: matrix: - php-versions: [ '8.2', '8.4'] + php-versions: [ '8.2', '8.5'] + symfony-versions: [ '6.4.*', '7.4.*'] experimental: [false] timeout-minutes: 15 - name: PHP ${{ matrix.php-versions }} on Ubuntu latest. Experimental == ${{ matrix.experimental }} + name: PHP ${{ matrix.php-versions }} / Symfony ${{ matrix.symfony-versions }} on Ubuntu latest. Experimental == ${{ matrix.experimental }} steps: - name: Install PHP uses: shivammathur/setup-php@v2 @@ -17,8 +18,10 @@ jobs: php-version: ${{ matrix.php-versions }} - name: Checkout uses: actions/checkout@master - - name: Install dependencies - run: composer install + - name: Install Symfony ${{ matrix.symfony-versions }} + run: | + composer require symfony/framework-bundle:${{ matrix.symfony-versions }} symfony/security-bundle:${{ matrix.symfony-versions }} --no-update --no-interaction + composer update --prefer-dist --no-interaction continue-on-error: ${{ matrix.experimental }} - id: checks name: Run CI tests diff --git a/CHANGELOG.md b/CHANGELOG.md index 199d7a2c..af971567 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,11 @@ +# Unreleased +- Add `MduiChunk` to parse and expose `mdui:UIInfo` / `mdui:DisplayName` from AuthnRequest SAML extensions +- Add `Extensions::getMduiChunk()` and `Extensions::hasMduiChunk()` +- Raise minimum Symfony 6 support from 6.3 (EOL) to 6.4 LTS +- Pin `simplesamlphp/saml2` to `<4.20` to avoid class redeclaration conflict with `xml-common` +- Remove `irstea/phpcpd-shim` (source unreachable from CI runners) +- CI matrix updated to PHP 8.2/8.5 and Symfony 6.4/7.4 + # 7.0.2 - Fix version constraint error in `symfony/templating` requirement diff --git a/README.md b/README.md index 798fa634..385c65ea 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,7 @@ Developed as part of the [OpenConext-Stepup Gateway][2] and related OpenConext-S composer require surfnet/stepup-saml-bundle ``` -How to install with SF6 +How to install with Symfony 6.3+ and 7.x 1. Require the bundle in the composer.json (version 4.1.9 or higher) 2. Enable the bundle in `config/bundles.php` add to the return statement: `Surfnet\SamlBundle\SurfnetSamlBundle::class => ['all' => true],` diff --git a/ci/qa/phpstan-baseline.neon b/ci/qa/phpstan-baseline.neon index dc1efd07..d6fa3286 100644 --- a/ci/qa/phpstan-baseline.neon +++ b/ci/qa/phpstan-baseline.neon @@ -486,12 +486,6 @@ parameters: count: 1 path: ../../src/SAML2/AuthnRequest.php - - - message: '#^Method Surfnet\\SamlBundle\\SAML2\\AuthnRequest\:\:getAuthenticationContextClassRef\(\) should return string\|null but returns mixed\.$#' - identifier: return.type - count: 1 - path: ../../src/SAML2/AuthnRequest.php - - message: '#^Method Surfnet\\SamlBundle\\SAML2\\AuthnRequest\:\:getDestination\(\) should return string but returns string\|null\.$#' identifier: return.type @@ -528,12 +522,6 @@ parameters: count: 1 path: ../../src/SAML2/AuthnRequest.php - - - message: '#^Parameter \#1 \$array of function reset expects array\|object, mixed given\.$#' - identifier: argument.type - count: 1 - path: ../../src/SAML2/AuthnRequest.php - - message: '#^Parameter \#1 \$string of function base64_encode expects string, string\|false given\.$#' identifier: argument.type @@ -642,18 +630,6 @@ parameters: count: 1 path: ../../src/SAML2/Extensions/Extensions.php - - - message: '#^Cannot call method saveXML\(\) on DOMDocument\|null\.$#' - identifier: method.nonObject - count: 1 - path: ../../src/SAML2/Extensions/GsspUserAttributesChunk.php - - - - message: '#^Method Surfnet\\SamlBundle\\SAML2\\Extensions\\GsspUserAttributesChunk\:\:toXML\(\) has no return type specified\.$#' - identifier: missingType.return - count: 1 - path: ../../src/SAML2/Extensions/GsspUserAttributesChunk.php - - message: '#^Parameter \#1 \$element of method Surfnet\\SamlBundle\\SAML2\\Extensions\\Chunk\:\:append\(\) expects DOMElement, DOMElement\|null given\.$#' identifier: argument.type @@ -678,12 +654,6 @@ parameters: count: 1 path: ../../src/SAML2/ReceivedAuthnRequest.php - - - message: '#^Method Surfnet\\SamlBundle\\SAML2\\ReceivedAuthnRequest\:\:getAuthenticationContextClassRef\(\) should return string\|null but returns mixed\.$#' - identifier: return.type - count: 1 - path: ../../src/SAML2/ReceivedAuthnRequest.php - - message: '#^Method Surfnet\\SamlBundle\\SAML2\\ReceivedAuthnRequest\:\:getScopingRequesterIds\(\) return type has no value type specified in iterable type array\.$#' identifier: missingType.iterableValue @@ -708,12 +678,6 @@ parameters: count: 1 path: ../../src/SAML2/ReceivedAuthnRequest.php - - - message: '#^Parameter \#1 \$array of function reset expects array\|object, mixed given\.$#' - identifier: argument.type - count: 1 - path: ../../src/SAML2/ReceivedAuthnRequest.php - - message: '#^Parameter \#1 \$xml of static method SAML2\\Message\:\:fromXML\(\) expects DOMElement, DOMNode given\.$#' identifier: argument.type diff --git a/composer.json b/composer.json index 5e8b8281..958a9c23 100644 --- a/composer.json +++ b/composer.json @@ -11,10 +11,10 @@ "ext-openssl": "*", "psr/log": "^3.0", "robrichards/xmlseclibs": "^3.1.4", - "simplesamlphp/saml2": "^4.6", + "simplesamlphp/saml2": ">=4.6 <4.20", "symfony/dependency-injection": "^6.3|^7.0", - "symfony/framework-bundle": "^6.3|^7.0", - "symfony/security-bundle": "^6.3|^7.0", + "symfony/framework-bundle": "^6.4|^7.4", + "symfony/security-bundle": "^6.4|^7.4", "symfony/templating": "^6.3|^7.0", "twig/twig": "^3" }, @@ -24,7 +24,6 @@ "require-dev": { "ext-libxml": "*", "ext-zlib": "*", - "irstea/phpcpd-shim": "^6.0", "malukenho/docheader": "^1.1", "mockery/mockery": "^1.5", "overtrue/phplint": "*", @@ -47,7 +46,6 @@ "@composer-validate", "@license-headers", "@phplint", - "@phpcpd", "@phpcs", "@phpmd", "@test", @@ -57,7 +55,6 @@ "composer-validate": "./ci/qa/validate", "phplint": "./ci/qa/phplint", "phpcs": "./ci/qa/phpcs", - "phpcpd": "./ci/qa/phpcpd", "phpmd": "./ci/qa/phpmd", "phpstan": "./ci/qa/phpstan", "phpstan-baseline": "./ci/qa/phpstan-update-baseline", @@ -85,6 +82,14 @@ "dealerdirect/phpcodesniffer-composer-installer": true, "phpstan/extension-installer": true, "simplesamlphp/composer-xmlprovider-installer": true + }, + "audit": { + "ignore": { + "PKSA-1fc7-xrz7-vw78": "GHSA-5cjr-mxj5-wmrx: DoS via XPath Transform in ds:Signature processing. No patched simplesamlphp/saml2 4.x release exists or is planned; 5.x/6.x are a ground-up rewrite without the SP/IdP response processing this bundle needs. Mitigated in application code: Surfnet\\SamlBundle\\Signing\\SignatureTransformGuard rejects any XPath Transform algorithm before the vulnerable code path is reached, called from Http/PostBinding.php::processResponse() before every signature verification. See src/Tests/Component/Signing/SignatureTransformGuardTest.php.", + "PKSA-yk3g-3g3t-ts6q": "GHSA-6929-8p9f-26jx: TLS validator confusion in HTTP-Artifact binding resolution. This bundle implements only HTTP-Redirect (Http/RedirectBinding.php) and HTTP-POST (Http/PostBinding.php) bindings; HTTP-Artifact binding is not implemented and this code path is unreachable.", + "PKSA-rxdv-j1j4-96fj": "GHSA-46r4-f8gj-xg56: HTTP-Redirect signature verification bypass, fixed in simplesamlphp/saml2 >=4.16.16. Locked version is 4.19.2 (see composer.lock), already patched.", + "PKSA-1983-c8jn-trgm": "GHSA-pxm4-r5ph-q2m2: XXE in SAML message parsing, fixed in simplesamlphp/saml2 >=4.6.14. Locked version is 4.19.2 (see composer.lock), already patched." + } } } } diff --git a/src/Http/PostBinding.php b/src/Http/PostBinding.php index d9ef75c7..06628418 100644 --- a/src/Http/PostBinding.php +++ b/src/Http/PostBinding.php @@ -36,6 +36,7 @@ use Surfnet\SamlBundle\Http\Exception\UnknownServiceProviderException; use Surfnet\SamlBundle\SAML2\AuthnRequest; use Surfnet\SamlBundle\SAML2\ReceivedAuthnRequest; +use Surfnet\SamlBundle\Signing\SignatureTransformGuard; use Surfnet\SamlBundle\Signing\SignatureVerifier; use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; @@ -72,6 +73,8 @@ public function processResponse( $asXml = DOMDocumentFactory::fromString($response); + SignatureTransformGuard::assertNoForbiddenTransforms($asXml); + try { $assertions = $this->responseProcessor->process( $serviceProvider, diff --git a/src/SAML2/Extensions/Extensions.php b/src/SAML2/Extensions/Extensions.php index 4f6b823b..c1277044 100644 --- a/src/SAML2/Extensions/Extensions.php +++ b/src/SAML2/Extensions/Extensions.php @@ -48,4 +48,17 @@ public function hasGsspUserAttributesChunk(): bool { return array_key_exists('UserAttributes', $this->chunks); } + + public function getMduiChunk(): ?MduiChunk + { + if (!$this->hasMduiChunk()) { + return null; + } + return new MduiChunk($this->chunks['UIInfo']->getValue()); + } + + public function hasMduiChunk(): bool + { + return array_key_exists('UIInfo', $this->chunks); + } } diff --git a/src/SAML2/Extensions/ExtensionsMapperTrait.php b/src/SAML2/Extensions/ExtensionsMapperTrait.php index 092cb979..7fd52c36 100644 --- a/src/SAML2/Extensions/ExtensionsMapperTrait.php +++ b/src/SAML2/Extensions/ExtensionsMapperTrait.php @@ -35,6 +35,9 @@ private function loadExtensionsFromSaml2AuthNRequest(): void 'UserAttributes' => $this->extensions->addChunk( new GsspUserAttributesChunk($rawChunk->getXML()) ), + 'UIInfo' => $this->extensions->addChunk( + new MduiChunk($rawChunk->getXML()) + ), default => $this->extensions->addChunk( new Chunk( $rawChunk->getLocalName(), diff --git a/src/SAML2/Extensions/GsspUserAttributesChunk.php b/src/SAML2/Extensions/GsspUserAttributesChunk.php index cb443440..c508669f 100644 --- a/src/SAML2/Extensions/GsspUserAttributesChunk.php +++ b/src/SAML2/Extensions/GsspUserAttributesChunk.php @@ -19,6 +19,7 @@ use DOMDocument; use DOMElement; +use RuntimeException; use SAML2\Utils; class GsspUserAttributesChunk extends Chunk @@ -65,9 +66,17 @@ public function addAttribute(string $name, string $format, string $value): void $this->append($doc->documentElement); } - public function toXML() + public function toXML(): string { - return $this->getValue()->ownerDocument->saveXML(); + $doc = $this->getValue()->ownerDocument; + if ($doc === null) { + throw new RuntimeException('DOMElement has no ownerDocument'); + } + $xml = $doc->saveXML(); + if ($xml === false) { + throw new RuntimeException('Failed to serialize XML document'); + } + return $xml; } public static function fromXML(string $xmlString): GsspUserAttributesChunk diff --git a/src/SAML2/Extensions/MduiChunk.php b/src/SAML2/Extensions/MduiChunk.php new file mode 100644 index 00000000..f8780b1b --- /dev/null +++ b/src/SAML2/Extensions/MduiChunk.php @@ -0,0 +1,97 @@ +createElementNS(self::MDUI_NAMESPACE, 'mdui:UIInfo'); + + if ($value && $value->hasChildNodes()) { + foreach ($value->childNodes as $child) { + $root->appendChild($doc->importNode($child->cloneNode(true), true)); + } + } + + $doc->appendChild($doc->importNode($root, true)); + $element = $doc->documentElement; + if ($element === null) { + throw new RuntimeException('Failed to create UIInfo root element'); + } + parent::__construct('UIInfo', self::MDUI_NAMESPACE, $element); + } + + /** + * @return array keyed by xml:lang + */ + public function getDisplayNames(): array + { + $names = []; + foreach ($this->getValue()->childNodes as $child) { + if (!($child instanceof DOMElement)) { + continue; + } + if ($child->localName !== self::DISPLAY_NAME_ELEMENT || $child->namespaceURI !== self::MDUI_NAMESPACE) { + continue; + } + $lang = $child->getAttribute('xml:lang'); + $value = $child->textContent; + if ($lang !== '' && $value !== '') { + $names[$lang] = $value; + } + } + return $names; + } + + public function toXML(): string + { + $doc = $this->getValue()->ownerDocument; + if ($doc === null) { + throw new RuntimeException('DOMElement has no ownerDocument'); + } + $xml = $doc->saveXML(); + if ($xml === false) { + throw new RuntimeException('Failed to serialize XML document'); + } + return $xml; + } + + public static function fromXML(string $xmlString): self + { + $doc = new DOMDocument(); + $previousInternalErrors = libxml_use_internal_errors(true); + try { + if (!$doc->loadXML($xmlString)) { + throw new RuntimeException('Unable to parse UIInfo XML'); + } + } finally { + libxml_use_internal_errors($previousInternalErrors); + } + return new self($doc->documentElement); + } +} diff --git a/src/Signing/SignatureTransformGuard.php b/src/Signing/SignatureTransformGuard.php new file mode 100644 index 00000000..d2d7023b --- /dev/null +++ b/src/Signing/SignatureTransformGuard.php @@ -0,0 +1,79 @@ +` algorithms that are not needed for SAML 2.0 signature + * verification, before the document reaches simplesamlphp/saml2's signature processing. + * + * simplesamlphp/saml2 <=4.20.2 (and the underlying robrichards/xmlseclibs it uses) has no + * upstream patch for GHSA-5cjr-mxj5-wmrx: an attacker-controlled `` element executes an arbitrary, attacker-supplied XPath + * expression during signature-reference canonicalization, which can be used for a Denial of + * Service. This guard implements the same mitigation the advisory describes ("restrict + * transforms to only the transform-algorithms mentioned in the SAML 2.0 Core Specifications") + * directly, since no patched release of the v4 line exists or is planned. simplesamlphp/saml2 + * v5+/v6+ fix this internally but are a ground-up rewrite without the SP/IdP response + * processing this bundle depends on, so are not a viable upgrade path yet. + * + * A SAML message can carry independently signed Response and Assertion elements, each with + * their own ``/`` chain, so this scans the whole document rather + * than a single signature node. + */ +final class SignatureTransformGuard +{ + /** + * The only Transform algorithms simplesamlphp/saml2's response/assertion signature + * verification legitimately needs to process. Anything else is rejected outright. + */ + private const ALLOWED_TRANSFORM_ALGORITHMS = [ + 'http://www.w3.org/2000/09/xmldsig#enveloped-signature', + 'http://www.w3.org/2001/10/xml-exc-c14n#', + 'http://www.w3.org/2001/10/xml-exc-c14n#WithComments', + 'http://www.w3.org/TR/2001/REC-xml-c14n-20010315', + 'http://www.w3.org/TR/2001/REC-xml-c14n-20010315#WithComments', + ]; + + public static function assertNoForbiddenTransforms(DOMDocument $document): void + { + $xpath = new DOMXPath($document); + $nodes = $xpath->query("//*[local-name()='Transform']"); + if ($nodes === false) { + throw new RuntimeException('Failed to evaluate XPath while scanning for signature transforms'); + } + + foreach ($nodes as $node) { + $algorithm = $node->attributes?->getNamedItem('Algorithm')?->nodeValue; + if ($algorithm === null || in_array($algorithm, self::ALLOWED_TRANSFORM_ALGORITHMS, true)) { + continue; + } + + throw new SignatureValidationFailedException(sprintf( + 'Rejected SAML message: contains a forbidden XML signature Transform algorithm "%s"' + . ' (see GHSA-5cjr-mxj5-wmrx)', + $algorithm + )); + } + } +} diff --git a/src/Tests/Component/Extensions/MduiChunkTest.php b/src/Tests/Component/Extensions/MduiChunkTest.php new file mode 100644 index 00000000..e30e20a5 --- /dev/null +++ b/src/Tests/Component/Extensions/MduiChunkTest.php @@ -0,0 +1,50 @@ +' + . 'My Service' + . 'Mijn Dienst' + . ''; + + $chunk = MduiChunk::fromXML($xml); + $restored = MduiChunk::fromXML($chunk->toXML()); + + $this->assertSame( + ['en' => 'My Service', 'nl' => 'Mijn Dienst'], + $restored->getDisplayNames() + ); + } + + public function test_from_xml_throws_on_malformed_xml(): void + { + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('Unable to parse UIInfo XML'); + + MduiChunk::fromXML(' + + http://idp.example.com/metadata.php + + + + + + + %s + + + irrelevant + + + irrelevant + + + http://idp.example.com/metadata.php + %s + + +XML; + + private const ASSERTION_SIGNATURE_TEMPLATE = << + + + + + + %s + + + irrelevant + + + irrelevant + +XML; + + private function buildDocument(string $responseTransforms, string $assertionSignature = ''): DOMDocument + { + $xml = sprintf(self::RESPONSE_WITH_SIGNATURE_TEMPLATE, $responseTransforms, $assertionSignature); + + $document = new DOMDocument(); + $document->loadXML($xml); + + return $document; + } + + private function enveloped(): string + { + return ''; + } + + private function exclusiveC14n(): string + { + return ''; + } + + private function xpathFiltering(): string + { + return '' + . 'count(//. | //@* | //namespace::*) > 1000000' + . ''; + } + + private function xpathFilter2(): string + { + return ''; + } + + public function test_it_allows_only_legitimate_transforms_on_the_response_signature(): void + { + $document = $this->buildDocument($this->enveloped() . $this->exclusiveC14n()); + + SignatureTransformGuard::assertNoForbiddenTransforms($document); + + self::assertTrue(true, 'no exception should have been thrown'); + } + + public function test_it_rejects_xpath_filtering_transform_on_the_response_signature(): void + { + $document = $this->buildDocument($this->enveloped() . $this->xpathFiltering()); + + self::expectException(SignatureValidationFailedException::class); + self::expectExceptionMessage('http://www.w3.org/TR/1999/REC-xpath-19991116'); + + SignatureTransformGuard::assertNoForbiddenTransforms($document); + } + + public function test_it_rejects_xpath_filter2_transform_on_the_response_signature(): void + { + $document = $this->buildDocument($this->enveloped() . $this->xpathFilter2()); + + self::expectException(SignatureValidationFailedException::class); + self::expectExceptionMessage('http://www.w3.org/2002/06/xmldsig-filter2'); + + SignatureTransformGuard::assertNoForbiddenTransforms($document); + } + + public function test_it_rejects_xpath_filtering_transform_on_an_assertion_level_signature_even_when_the_response_signature_is_clean(): void + { + $assertionSignature = sprintf( + self::ASSERTION_SIGNATURE_TEMPLATE, + $this->enveloped() . $this->xpathFiltering() + ); + $document = $this->buildDocument($this->enveloped() . $this->exclusiveC14n(), $assertionSignature); + + self::expectException(SignatureValidationFailedException::class); + self::expectExceptionMessage('http://www.w3.org/TR/1999/REC-xpath-19991116'); + + SignatureTransformGuard::assertNoForbiddenTransforms($document); + } + + public function test_it_allows_a_document_with_no_signature_at_all(): void + { + $document = new DOMDocument(); + $document->loadXML(''); + + SignatureTransformGuard::assertNoForbiddenTransforms($document); + + self::assertTrue(true, 'no exception should have been thrown'); + } +} diff --git a/src/Tests/Unit/Http/PostBindingSamlResponseTest.php b/src/Tests/Unit/Http/PostBindingSamlResponseTest.php index 86d6166c..dd21e840 100644 --- a/src/Tests/Unit/Http/PostBindingSamlResponseTest.php +++ b/src/Tests/Unit/Http/PostBindingSamlResponseTest.php @@ -32,6 +32,7 @@ use Surfnet\SamlBundle\Entity\ServiceProviderRepository; use Surfnet\SamlBundle\Http\Exception\AuthnFailedSamlResponseException; use Surfnet\SamlBundle\Http\Exception\NoAuthnContextSamlResponseException; +use Surfnet\SamlBundle\Http\Exception\SignatureValidationFailedException; use Surfnet\SamlBundle\Http\PostBinding; use Surfnet\SamlBundle\Signing\SignatureVerifier; use Symfony\Component\HttpFoundation\Request; @@ -97,6 +98,31 @@ public function test_process_response_happy_flow(): void self::assertInstanceOf(Assertion::class, $samlResponse); } + public function test_process_response_rejects_forbidden_signature_transform_before_processing(): void + { + $maliciousMessage = << +http://idp.example.com/metadata.phpcount(//. | //@* | //namespace::*) > 1000000irrelevantirrelevant +MESSAGE; + $requestUri = 'https://stepup.example.com/'; + $post = [ + 'SAMLResponse' => base64_encode($maliciousMessage), + 'RelayState' => '', + ]; + $request = new Request([], $post, [], [], [], ['REQUEST_URI' => $requestUri]); + $request->setMethod(Request::METHOD_POST); + $idp = m::mock(IdentityProvider::class); + $sp = m::mock(ServiceProvider::class); + + // The mocked processor must never be reached: the guard must reject before it is called. + $this->processor->shouldNotReceive('process'); + + self::expectException(SignatureValidationFailedException::class); + self::expectExceptionMessage('http://www.w3.org/TR/1999/REC-xpath-19991116'); + + $this->postBinding->processResponse($request, $idp, $sp); + } + public function test_process_response_must_have_saml_response(): void { $requestUri = 'https://stepup.example.com/';