Wicket 6786: Add Fetch Metadata support

[salcho]

Hello Wicket devs,

This PR builds Fetch Metadata support on top of Wicket's existing CSRF protection, namely:

• If a request has Sec-Fetch-* headers (i.e. comes from a modern browser), Fetch Metadata will be preferred. Otherwise, we will fall back to using the existing cross-request checks.
• One default Resource Isolation Policy is provided based on https://web.dev/fetch-metadata/, which prevents all major cross-site request forgery attacks.
• If the Origin or Referer headers are present, Fetch Metadata will apply the same exemptions as the existing Origin-based protection, i.e. allowing cross-origin requests from exempted origins.
• The Vary header has been added to responses through onEndRequest to ensure that any cached responses include Fetch Metadata headers in their key. This is an added layer of security against cache poisoning.

[martin-g]

It would be nice to clean up the commits before merging!

[papegaaij]

I'm on vacation right now, I'll be able to have a look at this pr at the end of July.

[salcho]

Thanks for the quick review Martin! I've sorted out the commits now and updated your comments. We'll hope Edmond will enjoy his holiday and wait for his review :)

[papegaaij]

The implementation looks good to me. The only problem I see is the API change, but that's easy to fix. Just make sure you do not change method signatures for public or protected methods.

[papegaaij]

This PR looks ok to me

[salcho]

That's great! Is there anything else we can do to have this PR merged? :)

[papegaaij]

It would be great if another Wicket dev could have a look as well. Perhaps @martin-g or @svenmeier ?

[svenmeier]

It would be great if another Wicket dev could have a look as well. Perhaps @martin-g or @svenmeier ?

I'll take a look.

[svenmeier]

IMHO it would be better to split CsrfPreventionRequestCycleListener into two (a list of?) 'RequestIsolationPolicies' (resource has a special meaning in Wicket, so I'd rather not have them named ResourceIsolationPolicies - this isn't a web standard terminology, is it?) :

• one for fetch-metadata
• one for the old referrer

So instead of the two requestListeners originally proposed in WICKET-6786, we have one listener with two policies instead.

This would prevent a lot of code duplication, checking the whitelist and handling conflicts would be done in CsrfPreventionRequestCycleListener once only.

Furthermore the whitelist is not checked against the "sec-fetch-site" header currently.
For this the policy would somehow have to provide the sourceUrl to the CsrfPreventionRequestCycleListener - maybe with an additional method.

I hope I was able to convey the solution I have in mind.

[salcho]

Hi Sven,

Thanks for your review! While this moves away from the original proposal (see the conversation on WICKET-6786), I agree that having multiple Resource Isolation Policies is a better pattern. We'll push a commit soon!

On the point of the whitelist not being checked against sec-fetch-site: that is by design, because Fetch Metadata header values are enums rather than origins, so the site in sec-fetch-site doesn't actually carry an origin but rather whether the request is same-origin, same-site or navigational. This is a little confusing, because Wicket implements a whitelist of origins and it's not always possible to get those origins, so this is a best-effort method.

[svenmeier]

Thanks for clarifying the whitelist handling. I was confused because checkRequestFetchMetadata() checks the origin too.

[salcho]

Hi @svenmeier!

@eozmen410 has pointed out something that I missed before that brings us back to the reason why this is implemented the way it is and why we rejected the alternative you propose in our design:

• The legacy check relies on the Referer and Origin headers to determine whether a request is cross-origin. These headers are not reliable since they are not always present and so the legacy check produces false positives, that is: it will, for instance, reject same-site requests that are legitimate because they might not carry Referer/Origin! Whenever we can determine the origin of a cross-origin request, this check works fine.

• Fetch Metadata solves this problem by explicitly marking the request as same/cross-origin reliably, even in the absence of Origin/Referer, which makes the new check false postive-free, hence improving the previous check. That is, in scenarios where the legacy check would have rejected a legitimate request (because there was no Origin header), the new check can now approve the request because sec-fetch-site indicates same-origin.

Because of this, running both checks serially would produce contradictions. The new check would approve and the legacy would reject. That is: whenever the legacy check makes a decision correctly, both legacy and new will agree on the final decision, but in cases where the legacy check produces a false positive, both legacy and new will contradict each other!

In the model you're proposing, if at least one check rejects then the request is fully rejected and so we would be back in the world of false positives that we're trying to get out of.

In summary: while having multiple Resource Isolation Policies is a good idea for the future (for implementing specific policies for iframes, for instance), this PR should be seen as improving only one of those policies and not as adding an additional one.

I hope that makes sense, but we would be happy to jump on a call or chat if you'd like to :)

What do you think?

(tagging @papegaaij for visibility and correctness, since he's very familiar with the legacy check)

[papegaaij]

Yes, I agree. The old approach is only a fallback scenario. When a browser sends Fetch Metadata, that is much more reliable and the old strategy should not be used. It is likely that we will encounter some corner cases with this piece of code, but IMHO the current code is easy enough to adapt by our users if that were to happen. I'm happy with this PR. @svenmeier shall I merge?

[svenmeier]

I'm all for this new feature, and the new implementation works fine as it seems.

But the integration into the old CsrfPreventionRequestCycleListener is just hacky. The whole class is centered around the request origin, and it shows in the new method checkRequestFetchMetadata()

• it checks a origin whitelist, when actually it has nothing to do with origins (note the method name)
• it calls matchingOrigin, when actually it didn't check the origin
• it takes action according to conflictingOriginAction, when actually it doesn't detect any origin conflicts
  There are many other inconsistencies that can easily spotted when reading the Javadoc - which haven't been updated btw.

If this is supposed to be a hack to get this new protection into production ASAP, go ahead - probably no users will see this code anyway.

But if we want to get this integrated nicely, I would suggest we create a new listener and deprecated the old one.

[salcho]

Hi @svenmeier , @papegaaij and @eozmen410

I think Sven is right in saying that the legacy CSRF protection makes many decisions based on the Origin that don't belong in Fetch Metadata. Since we all agree that the legacy protection is still useful to add support for legacy browsers, I've tried to find the right balance between the new security mechanism we want to provide and the legacy one in Wicket, without introducing breaking changes. I've pushed a new commit, here's a summary of the changes:

• It adds a new Fetch Metadata Request Cycle Listener that always runs the default resource isolation policy, which protects against all major CSRF attacks and can be extended in the future to add more policies.
• It makes the legacy CSRF Prevention listener implement ResourceIsolationPolicy. This allows us to reuse its logic around whitelisting origins, hosts and providing protection for legacy browsers. This is left as a choice for developers to opt in, if needed.
• It resolves the contradictory decision making trees of both the default and origin-based policies, such that we won't have the contradictions/false positives mentioned in one of my comments above.
• I have gone as far as marking the CSRF Prevention listener as deprecated! I don't know if this is the right decision and would be happy to get some feedback on this. Perhaps by marking it as such we can get developers to migrate to Fetch Metadata as a definitive security protection against CSRF attacks.

One possible improvement would be to extract the Origin-based protection entirely into a brand new OriginBasedResourceIsolationPolicy and run that by default too. That could potentially provide the best of both worlds.

I hope this finds the right balance/compromises to keep Wicket users safe while introducing no breaking changes!

WDYT?

[martin-g]

Just few minor comments on the logging.
Otherwise it looks very good to me now!
Thank you, @salcho !

[salcho]

Thanks so much for your review @martin-g!

@papegaaij I believe we're ready for merge now that the changes have been approved :)

[papegaaij]

The new RequestCycleListener has many limitations, which would make it impossible for our applications to use it. Also, in this setup it brings very little benefit, because it still has the same false positives as the old implementation.

I've noticed that the code formatting is way off from Wicket standards. The code will need to be reformatted before it can be merged.

[papegaaij]

This setup only allows specific paths to be used cross-site. The old implementation had an isChecked method that would allow whitelisting pages or even specific requests. In our application, we use the latter. Specific components are annotated to be allowed cross-site. This cannot be done with just a path. Why not use the same isChecked as the old implementation?

[papegaaij]

This loop brings us back to the original problem: the old implementation has false positives and now both have to return true. The idea was to use the old as a fallback in case the fetch metadata headers were missing.

The old implementation had 3 possible outcomes: allowed, disallowed and unknown (because no header was found). As far as I can see, the new implementation has the same 3 possible outcomes. The old implementation should only be consulted when the new implementation is unknown.

This also brings us to the next problem: what if both don't know? In the old implementation, you could configure the behavior for that. In fact, you could configure 3 different CsrfActions for both 'unknown' and 'disallowed'. This implementation only supports one: ABORT. Both SUPPRESS and ALLOW are missing.

[papegaaij]

This way the OriginBasedResourceIsolationPolicy is internal to this class and there is no way to configure its whitelist. Also, the whitelist has no effect, because DefaultResourceIsolationPolicy will block the requests anyway.

[martin-g]

I've noticed that the code formatting is way off from Wicket standards. The code will need to be reformatted before it can be merged.

We can do that after merging. This shouldn't be a stopper for contributors!

[papegaaij]

I've noticed that the code formatting is way off from Wicket standards. The code will need to be reformatted before it can be merged.

We can do that after merging. This shouldn't be a stopper for contributors!

Agreed

[salcho]

Hi @papegaaij , @svenmeier, @martin-g & @eozmen410

I notice that different reviewers have conflicting designs in mind. This conflict comes from the fact that @papegaaij's applications use the current CsrfPreventionRequestCycleListener not so much as a mitigation against CSRF attacks, but rather as an authorization or ACL component that exposes applications/pages/handlers to a set of whitelisted origins. Unfortunately, this is not what Fetch Metadata was designed for (and @svenmeier correctly points this out), but rather to simply reject cross-site requests to endpoints that aren't meant to be used cross-site, regardless of the source origin.

At this point, we can make changes to use Fetch Metadata as both a CSRF protection and an ACL component, but I believe we would be making Wicket a disservice by creating a security module that is: trivially bypassable (send a CORS-safelisted request to avoid setting the Origin header, which makes the check undecidable) and very hard to maintain, since the policies would be order-dependent and inter-dependent.

We are very happy to contribute to Wicket, but unfortunately we only have a limited number of resources to do so. Having said that, I have a proposal that I think can reconciliate both of these approaches:

• Revert all changes made to CsrfPreventionRequestCycleListener
• Contribute to Wicket with a pure FetchMetadataRequestCycleListener that is completely independent from the Origin header, providing effective protection against CSRF attacks.
• Allow other listeners to compose this new protection (either by composing the new listener or by picking indvidual resource isolation policies). This way Edmond can have full freedom to integrate Fetch Metadata into his module with all the nuances of his use case.

Our main goal is to provide easy to maintain security mitigations that are effective at keeping users safe in the web platform (here's an example of this in another Apache project) and I believe this proposal achieves just that.

Would Wicket developers be interested in seeing something like this? We would be happy to push another set of changes if so.

Thank you all for your patience and I'm looking forward to reading your thoughts! :)

[papegaaij]

I notice that different reviewers have conflicting designs in mind. This conflict comes from the fact that @papegaaij's applications use the current CsrfPreventionRequestCycleListener not so much as a mitigation against CSRF attacks, but rather as an authorization or ACL component that exposes applications/pages/handlers to a set of whitelisted origins. Unfortunately, this is not what Fetch Metadata was designed for (and @svenmeier correctly points this out), but rather to simply reject cross-site requests to endpoints that aren't meant to be used cross-site, regardless of the source origin.

@salcho The current situation is a bit unfortunate indeed. My application does not use the whitelisting, but others might. Looing at the usage of these whitelists, I think the reason these whitelists exist is not for ACL, but again to prevent false positives. False positives are common with the old implementation and hard to solve.

At this point, we can make changes to use Fetch Metadata as both a CSRF protection and an ACL component, but I believe we would be making Wicket a disservice by creating a security module that is: trivially bypassable (send a CORS-safelisted request to avoid setting the Origin header, which makes the check undecidable) and very hard to maintain, since the policies would be order-dependent and inter-dependent.

I don't think this is needed. CSRF protection is meant to do just that, it's not an ACL. However, it must be possible to exclude some requests from this protection. The old implementation allowed this via an isChecked(IRequestHandler) method, the new implementation should do the same.

I'll take a look at the PR, and see what I can do to the improve protection, keep the flexibility and prevent the false positives.

[svenmeier]

I think we're overdoing it ;)

The actual implementation by @salcho was fine, for me the API wasn't just correct any more with all the old references to 'origin'.

This is the simplest solution I can think of, e.g. renaming some methods and going through a change of responsibility in #checkRequest() is enough:
https://gist.github.com/svenmeier/e2e29ddad37e453c693004946ab125e7

I'd deprecate the old listener, and give the new one a fancy new name.

[papegaaij]

This is what I came up with:
papegaaij@c51372b

The biggest change is that a resource isolation policy now has 3 possible outcomes: ALLOWED, DISALLOWED and UNKNOWN. They are checked in order, and the first not UNKNOWN will determine the final outcome.

Other changes are the reintroduction of the configuration options from the old implementation and the 2 default policies are only added if you did not specify any.

[papegaaij]

Btw, I've also reformatted the code to adhere to the Wicket standards. @salcho and @svenmeier if you two agree with my changes, I can merge it all.

[salcho]

Your reference commit looks pretty good to me @papegaaij! +1 on merging with them!

Thank you!

[papegaaij]

I've merged the PR. Thanks for the excellent work @salcho and my apologies for the bumpy process. I think the whole discussion has led to a better implementation!