search

Buzzinoffbond / phpliteadmin

Automatically exported from code.google.com/p/phpliteadmin
0 stars 0 forks source link

"Remember me" cookie stores plaintext password #170

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Which is never a good thing to do. We could store a hash instead 
(SYSTEMPASSWORDENCRYPTED in the code, although not really encrypted), which is 
relatively better. Then, instead of checking 

`hash(<password from cookie> + <salt from cookie>) == <hash server-side>`

we can compare

`<hash from cookie> == hash(<password from config> + <salt from cookie>)`

Alternatively, we can also withhold the salt, and compare

`<hash from cookie> == hash(<password from config> + <salt from config>)`

Attached is a patch for the first behaviour, just to show how small the change 
would be.

Original issue reported on code.google.com by dreadnaut on 21 Jan 2013 at 7:05

Attachments:

GoogleCodeExporter commented 9 years ago 
Sorry, forget the second part, about using the salt from config. Salt must 
change every time we don't find it stored on the client, otherwise it's useless.

Original comment by dreadnaut on 21 Jan 2013 at 7:07

GoogleCodeExporter commented 9 years ago 
Rebased patch to r332. Can anyone double check it?

Original comment by dreadnaut on 9 Feb 2013 at 5:39

Attachments:

GoogleCodeExporter commented 9 years ago 
Thanks, I'll check your patch.

Original comment by crazy4ch...@gmail.com on 9 Feb 2013 at 5:44

GoogleCodeExporter commented 9 years ago 
Two things I noticed so far:
- salt is generated outside the Authorisation class. This means we have a 
global assumption (that some salt is generated). We should better put 
salt-management inside the class to decouple it (the rest of phpLiteAdmin 
doesn't use the salt).
- the salt is pretty short. Why do we use such a short salt? I don't see any 
good reason.

Those aren't problems introduced by your patch. But I just noticed those while 
having a look at our Authorisation code.

Original comment by crazy4ch...@gmail.com on 9 Feb 2013 at 6:11

GoogleCodeExporter commented 9 years ago 
Another thing, while we are at it: Why not using httpOnly cookies? Would be a 
great help against XSS/cookie attacks.
Only thing is PHP 5.2 would be required to use setcookie(), or we'd need to set 
cookies manually using header(). I am not sure which PHP version we currently 
require. I guess we probably already require 5.2 anyway, but I'm not sure.

Original comment by crazy4ch...@gmail.com on 9 Feb 2013 at 6:47

GoogleCodeExporter commented 9 years ago 
Okay: Please commit your patch. The other points I mentioned actually are 
different issues for which I will open separate issues.

Original comment by crazy4ch...@gmail.com on 9 Feb 2013 at 9:18

GoogleCodeExporter commented 9 years ago 
Thanks for looking at this. Patch committed, I'll head to the other issues in 
the next days.

Original comment by dreadnaut on 10 Feb 2013 at 1:33