PPB_Flash.Navigate(): Disallow certain HTTP request headers.

chrome/renderer/pepper/pepper_flash_renderer_host.cc

Index: chrome/renderer/pepper/pepper_flash_renderer_host.cc
diff --git a/chrome/renderer/pepper/pepper_flash_renderer_host.cc b/chrome/renderer/pepper/pepper_flash_renderer_host.cc
index e978b6a4e71fc68ce5656ad803d8ea3237c9a63a..5c69b93d6f70d49356a1383c85a40ec844627fa0 100644
--- a/chrome/renderer/pepper/pepper_flash_renderer_host.cc
+++ b/chrome/renderer/pepper/pepper_flash_renderer_host.cc
@@ -6,11 +6,13 @@
 
 #include <vector>
 
+#include "base/strings/string_util.h"
 #include "chrome/renderer/pepper/ppb_pdf_impl.h"
 #include "content/public/renderer/pepper_plugin_instance.h"
 #include "content/public/renderer/render_thread.h"
 #include "content/public/renderer/renderer_ppapi_host.h"
 #include "ipc/ipc_message_macros.h"
+#include "net/http/http_util.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/c/trusted/ppb_browser_font_trusted.h"
 #include "ppapi/host/dispatch_host_message.h"
@@ -33,6 +35,40 @@
 using ppapi::thunk::EnterResourceNoLock;
 using ppapi::thunk::PPB_ImageData_API;
 
+namespace {
+
+// This list is basically the HTTP/1.1 standard headers minus the request
+// headers disallowed by Flash for URLRequestHeader objects.
+// HTTP/1.1 standard headers: Section 4.5, 5.3, 7.1 in
+// http://www.ietf.org/rfc/rfc2616.txt
+// Headers disallowed by Flash for URLRequestHeader objects:
+// http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/net/URLRequestHeader.html
+//
+// There are a few exceptions:
+// - "Authorization" is no longer blocked according to
+//   http://helpx.adobe.com/flash-player/kb/actionscript-error-send-action-contains.html
+// - "Referer" may be set by the Flash player itself.

[abarth-chromium, 2014/01/26 02:10:38]

Don't we want to use the list of CORS simple headers?  Why is it safe to let
Flash set the Pragma header but not safe to let XMLHttpRequest set it?

[yzshen1, 2014/01/26 02:39:03]

I agree that the CORS simple headers list is preferable. However, Flash allows
these headers to be set by Flash contents via ActionScript. If we block them, we
might break some sites.

Besides, here the request is for frame navigation, we don't give the response to
Flash. Maybe that is somewhat acceptable? (I admit I don't know much about the
security implications here.)

What do you think? Thanks!

[abarth-chromium, 2014/01/26 02:44:36]

That's doesn't help with the security problem.  You can't read back a
cross-origin XMLHttpRequest unless the server opts in either.
I don't see any reason why this would be secure.  Without data on how many Flash
sites would break, it's hard to know what the right trade-off is here.  I'd
probably start by using the list of simple headers from CORS and letting this
change flow through the release channels so we can learn about compatibility
problems.  A more disciplined approach would be to combine that with an UMA
histogram to see how we would have allowed a header with this list that is
blocked because we're using the CORS list of simple headers.

[yzshen1, 2014/01/27 23:41:13]

Done. Changed to only allow simple headers && record UMA stats.

+const char* kAllowedHttpRequestHeaders[] = {
+  "accept",
+  "accept-language",
+  "authorization",
+  "cache-control",
+  "content-encoding",
+  "content-language",
+  "content-md5",
+  "content-type",
+  "expires",
+  "from",
+  "if-match",
+  "if-none-match",
+  "if-range",
+  "if-unmodified-since",
+  "pragma",
+  "referer"
+};

[abarth-chromium, 2014/01/26 02:09:31]

Presumably we have this list of headers elsewhere.  Shouldn't we re-use the
existing list instead of replicating it here?

[yzshen1, 2014/01/26 02:39:03]

There is a similar list is in the Flash source code, so we cannot re-use it.
(Moreover, the list there is a blacklist instead of a whitelist.

+
+}  // namespace
+
 PepperFlashRendererHost::PepperFlashRendererHost(
     content::RendererPpapiHost* host,
     PP_Instance instance,
@@ -210,6 +246,20 @@ int32_t PepperFlashRendererHost::OnNavigate(
   if (!plugin_instance)
     return PP_ERROR_FAILED;
 
+  if (allowed_headers_.empty()) {
+    for (size_t i = 0; i < arraysize(kAllowedHttpRequestHeaders); ++i)
+      allowed_headers_.insert(kAllowedHttpRequestHeaders[i]);
+  }
+
+  net::HttpUtil::HeadersIterator header_iter(data.headers.begin(),
+                                             data.headers.end(),
+                                             "\n\r");
+  while (header_iter.GetNext()) {
+    std::string lower_case_header = StringToLowerASCII(header_iter.name());
+    if (allowed_headers_.find(lower_case_header) == allowed_headers_.end())
+      return PP_ERROR_NOACCESS;
+  }
+
   // Navigate may call into Javascript (e.g. with a "javascript:" URL),
   // or do things like navigate away from the page, either one of which will
   // need to re-enter into the plugin. It is safe, because it is essentially