Привет, Друзья!
Хотел написать короткий пост по мотивам одного казалось бы простого ПР-а, который мы недавно получили в рамках Axelix: Open Core продукта для решения основных известных болей при разработке Spring Boot приложений (кстати, give us a star!).
В общем, контрибьютор пришёл к нам и исправил с виду совершенно безобидную проблему, которую мы упустили. Но на деле, эта проблема со временем могла привести к другим багам, которые дебажить было бы крайне тяжело. И я посчитал, что это стоит небольшой статьи, потому что сам помню проблемы в Spring Data, которые возникали из-за подобных просчётов.
Я не буду погружать вас в детали того, что мы делали, просто покажу на абстрактном примере.
Давайте с места в карьер. Посмотрите на этот код:
public class SeeminglyGoodCache { private final Map<String, Object> internalCache = new HashMap<>(); public void put(String key, Object value) { internalCache.put(key, value); } public Object get(String key) { return internalCache.get(key); } public Map<String, Object> getCache() { return this.internalCache; }}
Для упрощения ситуации, предположим, что мы выносим concurrency из уравнения — предположим однопоточное приложение. Пример намеренно сделан максимально тупым и наглядным. Видите ли вы в коде выше какие-то проблемы?
На деле проблем тут несколько, но есть одна, про которую я и хочу поговорить. У неё нет чётко устоявшегося названия, но заключается она в том, что метод getCache() возвращает ссылку на внутреннее изменяемое состояние объекта. На деле схожая ситуация и с методом get(), но getCache() сейчас гораздо важнее.
Почему Это Проблема?
Потому что текущий код позволяет взять и мутировать состояние кеша извне. Уже это само по себе влечет за собой 2 неприятности:
1. Сломанная Инкапсуляция
Когда человек начинает изучать ООП языки программирования, то инкапсуляция это одна из первых вещей, которую он слышит. Это реально важная штука, потому что несоблюдение инкапсуляции логики/состояния/complexity приводит к плохим последствиям.
У нас есть чёткое понимание, что есть слой работы с БД, и слой API никогда не будет ходить к БД в обход этого слоя. И за счёт этого на слое работы с БД мы можем накручивать различного рода аудит события (например, hibernate-envers). Мы также можем накручивать multi-tenancy (для SaaS архитектуры например) и т.д. И мы можем быть уверены, что любой доступ к бд будет аудирован. Почему? Потому что он всегда должен происходить через слой работы с БД.
Если наш слой API решает, что ему нужно бы сходить в БД самому, например, не через hibernate а через JdbcTemplate (ну так, быстренько, буквально 1 запрос выполнить), то он оказывается в неприятной ситуации — потенциально сломается как multi-tenancy, так и аудит с envers. Но мы же не лыком шитые, мы про это знаем.
Можем ли мы наш 1 запрос как-то доработать, чтобы он учитывал и multi-tenancy, и аудит? Да, но по сути это дублирование.А дублирование логики это просто прямой путь к неподдерживаемой в будущем системе и к бесконечным багам, т.к. мутирование структуры извне может легко сломать integrity и инварианты, которые SeeminglyGoodCache держал у себя внутри и рассчитывал на них, но не знал, что кто-то может прийти извне и всё поменять.
Применимо к нашему примеру: если кто-то и будет иметь возможность модифицировать internalCache, то это должен быть сам SeeminglyGoodCache. И мы на самом деле этого же и хотели, у нас для этого даже есть accessor методы — get и put. Однако проблема в том, что наш код допускает мутацию извне. И если он её допускает, значит, рано или поздно это произойдёт.
Почему для Axelix это особенно актуально?
Простому веб приложению в данном случае попроще, потому что традиционное веб приложение пакуется в JAR и не попадает ни к кому в classpath/modulepath, и соответственно все использования условного getCache можно посмотреть в IDE. Если же вы разрабатываете платформенное решение у себя в бигтехе, то ситуация принципиально иная.
Будучи библиотекой, и имея getCache как public API, то природу использования данного API вы заранее не знаете. Вы даже не можете этого предположить. Дизайнеры крупных библиотек должны помнить про Hyrum’s Law:
With a sufficient number of users of an API, it does not matter what you promise in the contract: all observable behaviors of your system will be depended on by somebody.
Несмотря на то, что Axelix имеет Master как компонент (а это отдельный Bootable Jar запакованый в образ), у Axelix также есть стартеры для Spring Boot, которые собственно и приносят всю эту магию по возможностям мониторинга ваших транзакций, запросов в рамках транзакций и т.д.
И для библиотек исправление подобной проблемы станет де-факто не backward compatible изменением, и его придётся заранее планировать на след. major release и это в лучшем случае.
2. Security Breach
Но что ещё страшнее, то такие вещи иногда могут становиться частью цепочки, которая в итоге приводит к HIGH/CRITICAL уязвимостям.
Например, в экосистеме Java в своё время были очень неприятные истории вокруг BeanUtils и механизмов data binding в целом. Проблема там была не в одном конкретном getXXX методе как таковом, а в том, что фреймворк позволял по внешнему input ходить по графу объектов через Java Bean property accessors. А это уже очень опасная история, потому что наружу внезапно начинают торчать изменяемые объекты, которые изначально вообще не планировалось давать пользователю в руки.
Если упростить, то атакующий мог передать специально сконструированный property path и начать проваливаться всё глубже во внутренности объекта. А там уже недалеко и до чувствительных штук вроде ClassLoader (который, например, можно получить по классу и который можно мутировать), конфигурации контейнера или других объектов, мутирование которых меняет поведение приложения совсем не так, как вы задумывали.
То есть моя мысль тут не в том, что любой getXXX, возвращающий mutable состояние, автоматически означает CVE. Конечно, нет. Но если вы систематически допускаете утечку mutable internal state наружу, то вы сильно расширяете attack surface своей системы. А дальше достаточно, чтобы где-то рядом оказался небезопасный binding, reflection-based mapper или слишком “умный” accessor framework, и ситуация может стать очень неприятной.
P.S: Ровно поэтому истории вроде Spring4Shell так болезненно и воспринимаются индустрией. Там проблема тоже была не в “магии Spring” как таковой, а в том, что удобные механизмы интроспекции и биндинга в какой-то момент начали давать доступ туда, куда внешний input доходить был не должен вообще в принципе.
Как Такое Решать
Решения основных тут 2, и я их приведу на примере Spring Framework.
Вариант 1. Возвращать Копию
Простой вариант — возвращать неизменямую копию. Например, делать Map.copyOf, List.copyOf или т.п. Так, например делает стандартный Environment в Spring Framework:
@Overridepublic String[] getActiveProfiles() {return StringUtils.toStringArray(doGetActiveProfiles()); // вот тут делается копия под капотом}
И с environment такой вариант прокатывает, поскольку активных профилей в рамках Spring Boot приложения, как правило, довольно мало. Но что делать, если, например, хранимый объем данных довольно большой, где могут быть десятки, сотни, а может и больше записей? Вот тут есть другой подход.
Вариант 2. Возвращать Read-Only обертку
Иногда создавать полную копию коллекции будет ну слишком непозволительно дорого. И в таких ситуациях можно сделать, например, как мы сделали в Axelix:
@Override public Map<MethodClassKey, SlidingWindow<TransactionRecord>> getAllStats() { return Collections.unmodifiableMap(statsMap); }
В массе своей статические методы unmodifiableXXX в рамках utility класса Collections не создают копию, они создают read-only wrapper поверх оригинальной коллекции. Кроме дополнительного объекта типа Map ничего по сути не аллоцируется, а мутировать наш внутренний state мы не даём.
Но! У этого подхода есть минус. Он в том, что если пользователь захочет сделать Map.put, то его ждёт exception, но это уже общая проблема дизайна коллекций в Java. Это можно легко отловить тестами, и этот трейдофф мы готовы принять.
К тому же, обратите внимание, что несомтря на то, что и в первом и во втором решении сама коллекция условно “безопасна” для её возврата во внешний мир, есть ещё один важный нюанс: сами элементы коллекции в иделе должны также быть immutable. Удовлетворить уже такое требование бывает не всегда на 100% возможно, но просто держите это в уме.
Выводы
Внимательно следите за тем, чтобы из своих абстракций не передать наружу возможность мутирования внутреннего состояния объекта (Мейнтейнеры библиотек — вы особенно). Это как принесёт Вам гору багов и сломанные инварианты в продакшене, так и потенциально открывает окно к новым уязвимостям.
Будьте аккуратны. Всем успехов!
ссылка на оригинал статьи https://habr.com/ru/articles/1046620/