simplesamlphp/saml2

PHP 8.1 fixes in AttributeValue are broken

tommy2d opened this issue · 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?

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?

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()

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

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.

I can confirm that the test case works for me.

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

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

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?

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.

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

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.

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.

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

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.

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 :-)

Was solved in v4.6.5