@Findus23 opened this Pull Request on May 12th 2019 Member

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 $idVisitor
  • setLanguageForSession to save the language code when anonymous user
  • IgnoreCookie for unknown reasons (but only for deleting cookies)
  • SessionInitializer which seems completly unused
  • somewhere in TagManager, but only for setting "1" and null

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.

@tsteur commented on May 12th 2019 Member

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.

@MichaelRoosz commented on May 12th 2019 Contributor

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.

@tsteur commented on October 2nd 2019 Member

@mattab do you maybe know more about this?

@mattab commented on October 4th 2019 Member

Thanks @Findus23 for the PR!
@MichaelHeerklotz Be great to merge your PR and this one into one. Would you maybe be able to help with this PR?

@tsteur commented on July 1st 2020 Member

btw we probably still need to extract signed content because of existing cookies.

@tsteur commented on July 1st 2020 Member

@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.

so it would only really help if we break existing cookie values which is maybe a no go @Findus23 @mattab ?

@Findus23 commented on July 1st 2020 Member

@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.

@tsteur commented on July 1st 2020 Member

@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.

@MichaelRoosz commented on July 1st 2020 Contributor

@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.

@tsteur commented on August 27th 2020 Member

@Findus23 are you still planning to work on this one? We'd need to make sure to not break any existing cookies.

@Findus23 commented on August 27th 2020 Member

@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.

@tsteur commented on August 27th 2020 Member

@Findus23 @MichaelHeerklotz I've merged the latest 4.X changes into this PR and tweaked the code so it's no longer signing content but it should still be able to read previously signed content.

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 = etc.

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.

Any thoughts?

@tsteur commented on September 30th 2020 Member

@Findus23 @sgiehl @MichaelRoosz

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 safe_unserialize.

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.

@tsteur commented on September 30th 2020 Member

If it's too risky or not too important we could also simply close the PR. No preference.

@tsteur commented on November 1st 2020 Member

@Findus23 @sgiehl @diosmosis any thoughts on this one as the RC will be released soon?

@diosmosis commented on November 1st 2020 Member

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?

@tsteur commented on November 1st 2020 Member

I have no preference. If there's an issue later it might not be discovered that quickly. Happy to move to 4.1

@diosmosis commented on November 1st 2020 Member

Well, the change looks good to me, so I guess it's up to you whether to merge it or not.

@tsteur commented on November 2nd 2020 Member

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?

This Pull Request was closed on November 2nd 2020
Powered by GitHub Issue Mirror