remove Sabre_DAV_Browser_Plugin

[DeepDiver1975]

fixes https://github.com/owncloud/security-tracker/issues/56

@LukasReschke @tanghus @karlitschek

[ghost]

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3684/

[karlitschek]

As discussed 👍

[LukasReschke]

This doesn't mitigate the issue completely. GETs on direct links are still working.

We have to set the content type to application/octet-stream to force a download instead of setting the correct mimetype and let the browser render : https://github.com/owncloud/core/blob/master/lib/private/connector/sabre/file.php#L206

Another option would be to ensure that we send a Content-Disposition: attachment header on every response.

@DeepDiver1975 Could you take care of this?

[DeepDiver1975]

@LukasReschke Honestly I'm not happy with neither of the solutions:

1. we really should return the true content type on GET - webdav clients rely on that?
2. Contant-Header: Actually the same: I have no idea about the impact on webdav clients

[LukasReschke]

1. we really should return the true content type on GET - webdav clients rely on that?

Well, SabreDAV itself will per default just return application/octet-stream, so I guess it won't break much.

1. Contant-Header: Actually the same: I have no idea about the impact on webdav clients

Me neither :-)

[DeepDiver1975]

@dragotin @danimo @guruz @ogoffart do you actually care about the content-type on GETting files?

[dragotin]

Yes, we do, to detect zip compressed replies.

[LukasReschke]

@dragotin But I guess sending a Content-Disposition header would work?

[guruz]

We use Content-Encoding to detect gzip replies in mirall.
We don't care about disposition or type.

[dragotin]

@guruz true. My comment was wrong.

But in PROPFIND handling we compare the Content-Type header if it is application/xml (csync_owncloud.c)

[DeepDiver1975]

@guruz true. My comment was wrong.

But in PROPFIND handling we compare the Content-Type header if it is application/xml (csync_owncloud.c)

Thanks - in this case I guess it'll be okay to always return application/octet-stream on GETs

[DeepDiver1975]

I suggest to prepare a new pull request with the content-type change, which we backport slowly after we see no issues poping up.
This pull request can be backported right away to fix the sec issues in OC5 and OC6

[LukasReschke]

👍 LGTM considering the �upcoming Content-Type fix.

[ogoffart]

owncloud do not care, but some proxy in between might care.
At least that's the reason why we have been sending application/octet-stream on PUT request.

[dragotin]

Hmm, maybe WebDAV Clients also do care to start the correct app after having downloaded a file, ie. PDF viewer or so? My gut feeling says that this might be a bad idea, I would test that with a couple of Windows WebDAV clients first.

[DeepDiver1975]

Hmm, maybe WebDAV Clients also do care to start the correct app after having downloaded a file, ie. PDF viewer or so? My gut feeling says that this might be a bad idea, I would test that with a couple of Windows WebDAV clients first.

That for sure requires test - that's why I suggested to perform that changes slowly.

[tanghus]

I suggest to prepare a new pull request with the content-type change,

Please @mention me on that so I can test what KDE says to it :)

[DeepDiver1975]

I will prepare backports .... tomorrow

[DeepDiver1975]

stable5: #7700
stable6: #7701

[PVince81]

@DeepDiver1975 @dragotin this breaks the sync client: when trying to authenticate the user when creating a new account, the client is doing a GET on remote.php/webdav
I'll raise an issue on the mirall project.

[guruz]

@PVince81 Can Sabre_DAV_Browser_Plugin be added only if the URL parameter is a directory? Then it would still work with all client versions.
(And not trigger the security issue? I'm not sure, I don't have access to the security repo)

[PVince81]

I don't know. From the code it looks like it's all or nothing, so it might not be possible to add it conditionally.

[dragotin]

Please mind backward compatibility: Already released clients must remain functional.

[PVince81]

Disabling the browser could be made configurable. It could be enabled by default but for setups (for example enterprise where client versions might be enforced) it could be disabled by the admin.

What do you think ?

Another idea would be to try and make that specific call go through, which means providing another plugin which purpose is only to allow authentication on that base URL.

[PVince81]

Ouch, bitten again by this bug while setting up an sync account on a test VM.

Should this be reverted until we find a solution that doesn't break ? The code isn't released yet and at least it wouldn't force people to manually re-add that line just when testing.

[DeepDiver1975]

revert stable6: a390bd2

[DeepDiver1975]

revert stable5: 140a65e