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

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

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 :)

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

It would be great if another Wicket dev could have a look as well. Perhaps @martin-g or @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.

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.

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

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)

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?

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.

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?

Thanks so much for your review @martin-g!

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

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!

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

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! :)

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.

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.

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.

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.

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

Thank you!

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!