Skip to content

Commit 85651e5

Browse files
tvdijentvdijen
committed
Backport: SimpleSAMLphp HTTP-Artifact TLS validator confusion allows cross-IdP authentication bypass
1 parent 6695eb9 commit 85651e5

2 files changed

Lines changed: 255 additions & 6 deletions

File tree

‎src/SAML2/SOAPClient.php‎

Lines changed: 45 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@
1414
use SimpleSAML\Configuration;
1515
use SimpleSAML\Utils\Config;
1616
use SimpleSAML\Utils\Crypto;
17-
//use SoapClient as BuiltinSoapClient;
17+
use SimpleSAML\XMLSchema\Type\AnyURIValue;
18+
use SoapClient as BuiltinSoapClient;
1819
use SOAP_1_1;
1920

2021
/**
@@ -142,7 +143,8 @@ public function send(Message $msg, Configuration $srcMetadata, ?Configuration $d
142143
}
143144

144145
/* Perform SOAP Request over HTTP */
145-
$soapresponsexml = $x->__doRequest($request, $destination, $action, SOAP_1_1);
146+
$x = $this->createSoapClient($options);
147+
$soapresponsexml = $this->doSoapRequest($x, $request, $destination, $action);
146148
if (empty($soapresponsexml)) {
147149
throw new Exception('Empty SOAP response, check peer certificate.');
148150
}
@@ -179,6 +181,45 @@ public function send(Message $msg, Configuration $srcMetadata, ?Configuration $d
179181
}
180182

181183

184+
/**
185+
* Factory method to create the built-in SoapClient. Overridable for testing.
186+
*
187+
* @param array $options
188+
* @return \SoapClient
189+
*/
190+
protected function createSoapClient(array $options): BuiltinSoapClient
191+
{
192+
return new BuiltinSoapClient(null, $options);
193+
}
194+
195+
196+
/**
197+
* Wrapper around __doRequest(), overridable for testing.
198+
*
199+
* NOTE: $destination is a generic xs:anyURI value (XMLSchema), since the SOAP endpoint URI
200+
* is transport-level and not necessarily subject to SAML-layer URI restrictions.
201+
*
202+
* @param \SoapClient $client
203+
* @param string|null $request
204+
* @param string $destination
205+
* @param string $action
206+
* @return string
207+
*/
208+
protected function doSoapRequest(
209+
BuiltinSoapClient $client,
210+
?string $request,
211+
string $destination,
212+
string $action
213+
): string {
214+
return (string) $client->__doRequest(
215+
$request,
216+
$destination,
217+
$action,
218+
SOAP_1_1
219+
);
220+
}
221+
222+
182223
/**
183224
* Add a signature validator based on a SSL context.
184225
*
@@ -239,10 +280,8 @@ public static function validateSSL(string $data, XMLSecurityKey $key) : void
239280
throw new Exception('Missing key in public key details.');
240281
}
241282

242-
if ($keyInfo['key'] !== $data) {
243-
$container->getLogger()->debug('Key on SSL connection did not match key we validated against.');
244-
245-
return;
283+
if (trim($keyInfo['key']) !== trim($data)) {
284+
throw new Exception('Key on SSL connection did not match key we validated against.');
246285
}
247286

248287
$container->getLogger()->debug('Message validated based on SSL certificate.');

‎tests/SAML2/SOAPClientTest.php‎

Lines changed: 210 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,210 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace SimpleSAML\Test\SAML2;
6+
7+
use DOMDocument;
8+
use Exception;
9+
use PHPUnit\Framework\Attributes\CoversClass;
10+
use PHPUnit\Framework\Attributes\DataProvider;
11+
use PHPUnit\Framework\TestCase;
12+
use RobRichards\XMLSecLibs\XMLSecurityKey;
13+
use SAML2\CertificatesMock;
14+
use SAML2\Message;
15+
use SAML2\Response;
16+
use SAML2\SOAPClient;
17+
use SimpleSAML\Configuration;
18+
use SimpleSAML\XMLSchema\Type\AnyURIValue;
19+
20+
/**
21+
* Tests for {@see \SimpleSAML\SAML2\SOAPClient}:
22+
* - SSL peer key validation behavior ({@see \SimpleSAML\SAML2\SOAPClient::validateSSL()})
23+
* - send() fail-fast and error handling behavior
24+
*
25+
* Notes:
26+
* - SSL validation tests use deterministic PEM fixtures from simplesamlphp/xml-security
27+
* (via {@see \SAML2\CertificatesMock}) to avoid depending on
28+
* runtime-generated keys.
29+
* - send() tests avoid network I/O by overriding {@see \SimpleSAML\SAML2\SOAPClient::doSoapRequest()}.
30+
*/
31+
#[CoversClass(SOAPClient::class)]
32+
final class SOAPClientTest extends TestCase
33+
{
34+
/**
35+
* @return array<string, array{0: bool}>
36+
*/
37+
public static function provideSslKeyMatchCases(): array
38+
{
39+
return [
40+
'tls key matches xml key' => [true],
41+
'tls key differs from xml key' => [false]
42+
];
43+
}
44+
45+
46+
/**
47+
* Use case: the SSL peer key validator must be fail-closed.
48+
*
49+
* - If the peer key material provided to {@see SOAPClient::validateSSL()} matches the key being validated,
50+
* validation succeeds (no exception).
51+
* - If it does not match, validation must throw (reject).
52+
*
53+
* This test reuses deterministic key fixtures from {@see CertificatesMock}.
54+
*
55+
* @param bool $shouldMatch Whether the peer key material and the XMLSecurityKey should match.
56+
*
57+
* @dataProvider provideSslKeyMatchCases
58+
*/
59+
public function testValidateSslThrowsOnMismatchAndPassesOnMatch(bool $shouldMatch): void
60+
{
61+
$tlsPublicKeyPem = CertificatesMock::PUBLIC_KEY_PEM;
62+
$otherPublicKeyPem = CertificatesMock::PUBLIC_KEY_2_PEM;
63+
64+
$xmlPublicKeyPem = $shouldMatch ? $tlsPublicKeyPem : $otherPublicKeyPem;
65+
$key = $this->buildXmlSecurityPublicKey($xmlPublicKeyPem);
66+
67+
if (!$shouldMatch) {
68+
$this->expectException(Exception::class);
69+
$this->expectExceptionMessage('Key on SSL connection did not match key we validated against.');
70+
}
71+
72+
SOAPClient::validateSSL($tlsPublicKeyPem, $key);
73+
74+
if ($shouldMatch) {
75+
$this->addToAssertionCount(1);
76+
}
77+
}
78+
79+
80+
/**
81+
* Build an {@see XMLSecurityKey} from a PEM-encoded public key, for use with
82+
* {@see SOAPClient::validateSSL()}.
83+
*
84+
* @param string $publicKeyPem PEM-encoded public key (e.g. "-----BEGIN PUBLIC KEY----- ...").
85+
*/
86+
private function buildXmlSecurityPublicKey(string $publicKeyPem): XMLSecurityKey
87+
{
88+
$key = new XMLSecurityKey(XMLSecurityKey::RSA_SHA256, ['type' => 'public']);
89+
$key->loadKey($publicKeyPem, false);
90+
91+
return $key;
92+
}
93+
94+
95+
/**
96+
* @return array<string, array{soapResponse: string, expectedMessage: string}>
97+
*/
98+
public static function provideBadSoapResponses(): array
99+
{
100+
return [
101+
// Use case: SOAP transport returns an empty response body (e.g., TLS/cert failure, transport error).
102+
'empty SOAP response' => [
103+
'soapResponse' => '',
104+
'expectedMessage' => 'Empty SOAP response, check peer certificate.'
105+
],
106+
// Use case: SOAP endpoint returns a SOAP Fault; send() must throw with extracted fields.
107+
'SOAP fault response' => [
108+
'soapResponse' =>
109+
'<env:Envelope xmlns:env="http://schemas.xmlsoap.org/soap/envelope/">' .
110+
'<env:Body>' .
111+
'<env:Fault>' .
112+
'<faultcode>env:Server</faultcode>' .
113+
'<faultstring>oops</faultstring>' .
114+
'<faultactor>actor-1</faultactor>' .
115+
'</env:Fault>' .
116+
'</env:Body>' .
117+
'</env:Envelope>',
118+
'expectedMessage' => "Actor: 'actor-1'; Message: 'oops'; Code: 'env:Server'"
119+
]
120+
];
121+
}
122+
123+
124+
/**
125+
* Use case: send() must throw if the outbound message has no Destination.
126+
public function testSendThrowsWhenDestinationIsMissing(): void
127+
{
128+
$client = new class extends SOAPClient {
129+
protected function createSoapClient(array $options): \SoapClient
130+
{
131+
throw new \LogicException('createSoapClient() should not be called when destination is missing.');
132+
}
133+
134+
135+
protected function doSoapRequest(
136+
\SoapClient $client,
137+
?string $request,
138+
string $destination,
139+
string $action,
140+
): string {
141+
throw new \LogicException('doSoapRequest() should not be called when destination is missing.');
142+
}
143+
};
144+
145+
$msg = $this->createStub(Message::class);
146+
$msg->method('getDestination')->willReturn(null);
147+
148+
$src = Configuration::loadFromArray([], '[src]');
149+
150+
$this->expectException(Exception::class);
151+
$this->expectExceptionMessage('Cannot send SOAP message, no destination set.');
152+
153+
$client->send($msg, $src, null);
154+
}
155+
*/
156+
157+
158+
/**
159+
* Use case: send() must throw on an empty SOAP response and on a SOAP Fault response.
160+
#[DataProvider('provideBadSoapResponses')]
161+
public function testSendThrowsOnEmptySoapResponseOrSoapFault(string $soapResponse, string $expectedMessage): void
162+
{
163+
$destination = 'https://example.org/soap-endpoint';
164+
165+
$client = new class ($soapResponse) extends SOAPClient {
166+
public function __construct(private readonly string $soapResponse)
167+
{
168+
}
169+
170+
171+
protected function createSoapClient(array $options): \SoapClient
172+
{
173+
// A real SoapClient instance, but it will never be used for I/O because doSoapRequest() is overridden.
174+
return new \SoapClient(null, [
175+
'location' => 'http://localhost/soap',
176+
'uri' => 'urn:test',
177+
'exceptions' => true,
178+
'trace' => false,
179+
]);
180+
}
181+
182+
183+
protected function doSoapRequest(
184+
\SoapClient $client,
185+
?string $request,
186+
string $destination,
187+
string $action,
188+
): string {
189+
return $this->soapResponse;
190+
}
191+
};
192+
193+
$msg = $this->createStub(Response::class);
194+
$msg->method('getIssuer')->willReturn(null);
195+
$msg->method('getDestination')->willReturn($destination);
196+
197+
$msg->method('toUnsignedXML')->willReturnCallback(static function () {
198+
$doc = new DOMDocument('1.0', 'UTF-8');
199+
return $doc->appendChild($doc->createElement('TestRequest'));
200+
});
201+
202+
$src = Configuration::loadFromArray([], '[src]');
203+
204+
$this->expectException(Exception::class);
205+
$this->expectExceptionMessage($expectedMessage);
206+
207+
$client->send($msg, $src, null);
208+
}
209+
*/
210+
}

0 commit comments

Comments
 (0)