Validate Sec-Fetch headers and Origin header

[Zoey2936]

Currently AIO uses CSRF-Tokens, this is already great, but the protection could be extended by also verifying the Sec-Fetch-* Headers and the origin Header sent by the browser to further check this.

[szaimen]

Hi, this sounds like a great idea to further improve the security!

Would you mind creating a PR? :)

Also cc @pabzm

[Zoey2936]

The only issue is that any redirect issued by /api/auth/getlogin is also counted as no-same-site, so the redirect to / and then to /containers is also counted as cross-site, the only fix would be to not use a Location header based redirect

Any ideas on this?

[pabzm]

If that means we need to use a meta-element with refresh attribute for the redirect, I would oppose this. That kind of redirect is really bad for accessibility. I would prefer less strict checks of sec-fetch-header instead as a trade off.

But are you sure about the redirects from /api/auth/getlogin to / (and further) not being same-site? In my understanding they should even be same-origin, and only the request to /api/auth/getlogin obviously needs to be allowed cross-site. Maybe my understanding is wrong, do you have a hint?

[Zoey2936]

Yes, I also don't like meta+refresh as solution.

But are you sure about the redirects from /api/auth/getlogin to / (and further) not being same-site?

Yes, I'm sure and I tested it. It is the similar to #8064

A less strict rule set could be:

1. allow all "none" and "same-origin" request
2. allow all GET requests to /api/auth/getlogin, / and /containers (and /login for failed logins from nextcloud), with sec-fetch-mode set to navigate and sec-fetch-dest to document
3. block everything else

After some checks I also noticed that the Origin header cannot be checked since browser sets it to the string "null" instead of not sending it at all in some cases (normal html forms like aio uses them) so allowed requests would be blocked. Or the string "null" could be handled as "not set" (this could be spoofed) or the referrer policy is changed to same-origin so the header is set instead of "null", but I think it is simpler to not check this header, since the Sec-Fetch-* headers already do the same for us if we enforce them, so missing headers are blocked. But checking this header would be required if empty sec-fetch header are allowed for older browsers.

The question is: How sensitive are GET requiests to /api/auth/getlogin, /, /containers and /login? Since they would be allowed cross origin

[Zoey2936]

see #8786

[pabzm]

With the changes from #8793 in place, I'm happy to require Sec-Fetch-Headers.

And with #8765 merged I think the redirect problem is reduced to allow cross-site-requests to /api/auth/getlogin and /login, which I would be fine with.

[szaimen]

This is now released with v14.4.0 Beta. Testing and feedback is welcome! See https://github.com/nextcloud/all-in-one#how-to-switch-the-channel