How we found the critical vulnerability of AspNetCore.Mvc and switched to our own serialization
Hi, Habr!
In this article we want to share our experience in optimizing performance and exploring the features of AspNetCore.Mvc.
A few years ago, at one of our busy services, we noticed a significant consumption of CPU resources. It looked strange, since the task of the service was to actually take the message and put it into a queue, having previously performed some operations on it, such as validation, data addition, etc.
As a result of profiling, we found that most of the CPU time is “eaten up” by deserialization. We threw out the standard serializer and wrote ours on Jil, as a result of which the consumption of resources decreased significantly. Everything worked as it should, and we managed to forget about it.
We are constantly improving our service in all directions, including security, performance and fault tolerance, therefore, the “security guards” conduct various kinds of tests of our services. And some time ago an alert about an error in the log “arrives” to us - somehow we missed the invalid message further in turn.
With a detailed analysis, everything looked rather strange. There is a request-model (here I will give a simplified example of code):
There is a controller with action Post, for example:
Everything seems logical. If the request comes with a Body view
The API will return BadRequest, and there are tests for it.
However, in the log we see the opposite. We took a message from the logs, sent it to the API and got the status OK ... and ... a new error in the log. Not believing our eyes, we took it easy and made sure that yes, indeed ModelState.IsValid == true. At the same time, we paid attention to the unusually long request execution time - about 500ms, while the usual response time rarely exceeds 50ms and this is in production, which serves thousands of requests per second!
The difference between this query and our tests was only in the fact that the query contained 600+ blank lines ...
There will be a lot of code bulls next.
Began to understand what is wrong here. To eliminate the error, they wrote a clean application (whence I gave an example), with which they reproduced the situation described. In total, we spent a couple of man-days on research, tests, mental debagging the AspNetCore.Mvc code and it turned out that the problem is in JsonInputFormatter .
JsonInputFormatter uses JsonSerializer, which, when receiving a stream for deserialization and type, tries to serialize each property, if it is an array - each element of this array. At the same time, if an error occurs during serialization, JsonInputFormatter saves every error and the path to it, marks it as processed so that the deserialization can proceed further.
Here is the code for the JsonInputFormatter error handler (it is available on Github at the link above):
Marking an error as processed is done at the very end of the handler.
Thus, the feature of outputting all deserialization errors and paths to them, in which fields / elements they were, is implemented, well ... almost all ...
The fact is that the JsonSerializer has a 200 error limit, after which it falls, while all the code falls and ModelState becomes ... valid! ... errors are lost.
Without hesitation, we implemented our Json formatter for Asp.Net Core using Jil Deserializer. Since the number of errors in body is absolutely not important for us, and only the fact of their presence is important (and generally it’s hard to imagine a situation when it would be really useful), the serializer code turned out to be quite simple.
I will give the main code of the custom JilJsonInputFormatter:
Attention! Jil is case sensitive, meaning Body
and
not the same thing. In the first case, if camelCase is used in the ItemIds property after deserialization will be null.
But since this is our API, we use and control it, for us it is not critical. The problem may arise if it is a public API and someone already calls it, passing the name of the parameters not in camelCase.
The result even exceeded our expectations, the API was expected to return the BadRequest request to the desired one and do it very quickly. Below are some screenshots of some of our graphs, which show changes in response time and CPU, before and after deployment. Lead
Time:
Around 16:00 was deployed. Before deployment, the p99 runtime was 30-57ms, after deploying it was 9-15ms. (You can ignore repeated peaks at 18:00 - this was already a different deployment).
This is how the CPU schedule changed:
On this occasion, we started an issue in Github, at the time of this writing it was marked as a bug with milestone 3.0.0-preview3.
While the problem is not solved, we believe that it is better to abandon the use of standard deserialization, especially if you have a public API. Knowing this problem, an attacker can easily put your service, throwing a bunch of such invalid requests into it, because the larger the error array, the larger the Body, the longer it takes to process in JsonInputFormatter.
Artem Astashkin, Development Team Leader
In this article we want to share our experience in optimizing performance and exploring the features of AspNetCore.Mvc.
Prehistory
A few years ago, at one of our busy services, we noticed a significant consumption of CPU resources. It looked strange, since the task of the service was to actually take the message and put it into a queue, having previously performed some operations on it, such as validation, data addition, etc.
As a result of profiling, we found that most of the CPU time is “eaten up” by deserialization. We threw out the standard serializer and wrote ours on Jil, as a result of which the consumption of resources decreased significantly. Everything worked as it should, and we managed to forget about it.
Problem
We are constantly improving our service in all directions, including security, performance and fault tolerance, therefore, the “security guards” conduct various kinds of tests of our services. And some time ago an alert about an error in the log “arrives” to us - somehow we missed the invalid message further in turn.
With a detailed analysis, everything looked rather strange. There is a request-model (here I will give a simplified example of code):
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; }
}
There is a controller with action Post, for example:
[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();
}
}Everything seems logical. If the request comes with a Body view
{"itemIds":["","","" … ] } The API will return BadRequest, and there are tests for it.
However, in the log we see the opposite. We took a message from the logs, sent it to the API and got the status OK ... and ... a new error in the log. Not believing our eyes, we took it easy and made sure that yes, indeed ModelState.IsValid == true. At the same time, we paid attention to the unusually long request execution time - about 500ms, while the usual response time rarely exceeds 50ms and this is in production, which serves thousands of requests per second!
The difference between this query and our tests was only in the fact that the query contained 600+ blank lines ...
There will be a lot of code bulls next.
Cause
Began to understand what is wrong here. To eliminate the error, they wrote a clean application (whence I gave an example), with which they reproduced the situation described. In total, we spent a couple of man-days on research, tests, mental debagging the AspNetCore.Mvc code and it turned out that the problem is in JsonInputFormatter .
JsonInputFormatter uses JsonSerializer, which, when receiving a stream for deserialization and type, tries to serialize each property, if it is an array - each element of this array. At the same time, if an error occurs during serialization, JsonInputFormatter saves every error and the path to it, marks it as processed so that the deserialization can proceed further.
Here is the code for the JsonInputFormatter error handler (it is available on Github at the link above):
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;
}
Marking an error as processed is done at the very end of the handler.
eventArgs.ErrorContext.Handled = true; Thus, the feature of outputting all deserialization errors and paths to them, in which fields / elements they were, is implemented, well ... almost all ...
The fact is that the JsonSerializer has a 200 error limit, after which it falls, while all the code falls and ModelState becomes ... valid! ... errors are lost.
Decision
Without hesitation, we implemented our Json formatter for Asp.Net Core using Jil Deserializer. Since the number of errors in body is absolutely not important for us, and only the fact of their presence is important (and generally it’s hard to imagine a situation when it would be really useful), the serializer code turned out to be quite simple.
I will give the main code of the custom 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();
}Attention! Jil is case sensitive, meaning Body
{"ItemIds":["","","" … ] }and
{"itemIds":["","","" … ] } not the same thing. In the first case, if camelCase is used in the ItemIds property after deserialization will be null.
But since this is our API, we use and control it, for us it is not critical. The problem may arise if it is a public API and someone already calls it, passing the name of the parameters not in camelCase.
Result
The result even exceeded our expectations, the API was expected to return the BadRequest request to the desired one and do it very quickly. Below are some screenshots of some of our graphs, which show changes in response time and CPU, before and after deployment. Lead
Time:
Around 16:00 was deployed. Before deployment, the p99 runtime was 30-57ms, after deploying it was 9-15ms. (You can ignore repeated peaks at 18:00 - this was already a different deployment).
This is how the CPU schedule changed:
On this occasion, we started an issue in Github, at the time of this writing it was marked as a bug with milestone 3.0.0-preview3.
Finally
While the problem is not solved, we believe that it is better to abandon the use of standard deserialization, especially if you have a public API. Knowing this problem, an attacker can easily put your service, throwing a bunch of such invalid requests into it, because the larger the error array, the larger the Body, the longer it takes to process in JsonInputFormatter.
Artem Astashkin, Development Team Leader