Make WordPress Core

Opened 13 years ago

Closed 13 years ago

#19337 closed defect (bug) (worksforme)

is_ssl() fails behind nginx proxy

Reported by: niklasbr's profile niklasbr Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.3
Component: Security Keywords: has-patch
Focuses: Cc:

Description

LiteSpeed has some minor differences compared to Apache even though it is advertised as interchangeable, on of these differences is how $_SERVER works. In Apache $_SERVERhttps? exists if the visitor is loading the page via https protocol, but in LiteSpeed $_SERVERhttps? does not exists, LiteSpeed uses $_SERVERHTTP_HTTPS?.

This means that is_ssl() returns false even if the page is visited via https. The patch is to extend the function to solve this case the is_ssl() function would look like this:

function is_ssl() {
	if ( isset($_SERVER['HTTPS']) ) {
		if ( 'on' == strtolower($_SERVER['HTTPS']) )
			return true;
		if ( '1' == $_SERVER['HTTPS'] )
			return true;
	} elseif ( isset($_SERVER['SERVER_PORT']) && ( '443' == $_SERVER['SERVER_PORT'] ) ) {
		return true;
	} elseif ( isset($_SERVER['HTTP_HTTPS']) && 'on' == strtolower($_SERVER['HTTP_HTTPS'])) {
		return true;
	}

	return false;
}

Change History (18)

#1 @kurtpayne
13 years ago

  • Cc kpayne@… added
  • Keywords reporter-feedback added

I was not able to reproduce this using php 5.3.8, litespeed SAPI 5.5, and WordPress 3.3-beta4. Screenshot.

Are you using a port other than 443, or are you seeing $_SERVER['SERVER_PORT'] not being passed correctly? If the $_SERVER['HTTPS'] check fails, then the port check should catch the page being served over SSL.

Can you provide extra info on how to recreate the problem you're seeing?

#2 @nacin
13 years ago

I spent a while the other day searching for any documentation or reference of $_SERVER['HTTP_HTTPS'] and LiteSpeed. Couldn't. This ticket was the most prominent result.

WP.com ran LiteSpeed for a long while. This doesn't seem like a LS issue, and rather something very peculiar to your configuration.

#3 @niklasbr
13 years ago

This is the var_dump($_SERVER) when visiting the site over https, the SSL certificate validates fine with the browser.

array(33) {
  ["SERVER_SOFTWARE"]=>
  string(9) "LiteSpeed"
  ["REQUEST_URI"]=>
  string(8) "/ebutik/"
  ["HTTP_ACCEPT"]=>
  string(63) "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8"
  ["HTTP_ACCEPT_LANGUAGE"]=>
  string(5) "sv-se"
  ["HTTP_CONNECTION"]=>
  string(10) "keep-alive"
  ["HTTP_COOKIE"]=>
  string(773) "__unam=bfcc9ef-133c7419056-330be7f5-239; __utma=56261164.472435995.1321897923.1322472211.1322512573.11; __utmb=56261164.3.10.1322512573; __utmc=56261164; __utmv=56261164.|1=logged-in=yes=1,2=user-id=7=1,3=username=niklas=1; __utmz=56261164.1321897923.1.1.utmcsr=(direct)|utmccn=(direct)|utmcmd=(none); BIGipServerLitespeed-Cluster-02=2064361664.20480.0000; PHPSESSID=46c28e99a0d7dc1edc323cfef9ecf295; wp-settings-7=m13%3Do%26m12%3Do%26m10%3Do%26m11%3Do%26m7%3Do%26m6%3Do%26m5%3Do%26m4%3Do%26m9%3Do%26m8%3Do%26m1%3Dc%26m2%3Do; wp-settings-time-7=1322210344; wordpress_logged_in_b9666c95518e649df272996ade5abf70=niklas%7C1323249736%7C233f731ad9d613973763954aea5cfc28; wordpress_logged_in_f0da2b789df28a217b2da742999f5dbe=niklas%7C1323159387%7C19390f0db7d15d2d4bc5da53b1169be5"
  ["HTTP_HOST"]=>
  string(16) "villasjoviken.se"
  ["HTTP_REFERER"]=>
  string(25) "https://villasjoviken.se/"
  ["HTTP_USER_AGENT"]=>
  string(119) "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_2) AppleWebKit/534.51.22 (KHTML, like Gecko) Version/5.1.1 Safari/534.51.22"
  ["HTTP_CACHE_CONTROL"]=>
  string(9) "max-age=0"
  ["HTTP_DNT"]=>
  string(1) "1"
  ["HTTP_REMOTE_ADDR"]=>
  string(14) "109.228.152.14"
  ["HTTP_LOCAL_ADDR"]=>
  string(14) "109.235.168.73"
  ["HTTP_X_FORWARDED_PROTO"]=>
  string(5) "https"
  ["HTTP_HTTPS"]=>
  string(2) "on"
  ["DOCUMENT_ROOT"]=>
  string(55) "/storage/content/33/108133/villasjoviken.se/public_html"
  ["REMOTE_ADDR"]=>
  string(14) "192.168.11.100"
  ["REMOTE_PORT"]=>
  string(5) "36232"
  ["SERVER_ADDR"]=>
  string(14) "192.168.11.123"
  ["SERVER_NAME"]=>
  string(16) "villasjoviken.se"
  ["SERVER_PORT"]=>
  string(2) "80"
  ["REDIRECT_STATUS"]=>
  string(3) "200"
  ["REDIRECT_URL"]=>
  string(8) "/ebutik/"
  ["SCRIPT_FILENAME"]=>
  string(65) "/storage/content/33/108133/villasjoviken.se/public_html/index.php"
  ["QUERY_STRING"]=>
  string(0) ""
  ["CUR_REQUEST_URI"]=>
  string(10) "/index.php"
  ["SCRIPT_NAME"]=>
  string(10) "/index.php"
  ["SERVER_PROTOCOL"]=>
  string(8) "HTTP/1.1"
  ["REQUEST_METHOD"]=>
  string(3) "GET"
  ["PHP_SELF"]=>
  string(10) "/index.php"
  ["REQUEST_TIME"]=>
  string(10) "1322512854"
  ["argv"]=>
  array(0) {
  }
  ["argc"]=>
  string(1) "0"
}
Version 0, edited 13 years ago by niklasbr (next)

#4 @nacin
13 years ago

nginx prepends anything set via proxy_set_header with HTTP_. That is probably what is happening here for LS.

We've generally maintained that things like setting REMOTE_ADDR based on HTTP_X_FORWARDED_FOR (and HTTPS based on HTTP_X_FORWARDED_PROTO) is a server configuration thing, and not something WordPress should try to mess with. Indeed, WordPress.org itself deals with this in wp-config.php. (See #9235.)

Until that design decision changes, I would think the same should for for HTTP_HTTPS versus HTTPS.

A change to is_ssl() — which already tries pretty hard to detect SSL via 'on', '1', and 443 — might be something we'd consider. Then again, we're still not addressing HTTP_X_FORWARDED_PROTO so I don't think HTTP_HTTPS would be something we'd consider either.

#5 @nacin
13 years ago

niklasbr: I suggest you immediately go into wp-config.php and change your secret keys and salts. You posted enough information in that comment (your cookies) for someone to easily steal your session.

https://api.wordpress.org/secret-key/1.1/salt/

#6 follow-up: @niklasbr
13 years ago

Don't worry, already changed the keys/salts. Thanks though!

What would be the negative consequence of adding $_SERVERHTTP_HTTPS? to the is_ssl() function?

#7 in reply to: ↑ 6 @kurtpayne
13 years ago

Replying to niklasbr:

What would be the negative consequence of adding $_SERVERHTTP_HTTPS? to the is_ssl() function?

At bare minimum, it would establish a precedent of supporting a new platform above and beyond what's stated in the minimum requirements page. Whenever that new platform updates, WordPress users would expect an update. New tickets, new tests. Then users of another platform would point and say "Why not support our platform? You support this other platform!" and the cycle would continue.

This may seem hyperbolic, but it's a risk that must be considered. The maintenance costs seem minimal now, but WordPress has been around for a long time, and these little risks and costs add up. The decision to support a new platform, php extension, browser, etc. has to be weighed by the core team carefully.

There are volunteers, like us, who help with the development and testing of WordPress, but ultimately, the core team is responsible for full time, day to day operation, development, and maintenance of everything WordPress related and they have the final say.

#8 follow-up: @dd32
13 years ago

The problem here is, What Proxy configurations do you support? There are at least 4 different fields you can use for the Remote IP when proxied, none of them guaranteed to be reliable, Many different ways to present SSL information, etc

In this case, Remapping/correcting the $_SERVER vars to what you know is good for your server configuration after it's been proxied is a must, only the server admin really knows what fields their proxy is passing that the proxy has been set and can trust the value of.

You'll want to correct REMOTE_ADDR as well in wp-config.php (for spam filtering and comment logging)

#9 in reply to: ↑ 8 @niklasbr
13 years ago

Replying to dd32:

The problem here is, What Proxy configurations do you support?

I would also like to know this because wp-support.se (The Swedish WordPress support forum) is hosted on the same server, they may have to move to a different provider if you officially do not support this type of provider.

#10 follow-ups: @dd32
13 years ago

they may have to move to a different provider if you officially do not support this type of provider.

My point was more of a "There are so many ways you can configure something, Supporting one leads to supporting many". Any proxy configuration is "supported" by WordPress, you just need to remap the server vars based on whatever that particular proxy configuration is using.

Most of the IP-related proxy fields can't be trusted by WordPress (as any browser can add them), supporting half the proxy doesn't make sense (ie. just the SSL stuff) and can lead more support queries as to why somethings work, but others don't.

#11 @dd32
13 years ago

  • Summary changed from is_ssl() fails on LiteSpeed server to is_ssl() fails behind nginx proxy

#12 in reply to: ↑ 10 @niklasbr
13 years ago

Ok. How can I best override is_ssl() with my own function (so that plugins and WP will use another is_ssl() while at the same time allowing me to update Wordpress to future versions) if you will not include this patch?

#13 follow-up: @dd32
13 years ago

In your case, you need this in your wp-config.php file (probably best placed at the top of the file):

$_SERVER['REMOTE_ADDR'] = $_SERVER['HTTP_REMOTE_ADDR'];
$_SERVER['HTTPS'] = !empty($_SERVER['HTTP_HTTPS']) ? $_SERVER['HTTP_HTTPS'] : 0;

That should work for any nginx proxies using the default reverse proxy rules, extra lines would be needed for those using sub-directory proxying, or using older nginx versions, or using other reverse proxies (varnish, squid, etc)

Last edited 13 years ago by dd32 (previous) (diff)

#14 @niklasbr
13 years ago

Thanks dd32

#15 in reply to: ↑ 13 @SergeyBiryukov
13 years ago

Replying to dd32:

$_SERVER['HTTP_REMOTE_ADDR'] = $_SERVER['HTTP_REMOTE_ADDR'];

Shouldn't it be REMOTE_ADDR on the left?

#16 @dd32
13 years ago

shouldn't it be REMOTE_ADDR on the left?

Good catch. Updated.

#17 @niklasbr
13 years ago

Do you still need some sort of reporter-feedback?

#18 in reply to: ↑ 10 @SergeyBiryukov
13 years ago

  • Keywords reporter-feedback removed
  • Milestone Awaiting Review deleted
  • Resolution set to worksforme
  • Status changed from new to closed

Replying to dd32:

Any proxy configuration is "supported" by WordPress, you just need to remap the server vars based on whatever that particular proxy configuration is using.

Note: See TracTickets for help on using tickets.