At 8/1/16 04:58 PM, egg82 wrote:
Shameless plug:
Good password storage on Github
You don't need to do this:
$retarr = array();
while($row = $statement->fetch()) {
array_push($retarr, $row);
}
Just do this:
$retarr = $statement->fetchAll(\PDO::FETCH_BOTH);
I also don't really know why you made a query() function and then made it use prepare and not use any of the additional features that prepare gives you.
This:
public function query($q) {
global $logger;
try {
$statement = $this->connection->prepare($q);
$statement->setFetchMode(\PDO::FETCH_BOTH);
$statement->execute();
$retarr = array();
while($row = $statement->fetch()) {
array_push($retarr, $row);
}
return $retarr;
} catch (\PDOException $ex) {
$logger->log("PDO", "ERROR", $ex);
return null;
}
}
Can be simplified to this:
public function query($q) {
global $logger;
try {
$statement = $this->connection->query($q);
return $statement->fetchAll(\PDO::FETCH_BOTH);
} catch (\PDOException $ex) {
$logger->log("PDO", "ERROR", $ex);
return null;
}
}
And why are you using PDOs but still manually escaping every input? Just use parameterised queries and not worry about that. You don't need to manually escape stuff like you would if you were using the procedural mysql_ functions (which should never be used for several reasons). Just use prepare and bind the values:
$stmt = $database->prepare("
SELECT `id`, `$user_field`, `$pass_field`, `$key_field`, `$iv_field`
FROM `$table`
WHERE `$user_field` = :user OR `$email_field` = :user;
");
$stmt->bindParam(":user", $_COOKIE["user"], \PDO::PARAM_STR);
$stmt->execute();
It's also rather redundant to be using the backticks everywhere, as they don't do anything if your table and column names don't have SQL keywords in them. But that's personal taste, I suppose. (I never use them unless I need them; they're ugly.)
And, like in that snippet, you don't need to concatenate your strings to include variables. There's no significant performance one way or the other in regard to using them, but using them makes your code hard to read and hard to change; I highly recommend plopping variables in the middle of double-quotes.
As for binding parameters: pretty much the only things that can't be bound are database, table, and column names, so you would still need to escape those if you wanted to add them to your query from an un-trusted source for some insane reason.
And, although I think encrypting the result of a hashing algorithm is rather pointless, you should use OpenSSL to handle encryption rather than mcrypt; mcrypt outdated and abandoned and all the other reasons listed here.
You also included a rather significant security risk with this:
if (count($result) == 0) {
if ($this->prevent_user_enum) {
// Preventing timing attacks.
$this->check_pass("testing", [...]);
echo("User/E-mail does not exist, or password is invalid!");
return;
} else {
echo("User/E-mail does not exist!");
return;
}
}
if (!$this->check_pass($pass, $result[0]['pass'], $result[0]['key'], $result[0]['iv'])) {
if ($this->prevent_user_enum) {
echo("User/E-mail does not exist, or password is invalid!");
return;
} else {
echo("Password is invalid!");
return;
}
}
You should never be reporting that much information via a login dialog, as it would make finding existing usernames and emails significantly easier. There's no good reason to provide anything other than "invalid credentials" regardless of what actually failed.
And this is a pretty bad idea, too:
$security->set_cookie("encrypted_pass", $security->encrypt($pass, $result[0]['key'], base64_decode($result[0]['iv'])));
Passwords should only ever be stored as hashes, and only when it's absolutely necessary. There isn't any good reason to encrypt a plaintext password, let alone encrypt a plaintext password and store it as a cookie.
And, as I alluded to above, this just doesn't make sense:
$encrypted_pass = $security->encrypt($security->pass_hash($pass), $key, $iv);
What's the point of encrypting a hash? A correctly constructed hash doesn't contain any useful information.
All that aside, this is a rather bizarre mishmash of OOP and procedural code with multiple Main classes being used in a non-useful manner. I'm assuming this was never intended to actually be used in something (I hope so, at least) but it should still at least be a RESTful MVC framework; right now it's just a mess of spaghetti code.
Far from being even close to some of the horrible attempts I've seen at handling user management, but this is certainly not logging in and registration done right. The security problems not withstanding, this is the sort of web development style of coding that makes me want to rip my hair out when I have to work with it: it's, honestly, just a hard-to-read mess.
You should patch up those security holes; reorganise the code to be an MVC framework (pretty much the best paradigm for web development) and, if you feel like it, make it RESTful, which isn't super important but still not at all a bad idea; and read up on how to properly implement PDOs. Looks like you're trying to use PDOs the same way the deprecated procedural mysql_ functions were used, which is over-complicating your code.