nextcloud / server

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

Access forbidden: CSRF check failed on logout #17065

Closed alvvdc closed 3 years ago

alvvdc commented 4 years ago

Steps to reproduce

  1. Install Lighttpd and PHP.
  2. Download Nextcloud server ZIP.
  3. Log in Nextcloud, and when you log out, in the most cases you get the next error:

Access forbidden CSRF check failed

Screenshot at 2019-09-09 15-18-28

Expected behaviour

Log out succesfully.

Actual behaviour

When I try logout, I get the telled error. Then, if I refresh the website the session is still active.

In Network Firefox explorer (from F12) the logout request get a 412 status code:

Screenshot at 2019-09-09 15-27-30

This error is with Lighttpd web service, with Nginx works fine.

Server configuration

Operating system: Debian / Raspbian Web server: Lighttpd Database: MariaDB PHP version: PHP 7.3 Nextcloud version: (see Nextcloud admin page) 16.0.4 Updated from an older Nextcloud/ownCloud or fresh install: Fresh install Where did you install Nextcloud from: https://download.nextcloud.com/server/releases/nextcloud-16.0.4.zip

simogeo commented 4 years ago

I'm also impacted with CSRF major issue. This issue make talk/spreed unusable ....

I understand you are still working on fixing this.

My simple question is : Is it possible to disable CSRF check in configuration file ?

Thanks for replying me

MichaIng commented 4 years ago

@simogeo The problem is not the included security check itself, but that the whole URI query string is "destroyed" by any handler before it reaches the Nextcloud script. In case of the logout, the session token is added as regular HTTP query string as identifier which user to logout from which session. Hence it must be intact for the script to do it's job, while the security check failure is only a side-symptom of the actual issue.

Please check your webserver/proxy/load balancer, if present, if those manipulate the URI, especially the query string, like Lighttpd with this option enabled.

It must be taken as a matter of fact, that transferring information via URI query simply is not failsafe since webservers, load balancers, proxies and all such can and in at best rare cases alter/decode those, before it reaches the PHP handler, where it is decoded a second time, before it reaches Nextcloud scripts. For me it is not really understandable, why any of this entities should do this, as IMO it must be matter of the final page/script to do what ever it wants to do with the query string.

simogeo commented 4 years ago

thanks @MichaIng for you reply.

Actually I use Apache2 as webserver with a standard configuration (and no load balancing). I never experienced that kind of CSRF issue and do not have on others applications running on same machine ... That's why I'm suspicous on nextcloud. Furthermore, I get this issue since some updated versions ...

By the way, I get an erratic behaviour on that issue. It sometimes works correctly, sometimes not. I don't know from where to start investigating.

D'ont you think this could be due to many trusted_domain in config file ? (I have two)

MichaIng commented 4 years ago

@simogeo A wrong or missing trusted domain should lead to a different error message.

That the error occurs sometimes and sometimes not is expected when you read the above discussion. I works if your current session token contains no + character and it does not work if it contains one. This is since 2%B decodes to + while a + decodes to a white space. This is the only known case currently where a doubled decoding leads to a wrong character at the final PHP script.

So the behaviour in your case indicates that the root issue is the same. I am just not sure what or where the doubled decoding happens. Probably Apache2 has a similar setting than Lighttpd, but I don't know currently which it is and where to look for it. I suggest you go through your Apache2 config + all included config snippets and mod configs and watch out for things like HTTP/URL/query normalization/decoding.

LeSpocky commented 4 years ago

Note that there's a known bug in lighttpd-1.4.53 involving expansion of + characters. This is planned to be fixed in 1.4.55.

https://redmine.lighttpd.net/issues/2999

I was running NC17 on Debian 10 (buster) with lighttpd 1.4.53 and was also affected by this. After upgrading lighttpd to 1.4.55 from buster backports, logout was successful now without that CSRF error message anymore.

simogeo commented 4 years ago

Thanks again @MichaIng And I guess, the default nextcloud .htaccess cannot cause this trouble ?

Here is mine :

<IfModule mod_headers.c>
  <IfModule mod_setenvif.c>
    <IfModule mod_fcgid.c>
       SetEnvIfNoCase ^Authorization$ "(.+)" XAUTHORIZATION=$1
       RequestHeader set XAuthorization %{XAUTHORIZATION}e env=XAUTHORIZATION
    </IfModule>
    <IfModule mod_proxy_fcgi.c>
       SetEnvIfNoCase Authorization "(.+)" HTTP_AUTHORIZATION=$1
    </IfModule>
  </IfModule>

  <IfModule mod_env.c>
    # Add security and privacy related headers
    Header always set Referrer-Policy "no-referrer"
    Header always set X-Content-Type-Options "nosniff"
    Header always set X-Download-Options "noopen"
    Header always set X-Frame-Options "SAMEORIGIN"
    Header always set X-Permitted-Cross-Domain-Policies "none"
    Header always set X-Robots-Tag "none"
    Header always set X-XSS-Protection "1; mode=block"
    SetEnv modHeadersAvailable true
  </IfModule>

  # Add cache control for static resources
  <FilesMatch "\.(css|js|svg|gif)$">
    Header set Cache-Control "max-age=15778463"
  </FilesMatch>

  # Let browsers cache WOFF files for a week
  <FilesMatch "\.woff2?$">
    Header set Cache-Control "max-age=604800"
  </FilesMatch>
</IfModule>
<IfModule mod_php7.c>
  php_value mbstring.func_overload 0
  php_value default_charset 'UTF-8'
  php_value output_buffering 0
  <IfModule mod_env.c>
    SetEnv htaccessWorking true
  </IfModule>
</IfModule>
<IfModule mod_rewrite.c>
  RewriteEngine on
  RewriteCond %{HTTP_USER_AGENT} DavClnt
  RewriteRule ^$ /remote.php/webdav/ [L,R=302]
  RewriteRule .* - [env=HTTP_AUTHORIZATION:%{HTTP:Authorization}]
  RewriteRule ^\.well-known/host-meta /public.php?service=host-meta [QSA,L]
  RewriteRule ^\.well-known/host-meta\.json /public.php?service=host-meta-json [QSA,L]
  RewriteRule ^\.well-known/webfinger /public.php?service=webfinger [QSA,L]
  RewriteRule ^\.well-known/nodeinfo /public.php?service=nodeinfo [QSA,L]
  RewriteRule ^\.well-known/carddav /remote.php/dav/ [R=301,L]
  RewriteRule ^\.well-known/caldav /remote.php/dav/ [R=301,L]
  RewriteRule ^remote/(.*) remote.php [QSA,L]
  RewriteRule ^(?:build|tests|config|lib|3rdparty|templates)/.* - [R=404,L]
  RewriteCond %{REQUEST_URI} !^/\.well-known/(acme-challenge|pki-validation)/.*
  RewriteRule ^(?:\.|autotest|occ|issue|indie|db_|console).* - [R=404,L]
</IfModule>
<IfModule mod_mime.c>
  AddType image/svg+xml svg svgz
  AddEncoding gzip svgz
</IfModule>
<IfModule mod_dir.c>
  DirectoryIndex index.php index.html
</IfModule>
AddDefaultCharset utf-8
Options -Indexes
<IfModule pagespeed_module>
  ModPagespeed Off
</IfModule>
#### DO NOT CHANGE ANYTHING ABOVE THIS LINE ####

ErrorDocument 403 //
ErrorDocument 404 //
MichaIng commented 4 years ago

@simogeo Nope the .htaccess works fine on all instances I know. Do you use php-fpm or mod-php on Apache?

simogeo commented 4 years ago

It is supposed to be mod-php !

MichaIng commented 4 years ago

@simogeo Okay yes if you did not actively setup php-fpm it is mod_php by default on all distros I know.

Sadly this means I have no idea why you face the issue. Lastly it means that transferring access tokens via GET request or browser URI query string is simply not reliable due to some IMO bad webserver/proxy/whatever practice and Nextcloud should switch to POST requests where possible

simogeo commented 4 years ago

The issue makes nextcloud instance web interface almost unusable ...

@MichaIng : thanks for all your effort and feedback. If someone has new idea, I'll take it !

hoonto commented 4 years ago

I came upon this issue as it also affects front-channel logout uri usage when using nextcloud in an OIDC configuration where login/logout are not driven by the user clicking something in nextcloud itself. Logout needs to be a GET and needs to not have a CSRF token for that use case I believe (please correct me if I'm wrong - I'd love to be wrong here).

For an example of that see: https://gluu.org/docs/gluu-server/operation/logout/

I would need to affect a logout of nextcloud by logging out of a different application both of which are controlled by an OP such as gluu.

It is possible I could work around doing the single logout scenario in that fashion (the GET, which won't work due to CSRF token) if I had a post endpoint, and a URI that wouldn't fail (and therefore also without CSRF token).

simogeo commented 4 years ago

just to let you know. I've been testing a bit with chrome and it seems I do note have the same issue. That means the problem could be caused by Firefox itself or some installed plugins. I'll investigate a bit more.

(I had a look on https://github.com/nextcloud/server/issues/13893)

MichaIng commented 4 years ago

From all we found, the issue is not related to the browser, but to the webserver. You can verify by trying a bid more often (multiple sessions) with both browsers. Expected is that logout sometimes fails and sometimes succeeds, as the reason is if the session token by chance contains a + character or not.

LeSpocky commented 4 years ago

Expected is that logout sometimes fails and sometimes succeeds, as the reason is if the session token by chance contains a + character or not.

Okay then, wild idea: why not make sure there's no '+' in the session token but only alphanumerics (7bit ascii) in the first place?

simogeo commented 4 years ago

From all we found, the issue is not related to the browser, but to the webserver. You can verify by trying a bid more often (multiple sessions) with both browsers. Expected is that logout sometimes fails and sometimes succeeds, as the reason is if the session token by chance contains a + character or not.

Indeed ... I have to say again that my problem is not only on logout but on "random" (ajax) call ... I've tried not forcing SSL redirect from .htaccess Apache file. (Should I start a new issue ?)

MichaIng commented 4 years ago

Very likely AJAX sends a GET request as well. To be true, I am not sure how those session tokens are created, probably a 3rd party library does this so that the characters are out of control of Nextcloud. I think the sustainable solution indeed is to switch to POST requests everywhere and by this avoid bearing the query string at all. While in case of single characters, the + sign seems to be the only issue here, just in theory the decoded string could contain other patterns which would then be re-decoded non-intentionally.

@ChristophWurst @rullzer Any more info required from your side? I can do further testing/debugging and such, but I lack the insights and PHP coding experience to open a PR. As well it looks like a more fundamental change to switch everything to POST (not only logout), but it makes sense to implement it fully and consequently when already touching it?

twiguard commented 4 years ago

In my case the "CSFR check failed" appeared in Firefox after I turned off opcache.save-comments, because I wanted to test the caching. Vivaldi showed me where the problem was.

MichaIng commented 4 years ago

Please verify that it can be repeatedly (new login sessions) replicated with opcache.save-comments enabled respectively disabled. Would be an interesting find but I cannot imagine how it can have an influence since cached PHP script comments should have no influence on the query string? But probably it breaks something else, causing the same error. Since the error does not show up consistently based on the change of a + in the query string, it is important to verify that any change causes or fixes the error consistently.

Could you show the exact hint that Vivaldi gave you, pointing to opcache.save-comments?

simogeo commented 4 years ago

I finally identified the issue which was relative to permissions on PHP sessions folder.

A chmod 777 / 755 on /var/lib/php/sessions fixed it !

Sessions works well on others vhosts...

MichaIng commented 4 years ago

Good to know that this leads to the same error. 733 should be btw sufficient, so that the webserver/PHP user is able to create session files. However doesn't matter much as long as the contained session files have 600.

simogeo commented 4 years ago

Good to know that this leads to the same error. 733 should be btw sufficient, so that the webserver/PHP user is able to create session files. However doesn't matter much as long as the contained session files have 600.

Actually and strangely I needed to chmod 777 ... with 755 permissions users were not able to login !

MichaIng commented 4 years ago

755 of course does not work since it does not include write permissions (5 = 1 (execute) + 4 (read)). So the webserver/PHP is not able to create the session files. 733 implies write permissions (3 = 1 (execute) + 2 (write)) but denies the non-required read (file listing) permissions. Since session files are sensitive, I would follow the defaults and deny content listing for all users (but root).

simogeo commented 4 years ago

Sure ! Thanks for pointing this out !

LeSpocky commented 4 years ago

Actually and strangely I needed to chmod 777 ... with 755 permissions users were not able to login !

777 is most probably not necessary. Your lighttpd runs as certain user/group. Rights are given for user, group, and other. Why would every other user need write permissions to your session files? And besides: this is also most certainly off-topic for this ticket?!

MichaIng commented 4 years ago

The default on Debian/Raspbian/Ubuntu is:

chown root:root /var/lib/php/sessions
chmod 1733 /var/lib/php/sessions

And besides: this is also most certainly off-topic for this ticket?!

That is true, enough about that 😅, however it is good to have PHP sessions dir in mind now to rule this out for further reports + testing.

simogeo commented 4 years ago

And besides: this is also most certainly off-topic for this ticket?!

True but connected in a way ( " CSRF check failed" symptoms ....) and very worth value ! Stop polluting then, but I wanted to shared that, just in case - and for others. @MichaIng : I will try default settings. I don't get why I ended up with this issue (I did not do any changes manually)

praveensl commented 4 years ago

I ran into the same issue.

I tried to find through open issues and couldn't find any way to resolve it.

praveensl commented 4 years ago

@hoonto did you manage to find a force logout URL that doesn't need a token reference?

MichaIng commented 4 years ago

You mean a way to logout ALL current sessions? Since the token contains the info about which session to be logged out, only logging out all sessions would then be logically possible without it. But you don't want this being possible by just visiting an URL unauthenticated 😄.

Just to assure that its the same issue. When hovering the logout button one should see and be able to copy the URL, like

https://my.domain.org/nextcloud/logout?requesttoken=3%2BG8PjLJ%2BmziNaa9qToxFbzi7HhxYdp9aICSNePgqBU%3D%3AmtDTd2WTiiunds3y5QpnX8u4vyo2CI1KBPq5bKyF5X4%3D

There are two %2B sequences, coding for +. Replace those, and only those, percent signs with their encoding %25, so that %2B becomes %252B and the whole example string above becomes

https://my.domain.org/nextcloud/logout?requesttoken=3%252BG8PjLJ%252BmziNaa9qToxFbzi7HhxYdp9aICSNePgqBU%3D%3AmtDTd2WTiiunds3y5QpnX8u4vyo2CI1KBPq5bKyF5X4%3D

Now the first decoding will decode those back and the second decoding will result in the wanted +, so logout should always succeed, at least this is the theory. If this is repeatedly (!) the case (since the error depends on the by chance occurrence of + in session token), then you know that you need to find the proxy/webserver/whatever that causes the doubled URL decoding.

praveensl commented 4 years ago

I had three %2B sections in my URI, I replaced them with %252B but it still gives me the same error. so i guess that hunch doesnt work for in this scenario

gstrauss commented 4 years ago

As was posted above, lighttpd's handling of %2B when normalizing URLs was broken, and URL normalization was enabled by default in lighttpd 1.4.53 [Correction: enabled by default in lighttpd 1.4.54, but also enabled in Debian Buster lighttpd 1.4.53 lighttpd.conf]. The bug is fixed in lighttpd 1.4.55 (released Jan 2020), which is available in buster-backports. @LeSpocky confirmed this above on Mar 25: https://github.com/nextcloud/server/issues/17065#issuecomment-604067469

Please keep lighttpd's URL normalization enabled in lighttpd 1.4.55 and later, and upgrade to lighttpd 1.4.55 if running earlier versions.

@praveensl Using %252B is not the right long-term solution and will likely bite you later. It may be a short-term band-aid until you can upgrade to lighttpd 1.4.55.

To clarify, lighttpd URL normalization does not url-decode. Instead, it normalizes the encoding so that different % encodings all look the same, e.g. there is no need to encode a to %61 unless you are trying to fudge your way around a configuration that checks for a literal a and silently passes %61 (which is later decoded to a by the application). If you want to protect your /admin page, you do not want someone to bypass the restriction with /%61dmin.

MichaIng commented 4 years ago

Using %252B is not the right long-term solution and will likely bite you later. It may be a short-term band-aid until you can upgrade to lighttpd 1.4.55.

Of course this was only to test whether there is indeed doubled decoding the issue 😉. Completely unhandy anyway compared to simply trying a few more times.

If you want to protect your /admin page, you do not want someone to bypass the restriction with /%61dmin.

If this is possible, then this is a serious internal security issue as of course access permissions should be verified according to the final file/resource that is accessed. I mentioned it on the other issue, IMO manipulating the URI (that is passed and parsed by another handlers in case) cannot be a solution for internal security or stability issues that can be exploited by simply using a certain special URI. One never knows which part of the "unnormal" URI was intended and which one not, especially when its about the query string, but also thinking of special (intended) rewrites and such. Having now a few URL-codings decoded or changed while others stay the same only creates a completely unpredictable result for what any handler gets.

But while this topic does not belong here (lets focus on switching to POST) it is quite important if there are really security concerns when leaving those settings disabled in Lighttpd. I suggest we discuss this further on the Pi-hole issue: https://github.com/pi-hole/pi-hole/pull/3329 I'm especially interested if there are really any verified issues and why those cannot be (or were not) fixed internally then, or e.g. the URI is normalised only after all rewrites/redirects/handlers have been passed so that it is in fact only used by Lighttpd to access a local resource directly.

praveensl commented 4 years ago

@gstrauss @MichaIng I'm currently testing out the snap based installation, so it runs on apache. also i tried to find the snap project to see if someone has similar issue but I cant seems to find anything relevant there. I'll try a fresh installation without snap and update here.

one of the inconveniences i've noticed here is the fact that issues in different projets (desktop/andriod/main/etc..) dont seems to get linked up well. I'm new here, but will try to keep linking issues where needed.

praveensl commented 4 years ago

Setup a new ubuntu server and installed 19.01 on LAMP stack. I cant reproduce this issue anymore.

gstrauss commented 4 years ago

(multi-posting to https://github.com/pi-hole/pi-hole/pull/3329 Follow-ups there.)

One never knows which part of the "unnormal" URI was intended and which one not, especially when its about the query string, but also thinking of special (intended) rewrites and such. Having now a few URL-codings decoded or changed while others stay the same only creates a completely unpredictable result for what any handler gets.

I think you misunderstood what is meant by normalization. %-encoding has some complexity since there are slightly different rules for required characters to %-encode in different parts of the URI, but there is a normal form which consists of %-encoding the characters required to be %-encoded. Then, if a specific application is trying encode information in the URI, and needs non-required characters to remain %-encoded to preserve semantics, an application can do so more robustly by using base64 or hex (without %) or other encoding. If %-encoding is used instead, lighttpd provides some flags to loosen the normalization, e.g. url-path-2f-decode, url-path-dotseg-remove, url-query-20-plus.

If you want to protect your /admin page, you do not want someone to bypass the restriction with /%61dmin.

That example was oversimplified and does not apply to lighttpd. lighttpd %-decodes the url-path component for use in $HTTP["url"] conditions in lighttpd.conf. .. path segments are also removed to defend against path traversal attacks (applies to all web servers). This typically matches the user expectation of applying the lighttpd.conf condition to a path which often maps to a filesystem path.

However, URL rewrite and URL redirect produces a URI which must be properly %-encoded. The matching rules constructed by the end-user should match what is intended, and the resulting URI constructed from captured matches should be what is intended by the end-user. mod_rewrite and mod_redirect match on the %-encoded URI. To do otherwise could easily lose information in the encoding and result in broken URIs.

URI normalization allows an end-user to write mod_rewrite and mod_redirect matching rules relying on the fact that the URI against which it is matching is in a normalized form. When you disable this protection in lighttpd, you expose end-users to malicious actors that can manipulate the rewrite and redirect rules.

For those who need to construct complex URIs, lighttpd provides an enhanced syntax for %-encoding or %-decoding matched parts of the URI. https://redmine.lighttpd.net/projects/lighttpd/wiki/Docs_ModRewrite

The official lighttpd code base does not ship with mod_rewrite or mod_redirect loaded, and does not ship with any rewrite or redirect rules.

URI normalization enhances security for end-users employing URL rewrite and URL redirect and gives the end-user more power in controlling the construction of the resulting URI.

Any well-maintained linux distro is either shipping lighttpd 1.4.55, or has backported the lighttpd patch for the %2b decoding bug.

URI normalization in lighttpd first shipped in lighttpd 1.4.50 (Aug 2018) and was enabled by default in lighttpd 1.4.54 (May 2019). The %2b query string decoding bug was reported 7 months later in Jan 2020, patched the same day, and released in lighttpd 1.4.55 (Jan 2020) https://redmine.lighttpd.net/issues/2999

MichaIng commented 4 years ago

I hided and marked the discussion about whether the Lighttpd parse options are reasonable or not as OOT. Its not on Nextcloud to decide this, as Nextcloud does not aim to ship the whole webserver setup, compared to Pi-hole: https://github.com/pi-hole/pi-hole/pull/3329#issuecomment-667824140 Lets focus on collecting and debugging the cases that cause the CSRF check failure to either implement a fix or workaround into Nextcloud or document these in e.g. troubleshooting section of Nextcloud documentation: https://docs.nextcloud.com/server/latest/admin_manual/issues/general_troubleshooting.html#troubleshooting-web-server-and-php-problems

hoonto commented 4 years ago

@hoonto did you manage to find a force logout URL that doesn't need a token reference?

No I didn't, but my solution around nextcloud allows me to grab the token from the head attribute data-requesttoken. Which is clumsy and brittle, but it is my only option right now.

It works, but I fear the day someone changes that name of that attribute or for some reason removes it entirely. I'd really like to have a simple POST logout.

PenguinzPlays commented 4 years ago

Seeing this with the latest version. Only thing my logs show interestingly is a failed call to /apps items: 75.172.96.53 - - [09/Aug/2020:16:58:49 +0000] "GET /apps/accessibility/js/accessibility?v=0 HTTP/1.1" 200 85 "https://app.penguinzmedia.group/ocs/v2.php/cloud/capabilities" "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:79.0) Gecko/20100101 Firefox/79.0" 75.172.96.53 - - [09/Aug/2020:16:58:50 +0000] "GET /apps/theming/js/theming?v=0 HTTP/1.1" 200 232 "https://app.penguinzmedia.group/ocs/v2.php/cloud/capabilities" "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:79.0) Gecko/20100101 Firefox/79.0" 75.172.96.53 - - [09/Aug/2020:16:58:50 +0000] "GET /apps/encryption/ajax/getStatus HTTP/1.1" 200 77 "https://app.penguinzmedia.group/ocs/v2.php/cloud/capabilities" "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:79.0) Gecko/20100101 Firefox/79.0" 75.172.96.53 - - [09/Aug/2020:16:58:57 +0000] "GET /ocs/v2.php/apps/notifications/api/v2/notifications HTTP/1.1" 200 492 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:79.0) Gecko/20100101 Firefox/79.0" 75.172.96.53 - - [09/Aug/2020:16:59:27 +0000] "GET /ocs/v2.php/apps/notifications/api/v2/notifications HTTP/1.1" 200 492 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:79.0) Gecko/20100101 Firefox/79.0"

kroll-j commented 3 years ago

FYI, I ran into the same issue, in my case with lighttpd, and it seems I found a workaround here. I created and enabled /etc/lighttpd/conf-available/10-parseopts-url-normalize-required.conf with this content:

server.http-parseopts := (
    "url-normalize-required" => "disable"
)

Not quite sure what other implications this will have, but I logged in&out a dozen or so times and so far it seems to be working. url-normalize-required is documented here.

I wonder if the problem is related to this lighttpd issue, but that seems not to be the case if the same happens with other web servers.

MichaIng commented 3 years ago

Jep as documented above: https://github.com/nextcloud/server/issues/17065#issuecomment-573261112 And yes, the Lighttpd issue you linked is the upstream report and fix, as the %2B decoding that causes the issue was not intended. Current stable Debian and Raspbian versions (Buster) will most likely not receive this fix anymore, hence disabling the URL normalization is (aside of source compiling) the only way to fix it there, likely on other non-rolling Linux distros as well. And although having some discussion about the impact of this Lighttpd option, until now I could not find a single real example that disabling it has any security (or other negative) impact, means it should be just fine 😉.

gstrauss commented 3 years ago

lighttpd developer here: it is a terrible idea to disable that security setting since you will probably never re-enable it. Yes, there was a bug in lighttpd that was fixed the same day it was reported and released the same month, and that was over 7 months ago.

If you want an update in your favorite distro, please make the request to the distro. I submitted patches to Debian, but they're just sitting there: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=961843 If enough people ask Debian to pick up the patch, maybe they'll do so. That's a much better approach than disabling security features.

The fix is already available in Debian backports, which has lighttpd 1.4.55. The above link is to backport the fix to the main branch.

gstrauss commented 3 years ago

until now I could not find a single real example that disabling it has any security (or other negative) impact, means it should be just fine

That is a baseless statement and is incorrect. The setting would not exist without a reason.

url-normalization does not protect lighttpd, it protects custom lighttpd configurations and backend application servers, where admins and programmers may not be so careful to have explict allow and default deny rulesets.

MichaIng commented 3 years ago

It potentially breaks backend applications (as long as the query string is still manipulated by Lighttpd) which are responsible for their own security and how they deal with incoming requests. If a backend/application gets an unexpectedly manipulated query string, that potentially causes a security issue (like in this case, I know due to a bug) instead of fixing some. Whatever the webserver does, admins and application developers will always be able to create wide security holes by misconfiguration and insecure code/methods. And when backend developers are forced to double check query strings for each and every webserver individually, implement workarounds and compatibility code, exceptions or whatever, this potentially creates additional security issues.

I mean you know best if Lighttpd is secure enough for production use or not, without having these options enabled, but I just want to give you the view from PHP/backend developer perspective. Nextcloud here is not the only case where this v1.4.53 bug causes issues it is not the only case where Lighttpd creates the need for additional workarounds/compatibility code, at least would, but finally full/official support for this webserver is simply not implemented because of that, which is a pity as it always worked great for me. Sometimes it's good to stick with standards (how other webservers handle it) which developers and admins can rely on, trust in them being able to configure/code their environment securely, otherwise take the responsibility and consequences themselves.

Good point to vote for merging v1.4.55 into Buster stable 👍, that would really help: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=961843

gstrauss commented 3 years ago

I mean you know best how secure Lighttpd is without having these options enabled

Did you intentionally state the opposite of what I said and then attribute it to me? Your statement reads with malicious intent.

I do know how to secure lighttpd and it is best practice to retain the secure defaults (enabled). It is also best practice to run the latest official lighttpd release, where all known bugs have been addressed.

tl;dr: lighttpd developer states: do not disable the secure defaults @MichaIng does not know better.

MichaIng commented 3 years ago

Sorry bad wording, "how secure" was meant to include "insecure", I rephrased it, so yes you know best if it is secure enough or not to disable this setting to workaround the bug. Sadly upgrading is simply not an option for many admins, when not being experienced with compiling and not all distros provide backports, Raspbian does not.

Btw, as Apache and Nginx do not have such options, are they insecure? Or does Lighttpd have some internal differences that requires those options to gain same security? Since you mentioned backend software as well of course this has nothing to do with Lighttpd and you simply seem to have a different view about security than Nginx and Apache developers (whether to normalise an URL before passing to handler or whether this is the job ob the backend application) which AFAIK do no query string normalisation before passing the request to an external PHP handler.

Hedgehog57 commented 3 years ago

I have this issue on nginx + php-fpm + nextcloud 19.0.2

If we have a symbol '+' in the requesttoken autologout is failed and CSRF is received. Changing '+' to '%2B' in the URL give a result.

Nginx also decode URL if one make a rewrite. There you can see a ticket about and a thread on stackoverflow

https://stackoverflow.com/questions/28684300/nginx-pass-proxy-subdirectory-without-url-decoding/37584637#37584637 https://trac.nginx.org/nginx/ticket/727

MichaIng commented 3 years ago

Changing '+' to '%2B' in the URL give a result.

Was the + already visible in the browser address bar when doing the logout? Actually it should be always encoded there as %2B, at least was on all my tests, otherwise the expected decoding in the PHP $_GET array would already break it, so you mean the request send by the proxy, I guess. Above in this thread proxies (with other webserver) were as well mentioned as a possible reason for doubled decoding (once at the proxy, once by PHP). If I understand correctly with Nginx, it is the rewrite (without using $request_uri) that causes the decoding, not proxy_pass itself (basically you said it)? The last answer looks quite simply to omit the path element of the location without having anything decoded (due to no rewrite), but zero up-votes probably mean that it doesn't work.

One could see the whole issue from the other side and could argue to not use any character in URI/query string that can be URL decoded, in this case not use + at all. But it seems to be pretty standard (don't remember an exact example but pretty sure saw it elsewhere)? The (decoded) token is a combined hash already, so probably this is not possible without switching the hash algorithm? Here my insights end 😅.


I just asked about the status of the requested Lighttpd patches for Debian Buster. Good step to ask for integrating the patches only (instead of asking to merge the new upstream release) which should hopefully succeed "soon" 👍.

Hedgehog57 commented 3 years ago

Was the + already visible in the browser address bar when doing the logout?

Yes it is. Changing it in browser to %2B and pressing Enter make a successfull logout. Maybe a recommended Nginx configuration from Nextcloud can be modified to use a non-decoded $request_url in case of /logout?requesttoken=... ?

https://docs.nextcloud.com/server/latest/admin_manual/installation/nginx.html

MichaIng commented 3 years ago

That is very strange, as the request token is definitely encoded, otherwise would never work. Code has changed since we looked through it, but here is how it was that time: https://github.com/nextcloud/server/blob/5bf3d1bb384da56adbf205752be8f840aac3b0c5/lib/private/legacy/user.php#L280

Here is the logout URL of my instance:

https://<omitted>.org/nextcloud/logout?requesttoken=uCDwHbPgdKsuJPK71ePeLml92t%2BmDgGa0%2BuKxdXEbVs%3D%3A4nSCLtquRuJeEsKMsa%2B6VgVLqKbLOTDDqb35joTxAhA%3D

You see %2B and %3D, all as it needs to be.

Maybe a recommended Nginx configuration from Nextcloud can be modified to use a non-decoded $request_url in case of /logout?requesttoken=... ?

The correct logout URL is generated within the scripts based on the CSRF token and a secret, merged and encrypted, so it cannot be retrieved from any Nginx variable. The final URL is URL-encoded and that is what you see as link on the logout button. If that is really a non-encrypted version, there is something seriously wrong, prooobably it has to do with the browser (only showing or actually using decoded URLs from links), did you try a different one?

Hedgehog57 commented 3 years ago

There are some strings from nginx access log:

1. <ip> - - [11/Sep/2020:18:32:07 +0300] "GET /logout?requesttoken=uBHp/FX7tBNnOkA/CQd/fDRDXH8Y4LO71mVVKKRxY2w=:90Gb0wO9zUkGTRpbTz4qNWcCDkh0st+LsDwUQvc3Ox4= HTTP/2.0" 412 15006 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.83 Safari/537.36" "-"
2. <ip> - - [11/Sep/2020:18:35:01 +0300] "GET /logout?requesttoken=iOwyYG5amY2Eo1buEb0MtcIhNJZ3MVSZO1AXgsHmhJc=:uKl+KDgu8b7A0SCfYOp59IdYR9geVS7AYh5UqZKpzfw= HTTP/2.0" 412 15006 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:80.0) Gecko/20100101 Firefox/80.0" "-"
3. <ip> - - [11/Sep/2020:18:52:09 +0300] "GET /logout?requesttoken=/Lon6w9VZqAp5+qoepPuw3HksLoX6TYmZsnpYtcebTw=:s+pVxFkTH/pIkLDMPKq7iiKl4o17u1oWAJCoCIRYNU4= HTTP/2.0" 412 15006 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.83 Safari/537.36" "-"
4. <ip> - - [11/Sep/2020:18:55:03 +0300] "GET /logout?requesttoken=wPvSJg8KdyIp9W70TVCVxlINcpR1sjLRz/C9kdEV/ps=:8L6ebll+HxFthxiFPAfghxd0Adoc1kiIlr7+uoJat/A= HTTP/2.0" 412 15006 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:80.0) Gecko/20100101 Firefox/80.0" "-"
5. <ip> - - [11/Sep/2020:19:12:10 +0300] "GET /logout?requesttoken=4GU4sfMAhCt+zrtAQGYpXXYnhgmIcXpfGkXKFMWESsU=:rzVKnqVG/XEfueEkBl98FCVm1D7kIxZvfByLfpbCErc= HTTP/2.0" 412 15006 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.83 Safari/537.36" "-"
6. <ip> - - [11/Sep/2020:19:15:05 +0300] "GET /logout?requesttoken=/hgdn/MkXCSuWqAUI/GYiu3GYuy3rUysrKw6GoFefVc=:zl1R16VQNBfqKNZlUqbty6i/EaLeyTb19eJ5MdIRNDw= HTTP/2.0" 499 0 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:80.0) Gecko/20100101 Firefox/80.0" "-"
7. <ip> - - [11/Sep/2020:19:15:05 +0300] "GET /logout?requesttoken=/hgdn/MkXCSuWqAUI/GYiu3GYuy3rUysrKw6GoFefVc=:zl1R16VQNBfqKNZlUqbty6i/EaLeyTb19eJ5MdIRNDw= HTTP/2.0" 303 0 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:80.0) Gecko/20100101 Firefox/80.0" "-"
8. <ip> - - [11/Sep/2020:19:22:35 +0300] "GET /logout?requesttoken=4GU4sfMAhCt+zrtAQGYpXXYnhgmIcXpfGkXKFMWESsU=:rzVKnqVG/XEfueEkBl98FCVm1D7kIxZvfByLfpbCErc= HTTP/2.0" 412 15006 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.83 Safari/537.36" "-"
9. <ip> - - [11/Sep/2020:19:23:13 +0300] "GET /logout?requesttoken=4GU4sfMAhCt%2BzrtAQGYpXXYnhgmIcXpfGkXKFMWESsU=:rzVKnqVG/XEfueEkBl98FCVm1D7kIxZvfByLfpbCErc= HTTP/2.0" 303 0 "-" "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.83 Safari/537.36" "-"

412 - is the code when we have + in the URL. 303 - is the code when + is masked by %2B

Strings 6 and 7 - that was a session w/o + in the URL so it has made an autologout by himself Strings 8 and 9 - I have changed + to %2B in the URL by hand after CSRF error.

URLs looks excactly like that in browser too. With + and :.

If that is really a non-encrypted version, there is something seriously wrong, prooobably it has to do with the browser (only showing or actually using decoded URLs from links), did you try a different one?

Of cource. Above You can see FF and Chrome. It is like that for may of our users, not only me.