Skip to content
This repository has been archived by the owner. It is now read-only.

SetDoNotCacheHeaders overwrites headers that are more restrictive #156

Closed
openidauthority opened this issue Aug 4, 2017 · 2 comments
Closed

Comments

@openidauthority
Copy link

@openidauthority openidauthority commented Aug 4, 2017

If a user has set the Cache-Control header for a page, the form tag helper will overwrite the headers. In some cases this is appropriate, but if the Cache-Control header specifies a restriction such as no-store or must-revalidate, the antiforgery system wipes this out. This can open security holes in an application. Here is the relevant code as currently implemented:

    private void SetDoNotCacheHeaders(HttpContext httpContext)
    {
        // Since antifogery token generation is not very obvious to the end users (ex: MVC's form tag generates them
        // by default), log a warning to let users know of the change in behavior to any cache headers they might
        // have set explicitly.
        LogCacheHeaderOverrideWarning(httpContext.Response);

        httpContext.Response.Headers[HeaderNames.CacheControl] = "no-cache";
        httpContext.Response.Headers[HeaderNames.Pragma] = "no-cache";
    }

This should be changed to check the header before overwriting it. If no-cache is already present in the header, the header should be left alone. For more information see https://openidauthority.com/asp-net-core-mvc-anti-forgery-system-opens-security-hole/

@blowdart blowdart added this to the 2.1.0 milestone Aug 4, 2017
@blowdart
Copy link
Member

@blowdart blowdart commented Aug 4, 2017

@Eilon we should address this in 2.1, by splitting the cache control header apart, checking for no-cache and no-store, and appending them to the existing header (if any) rather than just blowing the header away. Forcing the Pragma header is fine, because it only has one value anyway :)

@aspnet-hello
Copy link

@aspnet-hello aspnet-hello commented Jan 1, 2018

This issue was moved to dotnet/aspnetcore#2414

@aspnet aspnet locked and limited conversation to collaborators Jan 1, 2018
@aspnet-hello aspnet-hello removed this from the 2.1.0-preview1 milestone Jan 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants