-
-
Notifications
You must be signed in to change notification settings - Fork 473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue decrypting signed assertion under PHP 8.1 (7.4 is just fine) #562
Comments
Seems to be
|
Saving and restoring the namespace prefix works around the issue as there doesn't seem to be a way to stop PHP from changing the namespace on you....
|
For splicing in the decrypted XML does this make sense? Surely it should be as decrypted and not manipulated! |
@yphoenix thanks for your research, I will investigate and merge your PR if makes sense. |
I might be running into the same issue after updating my server from PHP 8.2.7 to 8.2.8. Signatures are no longer valid and SAML login fails. It's in a Laravel 10 app using 24slides/laravel-saml2 (2.2.0), which uses onelogin/php-saml (4.1.0). Everything works great on PHP 8.2.7 and before, no longer on 8.2.8 and up. As someone mentioned before, there are quite some DOM patches in 8.2.8. When using the fixed version (php-saml-4.1.0-Issio-Fix-10609) it does not work yet, getting a different error though: |
Some additional info, the exact errors i'm getting are: and Reference validation failed from vendor/robrichards/xmlseclibs/src/XMLSecurityDSig.php:614 (v 3.1.1) |
Try the fix I proposed above to see if it fixes things for you. I only had issues with encrypted assertions. When it was decrypted the XML tree would get messed up when treeCopyReplace() replaced the encrypted node with the decrypted version. |
Like i mentioned i have tried the fixed version (from the merge), but it did not resolve the issue :( |
In that case probably something different. Took me a good 18 hours of stepping through the code running it both under 7.4 and 8.1 to find the difference and thus my fix to my problem. Looks like your issue might be in the xmlseclibs library. |
PHP DOM maintainer here. |
@nielsdos , thanks for the update! |
The xmlDOMWrapReconcileNamespaces method we used to fix the namespace corruption issues in 8.1.21/8.2.8 caused regressions. Primarily, there is a similar corruption that the xmlReconciliateNs method used to have in which a namespace is suddenly shifted (SAML-Toolkits/php-saml#562) and the side-effect of removing redundant namespaces causes problems when a specific serialization is required. Closes GH-12308.
Thanks @nielsdos - looking forward to not needing the patch anymore. |
So this looks like it will be in the next release of PHP 8.x, missed the release that was put out today (2023-09-28) |
@yphoenix, should we detect at the code affected PHP versions and apply your patch only on those cases? |
That is an interesting idea, however, first I need to check that it is indeed fixed in 8.1.25 and then decided whether to work on a fix that is needed for PHP 8.1.21-8.1.24 (and appropriate 8.2 / 8.3 versions) or suggest that it is documented that they are broken and leave it at that. |
Issue seems to be resolved in PHP 8.2.12! |
Suspect it is also fixed in 8.1.25 but haven’t tested it yet. |
@yphoenix, do you thinhk you gonna have time to review the work to be done here? |
FWIW, this is indeed also fixed in 8.1.25, 8.2.12, 8.3+. |
Starting with the response......
It all gets decrypted just fine (8.1 & 7.4), ...
In particular the ds:Signature section..
Now all goes well until the decrypted XML is merged in to replace the encrypted XML in the
Utils::treeCopyReplace()
routineAfter which it becomes...
PHP 7.4
All OK, but PHP 8.1
The
<ds:...>
blocks become<ns3:...>
blocks and of course the signature then fails.... because the data that was signed has been mangled and become something else.Note: PHP 7.4, perfect, 8.1, not so much.
Thanks
The text was updated successfully, but these errors were encountered: