WebSocketExtensionParser: reject top-bit-set characters

net/websockets/websocket_extension_parser.cc

Index: net/websockets/websocket_extension_parser.cc
diff --git a/net/websockets/websocket_extension_parser.cc b/net/websockets/websocket_extension_parser.cc
index b737d5eab6fb9fc18577d3bdc2451fc8a8e77426..98e3f76e7b8c8ac3d89eb58e851c18f4755c10dd 100644
--- a/net/websockets/websocket_extension_parser.cc
+++ b/net/websockets/websocket_extension_parser.cc
@@ -5,9 +5,16 @@
 #include "net/websockets/websocket_extension_parser.h"
 
 #include "base/strings/string_util.h"
+#include "net/http/http_util.h"
 
 namespace net {
 
+namespace {
+bool IsControl(char c) {
+  return (0 <= c && c <= 31) || c == 127;
+}
+}  // namespace
+
 WebSocketExtensionParser::WebSocketExtensionParser() {}
 
 WebSocketExtensionParser::~WebSocketExtensionParser() {}
@@ -19,7 +26,7 @@ bool WebSocketExtensionParser::Parse(const char* data, size_t size) {
 
   bool failed = false;
 
-  while (true) {
+  do {
     WebSocketExtension extension;
     if (!ConsumeExtension(&extension)) {
       failed = true;
@@ -28,11 +35,7 @@ bool WebSocketExtensionParser::Parse(const char* data, size_t size) {
     extensions_.push_back(extension);
 
     ConsumeSpaces();
-
-    if (!ConsumeIfMatch(',')) {
-      break;
-    }
-  }
+  } while (ConsumeIfMatch(','));
 
   if (!failed && current_ == end_)
     return true;
@@ -43,9 +46,8 @@ bool WebSocketExtensionParser::Parse(const char* data, size_t size) {
 
 bool WebSocketExtensionParser::Consume(char c) {
   ConsumeSpaces();
-  if (current_ == end_ || c != current_[0]) {
+  if (current_ == end_ || c != *current_)
     return false;
-  }
   ++current_;
   return true;
 }
@@ -94,12 +96,10 @@ bool WebSocketExtensionParser::ConsumeExtensionParameter(
 bool WebSocketExtensionParser::ConsumeToken(base::StringPiece* token) {
   ConsumeSpaces();
   const char* head = current_;
-  while (current_ < end_ &&
-         !IsControl(current_[0]) && !IsSeparator(current_[0]))
+  while (current_ < end_ && HttpUtil::IsTokenChar(*current_))
     ++current_;
-  if (current_ == head) {
+  if (current_ == head)
     return false;
-  }
   *token = base::StringPiece(head, current_ - head);
   return true;
 }
@@ -109,21 +109,22 @@ bool WebSocketExtensionParser::ConsumeQuotedToken(std::string* token) {
     return false;
 
   *token = "";
-  while (current_ < end_ && !IsControl(current_[0])) {
-    if (UnconsumedBytes() >= 2 && current_[0] == '\\') {
-      char next = current_[1];
-      if (IsControl(next) || IsSeparator(next)) break;
-      *token += next;
-      current_ += 2;
-    } else if (IsSeparator(current_[0])) {
-      break;
+  while (current_ < end_ && !IsControl(*current_)) {

[tyoshino (SeeGerritForStatus), 2016/09/16 04:44:23]

we can remove this IsControl() call?

[Adam Rice, 2016/09/16 05:57:56]

Yes! I didn't notice that during refactoring it had become
unnecessary.

+    if (*current_ == '\\') {

[tyoshino (SeeGerritForStatus), 2016/09/16 04:44:23]

good catch

+      ++current_;
+      if (current_ == end_ || !HttpUtil::IsTokenChar(*current_))
+        return false;
+      *token += *current_;
+      ++current_;
     } else {
-      *token += current_[0];
+      if (!HttpUtil::IsTokenChar(*current_))
+        break;

[tyoshino (SeeGerritForStatus), 2016/09/16 04:44:23]

we can return with false here too?

[Adam Rice, 2016/09/16 05:57:56]

I tried it but it doesn't work. If the character is a '"' we need to fall
through.

This code was quite subtle so I have changed it to be more obvious. PTAL.

[tyoshino (SeeGerritForStatus), 2016/09/16 06:44:23]

Got it!

+      *token += *current_;
       ++current_;
     }
   }
   // We can't use Consume here because we don't want to consume spaces.
-  if (current_ >= end_ || current_[0] != '"')
+  if (current_ >= end_ || *current_ != '"')
     return false;
 
   ++current_;
@@ -132,7 +133,7 @@ bool WebSocketExtensionParser::ConsumeQuotedToken(std::string* token) {
 }
 
 void WebSocketExtensionParser::ConsumeSpaces() {
-  while (current_ < end_ && (current_[0] == ' ' || current_[0] == '\t'))
+  while (current_ < end_ && (*current_ == ' ' || *current_ == '\t'))
     ++current_;
   return;
 }
@@ -154,15 +155,4 @@ bool WebSocketExtensionParser::ConsumeIfMatch(char c) {
   return true;
 }
 
-// static
-bool WebSocketExtensionParser::IsControl(char c) {
-  return (0 <= c && c <= 31) || c == 127;
-}
-
-// static
-bool WebSocketExtensionParser::IsSeparator(char c) {
-  const char separators[] = "()<>@,;:\\\"/[]?={} \t";
-  return strchr(separators, c) != NULL;
-}
-
 }  // namespace net