Merge tag '6.38' into pressflow-6.38

[mparker17]

https://www.drupal.org/SA-CORE-2016-001

Note there were conflicts in CHANGELOG.txt and includes/common.inc, that were easy to merge, however, one conflict in drupal_set_header() (in modules/system/system.module) was not, and I had to write custom code for it.

Essentially drupal_set_header() had been moved from includes/common.inc into includes/bootstrap.inc, and the two functions had diverged somewhat since the move. Looking at the Pressflow code, it appeared to be vulnerable to the same attack that was patched in Drupal core 6.38 (HTTP header injection using line breaks on PHP versions earlier than 5.1.2).

In Drupal core, the patch was:

diff --git a/includes/common.inc b/includes/common.inc
index 19798ba..9a28c06 100644
--- a/includes/common.inc
+++ b/includes/common.inc
@@ -150,8 +154,15 @@ function drupal_set_header($header = NULL) {
   static $stored_headers = array();

   if (strlen($header)) {
-    header($header);
-    $stored_headers[] = $header;
+    // Protect against header injection attacks if PHP is too old to do that.
+    if (version_compare(PHP_VERSION, '5.1.2', '<') && (strpos($header, "\n") !== FALSE || strpos($header, "\r") !== FALSE)) {
+      // Use the same warning message that newer versions of PHP use.
+      trigger_error('Header may not contain more than a single header, new line detected', E_USER_WARNING);
+    }
+    else {
+      header($header);
+      $stored_headers[] = $header;
+    }
   }
   return implode("\n", $stored_headers);
 }

• see http://cgit.drupalcode.org/drupal/commit/?h=6.38&id=18f1c229fcb87b1a1f94fcb1f0785ba3d40fc402

Pressflow constructs headers differently though: it constructs a $headers array, where it maps $name_lower (i.e.: the properly-cased HTTP header name) to $value (i.e.: the value of that header). It then passes the $headers array to drupal_send_headers().

So in this patch, I added the check around the code which modifies the $headers array:

diff --git a/includes/bootstrap.inc b/includes/bootstrap.inc
index 58b74a6..e50053b 100644
--- a/includes/bootstrap.inc
+++ b/includes/bootstrap.inc
@@ -835,17 +835,32 @@ function drupal_set_header($name = NULL, $value = NULL, $append = FALSE) {
   }
   _drupal_set_preferred_header_name($name);

-  if (!isset($value)) {
-    $headers[$name_lower] = FALSE;
-  }
-  elseif (isset($headers[$name_lower]) && $append) {
-    // Multiple headers with identical names may be combined using comma (RFC
-    // 2616, section 4.2).
-    $headers[$name_lower] .= ',' . $value;
+  // Protect against header injection attacks if PHP is too old to do that.
+  // See https://www.drupal.org/SA-CORE-2016-001, and
+  // https://www.drupal.org/drupal-6.38-release-notes
+  if (version_compare(PHP_VERSION, '5.1.2', '<') &&
+    (
+      (strpos($name_lower, "\n") !== FALSE || strpos($name_lower, "\r") !== FALSE) ||
+      (strpos($value, "\n") !== FALSE || strpos($value, "\r") !== FALSE)
+    )
+  ) {
+    // Use the same warning message that newer versions of PHP use.
+    trigger_error('Header may not contain more than a single header, new line detected', E_USER_WARNING);
   }
   else {
-    $headers[$name_lower] = $value;
+    if (!isset($value)) {
+      $headers[$name_lower] = FALSE;
+    }
+    elseif (isset($headers[$name_lower]) && $append) {
+      // Multiple headers with identical names may be combined using comma (RFC
+      // 2616, section 4.2).
+      $headers[$name_lower] .= ',' . $value;
+    }
+    else {
+      $headers[$name_lower] = $value;
+    }
   }
+
   drupal_send_headers(array($name => $headers[$name_lower]), TRUE);
 }

I would very much appreciate some code review and testing. It's been a while since I've worked with Drupal 6 code and Drupal 6 sites!

[fluxsauce]

Paging @davidstrauss for some additional perspective.

[pwolanin]

The changes outside the headers section look ok.

As an alternative to this code, is there any reason at this point pressflow shouldn't have the same minimum PHP version as Drupal 7?

[dsnopek]

I did the drupal_set_header() bit differently:

index 58b74a6..62b8939 100644
--- a/includes/bootstrap.inc
+++ b/includes/bootstrap.inc
@@ -797,6 +797,10 @@ function drupal_load($type, $name) {
   * Note: When sending a Content-Type header, always include a 'charset' type,
   * too. This is necessary to avoid security bugs (e.g. UTF-7 XSS).
   *
 + * Note: No special sanitizing needs to be done to headers. However if a header
 + * value contains a line break a PHP warning will be thrown and the header
 + * will not be set.
 + *
   * @param $name
   *   The HTTP header name, or a status code followed by a reason phrase, e.g.
   *   "404 Not Found".
@@ -812,7 +816,18 @@ function drupal_set_header($name = NULL, $value = NULL, $append = FALSE) {
    if (!isset($name)) {
      return $headers;
    }
 -  
 +
 +  // Protect against header injection attacks if PHP is too old to do that.
 +  if (version_compare(PHP_VERSION, '5.1.2', '<')) {
 +    foreach (array($name, $value) as $part) {
 +      if (strpos($part, "\n") !== FALSE || strpos($part, "\r") !== FALSE) {
 +        // Use the same warning message that newer versions of PHP use.
 +        trigger_error('Header may not contain more than a single header, new line detected', E_USER_WARNING);
 +        return;
 +      }
 +    }
 +  }
 +

Mine checks earlier, and bails earlier. It also changes less stuff.

[pwolanin]

Drupal 7:

define('DRUPAL_MINIMUM_PHP', '5.2.4');

[mparker17]

@dsnopek Simpler + faster sounds good to me!

[DavidRothstein]

(copying from IRC) I skimmed the commit and think it looks good. For the headers you're basically checking for line breaks in any of the input, which seems right. Another way to do it might have been in the same function that actually calls the PHP header() function (doing it there would more closely match what newer versions of PHP themselves do). But either way seems OK to me.

[fluxsauce]

re: PHP Version, let's not change the requirements within the scope of this security work.

[DavidRothstein]

@dsnopek yours looks like it might have a bug to me. Where is the $header variable defined?

[dsnopek]

@DavidRothstein: Eeep! You are right. Too much patching, reverting, and git'ing around. It should be $part not $header.

[dsnopek]

I've updated the patch in my comment above to remove the bug that @DavidRothstein noticed

[mparker17]

@fluxsauce I've pushed another commit to switch to the code in @dsnopek's earlier comment

[dsnopek]

New code looks good! :-) This is missing the addition to the docblock from the Drupal 6.38 patch (also shown in my earlier comment), but I don't know how important that really is.

[pwolanin]

@mparker17 docblock might be good to add and also update the 2nd commit message maybe to refer to the SA for context looking at it later?

[mparker17]

@dsnopek @pwolanin done!

[pwolanin]

Can you rebase those 2 commits together (or we can at merge time)?

[mparker17]

@pwolanin done!

[pwolanin]

Applied the change set from this PR locally.

clean install was fine.

not many automated tests, but those pass 158 passes, 0 fails, and 0 exceptions

[fluxsauce]

Thank you to all who collaborated on this!