@mattab opened this Pull Request on September 18th 2017 Owner

If the builds isn't green we should figure out why, to make sure that users who use Socket can still auto-upgrade Piwik (a security concern)

refs https://github.com/piwik/piwik/issues/11999

@mneudert commented on September 18th 2017 Member

Using builds.piwik.org probably triggers a false positive here as the URL is also available without https!

Look at this minimum PHP example to trigger the Location header:

$fsock = fsockopen('piwik.org', 80, $errno, $errstr);
fwrite($fsock, "GET / HTTP/1.0\r\nHost: piwik.org\r\nConnection: close\r\n\r\n");
while(!feof($fsock)) { echo fgets($fsock, 1024); }

You will receive the Location header here as seen in the tests. The weird part I found here is that Http::sendHttpRequestBy ignores https and connects using port 80. However if we would fix that to get the following snippet:

$fsock = fsockopen('ssl://piwik.org', 443, $errno, $errstr);
fwrite($fsock, "GET / HTTP/1.0\r\nHost: piwik.org:443\r\nConnection: close\r\n\r\n");
while(!feof($fsock)) { echo fgets($fsock, 1024); }

Still those location headers!

builds.piwik.org behaves as expected but looking at it being reachable via both http and https while piwik.org is broken it still looks fishy...

@mneudert commented on September 18th 2017 Member

I have to amend the above comment with an example not triggering an error:

$fsock = fsockopen('ssl://piwik.org', 443, $errno, $errstr);
fwrite($fsock, "GET / HTTP/1.0\r\nHost: piwik.org\r\nConnection: close\r\n\r\n");
while(!feof($fsock)) { echo fgets($fsock, 1024); }

The port in the host header triggered the redirect!

Could you change your PR to include a modification of sendHttpRequestBy to incorporate the SSL port and ssl:// to see if nothing breaks?

@mattab commented on September 20th 2017 Owner

Hi @mneudert

Could you change your PR to include a modification of sendHttpRequestBy to incorporate the SSL port and ssl:// to see if nothing breaks?

as on a quick read I'm not fully understanding what i should do, would be awesome if you could find some time to try this 🙂

@mattab commented on September 20th 2017 Owner

@mneudert Will release 3.1.1 shortly. Should we merge this PR prior or wait for the next release?

@mattab commented on September 20th 2017 Owner

Looks good Marc, I'll merge as it may help users auto-update and looks safe

@mneudert commented on September 20th 2017 Member

I hope this goes well :D

The problem is with the last commit. It makes the test green, but as far as I know it may violate the HTTP specification.

The problem is that the Host header should contain a port if it differs from 80 (see spec here). It does not specifically mention you can just ignore the port for https.

So if we keep this one we should definitely be aware of potential side effects that may come up! There are places on the internet that say the modified behaviour is correct (like here) but I would still keep an eye on this.

@mattab commented on September 21st 2017 Owner

@mneudert in which cases would the new implement create bugs, is it only when the HTTPS port is different from 443?

@mneudert commented on September 21st 2017 Member

From my understanding before this change every socket connection has always been made using HTTP even if HTTPS was requested.

As HTTPS behaves a bit differently we might see something come up like certificate problems.

But as I just looked up the circumstances necessary for a socket connection to be used I would not worry at all. Having all of "no curl installed", "allow_url_fopen forbidden" AND some "HTTPS certificate issue" at once should be nearly impossible :D

This Pull Request was closed on September 20th 2017
Powered by GitHub Issue Mirror