Add a buffer reader/writer for NTLM.

net/http/ntlm_buffer_reader.cc

+// Copyright (c) 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/http/ntlm_buffer_reader.h"
+
+#include <string.h>
+
+#include "base/logging.h"
+
+namespace net {
+
+#if defined(ARCH_CPU_LITTLE_ENDIAN)
+#define IS_LITTLE_ENDIAN 1
+#undef IS_BIG_ENDIAN
+#elif defined(ARCH_CPU_BIG_ENDIAN)
+#define IS_BIG_ENDIAN 1
+#undef IS_LITTLE_ENDIAN
+#else
+#error "Unknown endianness"
+#endif
+
+NtlmBufferReader::NtlmBufferReader(base::StringPiece buffer)
+    : buffer_(buffer), cursor_(0) {
+  DCHECK(buffer.data() != nullptr);

[Ryan Sleevi, 2017/06/08 18:36:33]

DCHECK(buffer.data()) ?

[zentaro, 2017/06/12 23:12:05]

Done.

+}
+
+NtlmBufferReader::NtlmBufferReader(const uint8_t* ptr, size_t len)
+    : NtlmBufferReader(
+          base::StringPiece(reinterpret_cast<const char*>(ptr), len)) {}
+
+NtlmBufferReader::~NtlmBufferReader() {}
+
+bool NtlmBufferReader::CanRead(size_t len) const {
+  return CanReadFrom(cursor_, len);
+}
+
+bool NtlmBufferReader::CanReadFrom(size_t offset, size_t len) const {
+  if (len == 0)
+    return true;
+
+  return (offset + len > offset) && (offset + len <= GetLength());

[Ryan Sleevi, 2017/06/08 18:36:33]

This usually triggers a pattern matching warning (you're checking for
wrap-around, right?)

return (len <= GetLength() && offset <= GetLength() - len)

Same thing, right?

[zentaro, 2017/06/12 23:12:05]

Done.

+}
+
+bool NtlmBufferReader::SetCursor(size_t cursor) {
+  if (cursor > GetLength())
+    return false;
+
+  cursor_ = cursor;
+  return true;
+}
+
+bool NtlmBufferReader::ReadUInt16(uint16_t* value) {
+  return ReadUInt<uint16_t>(value);
+}
+
+bool NtlmBufferReader::ReadUInt32(uint32_t* value) {
+  return ReadUInt<uint32_t>(value);
+}
+
+bool NtlmBufferReader::ReadUInt64(uint64_t* value) {
+  return ReadUInt<uint64_t>(value);
+}
+
+template <typename T>
+bool NtlmBufferReader::ReadUInt(T* value) {
+  size_t int_size = sizeof(T);
+  if (!CanRead(int_size))
+    return false;
+
+  *value = 0;
+  for (size_t i = 0; i < int_size; i++) {
+    *value += static_cast<T>(GetByteAtCursor()) << (i * 8);
+    cursor_++;
+  }
+
+  return true;
+}
+
+bool NtlmBufferReader::ReadBytes(uint8_t* buffer, size_t len) {
+  if (!CanRead(len))
+    return false;
+
+  memcpy(reinterpret_cast<void*>(buffer),
+         reinterpret_cast<const void*>(GetBufferAtCursor()), len);
+
+  cursor_ += len;
+  return true;
+}
+
+bool NtlmBufferReader::ReadSecurityBuffer(ntlm::SecurityBuffer* sec_buf) {
+  if (!CanRead(ntlm::SECURITY_BUFFER_LEN))
+    return false;
+
+  if (ReadUInt16(&sec_buf->length) && SkipBytes(sizeof(uint16_t)) &&

[Ryan Sleevi, 2017/06/08 18:36:33]

heh, I'm torn on whether sizeof(uint16_t) is really better than just 2 ;)

[asanka, 2017/06/08 18:44:58]

nit: if (X) return true; return false;  ---> return X;

[zentaro, 2017/06/12 23:12:05]

Seemed more natural here because it makes it more obvious that this is
essentially SkipUInt16(). I don't feel really strongly about it!

+      ReadUInt32(&sec_buf->offset))
+    return true;
+
+  NOTREACHED();
+  return false;
+}
+
+bool NtlmBufferReader::ReadAsciiString(std::string* value, size_t num_bytes) {
+  if (!CanRead(num_bytes))
+    return false;
+
+  value->assign(reinterpret_cast<const char*>(GetBufferAtCursor()), num_bytes);
+  cursor_ += num_bytes;
+  return true;
+}
+
+bool NtlmBufferReader::ReadUnicodeString(base::string16* value,
+                                         size_t num_bytes) {
+  if (!CanRead(num_bytes) || (num_bytes % 2 != 0))
+    return false;
+
+  value->assign(reinterpret_cast<const base::char16*>(GetBufferAtCursor()),
+                num_bytes / 2);
+#if IS_BIG_ENDIAN

[asanka, 2017/06/08 18:44:58]

Do we build and test on BIG_ENDIAN?

[zentaro, 2017/06/12 23:12:05]

No idea! If the rest of chrome doesn't support it, I'd love to rip these code
paths out and hard fail at compile time.

+  // The NTLM buffer is always little-endian. So when it was assigned to the
+  // string above all the chars will be in little-endian order.
+  // Now swap them all so that the resulting string |value| is big-endian.
+  uint8_t* ptr = reinterpret_cast<uint8_t*>(value->c_str());
+  for (int i = 0; i < num_bytes; i += 2) {
+    std::swap(ptr[i], ptr[i + 1]);
+  }
+#endif
+  cursor_ += num_bytes;
+  return true;
+}
+
+bool NtlmBufferReader::ReadUnicodePayload(base::string16* value) {
+  // First read the security buffer.
+  ntlm::SecurityBuffer sec_buf;
+  if (!ReadSecurityBuffer(&sec_buf))
+    return false;
+
+  if (!CanReadFrom(sec_buf)) {
+    cursor_ -= ntlm::SECURITY_BUFFER_LEN;

[asanka, 2017/06/08 18:44:58]

nit: store and restore the original cursor instead of undoing the cursor
movement that ReadSecurityBuffer() supposedly did.

[zentaro, 2017/06/12 23:12:05]

Done.

+    return false;
+  }
+
+  size_t old_cursor = GetCursor();
+
+  bool result = SetCursor(sec_buf.offset) &&
+                ReadUnicodeString(value, sec_buf.length) &&
+                SetCursor(old_cursor);
+
+  DCHECK(result);
+  return result;
+}
+
+bool NtlmBufferReader::ReadBytesPayload(uint8_t* buffer, size_t buffer_len) {
+  // First read the security buffer.
+  ntlm::SecurityBuffer sec_buf;
+  if (!ReadSecurityBuffer(&sec_buf))
+    return false;
+
+  if (!CanReadFrom(sec_buf) || sec_buf.length > buffer_len) {
+    cursor_ -= ntlm::SECURITY_BUFFER_LEN;
+    return false;
+  }
+
+  size_t old_cursor = GetCursor();
+  bool result = SetCursor(sec_buf.offset) &&
+                ReadBytes(buffer, sec_buf.length) && SetCursor(old_cursor);

[Ryan Sleevi, 2017/06/08 18:36:33]

Note: if ReadBytes() fails, SetCursor(old_cursor) won't happen, and the
invariant in the header won't be correct.

But from reading this, I'm not sure the documentation *is* correct, since it
says:

" 159   // In the case of failure the cursor will remain as if it never read the
 160   // security buffer. In the case of success the cursor will point after
the
 161   // security buffer."

But that's not the case - in the case of success, the cursor points to the
current location, in the case of failure, the cursor state is undefined (since
it could be old - if SetCursor failed, new - if ReadBytes failed)

[zentaro, 2017/06/12 23:12:05]

Set cursor should never fail. Because we already checked that in the if
statement above (CanReadFrom would have failed). We DCHECK the final result just
below too.

However I realized it was redundant in every case to return a bool from
SetCursor and modified it so that it internally DCHECKs and returns nothing.

+  DCHECK(result);
+  return result;
+}
+
+bool NtlmBufferReader::ReadAsciiPayload(std::string* value) {
+  // First read the security buffer.
+  ntlm::SecurityBuffer sec_buf;
+  if (!ReadSecurityBuffer(&sec_buf))
+    return false;
+
+  if (!CanReadFrom(sec_buf)) {
+    cursor_ -= ntlm::SECURITY_BUFFER_LEN;
+    return false;
+  }
+
+  size_t old_cursor = GetCursor();
+  bool result = SetCursor(sec_buf.offset) &&
+                ReadAsciiString(value, sec_buf.length) && SetCursor(old_cursor);
+
+  DCHECK(result);
+  return result;
+}
+
+bool NtlmBufferReader::ReadMessageType(ntlm::MessageType* message_type) {
+  uint32_t temp;

[asanka, 2017/06/08 18:44:58]

everything is temporary :-) . This is the raw message value.

[zentaro, 2017/06/12 23:12:05]

Done.

+  if (!ReadUInt32(&temp))
+    return false;
+
+  if (temp != ntlm::MESSAGE_NEGOTIATE && temp != ntlm::MESSAGE_CHALLENGE &&
+      temp != ntlm::MESSAGE_AUTHENTICATE) {
+    cursor_ -= sizeof(uint32_t);
+    return false;
+  }
+
+  *message_type = static_cast<ntlm::MessageType>(temp);
+  return true;
+}
+
+bool NtlmBufferReader::SkipSecurityBuffer() {
+  return SkipBytes(ntlm::SECURITY_BUFFER_LEN);
+}
+
+bool NtlmBufferReader::SkipSecurityBufferWithValidation() {
+  ntlm::SecurityBuffer sec_buf;
+  if (!ReadSecurityBuffer(&sec_buf)) {
+    return false;
+  }
+
+  // If the security buffer indicates to read outside
+  // the message buffer then fail.
+  if (!CanReadFrom(sec_buf)) {
+    cursor_ -= ntlm::SECURITY_BUFFER_LEN;
+    return false;
+  }
+
+  return true;
+}
+
+bool NtlmBufferReader::SkipBytes(size_t count) {
+  if (!CanRead(count))
+    return false;
+
+  cursor_ += count;
+  return true;
+}
+
+bool NtlmBufferReader::MatchSignature() {
+  if (!CanRead(ntlm::SIGNATURE_LEN))
+    return false;
+
+  if (memcmp(ntlm::SIGNATURE, GetBufferAtCursor(), ntlm::SIGNATURE_LEN) != 0)
+    return false;
+
+  cursor_ += ntlm::SIGNATURE_LEN;
+  return true;
+}
+
+bool NtlmBufferReader::MatchMessageType(ntlm::MessageType message_type) {
+  size_t old_cursor = cursor_;
+  ntlm::MessageType actual_message_type;
+  if (!ReadMessageType(&actual_message_type))
+    return false;
+
+  if (actual_message_type != message_type) {
+    cursor_ = old_cursor;
+    return false;
+  }
+
+  return true;
+}
+
+bool NtlmBufferReader::MatchMessageHeader(ntlm::MessageType message_type) {
+  size_t old_cursor = cursor_;

[asanka, 2017/06/08 18:44:58]

Nit: let's be consistent about GetCursor()/SetCursor() vs. accessing cursor_
directly. I'm not opposed to getting rid of GetCursor()/SetCursor() if no
outside consumer needs it.

[zentaro, 2017/06/12 23:12:05]

Yep. SetCursor() it is. It was added in a refactor so I guess I missed a few
spots. Also added an AdvanceCursor for the other cases where cursor_ is being
moved directly.

Considered adding a tiny helper class eg. CursorRestorer() that would
automatically restore when it went out of scope unless told otherwise. But
seemed maybe overkill?

+  if (MatchSignature() && MatchMessageType(message_type))
+    return true;
+
+  cursor_ = old_cursor;
+  return false;
+}
+
+bool NtlmBufferReader::MatchZeros(size_t count) {
+  if (!CanRead(count))
+    return false;
+
+  for (size_t i = 0; i < count; i++) {
+    if (GetBufferAtCursor()[i] != 0)
+      return false;
+  }
+
+  cursor_ += count;
+  return true;
+}
+
+bool NtlmBufferReader::MatchEmptySecurityBuffer() {
+  return MatchZeros(ntlm::SECURITY_BUFFER_LEN);
+}
+
+}  // namespace net