andrewlock / NetEscapades.AspNetCore.SecurityHeaders

Small package to allow adding security headers to ASP.NET Core websites
MIT License
704 stars 73 forks source link

.Net 9 Blazor importmap sha creation #189

Closed ghidello closed 2 months ago

ghidello commented 2 months ago

Hi Andrew, I haven't looked at this project in a month and you transformed it! Great job!

I attempted to add your security headers to a .NET 9 Blazor Auto project but I encountered a new importmap script, generated by the main component, which requires either a SHA or a nonce in the CSP. I initially tought to use a nonce and I opened this issue dotnet/aspnetcore#57277

As suggested in the issue, a better approach would be to compute the sha retrieving the script content with HttpContext?.GetEndpoint()?.Metadata.GetMetadata<ImportMapDefinition>(); and adding the result to the CSP.

Do you think it’s possible to create a new extension to add this specific SHA to the script-src-elem directive? Alternatively, could the CspDirectiveBuilder's SourceBuilders list be exposed in some way, allowing a library consumer to build new extensions with a dependency on HttpContext?

andrewlock commented 2 months ago

Hi @ghidello!

There's a couple of aspects to this. Hashes are certainly better than nonces from a security PoV, that's true; nonces are what you need if you don't have any other options.

Now, if this was an MVC/Razor pages option there's an easy solution - the tag helper we have does exactly this. I think we could do this for Blazor too, so that there are dedicated extensions to handle this.

I haven't done a huge amount with Blazor yet though - if the value of ImportMapDefinition is fixed for the app it might be better to determine it statically once instead of re-hashing on every request? πŸ€”

Alternatively, could the CspDirectiveBuilder's SourceBuilders list be exposed in some way, allowing a library consumer to build new extensions with a dependency on HttpContext?

Yeah, that should be doable, it doesn't exist today, but could be worth adding I think πŸ€”

FWIW, I'm working on big changes in a major version which would make this all more doable, but I still think we can do more..

ghidello commented 2 months ago

I haven't done a huge amount with Blazor yet though - if the value of ImportMapDefinition is fixed for the app it might be better to determine it statically once instead of re-hashing on every request? πŸ€”

I think it's fixed but lazy: it will be generated only on the first request to an endpoint (or only to a Razor component's endpoint, I'm not sure). Emphasis on "I think" πŸ˜„

FWIW, I'm working on big changes in a major version which would make this all more doable, but I still think we can do more..

That big change is the transformation I was talking about πŸ˜„ Obviously I noticed it only after making my very simple middleware version because I needed a way to add different headers to different endpoints or group of endpoints. Once again, great job.

damienbod commented 2 months ago

I believe nonces are better to use than hashes. Maintaining hashes in a project is a nightmare and time consuming. Nonces can be applied anywhere and easily maintained. CSP nonces should be the recommended approach. I recommend avoiding tech stacks which don't support CSP nonces.

@andrewlock "Hashes are certainly better than nonces from a security PoV" I don't think this is correct, I believe them equally good.

andrewlock commented 2 months ago

Maintaining hashes in a project is a nightmare and time consuming

That's true if you can't automate it, for sure. Luckily the tag helpers make it easy πŸ˜„ That doesn't apply to Blazor components though obviously...

I don't think this is correct, I believe them equally good.

Interesting πŸ€” For hashes you're essentially saying that only the script that matches should be accepted, but for nonce you're saying anything that has the nonce should be accepted. So my gut thought was that the hash is "more secure". However, as I wrote in the linked issue:

If you can't calculate the expected value ahead of time, and instead need to calculate it at runtime by serializing the ImportMapDefinition, then I don't see how that adds any stronger guarantee? πŸ€” Essentially you're allowing "whatever is in the import map definition" in both cases, the hash is just more expensive to calculate, no? πŸ˜„

In practice, an "attack" using a hash could read the csp ahead of time, and then you'd have find a script that has a hash collision and inject it into the body. In comparison for a nonce, you'd have to read the headers for the given request, then inject into the body.

In both cases you need to inject into the body, so the practical difference is that for the hash you need to find a hash collision (but you can likely do that ahead of time) while for the nonce you have to be able to read the headers in realtime. So it's not an obvious security win either way I guess?

Perf-wise, hashes have the advantage that they can be statically calculated in advance, but if you're using the automatic support, that win kind of goes out the window anyway πŸ€·β€β™‚οΈ I guess one con for nonce is that you can't cache the output πŸ€”

I can't really see anywhere particularly recommending using nonce vs hash one way or another, so overall the assessment makes sense to me @damienbod πŸ˜„

Does that make sense to you @ghidello?

ghidello commented 2 months ago

I often read @damienbod's blog to learn about security so definitely I'm not going to argue about it with him! πŸ˜„ Currently I'm using a custom middleware for calculating the sha at the first request, cache the whole CSP (that sha is the only "mutable" one I have) and check at every request if it has to be recomputed just to be safe. Using a nonce would be way simpler.

andrewlock commented 2 months ago

Great, if I understand the fix in the linked issue, then you should be able to grab the nonce generated by the library (by calling HttpContext.GetNonce()) and set it as an attribute on the <ImportMap />?

ghidello commented 2 months ago

Exactly!

andrewlock commented 2 months ago

Cool, in which case I'll close this issue for now then, thanks @ghidello!