From 25c9bf57ef9c10e9dd33eec686d11adf959afc98 Mon Sep 17 00:00:00 2001
From: Brett Simmers <bsimmers@fb.com>
Date: Mon, 14 Oct 2013 16:15:23 -0700
Subject: [PATCH] Clean up http header initialization code

This diff fixes a few things I ran into while debugging an unrelated
issue:
- The request counter was global and not synchronized. It's not critical for
  there to be no races on this counter but it's not a perf-sensitive path so I
  can't see any reason to not fix it.
- The HeaderMangle warning is now more informative, including the entire set of
  received headers. It also prints at most one warning per request, though that
  warning may have multiple lines.
- The code to actually put the headers in $_SERVER['HTTP_*'] was looping over
  the vector of values for each header, assigning each one to the same key in
  $_SERVER. Unless I'm missing something really subtle, this is equivalent to
  just assigning the final element of the vector to the key, so I changed it to
  do that.

Reviewed By: @markw65

Differential Revision: D1010458
---
 hphp/runtime/server/http-protocol.cpp | 73 +++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 29 deletions(-)

diff --git a/hphp/runtime/server/http-protocol.cpp b/hphp/runtime/server/http-protocol.cpp
index d0e3671c67a..36b1fa64c1a 100644
--- a/hphp/runtime/server/http-protocol.cpp
+++ b/hphp/runtime/server/http-protocol.cpp
@@ -253,38 +253,53 @@ void HttpProtocol::PrepareSystemVariables(Transport *transport,
   HeaderMap headers;
   transport->getHeaders(headers);
 
-  static int bad_request_count = -1;
-  for (HeaderMap::const_iterator iter = headers.begin();
-       iter != headers.end(); ++iter) {
-    const vector<string> &values = iter->second;
-
-    // Detect suspicious headers.  We are about to modify header names
-    // for the SERVER variable.  This means that it is possible to
-    // deliberately cause a header collision, which an attacker could
-    // use to sneak a header past a proxy that would either overwrite
-    // or filter it otherwise.  Client code should use
-    // apache_request_headers() to retrieve the original headers if
-    // they are security-critical.
-    if (RuntimeOption::LogHeaderMangle != 0) {
-      String key = "HTTP_";
-      key += f_strtoupper(iter->first).replace("-","_");
-      if (server.asArrRef().exists(key)) {
-        if (!(++bad_request_count % RuntimeOption::LogHeaderMangle)) {
-          Logger::Warning(
-            "HeaderMangle warning: "
-            "The header %s overwrote another header which mapped to the same "
-            "key. This happens because PHP normalises - to _, ie AN_EXAMPLE "
-            "and AN-EXAMPLE are equivalent.  You should treat this as "
-            "malicious.",
-            iter->first.c_str());
+  static std::atomic<int> badRequests(-1);
+
+  std::vector<std::string> badHeaders;
+  for (auto const& header : headers) {
+    auto const& key = header.first;
+    auto const& values = header.second;
+    auto normalizedKey = String("HTTP_") + f_strtoupper(key).replace("-", "_");
+
+    // Detect suspicious headers.  We are about to modify header names for
+    // the SERVER variable.  This means that it is possible to deliberately
+    // cause a header collision, which an attacker could use to sneak a
+    // header past a proxy that would either overwrite or filter it
+    // otherwise.  Client code should use apache_request_headers() to
+    // retrieve the original headers if they are security-critical.
+    if (RuntimeOption::LogHeaderMangle != 0 &&
+        server.asArrRef().exists(normalizedKey)) {
+      badHeaders.push_back(key);
+    }
+
+    if (!values.empty()) {
+      // When a header has multiple values, we always take the last one.
+      server.set(normalizedKey, String(values.back()));
+    }
+  }
+
+  if (!badHeaders.empty()) {
+    auto reqId = badRequests.fetch_add(1, std::memory_order_acq_rel) + 1;
+    if (!(reqId % RuntimeOption::LogHeaderMangle)) {
+      std::string badNames = folly::join(", ", badHeaders);
+      std::string allHeaders;
+
+      const char* separator = "";
+      for (auto const& header : headers) {
+        for (auto const& value : header.second) {
+          folly::toAppend(separator, header.first, ": ", value,
+                          &allHeaders);
+          separator = "\n";
         }
       }
-    }
 
-    for (unsigned int i = 0; i < values.size(); i++) {
-      String key = "HTTP_";
-      key += f_strtoupper(iter->first).replace("-", "_");
-      server.set(key, String(values[i]));
+      Logger::Warning(
+        "HeaderMangle warning: "
+        "The header(s) [%s] overwrote other headers which mapped to the same "
+        "key. This happens because PHP normalises - to _, ie AN_EXAMPLE "
+        "and AN-EXAMPLE are equivalent.  You should treat this as "
+        "malicious. All headers from this request:\n%s",
+        badNames.c_str(), allHeaders.c_str());
     }
   }
 
-- 
2.11.4.GIT