Server: redirect_url seems to be double URL-decoded after login

[soudis]

Steps to reproduce

1. open index.php/login?redirect_url=/index.php/apps/discoursesso%3Fsso%3Dbm9uY2U9YjVlYzMxNDE3MjFiZTY5Nzg2OGZiMzRiZTMwY2RmZDQmcmV0dXJu%250AX3Nzb191cmw9aHR0cCUzQSUyRiUyRmRpc2NvdXJzZS5oYWJpZGF0Lm9yZyUy%250ARnNlc3Npb24lMkZzc29fbG9naW4%253D%250A%26sig%3D99e5cf7ede3c4819595321f1b0139a226820bb2ae4336a2d3b4a7180244a85df

Expected behaviour

This should actually redirect to my app with the parameters sso and sig

Actual behaviour

Instead it shows a blank page and below error message in the logs. The reason is that the parameter sso contains string "%0A" (URL encoded %250A), which is the URLencoded version of newline. Somewhere this is decoded to the actual new line character, therefore somewhere urldecode is called too often.

{"reqId":"OFod0w9WrbeszhcZQkVk","level":0,"time":"2017-10-12T09:07:55+00:00","remoteAddr":"90.146.106.214","user":"fhumer","app":"PHP","method":"GET","url":"\/index.php\/login?redirect_url=\/index.php\/apps\/discoursesso%3Fsso%3Dbm9uY2U9YjVlYzMxNDE3MjFiZTY5Nzg2OGZiMzRiZTMwY2RmZDQmcmV0dXJu%250AX3Nzb191cmw9aHR0cCUzQSUyRiUyRmRpc2NvdXJzZS5oYWJpZGF0Lm9yZyUy%250ARnNlc3Npb24lMkZzc29fbG9naW4%253D%250A%26sig%3D99e5cf7ede3c4819595321f1b0139a226820bb2ae4336a2d3b4a7180244a85df","message":"Header may not contain more than a single header, new line detected at \/var\/www\/html\/lib\/private\/AppFramework\/Http\/Output.php#68","userAgent":"Mozilla\/5.0 (X11; Linux x86_64) AppleWebKit\/537.36 (KHTML, like Gecko) Chrome\/60.0.3112.101 Safari\/537.36","version":"12.0.3.3"}

I checked the generateRedirect function in the LoginController and there the parameter $redirectUrl is already passed like this:

\/index.php\/apps\/discoursesso?sso=bm9uY2U9YjVlYzMxNDE3MjFiZTY5Nzg2OGZiMzRiZTMwY2RmZDQmcmV0dXJu%0AX3Nzb191cmw9aHR0cCUzQSUyRiUyRmRpc2NvdXJzZS5oYWJpZGF0Lm9yZyUy%0ARnNlc3Npb24lMkZzc29fbG9naW4%3D%0A&sig=99e5cf7ede3c4819595321f1b0139a226820bb2ae4336a2d3b4a7180244a85df

Therefore it has already been decoded once, but in the functioin it gets decoded (urldecode()) again which leads to the error.

Server configuration

Operating system: Debian

Web server: Apache 2

Database: Mysql 10.1.21

PHP version: 7.1.10

Nextcloud version: 12.0.3.3.

Updated from an older Nextcloud/ownCloud or fresh install:

Where did you install Nextcloud from:

Signing status:

Signing status

No errors have been found.

List of activated apps:

App list

Nextcloud configuration:

Config report

{
"system": {
"memcache.local": "\OC\Memcache\APCu",
"instanceid": "ocw8n0b1uteh",
"passwordsalt": "REMOVED SENSITIVE VALUE",
"secret": "REMOVED SENSITIVE VALUE",
"trusted_domains": [
"cloud.willy-fred.org",
"cloud.habidat.org",
"cloud.lebeningemeinschaft.ag",
"cloud.lebeningemeinschaft.at",
"office.willy-fred.org",
"office.habidat.org",
"discourse.habidat.org"
],
"trusted_proxies": [
"172.19.0.200"
],
"datadirectory": "\/var\/www\/html\/data",
"dbtype": "mysql",
"version": "12.0.3.3",
"dbname": "nextcloud",
"dbhost": "habidat-mysql-db",
"dbport": "",
"dbtableprefix": "oc_",
"dbuser": "REMOVED SENSITIVE VALUE",
"dbpassword": "REMOVED SENSITIVE VALUE",
"logtimezone": "UTC",
"installed": true,
"ldapIgnoreNamingRules": false,
"ldapProviderFactory": "\OCA\User_LDAP\LDAPProviderFactory",
"appstore.experimental.enabled": true,
"mail_from_address": "nextcloud",
"mail_smtpmode": "smtp",
"mail_smtphost": "mail.xaok.org",
"mail_domain": "willy-fred.org",
"mail_smtpport": "25",
"maintenance": false,
"loglevel": 0,
"log_rotate_size": 10485760,
"ldapUserCleanupInterval": 10,
"apps_paths": [
{
"path": "\/var\/www\/html\/apps",
"url": "\/apps",
"writable": false
},
{
"path": "\/var\/www\/html\/custom_apps",
"url": "\/custom_apps",
"writable": true
}
],
"debug": true,
"overwrite.cli.url": "https:\/\/cloud.habidat.org",
"overwriteprotocol": "https"
}
}

[nickvergessen]

So I want to open https://nextcloud13.local/index.php/apps/activity/?filter=by
I'm redirected to https://nextcloud13.local/index.php/login?redirect_url=/index.php/apps/activity/%3Ffilter%3Dby and the hidden redirect_url field is filled with /index.php/apps/activity/?filter=by
After the login I'm successfully redirected to https://nextcloud13.local/index.php/apps/activity/?filter=by

Also just for the record index.php/apps/discoursesso?sso=bm9uY2U9YjVlYzMxNDE3MjFiZTY5Nzg2OGZiMzRiZTMwY2RmZDQmcmV0dXJu%0AX3Nzb191cmw9aHR0cCUzQSUyRiUyRmRpc2NvdXJzZS5oYWJpZGF0Lm9yZyUy%0ARnNlc3Npb24lMkZzc29fbG9naW4%3D%0A&sig=99e5cf7ede3c4819595321f1b0139a226820bb2ae4336a2d3b4a7180244a85df is not url encoded, because there are still ? and =. So encoding this is just fine.

So this might be a problem of the discoursesso app

[soudis]

Thanks for the quick reply!

Yes I know the url is not encoded, that's what I wanted to say (sorry for my English). The string is already decoded when it's passed to the generateRedirect() function of the LoginController and in there it's decoded again.

Maybe try:

https://nextcloud13.local/index.php/apps/activity/?filter=b%0Ay

This is of course bogus, but then you also get a blank page and basically the same error.

It cannot be an issue of the discoursesso app because the discouresesso app ist not called at this time. The error happens before the redirect to the app happens.

[nickvergessen]

okay the encoded line break seems to be the issues.
Actually I'm not sure about the urldecode in
https://github.com/nextcloud/server/blob/0bccd5a0d9cda570f405e4f6add4d0a27ec3aee4/core/Controller/LoginController.php#L205

For me the URL is already decoded, maybe we check whether the string already contains = or ? and in case it does we don't decode again? @LukasReschke

[soudis]

@LukasReschke Seems this is waiting of your approval. I'm using a workaround for this for already 4 months now and everything else on our (quite big) nextcloud instance is working just fine. The workaround is to just get rid of the "urldecode" at this this line of code. As @nickvergessen mentioned, the URL is already decoded at this time, so the urldecode here is clearly too much.

I'd be very happy if this could be fixed in 13.0.1, so I can get rid of the workaround (which leads to code integrity check failing all the time). And otherwise the discoursesso (https://github.com/soudis/discoursesso) app will also not be working correctly for other users.

[MorrisJobke]

I could not reproduce this anymore in master (same on 13.0.0) :/

• I browsed to http://localhost:8000/index.php/apps/activity/?filter=by not logged in
• redirect to login happened (to http://localhost:8000/index.php/login?redirect_url=/apps/activity/%3Ffilter%3Dby&user=mjob
• I used the wrong credentials (to cause even one other possible problem)
• I used the correct credentials
• got the proper redirect to the original page

[soudis]

The way to reproduce the issue is to encode some bogus character in the filter parameter, like:

http://localhost:8000/index.php/apps/activity/?filter=b%0Ay

[soudis]

I know, it seems that my plugin (discoursesso) is the only one having troubles with this, but it is a real simple change, I created a pull request and would be really really happy if it would go into nextcloud 14

10681

[blizzz]

I cannot reproduce this on master either

[soudis]

Sorry for not replying for so long and thanks for trying to reproduce the issue. Please (!) take your time and read this carefully, this is a tiny problem for most people, but it is a real bug and a real pain in the ass for me and hundreds of users. I try to explain the problem in more detail and it's connection to the discoursesso App. When using the app Discourse is authenticating through the discoursesso App of Nextcloud. While doing so it redirects to the following URL of nextcloud:

STEP 1 - Original URL:

https://localhost/index.php/apps/discoursesso?sso=xm[...]Z4%3D&sig=dh7[...]91

When not logged in this is redirected to:

STEP 2 - Login URL:

https://localhost/index.php/login?redirect_url=/index.php/apps/discoursesso%3Fsso%3Dxm[...]Z4%253D%26sig%3Ddh7[...]91

Now the problem is, that the original URL already includes an encoded character %3D which is = . This is needed to terminate the SSO secret. When transformed into the redirect_url parameter of the login URL it is encoded again, so the whole thing becomes %253D. Until here everything is fine. BUT: after the login nextcloud redirects to the URL in "redirect_url" but decodes it twice! The result is, that the %3D becomes = in the URL which results in:

STEP 3 - Redirect URL:

https://localhost/index.php/apps/discoursesso?sso=xm[...]Z4=&sig=dh7[...]91

This URL is malformed and the screen remains white without any error message.

As you can see nextcloud messes up the URL that was originally tried to access. The reason being, that the LoginController decodes the redirect_url twice, but it was only encoded once.

Easy test case to reproduce the issue:

As it would be very difficult for you to set up an environment with Discourse to test this, I created the test case above. I try to also explain it again:

1. Log out of nextcloud
2. Go to: http://localhost:8000/index.php/apps/activity/?filter=b%0Ay (%0A being a line feed character)

Nextcloud encodes the URL once into redirect_url and redirects to http://localhost:8000/login?redirect_url=/index.php/apps/activity/%3Ffilter%3Db%250Ay

1. Log in

URL is double-decoded, therefore malformed:

http://localhost:8000/index.php/apps/activity/?filter=b
y

and screen remains white

[nickvergessen]

steps still happen like this in master
I stand with my comment from https://github.com/nextcloud/server/issues/6822#issuecomment-336118361

I think the decode is wrong there, we just never tripped because we never encoded twice and therefor got broken data.

[vranki]

This bug from 2017 effectively prevents use of the discourse sso plugin easily. The fix is one line change (see discourse sso README).

[skjnldsv]

@vranki maybe you could have opened a pr? :see_no_evil:

[skjnldsv]

https://github.com/nextcloud/server/pull/20401

[vranki]

I don't have a NC dev enviornment, I did the change in production server. Also no clue if the fix is really correct, i just "found it on the internet". Sorry.

[skjnldsv]

It was given by a coworker, so I took my chances :stuck_out_tongue_closed_eyes:
Thanks anyway :)