Photo by Elisa Ventur
Introduction
An old colleague of mine (Nils Stjernstrid) wrote to me a couple of weeks ago and told me that he wanted me to be aware of a potential problem with the default implementation of ProblemDetails in ASP.NET Core.
We talked for a bit and then we created a minimal repro. I then contacted Microsoft and told them about our findings. Microsoft responded after a couple of days and they said that yes, they could see our point but they didn't deemed it that serious so they won't "fix it".
So the only thing left for me to do is to show what we've found and how to protect yourself against it.
The problem
We have created a new ASP.NET Core 6 application, we have not added anything to it, it's the default application you get from the templates.
Our WeatherForecastController looks like this (we've added a query parameter, Date).
[HttpGet(Name = "GetWeatherForecast")]
public IEnumerable<WeatherForecast> Get([FromQuery] DateTime date)
{
return Enumerable.Range(1, 5).Select(index => new WeatherForecast
{
Date = DateTime.Now.AddDays(index),
TemperatureC = Random.Shared.Next(-20, 55),
Summary = Summaries[Random.Shared.Next(Summaries.Length)]
})
.ToArray();
}
We can then call our endpoint like this:
GET
/WeatherForecast?date=2021-01-23
And get the following response:
[{
"date": "2021-11-08T15:34:19.589784+01:00",
"temperatureC": 1,
"temperatureF": 33,
"summary": "Mild"
}, {
"date": "2021-11-09T15:34:19.5900947+01:00",
"temperatureC": 34,
"temperatureF": 93,
"summary": "Hot"
}, {
"date": "2021-11-10T15:34:19.5900975+01:00",
"temperatureC": 38,
"temperatureF": 100,
"summary": "Hot"
}, {
"date": "2021-11-11T15:34:19.5900979+01:00",
"temperatureC": -3,
"temperatureF": 27,
"summary": "Bracing"
}, {
"date": "2021-11-12T15:34:19.5900982+01:00",
"temperatureC": 12,
"temperatureF": 53,
"summary": "Cool"
}]
So far so good.
Now, imagine that we want the user to be able to enter a date in an input field (I'm really bad at UX, so no fancy datepickers here).
If the user enters something that can't be parsed to a valid DateTime, the API will return a 400 Bad Request together with a response body that contains more information about the error.
GET
/WeatherForecast?date=not-a-date
The response from the API looks like this:
{
"type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
"title": "One or more validation errors occurred.",
"status": 400,
"traceId": "00-dc24c65b22855bbe9fc5dfa186bda61d-09be45928cc0bf37-00",
"errors": {
"date": ["The value 'not-a-date' is not valid."]
}
}
The error message says that not-a-date is not a valid Date, makes sense, right?
To help the user, I will just print out that error on the page. Do you see where this is going? :)
Now, imagine that the user enters something like this instead:
GET
/WeatherForecast?date=2021-11-07<script>alert('you%20have%20been%20hacked')</script>
The response from the API would look like this:
{
"type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
"title": "One or more validation errors occurred.",
"status": 400,
"traceId": "00-5b8328e1ef9f50b712d4e683e06a8c8a-d7cdbc21eb9fbc02-00",
"errors": {
"date": ["The value '2021-10-28<script>alert('you have been hacked')</script>' is not valid."]
}
}
And since we happily print out whatever is in the errors.date array, the browser will run the script, and an alert prompt will be shown.
This is not good, of course. You should never trust user input, of course, you should always validate it and also escape it if you ever want to display it.
Now, remember that I haven't done ANY coding on the backend here, this is the default behaviour of ASP.NET Core.
One could argue if this is something that should be fixed or not. I don't want to point any fingers, I just want people to be aware of this. And as I said before, it's ALWAYS a bad idea to just blindly trust user input but since this is the default behaviour of the framework, many people might not even know about this. And let's be honest, the scenario that I showed above is not that uncommon.
- Make a request towards an API
- If something goes wrong, print out the error response.
A more "safe" approach is, ofc, to not blindly trust the data returned from the API, but then again, where do you draw the line? Should you escape all data coming from the API? Maybe? I don't know, I just want people to be aware of this.
Alternative implementation
It's quite easy to alter the behaviour of the ProblemDetails implementation. We will create our own ProblemDetailsFactory.
The default implementation looks like this:
internal sealed class DefaultProblemDetailsFactory : ProblemDetailsFactory
{
private readonly ApiBehaviorOptions _options;
public DefaultProblemDetailsFactory(IOptions<ApiBehaviorOptions> options)
{
_options = options?.Value ?? throw new ArgumentNullException(nameof(options));
}
public override ProblemDetails CreateProblemDetails(
HttpContext httpContext,
int? statusCode = null,
string? title = null,
string? type = null,
string? detail = null,
string? instance = null)
{
statusCode ??= 500;
var problemDetails = new ProblemDetails
{
Status = statusCode,
Title = title,
Type = type,
Detail = detail,
Instance = instance,
};
ApplyProblemDetailsDefaults(httpContext, problemDetails, statusCode.Value);
return problemDetails;
}
public override ValidationProblemDetails CreateValidationProblemDetails(
HttpContext httpContext,
ModelStateDictionary modelStateDictionary,
int? statusCode = null,
string? title = null,
string? type = null,
string? detail = null,
string? instance = null)
{
if (modelStateDictionary == null)
{
throw new ArgumentNullException(nameof(modelStateDictionary));
}
statusCode ??= 400;
var problemDetails = new ValidationProblemDetails(modelStateDictionary)
{
Status = statusCode,
Type = type,
Detail = detail,
Instance = instance,
};
if (title != null)
{
// For validation problem details, don't overwrite the default title with null.
problemDetails.Title = title;
}
ApplyProblemDetailsDefaults(httpContext, problemDetails, statusCode.Value);
return problemDetails;
}
private void ApplyProblemDetailsDefaults(HttpContext httpContext, ProblemDetails problemDetails, int statusCode)
{
problemDetails.Status ??= statusCode;
if (_options.ClientErrorMapping.TryGetValue(statusCode, out var clientErrorData))
{
problemDetails.Title ??= clientErrorData.Title;
problemDetails.Type ??= clientErrorData.Link;
}
var traceId = Activity.Current?.Id ?? httpContext?.TraceIdentifier;
if (traceId != null)
{
problemDetails.Extensions["traceId"] = traceId;
}
}
}
Our custom implementation will look like this:
HtmlEncodedProblemDetailsFactory
public class HtmlEncodedProblemDetailsFactory : ProblemDetailsFactory
{
private readonly ApiBehaviorOptions _options;
public HtmlEncodedProblemDetailsFactory(IOptions<ApiBehaviorOptions> options)
{
_options = options.Value ?? throw new ArgumentNullException(nameof(options));
}
public override ProblemDetails CreateProblemDetails(HttpContext httpContext, int? statusCode = null, string? title = null,
string? type = null, string? detail = null, string? instance = null)
{
statusCode ??= 500;
var problemDetails = new ProblemDetails
{
Status = statusCode,
Title = HtmlEncoder.Default.Encode(title ?? string.Empty),
Type = HtmlEncoder.Default.Encode(type ?? string.Empty),
Detail = HtmlEncoder.Default.Encode(detail ?? string.Empty),
Instance = HtmlEncoder.Default.Encode(instance ?? string.Empty),
};
ApplyProblemDetailsDefaults(httpContext, problemDetails, statusCode.Value);
return problemDetails;
}
public override ValidationProblemDetails CreateValidationProblemDetails(HttpContext httpContext,
ModelStateDictionary modelStateDictionary, int? statusCode = null, string? title = null, string? type = null,
string? detail = null, string? instance = null)
{
if (modelStateDictionary == null)
{
throw new ArgumentNullException(nameof(modelStateDictionary));
}
statusCode ??= 400;
var htmlEncodedErrors = CreateErrorDictionary(modelStateDictionary);
var problemDetails = new ValidationProblemDetails(htmlEncodedErrors)
{
Status = statusCode,
Type = HtmlEncoder.Default.Encode(type ?? string.Empty),
Detail = HtmlEncoder.Default.Encode(detail ?? string.Empty),
Instance = HtmlEncoder.Default.Encode(instance ?? string.Empty),
};
if (title != null)
{
// For validation problem details, don't overwrite the default title with null.
problemDetails.Title = HtmlEncoder.Default.Encode(title); ;
}
ApplyProblemDetailsDefaults(httpContext, problemDetails, statusCode.Value);
return problemDetails;
}
private void ApplyProblemDetailsDefaults(HttpContext httpContext, ProblemDetails problemDetails, int statusCode)
{
problemDetails.Status ??= statusCode;
if (_options.ClientErrorMapping.TryGetValue(statusCode, out var clientErrorData))
{
problemDetails.Title ??= HtmlEncoder.Default.Encode(clientErrorData.Title ?? string.Empty);
problemDetails.Type ??= HtmlEncoder.Default.Encode(clientErrorData.Link ?? string.Empty);
}
var traceId = Activity.Current?.Id ?? httpContext?.TraceIdentifier;
if (traceId != null)
{
problemDetails.Extensions["traceId"] = traceId;
}
}
private static IDictionary<string, string[]> CreateErrorDictionary(ModelStateDictionary modelState)
{
if (modelState == null)
{
throw new ArgumentNullException(nameof(modelState));
}
var errorDictionary = new Dictionary<string, string[]>(StringComparer.Ordinal);
foreach (var keyModelStatePair in modelState)
{
var key = keyModelStatePair.Key;
var errors = keyModelStatePair.Value.Errors;
if (errors.Count > 0)
{
if (errors.Count == 1)
{
var errorMessage = HtmlEncoder.Default.Encode(errors[0].ErrorMessage);
errorDictionary.Add(key, new[] { errorMessage });
}
else
{
var errorMessages = new string[errors.Count];
for (var i = 0; i < errors.Count; i++)
{
errorMessages[i] = HtmlEncoder.Default.Encode(errors[i].ErrorMessage);
}
errorDictionary.Add(key, errorMessages);
}
}
}
return errorDictionary;
}
}
This is, more or less, a copy paste of the default implementation. The only difference is that we've now wrapped the properties in a HtmlEncoder.Default.Encode call. We've encoded ALL properties, it might be enough to just encode the errors, but it's better to be safe than sorry.
The response from the API will now look like this:
{
"type": "",
"title": "One or more validation errors occurred.",
"status": 400,
"detail": "",
"instance": "",
"traceId": "00-f53b195da9558839c85c1be81c2e3a75-fc861d82402982df-00",
"errors": {
"date": ["The value '2021-10-28<script>alert('you have been hacked')</script>' is not valid."]
}
}
I'm SURE that there's other solutions to this problem, maybe you could rely on the json serializer to do this for you? This was just a quick write up to "warn" people about the default behaviour.
