Add a buffer reader/writer for NTLM.

net/http/ntlm_buffer_reader.h

+// 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.
+
+#ifndef NET_BASE_NTLM_BUFFER_READER_H_
+#define NET_BASE_NTLM_BUFFER_READER_H_
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include <string>
+
+#include "base/strings/string_piece.h"
+#include "net/base/net_export.h"
+#include "net/http/ntlm_message.h"
+
+namespace net {
+
+// Supports various bounds checked low level buffer
+// operations required by an NTLM implementation.
+//
+// The class supports sequential read of a provided
+// buffer. All reads perform bounds checking to ensure
+// enough space is remaining in the buffer.
+//
+//
+// Read* methods read from the buffer at the current cursor
+// position and perform any necessary type conversion and
+// provide the data in out params. After a successful read
+// the cursor position is advanced past the read field.
+//
+// Failed reads leave the internal cursor at the same
+// position as before the call.
+//
+//
+// Read*Payload methods first read a security buffer see
+// |ReadSecurityBuffer| then read the requested payload
+// from the offset and length stated in the security buffer.
+//
+// If the length and offset in the security buffer would
+// cause a read outside the message buffer the payload will
+// not be read and the function will return false.
+//
+// The cursor will remain as it was before the call as if
+// the security buffer had not been read.
+//
+//
+// Skip* methods skip the cursor over that same number
+// of bytes that the equivalent Read method would without
+// reading or returning the values.
+//
+//
+// Match* methods are used to validate fields in the
+// buffer that should have expected values, such as
+// signatures or message identifiers.

[Ryan Sleevi, 2017/05/30 19:02:22]

I can't tell - are these aligned to the maximal use of horizontal space? It
doesn't seem to be, judging by the code below.

[zentaro, 2017/06/05 17:28:44]

Done.

+//
+//
+// Based on [MS-NLMP]: NT LAN Manager (NTLM) Authentication
+// Protocol specification version 28.0 [1]
+//
+// [1] https://msdn.microsoft.com/en-us/library/cc236621.aspx
+class NET_EXPORT NtlmBufferReader {
+ public:
+  NtlmBufferReader(const base::StringPiece buffer);

[Ryan Sleevi, 2017/05/30 19:02:22]

style: you can drop the const since you're passing by-val

[Ryan Sleevi, 2017/05/30 19:02:22]

style: "explicit" -
https://google.github.io/styleguide/cppguide.html#Implicit_Conversions

[zentaro, 2017/06/05 17:28:44]

Done. I previously was passing by const-ref then saw something saying don't do
that for StringPiece and forgot to remove the const.

[zentaro, 2017/06/05 17:28:44]

Done.

+  NtlmBufferReader(const uint8_t* ptr, size_t len);

[Ryan Sleevi, 2017/05/30 19:02:22]

Since you accept a base::StringPiece, is this overload needed?

[zentaro, 2017/06/05 17:28:44]

I could I guess try and convert all the callers to construct a StringPiece. It's
really clunky with all the casting though.

Even on the other overload StringPiece doesn't feel right to use with uint8_t.
Ideally it should be something like a 'span' or 'array_view' though I don't see
anything like that in chrome already?

That would also solve something else I don't really like is that in many places
I just pass naked uint8_t* to buffers with implied length . ie most of the
various fields are all fixed size inputs/hashes and their end destination is
usually to a crypto function which makes the same assumption that it gets a
naked pointer with an assumed min length.

Some of these params could also  std::array though in most cases the naked
pointer would end up being pulled out and sent to a crypto/hash anyway and there
are several cases where it's preferable to pass pointers into a larger array
which would require extra copying to use std::array as a param. Thoughts?

The nice thing about a span/array_view is that you can just pass a std::array or
std::vector to it or have to_span(ptr, len) for the cases where you need to pull
a piece out of another buffer.

+  ~NtlmBufferReader();
+
+  size_t GetLength() const { return buffer_.length(); }
+  size_t GetCursor() const { return cursor_; }
+  bool IsEndOfBuffer() const { return cursor_ >= GetLength(); }
+
+  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.
+  const uint8_t* GetBufferAtCursor() const { return GetBufferPtr() + cursor_; }

[Ryan Sleevi, 2017/05/30 19:02:22]

Both this and the function on line 72 seem like they introduce potentially error
prone possibilities of reading off more than they can chew (that is, because of
the distinct "CanRead" pattern that can be easily omitted)

Did you consider making these variants where the caller must pre-declare the
size of what they're reading/accessing, to force size checks, and omitting the
CanRead/CanReadFrom?

This is different than ReadBytes() [which seems to copy - is that necessary if
you had that here?].

That is, something like "bool GetBuffer(uint8_t** buffer, size_t desired)" or
something?

[zentaro, 2017/06/05 17:28:44]

I managed to refactor out all the public uses of these. I made the unit tests
use a helper that wraps up all the casting over a stringpiece. And in other
cases (in the future CLs) things like backfilling the MIC I just do on the naked
pointer that is being returned anyway.

+
+  // Returns true if there are |len| more bytes between the
+  // current cursor position and the end of the buffer.
+  bool CanRead(size_t len) const;
+
+  // Returns true if there are |len| more bytes between |offset|
+  // and the end of the buffer. The cursor position is not used
+  // or modified.
+  bool CanReadFrom(size_t offset, size_t len) const;
+
+  bool ReadUInt16(uint16_t* value);

[Ryan Sleevi, 2017/05/30 19:02:22]

Document a bit more - for example, what's the endianness being read here (I'm
guessing it's related to the NTLM specification in line 58 - if so, document)

[zentaro, 2017/06/05 17:28:44]

Done.

+  bool ReadUInt32(uint32_t* value);
+  bool ReadUInt64(uint64_t* value);
+
+  bool ReadBytes(uint8_t* buffer, size_t len);
+
+  // 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.
+  //
+  // The structure is (little endian fields):
+  //     uint16 - |length| Length of payload
+  //     uint16 - Allocation (this is always ignored and not returned)
+  //     uint32 - |offset| Offset from start of message
+  bool ReadSecurityBuffer(uint16_t* length, uint32_t* offset);
+
+  bool ReadAsciiString(std::string* value, size_t len);
+  bool ReadUnicodeString(base::string16* value, size_t len);

[Ryan Sleevi, 2017/05/30 19:02:22]

I suspect you're borrowing from the NTLM language, but it would probably help to
be more explicit here that you're talking about a UTF-16 string (or is it a BMP
string?)

+
+  // There are 3 message types Negotiate (sent by client),
+  // Challenge (sent by server), and Authenticate (sent by client).
+  //
+  // This reads the message type from the header and will return
+  // false if the value is invalid.
+  bool ReadMessageType(NtlmMessage::MessageType* message_type);
+
+  bool ReadAsciiPayload(std::string* value);
+  bool ReadUnicodePayload(base::string16* value);
+  bool ReadBytesPayload(uint8_t* buffer, size_t buffer_len);

[Ryan Sleevi, 2017/05/30 19:02:22]

How are the *Payload functions different from the *String / *Bytes functions?

[zentaro, 2017/06/05 17:28:44]

Added some additional docs to clarify. But the 'payload' variants perform a two
step process. First read the security buffer at the cursor (ie offset and
length), then perform ReadString/ReadBytes from the payload (after the all the
fixed position headers).

The cursor never navigates directly through the payload because there is no
context there on how to do it. The security buffer describes how to extract the
that fields bytes from the payload..

+
+  bool SkipSecurityBuffer();
+
+  // Skips over the security buffer without returning the values

[Ryan Sleevi, 2017/05/30 19:02:22]

grammar: "returning the values, " (add a comma)

[zentaro, 2017/06/05 17:28:44]

Done.

+  // but fails if the values would cause a read outside the buffer
+  // if the payload was actually read.
+  bool SkipSecurityBufferWithValidation();
+
+  // Skips over |count| bytes in the buffer. Returns false if there
+  // is not |count| bytes left in the buffer.
+  bool SkipBytes(size_t count);
+
+  // Reads and returns true if the next 8 bytes matches the signature in
+  // an NTLM message "NTLMSSP\0"
+  bool MatchSignature();
+
+  // Reads the message type from the cursor and returns true
+  // if it is |message_type|.
+  bool MatchMessageType(NtlmMessage::MessageType message_type);
+
+  // Performs |MatchSignature| then |MatchMessageType|.
+  bool MatchMessageHeader(NtlmMessage::MessageType message_type);
+
+  // Reads and returns true if there are |count| bytes of zeros.
+  bool MatchZeros(size_t count);
+
+  // Reads and returns true if the next 8 bytes contain an empty
+  // ie. all zero security buffer.
+  bool MatchEmptySecurityBuffer();
+
+ private:
+  // Reads |sizeof(T)| bytes of an integer type from a little-endian
+  // buffer.
+  template <typename T>
+  bool ReadUInt(T* value);
+
+  // Sets the cursor position. This is used when reading payloads to
+  // move the cursor to the start of the payload indicated by the
+  // security buffer.
+  bool SetCursor(size_t cursor);
+
+  uint8_t GetByteAtCursor() const {
+    DCHECK(!IsEndOfBuffer());
+    return *(GetBufferAtCursor());
+  }
+
+  const base::StringPiece buffer_;
+  size_t cursor_;
+
+  DISALLOW_COPY_AND_ASSIGN(NtlmBufferReader);
+};
+
+}  // namespace net
+
+#endif  // NET_BASE_NTLM_BUFFER_READER_H_