Backport server configuration code from SA-CORE-2016-003 to Drupal 7

I closed #2783239: Backport DRUPAL-SA-CORE-2016-003 hardening as a duplicate, but it pointed out via the linked issue that the IIS code that went into Drupal 8 may have problems - so this issue likely needs work for that too.

Comment added to web.config.

The addition of a comment line in the patch introduced in #8 is helpful.

And further to comment #7, for clarity, the issue regarding problems with IIS is #2783079: DRUPAL-SA-CORE-2016-003 Completely broke IIS drupal deployments

The patch in #8 is favoured because it includes an additional comment line explaining the purpose of the code changes. It looks good and we have been using it for some time without problems.

IIUC if we committed this to D7 there's a risk we'd break IIS deployments in the way outlined by #2783079: DRUPAL-SA-CORE-2016-003 Completely broke IIS drupal deployments (per #7).

So I'd want to see the patch in that issue backported rather than adding the web.config lines which apparently caused problems in IIS.

@mcdruid, Thanks, good catch.

Here is my first attempt at backport of #2783079: DRUPAL-SA-CORE-2016-003 Completely broke IIS drupal deployments as requested in comment #11. Attached is a patch and interdiff.

The patch in comment #13 is incomplete. Here is a patch that is a combined backport of SA-CORE-2016-003 and #2783079: DRUPAL-SA-CORE-2016-003 Completely broke IIS drupal deployments and an interdiff comparing with patch in comment #8.

I wonder if a change record will be required for existing sites to alert them that a deployment change may be required for earlier versions of php.

This will not bother users with PHP >= 7 (where the issue does not exist) and will allow users with PHP < 7 to notice that they need to change some deployment setting instead of just breaking their sites.

and

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.

I think a CR makes sense for this, yep.

We'll need to call out the changes to .htaccess and web.config in release notes.

I'm not crazy about the formatting / indentation here:

  // Warning for httpoxy on IIS with affected PHP versions                          
  // @see https://www.drupal.org/node/2783079                                       
  if (strpos($software, 'Microsoft-IIS') !== FALSE                                  
    && (                                                                            
    version_compare(PHP_VERSION, '5.5.38', '<')                                     
    || (version_compare(PHP_VERSION, '5.6.0', '>=') && version_compare(PHP_VERSION, '5.6.24', '<'))
    || (version_compare(PHP_VERSION, '7.0.0', '>=') && version_compare(PHP_VERSION, '7.0.9', '<'))
    )) { 

...but it looks like the only thing CodeSniffer complains about is the absence of any punctuation at the end of the comment (first line in snippet above):

523 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses

That could be fixed on commit.

It looks like this should be harmless as the addition to web.config is commented out, but it'd be good to have confirmation of a manual test on IIS. Even better if someone could check the status report when running a vulnerable PHP version, but just a "it doesn't break everything" would be sufficient. I've added a tag accordingly, and will try to find a tester in slack next week if necessary.

I did a quick manual test in IIS running in an old VM. Confirmed that the (commented out) changes to web.config don't break anything.. which is as we'd expect.

I also checked the status report warning (I cheated and changed the condition so that my PHP version triggered it) and found a couple of small issues; I hadn't spotted that there's a short array syntax copy-paste in the description (perhaps the PHP 5.3 test I've just kicked off will pick that up?). Plus the :variable syntax being used for the link doesn't work in D7 so we get some malformed HTML.

So this patch fixes those minor glitches, and adds the full stop after the comment that I mentioned previously.

I've also tweaked the layout of the long conditional a little; this may just be subjective but I prefer it spanning fewer lines.

Oops; missed another short array syntax copypasta.

@mcdruid Thanks. The patch looks good and the formatting does improve it. Sorry for missing the short arrays on back port from D8. Patch also applies without difficulty on my tests.

I have created a draft change record, https://www.drupal.org/node/3215527

Thanks for starting the CR.

The changes will not affect sites with PHP >= 7 (where the issue does not exist)

...doesn't match https://www.drupal.org/project/drupal/issues/2783079#comment-11509375 and the logic that we're adding to the requirements check.

If we want to be really helpful, let's show (or link to) the exact changes being made to .htaccess and web.config to make it easier for those to be manually applied to any customised versions of those files.

Taking #19 as RTBC :)

I've modified the change record and decided to show the changes made to .htaccess and web.config rather than the extra step of a link. It's still a draft and open for improvements.

RTBC + 1, patch looks good and security hardening is a good idea

Thank you everybody!