PHP 8.1 fixes in AttributeValue are broken

Question

PHP 8.1 fixes in AttributeValue are broken

tommy2d opened this issue 2 years ago · 17 comments

The __serialize and __unserialize additions in AttributeValue are causing fatal errors when simplesaml is used with the Dutch Digid IDP:

: PHP Fatal error: Uncaught TypeError: SAML2\XML\saml\AttributeValue::getElement(): Return value must be of type DOMElement, null returned

I can work around this issue by commenting out both the __serialize() and __unserialize() implementations, falling back to default PHP behavior.

How to proceed? does it help to provide serialized session contents for both the patched and the unpatched situation?

Answer 1 · 2022-05-23T15:26:57.000Z

I'm not sure what you mean by session contents and I fail to see the relation between the TypeError and the magic methods..
Are you running v4.6.0 of this library and can you give us a full stack trace?

Answer 2 · 2022-05-23T17:14:30.000Z

The type errors are caused by $element never being set (as part of the deserialization / serialization routine). Yes, I am using tag v4.6.0 (46d93da). This is the stack trace:

==> /var/log/nginx/ssl-brightfish-sharedservices-tvs-proxy.error.log <== 2022/05/23 19:13:32 [error] 13220#13220: *49603 FastCGI sent in stderr: "PHP message: PHP Fatal error: Uncaught TypeError: SAML2\XML\saml\AttributeValue::getElement(): Return value must be of type DOMElement, null returned in /mnt/sharedservices-code/tvs/vendor/simplesamlphp/saml2/src/SAML2/XML/saml/AttributeValue.php:72 Stack trace: #0 /mnt/sharedservices-code/tvs/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/Session.php(241): SAML2\XML\saml\AttributeValue->getElement() #1 [internal function]: SimpleSAML\Session->unserialize() #2 /mnt/sharedservices-code/tvs/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/Store/SQL.php(337): unserialize() #3 /mnt/sharedservices-code/tvs/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/SessionHandlerStore.php(55): SimpleSAML\Store\SQL->get() #4 /mnt/sharedservices-code/tvs/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/Session.php(348): SimpleSAML\SessionHandlerStore->loadSession() #5 /mnt/sharedservices-code/tvs/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/Session.php(263): SimpleSAML\Session::getSession()

Answer 3 · 2022-05-23T17:19:28.000Z

The problem is 100% reproducible for me when authenticating against ToegangsVerleningsService(TVS), and it seems to happen as part of the Unserialization of ActingSubjectID.

Answer 4 · 2022-05-23T17:41:31.000Z

My best guess by looking at this is that it breaks when AtributeValue contains an XML-structure instead of SimpleContent.
The way they changed serialization in PHP between 7.4 and 8.1 is absolutely horrible and I'm not sure if we can fix this in a way we can support all those versions at the same time.

Answer 5 · 2022-05-23T17:51:17.000Z

The scenario's are covered by a unit test though: https://github.com/simplesamlphp/saml2/blob/release-4.x/tests/SAML2/XML/saml/AttributeValueTest.php#L94-L118

Answer 6 · 2022-05-31T13:38:54.000Z

I can confirm that the test case works for me.

Answer 7 · 2022-05-31T16:30:23.000Z

I think I understand what's going on here.. What happens if you set the element-property to public instead of the current private?

Answer 8 · 2022-05-31T21:54:46.000Z

It doesn't change a thing I'm afraid. Same error, even after cleaning the sqlite db.

Answer 9 · 2022-05-31T22:01:09.000Z

I don't see what a database has to do with serializing xml, but unfortunately that leaves me with no clues again..
Can you get us an actual AuthnResponse that triggers this?

@thijskh You have any clue here?

Answer 10 · 2022-05-31T22:09:38.000Z

The serialized attributeValue is stored in sqlite (SimpleSAMLphp_kvstore) right?

none-working (stock) version:

a:1:{i:0;O:29:"SAML2\XML\saml\AttributeValue":1:{i:0;s:2716:"<saml:AttributeValue xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:soapenv="http://schemas.xmlsoap.org/soap/envelope/" xmlns:xenc="http://www.w3.org/2001/04/xmlenc#" xmlns:ds="http://www.w3.org/2000/09/xmldsig#" xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"><saml:EncryptedID xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion"><xenc:EncryptedData xmlns:xenc="http://www.w3.org/2001/04/xmlenc#" Id="_df46efefd4d8e83b9f352be544afb3e1-1" Type="http://www.w3.org/2001/04/xmlenc#Element"><xenc:EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#aes256-cbc"/><ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#"><ds:RetrievalMethod Type="http://www.w3.org/2001/04/xmlenc#EncryptedKey" URI="#_9e23b6bd3fb43d820553fc88230c5901-1"/></ds:KeyInfo><xenc:CipherData><xenc:CipherValue>/ser96syVNo+mPtHCvYg2oqs7cPgGzczehq8+DQB08gy19RjCEat6YCRSDrilpcRw+lgqyEcr1zJKeU0i84tlGrUdfJ5uWND5H9+FQclLWD36BBtJ4GfsyzSADFfKcu4R6MubZpTxAdDApTopAt8+ZAy9PV9QVWvDZntfzTlise2v9pnTs1xWHmBAO1AmsIoZyFwaoZQUgJrsp2gyuXRTbM4dL96naxj3T0vpPsCUHbNUx1Vw7JFuEZn/JW98gab429w4zM+OY6rPdwwSfAyLq+zGFbxOd33ZihlQHSGLQU=</xenc:CipherValue></xenc:CipherData></xenc:EncryptedData><xenc:EncryptedKey xmlns:xenc="http://www.w3.org/2001/04/xmlenc#" Id="_9e23b6bd3fb43d820553fc88230c5901-1" Recipient="urn:nl-eid-gdi:1.0:DV:00000009900006840000:entities:9780"><xenc:EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#rsa-oaep-mgf1p"><ds:DigestMethod xmlns:ds="http://www.w3.org/2000/09/xmldsig#" Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/></xenc:EncryptionMethod><ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#"><ds:KeyName>_b420654655d491b49555c698f80efb7bda3ac6ef</ds:KeyName></ds:KeyInfo><xenc:CipherData><xenc:CipherValue>xcF7/aS3RW0yGEiG1mfDSAmqhwLWpWC8msGp2vlaHsXHpBrDbQb4NYE7AQ/t9cRbDDvmsZmwIZe2B7oMUPz1jhHymmQqJ7WTJOBc7V04zbA/cuzgkxa0gSIPoTHkrRhF7bEvDkkTTZn+X+U2QdSFZc+AB8dIvc83hc0hvgtE1hQUlAzgnw3FO0lnQuVTDI57j9mJqyjfHaN3BqusSqmwuOcZsN+cx9fbWsyFznRlUxoK9Sj5Sn0xuc3Qg5IBjDWTB8y8Ab+vLp3DlNjGfEQIpg9mzbKaMf2fN4Dx7GUya7tdOR5XBkLmu58+opsKb73t5OgjBEdo4q+ETW8fe9w4nRhFD6skRSFA5XyIc0iF3YsJkCfxbUt9BefjwuKAbg8T4FBxyao7kQckfl6k5+wcyX6RQ6wK23orrihgNQCfvvSzmsivtArpXwB7kg4M/H1DUeZYLjh3xrVPZ37wrcwc9fnudKUgzkWjTAc8/si6PqL4LWb35Qk3LqwkdX+dy4qYSvawOa9eqjtalAJYFjYbhAb4csQUm10KFnFNTy2SJRXkf6If2k38UfFJok0cN2RTpaSrn1O1U0BMmhLh3K/bdSmWT6lVGgr7OXdhLfADAqu7BORI/gpFMsDhs41ul0i1vv6hBa0ng/1B2BR4qo1yCaerYV0u0+oF0K5nd5xxFI0=</xenc:CipherValue></xenc:CipherData><xenc:ReferenceList><xenc:DataReference URI="#_df46efefd4d8e83b9f352be544afb3e1-1"/></xenc:ReferenceList></xenc:EncryptedKey></saml:EncryptedID></saml:AttributeValue>";}}

v.s. (working, with the serialization/deserilization functiones removed in AttributeValue):

a:1:{i:0;C:29:"SAML2\XML\saml\AttributeValue":2726:{s:2716:"<saml:AttributeValue xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:soapenv="http://schemas.xmlsoap.org/soap/envelope/" xmlns:xenc="http://www.w3.org/2001/04/xmlenc#" xmlns:ds="http://www.w3.org/2000/09/xmldsig#" xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"><saml:EncryptedID xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion"><xenc:EncryptedData xmlns:xenc="http://www.w3.org/2001/04/xmlenc#" Id="_4ea05f00adb06c642e0cb52f063e2570-1" Type="http://www.w3.org/2001/04/xmlenc#Element"><xenc:EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#aes256-cbc"/><ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#"><ds:RetrievalMethod Type="http://www.w3.org/2001/04/xmlenc#EncryptedKey" URI="#_dc9043a7cbec55c6fcc61f1cf64cf868-1"/></ds:KeyInfo><xenc:CipherData><xenc:CipherValue>vErnRkA0oSmtQGamjZGa9RFN25SUx1UVLsLAOtopt7pyywTD7wu9pyocfD4HqduXCsvaiZpJykz11utZdvtJ0sOdm9oE+lAtNTUnKzGSNoSopGCzwNu5pqwhIEvWEWeilmJayAC2elpRYOnUs/rePxibz0Wbqa7BItLt6ZkKTtMkv0U0PpgGenF1pWzsahRtw6Y5tFq7xFQkG/z0Lz5rJ+IxExYXgB3LN6FBmVcB1ioahk2ovOwbLQ+lNAdqUMhpZx6fgdL2v7g4OYPK0rDgSALU3gU3dvU4hC/Kk9N5Rkw=</xenc:CipherValue></xenc:CipherData></xenc:EncryptedData><xenc:EncryptedKey xmlns:xenc="http://www.w3.org/2001/04/xmlenc#" Id="_dc9043a7cbec55c6fcc61f1cf64cf868-1" Recipient="urn:nl-eid-gdi:1.0:DV:00000009900006840000:entities:9780"><xenc:EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#rsa-oaep-mgf1p"><ds:DigestMethod xmlns:ds="http://www.w3.org/2000/09/xmldsig#" Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/></xenc:EncryptionMethod><ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#"><ds:KeyName>_b420654655d491b49555c698f80efb7bda3ac6ef</ds:KeyName></ds:KeyInfo><xenc:CipherData><xenc:CipherValue>d5X+psMTEy9DWDoVotB5sPHdNpofv5BdPUleflhGfjbjGIfWbV9fK+jMkQ4cqwSWwmTGSQ8OO+lYA9IwZasnWygAu3dSSlYd+sd/m2waz83MrTBsTtZUzwy8N18tNMu4xB/tb45XPvis9agg6b2RdpDSS1m5BKK+MMKgX2ZYIqOW6cbXxl73YJHHHpcTi1TsI+tx6DLNWB2ku5wpS3cFB3c0Tws9qG+YBwTzfp1gFhzvBQDNxLWVjPKEWEw5Do1dp8f7jOPWq0sscz6DaTJH3RStwOSLF2vsayGZUgqNXyPHX12dFNGHiTXEWVYcuXsqG8sXJQyUmE5kYqN/D8NT1BIq50L6xIKkQGxNSlyAAkvkV87b1z6X2Fvk6Hvmx+eqauJX/BKknqgNZL/I5xfysDcNG5i0vA/kOVai4LvmdEoUSq1dIWcGsiW5pbM13R+DddkUegGcZctmFLcafBM2A3WztGclLWSJea+hh24YurcRGQVoOaZcYRmGVA0cYRQjfcSb3mvGxl01I9f27Hzv6/pXMWmEetpNhO4Pxy4GIwatJ7clW+s679f6N4+qepzbnxqjMLOmRPilJ0t9+wHYQ9wc2+NN4lssLNNz852rb5Rs9HDTvOyvtV7ew4HdMqGjXeJt545pQzSonuzCVVs67Nuhi/rmOktsav8tP0N8izE=</xenc:CipherValue></xenc:CipherData><xenc:ReferenceList><xenc:DataReference URI="#_4ea05f00adb06c642e0cb52f063e2570-1"/></xenc:ReferenceList></xenc:EncryptedKey></saml:EncryptedID></saml:AttributeValue>";}}

The first difference that I notice is C versus O.

Answer 11 · 2022-06-01T05:26:45.000Z

Ah, you're using SQLite for session storage? I didn't get that from the issue description..
The C/O difference appears to be the only difference here.. I'll try and investigate some more

Answer 12 · 2022-06-01T08:12:00.000Z

I'm indeed using sqlite for session storage. But this is only to make it easier to access the serialized data: the issue exists on all session storage providers. Let me know if I can help in any way.

Answer 13 · 2022-06-08T06:43:26.000Z

We have the exact same issue. Re-naming the __serialize and __unserialize function to something random fixed the problem. We're still running PHP 7.4 (will upgrade to 8.1 soon).

If this bug was indeed introduced by #292 then it would be introduced in version 4.5.0.

We will revert to 4.4.1.

Could it be that this is specific to EncryptedID attribute values? There is no specific test for that.

Answer 14 · 2022-06-08T08:52:19.000Z

~~That could be part of the problem, yes.. When I unserialize, the value is that of the CipherValue instead of the entire xml-structure.~~

Update: I was able to narrow it down to the constructor of AttributeValue.. I think for some reason Utils::copyElement is not doing what we want it to do; 8a7d72e

Answer 15 · 2022-06-08T15:30:04.000Z

Interesting. I just mirrored the branch onto our gitlab environment so I will be able to test. Keep me posted in case I need to perform additional tests using this branch.

Answer 16 · 2022-06-26T07:51:12.000Z

I tried to adjust the php81_issue branch to create a failing test, as a start point for a fix. But I don't really understand where it is broken.

In this test:

    public function testSerializeEncryptedID() : void
    {
        $element = new \DOMDocument();
        $element->loadXML(<<<ATTRIBUTEVALUE
<saml:EncryptedID xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion"><xenc:EncryptedData xmlns:xenc="http://www.w3.org/2001/04/xmlenc#" Id="_4ea05f00adb06c642e0cb52f063e2570-1" Type="http://www.w3.org/2001/04/xmlenc#Element"><xenc:EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#aes256-cbc"/><ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#"><ds:RetrievalMethod Type="http://www.w3.org/2001/04/xmlenc#EncryptedKey" URI="#_dc9043a7cbec55c6fcc61f1cf64cf868-1"/></ds:KeyInfo><xenc:CipherData><xenc:CipherValue>vErnRkA0oSmtQGamjZGa9RFN25SUx1UVLsLAOtopt7pyywTD7wu9pyocfD4HqduXCsvaiZpJykz11utZdvtJ0sOdm9oE+lAtNTUnKzGSNoSopGCzwNu5pqwhIEvWEWeilmJayAC2elpRYOnUs/rePxibz0Wbqa7BItLt6ZkKTtMkv0U0PpgGenF1pWzsahRtw6Y5tFq7xFQkG/z0Lz5rJ+IxExYXgB3LN6FBmVcB1ioahk2ovOwbLQ+lNAdqUMhpZx6fgdL2v7g4OYPK0rDgSALU3gU3dvU4hC/Kk9N5Rkw=</xenc:CipherValue></xenc:CipherData></xenc:EncryptedData><xenc:EncryptedKey xmlns:xenc="http://www.w3.org/2001/04/xmlenc#" Id="_dc9043a7cbec55c6fcc61f1cf64cf868-1" Recipient="urn:nl-eid-gdi:1.0:DV:00000009900006840000:entities:9780"><xenc:EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#rsa-oaep-mgf1p"><ds:DigestMethod xmlns:ds="http://www.w3.org/2000/09/xmldsig#" Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/></xenc:EncryptionMethod><ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#"><ds:KeyName>_b420654655d491b49555c698f80efb7bda3ac6ef</ds:KeyName></ds:KeyInfo><xenc:CipherData><xenc:CipherValue>d5X+psMTEy9DWDoVotB5sPHdNpofv5BdPUleflhGfjbjGIfWbV9fK+jMkQ4cqwSWwmTGSQ8OO+lYA9IwZasnWygAu3dSSlYd+sd/m2waz83MrTBsTtZUzwy8N18tNMu4xB/tb45XPvis9agg6b2RdpDSS1m5BKK+MMKgX2ZYIqOW6cbXxl73YJHHHpcTi1TsI+tx6DLNWB2ku5wpS3cFB3c0Tws9qG+YBwTzfp1gFhzvBQDNxLWVjPKEWEw5Do1dp8f7jOPWq0sscz6DaTJH3RStwOSLF2vsayGZUgqNXyPHX12dFNGHiTXEWVYcuXsqG8sXJQyUmE5kYqN/D8NT1BIq50L6xIKkQGxNSlyAAkvkV87b1z6X2Fvk6Hvmx+eqauJX/BKknqgNZL/I5xfysDcNG5i0vA/kOVai4LvmdEoUSq1dIWcGsiW5pbM13R+DddkUegGcZctmFLcafBM2A3WztGclLWSJea+hh24YurcRGQVoOaZcYRmGVA0cYRQjfcSb3mvGxl01I9f27Hzv6/pXMWmEetpNhO4Pxy4GIwatJ7clW+s679f6N4+qepzbnxqjMLOmRPilJ0t9+wHYQ9wc2+NN4lssLNNz852rb5Rs9HDTvOyvtV7ew4HdMqGjXeJt545pQzSonuzCVVs67Nuhi/rmOktsav8tP0N8izE=</xenc:CipherValue></xenc:CipherData><xenc:ReferenceList><xenc:DataReference URI="#_4ea05f00adb06c642e0cb52f063e2570-1"/></xenc:ReferenceList></xenc:EncryptedKey></saml:EncryptedID>
ATTRIBUTEVALUE
        );

        $av3 = new AttributeValue($element->documentElement);
        $this->assertEquals(
            $av3->getElement()->ownerDocument->saveXML(),
            unserialize(serialize($av3))->getElement()->ownerDocument->saveXML()
        );
    }

The test passes, because the ->saveXML() returns an empty xml string even without serialize. So it doesn't seem related to the serialization, right?

EDIT: I did find a way to test this, see PR: #299

Would be great to see this merged, so we can safely update to newer versions again :-)

Answer 17 · 2022-12-05T22:00:16.000Z

Was solved in v4.6.5