Disable the cookie check when authorization headers are send.

[brantje]

I had a really hard time detecting my redirecting errors.

Finally found them, seems that when the extension calls an endpoint (with an Authorization header set) it send the cookies with them, and no way to disable that.
Nextcloud see's the cookie's and does a check on them using passesLaxCookieCheck and cookieCheckRequired.
In my opinion the cookie check is not needed when using an Authorization header, since you already supply the credentials with it.

Really hope that this could get merged fast, since this is the major blocker for releasing the passman extension.

[skjnldsv]

I'm okay with this. 👍

@nextcloud/security ?

[LukasReschke]

This has a negative security impact. Please explain more detailed what problem you're facing at the moment and then we can take a closer look.

[skjnldsv]

That's what I was afraid of :/
Code looks good and works, but don't know a thing about security on this side of nextcloud!
Thanks for your feedback @LukasReschke 👍

[brantje]

Please explain more detailed what problem you're facing at the moment and then we can take a closer look.

The problem occurs when a user is logged into the nextcloud instance.
When my extension sends an xhr request (which uses the Authorization header) to the instance it sends the session cookie with them.
That is where it fails, and goes crazy and causes an infinite loop.

Extension source: -removed-

The request the build here: https://git.passman.cc/passman/passman-webextension/blob/master/js/lib/api.js#L123
Tried various methods, plain xhr, $.ajax, $http (angular), all giving the same result.

[codecov-io]

Codecov Report

Merging #3361 into master will increase coverage by -0.01%.

@@             Coverage Diff              @@
##             master    #3361      +/-   ##
============================================
- Coverage     53.99%   53.99%   -0.01%     
- Complexity        0    21009   +21009     
============================================
  Files          1303     1303              
  Lines         80377    80377              
  Branches       1253     1253              
============================================
- Hits          43397    43396       -1     
- Misses        36980    36981       +1

Impacted Files	Coverage Δ	Complexity Δ
lib/private/AppFramework/Http/Request.php	86.6% <100%> (ø)	146 <ø> (+146)	✅
lib/private/Files/Cache/Propagator.php	94.93% <ø> (-1.27%)	16% <ø> (+16%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b946ec9...11bb861. Read the comment docs.

[brantje]

Another side effect of placing cookies when a request is made with Authorization headers is that they are instantly logged into the Nextcloud instance with the credentials they supplied in the Authorization header.

So if i logout of nextcloud, make a request with the extension (with auth headers), then i'm back logged into nextcloud.

[animalillo]

I also agree that if you are using basic auth method cookies make no sense at all.
Example: a request from a "super thin" client or an specific hardware client, which will discard your cookies because most of the time that's not worth or even feasible to handle on some hardware.
Our problem with the extension is just the tip of the trouble this can cause.

[brantje]

@LukasReschke Can you update us? We really want to release the extension....

[tapir]

Is there any update on the issue? What kind of negative security impact are you talking about @LukasReschke?

[LukasReschke]

Is there any update on the issue? What kind of negative security impact are you talking about @LukasReschke?

This basically allows bypassing the Same-Site cookie check completely.

Extension source: -removed-

This does not really encourage me to dig into this pull request 😉 – Please post at least a minimum working example of your code so that I can see what you are in detail doing here.

[brantje]

At the moment source is private until we solve this.

Code for making the api request is here: https://gist.github.com/brantje/d7ef651807204c830a1c2bfcd7164359

[LukasReschke]

At the moment source is private until we solve this.

Then please provide a minimum working example of JS code that showcases the problem. The JS code you've pasted is not what I'd consider minimal example. (e.g. not something that I can easily paste in my browser console and have it do something)

[brantje]

Signup on https://git.passman.cc and i will give you access to the repo.
Creating a new extension to debug this is a waste of time, and can be done with the passman extension.

[brantje]

Just found a new problem.
I've setup the extension with UserB.
UserB logs out of the nextcloud instance.
UserA signs on, does some things in nextcloud
Then the extension does a refresh to get new data.
Suddenly UserA is logged out and UserB is logged in.

Looks like an account take over, because nextcloud sets cookies when HTTP_AUTH is used.

[LukasReschke]

Looks like an account take over, because nextcloud sets cookies when HTTP_AUTH is used.

This is the expected behaviour as otherwise the sync clients would need to do an reauthentication action on every attempt which is not really performant and also may result in external network calls. (e.g. LDAP / …)

I don't really see any issue here. Quite frankly, Basic Auth is not the ideal candidate when it comes to authenticating. – Though at the moment it's the primary authentication mechanism but we'll have in the mid- to long-term some more token based authentications in offer.

When doing a Basic Auth you'll be issued HTTP cookies and can then either to decide to reuse those or not. If you reuse them it is however also expected that your application is properly consuming cookies. (i.e. resends all of them and not only a subset)

If you want more flexibility right now you probably want to write your own SyncControllers that can handle authentication the way you require them to be.

Creating a new extension to debug this is a waste of time, and can be done with the passman extension.

And so is signing up for me on some other service 😉 – Please send it via email then or so…

[maestroi]

@brantje this means you could hijack an account?

But at the issue as i understand is that if value teturns false it does will execute rest what will result in nothing so it goes back to start so endless loop!

[brantje]

@maestroi Yes, but @LukasReschke pointed out thats 'by design'

[brantje]

@LukasReschke I've mailed the extension source to your nextcloud mail. The issue should be reproducible without passman.

Not enough knowledge to review now

[brantje]

Closing, issue solved.