Question about latest phpBB security fix

[schotte]

Hello,

I found the following on the phpBB website:


The first issue is critical (session handling allowing everyone gaining administrator rights) and we urge you to fix it on your forums as soon as possible:

Open includes/sessions.php

Find:
Code:
if( $sessiondata['autologinid'] == $auto_login_key )

Replace with:
Code:
if( $sessiondata['autologinid'] === $auto_login_key )


I don't see where the problem is with this part of the code. Is it seriously important that the datatype is checked as well? If so, why? I mean how could you exploit this? It is not like you could use eval or so, as it compares two md5-hashs.

$auto_login_key comes from the database as an md5 hash, so you can't really change it. And $sessiondata['autologinid'] is from the cookie set.

Please "enlighten" me on this one :)

Schotte

[JamieR]

Note where I have coloured the third = red.

Find:
Code:
if( $sessiondata['autologinid'] == $auto_login_key )

Replace with:
Code:
if( $sessiondata['autologinid'] === $auto_login_key )

:D Jamie.

[schotte]

I know that there needs to be a third =-sign. My question was what difference does it make in this particular case. As you can't exploit it - at least that is what I think. It's not about how to achieve it, it is why phpBB changed it.

Schotte

[mordred]

It's a little hard to explain why they changed this particular line without getting into detail what makes it exploitable. But as this vulnerability has been announced some days ago, and probably everyone responsible for maintaining a phpBB will have got the news and reacted accordingly, I think it's safe to explain a little about a curious behaviour of PHP when comparing types. Moderators, if you feel otherwise, delete this post.

Look at this code and spot the bug:


# simulates a key from the database
$auto_login_key = md5('codingforums');

# simulates serialized cookie data
$data = addslashes(serialize(array('autologinid' => 0)));

# recreate data stored in cookie
$sessiondata = unserialize(stripslashes($data));

# hmm...
if( $sessiondata['autologinid'] == $auto_login_key ) {
print "h4x0red";
}


Basically, what gets compared in the conditional is that (in pseudo-code):


if (0 == "aa3af0cd9f23408179355681f4555e44")


So why do right and left side of the comparison evaluate to "true"? Answer: PHP has type juggling. That means it tries to figure out how to treat a variable's type depending on the context. The details are laid out in the manual, but in our particular case it sees on the left side a 0 and thinks "ah, the user wants to compare integers. Fine. I'll try to convert the string on the right side to an integer. User will be pleased".

So how does PHP convert strings to integers? It just tries:

"23" => 23 // that was easy
"23something" => 23 // ok, take the first numbers..
"something" => 0 // sorry, no number found. use 0 instead

And it's the last case who's responsible for the behaviour above, because now we compare 0 == 0 and evaluate to true. A comparison with the identity operator === triggers a different behaviour and returns false, because it does not juggle the types.

So now we only have to ask: How in all world to we get an integer into that comparison? All data you send per GET, POST, COOKIE to a PHP script gets imported as a string (or an array of strings). But because we unserialize an array within cookie data, we can plant variables with real types there. Never ever use unserialize on cookie data! You can't imagine every possible case of bad data your users will send to you.

One has to be careful with phpBB these days... it's weaknesses seem to have got the attention of a bunch of maligned crackers.

EDIT: One more thing: The comparison will only evaluate to true if the tested string on the right side does not begin with a number. Quite a few hashed passwords to begin with one though, so it's not universally applicable. Assuming a standard hashing routine that is, I have no idea how exactly phpBB creates and stores passwords.

[Kurashu]

phpBB uses md5

[firepages]

enlightening post Mordred :thumbsup:

[schotte]

That's an awesome explanation. Thanks a lot Mordred.

Schotte