Bug 9102 - [SECURITY] We should set httponly on our session cookie
Summary: [SECURITY] We should set httponly on our session cookie
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 9401
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-18 07:00 UTC by Chris Cormack
Modified: 2014-05-26 21:04 UTC (History)
5 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:


Attachments
Bug 9102 : Set HttpOnly on the CGISESSID cookie (2.04 KB, patch)
2012-11-18 07:02 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9102 : Set HttpOnly on the CGISESSID cookie (2.91 KB, patch)
2012-11-19 21:05 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9102 : Set HttpOnly on the CGISESSID cookie (3.00 KB, patch)
2012-11-21 13:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9102 : Followup Set HttpOnly on the CGISESSID cookie (5.42 KB, patch)
2012-12-03 10:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9102 : Set HttpOnly on the CGISESSID cookie (3.80 KB, patch)
2013-01-03 07:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9102 : Followup Set HttpOnly on the CGISESSID cookie (5.47 KB, patch)
2013-01-16 05:57 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9102 : Followup Set HttpOnly on the CGISESSID cookie (5.57 KB, patch)
2013-02-01 08:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2012-11-18 07:00:11 UTC
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.
Comment 1 Chris Cormack 2012-11-18 07:01:26 UTC
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
Comment 2 Chris Cormack 2012-11-18 07:02:43 UTC Comment hidden (obsolete)
Comment 3 Owen Leonard 2012-11-19 20:58:43 UTC
The test works as expected, but can we get a resubmission without tab indentation on the affected lines?
Comment 4 Chris Cormack 2012-11-19 21:05:02 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2012-11-21 13:40:11 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2012-11-26 16:11:02 UTC
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?
Comment 7 Chris Cormack 2012-11-26 18:14:40 UTC
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.
Comment 8 Jonathan Druart 2012-12-03 10:19:01 UTC Comment hidden (obsolete)
Comment 9 Owen Leonard 2012-12-27 16:08:58 UTC
(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?
Comment 10 Jonathan Druart 2012-12-27 16:16:15 UTC
(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.
Comment 11 Chris Cormack 2013-01-01 22:27:16 UTC
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
Comment 12 Katrin Fischer 2013-01-03 07:36:04 UTC Comment hidden (obsolete)
Comment 13 Jared Camins-Esakov 2013-01-04 13:14:14 UTC
The first patch has been pushed to master.
Comment 14 Galen Charlton 2013-01-16 04:54:32 UTC
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
Comment 15 Galen Charlton 2013-01-16 05:57:08 UTC Comment hidden (obsolete)
Comment 16 Galen Charlton 2013-01-16 05:58:46 UTC
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.
Comment 17 Marcel de Rooy 2013-02-01 08:52:41 UTC
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>
Comment 18 Marcel de Rooy 2013-02-01 08:56:49 UTC
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.
Comment 19 Jared Camins-Esakov 2013-02-01 15:49:29 UTC
This patch has been pushed to master.