robrichards / xmlseclibs

A PHP library for XML Security
BSD 3-Clause "New" or "Revised" License
386 stars 180 forks source link

Reference validation failed after (minor!) PHP update from 8.2.7 to 8.2.8 #257

Open Navi2016 opened 10 months ago

Navi2016 commented 10 months ago

I'm running into an issue after updating my server from PHP 8.2.7 to 8.2.8.

Getting: Reference validation failed from vendor/robrichards/xmlseclibs/src/XMLSecurityDSig.php:614

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), which uses robrichards/xmlseclibs (3.1.1)

Everything works great on PHP 8.2.7 and before, no longer on 8.2.8 and up.

I have tried a possible fix in: https://github.com/SAML-Toolkits/php-saml/issues/562

But the author of onelogin/php-saml thinks the issue might be in xmlseclibs.

Any idea?

tvdijen commented 10 months ago

PHP has been messing a lot with the builtin XML/DOM-libraries and as an author of simplesamlphp/xml-security I've seen similar issues over the last few months.

Most notably:

This for me was my primary suspect, because in my case marshalling/unmarshalling XML documents led to different documents with namespace declarations in different places

Navi2016 commented 10 months ago

Well it seems this package no longer works in PHP 8.2.8 and up because of these changes. Pretty strange they would make these updates in a minor release.

When looking at $data in method validateDigest i do see a difference between PHP 8.2.7 and 8.2.8.

In 8.2.7 there are no prefixes in the <Assertion message, while in 8.2.8 every node in the <Assertion message has a saml: prefix.

afbeelding

So it does 2 passes, the first SAML response validates fine, it's the second pass validating the Assertion which does not validate.

Could this be the cause? How could this be fixed?

Update: When disabling assertion encryption on the idp (codegreencreative/laravel-samlidp) via encrypt_assertion false, it works. See also the related issue below. Still needs a fix though.

nielsdos commented 10 months ago

PHP DOM maintainer here. This is caused by a regression introduced by a bugfix in PHP. This is a behaviour change that's caused by an unintended side-effect of using a newer namespace API from libxml2. I have opened a PR to fix this at PHP's end by restoring the original behaviour.

Pretty strange they would make these updates in a minor release.

This was not an intentional behaviour change, and our tests didn't catch this. In any case, if you see such a sudden break during a minor upgrade, please also report this to PHP's bug tracker so we can help quicker. Thanks.

tvdijen commented 10 months ago

I think we've seen a similar thing around June/July this year, but these things are so hard to pinpoint.. Thanks for reaching out @nielsdos

nielsdos commented 10 months ago

I think we've seen a similar thing around June/July this year, but these things are so hard to pinpoint..

Yes, I found too that DOM bugs often occur when specific sequences of actions/tree changes are performed. So when they happen it's hard to find why and where. We had a regression in June I think with tree manipulations but that should be fixed. In any case, just let me know if you see something weird by opening a bug. I rather have a bug report too much than one too little :slightly_smiling_face:

tvdijen commented 10 months ago

We had a regression in June I think with tree manipulations but that should be fixed.

Yes, it was fixed.. But tests failing and by the time I had figured out the issue was PHP, it was fixed with a bugfix release. I never got to file a bug... My issue was with a different library though, so let's not hijack this issue with PHP bugs ;)

Navi2016 commented 8 months ago

Issue seems to be resolved in PHP 8.2.12!

tvdijen commented 8 months ago

From the changelog:

Version 8.2.12 26 Oct 2023

[..]

DOM:
    Restore old namespace reconciliation behaviour.

Makes sense