Explicitly handle zero-byte NTLM buffer writes

r558579 changed the backing store of NTLM writes from being a
std::basic_string<uint8_t> to a std::vector<uint8_t>, to better align
with its purpose. This change had the effect of causing .data() to now
return nullptr when .size() == 0, rather than the previous behaviour,
in which .data() would return a 1-byte buffer that was a NUL ('\0')
terminator - the consequence of using std::basic_string<>.

This had the effect on writes which expected zero-byte writes (aka
no-ops) to succeed, in that GetBufferPtr() now returned nullptr, rather
than a valid pointer. Since zero-byte memcpys immediately follow, this
needs to be explicitly handled, as memcpy() requires both pointers be
valid, even for zero-byte writes.

BUG=843066

Change-Id: I9ff8f825469142da31abc866c81f0418ae25f93f
Reviewed-on: https://chromium-review.googlesource.com/1059449
Commit-Queue: Ryan Sleevi <[email protected]>
Reviewed-by: Asanka Herath <[email protected]>
Cr-Commit-Position: refs/heads/master@{#558812}

net/ntlm/ntlm_buffer_reader.cc

@@ -54,6 +54,9 @@ bool NtlmBufferReader::ReadBytes(base::span<uint8_t> buffer) {
   if (!CanRead(buffer.size()))
     return false;
 
+  if (buffer.empty())
+    return true;
+
   memcpy(buffer.data(), GetBufferAtCursor(), buffer.size());
 
   AdvanceCursor(buffer.size());
@@ -65,6 +68,9 @@ bool NtlmBufferReader::ReadBytesFrom(const SecurityBuffer& sec_buf,
   if (!CanReadFrom(sec_buf) || buffer.size() < sec_buf.length)
     return false;
 
+  if (buffer.empty())
+    return true;
+
   memcpy(buffer.data(), GetBufferPtr() + sec_buf.offset, sec_buf.length);
 
   return true;

net/ntlm/ntlm_buffer_reader_unittest.cc

@@ -41,6 +41,24 @@ TEST(NtlmBufferReaderTest, EmptyBuffer) {
   ASSERT_TRUE(reader.CanRead(0));
   ASSERT_FALSE(reader.CanRead(1));
   ASSERT_TRUE(reader.IsEndOfBuffer());
+
+  // A read from an empty (zero-byte) source into an empty (zero-byte)
+  // destination buffer should succeed as a no-op.
+  std::vector<uint8_t> dest;
+  ASSERT_TRUE(reader.ReadBytes(dest));
+
+  // A read from a non-empty source into an empty (zero-byte) destination
+  // buffer should succeed as a no-op.
+  std::vector<uint8_t> b2{0x01};
+  NtlmBufferReader reader2(b2);
+  ASSERT_EQ(0u, reader2.GetCursor());
+  ASSERT_EQ(1u, reader2.GetLength());
+
+  ASSERT_TRUE(reader2.CanRead(0));
+  ASSERT_TRUE(reader2.ReadBytes(dest));
+
+  ASSERT_EQ(0u, reader2.GetCursor());
+  ASSERT_EQ(1u, reader2.GetLength());
 }
 
 TEST(NtlmBufferReaderTest, NullBuffer) {
@@ -51,6 +69,11 @@ TEST(NtlmBufferReaderTest, NullBuffer) {
   ASSERT_TRUE(reader.CanRead(0));
   ASSERT_FALSE(reader.CanRead(1));
   ASSERT_TRUE(reader.IsEndOfBuffer());
+
+  // A read from a null source into an empty (zero-byte) destination buffer
+  // should succeed as a no-op.
+  std::vector<uint8_t> dest;
+  ASSERT_TRUE(reader.ReadBytes(dest));
 }
 
 TEST(NtlmBufferReaderTest, Read16) {

net/ntlm/ntlm_buffer_writer.cc

@@ -6,12 +6,12 @@
 
 #include <string.h>
 
+#include <limits>
+
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 
-template class std::basic_string<uint8_t>;
-
 namespace net {
 namespace ntlm {
 
@@ -21,14 +21,14 @@ NtlmBufferWriter::NtlmBufferWriter(size_t buffer_len)
 NtlmBufferWriter::~NtlmBufferWriter() = default;
 
 bool NtlmBufferWriter::CanWrite(size_t len) const {
+  if (len == 0)
+    return true;
+
   if (!GetBufferPtr())
     return false;
 
   DCHECK_LE(GetCursor(), GetLength());
 
-  if (len == 0)
-    return true;
-
   return (len <= GetLength()) && (GetCursor() <= GetLength() - len);
 }
 
@@ -49,6 +49,9 @@ bool NtlmBufferWriter::WriteFlags(NegotiateFlags flags) {
 }
 
 bool NtlmBufferWriter::WriteBytes(base::span<const uint8_t> bytes) {
+  if (bytes.size() == 0)
+    return true;
+
   if (!CanWrite(bytes.size()))
     return false;
 
@@ -58,6 +61,9 @@ bool NtlmBufferWriter::WriteBytes(base::span<const uint8_t> bytes) {
 }
 
 bool NtlmBufferWriter::WriteZeros(size_t count) {
+  if (count == 0)
+    return true;
+
   if (!CanWrite(count))
     return false;
 
@@ -113,7 +119,13 @@ bool NtlmBufferWriter::WriteUtf8AsUtf16String(const std::string& str) {
 }
 
 bool NtlmBufferWriter::WriteUtf16String(const base::string16& str) {
+  if (str.size() > std::numeric_limits<size_t>::max() / 2)
+    return false;
+
   size_t num_bytes = str.size() * 2;
+  if (num_bytes == 0)
+    return true;
+
   if (!CanWrite(num_bytes))
     return false;
 

net/ntlm/ntlm_buffer_writer_unittest.cc

@@ -37,6 +37,42 @@ TEST(NtlmBufferWriterTest, Initialization) {
   ASSERT_FALSE(writer.CanWrite(2));
 }
 
+TEST(NtlmBufferWriterTest, EmptyWrite) {
+  NtlmBufferWriter writer(0);
+
+  ASSERT_EQ(0u, writer.GetLength());
+  ASSERT_EQ(0u, writer.GetBuffer().size());
+  ASSERT_EQ(0u, writer.GetCursor());
+  ASSERT_EQ(nullptr, GetBufferPtr(writer));
+
+  // An empty (zero-byte) write into a zero-byte writer should succeed as a
+  // no-op.
+  std::vector<uint8_t> b;
+  ASSERT_TRUE(writer.CanWrite(0));
+  ASSERT_TRUE(writer.WriteBytes(b));
+
+  ASSERT_EQ(0u, writer.GetLength());
+  ASSERT_EQ(0u, writer.GetBuffer().size());
+  ASSERT_EQ(0u, writer.GetCursor());
+  ASSERT_EQ(nullptr, GetBufferPtr(writer));
+
+  // An empty (zero-byte) write into a non-zero-byte writer should succeed as
+  // a no-op.
+  NtlmBufferWriter writer2(1);
+  ASSERT_EQ(1u, writer2.GetLength());
+  ASSERT_EQ(1u, writer2.GetBuffer().size());
+  ASSERT_EQ(0u, writer2.GetCursor());
+  ASSERT_NE(nullptr, GetBufferPtr(writer2));
+
+  ASSERT_TRUE(writer2.CanWrite(0));
+  ASSERT_TRUE(writer2.WriteBytes(b));
+
+  ASSERT_EQ(1u, writer2.GetLength());
+  ASSERT_EQ(1u, writer2.GetBuffer().size());
+  ASSERT_EQ(0u, writer2.GetCursor());
+  ASSERT_NE(nullptr, GetBufferPtr(writer2));
+}
+
 TEST(NtlmBufferWriterTest, Write16) {
   uint8_t expected[2] = {0x22, 0x11};
   const uint16_t value = 0x1122;