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.
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.
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...
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.
(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.
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
*** Bug 19571 has been marked as a duplicate of this bug. ***
Annnd my patch is buggy, because HTTPS "OFF" will turn on the secure cookie...
(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...
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.
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.
Created attachment 104642 [details] [review] Bug 25360: [Follow-up] Fix typo in C4/InstallAuth.pm
*** Bug 25715 has been marked as a duplicate of this bug. ***
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.
(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!
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.
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.
Created attachment 106178 [details] [review] Bug 25360: [Follow-up] Fix typo in C4/InstallAuth.pm
I don't actually love these patches, but they have been working so far. That said, the cookie creation is begging for some refactoring...
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.
(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
I expected to find SameSite defaulting to Lax and Secure is true, but what I see now is SameSite=None and Secure=true ?
(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
Should set network.cookie.sameSite.laxByDefault in FF about:config
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
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>
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.
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?
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>
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>
All works as expected.. Passing QA
I notice it says released in 19.05.14. I'm guessing that's an error?
backported to 19.11.x for 19.11.09
(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?
(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?
(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)
(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...
backported to 20.05.x for 20.05.03
Pushed to master for 20.11, thanks to everybody involved!