Page MenuHomePhabricator

HTTPS detection is not reliable
Closed, ResolvedPublic

Description

Author: jskarvad

Description:
HTTPS detection is not reliable. It uses $_SERVER['HTTPS'] == 'on', but according to PHP docs [1]:

'HTTPS' Set to a non-empty value if the script was queried through the HTTPS protocol.

This maybe problem e.g. when mediawiki is running on Amazon cloud through HTTPS, their load balancer sets the HTTPS to '1'.

Also the detection code suppose the HTTPS to be SSL on port 443, but IMHO it can be also TLS on port 80 and injecting the explicit port in this case also breaks things for me (Amazon/HTTPS/Firefox). Attached is the fix for mediawiki 1.16. It seems that the latest mediawiki 1.20.3 is also affected, but the code is slightly different there.

This problem results in e.g. inability to save the user preferences.

[1] http://php.net/manual/en/reserved.variables.server.php


Version: 1.21.x
Severity: normal

Details

Reference
bz46511

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 1:15 AM
bzimport set Reference to bz46511.

jskarvad wrote:

Proposed fix for mediawiki-1.16

The latest trunk has the code refactored into functions, but the principle should be the same.

Attached:

jskarvad wrote:

(In reply to comment #1)

Created attachment 11983 [details]
Proposed fix for mediawiki-1.16

Maybe the check should be against '' and not 'off' to be according to the PHP docs.

Attached:

jskarvad wrote:

(In reply to comment #2)

(In reply to comment #1)

Created attachment 11983 [details]
Proposed fix for mediawiki-1.16

Maybe the check should be against '' and not 'off' to be according to the PHP
docs.

ISAPI with IIS uses 'off', so probably the following could be OK:

$wgProto = (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] != '' && $_SERVER['HTTPS'] != 'off') ? 'https' : 'http';

Attached:

jskarvad wrote:

Maybe the relative URLs could be used.

Hi Jaroslav! Thanks for your patch, but unfortunately 1.16 is not maintained anymore. Could you provide a hint (line number) where to see the same problem in 1.20.3, if possible?

jskarvad wrote:

(In reply to comment #5)

Hi Jaroslav! Thanks for your patch, but unfortunately 1.16 is not maintained
anymore. Could you provide a hint (line number) where to see the same problem
in 1.20.3, if possible?

It seems to be in the includes/WebRequest.php (starting from line 1560, functions detectServer() and detectProtocolAndStdPort(). I will try to provide the patch (I need to get it into cloud first to check whether the fix really works there).

jskarvad wrote:

(In reply to comment #6)

starting from line 1560,

Actually line 156.

jskarvad wrote:

Proposed fix for mediawiki-1.20.3

This resolves the problem for me.

  • Fixed HTTPS check to be according to PHP docs.
  • Added port 80 as another default port for HTTPS (TLS)

Reproducer:

  • hosted mediawiki-1.20.3 on Openshift
  • accessed the user preferences through TLS, changed anything and clicked save

Result without patch:
Warning, that the data will be sent unencrypted, if HTTPS is enforced through the Apache rewrite rule, nothing get saved

Result with patch:
No warning, data get saved

Attached:

Platonides: Wondering if you'd be interested in this / could comment, as per bug 28798 comment 10 but maybe I'm wrong.

Anybody could put the patch into Gerrit so we can get it in. See

https://www.mediawiki.org/wiki/Developer_access    and
https://www.mediawiki.org/wiki/Git/Tutorial

(In reply to Jaroslav Škarvada from comment #8)

Created attachment 11984 [details]
Proposed fix for mediawiki-1.20.3

Hmm, https://git.wikimedia.org/tree/mediawiki%2Fcore.git does not list any "php" subfolder in the top level, neither does the 1.19.12 tarball, so it cannot be applied and I am not sure which base this patch is against. Can you clarify?

(Also, this needs to go into Gerrit nowadays: See https://www.mediawiki.org/wiki/Developer_access and https://www.mediawiki.org/wiki/Git/Tutorial )

Attached:

jskarvad wrote:

(In reply to Andre Klapper from comment #10)

(In reply to Jaroslav Škarvada from comment #8)

Created attachment 11984 [details]
Proposed fix for mediawiki-1.20.3

Hmm, https://git.wikimedia.org/tree/mediawiki%2Fcore.git does not list any
"php" subfolder in the top level, neither does the 1.19.12 tarball, so it
cannot be applied and I am not sure which base this patch is against. Can
you clarify?

I will check the current status and in case the problem is still there I will refresh the patch.

(Also, this needs to go into Gerrit nowadays: See
https://www.mediawiki.org/wiki/Developer_access and
https://www.mediawiki.org/wiki/Git/Tutorial )

OK, thanks for info, I will sign-up for the Gerrit account.

Attached:

jskarvad wrote:

It seems the problem with the Amazon WS load balancer was already resolved in the GIT, but the issue with the $_SERVER['HTTPS'] seems still to be unresolved.

The problem is that $_SERVER['HTTPS'] == 'on' is used in the MediaWiki for detections of the HTTPS protocol which is not correct according to PHP documentation [1], citing:


'HTTPS'

Set to a non-empty value if the script was queried through the HTTPS protocol.

    Note: Note that when using ISAPI with IIS, the value will be off if the request was not made through the HTTPS protocol.

I also saw implementation where it was set to '1' instead of 'on'. So to be in sync with the PHP documentation I think it should use:
$_SERVER['HTTPS'] != '' && $_SERVER['HTTPS'] != 'off'
instead of:
$_SERVER['HTTPS'] == 'on'

I will post the patch through Gerrit.

[1] http://www.php.net/manual/en/reserved.variables.server.php

Change 116943 had a related patch set uploaded by Yarda:
Fix HTTPS protocol detection

https://gerrit.wikimedia.org/r/116943

jskarvad wrote:

I have no idea who to assign the review, the list http://www.mediawiki.org/wiki/Developers/Maintainers wasn't helpful, so keeping it as unassigned.

(In reply to Gerrit Notification Bot from comment #13)

Change 116943 had a related patch set uploaded by Yarda:
Fix HTTPS protocol detection

https://gerrit.wikimedia.org/r/116943

Awesome, welcome in gerrit and thanks for your first patch; hopefully the first of a long series.

(In reply to Jaroslav Škarvada from comment #14)

I have no idea who to assign the review, the list
http://www.mediawiki.org/wiki/Developers/Maintainers wasn't helpful, so
keeping it as unassigned.

Usually we just assign to whoever submits the patch, but then it's a collaborative effort. I see you already received a review, if after several days everything is still silent please add more reviewers active on those files and related and/or poke someone on IRC.

Change 132435 had a related patch set uploaded by Reedy:
Fix HTTPS protocol detection

https://gerrit.wikimedia.org/r/132435

Change 132435 abandoned by Reedy:
Fix HTTPS protocol detection

https://gerrit.wikimedia.org/r/132435

Change 116943 merged by jenkins-bot:
Fix HTTPS protocol detection

https://gerrit.wikimedia.org/r/116943

Kizule closed this task as Resolved.Sep 29 2018, 10:18 PM
Kizule assigned this task to Reedy.
Kizule added a subscriber: Kizule.

I think this is resolved, so I closing this task as resolved. Task assigned to Reedy because he made patch for this task.