Недостатки моего кода

[li4e]

Добрый день, собственно вопрос в том почему код ниже является плохим?

Появилась необходимость запретить просмотр определенной страницы посторонним и решил изучить php и реализовать хоть какую никакую авторизацию и разграничение доступа! Так то я сделал и все работает, но есть сомнения на счет безопасности!

 

Сомнения появились, после того как я просмотрел примеры авторизации на различных сайтах, они там вместе с сеансами используют еще и куки, и пароль шифруют. Но как я понял все переменные $_SESSION хранятся на сервере, тогда чего мне бояться? Заранее спасибо!

 

Файл "check.php" его я размещаю в начало страниц, к которым нужно закрыть доступ

<?phpsession_start();if (isset($_GET['logout'])) { //Это для выхода из аккаунта, просто GET запросомunset($_SESSION['auth']);}if (isset($_SESSION['auth']) AND ($_SESSION['level'] == 1)) //Проверка, авторизован ли пользователь и какими правами доступа обладает{}elseif (isset($_SESSION['auth'])) {header('Location: /index.php');//Если авторизован, но без прав доступа, то редиркет на гланую сайтаexit();}else{header('Location: /admin/auth.php'); //Если не авторизован, то предлагаю авторизоваться}?>

А вот и сам код авторизации "auth.php"

<?phpinclude '../config.php'; //Подключаю конфиги для соединени с бдsession_start();if ($_POST['submit']) //Действия по нажатию кнопки войти{if (empty($_POST['login']) OR empty($_POST['password'])) //Проверка на заполненность полей, чтобы не были пустыми{$error = "<p id='error'>Заполни поля!</p>";}else{//дальше идет поиск введенных значений в бд$data = mysql_fetch_array(mysql_query("SELECT login, password, level FROM users WHERE login='".$_POST[login]."' AND password='".$_POST[password]."'"));if (($_POST['login'] == $data[login]) AND ($_POST['password'] == $data[password])) {$_SESSION['auth'] = '1';$_SESSION['level'] = $data[level];header('Location: /admin/index.php');}else{$error = "<p id='error'>Неверный логин или пароль!</p>";}}}if (isset($_SESSION['auth'])) //Если пользователь уже авторизован, его кидает на главную страницу админки{header('Location: /admin/index.php');}else{include "tmp/header.php";echo '<div id="nav-wrapper">	<ul id="sidebar">		<li><a href="../index.php">Главная</a></li>	</ul></div><form method="POST" id="auth">'.$error.'<h1>Вход в ПУ</h1>  Логин <br><input class="s" name="login" type="text"><br>   Пароль <br><input class="s" name="password" type="password"><br>   <input style="width: 100px;" name="submit" type="submit" value="Войти">   </form>';include "tmp/footer.php";}?>

Edited November 18, 2013 by li4e

[rus]

и правильно сомневаетесь, почитайте хотя бы вот это.

[li4e]

Спасибо, сейчас почитаю!

Значит после успешной авторизации, мы должны назначить уникальный идентификатор пользователю и записать его в бд в строку пользователя, заранее создав спец поле для этого?

Ну и затем занести в куки?

[CoDy]

НИКОГДА так не делай:

mysql_query("SELECT login, password, level FROM users WHERE login='".$_POST[login]."' AND password='".$_POST[password]."'")

Давать возможность пользователю воткнуть данные в SQL-запрос "как есть", без "фильтрации" - первая "дыра" в безопасности, которую будут искать.

Вот во что пойдет на выполнение, если закинуть в $_POST[login] строку "1' OR true limit 1; -- \"(это называется SQL-инекция):

SELECT login, password, level FROM users WHERE login='1' OR true LIMIT 1; -- \' AND password=''

Как минимум строки нужно пропускать через addslashes(), числа перегонять в числа через (int/float):

mysql_query("SELECT login, password, level FROM users WHERE login='".addslashes($_POST[login])."' AND password='".addslashes($_POST[password])."'")

Еще обязательно проверяй количество найденных записей по паре значений логин и пароль. Мало того, что они должны существовать, - они еще должны быть в единственном числе.

[iKNG]

НИКОГДА так не делай:

mysql_query("SELECT login, password, level FROM users WHERE login='".$_POST[login]."' AND password='".$_POST[password]."'")

Давать возможность пользователю воткнуть данные в SQL-запрос "как есть", без "фильтрации" - первая "дыра" в безопасности, которую будут искать.

Вот во что пойдет на выполнение, если закинуть в $_POST[login] строку "1' OR true limit 1; -- \"(это называется SQL-инекция):

SELECT login, password, level FROM users WHERE login='1' OR true LIMIT 1; -- \' AND password=''

Как минимум строки нужно пропускать через addslashes(), числа перегонять в числа через (int/float):

mysql_query("SELECT login, password, level FROM users WHERE login='".addslashes($_POST[login])."' AND password='".addslashes($_POST[password])."'")

Еще обязательно проверяй количество найденных записей по паре значений логин и пароль. Мало того, что они должны существовать, - они еще должны быть в единственном числе.

И то же самое, если будете использовать Cookie. Их тоже можно легко подменить на SQL инъекцию. А вообще лучше не addslashes, а htmlspecialchars.

[CoDy]

Ну htmlspecialchars обычно полюзуют при вставке в базу именно html-кода, который в последствии будет воспроизводиться на странице, дабы не воткнули туда всякой гадости типа скриптов. А для данных типа логина, думаю, достаточно и экрана: строки не "раздуваются". Дастаточно htmlspecialchars пользовать при выводе логина, как данных на сайте.

Хотя это уже дело логики.

[li4e]

Спасибо, все понял, все правлю! Вот именно таких советов, как "Никогда так не делай" с объяснениями Я и ждал! Как доделаю скину!

[rus]

достаточно будет написать вот такую простенькую ф-цию:

	// Функция экранирования переменных	function quote_smart($value) {    // если magic_quotes_gpc включена - используем stripslashes    	if (get_magic_quotes_gpc()) {			$value = stripslashes($value);    	}    // Если переменная - число, то экранировать её не нужно    // если нет - то окружем её кавычками, и экранируем    	if (!is_numeric($value)) {			$value = mysql_real_escape_string($value);    	}   		return $value;	}

использовать так:

$login =$this->quote_smart($_POST['login']);

и можно особо не париться на счет инъекций.

[li4e]

Все таки Я еще раз прочитал статью, ссылку на которую вы скинули выше! И почитал про различные методы авторизации, конечно с тем что надо обрабатывать вводимую пользователем информацию все ясно, но вот все никак не могу понять логику, почему нельзя использовать для авторизации только сессии? Ведь они же все равно хранятся на сервере и не доброжелатель не сможет никак подменить данные, как в случае с куками! Объясните пожалуйста!

[Veseloff]

rus, не советуй людям глупости всякие. Модуль mysql уже устарел. А использовать надо prepared statements и всем будет счастье.

[li4e]

А что касательно авторизации?

[rus]

rus, не советуй людям глупости всякие. Модуль mysql уже устарел. А использовать надо prepared statements и всем будет счастье.

почему сразу глупость? ведь на жигули "шестерках" до сих пор катаются, разве нет?

pdo конечно крут, но списывать со счетов mysql пока что рано мне кажется.

к стати, перевод по твоей ссылке: http://php.net/manual/ru/pdo.prepared-statements.php все-же читается лучше )

[rus]

Все таки Я еще раз прочитал статью, ссылку на которую вы скинули выше! И почитал про различные методы авторизации, конечно с тем что надо обрабатывать вводимую пользователем информацию все ясно, но вот все никак не могу понять логику, почему нельзя использовать для авторизации только сессии? Ведь они же все равно хранятся на сервере и не доброжелатель не сможет никак подменить данные, как в случае с куками! Объясните пожалуйста!

да можно в принципе и без кук, просто куки - это для удобства идентификации вас (вашего браузера) при повторном входе, а в плане защиты все-таки нужно использовать шифрование пароля.

[li4e]

Т.е просто шифровать пароль, и использовать такую авторизацию как у меня выше будет нормально в плане безопасности? Ну естественно обрабатывая вводимую информацию предворительно.

[rus]

да, можно.

как пример:

/*** Функция для генерации соли, используемоей в хешировании пароля** возращает 3 случайных символа*/function GenerateSalt($n=3){	$key = '';	$pattern = '1234567890abcdefghijklmnopqrstuvwxyz.,*_-=+';	$counter = strlen($pattern)-1;	for($i=0; $i<$n; $i++)	{		$key .= $pattern{rand(0,$counter)};	}	return $key;}

использовать:

$salt = GenerateSalt();$hashed_password = md5(md5($password) . $salt);

теперь в $hashed_password находится зашифрованный пароль.

[li4e]

Да с этим и так было понятно, спасибо!

[Int]

if (isset($_SESSION['auth']) AND ($_SESSION['level'] == 1)) //Проверка, авторизован ли пользователь и какими правами доступа обладает {}elseif (isset($_SESSION['auth']))  {}else {}

Тут можно чуть облегчить понимание кода, убрав повторяющееся условие

if (isset($_SESSION['auth'])){if ($_SESSION['level'] == 1) {}else {}}else {}

А ещё не нужно писать $_POST[password], потому что контанта password наверняка не определена. Такое прокатывает в Perl.

$_POST['login'] == $data[login]

Вот же, про кавычки знаешь, но почему-то тут же забываешь. Или где-то всё же есть константы, просто не показаны?

С заменой || и && на or и and нужно быть поаккуратнее, если делаешь это не сознательно, потому что приоритет у "буквенных" оператров ниже, чем у присваивания и тернарного оператора, тогда как у || и && — выше.

[li4e]

if (isset($_SESSION['auth']) AND ($_SESSION['level'] == 1)) //Проверка, авторизован ли пользователь и какими правами доступа обладает {}elseif (isset($_SESSION['auth']))  {}else {}

Тут можно чуть облегчить понимание кода, убрав повторяющееся условие

if (isset($_SESSION['auth'])){if ($_SESSION['level'] == 1) {}else {}}else {}

А ещё не нужно писать $_POST[password], потому что контанта password наверняка не определена. Такое прокатывает в Perl.

$_POST['login'] == $data[login]

Вот же, про кавычки знаешь, но почему-то тут же забываешь. Или где-то всё же есть константы, просто не показаны?С заменой || и && на or и and нужно быть поаккуратнее, если делаешь это не сознательно, потому что приоритет у "буквенных" оператров ниже, чем у присваивания и тернарного оператора, тогда как у || и && — выше.

Нет, константы не описаны, просто не набил ещё руку. Насчёт остального спасибо, учту!

[iKNG]

$_SESSION['auth'] = '1';

Тут лучше делать так:

$_SESSION['auth'] = true;

Тогда можно будет делать просто так:

if ($_SESSION['auth'])...

Да и 1 бит (вроде столько занимает в ОЗУ boolean) меньше символьного значения 1 (вроде 1 байт), что позволяет экономить процессорное время и ресурсы сервера.

Edited November 22, 2013 by iKNG

[CoDy]

Да и 1 бит (вроде столько занимает в ОЗУ boolean) меньше символьного значения 1 (вроде 1 байт), что позволяет экономить процессорное время и ресурсы сервера.

 

Минимальная адресуемая единица памяти 1 Байт.

[iKNG]

 

Да и 1 бит (вроде столько занимает в ОЗУ boolean) меньше символьного значения 1 (вроде 1 байт), что позволяет экономить процессорное время и ресурсы сервера.

 

Минимальная адресуемая единица памяти 1 Байт.

 

Все равно, с булевым типом в данном случае удобнее, чем со строками.