Not work "Remember me"

[Former user]

Originally reported on Google Code with ID 164

Not work "Remember me".

Reported by ktretyakGM on 2013-01-01 13:41:01

[Former user]

Sorry. I have phpLiteAdmin v1.9.4

Reported by ktretyakGM on 2013-01-01 13:45:52

[Christopher Kramer]

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

[dreadnaut]

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

[Christopher Kramer]

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

[dreadnaut]

> 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

[dreadnaut]

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

[Christopher Kramer]

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

[dreadnaut]

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

[Christopher Kramer]

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

[dreadnaut]

Done, committed as r319.

Reported by dreadnaut on 2013-01-21 19:12:13 - Status changed: Fixed

[Christopher Kramer]

Thanks.

Reported by crazy4chrissi on 2013-01-21 19:13:14

[dreadnaut]

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

[Former user]

It's works! Thank you!

Reported by ktretyakGM on 2013-01-21 19:36:57