Harden get url content for apps own cloud com #9473
Conversation
| @@ -17,9 +17,10 @@ class AppHelper implements \OCP\IHelper { | |||
| * Gets the content of an URL by using CURL or a fallback if it is not | |||
| * installed | |||
| * @param string $url the url that should be fetched | |||
| * @param bool $verifyCert Whether the SSL certificate should get verified (defaults to yes) | |||
There was a problem hiding this comment.
defaults to yes => defaults to false
|
@LukasReschke Is this ready to go when we add the public crt? |
Yes. I'd still appreciate some unit tests. |
|
@georgehrke @LukasReschke is this PR to fix #9054 ? |
|
Yes |
| * @return string the content of the webpage | ||
| */ | ||
| public function getUrlContent($url); | ||
| public function getUrlContent($url, $verifyCert); |
There was a problem hiding this comment.
please add default - otherwise you break the api
|
@georgehrke can you finish this today? |
|
setting milestone oc7ce |
|
I'll try, but I am in the university at least til 5pm |
|
@georgehrke @LukasReschke please squash the commits - we need to backport this - thx |
|
@georgehrke I'll hijack this pr then |
|
✅ tests passed https://ci.owncloud.org/job/pull-request-analyser/6085/ |
|
code looks good - requires testing |
|
How to test ? |
I'd try to load apps from the appstore - maybe |
|
Okay, I thought I'd need setting up some kind of local server with valid and invalid certificates. |
Shouldn't work as we still do not have a proper SSL certificate on api.apps.owncloud.com |
So we can test the negative scenario - better than nothing 😉 |
|
@georgehrke @LukeOwncloud I guess we should enable the additional option as well - right? |
|
@DeepDiver1975 which additional option? |
the option to validate the cert in https://github.com/owncloud/core/blob/master/lib/private/ocsclient.php#L56 |
|
@karlitschek @craigpg @LukasReschke I would vote that as a showstopper. Please triage this and rate it yourself. |
|
While having proper HTTPS on the AppStore (and for updates) would be very nice and a nice addition we have to consider that HTTPS does not necessarily have the same security implications as e.g. in a regular WIFI network. To be able to abuse this vulnerability you would need to be able to intercept (or modify the routes) to the API server. This is not something that an attacker is usually able to do on a remote server network. (he could play with BGP or spoofed DNS reply and a few other things but if an attacker is able to do this he can do you much more harm anyways) This is therefore not a showstopper in my opinion as the real-world impact is much less than one would usually assume. However, we should certainly fix this and rate this as an important improvement, but doing TLS properly in PHP is basically a pain and may be depending on the PHP version not even possible. I can take a look at this, but this requires me to setup a proper test environment with a lot of different PHP versions and operating systems. Most likely it will end that we try to use secure HTTPS but if it is not possible by the PHP version we will just have to fallback to "insecure" (unverified) HTTPS. |
|
@LukasReschke Thanks for the clarification. |
|
Yes. This is definitely a nice to have improvement. But not a showstopper. |
|
is this something we want for ownCloud 8? |
|
💣 Test FAILed. 💣 |
|
Do we want this in OC 8 ? |
|
Nice to have but not urgent. Let´s focus on the features and bugs if you ask me. I will check how stable we can make our ssl server cert here so it doesn't break in a few years. |
|
@DeepDiver1975 @LukasReschke Something for 8.1? |
|
Yes – together with #13056 |
|
Requires another approach then though as the server config is now less error-prone. Let's close this for now and tackle again as part of above issue. |
fixes #9054
@LukasReschke please take a look