simplesamlphp / saml2

SimpleSAMLphp low-level SAML2 PHP library
https://www.simplesamlphp.org
GNU Lesser General Public License v2.1
284 stars 136 forks source link

SAML2\SOAPClient fails to detect non-SOAP response #253

Closed RoSk0 closed 3 years ago

RoSk0 commented 4 years ago

While testing new certificates I encountered that if I receive this response:

<problem xmlns="urn:ietf:rfc:7807">
  <status>401</status>
  <title>Unauthorized</title>
  <type>https://tools.ietf.org/html/rfc7235#section-3.1</type>
  <traceId>00-0000000000000000-0000000000-00</traceId>
</problem>

from Artifact binding endpoint it passes

$dom = DOMDocumentFactory::fromString($soapresponsexml);

and

$soapfault = $this->getSOAPFault($dom);
        if (isset($soapfault)) {
            throw new \Exception($soapfault);
        }

and goes into

$samlresponse = Utils::xpQuery($dom->firstChild, '/soap-env:Envelope/soap-env:Body/*[1]');
$samlresponse = Message::fromXML($samlresponse[0]);

producing white screen for the end user and this in the logs:

SimpleSAML\Error\Exception: Error 8 - Undefined offset: 0 at /var/www/php/vendor/simplesamlphp/saml2/src/SAML2/SOAPClient.php:161
Backtrace:
5 /var/www/php/vendor/simplesamlphp/simplesamlphp/www/_include.php:48 (SimpleSAML_error_handler)
4 /var/www/php/vendor/simplesamlphp/saml2/src/SAML2/SOAPClient.php:161 (SAML2\SOAPClient::send)
3 /var/www/php/vendor/simplesamlphp/saml2/src/SAML2/HTTPArtifact.php:149 (SAML2\HTTPArtifact::receive)
2 /var/www/php/vendor/simplesamlphp/simplesamlphp/modules/saml/www/sp/saml2-acs.php:35 (require)
1 /var/www/php/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/Module.php:260 (SimpleSAML\Module::process)
0 /var/www/php/vendor/simplesamlphp/simplesamlphp/www/module.php:10 (N/A)
SimpleSAML\Error\Exception: Error 1 - Argument 1 passed to SAML2\Message::fromXML() must be an instance of DOMElement, null given, called in /var/www/php/vendor/simplesamlphp/saml2/src/SAML2/SOAPClient.php on line 161 at /var/www/php/vendor/simplesamlphp/saml2/src/SAML2/Message.php:560
Backtrace:
2 /var/www/php/vendor/simplesamlphp/simplesamlphp/www/_include.php:48 (SimpleSAML_error_handler)
1 /var/www/php/vendor/simplesamlphp/simplesamlphp/www/_include.php:25 (SimpleSAML_exception_handler)

There should be smarts to detect this in either getSOAPFault() or send() method itself.

tvdijen commented 4 years ago

I think it's an easy fix if we flip these two parts of the code; https://github.com/simplesamlphp/saml2/blob/master/src/SAML2/SOAPClient.php#L158-L165 i.e. first parse the message (your case with non-soap response should fail there) and then see if the SOAP-response contains a Fault.

@RoSk0 is this something you can test for us?

RoSk0 commented 3 years ago

Hi @tvdijen ,

Thanks for suggestion. I implemented this fix and tested it - now I see only one log entry for the bad( non soap ) response and SimpleSAMLphp show the user user error screen which is great.

Take a look at a PR please, it's not exactly as you suggested but I think close enough.

tvdijen commented 3 years ago

If it works, I'm not complaining ;) Thanks a lot @RoSk0 for your work! I'll tag 4.2.0 and make sure this change finds it's way upstream

RoSk0 commented 3 years ago

Thanks!