@diosmosis opened this Pull Request on September 6th 2018 Member

Cause of duplicate headers is due to following workflow:

  1. User uses logme API endpoint, w/o having a session already started.
  2. FrontController calls Session::start() which notices there's no session currently and creates one.
  3. PHP calls setcookie() w/ session id resulting in one set-cookie header.
  4. logme() authenticates user & SessionInitializer regenerates the session ID.
  5. PHP calls setcookie() to change the session ID. This results in a new set-cookie header (it doesn't change the existing one).
  6. the response contains 2 set-cookie headers

Fixed by introducing new hidden event Session.shouldStartSession. The Login plugin does not start a session for logme(). Then in SessionInitializer, instead of calling Session::regenerateId() (which will have no effect if the session is not started), we call Session::start() if the session hasn't been started yet, and Session::regenerateId() only if there is already an existing session.

Fixes #13393

@tsteur commented on September 16th 2018 Member

@diosmosis things around session and authentication are starting to get a bit complicated... I presume there is no simpler solution to this?

@diosmosis commented on September 16th 2018 Member

Simplest solution would be to do nothing, AFAIK browsers handle multiple set-cookie headers correctly. This is perhaps more of an issue for some kind of automation.

Otherwise since PHP will always send multiple set-cookie headers in this case, only solution is to avoid regenerating the session ID before the session is started for the first time.

@tsteur commented on September 17th 2018 Member

Just to understand... what triggers the 2nd set-cookie? Is it session_start being called twice? or session_start and then session_regenerate_id? If it is the session_regenerate_id then we maybe do nothing indeed... it is eg even in the PHP examples: http://us2.php.net/manual/en/function.session-regenerate-id.php

@diosmosis commented on September 17th 2018 Member

The 2nd set-cookie ONLY happens when there's no session (ie, no session cookie), and we call session_start() and session_regenerate_id() in the same request. The first function creates a session and adds a set-cookie w/ the session ID. The second call creates another session, but php can't edit the old set-cookie, so it adds another one.

EDIT: when there's already a session (ie, there's a session ID in the cookie), session_start() doesn't need to issue a set-cookie call. So it's fine in that case.

@tsteur commented on September 17th 2018 Member

I've just had a look how symfony does it. Can maybe session_status help to decide if we need to regenerate a session or not (https://github.com/symfony/http-foundation/blob/0daf24b01a3586fdfa22d4bfd649f22803908a0a/Session/Storage/NativeSessionStorage.php#L202)? By looking at the code it might actually not help as it only stops from regenerating if no session exists...

I'm tempted to simply send two cookies as browsers handle it fine. Unless there was actually a bug or problem.

@diosmosis commented on September 17th 2018 Member

I think that would replace the cookie check? ie instead of checking for the cookie, just calling session_status() === SESSION_NONE. I don't think it would remove the need for a new event and not starting the session for logme, etc. Actually maybe session_status() will return SESSION_NONE even if the cookie exists, but session_start() hasn't been called...

I guess we could do this: In SessionInitializer, do not regenerate ID if there is no session cookie. That would prevent two set-cookies (I think). Not sure it's a security issue, but since this all happens in one request, perhaps it's not a big deal.

@tsteur commented on September 17th 2018 Member

I guess we could do this: In SessionInitializer, do not regenerate ID if there is no session cookie. That would prevent two set-cookies (I think). Not sure it's a security issue, but since this all happens in one request, perhaps it's not a big deal.

I haven't dealt with sessions in a while... from what I remember we should always call session_regenerate_id when calling session_start and there was no session before... otherwise an attacker could use session fixation etc. I wonder if two session cookies are basically unavoidable?

@diosmosis commented on September 17th 2018 Member

In this case there's no session ID in the first request, so the session created would be the first ID. If an attacker can get this, then I guess it should be regenerated, but if session fixation is only possible w/ a pre-existing unauthenticated session, then it should be fine in this case?

@tsteur commented on September 17th 2018 Member

If an attacker can get this, then I guess it should be regenerated, but if session fixation is only possible w/ a pre-existing unauthenticated session, then it should be fine in this case?

I guess it really depends on the implementation but would need to think into it again. Basically yes if you create a session without regenerating an ID, there is possibly a security problem as the attacker would eg do &action=logme&PIWIK_SESSID=1234567.

From what I understand so far is, that we start the session because we don't know if we will use logme later as this would cause a regenerate_id and therefore a 2nd cookie... The regenerate_id on logme always needs to happen no matter if session exists or not I would say. Isn't it the same behaviour on the login page?

@matzesa was there a specific problem by having two set cookie headers?

@diosmosis commented on September 17th 2018 Member

The regenerate_id on logme always needs to happen no matter if session exists or not I would say. Isn't it the same behaviour on the login page?

Yes & no. The logic on the login page is split over multiple requests. The first request shows the page and starts a session. Then the next request logs in, which regenerates the session ID.

The problem is that logme() combines those steps, so it will start the session and then right after regenerate the ID, within the same request. Which results in two headers.

@matzesa commented on September 17th 2018

@matzesa was there a specific problem by having two set cookie headers?

We use an proxy Tool in our CMS to easily login user in Matomo. This stopped working with Matomo 3.6.0. I investigated the problem again and found, that not the doubled "Set-Cookie"-Header was the problem. It seems that Matomo missed some request header(s). I now copy all headers from the client request and it is working again. Sorry for the inconvenience.

@tsteur commented on September 17th 2018 Member

Cheers @matzesa
@diosmosis I will close this PR and issue.

This Pull Request was closed on September 17th 2018
Powered by GitHub Issue Mirror