Replace NTLMv1 implementation with a functionally equivalent one.

net/ntlm/ntlm_client.cc

+// Copyright 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/ntlm/ntlm_client.h"
+
+#include <string.h>
+
+#include "base/logging.h"
+#include "base/strings/utf_string_conversions.h"
+#include "net/ntlm/ntlm.h"
+#include "net/ntlm/ntlm_buffer_reader.h"
+#include "net/ntlm/ntlm_buffer_writer.h"
+
+namespace net {
+namespace ntlm {
+
+namespace {
+// Parses the challenge message and returns the |challenge_flags| and writes
+// the |server_challenge| into the supplied buffer.
+// |server_challenge| must contain at least 8 bytes.
+static bool ParseChallengeMessage(const uint8_t* challenge_message,
+                                  size_t challenge_message_len,
+                                  NegotiateFlags* challenge_flags,
+                                  uint8_t* server_challenge) {
+  NtlmBufferReader challenge_reader(challenge_message, challenge_message_len);
+
+  return challenge_reader.MatchMessageHeader(MessageType::CHALLENGE) &&
+         challenge_reader.SkipSecurityBufferWithValidation() &&
+         challenge_reader.ReadFlags(challenge_flags) &&
+         challenge_reader.ReadBytes(server_challenge, CHALLENGE_LEN);
+}
+
+static bool WriteAuthenticateMessage(NtlmBufferWriter* authenticate_writer,
+                                     SecurityBuffer lm_payload,
+                                     SecurityBuffer ntlm_payload,
+                                     SecurityBuffer domain_payload,
+                                     SecurityBuffer username_payload,
+                                     SecurityBuffer hostname_payload,
+                                     NegotiateFlags authenticate_flags) {
+  return authenticate_writer->WriteMessageHeader(MessageType::AUTHENTICATE) &&
+         authenticate_writer->WriteSecurityBuffer(lm_payload) &&
+         authenticate_writer->WriteSecurityBuffer(ntlm_payload) &&
+         authenticate_writer->WriteSecurityBuffer(domain_payload) &&
+         authenticate_writer->WriteSecurityBuffer(username_payload) &&
+         authenticate_writer->WriteSecurityBuffer(hostname_payload) &&
+         authenticate_writer->WriteSecurityBuffer(
+             SecurityBuffer(AUTHENTICATE_HEADER_V1_LEN, 0)) &&
+         authenticate_writer->WriteFlags(authenticate_flags);
+}
+
+static bool WriteResponsePayloads(NtlmBufferWriter* authenticate_writer,
+                                  const uint8_t* lm_response,
+                                  size_t lm_response_len,
+                                  const uint8_t* ntlm_response,
+                                  size_t ntlm_response_len) {
+  return authenticate_writer->WriteBytes(lm_response, lm_response_len) &&
+         authenticate_writer->WriteBytes(ntlm_response, ntlm_response_len);
+}
+
+static bool WriteStringPayloads(NtlmBufferWriter* authenticate_writer,
+                                bool is_unicode,
+                                const base::string16& domain,
+                                const base::string16& username,
+                                const std::string hostname) {
+  if (is_unicode) {
+    return authenticate_writer->WriteUtf16String(domain) &&
+           authenticate_writer->WriteUtf16String(username) &&
+           authenticate_writer->WriteUtf8AsUtf16String(hostname);
+  } else {
+    return authenticate_writer->WriteUtf16AsUtf8String(domain) &&
+           authenticate_writer->WriteUtf16AsUtf8String(username) &&
+           authenticate_writer->WriteUtf8String(hostname);
+  }
+}
+
+// Returns the size in bytes of a string16 depending whether unicode
+// was negotiated.
+static size_t GetStringPayloadLength(const base::string16& str,
+                                     bool is_unicode) {
+  if (is_unicode)
+    return str.length() * 2;
+
+  // When |WriteUtf16AsUtf8String| is called with a |base::string16|, the string
+  // is converted to UTF8. Do the conversion to ensure that the character
+  // count is correct.
+  return base::UTF16ToUTF8(str).length();
+}
+
+// Returns the size in bytes of a std::string depending whether unicode
+// was negotiated.
+static size_t GetStringPayloadLength(const std::string& str, bool is_unicode) {
+  if (!is_unicode)
+    return str.length();
+
+  return base::UTF8ToUTF16(str).length() * 2;
+}
+
+}  // namespace
+
+NtlmClient::NtlmClient() : negotiate_flags_(NEGOTIATE_MESSAGE_FLAGS) {
+  // Just generate the negotiate message once and hold on to it. It never
+  // changes and in a NTLMv2 it's used as an input
+  // to the Message Integrity Check in the Authenticate message.
+  GenerateNegotiateMessage();
+}
+
+NtlmClient::~NtlmClient() {}
+
+void NtlmClient::GetNegotiateMessage(uint8_t** negotiate_message,
+                                     size_t* negotiate_message_len) const {
+  *negotiate_message_len = NEGOTIATE_MESSAGE_LEN;
+  *negotiate_message = new uint8_t[NEGOTIATE_MESSAGE_LEN];

[asanka, 2017/07/12 20:38:37]

HttpAuthHandlerNTLM::GenerateAuthTokenImpl ultimately uses free() to free this
buffer. Let's use malloc() here. It's not safe in general to mix malloc/free vs.
new/delete. This also applies to NltmBufferWriter.

Alternately, we can switch over HttpAuthHandlerNTLM + the NTLM implementation +
the implementation on Windows to use new uint8_t[], pass uint8_ts everywhere,
and then use delete[] to free it.

[zentaro, 2017/07/13 20:27:18]

I like that latter option in the long run but don't really want to go off on a
another refactoring right now.

I specialized a unique_ptr to use free for the tests. I removed ReleaseBuffer
from the BufferWriter (it didn't really need it anyway) and now everything
coming out of the client is from malloc. 

I'll maybe look at the other refactoring once we get through all this.

[asanka, 2017/07/14 16:52:38]

I apologize for not pushing harder for something better earlier. I had some
doubts about whether returning raw buffers would make the implementation easier.
But now that the implementation is further along the issues with raw buffers are
becoming clearer. In the latest patchset, for example, the code is copying
buffers just to get the memory management semantics right.

Compare that with something like the following where std::string is used as a
container for the message:

   class NtlmClient {
    public:
     std::string GetNegotiateMessage();

     std::string GenerateAuthenticateMessage(...); // returns empty string on
error.
   }

Also, as we've observed before, BufferWriter can work just as easily with a
buffer that's owned by the caller rather than allocating its own buffer.

There's no reason for HttpAuthHandlerNTLM to take a raw buffer either. We can
change that to use something more sensible (like a std::string) when we cut over
to the new client.

Overall, I now believe strongly that the use of raw pointers and inconsistent
memory management is adding more complexity to the code. Let's fix it before
things go much further. It's easier to address it now than to wait until there's
even more code depending on the API.

Note that instead of a std::string, you can just as easily use a
std::vector<uint8_t>. The latter has the benefit of having a non-const data()
method that'll give you access to a reserved() internal buffer. std::string()
has the same in C++17, but we aren't there yet. std::string() is, however,
*much* more efficient at dealing with smaller buffers due to short string
optimization than std::vector.

+  memcpy(*negotiate_message, negotiate_message_.get(), NEGOTIATE_MESSAGE_LEN);
+}
+
+void NtlmClient::GenerateNegotiateMessage() {
+  NtlmBufferWriter writer(NEGOTIATE_MESSAGE_LEN);
+  bool result =
+      writer.WriteMessageHeader(MessageType::NEGOTIATE) &&
+      writer.WriteFlags(negotiate_flags_) &&
+      writer.WriteSecurityBuffer(SecurityBuffer(NEGOTIATE_MESSAGE_LEN, 0)) &&
+      writer.WriteSecurityBuffer(SecurityBuffer(NEGOTIATE_MESSAGE_LEN, 0)) &&
+      writer.IsEndOfBuffer();
+
+  DCHECK(result);
+
+  negotiate_message_ = writer.ReleaseBuffer();
+}
+
+bool NtlmClient::GenerateAuthenticateMessage(
+    const base::string16& domain,
+    const base::string16& username,
+    const base::string16& password,
+    const std::string& hostname,
+    const uint8_t* client_challenge,
+    const uint8_t* challenge_message,
+    size_t challenge_message_len,
+    uint8_t** authenticate_message,
+    size_t* authenticate_message_len) const {
+  *authenticate_message = nullptr;
+  *authenticate_message_len = 0;
+  NegotiateFlags challenge_flags;
+  uint8_t server_challenge[CHALLENGE_LEN];
+
+  if (!ParseChallengeMessage(challenge_message, challenge_message_len,
+                             &challenge_flags, server_challenge)) {
+    return false;
+  }
+
+  // Calculate the responses for the authenticate message.
+  uint8_t lm_response[RESPONSE_V1_LEN];
+  uint8_t ntlm_response[RESPONSE_V1_LEN];
+
+  // Always use extended session security even if the server tries to downgrade.
+  NegotiateFlags authenticate_flags = (challenge_flags & negotiate_flags_) |
+                                      NegotiateFlags::EXTENDED_SESSIONSECURITY;
+
+  // Generate the LM and NTLM responses.
+  GenerateResponsesV1WithSS(password, server_challenge, client_challenge,
+                            lm_response, ntlm_response);
+
+  // Calculate all the payload lengths and offsets.
+  bool is_unicode =
+      static_cast<bool>(authenticate_flags & NegotiateFlags::UNICODE);
+
+  SecurityBuffer lm_info;
+  SecurityBuffer ntlm_info;
+  SecurityBuffer domain_info;
+  SecurityBuffer username_info;
+  SecurityBuffer hostname_info;
+  CalculatePayloadSizes(is_unicode, domain, username, hostname, &lm_info,
+                        &ntlm_info, &domain_info, &username_info,
+                        &hostname_info, authenticate_message_len);
+
+  // Write the authenticate message header.
+  NtlmBufferWriter authenticate_writer(*authenticate_message_len);
+  bool writer_result = WriteAuthenticateMessage(
+      &authenticate_writer, lm_info, ntlm_info, domain_info, username_info,
+      hostname_info, authenticate_flags);
+  DCHECK(writer_result);
+  DCHECK(authenticate_writer.GetCursor() == GetAuthenticateHeaderLength());
+
+  // Write the response payloads.
+  writer_result =
+      WriteResponsePayloads(&authenticate_writer, lm_response, lm_info.length,
+                            ntlm_response, ntlm_info.length);
+  DCHECK(writer_result);
+  DCHECK(authenticate_writer.GetCursor() == domain_info.offset);
+
+  // Write the string field payloads.
+  writer_result = WriteStringPayloads(&authenticate_writer, is_unicode, domain,
+                                      username, hostname);
+  DCHECK(writer_result);
+  DCHECK(authenticate_writer.IsEndOfBuffer());
+
+  *authenticate_message = authenticate_writer.ReleaseBuffer().release();
+  return true;
+}
+
+size_t NtlmClient::CalculateAuthenticateMessageLength(
+    bool is_unicode,
+    const base::string16& domain,
+    const base::string16& username,
+    const std::string& hostname) const {
+  return GetAuthenticateHeaderLength() +
+         GetStringPayloadLength(domain, is_unicode) +
+         GetStringPayloadLength(username, is_unicode) +
+         GetStringPayloadLength(hostname, is_unicode) + RESPONSE_V1_LEN +
+         GetNtlmResponseLength();
+}
+
+void NtlmClient::CalculatePayloadSizes(bool is_unicode,

[asanka, 2017/07/12 20:38:37]

This is calculating payload layout.

[zentaro, 2017/07/13 20:27:18]

Done.

+                                       const base::string16& domain,
+                                       const base::string16& username,
+                                       const std::string& hostname,
+                                       SecurityBuffer* lm_info,
+                                       SecurityBuffer* ntlm_info,
+                                       SecurityBuffer* domain_info,
+                                       SecurityBuffer* username_info,
+                                       SecurityBuffer* hostname_info,
+                                       size_t* authenticate_message_len) const {
+  size_t upto = GetAuthenticateHeaderLength();
+
+  lm_info->offset = upto;
+  lm_info->length = RESPONSE_V1_LEN;
+  upto += lm_info->length;
+
+  ntlm_info->offset = upto;
+  ntlm_info->length = GetNtlmResponseLength();
+  upto += ntlm_info->length;
+
+  domain_info->offset = upto;
+  domain_info->length = GetStringPayloadLength(domain, is_unicode);
+  upto += domain_info->length;
+
+  username_info->offset = upto;
+  username_info->length = GetStringPayloadLength(username, is_unicode);
+  upto += username_info->length;
+
+  hostname_info->offset = upto;
+  hostname_info->length = GetStringPayloadLength(hostname, is_unicode);
+  upto += hostname_info->length;
+
+  // Calculate the full message size including payload.
+  *authenticate_message_len = CalculateAuthenticateMessageLength(
+      is_unicode, domain, username, hostname);
+
+  // Double check the end of the payloads matches the end of the message.
+  DCHECK(*authenticate_message_len == upto);

[asanka, 2017/07/12 20:38:37]

Why call CalculateAuthenticateMessageLength() instead of setting
*authenticate_message_len = upto?

It seems CAML is doing the same calculation as done here, but just looking at
lengths.

[zentaro, 2017/07/13 20:27:18]

Done.

+}
+
+size_t NtlmClient::GetAuthenticateHeaderLength() const {
+  return AUTHENTICATE_HEADER_V1_LEN;
+}
+
+size_t NtlmClient::GetNtlmResponseLength() const {
+  return RESPONSE_V1_LEN;
+}
+
+}  // namespace ntlm
+}  // namespace net