Bug 25360 - Use secure flag for CGISESSID cookie
Summary: Use secure flag for CGISESSID cookie
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Keywords:
: 19571 25715 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-05-03 23:49 UTC by David Cook
Modified: 2020-09-03 08:43 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03, 19.11.09, 19.05.14


Attachments
Bug 25360: Use secure flag for CGISESSID cookie when using HTTPS (4.65 KB, patch)
2020-05-04 02:20 UTC, David Cook
Details | Diff | Splinter Review
Bug 25360: [Follow-up] Test for "on" or "ON" value for HTTPS env var (7.15 KB, patch)
2020-05-07 00:29 UTC, David Cook
Details | Diff | Splinter Review
Bug 25360: [Follow-up] Test for "on" or "ON" value for HTTPS env var (7.15 KB, patch)
2020-05-07 00:30 UTC, David Cook
Details | Diff | Splinter Review
Bug 25360: [Follow-up] Fix typo in C4/InstallAuth.pm (845 bytes, patch)
2020-05-11 00:50 UTC, David Cook
Details | Diff | Splinter Review
Bug 25360: Use secure flag for CGISESSID cookie when using HTTPS (4.65 KB, patch)
2020-06-23 06:25 UTC, David Cook
Details | Diff | Splinter Review
Bug 25360: [Follow-up] Test for "on" or "ON" value for HTTPS env var (7.16 KB, patch)
2020-06-23 06:25 UTC, David Cook
Details | Diff | Splinter Review
Bug 25360: [Follow-up] Fix typo in C4/InstallAuth.pm (845 bytes, patch)
2020-06-23 06:25 UTC, David Cook
Details | Diff | Splinter Review
Bug 25360: Use secure flag for CGISESSID cookie when using HTTPS (7.36 KB, patch)
2020-08-05 14:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 25360: (follow-up) Remove the https FIXME in Auth.pm (3.32 KB, patch)
2020-08-05 14:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 25360: Use secure flag for CGISESSID cookie when using HTTPS (7.39 KB, patch)
2020-08-13 08:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25360: (follow-up) Remove the https FIXME in Auth.pm (3.35 KB, patch)
2020-08-13 08:02 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2020-05-03 23:49:46 UTC
We should consider setting the "secure" flag for the CGISESSID cookie (https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Secure_and_HttpOnly_cookies), so that it can only be sent  to the server from the browser over HTTPS.
Comment 1 David Cook 2020-05-03 23:54:04 UTC
I'm debating with myself how best to implement it. 

On one hand, requiring a cookie to be sent over HTTPS could make legitimate automated testing harder/impossible, and not everyone necessarily has access to HTTPS (although the latter is less common all the time).

Here's some thoughts:

1) Use a system preference to force it

2) Try reading $type and relevant *BaseURL system preference to determine whether a HTTP or HTTPS URL is defined (since we can't reliably determine HTTP vs HTTPS for proxied connections unless we used a header like X-Forwarded-Proto)

That's about it for ideas right now. But open to other people's ideas.
Comment 2 David Cook 2020-05-04 00:53:47 UTC
Actually, it looks like Bug 21267 adds support for X-Forwarded-Proto for Plack-enabled Koha. 

I think for now I'll write this patch using $ENV->{HTTPS}, which will only work for Plack-enabled Koha, and I'll use Bug 25361 to add support for X-Forwarded-Proto to CGI Koha...
Comment 3 David Cook 2020-05-04 02:20:48 UTC
Created attachment 104244 [details] [review]
Bug 25360: Use secure flag for CGISESSID cookie when using HTTPS

This patch adds the secure flag to the CGISESSID cookie when using HTTPS.
This prevents the cookie being used again over a normal HTTP
request.
Comment 4 David Cook 2020-05-04 02:27:57 UTC
(In reply to David Cook from comment #2)
> Actually, it looks like Bug 21267 adds support for X-Forwarded-Proto for
> Plack-enabled Koha. 
> 
> I think for now I'll write this patch using $ENV->{HTTPS}, which will only
> work for Plack-enabled Koha, and I'll use Bug 25361 to add support for
> X-Forwarded-Proto to CGI Koha...

On a CGI Koha, I just added the following to Apache:

SetEnvIf X-Forwarded-Proto "https" HTTPS=on

Of course, I also had to set X-Forwarded-Proto on my reverse proxy in order to get it to work.
Comment 5 David Cook 2020-05-04 02:29:01 UTC
Test Plan:

1) Apply patch
2) Login to your Koha site using HTTPS
3) Note a successful login
4) Login to your Koha site using HTTP
5) Note that your session isn't recognized when using HTTP
6) Reload the page on your HTTPS Koha
7) Note that your session is still recognized when using HTTPS
Comment 6 Jonathan Druart 2020-05-05 07:17:30 UTC
*** Bug 19571 has been marked as a duplicate of this bug. ***
Comment 7 David Cook 2020-05-06 23:51:45 UTC
Annnd my patch is buggy, because HTTPS "OFF" will turn on the secure cookie...
Comment 8 David Cook 2020-05-07 00:01:51 UTC
(In reply to David Cook from comment #7)
> Annnd my patch is buggy, because HTTPS "OFF" will turn on the secure
> cookie...

Ahhh it's because of CGI::Emulate::PSGI (https://metacpan.org/release/CGI-Emulate-PSGI/source/lib/CGI/Emulate/PSGI.pm). Neither Apache nor other reverse proxies I use are setting the HTTPS environmental variable. It's CGI::Emulate::PSGI. So it's only an issue for non-SSL Plack-enabled instances. I'll just fix that...
Comment 9 David Cook 2020-05-07 00:29:07 UTC
Created attachment 104469 [details] [review]
Bug 25360: [Follow-up] Test for "on" or "ON" value for HTTPS env var

This patch tests for HTTPS "on" or "ON" before setting the secure
cookie.
Comment 10 David Cook 2020-05-07 00:30:30 UTC
Created attachment 104470 [details] [review]
Bug 25360: [Follow-up] Test for "on" or "ON" value for HTTPS env var

This patch tests for HTTPS "on" or "ON" before setting the secure
cookie.
Comment 11 David Cook 2020-05-11 00:50:34 UTC
Created attachment 104642 [details] [review]
Bug 25360: [Follow-up] Fix typo in C4/InstallAuth.pm
Comment 12 Katrin Fischer 2020-06-17 07:32:10 UTC
*** Bug 25715 has been marked as a duplicate of this bug. ***
Comment 13 Martin Renvoize 2020-06-22 12:14:21 UTC
Soory David.. I'm getting the dreaded 

error: sha1 information is lacking or useless (C4/Auth.pm).
error: could not build fake ancestor

when applying this patchset.
Comment 14 David Cook 2020-06-23 06:23:55 UTC
(In reply to Martin Renvoize from comment #13)
> Soory David.. I'm getting the dreaded 
> 
> error: sha1 information is lacking or useless (C4/Auth.pm).
> error: could not build fake ancestor
> 
> when applying this patchset.

That's weird, since I was able to apply it just fine, although it did have 1 merge conflict.

In any case, I'll re-post with the merge conflict fixed!
Comment 15 David Cook 2020-06-23 06:25:11 UTC
Created attachment 106176 [details] [review]
Bug 25360: Use secure flag for CGISESSID cookie when using HTTPS

This patch adds the secure flag to the CGISESSID cookie when using HTTPS.
This prevents the cookie being used again over a normal HTTP
request.
Comment 16 David Cook 2020-06-23 06:25:15 UTC
Created attachment 106177 [details] [review]
Bug 25360: [Follow-up] Test for "on" or "ON" value for HTTPS env var

This patch tests for HTTPS "on" or "ON" before setting the secure
cookie.
Comment 17 David Cook 2020-06-23 06:25:18 UTC
Created attachment 106178 [details] [review]
Bug 25360: [Follow-up] Fix typo in C4/InstallAuth.pm
Comment 18 David Cook 2020-06-23 06:27:56 UTC
I don't actually love these patches, but they have been working so far. 

That said, the cookie creation is begging for some refactoring...
Comment 19 Tomás Cohen Arazi 2020-08-03 20:43:05 UTC
As I understand it, the 'SameSite=None; Secure' setup requires SSL for cross-site cookie access. This means it shouldn't cause problem, normally. So I would have a syspref for controlling wether we want to be 'Lax' instead. So if you want to be able to cross-site access the CGISESSID cookie from an 'insecure' site, we would set

  -sameorigin => 'Lax'
  -secure     => 0

If you want to refuse them, we would set

  -sameorigin => 'Strict'
  -secure     => 1

This is what literature pointed me to when I dug on a solution for bug 26019.
Comment 20 David Cook 2020-08-04 00:17:29 UTC
(In reply to Tomás Cohen Arazi from comment #19)
> As I understand it, the 'SameSite=None; Secure' setup requires SSL for
> cross-site cookie access. This means it shouldn't cause problem, normally.
> So I would have a syspref for controlling wether we want to be 'Lax'
> instead. So if you want to be able to cross-site access the CGISESSID cookie
> from an 'insecure' site, we would set
> 
>   -sameorigin => 'Lax'
>   -secure     => 0
> 
> If you want to refuse them, we would set
> 
>   -sameorigin => 'Strict'
>   -secure     => 1
> 
> This is what literature pointed me to when I dug on a solution for bug 26019.

Were you reading https://web.dev/samesite-cookies-explained/? I think that you may have misunderstood it.

As far as I can tell "Secure" is only *required* when SameSite=None, but SameSite=None is only used for third-party tracking cookies, which we wouldn't be using.

You'd want to use SameSite=Strict for cookies that are just used for internal functionality in the site (like the OPAC cart), but that shouldn't require SSL/Secure (even though it's a best practice).

With an authentication session cookie like CGISESSID... I think the norm would be to use SameSite=Lax so that the cookie is sent when navigating from other sites or from entering the URL by hand in the address bar of the browser. Otherwise, if you use SameSite=Strict, and you type in https://mykoha.mykoha, it will prompt you to log in even if you've already logged in - I believe.

It would be worthwhile testing those above statements though. It seems like different websites articulate things differently. For instance, https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite makes it seem like third parties can send your cookie, but https://www.thinktecture.com/en/identity/samesite/samesite-in-a-nutshell/ seems to indicate that they can't (except for navigation purposes). 

In any case, I don't think SameSite is relevant for this bug. This bug is just for the Secure attribute, which is separate.

I suppose Bug 26019 might want some way of forcing the Secure flag if SameSite=None is done though... although that seems unlikely
Comment 21 Marcel de Rooy 2020-08-05 13:38:23 UTC
I expected to find SameSite defaulting to Lax and Secure is true, but what I see now is SameSite=None and Secure=true ?
Comment 22 Marcel de Rooy 2020-08-05 13:39:33 UTC
(In reply to Marcel de Rooy from comment #21)
> I expected to find SameSite defaulting to Lax and Secure is true, but what I
> see now is SameSite=None and Secure=true ?

For CGISESSID cookie to be complete in Firefox 79.0
Comment 23 Marcel de Rooy 2020-08-05 13:46:37 UTC
Should set network.cookie.sameSite.laxByDefault in FF about:config
Comment 24 Marcel de Rooy 2020-08-05 14:08:56 UTC
Created attachment 107833 [details] [review]
Bug 25360: Use secure flag for CGISESSID cookie when using HTTPS

This patch adds the secure flag to the CGISESSID cookie when using HTTPS.
This prevents the cookie being used again over a normal HTTP
request.

Bug 25360: [Follow-up] Test for "on" or "ON" value for HTTPS env var

This patch tests for HTTPS "on" or "ON" before setting the secure
cookie.

Bug 25360: [Follow-up] Fix typo in C4/InstallAuth.pm

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Amended number of tests in Context.t
Comment 25 Marcel de Rooy 2020-08-05 14:09:05 UTC
Created attachment 107834 [details] [review]
Bug 25360: (follow-up) Remove the https FIXME in Auth.pm

The FIXME is no longer valid since we fixed the X-Forwarded headers
for Plack. And since we do not even use using_https anymore in
the templates (see bug 21094).

Test plan:
Run Auth.t
Git grep for using_https

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2020-08-05 14:17:31 UTC
Taking the liberty to promote this patch to PQA. If the RM feels we need another QAer to have a look, he knows what to do :)
The discussion about SameSite here is interesting but actually is not relevant for this patch set. We should set SameSite on bug 26019. What we do here, is simple: If we use https (and we did recognize it), we set the Secure attribute on the cookie. Which is fine.
Comment 27 Jonathan Druart 2020-08-06 08:43:57 UTC
As we are going to backport this one in all stable releases at the same time I'd like another QA stamp on this one.

Martin, Tomas?
Comment 28 Martin Renvoize 2020-08-13 08:02:00 UTC
Created attachment 108161 [details] [review]
Bug 25360: Use secure flag for CGISESSID cookie when using HTTPS

This patch adds the secure flag to the CGISESSID cookie when using HTTPS.
This prevents the cookie being used again over a normal HTTP
request.

Bug 25360: [Follow-up] Test for "on" or "ON" value for HTTPS env var

This patch tests for HTTPS "on" or "ON" before setting the secure
cookie.

Bug 25360: [Follow-up] Fix typo in C4/InstallAuth.pm

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Amended number of tests in Context.t
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2020-08-13 08:02:04 UTC
Created attachment 108162 [details] [review]
Bug 25360: (follow-up) Remove the https FIXME in Auth.pm

The FIXME is no longer valid since we fixed the X-Forwarded headers
for Plack. And since we do not even use using_https anymore in
the templates (see bug 21094).

Test plan:
Run Auth.t
Git grep for using_https

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2020-08-13 08:02:29 UTC
All works as expected.. Passing QA
Comment 31 David Cook 2020-08-26 01:04:49 UTC
I notice it says released in 19.05.14. I'm guessing that's an error?
Comment 32 Aleisha Amohia 2020-08-26 04:38:37 UTC
backported to 19.11.x for 19.11.09
Comment 33 David Cook 2020-08-26 05:38:50 UTC
(In reply to Aleisha Amohia from comment #32)
> backported to 19.11.x for 19.11.09

I don't think it's been pushed to master yet though? 

Or has it been pushed to master but Jonathan/tooling hasn't updated this bug report yet?
Comment 34 David Cook 2020-08-26 05:40:24 UTC
(In reply to David Cook from comment #33)
> (In reply to Aleisha Amohia from comment #32)
> > backported to 19.11.x for 19.11.09
> 
> I don't think it's been pushed to master yet though? 

I just took a peek at origin/master and don't see it?
Comment 35 Jonathan Druart 2020-08-26 07:11:20 UTC
(In reply to David Cook from comment #34)
> (In reply to David Cook from comment #33)
> > (In reply to Aleisha Amohia from comment #32)
> > > backported to 19.11.x for 19.11.09
> > 
> > I don't think it's been pushed to master yet though? 
> 
> I just took a peek at origin/master and don't see it?

We are scheduling stable releases. This is a security bug, tt's expected to not be public before everything is ready for people to upgrade (ie. debian packages)
Comment 36 David Cook 2020-08-26 07:42:52 UTC
(In reply to Jonathan Druart from comment #35)
> We are scheduling stable releases. This is a security bug, tt's expected to
> not be public before everything is ready for people to upgrade (ie. debian
> packages)

Ah, thanks for the clarification. I was wondering if I was just going crazy...
Comment 37 Lucas Gass 2020-08-31 21:18:28 UTC
backported to 20.05.x for 20.05.03
Comment 38 Jonathan Druart 2020-09-03 08:43:15 UTC
Pushed to master for 20.11, thanks to everybody involved!