Pull to refresh

Comments 83

А как «квуатирование»?

$select = $db->select()
->from(array('p' => 'products'),
array('origin' =>
'(p.'. $db->quoteIdentifier('from'). ' + 10)')
);


или

$where = $table->getAdapter()->quoteInto('bug_id = ?', 1234);
А, вообще, если говорить о сырой отправки данных PDO сам позаботиться о безопасности запроса если использовать метод prepare().

Пример:
$db = new PDO('mysql:dbname=someDB;host=someHost');
$statement = $db->prepare('SELECT something FROM someTable');
$statement->execute();
$results = $statement->fetchAll();


Проявление беспечности при написании запросов, в частности неиспользование фильтрации входных данных от пользователя да и не только сравнимо с «отрывом» в Таиланде без презерватива :-)
UFO just landed and posted this here
Конструкцию типа «SELECT * FROM ?» PDO::prepare обработает корректно? Честно не знаю. Когда в последний раз пробовал работать с параметрическими запросами, относящимися к схеме, а не данным, мне просто ошибку выдавало.
UFO just landed and posted this here
Не заметил, чтобы PMA их жёстко фильтровал — вроде как выдаёт ошибки самого мускула при неправильном указании имении поля или таблицы. PMA — быдлокод в принципе из-за этого?
UFO just landed and posted this here
разрешено вводить сам текст запроса

Это крайний случай. Я же имел в виду задачу типа «добавить поле в таблицу, имя таблицы и поля задаёт пользователь». У пользователя два поля ввода: «имя таблицы» и «имя поля».
UFO just landed and posted this here
Можно, конечно. Но проще бы было если бы сам мускул (либа) выдали «Incorrect table name» в случае если бы мы заранее параметризировали запрос «CREATE TABLE ?», а ещё лучше автоматически заключили бы его в обратные кавычки с экранированием этих кавычек в параметре.
И как же «экранировать» обратные кавычки в mysql?
Ни одна субд не позволяет таким образом параметризировать запросы. И это правильно. Потому что запрос должен быть консистентным — или он синтаксически корректен, или некорректен.
Так за 10+- лет никто этого и не реализовал? :(

Я, честно говоря, не разбираюсь в тонкостях SQL, каким должен быть запрос тем более, но задача редактирования схемы либо составления произвольных запросов на выборку передо мной встаёт постоянно. И я не понимаю почему я не могу подготовить такой запрос, чтобы СУБД (или её либа) экранировали по мере возможности вызовы с параметром в виде имени таблицы, поля или, даже, ключевого слова.

Пришёл синтаксически неверный параметр на этапе подстановки — выдай syntax error. Не пришёл — заэкранируй его в рамках синтаксиса. Дай мне возможность стрелять близко к ноге, так чтобы почувствовал раньше выстрела, но не давай стрелять в неё…
Возможность сгенерировать синтаксически некорректный запрос — это выстрел в голову, а не в ногу.

> Пришёл синтаксически неверный параметр на этапе подстановки — выдай syntax error. Не пришёл — заэкранируй его в рамках синтаксиса

Строка в рамках синтаксиса не означает, что конечный запрос в принципе будет корректный. Решение я уже в этом топике повторил миллион раз — белые списки :-) Не понимаю чего вы так упираетесь-то?
Я бы сказал PDO сам позаботится о безопасности подставных данных если использовать методы bind*(), т.е. placeholder-ы. Само по себе применение prepare() не сделает запрос SELECT * FROM $field = $val безопасным.
Расскажите о том, как PDO заботится о безопасности?
В данном случае не требуется заботится об экранизации кавычек в подставляемых данных.
Разве bind/plaxeholders работает с именами таблиц/полей?
Нет, именно поэтому нельзя сказать, что приложение защищено от инъекций, если используются prepared (об этом и статья, как я понял).
Статья, как я понял, о том, что нельзя доверять интуиции — нужно читать, увы, доки.
Часть запроса я заведомо сделал так: // Выброр поля для сортировки
switch($sort)
{
case 'OpenDate': { $select->order('t0.OpenDate DESC'); break; }
case 'Id': { $select->order('t0.ID'); break; }
case 'Status': { $select->order('t0.StatusID'); break; }
case 'Rating': { $select->order('t0.Rating DESC'); break; }
case 'Number': { $select->order('t0.Number DESC'); break; }
case 'NumberNegative': { $select->order('t0.Number ASC'); break; }
}
Почему не
$sorting = array(
"OpenDate" : "t0.OpenDate DESC",
"Id" : "t0.ID",
"Status" : "t0.StatusID"
);
if ( array_key_exists($sort, $sorting) ) { $select->order($sorting[$sort]); }
Вот вот. Намного лучше.
С точки зрения безопасности это конечно неплохо.
Но с точки зрения кошерности кода, это… не хочется критиковать… В общем мы прям содрогнулись, т.к. это напомнило нам как мы увидели пару лет назад в одном «самодельном» движке больше 100 строк аналогичного кода:)
Копипасту — бой!

p.s.: А вывод данных Вы наверное элегантно делаете как if($title==='Статья') echo 'Статья'; ?:) Для безопасности, во избежание xss?
Все указанные проблемы перестают быть проблемами, если использовать whitelist для полей и таблиц. Никогда не нужно имена полей и таблиц брать непосредственно из пользовательского ввода.
Категоричное заявление.
В крупных проектах иногда требуется возможность динамической подстановки имен полей и таблиц, и без этого никак. Видимо не сталкивались с таким функционалом?
Обычно это так — сначала приходят от пользователя, сохраняются в базу, а уже после подставляются в запрос.
Но это все так же имеет место быть.
Данные из базы точно так же можно (нужно) проверить на наличие в белом списке, не вижу в этом никакой проблемы.

ps: части запроса хранящиеся в БД :-S
К сожалению да, получаются фрагментированные запросы, которые динамически собираются из данных в базе. Но такова бизнес-логика.
Предлагаете хранить для каждого метода список таблиц/полей?
Думаю лучше не допускать unquoted данных :)
Совершенно верно — я предлагаю хранить белые списки, потому что это не только вопрос безопасности, но и запрос функционирования запроса.

Если придерживаться точки зрения, что «при любых входных данных запрос должен генерироваться синтаксически корректным», тогда проблем с любым приложением будет меньше.

А вы фактически предлагаете в качестве имени поля подставлять что угодно, лишь бы оно выглядело как валидное имя.
Я предлагаю изначально выстраивать такую логику, которая на протяжении всей операции (добавление значений пользователем, попадание их в базу, последующее использование с подстановкой в запрос) не подвергнется неожиданному разветвлению.
Я не говорю, что «белые списки» — плохо. Их стоит использовать, если не уверен, или для эдакого «контрольного» выстрела.
> Я предлагаю изначально выстраивать такую логику, которая на протяжении всей операции

Я не совсем понимаю что именно вы предлагаете, но белые списки — единственный способ гарантировать, что сгенерированный вами запрос в принципе будет корректным.

> Их стоит использовать, если не уверен, или для эдакого «контрольного» выстрела.

Ок, приехало значение поля «asd». Предложите без белых списков как это поле корректно вставить в запрос?
Если такое значение попало в базу, значит проблема не на этом этапе, а на предыдущем, где данные получили от пользователя.
И, действительно, на этапе получения данных от пользователя имеют место быть «белые списки», для 100% валидации. С этой частью комментария на данном этапе дискуссии я таки соглашусь :)
Вопрос, где это делать, на первом этапе — получение значений от пользователя, или на финишной прямой — перед самим запросом в базу, вопрос архитектуры.
Если вернуться к моему первоначальному комментарию, то можно заметить, что мой комментарий был о том что делать, а не где делать :-)

Так что всё таки белые списки ftw
Про "где" — это уже меня понесло в думки)
Единственное, что все проблемы этим не решить, возвращаясь к первоначальному комментарию.
Все таки решить :-) Использование белых списков решает все проблемы описанные в статье — в случае если от пользователя приходит имя таблицы/поля.

Если от пользователя приходит целый запрос (как в примере 9) — то это крепкий повод задуматься, а всё ли вы делаете правильно :-)
«в случае если от пользователя приходит имя таблицы/поля» — да, согласен.
Но ведь есть к примеру пункт 4, нет?)

Крепкий повод задуматься, нужен ли Zend, если игнорируются его механизмы)
Это крепкий повод задуматься об архитектуре приложения — вы принимаете или имя, или значение, но не целые выражения или даже запросы. В противном случае просто установите PMA и дайте всем права на запись.
И все же, не все проблемы решают белые списки, возвращаясь к комментарию.
Да, если вы принимаете от пользователя целые выражения или запросы — то такое вылечить уже невозможно.

А ещё белые списки не приносят тапочки и не варят кофе :-)
Ладно, исправлю тогда своё утверждение: белые списки решают все проблемы в нормально спроектированном приложении.
Если приложение кривое-косое, то только в морг.
В примерах конструкции не моего кода) Это обобщенные варианты. Личное обращение немного доставляет.
Как бы не было спроектировано приложение, в where (п. 4) не спасут никакие белые списки, по-моему, это очевидно. Я про него писал 2 комментом выше, и тут только «куотить».
Я повторю в очередной раз — принимать нужно ИЛИ имена полей, ИЛИ их значения, но не целые выражения.

Если принимаются целые выражения — то глупо бегать и кричать что зенд плохой — вы сами выставляете пользователям возможность писать запросы, то лучше установите им PMA.

Перефразируя: это не инструмент виноват, а программист, который не понимает, для чего инструмент предназначен.
Странные предложения пишите.
Я где-то критиковал Zend? Нет — не понятно к чему последнее предложение.
Ирония про PMA — хорошо, ладно.
И первое предложение. Речь я выше вел о where — это как раз значение поля.
> Речь я выше вел о where — это как раз значение поля.

Нет, вы неправы.

$select->from($table);
$value = «1)2'3 --»;
$select->where($value);

В этом примере передаётся не значение поля, а выражение. Для передачи значений в where существует другой синтаксис. И он (сюрприз-сюрприз) вполне себе описан в документации :-)
Дабы не наломать дров.
У меня полшестого утра, я пойду спать, а завтра, со свежей головой перечитаю еще раз всю нашу дискуссию)
Я обходился так: составлял карту соответствий пользовательских имён полей и таблиц и реальных. Правда приложение было прикладное, собственно основная его роль было GUI представление подмножества SQL. Что-то вроде MS Access или 1С, где пользователи могут формировать произвольные запросы и отчёты по базе.
Сейчас решаю задачу добавления столбца в таблицу на основании пользовательского ввода. user_column_1 видеть как-то не хочется.

Не спорю, что структура БД с точки зрения нормализации корявая, но такая досталась.
Добавление — другой вопрос уже.

Для добавления достаточно просто проверить, что имя соответствует правилам составления имён в данной конкретной субд. Просто ограничить ввод буквами, цифрами и подчёркиванием, и не надо больше городить ничего сложнее.
Так потом то, что добавили, нужно будет получать. Я к тому, что если есть возможность изменения схемы, то захардкодить белый список не получится. Или простая проверка, как вы сказали, на соответствие правилам именования. Или при каждом вводе анализировать текущую схему и составлять одноразовый белый список.
Простая проверка работать не будет — потому что она не приведёт к гарантированно корректным синтаксически запросам.
А белые списки — какая разница каким образом они составлены, вручную или автоматически?
Оверхед. На составление динамического белого списка нужен, как, минимум, один лишний запрос к БД. Причём в отличии от захардкоженого появляется возможность его исказить.
Захардкоженный запрос не даёт возможности создавать поля пользователям динамически, вы же сами об этом говорили. Если есть возможность захардкодить запрос, то вообще о чём разговор?
Я говорю не о захардкоженных запросах, а о захардкоженном белом списке параметрах запроса. Грубо говоря, о «SELECT * FROM $_GET['table']», где table на момент создания приложения неизвестна. Если известна (схема зашита на уровне приложения) — проблем нет. А если неизвестна?
Если неизвестна — то выгрести список таблиц из базы, положить в кэш и проверять по нему.

Неужели вы ещё и таблицы позволяете создавать юзерам?!
UFO just landed and posted this here
Ещё CouchDB (лично я её предпочитаю), ещё Гугловская БД. Несть им числа. Но мы говорим об SQL.
Не ожидал такого от зенд фреймворка. Интересно, есть ли во второй бете такие же недочёты?
Нет никаких недочётов, автор использует инструмент не по назначению, а потом делает выводы, что инструмент имеет проблемы.
Для начала момент. Я не использую Zend для разработки. В который раз утверждаете беспочвенно.
Статья больше не для разработчиков, а для пентестеров, которым попадаются приложения на Zend. Т.е. указание на пару моментов, где можно покапать уязвимости.
Возможно, стоит убрать выводы (в которых про проблемы Zend'a ничего не сказано, опять придуманные предложения), и оставить только P.S.
Окей, вы его не используете. Вы пишете о вещах, о которых знаете недостаточно информации, потому ваша статья только и может, что смущать новичков (см. комментарий выше, который я откомментил).

Я подозреваю, что стоит убрать не только выводы, но и пункты, в которых методы данного компонента фреймворка используются не по назначению.
Мне совсем не понятно почему.

$select->from($table);

Не экранирует обратную ковычку.
Потому что не существует способа экранирования обратной кавычки в идентификаторе. Предполагается, что таким экранированием будет удваивание, но в mysql это не работает.

Хотя конкретно тут «проблема» небольшая есть, и о ней стоит сообщить в багтреккер ЗФ, но я не удивлюсь, если «баг» будет закрыт с причиной bogus
А что делать, если программисты используют методы не по назначению?
Статья как раз для таких программистов в т.ч.
И, как уже написал, для пентестеров, которым попадется исследовать продукт программистов, использующих методы не по назначению.
Я специально для этого сделал «момент 1», чтобы исключить подобные комментарии.
Но, видимо, не вышло, т.к. это просто игнорируется и продолжаются пустые дискуссии.
Момент 1 никак всю остальную абсурдность не оправдывает.

Это как говорить что команда rf плохая, потому что она может удалить важный файл.
Сравнивать количество конечных потребителей веба и системных команд — абсурдно.
Вы не раздаете налево и направо доступ к выполнению определенным программам на сервере, так как это не требуется.
А запросы к БД идут постоянно, и, соответственно, код пишется с большей варьированностью.
Потребители веба точно так же не имеют доступа к коду.

Вы написали статью для программистов, которые не потребители, а созидатели. Нормально написанный код (см. с белыми списками) «проблемы» (которых на самом деле не существует) решает. Если программист использует компонент не так, как задумано, то ему ничего не мешает и команду rf использовать не так, как задумано, а потом плакаться о том, что он потерял все важные файлы.
Именно оправдывает. Даже если бы все продукты вами используемые были с открытым кодом вы бы стали каждую строчку этого кода проверять или, хотя бы, статистическому анализу (в случае php ещё интересней) подвергать? Мне кажется, что нет.

P.S. что за rf
volch@home:~/$ rf
rf: command not found
volch@home:~/man rf
Нет справочной страницы для rf
Упс, rm

Про анализ — нигде никем не утверждается, что таблицы будут и должны в принципе экранироваться. Равно как в документации описано, что выражения будут переданы в запрос как есть (потому что иначе никак).

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

Автор предупредил о возможных в таком случае проблемах.
Стало быть и отверткой пользоваться впредь не рекомендуется? Ведь если ей поковыряться в носу, то можно проблем наковырять.

Как-то так что ли?
having — не экранирование указано в документации… в тех редких случаях что там используется используем quoteIdentifier
union — обычно там идет подзапрос, уже составленный через $select->assemble(), т.е. с экранированными данными. так что повторное экранирование излишне.
where — насчет NQ наглая ложь! читаем документацию:
$select->where(' id=?', $userEvilData); — первый параметр и не должен экранироваться.
в $where-массивах в TableAbstract та же фигня — с плейсхолдерами все прекрасно экранируется.

Что касается join-ов, есть недостаток, согласен — но в условии соединения таблиц пользовательский ввод сам по себе смотрится странно, так что я за время долгой практике не упирался в это ограничение.
И ИМХО, все же «то, что ваш инструмент может повести себя не так, как вы ожидаете — факт» — пахнет желтизной. Инструмент ведет В ТОЧНОСТИ как в документации. Я бы еще понял если бы это было недокументированными возможностями:

Замечание: К выражению, указанному вами как условие объединения, не применяется автоматическое заключение в кавычки. Если нужно заключить в кавычки имена столбцов, то используйте quoteIdentifier() при формировании строки условия объединения.

(то же касается других частей. Да и вообще, если бы про where автор не написал, я бы и не начал свой комментарий писать).
Если у Zend программиста хороший стиль написания моделей, то он в функцию $table не когда не передаст, а только $where и $order. далее с $where все просто:
     $select->where("id = ?", $id);

про order() уже сказано выще. Статья ни о чем.
У меня, допустим, хороший стиль, но ТЗ обязывает пользователю возможность создавать свои таблицы и делать выборку из них. Пускай приложение типа phpmyadmin я пишу.
Тогда статью следует начинать со слов: «Допустим есть задача написать менеджер дб на zend, подводные камни...». Здесь задача шире, и она привязана к файловой системе: http://www.mysql.ru/docs/man/Legal_names.html. Т.е. имя таблицы предварительно надо прогонять через preg_match().
Зачем её с этого начинать? Автор перечислил выявленные им неочевидные проблемы с экранированием и прочими возможностями sql-inj. Вы либо учитываете их для своих юзкейсов, либо нет. Но потом не говорите, что вас не предупреждали.

Выше, да, указали, что вроде бы всё ведёт себя в соответствии с документацией. Но положа руку на сердце, вы хоть раз совершали ошибку из-за того, что очевидным вам казалось одно поведение, а оказалось документировано другое. Особенно при разработке на PHP.
Здорово, что выявил, но замечание мое относиться не к тому что, этого не надо знать, а область задач, где может появиться эта проблема очень мала.
Лучше знать о негативной возможности и её учитывать или пренебрегать сознательно («моя задача в этот кргу не входит»), чем не знать, не учитывать и пренебрегать лишь потому что не знаешь.
Области задач, где может появиться проблема, не существует.

Примеры выше сводятся к тому, что «я позволяю юзерам создавать таблицы и поля с любыми именами, а потом мне лень проверить входные данные на наличие в белых списках»
Если вы пишете приложение типа PMA, то вы всё равно или принимаете 2 параметра в виде: имя поля и его значение, и в этом случае имеете возможность сгенерировать валидный запрос (экранирование + белые списки), или передаёте в запрос целое выражение от пользователя, фактически предоставляя ему возможность выполнить всё, что он хочет, одновременно теряя возможность контролировать этот процесс.

Как бы чему удивляться тут?
Тому, что экранирование имён таблиц/полей нужно делать «ручками» вместо того чтобы создать таблицу с именем "'; DROP TABLE users".
Не нужно делать экранирование, нужно пришедшие данные проверить регуляркой на то, что к нам пришли только ожидаемые символы: буквы, цифры и знак подчеркивания. Вы делаете так для имен пользователей, но почему-то не хотите делать тут так же, а тем не менее ситуация аналогичная
Я конечно могу ошибаться, т.к. последний год отошел от программирования на ZF, но насколько я помню метод where является NQ только в том виде, в котором он у вас, т.к. не знает какие данные вы туда будете вставлять и это логично, что экранировать их не стоит. А в случаях, если использовать его полноценно — он автоматически экранирует нужные значения (FQ):
->where('field', 'value');
->where('primary', 5, 'INTEGER');
Sign up to leave a comment.

Articles