Opened 3 years ago

Last modified 2 months ago

#879 reopened дефект

Уязвимость для SQL injection

Reported by: san Owned by: Denis_N
Priority: major Component: БД изделий АДС
Keywords: Cc:

Description

Возможно в базе остались уязвимости для SQL injection.
Нужно проинспектировать код и устранить уязвимости

Change History (13)

comment:1 by san, 2 years ago

Owner: changed from san to Denis_N
Status: newassigned

comment:2 by san, 2 years ago

Для того чтобы навсегда забыть про возможные инъекции, я хотел предложить использовать подготовленные запросы.
Однако в таком случае невозможно будет использовать множественные запросы, что, как я понял, подразумевается в #870.
С другой, стороны множественные вопросы сами по себе повышают риск инъекций, по сравнению с обычными:

Рассмотрение аспектов безопасности
Функции API mysqli::query() и mysqli::real_query() во время работы не устанавливают на сервере специальный флаг, необходимый для выполнения мультизапросов. Отдельная API-функция для мультизапросов позволяет снизить вероятность случайных SQL-инъекций. Злоумышленник может попытаться добавить в конец запроса выражения, вроде ; DROP DATABASE mysql или ; SELECT SLEEP(999). Если ему это удастся, но не будет использоваться функция mysqli::multi_query, сервер не выполнит внедрённое и опасное SQL-выражение.

Хотел бы услышать ещё мнения.

Last edited 2 years ago by san (previous) (diff)

in reply to:  2 comment:3 by alx, 2 years ago

Replying to san:

Однако в таком случае невозможно будет использовать множественные запросы, что, как я понял, подразумевается в #870.
Хотел бы услышать ещё мнения.

Во-первых, предложение #870 по-моему никак не связано с мультизапросами. Предложение #870 решается (в общем случае) четырьмя запросами:

START TRANSACTION;
UPDATE `peoducts` SET ....;
INSERT INTO `history` .....;
COMMIT;

и ничто не мешает выполнять их отдельными вызовами API.

Во-вторых я нигде не видел утверждения о том, что prepared statements не могут использовать мультизапросы (хотя не встречал и утверждений обратного), типа таких:

$mysqli->prepare("UPDATE peoducts SET a=?, b=?; INSERT INTO history(a, b) VALUES(?, ?);")->bind_param("isis", 123, "abc", 456, "def");

comment:4 by san, 2 years ago

Во-первых, предложение #870 по-моему никак не связано с мультизапросами. Предложение #870 решается (в общем случае) четырьмя запросами:

О, спасибо за уточнение, на слово транзакция я не обратил внимание.

утверждения о том, что prepared statements не могут использовать мультизапросы

Вот тут:

Использование множества выражений в подготавливаемом запросе не поддерживается.

https://www.php.net/manual/ru/mysqli.quickstart.multiple-statement.php

И, я так понимаю, ничего не помешает внутри транзакции использовать подготовленные запросы и мои опасения, что это помешает #870, не оправдаются.
В таком случае предлагаю использовать в интерфейсе базы только подготовленные запросы, для обеспечения гарантированной защиты от инъекций.

Last edited 2 years ago by san (previous) (diff)

comment:5 by Denis_N, 2 years ago

Сделано и закончено в r99/base

comment:6 by Denis_N, 2 years ago

Resolution: готово
Status: assignedclosed

comment:7 by san, 2 years ago

Resolution: готово
Status: closedreopened

Денис вспомнил, что на главной странице подготавливаемые запросы ещё не реализованы.

comment:8 by san, 23 months ago

milestone: 1 очередь

Milestone deleted

comment:9 by Denis_N, 3 months ago

In 365/base:

Улучшение: Главная страница переведена на подготавливаемые запросы в тех местах, где есть возможность изменить данные подставляемые в текст запроса
See #879

Last edited 3 months ago by Denis_N (previous) (diff)

comment:10 by Denis_N, 3 months ago

In 367/base:

Полностью перевел главную на подготавливаемые запросы. Также для вставляемой строки для сортировки (order by) сделал отдельную функцию checkStr() для проверки отсутствия в ней sql-иньекции

See #879

in reply to:  10 ; comment:11 by alx, 3 months ago

Replying to Denis_N:

Также для вставляемой строки для сортировки (order by) сделал отдельную функцию checkStr()

А это так и задумано, что строка

    $str2 = substr($checkStr, $substrParam1, $substrParam2);

выполняется всегда, а переменные $substrParam1 и $substrParam2 определены только при некоторых услових?

И еще, простите за любопытство, но раз уж все равно пишу комментарий, спрошу - а зачем в запросе "distinct"? Разве может быть несколько столбцов с одним и тем же именем?

comment:12 by Denis_N, 3 months ago

In 368/base:

Исправлен баг: substr выполняется всегда, а переменные $substrParam1 и $substrParam2 определены только при некоторых условиях
See #879

in reply to:  11 comment:13 by Denis_N, 3 months ago

Привет, Алесей =)
Спасибо, согласен, не правильно сделал, что "переменные $substrParam1 и $substrParam2 определены только при некоторых условиях". Исправил это в https://trac.adc-line.ru/mc-04/changeset/368/base

А про distinct в запросе - при выводе (без distinct) дублируются столбцы UID, comment. Они есть и в products, и в history

Last edited 2 months ago by Denis_N (previous) (diff)
Note: See TracTickets for help on using tickets.