nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
27.31k stars 4.06k forks source link

Migrating logout requests to POST #23413

Open MichaIng opened 4 years ago

MichaIng commented 4 years ago

How to use GitHub

Is your feature request related to a problem? Please describe. Logouts are currently done as GET requests. To prevent CSRF attacks, the access token is passed via the GET requests query string. In some cases, certain webserver configs or proxy setups, the URL including query string can be (partly) URL-decoded before being passed to PHP. PHP decodes the query string already before storing into $_GET array, which can lead to issues due to doubled decoded parts. In particular this lead to an issue where %2B was decoded by Lighttpd to +, unexpectedly due to a now fixed bug, and by PHP further decoded to white space, leading to a failing CSRF check: https://github.com/nextcloud/server/issues/17065 While this particular issue is solved, there have been similar reports, especially in combination with proxy servers, where the URL or parts of it is decoded by the proxy (or load-balancer) before being passed to the final web server.

These kind of issues show that using GET (i.e. the query string) to transport data is not robust, when it can contain characters or a set of characters which represent as well an URL code. Aside of the apparent issues, POST generally suites better to transport sensitive information (even when being obfuscated), it is not cached, it does not show up in the browsers address bar or browser history and is has no length restrictions.

Describe the solution you'd like As of: https://github.com/nextcloud/server/issues/17065#issuecomment-573394436 Migrate passing the CSRF token to the logout script via POST request.

Describe alternatives you've considered URL or in general request normalisation is a concept, implying security concerns, to assure that all "handlers" along the path of the request interpret it the same way by altering the request to apply certain standards, as inconsistent interpretation can not only lead to failing requests or wrong responses but can be exploited by attackers to store wrong content in proxies caches (cache poisoning) and similar. This can include encoding certain special characters before passing the request as well as decoding certain parts or not allowing certain content at all. While it can be discussed if or how far the webserver should enforce such normalisation as default behaviour, it can be taken as a reason to construct GET request URLs a way that they are failsafe against doubled decoding and normalisation, which means to avoid any special characters which have an URL code meaning, like + and of course % and avoid other uncommon or special meaning characters, like backslashes and dots. While it might leave the scope of what Nextcloud should take care of, an additional documentation and/or debug output could be added to point admins/users into the right direction when a request fails because of a somehow invalid URL received by Nextcloud while being originally crafted by Nextcloud.

oreineke commented 4 years ago

I have this issue with Nextcloud 19 in Docker behind a Traefik Edge Router.

joshtrichards commented 3 days ago

May have to take into consideration handling for custom logout URLs via IdP added in #21966 for things like OIDC/SAML.