Don't perform CSRF check on OCS routes with Bearer auth by rullzer · Pull Request #7873 · nextcloud/server

rullzer · 2018-01-15T21:06:48Z

Todo:

@daita please verify
@Dagefoerde could you also check this out? THNX.

ArtificialOwl · 2018-01-16T11:01:53Z

clean nc14 (with the fix)+moodle34+plugin for moodle from owncloud :

nextcloud.log:

 {
"reqId": "0RVGuWfh1lauoLRxJaXi",
"level": 2,
"time": "2018-01-16T10:54:30+00:00",
"remoteAddr": "165.90.126.124",
"user": "--",
"app": "core",
"method": "POST",
"url": "/index.php/apps/oauth2/api/v1/token",
"message": "Login failed: 'MQvMykk8q2JuLDs8tuMx6gRpDmOiyS7AqZl0f8PJ8bimg33SQ6bAuLMMuoDkFeYz' (Remote IP: '165.90.126.124')",
"userAgent": "MoodleBot/1.0",
"version": "14.0.0.0"
}

apache:

165.90.126.124 - - [16/Jan/2018:10:54:24 +0000] "GET /index.php/apps/oauth2/authorize?client_id=MQvMykk8q2JuLDs8tuMx6gRpDmOiyS7AqZl0f8PJ8bimg33SQ6bAuLMMuoDkFeYz&response_type=code&redirect_uri=http%3A%2F%2Fmoodle%2Fadmin%2Foauth2callback.php&state=%2Frepository%2Frepository_callback.php%3Fcallback%3Dyes%26repo_id%3D19%26sesskey%3DkHGEq3wjlO&scope=openid%20profile%20email%20files%20ocs HTTP/2.0" 303 -
165.90.126.124 - - [16/Jan/2018:10:54:24 +0000] "GET /index.php/login/flow?clientIdentifier=MQvMykk8q2JuLDs8tuMx6gRpDmOiyS7AqZl0f8PJ8bimg33SQ6bAuLMMuoDkFeYz HTTP/2.0" 200 2912
165.90.126.124 - - [16/Jan/2018:10:54:25 +0000] "GET /core/css/login/authpicker.css?v=ad8161bf-0 HTTP/2.0" 200 146
165.90.126.124 - - [16/Jan/2018:10:54:25 +0000] "GET /core/css/guest.css?v=ad8161bf-0 HTTP/2.0" 200 4758
165.90.126.124 - - [16/Jan/2018:10:54:25 +0000] "GET /core/js/login/authpicker.js?v=ad8161bf-0 HTTP/2.0" 200 274
165.90.126.124 - - [16/Jan/2018:10:54:26 +0000] "GET /core/img/background.jpg?v=0 HTTP/2.0" 302 -
165.90.126.124 - - [16/Jan/2018:10:54:26 +0000] "GET /core/img/actions/confirm-white.svg?v=2 HTTP/2.0" 200 405
165.90.126.124 - - [16/Jan/2018:10:54:26 +0000] "GET /index.php/apps/files/ HTTP/2.0" 200 5912
165.90.126.124 - - [16/Jan/2018:10:54:28 +0000] "GET /index.php/login/flow/redirect?stateToken=Mj1KtGayVkdRVUFN4kSv5mcL6KoIfdLyK88nPxrafCSiTy6EuY3xqurhaq2QrhgQ&clientIdentifier=MQvMykk8q2JuLDs8tuMx6gRpDmOiyS7AqZl0f8PJ8bimg33SQ6bAuLMMuoDkFeYz&oauthState=/repository/repository_callback.php%3Fcallback%3Dyes%26repo_id%3D19%26sesskey%3DkHGEq3wjlO HTTP/2.0" 200 2709
165.90.126.124 - - [16/Jan/2018:10:54:28 +0000] "GET /core/js/login/redirect.js?v=ad8161bf-0 HTTP/2.0" 200 102
165.90.126.124 - - [16/Jan/2018:10:54:28 +0000] "GET /core/img/background.jpg?v=0 HTTP/2.0" 302 -
165.90.126.124 - - [16/Jan/2018:10:54:29 +0000] "POST /index.php/login/flow HTTP/2.0" 303 -
165.90.126.124 - - [16/Jan/2018:10:54:29 +0000] "GET /index.php/apps/files/ HTTP/2.0" 401 43
165.90.126.124 - MQvMykk8q2JuLDs8tuMx6gRpDmOiyS7AqZl0f8PJ8bimg33SQ6bAuLMMuoDkFeYz [16/Jan/2018:10:54:30 +0000] "POST /index.php/apps/oauth2/api/v1/token HTTP/1.1" 200 296
165.90.126.124 - - [16/Jan/2018:10:54:31 +0000] "PROPFIND /index.php/remote.php/webdav/ HTTP/1.1" 405 -
165.90.126.124 - - [16/Jan/2018:10:54:35 +0000] "PROPFIND /index.php/remote.php/webdav/ HTTP/1.1" 405 -

ArtificialOwl · 2018-01-16T11:14:11Z

nevermind, wrong endpoint for the webdav

rullzer · 2018-01-16T14:13:47Z

@pierreozoux maybe you can test this as well?

codecov · 2018-01-25T15:11:10Z

Codecov Report

Merging #7873 into master will decrease coverage by <.01%.
The diff coverage is 60%.

@@ Coverage Diff @@## master #7873 +/- ##
===========================================
- Coverage 51.7% 51.7% -0.01% - Complexity 25426 25427 +1 
===========================================
Files 1598 1598 Lines 95236 95239 +3 Branches 1376 1376 ===========================================
+ Hits 49243 49244 +1 - Misses 45993 45995 +2

Impacted Files	Coverage Δ	Complexity Δ
...amework/Middleware/Security/SecurityMiddleware.php	`78.04% <60%> (-1.7%)`	`26 <0> (+1)`
apps/files_trashbin/lib/Trashbin.php	`72.46% <0%> (-0.25%)`	`136% <0%> (ø)`
apps/files_trashbin/lib/Expiration.php	`91.93% <0%> (+1.61%)`	`29% <0%> (ø)`	⬇️

pierreozoux · 2018-01-27T17:53:01Z

I confirm it works! Thanks for the proper fix :)
(sorry, I'm a bit new to this code, so it is difficult for me, I thought that it was not that much of a big deal to allow people on this route :) )

rullzer · 2018-01-27T18:43:55Z

@pierreozoux thanks for testing! And also thanks for your first PR, getting to know the code indeed takes time. Looking forward to the next one 😄 !

Dagefoerde · 2018-01-29T13:25:46Z

Hi @rullzer, I tested your fix and it seems to work ok on Nextcloud 12.0.5 (+applied patch) in combination with the Moodle plugin. Thanks a lot for fixing this issue!! 👍

Some general remarks:

Why limit this to requests to OCSController? If requests are authenticated via a Bearer token then we can rule out CSRF issues. I feel that this should be extended towards all endpoints, not just OCS ones.
Is the documentation on the OCS API outdated? NC 12 docs and NC 13 docs say that the response format defaults to XML, but without the format=xml parameter the returned result was JSON. Of course this will lead to problems with our users as we assumed that the specification was correct and therefore relied on XML. Or am I experiencing a configuration issue? (unlikely as this was a fresh install of Nextcloud).
https://github.com/nextcloud/server/pull/7873/files#diff-0bb5cca5abd6cda96b669c31856a3f59R174 says that "we allow Bearer authenticated requests to fail on OCS routes", but it's the check that we allow to fail (but we let the request succeed regardless).

Fixes#5694 Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer · 2018-01-29T13:39:08Z

@Dagefoerde thanks for testing!

Why limit this to requests to OCSController? If requests are authenticated via a Bearer token then we can rule out CSRF issues. I feel that this should be extended towards all endpoints, not just OCS ones.

Because basically the OCS endpoints and the webdav endpoints are the 'official' endpoints we use. If you need more we can look into it. But I'd like to keep the rest of the system as locked down as possible ;)

Is the documentation on the OCS API outdated? NC 12 docs and NC 13 docs say that the response format defaults to XML, but without the format=xml parameter the returned result was JSON. Of course this will lead to problems with our users as we assumed that the specification was correct and therefore relied on XML. Or am I experiencing a configuration issue? (unlikely as this was a fresh install of Nextcloud).

Mmmm maybe you set Accept headers?
Can you show me the call you made?

https://github.com/nextcloud/server/pull/7873/files#diff-0bb5cca5abd6cda96b669c31856a3f59R174 says that "we allow Bearer authenticated requests to fail on OCS routes", but it's the check that we allow to fail (but we let the request succeed regardless).

My bad. Fixed!

Dagefoerde · 2018-01-29T13:49:02Z

If you need more we can look into it.

Not yet, but thank you. This was just intended as a general remark because I was wondering what would be the "right" extent of this patch.

Mmmm maybe you set Accept headers?
Can you show me the call you made?

Fair enough. Moodle's API sets an accept header (see below). Does Nextcloud use the value of this header? Thanks for the hint, I should be able to change that.

‌array (
0 => 'Content-type: multipart/form-data',
1 => 'Authorization: Bearer <>',
2 => 'Accept: application/json',
)

rullzer · 2018-01-29T13:49:59Z

Yes we use the accept header :) I shall fire of an issue to the doc repo to clarify this

rullzer · 2018-01-29T13:56:11Z

nextcloud/documentation#659

nickvergessen · 2018-01-29T13:58:15Z

Fine by me 👍

MorrisJobke · 2018-01-29T14:03:37Z

@rullzer Backport to stable13?

rullzer added enhancement 2. developing Work in progress security labels Jan 15, 2018

rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 16, 2018

rullzer added this to the Nextcloud 14 milestone Jan 16, 2018

MorrisJobke mentioned this pull request Jan 16, 2018
Remove the need for CSRF check on ocs::getCurrentUser #7798
Closed

rullzer force-pushed the fix_5694 branch from 2830a43 to a51a441Compare January 25, 2018 15:10

Dagefoerde mentioned this pull request Jan 29, 2018
making a copy of a file works, creating an alias/shortcut doesnt learnweb/moodle-repository_owncloud#28
Closed

rullzer added 2 commits January 29, 2018 14:37

Don't perform CSRF check on OCS routes with Bearer auth
c0adfa4
Fixes#5694 Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Update tests
7405dfb
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer force-pushed the fix_5694 branch from a51a441 to 7405dfbCompare January 29, 2018 13:37

MorrisJobke approved these changes Jan 29, 2018
View reviewed changes

rullzer added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 29, 2018

rullzer merged commit 6d86dcb into masterJan 29, 2018

rullzer deleted the fix_5694 branch January 29, 2018 14:02

MorrisJobke added the backport-request label Jan 29, 2018

rullzer mentioned this pull request Jan 29, 2018
[stable13] Don't perform CSRF check on OCS routes with Bearer auth #8099
Merged

MorrisJobke removed the backport-request label Feb 5, 2018

Uh oh!

Uh oh!

Conversation

rullzer commented Jan 15, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArtificialOwl commented Jan 16, 2018

Uh oh!

ArtificialOwl commented Jan 16, 2018

Uh oh!

rullzer commented Jan 16, 2018

Uh oh!

codecovBot commented Jan 25, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pierreozoux commented Jan 27, 2018

Uh oh!

rullzer commented Jan 27, 2018

Uh oh!

Dagefoerde commented Jan 29, 2018

Uh oh!

rullzer commented Jan 29, 2018

Uh oh!

Dagefoerde commented Jan 29, 2018

Uh oh!

rullzer commented Jan 29, 2018

Uh oh!

rullzer commented Jan 29, 2018

Uh oh!

nickvergessen commented Jan 29, 2018

Uh oh!

MorrisJobke commented Jan 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rullzer commented Jan 15, 2018•
edited
Loading

codecovBot commented Jan 25, 2018•
edited
Loading