Google Chrome/ChromeOS Bug (14508)

https://code.google.com/p/chromium/issues/detail?id=14508

http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_chunked_decoder.cc?r1=18687&r2=18686
   
Root Cause: Signedness error.
   
commit 9d65ad87c64ec57473b42ed290472ddec99e55c6
Author: abarth@chromium.org <abarth@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date:   Thu Jun 18 04:58:34 2009 +0000

    Improve chunked encoding parsing.
   
    R=abarth
    BUG=14508
    TEST=HttpChunkedDecoderTest.ExcessiveChunkLen
   
    Patch contributed by Chris Evans.
   
    git-svn-id: svn://svn.chromium.org/chrome/trunk/src@18687 0039d316-1c4b-4281-b951-d872f2087c98

$ git rev-list –parents -n 1 9d65ad87c64ec57473b42ed290472ddec99e55c6
9d65ad87c64ec57473b42ed290472ddec99e55c6 fc1124064b408b4be6c4302f9b2040d4fe5dbf77

$ git diff fc1124064b408b4be6c4302f9b2040d4fe5dbf77…9d65ad87c64ec57473b42ed290472ddec99e55c6
diff –git a/net/http/http_chunked_decoder.cc b/net/http/http_chunked_decoder.cc
index e030dc0..6ecdcad 100644
— a/net/http/http_chunked_decoder.cc
+++ b/net/http/http_chunked_decoder.cc
@@ -125,7 +125,8 @@ int HttpChunkedDecoder::ScanForChunkRemaining(const char* buf, int buf_len) {
       if (index_of_semicolon != StringPiece::npos)
         buf_len = static_cast<int>(index_of_semicolon);

–      if (!ParseChunkSize(buf, buf_len, &chunk_remaining_)) {
+      if (!ParseChunkSize(buf, buf_len, &chunk_remaining_) ||
+          chunk_remaining_ < 0) {
         DLOG(ERROR) << “Failed parsing HEX from: ” <<
             std::string(buf, buf_len);
         return ERR_INVALID_CHUNKED_ENCODING;
        
        
$ git branch  bug14508
$ git checkout bug14508
$ git reset –hard  fc1124064b408b4be6c4302f9b2040d4fe5dbf77

Here is the buggy code:

int HttpChunkedDecoder::FilterBuf(char* buf, int buf_len) {
  int result = 0;

  while (buf_len) {
    if (chunk_remaining_) {
      int num = std::min(chunk_remaining_, buf_len);

      buf_len -= num;
      chunk_remaining_ -= num;

      result += num;
      buf += num;

      // After each chunk’s data there should be a CRLF
      if (!chunk_remaining_)
        chunk_terminator_remaining_ = true;
      continue;
    } else if (reached_eof_) {
      break;  // Done!
    }

    int bytes_consumed = ScanForChunkRemaining(buf, buf_len);
    if (bytes_consumed < 0)
      return bytes_consumed; // Error

    buf_len -= bytes_consumed;
    if (buf_len)
      memmove(buf, buf + bytes_consumed, buf_len);
  }

  return result;
}

int HttpChunkedDecoder::ScanForChunkRemaining(const char* buf, int buf_len) {
  DCHECK(chunk_remaining_ == 0);
  DCHECK(buf_len > 0);

  int bytes_consumed = 0;

  size_t index_of_lf = StringPiece(buf, buf_len).find(‘\n’);
  if (index_of_lf != StringPiece::npos) {
    buf_len = static_cast<int>(index_of_lf);
    if (buf_len && buf[buf_len – 1] == ‘\r’)  // Eliminate a preceding CR.
      buf_len–;
    bytes_consumed = static_cast<int>(index_of_lf) + 1;

    // Make buf point to the full line buffer to parse.
    if (!line_buf_.empty()) {
      line_buf_.append(buf, buf_len);
      buf = line_buf_.data();
      buf_len = static_cast<int>(line_buf_.size());
    }

    if (reached_last_chunk_) {
      if (buf_len) {
        DLOG(INFO) << “ignoring http trailer”;
      } else {
        reached_eof_ = true;
      }
    } else if (chunk_terminator_remaining_) {
       if (buf_len) {
         DLOG(ERROR) << “chunk data not terminated properly”;
         return ERR_INVALID_CHUNKED_ENCODING;
       }
       chunk_terminator_remaining_ = false;
    } else if (buf_len) {
      // Ignore any chunk-extensions.
      size_t index_of_semicolon = StringPiece(buf, buf_len).find(‘;’);
      if (index_of_semicolon != StringPiece::npos)
        buf_len = static_cast<int>(index_of_semicolon);

      if (!ParseChunkSize(buf, buf_len, &chunk_remaining_)) {
        DLOG(ERROR) << “Failed parsing HEX from: ” <<
            std::string(buf, buf_len);
        return ERR_INVALID_CHUNKED_ENCODING;
      }

      if (chunk_remaining_ == 0)
        reached_last_chunk_ = true;
    } else {
      DLOG(ERROR) << “missing chunk-size”;
      return ERR_INVALID_CHUNKED_ENCODING;
    }
    line_buf_.clear();
  } else {
    // Save the partial line; wait for more data.
    bytes_consumed = buf_len;

    // Ignore a trailing CR
    if (buf[buf_len – 1] == ‘\r’)
      buf_len–;

    line_buf_.append(buf, buf_len);
  }
  return bytes_consumed;
}

This is a common error we see in multiple software. Many passes the data from this HTTP field into atoi() and use that. Actually atoi() can return a negative value.

Lesson Learned (Useful comment from the bug report):
    Generally, the network layer operates with a lot of “signed” ints for lengths.
    “unsigned ints” can be a lot better as then you don’t have to deal with the concept
    of “negative”.

    However, it’s challenging, because once you change one length to be unsigned, you
    have to change all the related variables which it is compared with, added to, etc.
    (You don’t want to mix and match signed / unsigned!)

    One added complication is some areas of the network stack use the value of “-1” to
    mean “not specified”, such as for example “Content-Length”. In that instance, you
    couldn’t change the value to be unsigned without also modifying surrounding logic.

Advertisements
This entry was posted in Chrome, chrome OS, Google and tagged , , . Bookmark the permalink.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s