Это безопасный метод, ой
$salt = $this->get_salt($username);
if (is_null($salt)) {
return FALSE;
}
$password = sha1(SITE_KEY . $password . $salt);
$sth = $this->db->prepare("SELECT id, username, active FROM user WHERE username = ? AND password = ?");
$sth->setFetchMode(PDO::FETCH_OBJ);
$sth->execute(array($username, $password));
if (($result = $sth->fetch()) !== FALSE) {
return $result;
}
return FALSE;
Вот что меня беспокоит:
I did not misunderstand the login method. I just don't think it should return that object. I may be wrong and what you are doing is perfectly fine, but I doubt it. You are returning the full user object from the database, password and all, to a potentially insecure script. Someone could potentially create a new file and then do something like var_dump($userObject); and have all that information
Also, I just find it unintuitive to return a "magic" property from something. It's just another one of those things that is not verified before being used. If you were to move that to a separate method in your auth class and have it return the value of "active" you could run any verification you needed to without your login.php script being any the wiser.
: Looked at it again, and while you would already need to know the login information if you were to abuse that object in that way. It is still better, in my opinion, to separate it in case there is some sort of leak. Not saying I know how someone would be able to do it. Just think of it as one less potential loop hole.
I don't pretend to understand how malicious users would do something. I just know that making it easier for them to do so doesn't seem wise. Maybe there isn't anything wrong with it, I'm not a security expert. I'm just poking at things that, if I were to write it, I would change because they seem hazardous to me. If you truly wish to know if it is dangerous or not, I'd suggest submitting that example to the regular stackoverflow community to see what they say.
Есть ли у него какой-то смысл того, что он говорит? Метод будет использоваться в контроллере страниц, который я имею:
$user = $auth->login('username, password)
if ($user) {
//do some additional checks... set session variables
}
Ответы на вопрос(2)
Увидетьэтот ответьте как использоватьbcrypt хэшировать пароли. Он более безопасен, чем ваш, потому что позволяет преднамеренно замедлять алгоритм хэширования (поэтому, если кто-то попытается использовать грубую силу, он будет ограничен 1M итерациями в секунду или около того, вместо 1B, что является разницей между часами). и годы).
Кроме того (независимо от безопасности) вы должны использовать именованные заполнители PDO вместо неназванных (:placeholder и не?).
Кроме того, вы можете построить полныйUser Возьмите объект и верните его, с ним будет легче работать (при условии, что у вас есть тот, который вы должны!).
Нет, предоставленный вами код небезопасен.
Во-первых, вы используете не повторяющийся простой хеш. Увидетьэтот ответ а такжеэтот блог для информации о том, почему.
Во-вторых, почему вы храните соль отдельно? Сольmust быть уникальным для каждой записи, чтобы она работала. Таким образом, логичное место для этого - пароль. В этом случае вы делаете два запроса вместо одного (при условии, что соль не является производной от имени пользователя и действительно случайна). Если это не случайно, это должно быть. Увидетьэта почта для дополнительной информации...
В-третьих, вы подвержены временным атакам на хэш пароля, поскольку база данных не использует сравнение, не зависящее от времени. См.Эта статья а такжеЭта статья для дополнительной информации.
Не делайте этого самостоятельно. Это & APOS; sreally трудно сделать безопасно. Используйте библиотеку. Ты можешь использоватьPHPASS или жеPHP-PasswordLib...
Edit: To The Comments from your friend:
I did not misunderstand the login method. I just don't think it should return that object. I may be wrong and what you are doing is perfectly fine, but I doubt it. You are returning the full user object from the database, password and all, to a potentially insecure script. Someone could potentially create a new file and then do something like var_dump($userObject); and have all that information
Это верный момент. Но в этот момент, если пользователь может ввестиvar_dump($userObject)он может бежать$this->db->prepare("SELECT * FROM user"); и получитьall ваши пользовательские данные, а не только его. Таким образом, хотя на это следует указывать, оно не стоит того, чтобы пытаться его обезопасить. Если вы не получили действительно анального и не поместили все это в хранимую процедуру в базе данных и не ограничили доступ для чтения из таблицы (так что вы вообще не можете выбирать из пользовательской таблицы). Но в этот момент вам необходимо перепроектировать все приложение с нуля, поскольку вы никогда не сможете получить какую-либо информацию о пользователе, кроме как через SP. И это будет трудно сделать хорошо (сложность - враг безопасности).
В конце концов, я думаю, что вы будетеfar Лучше всего защищаться от внедрения скриптов в первую очередь. Особенно, если у вас нет профессиональных администраторов, которые могут защитить ваш сервер до смехотворного количества, необходимого для предотвращения такого рода атак (тюрьмы chroot в процессе PHP и т. Д.).
Also, I just find it unintuitive to return a "magic" property from something. It's just another one of those things that is not verified before being used. If you were to move that to a separate method in your auth class and have it return the value of "active" you could run any verification you needed to without your login.php script being any the wiser.
Какое магическое свойство там? Вы используете эти предметы напрямую? Или вы используете их для заполнения пользовательского объекта (который затем знает, как обрабатывать данные) ...? Только из-за этого фрагмента я не вижу проблемы, на которую указывает твой друг ...
: Looked at it again, and while you would already need to know the login information if you were to abuse that object in that way. It is still better, in my opinion, to separate it in case there is some sort of leak. Not saying I know how someone would be able to do it. Just think of it as one less potential loop hole.
На самом деле вам просто нужно знать действительное имя пользователя из-за временной атаки, указанной выше ...
I don't pretend to understand how malicious users would do something. I just know that making it easier for them to do so doesn't seem wise. Maybe there isn't anything wrong with it, I'm not a security expert. I'm just poking at things that, if I were to write it, I would change because they seem hazardous to me. If you truly wish to know if it is dangerous or not, I'd suggest submitting that example to the regular stackoverflow community to see what they say.
В целом это хороший совет. Просто помните, что сложность - враг безопасности. Таким образом, если усложнение усложняет задачу, становится намного сложнее, вероятность добавления новой уязвимости значительно выше (например, была введена временная атака). Я ошибаюсь в сторону простоты. У меня есть библиотеки, которые хорошо протестированы для обработки сложных частей (например, хеширования паролей), но код, который я пишу, прост.
Рассмотрим самый сильный алгоритм шифрования:One Time Pad, Алгоритм очень прост:
encrypted = plaintext XOR key
Но расшифровать без ключа невозможно. Таким образом, безопасность алгоритма на самом деле лежит в секрете, а не в алгоритме. Что ты хочешь? Алгоритм должен быть общедоступным, а безопасность не должна зависеть от его секретности. Иначе это становится безопасностью через безвестность. Что дает вам теплое нечеткое чувство, пока вы не сломаетесь ...