Этот код PHP выглядит безопасным для системы входа в систему?

Question

Это только его часть, но сначала он устанавливает соединение, затем проверяет, существует ли имя пользователя, а затем вставляет данные в таблицу. Я не очень разбираюсь в PHP, поэтому мне не нужно врываться в меня. Просто пытаюсь учиться здесь, и мне интересно, нахожусь ли я на правильном пути.

require("constants.php");
try {  
  $DBH = new PDO("mysql:host=$host;dbname=$dbname", $dbconnect, $dbpass);  
  $DBH->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );    
}  
catch(PDOException $e) {  
    echo "sorry, something happened. try going back and try again.";  
    file_put_contents('PDOErrors.txt', $e->getMessage(), FILE_APPEND);  
}  

function checkName(){
$STH = $DBH->query('SELECT username FROM users WHERE username = $username');
$STH->setFetchMode(PDO::FETCH_OBJ);
while($row = $STH->fetch()) {
    if($username != $row->username){
    $check = 1;
    }
    else{
    $check = 0;
    }
    return $check;
}
function createSalt()
{
    $string = md5(uniqid(rand(), true));
    return substr($string, 0, 3);
}
function register(){
$check = checkName();
if($check == 1){
$salt = createSalt();
$hash = sha1($salt . $hash);
$data = array($username, $hash, $salt, $ip);
$STH = $DBH->("INSERT INTO users (username, password, salt, ip) values (?, ?, ?)");
$STH->execute($data);
}
}

Answer 1

1. Вы не определяете $ username и используете его в одной строке в кавычках, поэтому запрос SELECT не будет выполняться

2. Вам следует использоватьподготовленный оператор и привязать имя пользователя к параметру в запросе SELECT, а не вставлять имя пользователя непосредственно в строку

3. Вам не нужно выбирать пользователя с таким именем, чтобы увидеть,он уже используется, вам нужно только выбрать счетчик и посмотреть, не равен ли он нулю

4. Если ваш запрос SELECT не возвращает строк, ваша функция checkName не возвращает никакого значения, посколькувы возвращаетесь только внутри цикла через строки

Надеюсь, эти комментарии полезны.

Answer 2

1. Используйте подготовленные операторы вместо встроенных переменных
2. Прекратить использование глобальных переменных. Вместо этого используйте $_POST['name'].
3. if($username != $row->username){ что ??!?! Поскольку имя пользователя уникально - это может быть либо 1 правильная строка (всегда правильная), либо 0 строк.

Answer 3

Нет, это не безопасно / хорошо написано. Предполагается, что глобальные регистры включены, а использование глобальных переменных вместо аргументов функций делает код очень трудным для переноса в различные контексты. Оставляя в стороне плохое форматирование, полное отсутствие каких-либо комментариев усложняет обслуживание кода - хороший стиль кодирования является необходимым условием для хорошего программирования - и, следовательно, безопасности.

Существуют также определенные проблемы с самим кодом. Предполагается, что $ username в кавычках, будет работать неэффективно. И поскольку вы сравниваете строку $ username с тем, что возвращает база данных, она явно не экранирована должным образом, что означает, что код открыт для атак с использованием инъекций. Поскольку вы используете PDO, решение состоит в том, чтобы просто использовать подготовленную привязку оператора / переменной - что вы фактически сделали с INSERT!

Итерация по каждому подходящему имени пользователя не имеет никакого смысла и серьезно подрывает поведение. Лучший подход (но не правильный) будет:

/**
 * @param username string - a candidate username
 * @param DBH - connected PDO object referencing user database
 * @return bool - true if username does not exist already
 *
 * search the current list of users to see if the candidate username is available
 */
function checkName($username, $DBH){      
  $STH = $DBH->prepare('SELECT username FROM users WHERE username = :username');
  $STH->execute(array(':username'=>$username));
  $STH->setFetchMode(PDO::FETCH_OBJ);
  $row = $STH->fetch();
  if ($row === false) {
     die('whoops!');
  }
  return $username!==$row->username;
}

Правильное решение: если предположить, что ваши имена пользователей уникальны (и они действительно, ДЕЙСТВИТЕЛЬНО должны быть), тогда не беспокойтесь о проверке, существует ли имя пользователя до INSERT - создайте уникальный индекс и проверьте наличие ошибок дублирования ключа после INSERT.

Далее вы используете try / catch для начального соединения, но не проверяете ошибки в последующем запросе. Получив исключение, хотя вы записываете и сообщаете об ошибке, вы, кажется, не обращаетесь к потоку управления на этом этапе, чтобы предотвратить выполнение остальной части кода.

Извините - это не очень хороший код, не говоря уже о безопасности.