search

SAML-Toolkits / php-saml

Simple SAML toolkit for PHP
MIT License
1.23k stars 471 forks source link

Signature validation fails when using ADFS 3.0 as IdP with encryption #282

Closed gmoniker closed 1 year ago

gmoniker commented 6 years ago

We are trying to use this library to connect to an ADFS 3.0 as IdP.

Because we want responses to be signed and encrypted, or at least signed, we set the metadata of onelogin to signed and import it to ADFS to setup a trust with known SP public key to encrypt the assertion and ADFS signs it with its own private key, for which we enter the public key (certificate) into the settings of onelogin.

However with 'strict' setting enabled, each ADFS response for login is considered invalid because of a failure to verify the signature on the assertion.

We checked all our settings and knobs and then started dumping responses and looking into the code. The root cause of this issue turns out to be known, the decrypted node is imported into the XML response document with a replaceChild of DomDocument and then gets 'default' prefixes on the Signature element which changes the canonicalized form of the XML SignInfo and therefore invalidates the signature. This has to do with the fact that the ADFS 3.0 response with a signed Assertion has nested default namespaces.

See also https://github.com/onelogin/php-saml/issues/94 and https://github.com/onelogin/php-saml/issues/29.

Is there a solution for this problem?

pitbulk commented 6 years ago

Can you try to use the 3.0.0 branch that depends on a newer version of xmlseclibs and see if that solves the issue?

gmoniker commented 6 years ago

@pitbulk, I have tried the 3.0.0 branch with the same results as before. It fails to verify the signature after decryption of the contents of the EncryptedAssertion element.

Looking at the code of _decryptAssertion https://github.com/onelogin/php-saml/blob/b7d1f7ca8e6c52ddef64ceb3ceab16637af459f7/lib/Saml2/Response.php#L1042 https://github.com/onelogin/php-saml/blob/b7d1f7ca8e6c52ddef64ceb3ceab16637af459f7/lib/Saml2/Response.php#L1113

It uses the replaceChild method itself, so it would not be possible to solve this with an update of xmlseclibs, even if it changes the implementation of decryptNode with $replace=true