simplified cookies #14444

Findus23 · 2019-05-12T13:12:16Z

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 · 2019-05-12T20:12:28Z

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 · 2019-05-12T20:30:15Z

There is also my PR here #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 · 2019-10-02T23:28:16Z

@mattab do you maybe know more about this?

mattab · 2019-10-04T00:58:42Z

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 · 2020-07-01T01:49:31Z

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

tsteur · 2020-07-01T01:54:03Z

@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 · 2020-07-01T20:43:30Z

@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 · 2020-07-01T21:03:15Z

@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 · 2020-07-01T21:19:43Z

@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 · 2020-08-27T03:44:20Z

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

Findus23 · 2020-08-27T09:38:02Z

@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 · 2020-08-27T22:24:59Z

@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 · 2020-09-30T01:44:09Z

@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 · 2020-09-30T01:45:26Z

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

tsteur · 2020-11-01T20:28:40Z

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

diosmosis · 2020-11-01T22:14:40Z

core/Cookie.php

-            // sign cookie
-            $signature = sha1($cookieStr . SettingsPiwik::getSalt());
-            return $cookieStr . $signature;
+            $cookieStrArr[] = "$name=$value"; 


Is there a reason to not use json_encode? Don't think it really matters if this works, just wondering.

probably no particular reason I guess.

diosmosis · 2020-11-01T22:17:06Z

core/Cookie.php

-                $varValue = safe_unserialize($tmpValue);
+                if ($isSigned) {
+                    // only unserialise content if it was signed meaning the cookie was generated pre Matomo 4
+                    $varValue = safe_unserialize($tmpValue);


safe_unserialize uses our custom unserialize code which doesn't use unserialize at all, so I think this is fine? I guess it depends on how the cookie values are used to say if it's completely safe from injection. I guess we could also make sure there are only strings & numbers in the unserialized array to make it a little safer?

AFAIK the most risk re safe_unserialize would be from when PHP actually does the unserialize. This risk should be eliminated with the safe_unserialize (side note: I think since we now require PHP 7.2+ we could even use PHP unserialize with the additional parameter).

Not sure there's much benefit in checking it additionally. I guess with this PR people would need to know the general salt in order for this to be used so it shouldn't be much of an issue anymore anyway since newly signed cookies would no longer be signed

safe_unserialize is defined in /libs/upgradephp/upgrade.php (included in bootstrap.php), and it's our code. I don't think it's PHP API? So should be super safe, I think. Unless we, use a variable in an insecure way, like modifying a session variable based on the index we get from the cookie or something like that. This is what I meant at least w/ my comment.

Sorry I meant eventually we can use https://www.php.net/manual/en/function.unserialize.php like unserialize ($str, false) which I think should be the same as our safe_unserialize I reckon (but didn't fully check). PHP5.X didn't support the allowed_classes parameter

Imho it's fine to simply use Common::safe_unserialize everywhere. It automatically disallows all classes, if non where given. Also it catches all errors and logs them. Should be simpler than doing that manually everywhere again...

Just keep in mind that the safe_unserialize in Matomo is over 10 years old and might have some weird bugs in it. (The upgrade.php seems to be only intended for PHP_VERSION<5.2.

But it should be pretty safe as it only uses regex (unless someone finds an input value that creates a DOS).

Oh. sure. Thought that was the method from Common class 🙈 We shouldn't use the safe_unserialize from upgrade.php. Should be Common::safe_unserialize for sure!

Hm, now that only PHP7 is supported we might be able to remove that upgrade.php completely. I'll see what happens when I try.

I thought initially as well it could be removed but reading https://www.php.net/manual/en/function.unserialize.php and the warning it says clearly regardless of any options value it isn't safe for user input (which it may be here) so prefer to maybe keep the existing safe_unserialise usage here for now. It should be mostly only for previously created cookies anyway. Unless someone had the general salt then they could still create any user content for cookie and sign it and it would use unserialize.

diosmosis · 2020-11-01T22:31:57Z

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 · 2020-11-01T22:36:33Z

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

diosmosis · 2020-11-01T22:38:42Z

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

tsteur · 2020-11-02T00:24:10Z

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?

sgiehl

The test SessionInitializerTest::test_initSession_DeletesCookie_WhenAuthenticationFailed is currently failing. Not sure why, but if I see that correct, that test actually never worked correctly before. The faked auth_cookie does not have signed content and thus it was discarded before resulting in a working test. Now as this unsigned cookie content is handled as valid (again), the test starts failing as a setCookie actually does not erase the content in the cookie class. Guess sending a cookie using a Set-Cookie header might not update the $_COOKIE array, not sure if setCookie would do that directly

core/Cookie.php

Co-authored-by: Stefan Giehl <stefan@matomo.org>

Findus23 added 2 commits May 12, 2019 15:02

simplified cookies

37bd2fa

make hash example in ExampleSettingsPlugin less misleading

347cc78

mattab added this to the 3.11.0 milestone Jun 10, 2019

mattab added the Needs Review PRs that need a code review label Jun 10, 2019

mattab modified the milestones: 3.11.0, 3.12.0 Jun 27, 2019

MichaelRoosz mentioned this pull request Sep 20, 2019

Meta: Update Cookie Implementations #14904

Closed

tsteur modified the milestones: 3.13.0, 4.0.0 Nov 11, 2019

tsteur changed the base branch from 3.x-dev to 4.x-dev January 14, 2020 03:57

trigger build

3e40903

tsteur marked this pull request as ready for review July 1, 2020 01:44

tsteur added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Jul 1, 2020

tsteur added 4 commits August 28, 2020 09:55

Merge branch '4.x-dev' into simplifiedCookies

3d8054d

few tweaks

6836cee

better BC

7db73ea

make sure cookie exists

dfaf09e

tsteur modified the milestones: 4.0.0, 4.0.0 RC Sep 3, 2020

tsteur added 2 commits September 30, 2020 14:31

tweak logic

7d44a17

Merge branch '4.x-dev' into simplifiedCookies

a712d8e

tsteur added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Sep 30, 2020

add tests

59db423

diosmosis reviewed Nov 1, 2020

View reviewed changes

tsteur added 2 commits November 2, 2020 13:20

Merge branch '4.x-dev' into simplifiedCookies

eae05bd

update docs

069e702

sgiehl requested changes Nov 2, 2020

View reviewed changes

core/Cookie.php Outdated Show resolved Hide resolved

Update core/Cookie.php

16af3de

Co-authored-by: Stefan Giehl <stefan@matomo.org>

tsteur merged commit fd1d68c into 4.x-dev Nov 2, 2020

tsteur deleted the simplifiedCookies branch November 2, 2020 19:12

sgiehl mentioned this pull request Nov 2, 2020

don't install unneeded polyfills with composer #16653

Merged

This was referenced Nov 2, 2020

Fix build #16656

Closed

Fix build #16658

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplified cookies #14444

simplified cookies #14444

Findus23 commented May 12, 2019

tsteur commented May 12, 2019

MichaelRoosz commented May 12, 2019 •

edited

tsteur commented Oct 2, 2019

mattab commented Oct 4, 2019

tsteur commented Jul 1, 2020

tsteur commented Jul 1, 2020

Findus23 commented Jul 1, 2020

tsteur commented Jul 1, 2020

MichaelRoosz commented Jul 1, 2020 •

edited

tsteur commented Aug 27, 2020

Findus23 commented Aug 27, 2020

tsteur commented Aug 27, 2020

tsteur commented Sep 30, 2020 •

edited

tsteur commented Sep 30, 2020

tsteur commented Nov 1, 2020

diosmosis Nov 1, 2020

tsteur Nov 1, 2020

diosmosis Nov 1, 2020

tsteur Nov 1, 2020

diosmosis Nov 1, 2020

tsteur Nov 1, 2020

sgiehl Nov 2, 2020 •

edited

Findus23 Nov 2, 2020

sgiehl Nov 2, 2020

Findus23 Nov 2, 2020

tsteur Nov 2, 2020

diosmosis commented Nov 1, 2020

tsteur commented Nov 1, 2020

diosmosis commented Nov 1, 2020

tsteur commented Nov 2, 2020

sgiehl left a comment

simplified cookies #14444

simplified cookies #14444

Conversation

Findus23 commented May 12, 2019

tsteur commented May 12, 2019

MichaelRoosz commented May 12, 2019 • edited

tsteur commented Oct 2, 2019

mattab commented Oct 4, 2019

tsteur commented Jul 1, 2020

tsteur commented Jul 1, 2020

Findus23 commented Jul 1, 2020

tsteur commented Jul 1, 2020

MichaelRoosz commented Jul 1, 2020 • edited

tsteur commented Aug 27, 2020

Findus23 commented Aug 27, 2020

tsteur commented Aug 27, 2020

tsteur commented Sep 30, 2020 • edited

tsteur commented Sep 30, 2020

tsteur commented Nov 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgiehl Nov 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diosmosis commented Nov 1, 2020

tsteur commented Nov 1, 2020

diosmosis commented Nov 1, 2020

tsteur commented Nov 2, 2020

sgiehl left a comment

Choose a reason for hiding this comment

MichaelRoosz commented May 12, 2019 •

edited

MichaelRoosz commented Jul 1, 2020 •

edited

tsteur commented Sep 30, 2020 •

edited

sgiehl Nov 2, 2020 •

edited