Привет, Друзья!

Хотел написать короткий пост по мотивам одного казалось бы простого ПР-а, который мы недавно получили в рамках 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:

	@Override
	public 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% возможно, но просто держите это в уме.

Выводы

Внимательно следите за тем, чтобы из своих абстракций не передать наружу возможность мутирования внутреннего состояния объекта (Мейнтейнеры библиотек - вы особенно). Это как принесёт Вам гору багов и сломанные инварианты в продакшене, так и потенциально открывает окно к новым уязвимостям.

Будьте аккуратны. Всем успехов!