Not work "Remember me"
Originally reported on Google Code with ID 164
Not work "Remember me".
Reported by ktretyakGM
on 2013-01-01 13:41:01
Comments (13)
-
Account Deleted -
Thanks for reporting this issue. I can confirm it doesn't work with latest 1.9.4, but with 1.9.3.3 it works. dreadnaut, could you please have a look at this? It seems this fault got introduced with issue 161.
Reported by
crazy4chrissi
on 2013-01-20 22:46:17 - Status changed:Accepted
-
I rolled back a few versions, and the bug appears starting from r290, so it predates my changes (r307). I don't know much about that part of the code, but testing I noticed that a new salt is generated even if a cookie exists, which means that the comparison with the password hash will fail. (By the way, is there a _bisect_ command in SVN? It was a bit of a pain to find the failing revision manually)
Reported by
dreadnaut
on 2013-01-20 23:45:55 -
Hey. Thanks for checking the issue. Sorry that I thought the issue was introduced by your changes, I just assumed so because this part of code was touched by you last. Although I am also not very familiar with this part of the code, you can assign the issue to me if you like to. I'd then dig into it to fix it. Regarding bisect: I think there is no native svn-command, but there are tools that automate it like this one: http://search.cpan.org/dist/App-SVN-Bisect/bin/svn-bisect (I have not tried it myself yet and do not know whether it works well.)
Reported by
crazy4chrissi
on 2013-01-21 09:08:33 -
> Sorry that I thought the issue was introduced by your changes, I just assumed > so because this part of code was touched by you last. No problem, that's exactly what I thought too :) Looking at the code, I am a bit perplexed: are we storing a readable password in a cookie, and its salted and hashed value in the session? (line 296 in 1.9.3.3 and line 608 in 1.9.4) Shouldn't it be the opposite?
Reported by
dreadnaut
on 2013-01-21 13:40:23 -
Anyway, sneaky bug found! Cookie names cannot contain symbols other than underscores, so VERSION 1.9.x becomes 1_9_x when stored. When checking for the salt cookie on line 396, we still look for 1.9.x and fail.
Reported by
dreadnaut
on 2013-01-21 14:30:55 -
Ah, damn. So it was me who introduced it in revision 290 while "fixing" issue 145. I guess we should do something like define("COOKIENAME", $cookie_name.str_replace('.','_',VERSION)); Or do you have a better solution? But I think your other point is valid too. I also noticed lately that the cookie seems to contain a cleartext pw. Of course not the way to do it. (This is especially bad in combination with XSS vulnerabilities like the one discovered lately and fixed in 1.9.3.3). Maybe we should open another issue for that, though.
Reported by
crazy4chrissi
on 2013-01-21 17:46:35 - Status changed:Started
-
We can fix this one with str_replace and open a new one for the cleartext password. Attached is a patch for this specific issue, against r318. This is one the points I'd like to clear up with a nicer config/session/cookie system, about which I've been nagging people :)
Reported by
dreadnaut
on 2013-01-21 18:49:52<hr> * Attachment: cookiefix-vs-r318.diff
-
Yeah, go ahead and commit it to svn. Ah, I see, I still wanted to comment on the config-mailinglist topic. Maybe I'll find the time tomorrow.
Reported by
crazy4chrissi
on 2013-01-21 19:00:50 -
Done, committed as r319.
Reported by
dreadnaut
on 2013-01-21 19:12:13 - Status changed:Fixed
-
Thanks.
Reported by
crazy4chrissi
on 2013-01-21 19:13:14 -
Then again, it's probably safer to filter the whole string, so I committed r320 with the one line change below. -define("COOKIENAME", $cookie_name . '_' . preg_replace('/[^a-zA-Z0-9_]/', '_', VERSION) ); +define("COOKIENAME", preg_replace('/[^a-zA-Z0-9_]/', '_', $cookie_name . '_' . VERSION) );
Reported by
dreadnaut
on 2013-01-21 19:22:51 -
Account Deleted It's works! Thank you!
Reported by
ktretyakGM
on 2013-01-21 19:36:57 - Log in to comment
Reported by
ktretyakGM
on 2013-01-01 13:45:52