Earlier today, Microsoft released two security advisories for vulnerabilities I discovered in the ASP.NET Core 2.0 VS2017 templates and reported late October:
CVE-2018-0785 - ASP.NET Core Templates enable Cross Site Request Forgery: a CSRF vulnerability in
ManageController.GenerateRecoveryCodes()
allows re-generating the recovery codes associated with a victim's account, which may result in a definitive account lockout (aka per-user denial of service).CVE-2018-0784 - ASP.NET Core Templates enable Elevation Of Privilege Vulnerability.
While the first one is a very classic case of cross-site request forgery (CSRF), the second one is a bit more interesting as it relies on a specificity of ASP.NET Core MVC to be exploited.
Where was the vulnerability located?
When you create a new project based on the ASP.NET Core 2.0 templates offered by Visual Studio 2017 and opt for individual authentication (that uses ASP.NET Core Identity under the hood), a new ManageController
and a bunch of views are automatically added to the resulting solution.
One of the actions exposed by this controller, EnableAuthenticator
, allows you to generate a shared secret you can use in your favorite TOTP-based application (like Microsoft or Google Authenticator) to enable 2-factor authentication:
1 | [ ] |
For that, a random value is generated server-side by Identity, stored in the database and used to build a special otpauth://totp
URI which is rendered in the Razor view as an HTML attribute (that can be optionally read by a JS library to generate a QR code image client-side):
1 | <li> |
1 | <li> |
What does the vulnerability consist in?
As you can see, the view uses Html.Raw
to render the generated authenticator URI, which is confirmed by the fact the &
character is not properly HTML-encoded (it should be rendered in the HTML document as &
since it's a special character).
Innocently, you might think it's not a big deal since AuthenticatorUri
is generated server-side (and thus can't be directly set by an attacker).
Unfortunately, that's not exact: while it's true that adding an AuthenticatorUri
paramater will have initially no effect on the GET EnableAuthenticator
action (since the value will be always overridden when setting EnableAuthenticatorViewModel.AuthenticatorUri
before returning the view), the query string value will be used if the form is re-displayed.
Yet, that's exactly what the POST action does if the model state is not valid (e.g because the 2FA confirmation code was not correctly typed by the user or was invalid, which may happen if the date/time differ between the server and the device that generated the 2FA code):
1 | [ ] |
This feature – which is part of ASP.NET Core MVC's model binding/validation stack – is extremely useful since it's what allows the users of your websites to avoid re-typing all the values of an invalid form when it's re-displayed.
The bad news is that using this specificity alongside Html.Raw
can result in a XSS vulnerability being exploitable since an attacker can craft a special URL containing a malicious JavaScript payload that will be executed by the victim's browser if he or she sends an invalid 2FA confirmation code.
For instance, if a victim visits https://localhost:44370/Manage/EnableAuthenticator?AuthenticatorUri=%22%3E%3C/div%3E%00%00%00%00%00%00%00%3Cscript%3Ealert(%22XSS%22)%3C/script%3E
(which uses a special pattern to bypass Chrome 61's XSS auditor feature) and enters an invalid code, the following HTML source will be rendered:
1 | <li> |
How can I fix the vulnerability?
Detailed instructions are listed on Microsoft's announcement.
So, what's wrong with Html.Raw
?
In some cases, disabling HTML encoding is just unavoidable, for instance if the content is already encoded: not doing that would result in double-encoding.
So, why shouldn't you use Html.Raw
in your views? It's actually all about intent: Html.Raw
encourages developers to "disable" HTML encoding at the wrong layer (i.e in the view rather than in the controller itself) by declaring already-encoded HTML properties as simple string
properties in their view model rather than as HtmlString
properties.
Using this type instead of Html.Raw
has two big advantages, that would have helped avoid this vulnerability in the first place:
By marking your view model properties as
HtmlString
, you force the developer in charge of maintaining the controller code to manually create aHtmlString
instance from an existing string:new HtmlString(existingEncodedString)
. Technically, the result is exactly the same, but it's now clear by looking at either your view model or your controller code that a property is not a simplestring
, but something hightly susceptible of containing HTML payloads you should treat with extreme care.HtmlString
is not bindable: a form model containing aHtmlString
will never be populated with user input. In this specific case, exploiting the bug wouldn't have been possible withHtmlString
.
Please, do yourself a favor: stop using Html.Raw
.