Pull to refresh

Comments 148

Так не лучше?
$sql = "SELECT user FROM userslist WHERE userid='".mysql_real_escape_string($_GET['uid'])."'";
UFO just landed and posted this here
да, еще бы все так писали…
UFO just landed and posted this here
Биндинг к MySQL API практически прямой. На Си почти те же самые функции.

Уже объявлено depricated.
Простите, а где написано что она deprecated? И чем ее предлагается заменять, кроме ORM?
Сорри, не depricated вроде, а not recommended (был уверен в первом). На замену mysqli или PDO_MySQL.
о как, спасибо! Где-то уже натыкался на mysqli, но решил тогда, что это вообще другая СУБД.
Как по мне, так полная ерунда. С таким же успехом можно признать не пригодными еще достаточное количество полезных функций. Составляя запрос нужно думать как человек, который планирует SQL-инъекцию. Сама по себе mysql_query ни хорошая и не плохая — это просто инструмент, а как им пользоваться — личное дело каждого.
а аргумент что mysql медленее mysqli для вас тоже не существенен?
Это другой вопрос. Но в плане безопасности я особого изъяна не вижу. Естественно, что mysqi приоритетней.
UFO just landed and posted this here
Мне вообще непонятно, чем думали авторы PHP, когда добавляли туда mysql_query.

А почему нет? Их вины в небезопасном использовании функции разработчиками помоему нет.
UFO just landed and posted this here
Весело, программистов уже сравнивают с маленькими детьми. Думать головой уже не модно, нужно побольше магических функций которые делают всё за тебя.
Когда этот «программист» валит все свои проблемы на фреймворк, то вполне нормально сравнивать его с ребенком, который пробует все подряд без вникания в подробности зачем это сделано именно так.
В вопросах безопасности всех людей по умолчанию нужно считать маленькими детьми.
А почему MySQL делали такую функцию? Это как бы язык программирования, а не игрушка для тех кто младше 5 лет.
Во всех книгах по PHP написано, не доверять данным которые пришли из web.
не доверять данным которые пришли из web.

Вот-вот :)
Лучше так:
$result = $users->get( $params->uid );
UFO just landed and posted this here
нет, зачем для числа вызывать дорогую функцию mysql_real_escape_string, если приведение типов дешевле?
Только реальное экранирование, для реальных пацанов.
А вообще среди моих знакомых это частая практика — использовать громоздкие решения, там где есть изящные.
Да это ж любимая фишка гавнокодеров, они прям мечтают найти одну функцию которая от всех проблем помогает. Они бывает еще strip_tags и htmlentities добавляют для чисел, потому что где вычитали, что это защищает от XSS :)
Плюс к комменту выше: ещё не нужный оверхид с формированием и передачей кавычек (пускай пара байт, но всё же) и приведением строки в число уже в СУБД (тоже мелочь, но всё же). То есть ни одного плюса у этого варианта по сравнению с приведением типа в скрипте нет. Зачем он?
Между прочим, приведение типа — не мелочь. У меня как-то раз один запрос работал на два порядка медленнее из-за сравнения числа со строкой, в то время как можно было сравнивать напрямую два числа.
Вероятно потому, что из-за кастования субд не смогла использовать индексы. В то время как во время генерации запроса операция приведения типа ничего не стоит.
Да, думаю, именно из-за невозможности использования индексов.
О как. Не зря значит привожу тип в php, а не в мускуле. Хотя чисто из эстетических соображений.
Ну вообще говоря у mysql были (может сейчас уже исправлено) ситуации, когда по непонятным причинам он в выражениях вроде

WHERE int_field = 'int_value'

приводил к числовому типу не правую, а к строковому левую часть. И получали фуллскан. Сам лично видел такое, да и в интернетах статьи проскакивали о.
Никогда так не писал, потому не встречал, наверное. Но в чужом коде попадалось, да — иногда исправлял, иногда нет. Теперь всегда буду.
профайлинг — наше все.
Заведомо числовые данные, пришедшие от пользователя, проверяю с помощью RegExp.

Текстовые данные прогоняю через функцию
mysql_real_escape_string

Ведь правильно же?
Зачем регулярки и почему не использовать intval() или приведение типов?

Да и вообще лично я делаю запросы с использованием placeholder's в любых ЯП где сталкиваюсь с SQL базами данных.
placeholder — это очень правильно.
Плейсхолдер плейсхолдеру рознь. Наверное, вы всё же имеете в виду prepared statements.
Хммм… ну да, а можно поподробнее о разнице?
sprintf — тоже placeholder…
Ну, $sql = sprintf("SELECT * FROM users WHERE id = %d", $_GET['id']); не самый худший вариант экранирования для чисел, по-моему.
только медленный, если сравнивать с «SELECT * FROM `users` WHERE `id` = ».(int)$_GET['id']
Просто placeholder — это общее понятие, которое просто обозначает место в шаблоне, в которое будет подставлено значение.

Например, в C#:
String.Format("SELECT * FROM table WHERE column = {0}", "значение");

«Значение» будет подставлено вместо placeholder'а {0}. Но это не значит, что это безопасно с точки зрения SQL. То же самое и в PHP:
sprintf("SELECT * FROM table WHERE column = %s", "значение");

%s — плейсхолдер. Но к безопасности такие плейсхолдеры отношения не имеют.

В то же время, одно из предназначений prepared statements — как раз безопасность. Они тоже подставляют значения в плейсхолдеры, но делают это не напрямую (в виде строки), а в виде отдельных параметров-значений, на уровне базы данных.
%d приводит значение к целочисленному типу, причём делает это нестрого и втихую. Но да, %d поможет избежать инъекций.
А зачем выполнять запрос, если данные не корректны? intval _всегда_ вернет число, даже если пришла строка.
if( is_int($id) )
{
«select * from users where id=». $id;
}
И не нужноничего экранировать. И лишнего запроса не будет.
is_int($_GET['id']) — всегда false
Для начала сравните мой кусок со своим, а потом что-то говорите =)
Специально для вас можно использовать is_numeric, подойдет для обоих ситуаций.
А мускул поймёт id = 0xff?
echo is_numeric('0xff'); // 1
Вообще я вел к тому что интвал всегда возвращает число, даже если строка прийдет, и будет очень неприкольно, если в базе есть данные со значением 0, да и в любом случае не прикольно выполнять запрос, если он в принципе не нужен. Поэтому считаю что нужно сперва проверить число ли пришло, и затем, если да, выполнять уже запрос. Как-то так =) Ато лепят сразу преобразовав, а ошибку человеку увидеть не судьба.
Запрос вроде id=0 выполнится мгновенно, потому что 0 < MIN(id), так что мускул даже пытаться искать по индексу не станет.

Согласен по части «зачем делать лишнюю работу», но с точки зрения программиста — написание этого условия это как раз та самая лишняя работа :-)
стоит уточнить, что id может быть таки и отрицательным, особенно если не auto_increment :)
Технически — естественно :-) Практически — приведи пример, когда это имело бы смысл?
например когда мы говорим не про первичный ключ и вообще не про индекс.
Ну в данном случае мы говорим про первичный ключ id ;-) И я комментировал очень специфичную ситуацию
в целом да, выше скорее всего шла речь про первичный ключ, иначе не знаю, что еще может означать id в таблице users и выборка по нему, ну кроме случаев лапше- и индусо- кода ;)
Зависит от ситуации. Лично я на практике в Postresql использую отрицательные значения id для подчеркивания факта наличия «системных» записей. Это как номера TCP портов до 1024 или же uid-ы в линях до 1000-чи.

Просто к основному sequence инкремент возрастающего вверх от нуля есть sequence для системных записей который инкрементится от нуля вниз.

В общем в итоге это не более чем принятое соглашение. Какого соглашение для практического использования приняли, такое и работает.
Регуляркой делать.
Прекрасно выбирает строки с айди 19 =)
SELECT *
FROM transactions
WHERE user_ = 0x13
Вы ответили на комментарий, где автор рекомендует использовать intval. Написали, что intval вернет всегда число, поэтому надо делать is_int. И откуда тогда в контексте возьмется $id? Я вижу два варианта: $id = $_GET['id'] и $id = intval($_GET['id']). В обоих случаях смысл вашего ответа непонятен.

Функцию is_numeric я знаю. И если вы хотели написать ее вместо is_int, то достаточно было бы ответить, что вы перепутали функции.
>Для защиты от этой шаблонной уязвимости, лучше всего использовать приведение типов.

ОК, а если в get-параметре передаётся не число, а строка?
тогда как в примере, только в обрамлении кавычками
Вы получите в приведенном значение 0, что не есть ошибка безопасности.
Я обычно использую следующий подход:
$id = (int)$_GET['id'];
А как обезопасить себя, если в get параметре строка и эта строка нужна пример для поиска по базе и не использовать placeholders?
addslashes поможет?

$sql = «SELECT * FROM `table` WHERE title LIKE '%$search%'»;
Как-то так…
$search = base64_encode("%$search%");
$sql = "SELECT * FROM `table` WHERE title LIKE FROM_BASE64('$search')";
А для точного поиска можно использовать md5.
Жесть какая. Оверхед же.
Кто же спорит? Вот человек не может использовать подготовленные выражения, а такой способ позволит пускать строку как есть и при этом не бояться.
mysql_real_escape_string и кавычки чем хуже?
mysql_real_escape_string — изменяет строку.
base64_encode не изменяет что ли?
FROM_BASE64 — изменяет обратно.
Гм, ну и что? В чём недостаток-то? Помещение строки, пропущенной через mysql_real_escape_string тоже изменяет обратно в недрах MySQL.
Признаю свою ошибку. Не прав. Это, возможно, от того, что я не пользуюсь этой функцией.: )
Если применять mysql_real_escape_string к INSERT/UPDATE, а потом сделать SELECT, то строка вернётся в первозданном виде.
угу. вот только FROM_BASE64 в mysql появилась только в версии 5.6…
Ох как меня пропёрло. Действительно. наверное, плохая погода на меня подействовала так…
Для поиска я использую полнотекстовый поиск, но планирую перейти на Sphinx
Я пользуюсь MariaDB, там есть некоторая интеграция Sphinx в движок, правда я не разбирался насколько удобная интеграция.
Почему не использовать placeholders? Это к тому же наглядно:

$db = new mysqli(...);
$stmt = $db->prepare('update table_name set var1 = ?, var2 =? where var3 = ?');
$stmt->bind_param('sdd', $var1, $var2. $var3);

if ($stmt->execute())
{

}

При этом, в теории (ну не нужно мне было это проверять), подготовленный запрос можно использовать несколько раз вроде как без «перекомпиляции». А во вторых можно использовать именованные placeholders, в случае с PDO.
Я имел ввиду, что это не годится для случаев, когда там и должна быть строка.

Например, так:

$username = $_GET['username'];
// обеззараживаем $username
// ...
mysql_query("SELECT ALL FROM blablabla WHERE username = "{$username}");


Впрочем, это я уже придираюсь, наверное.
Если в таком запросе должна быть строка, то из-за отсутствия кавычек банально ничего не заработает и даже самый ленивый и неумелый программист пофиксит еще до инъекции.
Есть такая вещь как унаследованный код. И совершенно случайно работа с БД в нём в отдельный слой не выносится, как правило, хорошо если хоть какие-то слои есть.
Я о том только, что если ты даешь советы, — напиши основное.
ИМХО если мы тыкаемся в legacy код и видим в нем грязные mysql_ с невнятным поведением при некорректных аргументах — мы просто берем и заменяем эти вызовы на *ABSTR_DB_LAYER* (мы же новый код не по тем же принципам пишем, верно? у нас абстракции, модульность, программирование на уровне интерфейсов и вообще лучшие практики курят в сторонке).
Не всегда это просто.
Да. Не всегда.

У меня недавно был проект без архитектуры на инклудах, где магическая переменная магической погоняет. Это настолько классический случай лапши и плохого дизайна, что все сурсы можно запросто выкинуть на govnocode и получить +300 их рейтига.
Тогда я честно сказал заказчику, что если он хочет от меня что-то новое или правки старого функционала, это значит что мы будем удалять(выжигать железом) старый код и внедрять новый.

Понимаю, что не всегда дают деньги и время на это, но если нет времени на рефакторинг, значит с менеджментом что-то не так.
У «лапши» зачастую есть одно важное свойство — она работает худо-бедно и объяснить заказчику (или партнёру в моем случае), что нужно месяц минимум просто приводить в порядок код без всякого изменения функциональности очень сложно.
Тут было чуть попроще — я наладил обмен между ERP и e-commerce системой и с новыми данными половина системы не завелась. Из-за хардкода id, названий, неявных параметров.
Так что тут либо отказываться от обмена, либо переписывать компоненты. Заказчик в тему вьехал и дал добро.
Простите, а подход «не использовать вообще конкатенацию запросов, только передачу параметров» в PHP невозможен?
С расширением mysql — нет.
С расширением mysql и небольшой оберткой — да.
Это пишут в манах и учебниках «PHP за 1 час»?
По-вашему нет ничего среднего между prepared statements и ORM?
Есть. Но никто об этом не говорит/не пишет, хорошо если про prepared statements напишут.
Ээээ… ну тогда понятно, почему это такая больная тема.
зато уже давно есть mysqli и я не понимаю почему кто-то еще до сих пор может использовать mysql_*, кроме как на давно устаревших проектах.
одно непонятно — зачем пускать к базе непроверенные запросы-то? :) Одного if жалко?

$g_uid = intval($_GET['uid']);
if ($g_uid == $_GET['uid']) {
$sql = "SELECT user FROM userslist WHERE userid=".$g_uid;
}


Переменная дешевле чем два intval, а if дешевле чем холостой запрос к базе.
Почему два intval'а то? Один. И у вас один. Только лишние if и переменная. Тем более параметров зачастую не один, а больше. Соответственно, вы что, будете в if делать проверку 5-10 переменных?
А почему бы и нет, если это касается безопасности.
Если превратить код в помойку ненужными проверками, то безопаность от этого только проиграет. Нет ничего страшного в выборке «WHERE id = 0», которая случается только в случае крайне редких махинаций. Да и если постоянно идут запроса «WHERE id = N» (N > 0 ), то «WHERE id = 0» явно не повесит сервер.
>Если превратить код в помойку ненужными проверками

Это зависит от уровня восприятия. Если для вас if и пустые запросы в порядке вещей, то для меня это не приемлемо.
Первый вариант:
if (!isset($_GET['id'])) Core::forward404();
$topicId = intval($_GET['id']);
if ($topicId < 1) Core::forward404();
$db = Core::getDatabase();
$topic = $db->selectRow('SELECT * FROM topics WHERE topic_id = ?d', $topicId);
if (!$topic) Core::forward404();
...


Второй вариант:
$topicId = isset($_GET['id']) ? intval($_GET['id']) : 0;
$db = Core::getDatabase();
$topic = $db->selectRow('SELECT * FROM topics WHERE topic_id = ?d', $topicId);
if (!$topic) Core::forward404();
...


Я обычно использую второй, потому что разницы по сути нет, а вот писать каждый день сотни лишних строк я не хочу (времени жалко).
а так
$topicId = intval($_GET['id']);
if ($topicId) {
 $db = Core::getDatabase();
 $topic = $db->selectRow('SELECT * FROM topics WHERE topic_id = ?d', $topicId);
 if (!$topic) Core::forward404();
}
А так имеем:
1. Notice
2. Необработана ситуация topicId = 0
3. Запросы в базу при отрицательном topicId
Запрос, в котором не выполняется условие intval($_GET['uid']) == $_GET['uid'], может быть вполне корректен семантически. Да и даже если нет, проще сделать холостой запрос, чем «пачкать» код проверками, если число холостых пренебрежимо мало.
Можно несколько проще
if (is_numeric($_GET['uid']))
...
Вы ПХП-то знаете вообще? Суньте в _GET например такую строку: «9 AND UNION SELECT 1 FROM DUAL». Она чудесно пройдёт проверку.
К строке

$sql = "SELECT id FROM userslist WHERE username='" . mysql_real_escape_string($_GET['username']) . "'";

К числу

$sql = "SELECT user FROM userslist WHERE userid='" . intval($_GET['uid']) . "'";
Я обычно так пишу. Почти))

$sql = "SELECT `user` FROM `userslist` WHERE `userid`='" . intval($_GET['uid']) . "'";


Не знаю, есть ли какой-то дополнительный смысл в наличии/отсутствии кавычек вокруг имен таблиц и столбцов, но мне так проще читать код
В кавычках вокруг intval смысла нет, если userid число.
Я хотел бы услышать комментарий про другие кавычки)

В любом случае, предпочитаю ставить кавычки везде, в том числе вокруг intval
Без их использования есть риск потерять совместимость с будущими версиями (My)SQL. Также иногда можно получить ошибку при попытке использовать зарезервированное слово и приходится оборачивать (если увидел ошибку). Потому правильным считаю ставить в запросах для единообразия, но зачастую лень, если специального требования нет. Если же речь идёт о чём-то универсальном, где имя таблицы/поля заранее неизвестно (например задаётся пользователем), то нужно ставить однозначно.
Комментарий на эту тему: FYI: escaping table/field names is only required if you're using reserved words. Dumping everything into backticks just makes the query that much harder to read.
Не каждый держит в голове весь список зарезервированных слов :) Да и вдруг когда-нибудь появятся новые, а древний код все еще будет использоваться.
Кстати, лучше всё-таки „(int)“, в ПХП вызов функции — дорогая операция.
наконец-то кто-то это написал. Почему везде все пишут про intval(), когда дешевле и лучше просто воспользоваться приведением типов?
Я использую библиотеку DbSimple от Котерова. На сколько мне известно, она используется и на этом движке, на котом хабр.
Аналогично. Использую допиленный DbSimple. Плюс писал свою версию с нуля для MSSQL, так как очень не хватало привычных методов при работе с этой СУБД.
Ссылка: dklab.ru/lib/DbSimple/ (много примеров)

$ids = array(1, 101, 303);
$db->select('SELECT name FROM tbl WHERE id IN (?a)', $ids);
...
$name = $_GET['name'];
$db->selectRow('SELECT * FROM users WHERE name = ?', $name);
...
$row = array('name' => 'Hint', 'age' => 25);
$db->query('INSERT INTO users SET ?a', $row);
Выглядит красиво, но как производится сама проверка?
В комментарии я привел далеко не все плюшки.

Все подстановки с "?" экранируются как строки. Все элементы массивов при подстановке "?a" экранируются как строки, а ключи массивов при UPDATE и INSERT как идентификаторы (`name`).
Для подстановок с "?d" выполняется intval:
$topic = $db->selectRow('SELECT * FROM topics WHERE id = ?d', $_GET['id']);
Использую goDB, там всё тоже, только вместо множества методов указывается формат в котором представлять результат:
$db->query('SELECT * FROM users WHERE name=?', array($name))->row()
А разв Хабр на php? Я почему-то думал, что он на asp.
Ага, а куки PHPSESSID для маскировки :)
Обычно использую:
".select .. from ... where strcmp(field,'".$mustBeString."')=0 or id=".(int)$mustBeInt.";"
+ ессно автоэкранирование кавычек в строках
Мой сайт хакали именно через такую SQL инъекцию. Век живи, век учись.
если вы не знаете через что именно хакнули (какой запрос), как вы можете быть уверенным, что хакнули именно через SQL-инъекцию?
Взломщик оказался воспитанным и указал мне на ошибку.
повезло. По-больше бы таких взломщиков.
Если заведомо число передается, всегда делают some sql query where id=".(int)$value."… а вообще стараюсь чтобы всегда только числа передавались, а уж если строка, то кучу перепроверок + экранирование.
Тут народ intval предлагает, чем то лучше или хуже (а то вдруг 6 лет зря (int) пихаю)?
(int) будет быстрее, чем intval()
Функцию, в отличие от модификатора, можно передать калбэком куда-нибудь (не защищаю его, просто предлагаю случаи, когда модификатор использовать не получится)
это уже совсем редкий случай, как мне кажется
Ну я просто привел пример различий :-)
Только параметризованные запросы. Никаких конкатенаций. В PHP есть параметризованные запросы? Простите давно не писал на нем, не помню.
А под фреимворки регулярки не составляли?)
Когда нарочно обходят использование preapared statements конкатенацией.
Не составлял, сложно и муторно. Есть средства статического анализа кода, они позволяют выполнять эту работу более комплексно и не привязаны к конкретным конструкциями.
Средства статистического анализа кода, написанного на PHP-фреймворках?
Не подскажите какой-нибудь? Буду благодарен.
Статистический анализ кода так или иначе привязан к конкретным конструкциям.
у нас своя разработка статического анализатора. Статический анализ привязан скорее уже не к конструкциям, а к структуре кода, скорее всего вы про это и говорили. Поэтому такие вещи как RIPS не справляются с ООП. Но если знать, что хочется найти, то, скажем, выполнить задачу «поиск всех функций где аргумент попадает в SQL запрос без фильтрации» выполнить можно быстро и качественно.
А, понятно, у вас свой анализатор. Просто думал какой-то инструмент используете с уже собранными регулярками.
Действительно, теперь про анализ мы друг друга поняли.
С последним, да, у меня у меня именно так: наткнулся на уязвимую конструкцию и грепаешь весь код на подобные, действительно, быстро и качественно.
Ага, тоже так делал, пока не устал эти регулярки писать. Очень много получается веб-приложений и все такие разные, что пришлось изобретать какую-то утилиту под свои нужды. В целом, посмотрите на TXL (http://www.txl.ca/ www.txl.ca/examples/Grammars/PHP/README.txt)
только экранирование, только хардкор!
Only those users with full accounts are able to leave comments. Log in, please.