Add unit tests for NTLMv1 portable implementation

net/http/http_auth_handler_ntlm_portable_unittest.cc

Index: net/http/http_auth_handler_ntlm_portable_unittest.cc
diff --git a/net/http/http_auth_handler_ntlm_portable_unittest.cc b/net/http/http_auth_handler_ntlm_portable_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..fe1e2c8e88ac40677dad7be18559b9f5b6f2d3c2
--- /dev/null
+++ b/net/http/http_auth_handler_ntlm_portable_unittest.cc
@@ -0,0 +1,285 @@
+// 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/http_auth_handler_ntlm.h"
+
+#include <string>
+
+#include "base/base64.h"
+#include "base/memory/ptr_util.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "net/base/net_errors.h"
+#include "net/base/test_completion_callback.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/http/http_auth_challenge_tokenizer.h"
+#include "net/http/http_request_info.h"
+#include "net/http/mock_allow_http_auth_preferences.h"
+#include "net/log/net_log_with_source.h"
+#include "net/ssl/ssl_info.h"
+#include "net/test/gtest_util.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/platform_test.h"
+
+using net::test::IsError;
+using net::test::IsOk;
+
+namespace net {
+
+#if defined(NTLM_PORTABLE)
+
+const char NTLM_SIGNATURE[] = "NTLMSSP";
+
+class HttpAuthHandlerNtlmPortableTest : public PlatformTest {
+ public:
+  void SetUp() override {
+    http_auth_preferences_.reset(new MockAllowHttpAuthPreferences());
+    factory_.reset(new HttpAuthHandlerNTLM::Factory());
+    factory_->set_http_auth_preferences(http_auth_preferences_.get());
+    creds_ = AuthCredentials(base::ASCIIToUTF16("someuser"),
+                             base::ASCIIToUTF16("badpassword"));

[Ryan Sleevi, 2017/05/09 20:49:27]

Any reason for the SetUp vs CTOR?

(See
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...
)

[zentaro, 2017/05/15 20:48:38]

Nope. I just copied from the Negotiate one as a starting point. Moved to
constructor.

+  }
+
+  int CreateHandler() {
+    GURL gurl("foo.com");

[Ryan Sleevi, 2017/05/09 20:49:27]

Since you're creating a GURL, let's use a proper URL - e.g. https://foo.com

Implicitly, you're relying on GURL to guess what you want, so better to make it
explicit (and this also makes it clear it's HTTPS vs, currently, implicitly HTTP
- although if you meant HTTP, better to say that :P)

[zentaro, 2017/05/15 20:48:37]

Done.

+    SSLInfo null_ssl_info;
+
+    return factory_->CreateAuthHandlerFromString(
+        "NTLM", HttpAuth::AUTH_SERVER, null_ssl_info, gurl, NetLogWithSource(),
+        &auth_handler_);
+  }
+
+  std::string CreateType2Token(const char* message, size_t message_len) {

[Ryan Sleevi, 2017/05/09 20:49:27]

Consider using base::StringPiece ?

[zentaro, 2017/05/15 20:48:38]

Done.

+    std::string output;
+    std::string input(message, message_len);
+    base::Base64Encode(input, &output);
+
+    return "NTLM " + output;
+  }
+
+  void HandleAnotherChallenge(std::string challenge) {

[Ryan Sleevi, 2017/05/09 20:49:27]

Pass this by const-ref? Or did you mean to explicitly make a copy?

[zentaro, 2017/05/15 20:48:37]

Done.

+    HttpAuthChallengeTokenizer tokenizer(challenge.begin(), challenge.end());
+    EXPECT_EQ(HttpAuth::AUTHORIZATION_RESULT_ACCEPT,
+              GetAuthHandler()->HandleAnotherChallenge(&tokenizer));
+  }
+
+  int GenerateAuthToken(std::string* token) {
+    TestCompletionCallback callback;
+    HttpRequestInfo request_info;
+    return callback.GetResult(GetAuthHandler()->GenerateAuthToken(
+        GetCreds(), &request_info, callback.callback(), token));

[Ryan Sleevi, 2017/05/09 20:49:27]

There's no way to wait for the callback's result since you're deleting it on 69,
right?

I'm not entirely sure it's safe here, but I haven't dug in yet. Just trying to
flag it early in case you knew it was safe

[zentaro, 2017/05/15 20:48:37]

Looks like TestCompletionCallback::GetResult checks if ERR_IO_PENDING is
returned, and if it is it blocks waiting for the callback to fire. So this
effectively forces it to appear synchronous to the test even if it wasn't. Am I
misunderstanding how this works? All the other tests seem to do the same thing.

+  }
+
+  int GenerateAuthToken() {

[Ryan Sleevi, 2017/05/09 20:49:27]

Why this overload? This is a fairly surprising naming (compared with 68).

It also seems dangerous from a security perspective - if GenerateAuthToken
completes asynchronously, |*token| will be invalidated (because the stack frame
at 76 will have cleaned it up) - that doesn't seem good.

[zentaro, 2017/05/15 20:48:37]

As above I think this runs synchronously. The overload is because many of the
cases don't care about the token param. I renamed it GetGenerateAuthTokenResult.

+    std::string token;
+    return GenerateAuthToken(&token);
+  }
+
+  AuthCredentials* GetCreds() { return &creds_; }
+
+  HttpAuthHandlerNTLM* GetAuthHandler() {
+    return static_cast<HttpAuthHandlerNTLM*>(auth_handler_.get());
+  }
+
+  static void MockRandom(uint8_t* output, size_t n) {
+    static const uint8_t bytes[] = {0x55, 0x29, 0x66, 0x26,
+                                    0x6b, 0x9c, 0x73, 0x54};
+    static size_t current_byte = 0;
+    for (size_t i = 0; i < n; ++i) {
+      output[i] = bytes[current_byte++];
+      current_byte %= arraysize(bytes);
+    }
+  }
+
+  static std::string MockGetHostName() { return "MYHOSTNAME"; }
+
+ private:
+  AuthCredentials creds_;
+  std::unique_ptr<HttpAuthHandler> auth_handler_;
+  std::unique_ptr<MockAllowHttpAuthPreferences> http_auth_preferences_;
+  std::unique_ptr<HttpAuthHandlerNTLM::Factory> factory_;
+};
+
+TEST_F(HttpAuthHandlerNtlmPortableTest, SimpleConstruction) {
+  EXPECT_EQ(OK, CreateHandler());
+  ASSERT_TRUE(GetAuthHandler() != nullptr);
+}
+
+TEST_F(HttpAuthHandlerNtlmPortableTest, DoNotAllowDefaultCreds) {
+  EXPECT_EQ(OK, CreateHandler());
+  EXPECT_FALSE(GetAuthHandler()->AllowsDefaultCredentials());
+}
+
+TEST_F(HttpAuthHandlerNtlmPortableTest, AllowsExplicitCredentials) {
+  EXPECT_EQ(OK, CreateHandler());
+  EXPECT_TRUE(GetAuthHandler()->AllowsExplicitCredentials());
+}
+
+TEST_F(HttpAuthHandlerNtlmPortableTest, VerifyType1Message) {
+  EXPECT_EQ(OK, CreateHandler());
+
+  std::string token;
+  EXPECT_EQ(OK, GenerateAuthToken(&token));
+  // The type 1 message generated is always the same. The only variable
+  // part of the message is the flags and Chrome always offers the same
+  // set of flags.
+  EXPECT_EQ("NTLM TlRMTVNTUAABAAAAB4IIAAAAAAAAAAAAAAAAAAAAAAA=", token);
+}
+
+TEST_F(HttpAuthHandlerNtlmPortableTest, ValidType2Input) {
+  HttpAuthHandlerNTLM::ScopedProcSetter proc_setter(MockRandom,
+                                                    MockGetHostName);
+  EXPECT_EQ(OK, CreateHandler());
+
+  // This is a real type 2 message sent by a Windows Server in response to
+  // Chrome's type 1 message.

[Ryan Sleevi, 2017/05/09 20:49:27]

How was this computed? What are the values? How can it be reconstructed?

[zentaro, 2017/05/15 20:48:38]

It was just copied from a real transaction. I was just trying to add a known
good message as a sanity check.

In order to more easily add tests that actually poke all the various internal
fields that the server can send I have another CL -
https://codereview.chromium.org/2879353002 which provides some helper functions
to make reading and writing the messages easier. 

They will also be used in the actual implementation to consolidate all the
buffer manipulation primitives rather than the ad hoc buffer/cursor management
the current code does.

I was planning another set of additional tests once those helpers are available.
It's up to you if you want to land this minimal set of tests first or we can
wait until the above CL lands and I'll add the additional tests to this CL.

+  std::string challenge_str =
+      "NTLM "
+      "TlRMTVNTUAACAAAADAAMADgAAAAFgokCXziKeNIPIDYAAAAAAAAAAIYAhgBEAAAABgOAJQAA"
+      "AA9aAEUATgBEAE8ATQACAAwAWgBFAE4ARABPAE0AAQAMAFoARQBOAEQAQwAxAAQAFAB6AGUA"
+      "bgBkAG8AbQAuAGwAbwBjAAMAIgBaAGUAbgBEAEMAMQAuAHoAZQBuAGQAbwBtAC4AbABvAGMA"
+      "BQAUAHoAZQBuAGQAbwBtAC4AbABvAGMABwAIAN6N+IxGwNIBAAAAAA==";
+  std::string token;
+  HandleAnotherChallenge(challenge_str);
+  EXPECT_EQ(OK, GenerateAuthToken(&token));
+
+  // Type 3 message based on mocked RNG data.
+  EXPECT_EQ(
+      "NTLM "
+      "TlRMTVNTUAADAAAAGAAYAGQAAAAYABgAfAAAAAAAAABAAAAAEAAQAEAAAAAUABQAUAAAAAAA"
+      "AAAAAAAABYIIAHMAbwBtAGUAdQBzAGUAcgBNAFkASABPAFMAVABOAEEATQBFAFUpZiZrnHNU"
+      "AAAAAAAAAAAAAAAAAAAAAOYv6VCd3XSbN+S4BO5kmGWvVzlFBGnJOg==",
+      token);
+}
+
+TEST_F(HttpAuthHandlerNtlmPortableTest, Type2MessageTooShort) {
+  EXPECT_EQ(OK, CreateHandler());
+
+  // Fail because the minimum size valid message is 32 bytes.
+  char raw[31];
+  memset(raw, 0, sizeof(raw));
+  HandleAnotherChallenge(CreateType2Token(raw, sizeof(raw)));
+  EXPECT_EQ(ERR_UNEXPECTED, GenerateAuthToken());
+}
+
+TEST_F(HttpAuthHandlerNtlmPortableTest, Type2MessageNoSig) {
+  EXPECT_EQ(OK, CreateHandler());
+
+  // Fail because the first bytes don't match "NTLMSSP\0"
+  char raw[32];
+  memset(raw, 0, sizeof(raw));
+  HandleAnotherChallenge(CreateType2Token(raw, sizeof(raw)));
+  EXPECT_EQ(ERR_UNEXPECTED, GenerateAuthToken());
+}
+
+TEST_F(HttpAuthHandlerNtlmPortableTest, Type2WrongMessageType) {
+  EXPECT_EQ(OK, CreateHandler());
+
+  // Fail because the message type (starting byte 8) should be 0x00000002.
+  char raw[32];
+  memset(raw, 0, sizeof(raw));
+  memcpy(raw, NTLM_SIGNATURE, sizeof(NTLM_SIGNATURE));
+  HandleAnotherChallenge(CreateType2Token(raw, sizeof(raw)));
+  EXPECT_EQ(ERR_UNEXPECTED, GenerateAuthToken());
+}
+
+TEST_F(HttpAuthHandlerNtlmPortableTest, MinimalStructurallyValidType2) {
+  EXPECT_EQ(OK, CreateHandler());
+
+  char raw[32];
+  memset(raw, 0, sizeof(raw));
+  memcpy(raw, NTLM_SIGNATURE, sizeof(NTLM_SIGNATURE));
+  // Mark as a type 2 message.
+  raw[8] = 2;
+
+  HandleAnotherChallenge(CreateType2Token(raw, sizeof(raw)));
+  EXPECT_EQ(OK, GenerateAuthToken());
+}
+
+TEST_F(HttpAuthHandlerNtlmPortableTest, Type2MessageWithNoTargetName) {
+  EXPECT_EQ(OK, CreateHandler());
+
+  char raw[32];
+  memset(raw, 0, sizeof(raw));
+  memcpy(raw, NTLM_SIGNATURE, sizeof(NTLM_SIGNATURE));
+  // Mark as a type 2 message.
+  raw[8] = 2;
+  // Point the offset to the end of the buffer.
+  raw[16] = 32;
+
+  // Although the MinimalStructurallyValidType2 message with both
+  // length and offset equal zero is not forbidden by the spec, the
+  // spec (2.2.1.2) states that the length SHOULD be 0 and the
+  // offset SHOULD be where the payload would be if it was present.
+  HandleAnotherChallenge(CreateType2Token(raw, sizeof(raw)));
+  EXPECT_EQ(OK, GenerateAuthToken());
+}
+
+TEST_F(HttpAuthHandlerNtlmPortableTest, Type2MessageWithTargetName) {
+  EXPECT_EQ(OK, CreateHandler());
+
+  // One extra byte is provided for target name.
+  char raw[33];
+  memset(raw, 0, sizeof(raw));
+  memcpy(raw, NTLM_SIGNATURE, sizeof(NTLM_SIGNATURE));
+  // Mark as a type 2 message.
+  raw[8] = 2;
+  // The target name field is 1 byte long.
+  raw[12] = 1;
+  raw[14] = 1;
+  // Point the offset to the extra 1 byte space.
+  raw[16] = 32;
+
+  HandleAnotherChallenge(CreateType2Token(raw, sizeof(raw)));
+  EXPECT_EQ(OK, GenerateAuthToken());
+}
+
+TEST_F(HttpAuthHandlerNtlmPortableTest, NoTargetNameOverflowFromOffset) {
+  EXPECT_EQ(OK, CreateHandler());
+
+  char raw[32];
+  memset(raw, 0, sizeof(raw));
+  memcpy(raw, NTLM_SIGNATURE, sizeof(NTLM_SIGNATURE));
+  // Mark as a type 2 message.
+  raw[8] = 2;
+  // Claim that the target name field is 1 byte long.
+  raw[12] = 1;
+  raw[14] = 1;
+  // Point the offset outside the message buffer.
+  raw[16] = 32;
+
+  // The above malformed message could cause an implementation
+  // to read outside the message buffer because the offset is
+  // past the end of the message. Verify it gets rejected.
+  HandleAnotherChallenge(CreateType2Token(raw, sizeof(raw)));
+  EXPECT_EQ(ERR_UNEXPECTED, GenerateAuthToken());
+}
+
+TEST_F(HttpAuthHandlerNtlmPortableTest, NoTargetNameOverflowFromLength) {
+  EXPECT_EQ(OK, CreateHandler());
+
+  // Message has 1 extra byte of space after the header for the
+  // target name.
+  char raw[33];
+  memset(raw, 0, sizeof(raw));
+  memcpy(raw, NTLM_SIGNATURE, sizeof(NTLM_SIGNATURE));
+  // Mark as a type 2 message.
+  raw[8] = 2;
+  // Claim that the target name field is 2 bytes long.
+  raw[12] = 2;
+  raw[14] = 2;
+  // Point the offset to the extra 1 byte space.
+  raw[16] = 32;
+
+  // The above malformed message could cause an implementation
+  // to read outside the message buffer because the length is
+  // longer than available space. Verify it gets rejected.
+  HandleAnotherChallenge(CreateType2Token(raw, sizeof(raw)));
+  EXPECT_EQ(ERR_UNEXPECTED, GenerateAuthToken());
+}
+
+#endif  // defined(NTLM_PORTABLE)
+
+}  // namespace net