https://www.owasp.org/index.php/HttpOnly If the HttpOnly flag (optional) is included in the HTTP response header, the cookie cannot be accessed through client side script (again if the browser supports this flag). As a result, even if a cross-site scripting (XSS) flaw exists, and a user accidentally accesses a link that exploits this flaw, the browser (primarily Internet Explorer) will not reveal the cookie to a third party. If a browser does not support HttpOnly and a website attempts to set an HttpOnly cookie, the HttpOnly flag will be ignored by the browser, thus creating a traditional, script accessible cookie. As a result, the cookie (typically your session cookie) becomes vulnerable to theft of modification by malicious script. There is no reason for the client side to be accessing the session cookie, so for browsers that supports this, this helps protect the user from malicious javascript.
To test, use curl Before the patch % curl -I http://192.168.2.135 HTTP/1.1 200 OK Date: Sun, 18 Nov 2012 06:56:49 GMT Server: Apache/2.2.22 (Ubuntu) Pragma: no-cache Cache-control: no-cache Content-script-type: text/javascript Content-style-type: text/css Set-Cookie: CGISESSID=19689f6e7d8ec94c25269fecebf2f009; path=/ Vary: Accept-Encoding Content-Type: text/html; charset=UTF-8 After patch % curl -I http://192.168.2.135 HTTP/1.1 200 OK Date: Sun, 18 Nov 2012 07:01:04 GMT Server: Apache/2.2.22 (Ubuntu) Pragma: no-cache Cache-control: no-cache Content-script-type: text/javascript Content-style-type: text/css Set-Cookie: CGISESSID=da25baf03c0bc1e2c512a627028e43e6; path=/; HttpOnly Vary: Accept-Encoding Content-Type: text/html; charset=UTF-8
Created attachment 13504 [details] [review] Bug 9102 : Set HttpOnly on the CGISESSID cookie
The test works as expected, but can we get a resubmission without tab indentation on the affected lines?
Created attachment 13539 [details] [review] Bug 9102 : Set HttpOnly on the CGISESSID cookie
Created attachment 13580 [details] [review] Bug 9102 : Set HttpOnly on the CGISESSID cookie Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Hi Chris, There are 2 other occurrences of this kind in C4::Auth::check_api_auth: l.1141 my $cookie = $query->cookie( CGISESSID => $session->id ); l.1185 my $cookie = $query->cookie(CGISESSID => $sessionID); Don't you think we have to add this flag for them too?
Not sure about that Jonathan, since those ones are only used by the API, not rendered in a page. Possibly users of the API might want to interact with the cookie with javascript? More likely, since they wont be interacting with it with a browser that understands the httponly flag it will be ignored. We could add the flag just in case a user is tricked into going to a page from the api, that has been compromised and has xss in it. Maybe send a follow up, It can't really hurt to have it in it I think.
Created attachment 13838 [details] [review] Bug 9102 : Followup Set HttpOnly on the CGISESSID cookie
(In reply to comment #8) > Bug 9102 : Followup Set HttpOnly on the CGISESSID cookie Is there a way to test the follow-up like their was for Chris's patch?
(In reply to comment #9) > (In reply to comment #8) > > Bug 9102 : Followup Set HttpOnly on the CGISESSID cookie > > Is there a way to test the follow-up like their was for Chris's patch? I don't know... I think if you don't have any error on some pages the patch works. But if we have a doubt about this patch, we don't have to integrate it.
I think we should QA the first patch and push it, the second one should be safe and essentially a no-op but is hard to test. So if we could push the first one and then put this back to needs sign off
Created attachment 14384 [details] [review] [SIGNED-OFF] Bug 9102 : Set HttpOnly on the CGISESSID cookie https://www.owasp.org/index.php/HttpOnly Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> To test, use curl Before the patch % curl -I http://192.168.2.135 HTTP/1.1 200 OK Date: Sun, 18 Nov 2012 06:56:49 GMT Server: Apache/2.2.22 (Ubuntu) Pragma: no-cache Cache-control: no-cache Content-script-type: text/javascript Content-style-type: text/css Set-Cookie: CGISESSID=19689f6e7d8ec94c25269fecebf2f009; path=/ Vary: Accept-Encoding Content-Type: text/html; charset=UTF-8 After patch % curl -I http://192.168.2.135 HTTP/1.1 200 OK Date: Sun, 18 Nov 2012 07:01:04 GMT Server: Apache/2.2.22 (Ubuntu) Pragma: no-cache Cache-control: no-cache Content-script-type: text/javascript Content-style-type: text/css Set-Cookie: CGISESSID=da25baf03c0bc1e2c512a627028e43e6; path=/; HttpOnly Vary: Accept-Encoding Content-Type: text/html; charset=UTF-8
The first patch has been pushed to master.
I've spent quite a bit of time reviewing the second patch. I'm about ready to sign off on it, but I've identified some JavaScript related to tags that expects to read the CGISESSID cookie. Fortunately, it doesn't actually need to in order to work, but I want to remove references to it. The offending bits of JavaScript are contained in three files: koha-tmpl/intranet-tmpl/prog/en/modules/tags/review.tt koha-tmpl/opac-tmpl/prog/en/js/tags.js koha-tmpl/opac-tmpl/ccsr/en/js/tags.js
Created attachment 14618 [details] [review] Bug 9102 : Followup Set HttpOnly on the CGISESSID cookie Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Note for QA/RM - this patch stands alone, but I recommend that the patch be tested and pushed at the same time as the patch for bug 9401.
Created attachment 14994 [details] [review] Bug 9102 : Followup Set HttpOnly on the CGISESSID cookie Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested. See the HttpOnly flag in Firebug. Checked cookie processing in IE9 and FF with opac language and session. Also added an item without problems. Code looks good to me. Passed QA Note for RM: Will take a look too at the related report of Galen.
This patch has been pushed to master.