DRUPAL-SA-CORE-2016-003 Completely broke IIS drupal deployments

All we did we implement Microsoft's advice see https://support.microsoft.com/en-us/kb/3179800 - if that advice doesn't work is that our fault?

The PHP version thing is just probably reflecting which builds you tested with the latest PHP release... 5.6.23 and 5.5.37 both HTTP poxy fixes ... so does 7.0.8... but all the previous versions are vulnerable.

Weird the latest releases aren't on https://secure.php.net/releases/ hence my error.

@david_garcia I'm not sure this is critical tbh. If this breaks your webserver then you're probably not secure since I'd wager that many people using IIS are not on the latest PHP versions. In order to secure your webserver you have to configure your server correctly. IMO it would be critical if we continued to serve the site which with the current patch we'll continue to do. And if you think that the error given by IIS is not explicit enough that's a bug in IIS. To be honest the error looks pretty good.

Personally all I think we should do in this issue is an @todo to web.config and .htaccess to remove the http_poxy mitigations once the minimum version of Drupal is greater than 7.08. And perhaps we can add information about this to the security release notes.

This is introduced in 8.1.x, so moving there.

What I never got about HTTPoxy was that they said, that unsetting via PHP is not enough to be secure.

If I see both the getenv() and the $_SERVER to bogus values - if its set at all, I can break an application that depends on the HTTP_PROXY environment var, but 99% of sites would not use that.

Also looking at the PHP code it is enough to unset it / set it to something else.

So a "fix" in PHP would break those installations using an obscure environment var (instead of proper config), but I know of no system that does.

So maybe in addition to the requirement warning, we could just unset the proxy header AND the getenv() on affected PHP versions. (and possibly allow an override for it via a hidden setting)

After all they could be using some obscure nginx config or something else or whatever ...

--

OT: The part "do not even bother to fix this in PHP - you can't."

was the most panic-y thing I disliked about the HTTPoxy thingy, because that creates unnecessary FUD.

Of course you can, you just break your application for the case where that env var is set, but using env vars for configuring a proxy is "meh" anyway ...

So put it in a variable, putenv() it and override $_SERVER and call it a day ...

--

TL;DR: Lets make Drupal secure out of the box for HTTPoxy (on unsupported configurations and IIS) by unsetting both things (getenv, $_SERVER) if using an affected PHP version; allow to override it (for the 0.00001% edge case) and warn about it in requirements, so that broken sites know where to look.

If I had to guess exactly 0% Drupal 8 sites use the environment variable to configure a proxy for the application.

@Fabianx see https://gist.github.com/alexpott/c3903713ac888b069b915975162dc718 - it gets quite complex to work this out - apache_setenv()... I wonder what the equivalent would be for IIS? nginx, etc...

+++ b/core/modules/system/system.install
@@ -832,6 +832,44 @@ function system_requirements($phase) {
+      // Some mitigation on the PHP side.
+      global $DSA_HTTP_PROXY;
+      $DSA_HTTP_PROXY = $_SERVER["HTTP_PROXY"];
+      putenv("HTTP_PROXY=0");
+      unset($_SERVER["HTTP_PROXY"]);

This doesn't make sense to do in hook_install and also probably does not work.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -986,6 +989,18 @@ protected function initializeSettings(Request $request) {
+  protected function httpoxyMitigation(Request $request) {
+    putenv("HTTP_PROXY=0");
+    unset($_SERVER["HTTP_PROXY"]);
+  }

As detailed several times this is not enough. I think the patch is fine without the mitigation since setting up Guzzle to not use environment variables is already done by core and this is the only likely vulnerability. If contrib or custom set up Guzzle and do not use the D8 provided service then they are on their own. We could say this in the SA to cover our backs.

Probably should also discuss how we mitigate (or don't) for IIS and older PHP versions with the sec team.

At a minimum, I think we need to update the SA with complete instructions for IIS (as well as maybe a pull request to httpoxy.org) and add this as a known issue in our release notes.

1. +++ b/core/modules/system/system.install
   @@ -832,6 +832,39 @@ function system_requirements($phase) {
   +        'description' => t('Either update your PHP runtime version or uncomment the "Erase HTTP_PROXY" rule in your web.config file and add HTTP_PROXY to the allowed headers list to protect yourself against the Httpoxy vulnerability. See @url', ['@url' => 'https://www.drupal.org/node/2783079']),
   

   Isn't there a third option, something they can set in the IIS config?

   Also, we should not link issues in UI text. Instead, we should update the official SA and link that, or possibly https://support.microsoft.com/en-us/kb/3179800, with an accessible link text.

   Finally, I think the canonical form of the vulnerability name is all lowercase.

2. +++ b/core/modules/system/system.install
   @@ -832,6 +832,39 @@ function system_requirements($phase) {
   +        'severity' => REQUIREMENT_WARNING,
   

   Should it be an error instead of a warning?

New sites will not be able to install until they fix it if we make it an error. Sites that are already installed are currently in a situation where they are broken, but protected. If we were to release this fix, they would start working again, but be vulnerable until they took additional mitigations. With it as an error, a message will be displayed on their admin pages, so they only know to mitigate it if they check the status report. Is that sufficient to mitigate the vulnerability when they update? Would it require a PSA?

For better or worse we should copy https://httpoxy.org/ and go lower case.

I like the solution of ensuring new installs have to address the issue before installing and existing sites still work but have a big warning.

The patch looks good - @david_garcia, I agree with you "that's what they do in other places" is often a bad thing but in this case we are talking about a name. If I insisted that my name is "alex" and that I always wanted it to be lowercase - that would be okay - because it is my name and therefore my right to call myself what I like and insist of capitalisation I prefer. The namers of httpoxy have made a choice - whether we like their choice or disagree is immaterial.

From a security perspective making the web.config change optional but with an error on the status report if not implemented is better than the current situation where if the rule breaks your site you'd be tempted to remove the rule and not upgrade PHP. Also in core and anything that uses the http_client service is not vulnerable because of other, earlier, mitigations.

+++ b/core/modules/system/system.install
@@ -832,6 +832,39 @@ function system_requirements($phase) {
+    if (!$httpoxy_rewrite) {
+      $requirements['iis_httpoxy_protection'] = [
+        'title' => t('IIS httpoxy protection'),
+        'value' => t('Your PHP runtime version is affected by the httpoxy vulnerability.'),
+        'description' => t('Either update your PHP runtime version or uncomment the "Erase HTTP_PROXY" rule in your web.config file and add HTTP_PROXY to the allowed headers list. See more details in the <a href=":link">security advisory</a>.', [':link' => 'https://www.drupal.org/SA-CORE-2016-003']),
+        'severity' => REQUIREMENT_ERROR,
+      ];

I'd be happier if this sent an e-mail (for example if update status is configured to send security e-mail notifications). We can't always guarantee that people see hook_requirements() messages so theoretically we're making a site less secure (if they updated core, fixed their IIS config to allow the header or already had it set, then updated again to this version).

However since core itself isn't vulnerable, it might be OK to leave it like this.

To clarify why core in it's default configuration is not vulnerable. As part of the httpoxy mitigations added to core we initialize the http_client service using the values in \Drupal\Core\Http\ClientFactory::fromOptions():

      // Security consideration: prevent Guzzle from using environment variables
      // to configure the outbound proxy.
      'proxy' => [
        'http' => NULL,
        'https' => NULL,
        'no' => [],
      ]

The only way of telling Drupal to use an outbound proxy is to follow the example settings in default.settings.php:

/**
 * External access proxy settings:
 *
 * If your site must access the Internet via a web proxy then you can enter the
 * proxy settings here. Set the full URL of the proxy, including the port, in
 * variables:
 * - $settings['http_client_config']['proxy']['http']: The proxy URL for HTTP
 *   requests.
 * - $settings['http_client_config']['proxy']['https']: The proxy URL for HTTPS
 *   requests.
 * You can pass in the user name and password for basic authentication in the
 * URLs in these settings.
 *
 * You can also define an array of host names that can be accessed directly,
 * bypassing the proxy, in $settings['http_client_config']['proxy']['no'].
 */
# $settings['http_client_config']['proxy']['http'] = 'http://proxy_user:proxy_pass@example.com:8080';
# $settings['http_client_config']['proxy']['https'] = 'http://proxy_user:proxy_pass@example.com:8080';
# $settings['http_client_config']['proxy']['no'] = ['127.0.0.1', 'localhost'];

We used to say in default.settings.php that:

 * If these settings are not configured, the system environment variables
 * HTTP_PROXY, HTTPS_PROXY, and NO_PROXY on the web server will be used instead.

This is no longer true.

Maybe we should add some of this information to https://www.drupal.org/SA-CORE-2016-003?

OK I don't love this, but given the original patch was hardening rather than closing an actual security issue, it seems like the least worst option.

Committed/pushed to 8.3.x, thanks!

Leaving at 'to be ported' for 8.2.x, and agreed it would be good to add more information to the SA. @david_garcia would you be up for drafting something (if you post it here, one of us could edit it in easily from that).

It would be good to get that up before cherry-picking to 8.2.x, but it'd also be good to get this patch into 8.2.0, so see how it goes.

That's been quiet, so cherry-picked to 8.2.x, thanks!