I'm still utterly confused by how
core/Cookie works in Matomo and I can't see any security benefit by "signing" the cookie and only lots of unused complexity by allowing arbitrary data to be deserialized.
I could only find it used in:
makeThirdPartyCookieUID()to save the
setLanguageForSessionto save the language code when anonymous user
IgnoreCookiefor unknown reasons (but only for deleting cookies)
SessionInitializerwhich seems completly unused
so simply allowing to store a dictionary of strings should be more than enough for all use cases and far simpler.
This is more of a rough proof of concept to get my idea across, but I think this could be simplified in Matomo 4.
I'll probably append this PR draft with more similar commits.
Maybe it's still there from when there was a hash of the token (or something like this) stored in the cookie? Not sure why it was done.
There is also my PR here https://github.com/matomo-org/matomo/pull/13301 which sets the domain correctly for the IgnoreCookie and disables the signature check for it to allow multiple matomo installations on the same domain to share it. Maybe these prs can be combined? If we can keep support for old 3rd party cookies and migrate them this would be great.
btw we probably still need to extract signed content because of existing cookies.
@Findus23 any chance you could make it still work for old existing cookies? I suppose it won't have a big benefit then as you'd need to keep the unserialise anyway ... although without the salt an attacker shouldn't be able to do much there but of course you could brute force the salt etc.
@tsteur Hasn't this PR been mostly superceeded by #13301?
I'm still not sure if this "signature" couldn't just be removed, because if users are allowed to modify the cookie, then it is useless and just makes debugging harder and unnecessarily unserializes user data.
And if users are not allowed to modify cookies and the security depends on it, then this needs to be replaced by an actual cryptographic signiture with an HMAC as used by e.g. Django (https://code.djangoproject.com/wiki/Signing) or flask sessions (https://flask.palletsprojects.com/en/1.1.x/quickstart/#sessions) instead of just
sha1(substr($content, 0, -40) . SettingsPiwik::getSalt()) which doesn't work e.g. because of https://en.wikipedia.org/wiki/Length_extension_attack.
@Findus23 I checked through cookie usage and I'm all for it. I'm only meaning existing cookies would break by changing it the way the PR is currently right? So we'd need to detect whether the cookie is signed or not signed.
@tsteur @Findus23 I think we only have to be careful with the ignore and third party (pk_uid) cookie. For the ignore cookie the content doesn't matter only its presence, so I think we can safely change the cookie code without any additional work. However, since the third party cookie contains the global visitorid we should make sure that we still support reading third party cookies and auto upgrade them to the new cookie code. This will still break setups where multiple Matomo installations share the cookie and one of it is running an old version, but I guess that is acceptable.
@Findus23 are you still planning to work on this one? We'd need to make sure to not break any existing cookies.
@tsteur Shouldn't it be enough to just set https://github.com/matomo-org/matomo/pull/13301/files#diff-7de787015af7507a7278689396b18f7dR86 to false by default. This way it still splits the signature, but discards it.
For BC I had to add back the previous
safe_unserialise. Otherwise some values would not be read correctly. Also for strings I added back the
base64_encode as otherwise the cookie might end up with random values since
$name=$value wouldn't work if eg
$value contained an
I suppose now the content used to be signed so nobody could easily simply inject any kind of serialised information. Considering it's using
safe_unserialise maybe it's fine to use it though?
Only unserialising if the string looks like it is serialised doesn't really help cause anyone could just make it look like that.
be great to review this one. Newly set cookies won't use
serialize anymore (but still
base64_encode when the content is not a number). Meaning the ignore cookie looks eg like
ignore=Kg== which is maybe not ideal but still better than before.
Existing content will be still read and use
safe_unserialize will also still be used if the cookie content was "signed" (for security). Only then we still use the
I did a basic test using old cookie format and new cookie format and things worked for me. Be great if more eyes could have a look at this.
If it's too risky or not too important we could also simply close the PR. No preference.
Code looks fine, doesn't seem like there would be any issues, BUT it might be nice to have this in an early beta and test it a bit more rather than include it in a big release like 4.0 w/ little testing. This isn't a breaking change either, so maybe it would be better to merge in 4.1?
I have no preference. If there's an issue later it might not be discovered that quickly. Happy to move to 4.1
Well, the change looks good to me, so I guess it's up to you whether to merge it or not.
I just realised the cookie::set method no longer accepts an array as value so added this to the developer changelog and we should merge it soon as it's a breaking change. I checked and it looks like we are only setting numbers and strings but no arrays. If needed people could always json_encode an array value themselves.
@sgiehl maybe you can have a quick look as well if you spot anything?