Как мы нашли критичную уязвимость 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, до и после деплоя.
Время выполнения запроса:



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

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



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

В заключение

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

Артем Асташкин, Руководитель группы разработки