Как мы нашли критичную уязвимость AspNetCore.Mvc и перешли на собственную сериализацию

    Привет, Хабр!

    В этой статье мы хотим поделиться нашим опытом в оптимизации производительности и исследовании особенностей AspNetCore.Mvc.



    Предыстория


    Несколько лет назад на одном из наших нагруженных сервисов мы заметили существенное потребление ресурсов CPU. Это выглядело странно, так как задачей сервиса было фактически взять сообщение и переложить его в очередь, предварительно произведя над ним некоторые операции, такие как валидация, дополнение данными, и т.п.

    В результате профилирования мы обнаружили, что большую часть процессорного времени “съедает” десериализация. Мы выкинули стандартный сериализатор и написали свой на Jil, в результате чего потребление ресурсов снизилось в разы. Все работало как нужно и мы успели об этом позабыть.

    Проблема


    Мы постоянно совершенствуем наш сервис во всех направлениях, включая безопасность, производительность и отказоустойчивость, поэтому “безопасники” проводят разного рода тесты наших сервисов. И некоторое время назад к нам “прилетает” алерт об ошибке в логе — каким-то образом мы пропустили невалидное сообщение дальше по очереди.

    При детальном анализе все выглядело достаточно странно. Есть request-model (тут я приведу упрощенный пример кода):

    public class RequestModel
    {
        public string Key { get; set; }
    
        FromBody]
        Required]
        public PostRequestModelBody Body { get; set; }
    }
    
    public class PostRequestModelBody
    {
        [Required]
        [MinLength(1)]
        public IEnumerable<long> ItemIds { get; set; }
    }
    

    Есть контроллер с action Post, например:

    [Route("api/[controller]")]
    public class HomeController : Controller
    {
        [HttpPost]
        public async Task<ActionResult> Post(RequestModel request)
        {
            if (this.ModelState.IsValid)
            {
                return this.Ok();
            }
    
            return this.BadRequest();
        }
    }

    Все кажется логичным. Если придет запрос с Body вида

    {"itemIds":["","","" … ] } 

    API вернет BadRequest, и на это есть тесты.

    Тем не менее, в логе видим обратное. Мы взяли сообщение из логов, отправили в API и получили статус OK… и… новую ошибку в логе. Не веря своим глазам мы продебажились и убедились, что да, действительно ModelState.IsValid == true. При этом обратили внимание на необычно долгое время выполнения запроса — порядка 500ms, в то время как обычное время ответа редко превышает 50ms и это в продакшене, который обслуживает тысячи запросов в секунду!

    Отличие этого запроса от наших тестов было лишь в том, что запрос содержал 600+ пустых строк…

    Дальше будет много кода-букаф.

    Причина


    Стали разбираться, что же тут не так. Чтобы исключить ошибку, написали чистое приложение (откуда я и привел пример), с помощью которого воспроизвели описанную ситуацию. В общей сложности мы потратили пару человеко-дней на исследование, тесты, ментальный дебаг кода AspNetCore.Mvc и оказалось, что проблема в JsonInputFormatter.

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

    Приведу ниже код обработчика ошибок JsonInputFormatter (он доступен на Github по ссылке выше):

    
    void ErrorHandler(object sender, Newtonsoft.Json.Serialization.ErrorEventArgs eventArgs)
    {
        successful = false;
        // When ErrorContext.Path does not include ErrorContext.Member, add Member to form full path.
        var path = eventArgs.ErrorContext.Path;
        var member = eventArgs.ErrorContext.Member?.ToString();
        var addMember = !string.IsNullOrEmpty(member);
        if (addMember)
        {
            // Path.Member case (path.Length < member.Length) needs no further checks.
            if (path.Length == member.Length)
            {
                // Add Member in Path.Memb case but not for Path.Path.
                addMember = !string.Equals(path, member, StringComparison.Ordinal);
            }
            else if (path.Length > member.Length)
            {
                // Finally, check whether Path already ends with Member.
                if (member[0] == '[')
                {
                    addMember = !path.EndsWith(member, StringComparison.Ordinal);
                }
                else
                {
                    addMember = !path.EndsWith("." + member, StringComparison.Ordinal);
                }
            }
        }
    
    
        if (addMember)
        {
            path = ModelNames.CreatePropertyModelName(path, member);
        }
    
    
        // Handle path combinations such as ""+"Property", "Parent"+"Property", or "Parent"+"[12]".
        var key = ModelNames.CreatePropertyModelName(context.ModelName, path);
    
    
        exception = eventArgs.ErrorContext.Error;
    
        var metadata = GetPathMetadata(context.Metadata, path);
        var modelStateException = WrapExceptionForModelState(exception);
        context.ModelState.TryAddModelError(key, modelStateException, metadata);
    
        _logger.JsonInputException(exception);
    
    
        // Error must always be marked as handled
        // Failure to do so can cause the exception to be rethrown at every recursive level and
        // overflow the stack for x64 CLR processes
        eventArgs.ErrorContext.Handled = true;
    }
    

    Пометка ошибки, как обработанной производится в самом конце обработчика

    eventArgs.ErrorContext.Handled = true; 


    Таким образом реализована фича вывода всех ошибок десериализации и путей к ним, на каких полях/элементах они были, ну… почти всех…

    Дело в том, что у JsonSerializer есть ограничение на 200 ошибок, после которого он падает, при этом падает весь код и ModelState становится… валидной!… теряются и ошибки.

    Решение


    Не долго думая, мы реализовали свой форматтер Json для Asp.Net Core с использованием Jil Deserializer. Поскольку для нас абсолютно не важно количество ошибок в body, а важен лишь факт их наличия (да и в целом сложно представить себе ситуацию, когда это было бы действительно полезно), то код сериализатора получился достаточно простым.

    Приведу основной код кастомного JilJsonInputFormatter:

    using (var reader = context.ReaderFactory(request.Body, encoding))
    {
        try
        {
            var result = JSON.Deserialize(
                reader: reader,
                type: context.ModelType,
                options: this.jilOptions);
    
            if (result == null && !context.TreatEmptyInputAsDefaultValue)
            {
                return await InputFormatterResult.NoValueAsync();
            }
            else
            {
                return await InputFormatterResult.SuccessAsync(result);
            }
        }
        catch
        {
            // какая-то обработка ошибок
        }
    
        return await InputFormatterResult.FailureAsync();
    }

    Внимание! Jil чувствителен к регистру, это значит, что содержимое Body

    {"ItemIds":["","","" … ] }

    и

    {"itemIds":["","","" … ] } 

    не одно и тоже. В первом случае, если используется camelCase в свойство ItemIds после десериализации будет null.

    Но так как это наш API, мы используем и контролируем его, для нас это не критично. Проблема может возникнуть, если это публичный API и кто-то уже вызывает его, передавая имя параметров не в camelCase.

    Результат


    Результат даже превзошел наши ожидания, API ожидаемо стал возвращать на искомый запрос BadRequest и делать это очень быстро. Ниже приведу скрины некоторых наших графиков, на которых хорошо видны изменения времени ответа и CPU, до и после деплоя.
    Время выполнения запроса:

    image

    Примерно в 16:00 был деплой. До деплоя время выполнения p99 составляло 30-57ms, после деплоя стало 9-15ms. (На повторные пики 18:00 можно не обращать внимания — это был уже другой деплой)

    Вот так изменился график CPU:

    image

    По этому поводу мы завели issue в Github, на момент написания статьи она была помечена как bug с milestone 3.0.0-preview3.

    В заключение


    Пока проблема не решена, мы считаем что лучше отказаться от использования стандартной десериализации, особенно, если у вас публичный API. Зная эту проблему, злоумышленник может легко положить ваш сервис, закинув в него кучу подобных невалидных запросов, ведь чем больше ошибочный массив, чем больше Body, тем дольше происходит обработка в JsonInputFormatter.

    Артем Асташкин, Руководитель группы разработки
    • +19
    • 4,6k
    • 4
    Retail Rocket
    97,00
    Платформа для персонализации интернет-магазинов
    Поделиться публикацией

    Комментарии 4

      +2
      Я правильно понимаю, что если заслать сейчас в любой бэк на неткоре модель с более чем 200 ошибками, то валидация пройдет?
      А почему атрибут FromBody стоит в классе, а не на экшоне контроллера?
      Еще у вас там JSON.SerializeDynamic, а если JsonConvert.SerializeObject?
        0
        Я правильно понимаю, что если заслать сейчас в любой бэк на неткоре модель с более чем 200 ошибками, то валидация пройдет?

        Если модель, ошибки и валидация аналогичны — да, в статье есть ссылка на issue в GitHub, к которой прикреплен проект с тестом воспроизводящим проблему

        А почему атрибут FromBody стоит в классе, а не на экшоне контроллера?

        это аттрибут на поле из RequestModel и только оно идет из body. Если указать в экшене, то будет ожидаться вся модель из body

        Еще у вас там JSON.SerializeDynamic, а если JsonConvert.SerializeObject?

        в данном случае не имеет значения, просто используем Jil.JSON
        +1
        Тест в вашем приложении-примере проходит на 2.2.0, судя по всему, исправлено.
          0
          Хорошая новость! Видимо, в этой версии добавили хотфикс, но для ранних версий все еще актуально. Кроме того, проблема с производительностью судя по всему в 2.2 еще не решена

        Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

        Самое читаемое