Fix security bug that allowed invalid header fields to be injected by

webkit/plugins/ppapi/ppb_url_request_info_impl.cc

Index: webkit/plugins/ppapi/ppb_url_request_info_impl.cc
===================================================================
--- webkit/plugins/ppapi/ppb_url_request_info_impl.cc	(revision 96431)
+++ webkit/plugins/ppapi/ppb_url_request_info_impl.cc	(working copy)
@@ -44,6 +44,69 @@
 const int32_t kDefaultPrefetchBufferUpperThreshold = 100 * 1000 * 1000;
 const int32_t kDefaultPrefetchBufferLowerThreshold = 50 * 1000 * 1000;
 
+bool IsValidToken(const std::string& token) {
+  size_t length = token.size();
+  if (length == 0)
+    return false;
+
+  for (size_t i = 0; i < length; i++) {
+    char c = token[i];
+    if (c >= 127 || c <= 32)
+      return false;
+    if (c == '(' || c == ')' || c == '<' || c == '>' || c == '@' ||
+        c == ',' || c == ';' || c == ':' || c == '\\' || c == '\"' ||
+        c == '/' || c == '[' || c == ']' || c == '?' || c == '=' ||
+        c == '{' || c == '}')
+      return false;
+  }
+  return true;
+}
+
+// These methods are not allowed by the XMLHttpRequest standard.
+// http://www.w3.org/TR/XMLHttpRequest/#the-open-method
+const char* const kForbiddenHttpMethods[] = {
+  "connect",
+  "trace",
+  "track",
+};
+
+// These methods are listed in the XMLHttpRequest standard. They should be
+// converted to upper-case.

[bbudge, 2011/08/13 00:00:41]

This isn't quite true; this list comes from Webkit code, which states that it is
following Firefox's precedent of upper-casing these.

[brettw, 2011/08/14 18:30:42]

Can you expand on "they should be converted to upper-case when...". My first
thought was "if they should be converted to upper case, why don't you just do it
and make the constants upper case?"

[bbudge, 2011/08/15 15:03:53]

I'll improve the comment. I would prefer to make the constants upper case but
didn't see any suitable compare method. Are there case insensitive
(locale-invariant) string compares in string_util?

[brettw, 2011/08/15 15:54:56]

No, I think lower-case is fine for the constants.

+// http://www.w3.org/TR/XMLHttpRequest/#the-open-method
+const char* const kKnownHttpMethods[] = {
+  "get",
+  "post",
+  "put",
+  "head",
+  "copy",
+  "delete",
+  "index",
+  "lock",
+  "m-post",
+  "mkcol",
+  "move",
+  "options",
+  "propfind",
+  "proppatch",
+  "unlock",
+};
+
+std::string ValidateMethod(const std::string& method) {
+  for (size_t i = 0; i < arraysize(kForbiddenHttpMethods); ++i) {
+    if (LowerCaseEqualsASCII(method, kForbiddenHttpMethods[i]))
+      return std::string();
+  }
+  for (size_t i = 0; i < arraysize(kKnownHttpMethods); ++i) {
+    if (LowerCaseEqualsASCII(method, kKnownHttpMethods[i])) {
+      std::string method_upper(kKnownHttpMethods[i]);
+      StringToUpperASCII(method_upper);
+      return method_upper;
+    }
+  }
+  // Pass through unknown methods that are not forbidden.
+  return method;
+}
+
 // A header string containing any of the following fields will cause
 // an error. The list comes from the XMLHttpRequest standard.
 // http://www.w3.org/TR/XMLHttpRequest/#the-setrequestheader-method
@@ -329,8 +392,10 @@
       url_ = value;  // NOTE: This may be a relative URL.
       return true;
     case PP_URLREQUESTPROPERTY_METHOD:
-      method_ = value;
-      return true;
+      if (!IsValidToken(value))
+        return false;
+      method_ = ValidateMethod(value);
+      return !method_.empty();
     case PP_URLREQUESTPROPERTY_HEADERS:
       if (!AreValidHeaders(value))

[brettw, 2011/08/14 18:30:42]

Is there a similar attack if one of the other headers contains a newline?

[bbudge, 2011/08/15 15:03:53]

We use HttpUtil::HeadersIterator to parse the header name-value pairs. It looks
pretty robust to me.

         return false;