Csrf

[jmattheis]

Previously, the token was passed as X-Gotify-Key by the UI, so there was no way to forge a valid cross site request which included a cookie, because it wasn't passed as cookie.

The gotify token cookie is created with SameSite=strict, this provides some protection against csrf. But an subdomain takeover could still allow for csrf. E.g. evil.gotify.net could send authenticated requests to gotify.net.

This uses the go builtin cross origin protection, listed on the owasp page: https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#built-in-or-existing-csrf-implementations
Which internally uses the Sec-Fetch-Site header.

---

Can be reproduced by allowing all origins via cors GOTIFY_SERVER_CORS_ALLOWORIGINS=".*" and then using a form like this:

<form action="http://localhost:8080/message" method="POST" target="result">
  <label>message</label>
  <input name="message" value="CSRF via plain form">
  <label>appid (owned by the logged-in user)</label>
  <input name="appid" type="number" value="2">
  <button type="submit">POST /message</button>
</form>

<iframe name="result" title="response"></iframe>

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.54%. Comparing base (50b917a) to head (97c425c).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1003      +/-   ##
==========================================
+ Coverage   74.46%   74.54%   +0.08%     
==========================================
  Files          66       66              
  Lines        3473     3485      +12     
==========================================
+ Hits         2586     2598      +12     
  Misses        688      688              
  Partials      199      199              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[eternal-flame-AD]

Can be reproduced by allowing all origins via cors GOTIFY_SERVER_CORS_ALLOWORIGINS=".*" and then using a form like this

Does this mean that this CSRF requires the user to explicit put the hostile subdomain inside the server-side CORS allowlist to begin with? If so it seems like a documentation vagueness problem about the security modeling of these exceptions:

• if they are supposed to be used to identify alias of the same thing like "gotify.mysite.net" , "my-better-ui.gotify.mysite.net" , and "gotify.myhome.local", then this feels like a feature not a bug that these domains can share the access token stored inside the cookie.
• If they are supposed to be used as a third party client access feature like "gotify-ui.third-party.dev" then yes this is not ideal, although I am not sure why in this case this third party application would be deployed in the same site.

[jmattheis]

Does this mean that this CSRF requires the user to explicit put the hostile subdomain inside the server-side CORS allowlist to begin with?

Yes, but I don't think this is normally the case, it's more like a side-effect of the cors middleware that gotify uses. Normally, CORS is enforced client-side by the browser by specifying Access-Control-Allow-* headers.

Simple requests (like POSTing to /message with form variables) are sent to the server without validating CORS headers. The client sending the request, won't receive the response but the request is still processed by the server. https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/CORS#simple_requests For these cases the server should have some guard against CSRF.

The CORS library manually validates the origin against the allowed origins. See https://github.com/gin-contrib/cors/blob/master/config.go#L86 this is somewhat what CrossOriginProtection does, but for me it seems more like a bug, that this is done by the CORS library (and there is at least one ticket about this).

---

I'd also differentiate a bit. The CORS setting generally limit who may use the gotify api. You could e.g. have an extension that sends messages to gotify. In the extension you configure an application token and the gotify url and then it sends messages on some events.

I don't think, a user would expect that the extension is now also able to hijack the user session, and access client endpoints, just by having it configured via CORS.

if they are supposed to be used to identify alias of the same thing like "gotify.mysite.net", "my-better-ui.gotify.mysite.net" , and "gotify.myhome.local", then this feels like a feature not a bug that these domains can share the access token stored inside the cookie.

I could see this as a feature, but I don't think this is a good default.

---

So the change in this PR basically guards against allowed origins to re-use the existing user session of a gotify user.

What do you think?

[eternal-flame-AD]

thanks for the explaination, lgtm

[jmattheis]

Ahh, the extension example is wrong. SameSite=strict on the cookie should prevent the cookie to be added to the request from the extension.

Another similar example would be. A user has homelab.local. Gotify is under gotify.homelab.local. Another service is running under other.homelab.local. The user has configured an app token on other.homelab.local to send messages, but other.homelab.local would be allowed to use the cookie for requests without the change in this PR. same-site is bound to the registerable domain: https://developer.mozilla.org/en-US/docs/Glossary/Site

But yeah, this is not that severe I'd say. You still okay with this? I'm not fully sure anymore, but it does restrict access which is normally a good thing.

Another solution would be using __Host- as prefix for the cookie. But this would require gotify to be served via https, which we currently cannot guarantee. https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Set-Cookie#cookie_prefixes

[eternal-flame-AD]

I understand that this weakness is only applicable to a situation where the user have a subdomain that is hostile, and it must be explicitly allowed via CORS. The slight disagreement is merely the semantics on what the CORS allow list actually means.

It's up to you. I personally would say checking may be more intuitive , although if this is not urgent I prefer a more user intuitive and use-case focused server side CORS (like make an carveout for cross domain message posting with an app token ) and remove the need for the vast majority of overrides here.

[jmattheis]

Okay, then I'll merge this and create a new ticket for follow up. Thanks for the review!