Довольно странно, что один из классов угроз оказался знаком столь малому проценту от числа проголосовавших. Предлагаю решить небольшую задачку по устранению всех узявимостей в существующем коде, а потом расскажу подробнее, что это за класс и почему с ним наверняка сталкивались почти все разработчики, хотя видимо не знали, что оно так называется. Заодно проверим, действительно ли все знают, как появляются SQL-инъекции и как с ними бороться
Легенда: у нас есть движок форума, одна из страниц которого позволяет посмотреть список всех участников, их статистику (ну там, количество сообщений, рейтинг и т.п.) и фильтровать список по значениям этих полей (например, отобразить всех участников с количеством сообщений больше 100, но меньше 200). Вся информация о пользователе (логин, пароль, никнейм, почта, всевозможные данные его статистики и т.п.) хранятся в одной таблице Users. Ну или вьюхе — не суть. Разработчики реализовали это тривиальным образом и, кроме того, оставили за собой возможность расширять критерии SQL-запроса GET-параметром, но только в том случае, если движок собран в DEBUG-конфигурации:
var fieldName = Request["field"] ?? "Id";
var minValue = Request["min"] ?? Int32.MinValue;
var maxValue = Request["max"] ?? Int32.MaxValue;
var debugStr = Request["debug"] ?? string.Empty;
#if DEBUG
var debugTemplate = "{0}";
#else
var debugTemplate = string.Empty;
#endif
var queryTemplate = string.Format(
"SELECT Id, Nickname, Rating, MessageCount, TopicCount FROM Users WHERE {0} >= @minValue AND {0} <= @maxValue {1} ORDER BY {0}",
fieldName.Replace("'", string.Empty).Replace(" ", string.Empty).Replace("\\", string.Empty).Replace(",", string.Empty).Replace("(", string.Empty).Replace(")", string.Empty),
debugTemplate
);
var selectCommand = string.Format(queryTemplate, debugStr);
var cmd = new SqlCommand(selectCommand, dataConnection);
cmd.Parameters.Add(new SqlParameter("@minValue", minValue));
cmd.Parameters.Add(new SqlParameter("@maxValue", maxValue));
// Ну и так далее. Далее осуществляется SQL-запрос, и отобранные поля уходят в виде HTML-таблицы в ответ на HTTP-запрос.
ну с пьяного глаза первое(и последнее, честно говоря), что бросается в глаза — это безумно сложное выражение для escape'инга fieldName.
Если заменить его на что нить попроще, типа:
if (!fieldName.All(Char.IsLetter))
throw new Exception();
то лично я был бы гораздо спокойнее. Ну и естественно — нуно считать, что дебаг версия — никому левому не доступна. Иначе возможно всё что угодно.
J>if (!fieldName.All(Char.IsLetter))
J> throw new Exception();
J>
J>то лично я был бы гораздо спокойнее.
Хм, ок. А сколько и каких уязвимостей закроет это изменение?
J>Ну и естественно — нуно считать, что дебаг версия — никому левому не доступна.
Это кстати, не всегда уязвимость, но всегда замечание. Безопасность на уровне реализации не должна зависить от уровня развертывания.
J>Иначе возможно всё что угодно.
Здравствуйте, kochetkov.vladimir, Вы писали:
KV>Здравствуйте, Jack128, Вы писали:
J>>
J>>if (!fieldName.All(Char.IsLetter))
J>> throw new Exception();
J>>
J>>то лично я был бы гораздо спокойнее.
KV>Хм, ок. А сколько и каких уязвимостей закроет это изменение?
Вот честно говоря — хз. Я о безопасности раньше никогда не задумывался(не было необходимости), а вот сейчас кинули на web проэкт, а так как знаний/опыта ноль, приходится больше на интуиции работать. Ну и используя всякие LINQ'и, разоры, в надежде, что они помогут от самых распространённых касяков защитится.
KV>Да тут и в релизе все, что угодно возможно
Возможно. Опять же чисто интуитивно — мне не нравится то что MinValue/MaxValue имеют тип object, хотя судя по контексту должны иметь тип int.
вобщем если заменить исходный код на такой:
var fieldName = Request["field"] ?? "Id";
if (!fieldName.All(Char.IsLetter))
throw new Exception();
var minValue = Request["min"] == null ? Int32.MinValue : int.Parse(Request["min"]);
var maxValue = Request["max"] == null ? Int32.MaxValue : int.Parse(Request["max"]);;
#if DEBUG
var debugStr = Request["debug"] ?? string.Empty; // в string.Format добавлять плейсхолдер для другого string.Format'a - жто шиза!!! Проще нуно быть.#else
var debugStr = "";
#endif
var selectCommand = string.Format(
"SELECT Id, Nickname, Rating, MessageCount, TopicCount FROM Users WHERE {0} >= @minValue AND {0} <= @maxValue {1} ORDER BY {0}",
fieldName, debugStr
);
var cmd = new SqlCommand(selectCommand, dataConnection);
cmd.Parameters.Add(new SqlParameter("@minValue", minValue));
cmd.Parameters.Add(new SqlParameter("@maxValue", maxValue));
то я больше ничего полезного для взлома не вижу. Кроме случая когда Users очень большая таблица, а фильт идет по полю без индекса. Тогда можно легко заддосить приложение.
Здравствуйте, Jack128, Вы писали:
J>Вот честно говоря — хз. Я о безопасности раньше никогда не задумывался(не было необходимости), а вот сейчас кинули на web проэкт, а так как знаний/опыта ноль, приходится больше на интуиции работать. Ну и используя всякие LINQ'и, разоры, в надежде, что они помогут от самых распространённых касяков защитится.
Приходи на phdays (phdays.ru). Там будет несколько докладов и (вероятно) воркшопов/мастер-классов, которые могут быть полезны и для разработчиков. А доклад, который готовлю я, определенно будет полезен, причем именно тебе, судя по тому что ты написал выше
KV>>Да тут и в релизе все, что угодно возможно J>Возможно. Опять же чисто интуитивно — мне не нравится то что MinValue/MaxValue имеют тип object, хотя судя по контексту должны иметь тип int.
Не, это мой косяк, к заданию не относится
J>вобщем если заменить исходный код на такой:
Я пока код комментировать не буду, вдруг кто-то еще все же решится попробовать. Подождем чуть. Скажу лишь, что он действительно закрывает большую часть уязвимостей, но не все
J>то я больше ничего полезного для взлома не вижу. Кроме случая когда Users очень большая таблица, а фильт идет по полю без индекса. Тогда можно легко заддосить приложение.
Да, но это ближе к этапу развертывания, чем к реализации. Будем считать, что здесь речь только о ней.
Здравствуйте, Jack128, Вы писали:
KV>>Хм, ок. А сколько и каких уязвимостей закроет это изменение? J>Вот честно говоря — хз.
Кстати, если кто-нибудь хочет сказать, какие именно уязвимости это закрыло — не стесняйтесь. Дофига ж народу отметилось, что SQL-инъекции знают. Сможете их тут показать?
Выше написал — это мой косяк.
M8R>Собственно, вроде всё. M8R>Владимир, давайте ещё)
Дам, как только найдем все уязвимости Дело в том, что этот пример не вполне про SQL-инъекции (из первого абзаца же очевидно). SQLi тут так... к слову пришлись
M8R>>Ахтунг! А что у нас в queryTemplate и debugStr?) (см. 1 и 2)
KV>Что именно не так?
Кажется я ошибся в столь ранний час) Не в DEBUG'е всё должно быть ок.
Здравствуйте, kochetkov.vladimir, Вы писали:
KV>Здравствуйте, Jack128, Вы писали:
KV>>>Хм, ок. А сколько и каких уязвимостей закроет это изменение? J>>Вот честно говоря — хз.
KV>Кстати, если кто-нибудь хочет сказать, какие именно уязвимости это закрыло — не стесняйтесь. Дофига ж народу отметилось, что SQL-инъекции знают. Сможете их тут показать?
var fieldName = Request["field"] ?? "Id";
if (!fieldName.All(Char.IsLetter))
throw new Exception();
...
var selectCommand = string.Format(
"SELECT Id, Nickname, Rating, MessageCount, TopicCount FROM Users WHERE {0} >= @minValue AND {0} <= @maxValue {1} ORDER BY {0}",
fieldName, debugStr
);
Единственное, что мне тут не нравится, это то, что пользователь может подставить любое имя поля. Можеть быть это поможет узнать какие поля есть в таблице и какого типа. Хз что ещё тут можно сделать.
Здравствуйте, kochetkov.vladimir, Вы писали:
KV>Здравствуйте, Jack128, Вы писали:
J>>Вот честно говоря — хз. Я о безопасности раньше никогда не задумывался(не было необходимости), а вот сейчас кинули на web проэкт, а так как знаний/опыта ноль, приходится больше на интуиции работать. Ну и используя всякие LINQ'и, разоры, в надежде, что они помогут от самых распространённых касяков защитится.
KV>Приходи на phdays (phdays.ru). Там будет несколько докладов и (вероятно) воркшопов/мастер-классов, которые могут быть полезны и для разработчиков. А доклад, который готовлю я, определенно будет полезен, причем именно тебе, судя по тому что ты написал выше
Хм, можно. Опять же, "Несчастный случай" в качестве бонуса стимулирует -)
Здравствуйте, kochetkov.vladimir, Вы писали:
KV>Вся информация о пользователе (логин, пароль, никнейм, почта, всевозможные данные его статистики и т.п.) хранятся в одной таблице Users. KV>Найдите все уязвимости, допущенные в этом коде
Получается, что с помощью множества запросов можно сначала подобрать имена колонок с нужной информацией (пароль, ...), а потом получать список пользователей с нужными значениями в этих колонках. Вся база как на ладони.
Здравствуйте, MT-Wizard, Вы писали:
MW>Здравствуйте, kochetkov.vladimir, Вы писали:
KV>>Вся информация о пользователе (логин, пароль, никнейм, почта, всевозможные данные его статистики и т.п.) хранятся в одной таблице Users. KV>>Найдите все уязвимости, допущенные в этом коде
MW>Получается, что с помощью множества запросов можно сначала подобрать имена колонок с нужной информацией (пароль, ...), а потом получать список пользователей с нужными значениями в этих колонках. Вся база как на ладони.
Ё-ё-ё моё, какая дырень. Получаем всех пользователей, а потом половинным делением — пароль для любого заданного пользователя..
Здравствуйте, M8R, Вы писали:
M8R>Единственное, что мне тут не нравится, это то, что пользователь может подставить любое имя поля. Можеть быть это поможет узнать какие поля есть в таблице и какого типа. Хз что ещё тут можно сделать.
Еще можно, тем же способом, получить содержимое этих полей.
Здравствуйте, MT-Wizard, Вы писали:
MW>Получается, что с помощью множества запросов можно сначала подобрать имена колонок с нужной информацией (пароль, ...), а потом получать список пользователей с нужными значениями в этих колонках. Вся база как на ладони.
Здравствуйте, Jack128, Вы писали:
MW>>Получается, что с помощью множества запросов можно сначала подобрать имена колонок с нужной информацией (пароль, ...), а потом получать список пользователей с нужными значениями в этих колонках. Вся база как на ладони.
J>Ё-ё-ё моё, какая дырень. Получаем всех пользователей, а потом половинным делением — пароль для любого заданного пользователя..
Да, именно так. Манипулируя значением fieldName, атакующий может получить содержимое всех полей всей таблицы банальным двоичным поиском на особый лад. Причем безотносительно санитизации параметра fieldName в плане SQL-инъекций. Этот класс атак называется Abuse of Functionality. Критерием отнесения к этому классу, является возможность атакующего, реализовать какую-либо угрозу, используя только штатный функционал приложения: получить из побочных каналов информацию, недоступную рядовым пользователям (как в данном случае), затруднить или остановить работу приложения (например, заблокировав все аккаунты пользователей, осуществив максимальное количество попыток ввода неправильного пароля. В этом случае, это также относится и классу DoS), осуществить обход аутентификации, воспользовавшись слабой процедурой восстановления пароля и т.п.
Уязвимостью в данном случае является недостаточная обработка входных данных параметра fieldName. Для ее устранения, необходим контроль этого параметра по белому списку с тем, чтобы в нем могли быть имена только тех полей, которые явным образом отбираются SELECT'ом.
Здравствуйте, M8R, Вы писали:
KV>>Что именно не так? M8R>Кажется я ошибся в столь ранний час) Не в DEBUG'е всё должно быть ок.
Да нет, там не все ок, только не из-за DEBUG. Если в запросе придет fieldName со значением "{0}", то содержимое debugStr попадет в текст запроса безотносительно конфигурации сборки. Это приводит к SQL-инъекции. Уявзимостью, в данном случае, является опять-таки недостаточный контроль fieldName. С устранением AoF будет устранена и эта SQLi. Хотя правильнее, конечно, реализовать эту фичу так, как предложил Jack128 (Re[3]: [Этюд] По мотивам голосования
