 |
| Coder: 1 - Bugs: 0 |
This one appeared when I was writing a proof-of-concept for a login screen in PHP that would have, wait for it, hard-coded accounts. Why hard-coded? Well, it was just a throwaway POC and I really didn't want to go through the trouble of setting up a database for it because that wasn't the point of the POC.
What was the point of the POC, then? The point was, well... you know what? That isn't the point of today's bug-hunting session either. The point is that a bug appeared. And as bugs went, that was pretty elementary.
I had a HTML interface that was basically a login screen....
index.html
<!DOCTYPE html>
<html>
<head>
<title>POC Login</title>
</head>
<body>
<form action="login.php" method="post">
<label for="id">ID: <input name="id" id="id"> </label>
<br />
<label for="password">Password: <input name="password" id="password" type="password"> </label>
<br />
<button>Login</button>
</form>
</body>
</html>
<html>
<head>
<title>POC Login</title>
</head>
<body>
<form action="login.php" method="post">
<label for="id">ID: <input name="id" id="id"> </label>
<br />
<label for="password">Password: <input name="password" id="password" type="password"> </label>
<br />
<button>Login</button>
</form>
</body>
</html>
And yes, it was ugly as sin. Kiss my ass.
...and some PHP code that would process the form. Note that this is not the proper way a login should be handled, bug or no bug.
login.php
<?php
$id = $_POST["id"];
$password = $_POST["password"];
$users =
[
"test1" => ["password" => "password", "role" => "user"],
"test2" => ["password" => "password", "role" => "user"],
"test3" => ["password" => "password", "role" => "user"],
"adm1" => ["password" => "password", "role" => "admin"],
"adm2" => ["password" => "password", "role" => "admin"]
];
if (isPasswordMatch($id, $password, $users))
{
echo "Welcome, " . $id;
}
else
{
echo "Your password did not match.";
}
function isPasswordMatch($id, $password, $users)
{
if (array_key_exists($id, $users))
{
return $users[$id]["password"] = $password;
}
return false;
}
?>
$id = $_POST["id"];
$password = $_POST["password"];
$users =
[
"test1" => ["password" => "password", "role" => "user"],
"test2" => ["password" => "password", "role" => "user"],
"test3" => ["password" => "password", "role" => "user"],
"adm1" => ["password" => "password", "role" => "admin"],
"adm2" => ["password" => "password", "role" => "admin"]
];
if (isPasswordMatch($id, $password, $users))
{
echo "Welcome, " . $id;
}
else
{
echo "Your password did not match.";
}
function isPasswordMatch($id, $password, $users)
{
if (array_key_exists($id, $users))
{
return $users[$id]["password"] = $password;
}
return false;
}
?>
What went wrong
Well, I had five user accounts in there. Upon logging in successfully, the code was supposed to check if the submitted id/password combination was valid.Unfortunately, it appears that no matter what password I entered, the screen would still welcome the user! Let's try with password "xyz"...
Why it went wrong
This one was relatively easy to track down, least of all because a coder with all my years of experience simply has no business making a mistake like this. It was in the isPasswordMatch() function.Look at the return statement, in particular. There's only a single "=" operator. That basically meant that the function would always return true.
index.html
if (array_key_exists($id, $users))
{
return $users[$id]["password"] = $password;
}
{
return $users[$id]["password"] = $password;
}
How I fixed it
Yes, that's all it took. An extra "=" operator.index.html
if (array_key_exists($id, $users))
{
return $users[$id]["password"] == $password;
}
{
return $users[$id]["password"] == $password;
}
Now, once I enter any string other than "password", which is the password for all the hard-coded accounts (another bad practice), the script responds appropriately!
Conclusion
This one is basic. When testing for equality or equivalence in PHP (or similar languages), use "==". When trying to assign value, use "=". Those are two different operations altogether.That said, it's still a sucky way to handle logins, so don't do it. But the bug that resulted from it could happen anywhere a "==" operator is required.
Not all bugs are made equal,
T___T
T___T
No comments:
Post a Comment