In-Portal Issue Tracker - In-Portal CMS
Viewing Issue Advanced Details
ID: Category: Type: Reproducibility: Date Submitted: Last Update:
1436 [In-Portal CMS] Security feature request N/A 2012-11-05 11:24 2012-12-13 02:46
Reporter: alex Platform:  
Assigned To: alex OS:  
Priority: normal OS Version:  
Status: resolved Product Version: 5.1.0  
Product Build: Resolution: fixed  
 
ETA: none Fixed in Version: 5.3.0-B1  
Reference: https://groups.google.com/d/topic/in-portal-dev/qenm_MavpZc/discussion
Change Log Message: Add cookie encryption
Estimate Points: 1
Summary: 0001436: Encrypt cookie stored at client
Description: I've found an interesting article about mistrusting cookie values submitted by browser to web server - http://phpadvent.org/2011/bake-cookies-like-a-chef-by-michael-nitschinger.

That article explains in details how we can encode/hash cookie values to make sure that In-Portal did set these cookies and they were not faked by user, who wants to hack website.
We can use random string as password used to hash/encode cookies.
Steps To Reproduce:
Additional Information:
Relationships
related to 0001435resolved  (5.3.0)alex Generate random string at install 
Attached Files: patch encrypt_cookie_1436.patch (4,936) 2012-12-07 12:26
http://tracker.in-portal.org/file_download.php?file_id=1887&type=bug
patch encrypt_cookie_1436_v2.patch (9,487) 2012-12-10 10:31
http://tracker.in-portal.org/file_download.php?file_id=1888&type=bug
patch encrypt_cookie_1436_v3.patch (10,159) 2012-12-11 07:42
http://tracker.in-portal.org/file_download.php?file_id=1891&type=bug
patch encrypt_cookie_1436_v4.patch (13,880) 2012-12-12 05:30
http://tracker.in-portal.org/file_download.php?file_id=1893&type=bug
patch warning_on_cookie_decrypting.patch (1,183) 2012-12-13 02:44
http://tracker.in-portal.org/file_download.php?file_id=1894&type=bug
Issue History
Date Modified Username Field Change
2012-12-13 02:48 alex Changeset attached 5.3.x r15651
2012-12-13 02:46 alex Note Added: 0005340
2012-12-13 02:44 alex File Added: warning_on_cookie_decrypting.patch
2012-12-12 05:34 alex Changeset attached 5.3.x r15650
2012-12-12 05:34 alex Note Added: 0005339
2012-12-12 05:34 alex Status reviewed and tested => resolved
2012-12-12 05:34 alex Fixed in Version => 5.3.0-B1
2012-12-12 05:34 alex Resolution open => fixed
2012-12-12 05:30 alex File Added: encrypt_cookie_1436_v4.patch
2012-12-12 05:29 alex Note Added: 0005338
2012-12-12 05:29 alex Status needs testing => reviewed and tested
2012-12-11 07:43 erik Note Added: 0005335
2012-12-11 07:43 erik Assigned To erik => alex
2012-12-11 07:43 erik Status needs work => needs testing
2012-12-11 07:42 erik File Added: encrypt_cookie_1436_v3.patch
2012-12-11 03:05 alex Note Added: 0005331
2012-12-11 03:05 alex Assigned To alex => erik
2012-12-11 03:05 alex Status needs testing => needs work
2012-12-10 10:36 erik Note Added: 0005329
2012-12-10 10:36 erik Assigned To erik => alex
2012-12-10 10:36 erik Status needs work => needs testing
2012-12-10 10:31 erik File Added: encrypt_cookie_1436_v2.patch
2012-12-07 12:41 alex Note Added: 0005328
2012-12-07 12:41 alex Assigned To alex => erik
2012-12-07 12:41 alex Status needs testing => needs work
2012-12-07 12:27 erik Note Added: 0005327
2012-12-07 12:27 erik Assigned To => alex
2012-12-07 12:27 erik Developer => erik
2012-12-07 12:27 erik Status active => needs testing
2012-12-07 12:26 erik File Added: encrypt_cookie_1436.patch
2012-12-05 03:33 alex Note Added: 0005315
2012-11-05 11:24 alex Relationship added related to 0001435
2012-11-05 11:24 alex New Issue
2012-11-05 11:24 alex Reference => https://groups.google.com/d/topic/in-portal-dev/qenm_MavpZc/discussion
2012-11-05 11:24 alex Change Log Message => Add cookie encryption
2012-11-05 11:24 alex Estimate Points => 1

Notes
(0005315)
alex   
2012-12-05 03:33   
1. create kCookieHasher class (from article, which link is given in task) in "core/kernel/utility/cookie_hasher.php" file and register it in kApplication::RegisterDefaultClasses method

2. make $secret parameter in constructor optional and when it's not given use RandomString system setting value in it's place (see 0001435 task)

3. in Session::SetCookie method create instance of kCookieHasher class (via kApplication::makeClass - non static method !!!) and use it to hash actual cookie value that will be given to "setcookie" function

4. in kHTTPQuery::AddAllVars method before actual cookie values will be added into the class pre-process $_COOKIE array (in local variable) and undo hashing for cookies, that have it

5. cookies, written via JavaScript won't be affected by this change in any way
(0005327)
erik   
2012-12-07 12:27   
Patch attached. Needs testing
(0005328)
alex   
2012-12-07 12:41   
1. I don't see code, that checks if cookie isn't actually encrypted and passes it through. Instead I see code that gives even unencrypted cookies to kCookieHasher class which might even result in php error.

2. Only cookie value is hashed (not cookie name), that results in same hash for 2 cookies with identical values but different names. This way attacker can get 1 cookie value and place it in another cookie value.

3. What about upgrade, does it work normally when RandomString is generated during upgrade and suddenly cookies becomes encrypted.

4. I see potential problem with passing through cookies, that are not encrypted. This way attacker could make hacked cookie look like JS made cookie and pass it through without a problem. I don't know about a solution yet, any ideas?
(0005329)
erik   
2012-12-10 10:36   
New patch version attached. Needs testing.

1. Added code to prevent PHP warnings in cookies decoding process.

2. Added cookie name to hashable string

3,4. Made empty cookie values if after RandomString these becomes undecryptable.
(0005331)
alex   
2012-12-11 03:05   
1. Class kCookieHasher does all encryption/decryption of cookies and it also need to store cookies names, that doesn't need to be decrypted
2. No upgrade script
3. Usage of "var" is deprecated, please use corresponding visibility operator (e.g. private/public/protected)
4. Plain text cookie list contains cookie names, that In-Portal:
- don't write itself (e.g. "__ut*", "debug_*", "send_*", "use_*"), but keep "debug_session_id" and add "debug_off" cookie
- are written through PHP and must be encrypted (e.g. "cookies_on", "save_username")
4a. exclude all cookie names, that are written through PHP (e.g. Session::SetCookie call)

'catalog_active_prefix', 'cookies_on', 'debug_fastfile', 'debug_host', 'debug_port', 'debug_jit', 'debug_session_id', 'debug_stop', 'original_url', 'save_username', 'send_debug_header', 'send_sess_end', 'start_debug', 'TreeExpandStatus', 'use_remote', '__utma', '__utmz', '__unam'
(0005335)
erik   
2012-12-11 07:43   
1. Fixed
2. Fixed
3. Fixed
4. Fixed

New patch attached. Needs testing
(0005338)
alex   
2012-12-12 05:29   
Worked in general, but some minor things (related to code readability and disposition) were adjusted.

Fixes:
======
1. fixed spelling errors
2. added comments to all class properties/methods
3. added more checks on user entered plain text cookie names
4. fixed used ecryption key size to match used cipher (only first 32 symbols of 64 symbol random string are used)
5. a warning was produced during old cookie decryption after random string change

Failed expectations:
====================
1. any of encrypted cookies (even empty one) is 132 symbols long (too much, isn't it? but maybe serialized string with a signature would have similar length)
2. ecrypted cookies are completely unreadable (was expected), so readinging them without working In-Portal installation isn't possible
(0005339)
alex   
2012-12-12 05:34   
Fix committed to 5.3.x branch. Commit Message:

Fixes 0001436: Encrypt cookie stored at client
(0005340)
alex   
2012-12-13 02:46   
Patch "warning_on_cookie_decrypting.patch" solves problem, when cookie with "53741477" value being decrypted instead of being skipped because it isn't actually encrypted.