Don't copy NtlmBufferReader buffer input.

 - Add a method for reading a payload as another NtlmBufferReader
   with a subset range.

BUG=chromium:22532

Change-Id: I6f2f16f88b864fefbc46d93f2eab5aa780ec7506
Reviewed-on: https://chromium-review.googlesource.com/656137
Commit-Queue: Zentaro Kavanagh <zentaro@google.com>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505184}

net/ntlm/ntlm_buffer_reader.cc

@@ -11,17 +11,19 @@
 namespace net {
 namespace ntlm {
 
+NtlmBufferReader::NtlmBufferReader() : NtlmBufferReader(nullptr, 0) {}
+
 NtlmBufferReader::NtlmBufferReader(const Buffer& buffer)
-    : buffer_(buffer), cursor_(0) {
-  DCHECK(buffer.data());
-}
+    : NtlmBufferReader(
+          base::StringPiece(reinterpret_cast<const char*>(buffer.data()),
+                            buffer.length())) {}
 
 NtlmBufferReader::NtlmBufferReader(base::StringPiece str)
-    : NtlmBufferReader(reinterpret_cast<const uint8_t*>(str.data()),
-                       str.size()) {}
+    : buffer_(str), cursor_(0) {}
 
 NtlmBufferReader::NtlmBufferReader(const uint8_t* ptr, size_t len)
-    : NtlmBufferReader(Buffer(ptr, len)) {}
+    : NtlmBufferReader(
+          base::StringPiece(reinterpret_cast<const char*>(ptr), len)) {}
 
 NtlmBufferReader::~NtlmBufferReader() {}
 
@@ -80,6 +82,15 @@ bool NtlmBufferReader::ReadBytesFrom(const SecurityBuffer& sec_buf,
   return true;
 }
 
+bool NtlmBufferReader::ReadPayloadAsBufferReader(const SecurityBuffer& sec_buf,
+                                                 NtlmBufferReader* reader) {
+  if (!CanReadFrom(sec_buf))
+    return false;
+
+  *reader = NtlmBufferReader(GetBufferPtr() + sec_buf.offset, sec_buf.length);
+  return true;
+}
+
 bool NtlmBufferReader::ReadSecurityBuffer(SecurityBuffer* sec_buf) {
   return ReadUInt16(&sec_buf->length) && SkipBytes(sizeof(uint16_t)) &&
          ReadUInt32(&sec_buf->offset);
@@ -107,7 +118,11 @@ bool NtlmBufferReader::ReadTargetInfo(size_t target_info_len,
                                       std::vector<ntlm::AvPair>* av_pairs) {
   DCHECK(av_pairs->empty());
 
-  // There has to be at least one terminating header.
+  // A completely empty target info is allowed.
+  if (target_info_len == 0)
+    return true;
+
+  // If there is any content there has to be at least one terminating header.
   if (!CanRead(target_info_len) || target_info_len < ntlm::kAvPairHeaderLen) {
     return false;
   }
@@ -186,24 +201,15 @@ bool NtlmBufferReader::ReadTargetInfoPayload(
   if (!ReadSecurityBuffer(&sec_buf))
     return false;
 
-  // If the security buffer has zero length, an empty av_pairs will be the
-  // result.
-  if (sec_buf.length == 0)
-    return true;
-
-  // If there is a non-zero length payload there has to be at least one
-  // terminating header.
-  if (!CanReadFrom(sec_buf.offset, sec_buf.length) ||
-      sec_buf.length < ntlm::kAvPairHeaderLen)
+  NtlmBufferReader payload_reader;
+  if (!ReadPayloadAsBufferReader(sec_buf, &payload_reader))
     return false;
 
-  size_t old_cursor = GetCursor();
-  SetCursor(sec_buf.offset);
-  if (!ReadTargetInfo(sec_buf.length, av_pairs))
+  if (!payload_reader.ReadTargetInfo(sec_buf.length, av_pairs))
     return false;
 
-  SetCursor(old_cursor);
-  return true;
+  // |ReadTargetInfo| should have consumed the entire contents.
+  return payload_reader.IsEndOfBuffer();
 }
 
 bool NtlmBufferReader::ReadMessageType(MessageType* message_type) {

net/ntlm/ntlm_buffer_reader.h

@@ -47,6 +47,8 @@ namespace ntlm {
 // [2] http://davenport.sourceforge.net/ntlm.html
 class NET_EXPORT_PRIVATE NtlmBufferReader {
  public:
+  NtlmBufferReader();
+  // |buffer| is not copied and must outlive the |NtlmBufferReader|.
   explicit NtlmBufferReader(const Buffer& buffer);
   explicit NtlmBufferReader(base::StringPiece buffer);
 
@@ -99,6 +101,14 @@ class NET_EXPORT_PRIVATE NtlmBufferReader {
   bool ReadBytesFrom(const SecurityBuffer& sec_buf,
                      uint8_t* buffer) WARN_UNUSED_RESULT;
 
+  // Reads |sec_buf.length| bytes from offset |sec_buf.offset| and assigns
+  // |reader| an |NtlmBufferReader| representing the payload. If the security
+  //  buffer specifies a payload outside the buffer, then the call fails, and
+  // the state of |reader| is undefined. Unlike the other Read* methods, this
+  // does not move the cursor.
+  bool ReadPayloadAsBufferReader(const SecurityBuffer& sec_buf,
+                                 NtlmBufferReader* reader) WARN_UNUSED_RESULT;
+
   // A security buffer is an 8 byte structure that defines the offset and
   // length of a payload (string, struct or byte array) that appears after the
   // fixed part of the message.
@@ -198,7 +208,9 @@ class NET_EXPORT_PRIVATE NtlmBufferReader {
   void AdvanceCursor(size_t count) { SetCursor(GetCursor() + count); }
 
   // Returns a constant pointer to the start of the buffer.
-  const uint8_t* GetBufferPtr() const { return buffer_.data(); }
+  const uint8_t* GetBufferPtr() const {
+    return reinterpret_cast<const uint8_t*>(buffer_.data());
+  }
 
   // Returns a pointer to the underlying buffer at the current cursor
   // position.
@@ -210,10 +222,8 @@ class NET_EXPORT_PRIVATE NtlmBufferReader {
     return *(GetBufferAtCursor());
   }
 
-  const Buffer buffer_;
+  base::StringPiece buffer_;
   size_t cursor_;
-
-  DISALLOW_COPY_AND_ASSIGN(NtlmBufferReader);
 };
 
 }  // namespace ntlm

net/ntlm/ntlm_buffer_reader_unittest.cc

@@ -127,6 +127,51 @@ TEST(NtlmBufferReaderTest, ReadSecurityBufferPastEob) {
   ASSERT_FALSE(reader.ReadSecurityBuffer(&sec_buf));
 }
 
+TEST(NtlmBufferReaderTest, ReadPayloadAsBufferReader) {
+  const uint8_t buf[8] = {0xff, 0xff, 0x11, 0x22, 0x33, 0x44, 0xff, 0xff};
+  const uint32_t expected = 0x44332211;
+  NtlmBufferReader reader(buf, arraysize(buf));
+  ASSERT_EQ(0u, reader.GetCursor());
+
+  // Create a security buffer with offset 2 and length 4.
+  SecurityBuffer sec_buf(2, 4);
+  NtlmBufferReader sub_reader;
+  ASSERT_EQ(0u, sub_reader.GetLength());
+  ASSERT_EQ(0u, sub_reader.GetCursor());
+
+  // Read the 4 non-0xff bytes from the middle of |buf|.
+  ASSERT_TRUE(reader.ReadPayloadAsBufferReader(sec_buf, &sub_reader));
+
+  // |reader| cursor should not move.
+  ASSERT_EQ(0u, reader.GetCursor());
+  ASSERT_EQ(sec_buf.length, sub_reader.GetLength());
+  ASSERT_EQ(0u, sub_reader.GetCursor());
+
+  // Read from the payload in |sub_reader|.
+  uint32_t actual;
+  ASSERT_TRUE(sub_reader.ReadUInt32(&actual));
+  ASSERT_EQ(expected, actual);
+  ASSERT_TRUE(sub_reader.IsEndOfBuffer());
+}
+
+TEST(NtlmBufferReaderTest, ReadPayloadBadOffset) {
+  const uint8_t buf[4] = {0};
+  NtlmBufferReader reader(buf, arraysize(buf));
+
+  NtlmBufferReader sub_reader;
+  ASSERT_FALSE(
+      reader.ReadPayloadAsBufferReader(SecurityBuffer(4, 1), &sub_reader));
+}
+
+TEST(NtlmBufferReaderTest, ReadPayloadBadLength) {
+  const uint8_t buf[4] = {0};
+  NtlmBufferReader reader(buf, arraysize(buf));
+
+  NtlmBufferReader sub_reader;
+  ASSERT_FALSE(
+      reader.ReadPayloadAsBufferReader(SecurityBuffer(3, 2), &sub_reader));
+}
+
 TEST(NtlmBufferReaderTest, SkipSecurityBuffer) {
   const uint8_t buf[kSecurityBufferLen] = {0};
 
@@ -283,6 +328,15 @@ TEST(NtlmBufferReaderTest, ReadTargetInfoEolOnly) {
   ASSERT_TRUE(av_pairs.empty());
 }
 
+TEST(NtlmBufferReaderTest, ReadTargetInfoEmpty) {
+  NtlmBufferReader reader;
+
+  std::vector<ntlm::AvPair> av_pairs;
+  ASSERT_TRUE(reader.ReadTargetInfo(0, &av_pairs));
+  ASSERT_TRUE(reader.IsEndOfBuffer());
+  ASSERT_TRUE(av_pairs.empty());
+}
+
 TEST(NtlmBufferReaderTest, ReadTargetInfoTimestampAndEolOnly) {
   // Buffer contains a timestamp av pair and an EOL terminator.
   const uint8_t buf[16] = {0x07, 0,    0x08, 0,    0x11, 0x22, 0x33, 0x44,