Replace NTLMv1 implementation with a functionally equivalent one.

Add a functionally equivalent NTLMv1 implementation.

- Replacement NTLMv1 implementation.
- In preparation for new NTLMv2 implementation.
- Passes all unit tests and has the same behavior.
- Adds fuzzer for NtlmClient.
- Code is still not live.

BUG=chromium:22532
TEST=All unit tests pass.

[zentaro, 2017-06-08 15:45:45]

Description was changed from

==========
Replace NTLMv1 implementation with a functionally equivalent one.

- In preparation for new NTLMv2 implementation.
- Passes all unit tests and has the same behavior.
- Deletes existing code.

BUG=chromium:22532
TEST=All unit tests pass.
==========

to

==========
Replace NTLMv1 implementation with a functionally equivalent one.

- In preparation for new NTLMv2 implementation.
- Passes all unit tests and has the same behavior.
- Deletes existing code.
- Add unit tests at the NtlmClient level.

BUG=chromium:22532
TEST=All unit tests pass.
==========

[zentaro, 2017-06-08 15:45:45]

zentaro@chromium.org changed reviewers:
+ asanka@chromium.org, rsleevi@chromium.org

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

https://codereview.chromium.org/2904633002/diff/490001/net/ntlm/ntlm_client.cc
File net/ntlm/ntlm_client.cc (right):

https://codereview.chromium.org/2904633002/diff/490001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:113: *negotiate_message = new
uint8_t[NEGOTIATE_MESSAGE_LEN];
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.

https://codereview.chromium.org/2904633002/diff/490001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:213: void NtlmClient::CalculatePayloadSizes(bool
is_unicode,
This is calculating payload layout.

https://codereview.chromium.org/2904633002/diff/490001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:250: DCHECK(*authenticate_message_len == upto);
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.

https://codereview.chromium.org/2904633002/diff/490001/net/ntlm/ntlm_client_u...
File net/ntlm/ntlm_client_unittest.cc (right):

https://codereview.chromium.org/2904633002/diff/490001/net/ntlm/ntlm_client_u...
net/ntlm/ntlm_client_unittest.cc:28: negotiate_msg->reset(negotiate_msg_ptr);
as mentioned elsewhere, this assumes that negotiate_msg_ptr was allocated via
new[]. Just leaving this note here so we don't forget to check this file when we
address it.

[zentaro, 2017-07-13 20:27:11]

The CQ bit was checked by zentaro@chromium.org to run a CQ dry run

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

https://codereview.chromium.org/2904633002/diff/490001/net/ntlm/ntlm_client.cc
File net/ntlm/ntlm_client.cc (right):

https://codereview.chromium.org/2904633002/diff/490001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:113: *negotiate_message = new
uint8_t[NEGOTIATE_MESSAGE_LEN];
On 2017/07/12 20:38:37, asanka wrote:
> 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.

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.

https://codereview.chromium.org/2904633002/diff/490001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:213: void NtlmClient::CalculatePayloadSizes(bool
is_unicode,
On 2017/07/12 20:38:37, asanka wrote:
> This is calculating payload layout.

Done.

https://codereview.chromium.org/2904633002/diff/490001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:250: DCHECK(*authenticate_message_len == upto);
On 2017/07/12 20:38:37, asanka wrote:
> 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.

Done.

https://codereview.chromium.org/2904633002/diff/490001/net/ntlm/ntlm_client_u...
File net/ntlm/ntlm_client_unittest.cc (right):

https://codereview.chromium.org/2904633002/diff/490001/net/ntlm/ntlm_client_u...
net/ntlm/ntlm_client_unittest.cc:28: negotiate_msg->reset(negotiate_msg_ptr);
On 2017/07/12 20:38:37, asanka wrote:
> as mentioned elsewhere, this assumes that negotiate_msg_ptr was allocated via
> new[]. Just leaving this note here so we don't forget to check this file when
we
> address it.

Specialized the unique_ptr to call free.

[commit-bot: I haz the power, 2017-07-13 20:27:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-13 21:07:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-13 21:07:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[asanka, 2017-07-14 16:52:39]

https://codereview.chromium.org/2904633002/diff/490001/net/ntlm/ntlm_client.cc
File net/ntlm/ntlm_client.cc (right):

https://codereview.chromium.org/2904633002/diff/490001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:113: *negotiate_message = new
uint8_t[NEGOTIATE_MESSAGE_LEN];
On 2017/07/13 20:27:18, zentaro wrote:
> On 2017/07/12 20:38:37, asanka wrote:
> > 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.
> 
> 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.


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.

https://codereview.chromium.org/2904633002/diff/530001/net/http/http_auth_han...
File net/http/http_auth_handler_ntlm.h (right):

https://codereview.chromium.org/2904633002/diff/530001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm.h:75: static void GenerateRandom(uint8_t*
output, size_t n);
This doesn't need to be a part of HttpAuthHandlerNTLM. Instead move it into an
internal namespace in the .cc file.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm.cc
File net/ntlm/ntlm.cc (right):

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm.cc#newco...
net/ntlm/ntlm.cc:11: #include "base/strings/utf_string_conversions.h"
not used?

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm.cc#newco...
net/ntlm/ntlm.cc:14: #include "net/ntlm/ntlm_buffer_reader.h"
not used?

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.cc
File net/ntlm/ntlm_client.cc (right):

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:22: static bool ParseChallengeMessage(const uint8_t*
challenge_message,
|static| is redundant within an anonymous namespace.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:65: const std::string hostname) {
Use a |const std::string&| here. Otherwise you are forcing a copy at the
callsite at GenerateAuthenticateMessage().

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:101: NtlmClient::NtlmClient() :
negotiate_flags_(kNegotiateMessageFlags) {
why store negotiate_flags_ here? It's essentially immutable.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:128: negotiate_message_.reset(new
uint8_t[writer.GetLength()]);
Why create a new copy of the writer's buffer?

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:167: static_cast<bool>(authenticate_flags &
NegotiateFlags::kUnicode);
bool is_unicode = (authenticate_flags & NegotiateFlags::kUnicode) ==
NegotiateFlags::kUnicode

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:184: DCHECK(authenticate_writer.GetCursor() ==
GetAuthenticateHeaderLength());
DCHECK_EQ here and elsewhere.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:186: // Write the response payloads.
many of these comments seem unnecessary since the method names are just as
descriptive.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:201:
static_cast<uint8_t*>(malloc(authenticate_writer.GetLength()));
let's avoid unnecessary buffer copies.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.h
File net/ntlm/ntlm_client.h (right):

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.h...
net/ntlm/ntlm_client.h:37: // is responsible for cleaning up.
Let's discuss our buffer management strategy. I don't think it's quite right.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.h...
net/ntlm/ntlm_client.h:46: const std::string& hostname,
Indicate how this is used, and whether a FQDN is required.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.h...
net/ntlm/ntlm_client.h:47: const uint8_t* client_challenge,
Document buffer size and content.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.h...
net/ntlm/ntlm_client.h:48: const uint8_t* challenge_message,
Let's call this server_challenge_message or something like that. It a bit
confusing when there's  a |client_challenge| and a |challenge_message| next to
each other.

[zentaro, 2017-07-19 15:20:13]

https://codereview.chromium.org/2904633002/diff/530001/net/http/http_auth_han...
File net/http/http_auth_handler_ntlm.h (right):

https://codereview.chromium.org/2904633002/diff/530001/net/http/http_auth_han...
net/http/http_auth_handler_ntlm.h:75: static void GenerateRandom(uint8_t*
output, size_t n);
On 2017/07/14 16:52:38, asanka wrote:
> This doesn't need to be a part of HttpAuthHandlerNTLM. Instead move it into an
> internal namespace in the .cc file.

Done.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm.cc
File net/ntlm/ntlm.cc (right):

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm.cc#newco...
net/ntlm/ntlm.cc:11: #include "base/strings/utf_string_conversions.h"
On 2017/07/14 16:52:39, asanka wrote:
> not used?

Done.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm.cc#newco...
net/ntlm/ntlm.cc:14: #include "net/ntlm/ntlm_buffer_reader.h"
On 2017/07/14 16:52:39, asanka wrote:
> not used?

Done.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.cc
File net/ntlm/ntlm_client.cc (right):

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:22: static bool ParseChallengeMessage(const uint8_t*
challenge_message,
On 2017/07/14 16:52:39, asanka wrote:
> |static| is redundant within an anonymous namespace.

Done.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:65: const std::string hostname) {
On 2017/07/14 16:52:39, asanka wrote:
> Use a |const std::string&| here. Otherwise you are forcing a copy at the
> callsite at GenerateAuthenticateMessage().

Done.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:101: NtlmClient::NtlmClient() :
negotiate_flags_(kNegotiateMessageFlags) {
On 2017/07/14 16:52:39, asanka wrote:
> why store negotiate_flags_ here? It's essentially immutable.

In the next cl for v2 - I allow it along with a features bitset (to toggle
MIC/EPA etc) to be optionally passed to the constructor if the caller wants to
specify different flags.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:128: negotiate_message_.reset(new
uint8_t[writer.GetLength()]);
On 2017/07/14 16:52:39, asanka wrote:
> Why create a new copy of the writer's buffer?

N/A - It's a string now.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:167: static_cast<bool>(authenticate_flags &
NegotiateFlags::kUnicode);
On 2017/07/14 16:52:39, asanka wrote:
> bool is_unicode = (authenticate_flags & NegotiateFlags::kUnicode) ==
> NegotiateFlags::kUnicode

Done.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:184: DCHECK(authenticate_writer.GetCursor() ==
GetAuthenticateHeaderLength());
On 2017/07/14 16:52:39, asanka wrote:
> DCHECK_EQ here and elsewhere.

Done.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:186: // Write the response payloads.
On 2017/07/14 16:52:39, asanka wrote:
> many of these comments seem unnecessary since the method names are just as
> descriptive.

Deleted.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:201:
static_cast<uint8_t*>(malloc(authenticate_writer.GetLength()));
On 2017/07/14 16:52:39, asanka wrote:
> let's avoid unnecessary buffer copies.

N/A string now.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.h
File net/ntlm/ntlm_client.h (right):

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.h...
net/ntlm/ntlm_client.h:37: // is responsible for cleaning up.
On 2017/07/14 16:52:39, asanka wrote:
> Let's discuss our buffer management strategy. I don't think it's quite right.

Done.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.h...
net/ntlm/ntlm_client.h:46: const std::string& hostname,
On 2017/07/14 16:52:39, asanka wrote:
> Indicate how this is used, and whether a FQDN is required.

Done.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.h...
net/ntlm/ntlm_client.h:47: const uint8_t* client_challenge,
On 2017/07/14 16:52:39, asanka wrote:
> Document buffer size and content.

Done.

https://codereview.chromium.org/2904633002/diff/530001/net/ntlm/ntlm_client.h...
net/ntlm/ntlm_client.h:48: const uint8_t* challenge_message,
On 2017/07/14 16:52:39, asanka wrote:
> Let's call this server_challenge_message or something like that. It a bit
> confusing when there's  a |client_challenge| and a |challenge_message| next to
> each other.

Done.

[zentaro, 2017-07-19 20:19:34]

Description was changed from

==========
Replace NTLMv1 implementation with a functionally equivalent one.

- In preparation for new NTLMv2 implementation.
- Passes all unit tests and has the same behavior.
- Deletes existing code.
- Add unit tests at the NtlmClient level.

BUG=chromium:22532
TEST=All unit tests pass.
==========

to

==========
Replace NTLMv1 implementation with a functionally equivalent one.

- Replacement NTLMv1 implementation.
- In preparation for new NTLMv2 implementation.
- Passes all unit tests and has the same behavior.
- Deletes existing code.
- Changes interface to pass std::string instead of void*.
- Adds fuzzer for NtlmClient.


BUG=chromium:22532
TEST=All unit tests pass.
==========

[asanka, 2017-07-20 19:35:45]

asanka@chromium.org changed reviewers:
+ mmenke@chromium.org

[asanka, 2017-07-20 19:35:46]

+mmenke: Could you take a look at the fuzzer? I don't consider myself familiar
enough to review it. The expectation is that the fuzzer will land and be allowed
to cycle a bit before switching the HttpAuthHandlerNtlm to use the new
implementation.

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_u...
File net/ntlm/ntlm_client_unittest.cc (right):

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_u...
net/ntlm/ntlm_client_unittest.cc:58: uint8_t raw[sec_buf.length];
I thought we weren't going to do this? Perhaps std::unique_ptr<uint8_t[]>
instead?

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_u...
net/ntlm/ntlm_client_unittest.cc:121: raw[16] = 0x00;
Just for giggles, let's assert that raw[16] wasn't already 0. Same comment
applies to other tests that mutate a buffer.

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_unittest.cc
File net/ntlm/ntlm_unittest.cc (right):

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_unittest...
net/ntlm/ntlm_unittest.cc:21: #include "net/ntlm/ntlm.h"
duplicate

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_unittest...
net/ntlm/ntlm_unittest.cc:23: #include "net/ntlm/ntlm_buffer_writer.h"
why are these necessary? There doesn't seem to be any
NtlmBuffer{Reader,Writer}s?

[asanka, 2017-07-20 19:39:56]

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_f...
File net/ntlm/ntlm_client_fuzzer.cc (right):

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_f...
net/ntlm/ntlm_client_fuzzer.cc:22: }
Given that the library is for explicit credentials, and the credentials
themselves are provided by extensions and other external sources (including URL
embedded credentials currently), we'd need to consider the username/password to
also be possibly unsafe inputs.

As such, we'd need to fuzz the credentials as well in another fuzzer. This one
could respond to a known valid server challenge with fuzzed credentials.

[asanka, 2017-07-20 19:42:24]

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_f...
File net/ntlm/ntlm_client_fuzzer.cc (right):

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_f...
net/ntlm/ntlm_client_fuzzer.cc:22: }
On 2017/07/20 19:39:56, asanka wrote:
> Given that the library is for explicit credentials, and the credentials
> themselves are provided by extensions and other external sources (including
URL
> embedded credentials currently), we'd need to consider the username/password
to
> also be possibly unsafe inputs.
> 
> As such, we'd need to fuzz the credentials as well in another fuzzer. This one
> could respond to a known valid server challenge with fuzzed credentials.

If the server is allowed to disable Unicode, then we'd need at least two server
challenges that we can fuzz each credential input against.

[zentaro, 2017-07-20 19:42:52]

On 2017/07/20 19:39:56, asanka wrote:
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_f...
> File net/ntlm/ntlm_client_fuzzer.cc (right):
> 
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_f...
> net/ntlm/ntlm_client_fuzzer.cc:22: }
> Given that the library is for explicit credentials, and the credentials
> themselves are provided by extensions and other external sources (including
URL
> embedded credentials currently), we'd need to consider the username/password
to
> also be possibly unsafe inputs.
> 
> As such, we'd need to fuzz the credentials as well in another fuzzer. This one
> could respond to a known valid server challenge with fuzzed credentials.

Good point.

Based on the fuzzer function name "LLVMFuzzerTestOneInput" - I assume there's
some other n-input variants. I'll add some others too.

[mmenke, 2017-07-20 19:44:50]

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client.cc
File net/ntlm/ntlm_client.cc (right):

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:136: // Read the flags and the servers random challenge
from the challenge
nit:  server's

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_f...
File net/ntlm/ntlm_client_fuzzer.cc (right):

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_f...
net/ntlm/ntlm_client_fuzzer.cc:20: net::ntlm::test::kClientChallenge,
net::ntlm::Buffer(data, size));
Is there any benefit to fuzzing any of the other fields?  Differing challenge
flags in particular may be interesting.

It's pretty easy to split up input - see base::FuzzedDataProvider.  It has a
bunch of options fox fixed-size input.  For variable sized input, 
ConsumeRandomLengthString is the best way (And if some inputs can't possible be
passed in, you can then just check for them and exit the test early, though that
will make the fuzzer explore the space of your input validator, too).

[mmenke, 2017-07-20 19:45:47]

On 2017/07/20 19:42:52, zentaro wrote:
> On 2017/07/20 19:39:56, asanka wrote:
> >
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_f...
> > File net/ntlm/ntlm_client_fuzzer.cc (right):
> > 
> >
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_f...
> > net/ntlm/ntlm_client_fuzzer.cc:22: }
> > Given that the library is for explicit credentials, and the credentials
> > themselves are provided by extensions and other external sources (including
> URL
> > embedded credentials currently), we'd need to consider the username/password
> to
> > also be possibly unsafe inputs.
> > 
> > As such, we'd need to fuzz the credentials as well in another fuzzer. This
one
> > could respond to a known valid server challenge with fuzzed credentials.
> 
> Good point.
> 
> Based on the fuzzer function name "LLVMFuzzerTestOneInput" - I assume there's
> some other n-input variants. I'll add some others too.

There isn't a multiple input version (Or wasn't when I last checked), so you'll
have to slice and dice the input.

[mmenke, 2017-07-20 19:48:27]

https://codereview.chromium.org/2904633002/diff/690001/net/data/fuzzer_dictio...
File net/data/fuzzer_dictionaries/net_ntlm_ntlm_client_fuzzer.dict (right):

https://codereview.chromium.org/2904633002/diff/690001/net/data/fuzzer_dictio...
net/data/fuzzer_dictionaries/net_ntlm_ntlm_client_fuzzer.dict:30:
"\x03\x00\x00\x00"
May want to toss in \x08\x00 and \x00\x00, since they seem to be a common
themes.

[zentaro, 2017-08-01 18:21:36]

I modified it to get the other input fields from the fuzz data.

Now that it pulls everything randomly it seems like my handcrafted messages are
useless now? Previously they represented relevant challenge messages but now I
don't think I can handcraft a complete input vector. Do they add any value or
should I just delete them?

This caused it to generate many more (~250) "interesting" inputs running
locally. I just want to make sure I modified the fuzzer correctly before I
upload them all.

It seems like maybe this change has probably just made it go off exploring some
unrelated string conversion code since we don't really do much interesting with
those input strings.

Does it make sense to have 2 fuzzers - one with fixed domain/user/password/host
and one completely freeform? Or is the more freeform one good enough and it will
figure it out?

https://codereview.chromium.org/2904633002/diff/690001/net/data/fuzzer_dictio...
File net/data/fuzzer_dictionaries/net_ntlm_ntlm_client_fuzzer.dict (right):

https://codereview.chromium.org/2904633002/diff/690001/net/data/fuzzer_dictio...
net/data/fuzzer_dictionaries/net_ntlm_ntlm_client_fuzzer.dict:30:
"\x03\x00\x00\x00"
On 2017/07/20 19:48:27, mmenke wrote:
> May want to toss in \x08\x00 and \x00\x00, since they seem to be a common
> themes.

Done.

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client.cc
File net/ntlm/ntlm_client.cc (right):

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client.c...
net/ntlm/ntlm_client.cc:136: // Read the flags and the servers random challenge
from the challenge
On 2017/07/20 19:44:50, mmenke wrote:
> nit:  server's

Done.

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_u...
File net/ntlm/ntlm_client_unittest.cc (right):

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_u...
net/ntlm/ntlm_client_unittest.cc:58: uint8_t raw[sec_buf.length];
On 2017/07/20 19:35:46, asanka wrote:
> I thought we weren't going to do this? Perhaps std::unique_ptr<uint8_t[]>
> instead?

Done.

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_u...
net/ntlm/ntlm_client_unittest.cc:121: raw[16] = 0x00;
On 2017/07/20 19:35:46, asanka wrote:
> Just for giggles, let's assert that raw[16] wasn't already 0. Same comment
> applies to other tests that mutate a buffer.

Done.

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_unittest.cc
File net/ntlm/ntlm_unittest.cc (right):

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_unittest...
net/ntlm/ntlm_unittest.cc:21: #include "net/ntlm/ntlm.h"
On 2017/07/20 19:35:46, asanka wrote:
> duplicate

Done.

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_unittest...
net/ntlm/ntlm_unittest.cc:23: #include "net/ntlm/ntlm_buffer_writer.h"
On 2017/07/20 19:35:46, asanka wrote:
> why are these necessary? There doesn't seem to be any
> NtlmBuffer{Reader,Writer}s?

Done.

[mmenke, 2017-08-01 18:26:59]

On 2017/08/01 18:21:36, zentaro wrote:
> I modified it to get the other input fields from the fuzz data.
> 
> Now that it pulls everything randomly it seems like my handcrafted messages
are
> useless now? Previously they represented relevant challenge messages but now I
> don't think I can handcraft a complete input vector. Do they add any value or
> should I just delete them?

You can add them to the dictionary, if you want.  Though you can can still just
keep them where they are - they're still used as a seed, so it could still
stumble across adding headers/trailers to them, and reading them as they're
intended.  You could run the fuzzer manually for X cycles with them/without them
a couple times, and see if it makes a difference.

> This caused it to generate many more (~250) "interesting" inputs running
> locally. I just want to make sure I modified the fuzzer correctly before I
> upload them all.
> 
> It seems like maybe this change has probably just made it go off exploring
some
> unrelated string conversion code since we don't really do much interesting
with
> those input strings.
> 
> Does it make sense to have 2 fuzzers - one with fixed
domain/user/password/host
> and one completely freeform? Or is the more freeform one good enough and it
will
> figure it out?

You may want to ask someone on the fuzzer team (mmoroz?).  If there are only 250
more interesting inputs, that's small enough that I'm not sure an extra fuzzer
is worth it.

>
https://codereview.chromium.org/2904633002/diff/690001/net/data/fuzzer_dictio...
> File net/data/fuzzer_dictionaries/net_ntlm_ntlm_client_fuzzer.dict (right):
> 
>
https://codereview.chromium.org/2904633002/diff/690001/net/data/fuzzer_dictio...
> net/data/fuzzer_dictionaries/net_ntlm_ntlm_client_fuzzer.dict:30:
> "\x03\x00\x00\x00"
> On 2017/07/20 19:48:27, mmenke wrote:
> > May want to toss in \x08\x00 and \x00\x00, since they seem to be a common
> > themes.
> 
> Done.
> 
> https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client.cc
> File net/ntlm/ntlm_client.cc (right):
> 
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client.c...
> net/ntlm/ntlm_client.cc:136: // Read the flags and the servers random
challenge
> from the challenge
> On 2017/07/20 19:44:50, mmenke wrote:
> > nit:  server's
> 
> Done.
> 
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_u...
> File net/ntlm/ntlm_client_unittest.cc (right):
> 
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_u...
> net/ntlm/ntlm_client_unittest.cc:58: uint8_t raw[sec_buf.length];
> On 2017/07/20 19:35:46, asanka wrote:
> > I thought we weren't going to do this? Perhaps std::unique_ptr<uint8_t[]>
> > instead?
> 
> Done.
> 
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_u...
> net/ntlm/ntlm_client_unittest.cc:121: raw[16] = 0x00;
> On 2017/07/20 19:35:46, asanka wrote:
> > Just for giggles, let's assert that raw[16] wasn't already 0. Same comment
> > applies to other tests that mutate a buffer.
> 
> Done.
> 
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_unittest.cc
> File net/ntlm/ntlm_unittest.cc (right):
> 
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_unittest...
> net/ntlm/ntlm_unittest.cc:21: #include "net/ntlm/ntlm.h"
> On 2017/07/20 19:35:46, asanka wrote:
> > duplicate
> 
> Done.
> 
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_unittest...
> net/ntlm/ntlm_unittest.cc:23: #include "net/ntlm/ntlm_buffer_writer.h"
> On 2017/07/20 19:35:46, asanka wrote:
> > why are these necessary? There doesn't seem to be any
> > NtlmBuffer{Reader,Writer}s?
> 
> Done.

[mmenke, 2017-08-01 18:29:35]

On 2017/08/01 18:26:59, mmenke wrote:
> On 2017/08/01 18:21:36, zentaro wrote:
> > I modified it to get the other input fields from the fuzz data.
> > 
> > Now that it pulls everything randomly it seems like my handcrafted messages
> are
> > useless now? Previously they represented relevant challenge messages but now
I
> > don't think I can handcraft a complete input vector. Do they add any value
or
> > should I just delete them?
> 
> You can add them to the dictionary, if you want.  Though you can can still
just
> keep them where they are - they're still used as a seed, so it could still
> stumble across adding headers/trailers to them, and reading them as they're
> intended.  You could run the fuzzer manually for X cycles with them/without
them
> a couple times, and see if it makes a difference.

(And, given that fuzzers run for a long time, I'd recommend you let it run for
30 minutes to an hour per run, to get a very rough idea of how much it matters)

> > This caused it to generate many more (~250) "interesting" inputs running
> > locally. I just want to make sure I modified the fuzzer correctly before I
> > upload them all.
> > 
> > It seems like maybe this change has probably just made it go off exploring
> some
> > unrelated string conversion code since we don't really do much interesting
> with
> > those input strings.
> > 
> > Does it make sense to have 2 fuzzers - one with fixed
> domain/user/password/host
> > and one completely freeform? Or is the more freeform one good enough and it
> will
> > figure it out?
> 
> You may want to ask someone on the fuzzer team (mmoroz?).  If there are only
250
> more interesting inputs, that's small enough that I'm not sure an extra fuzzer
> is worth it.
> 
> >
>
https://codereview.chromium.org/2904633002/diff/690001/net/data/fuzzer_dictio...
> > File net/data/fuzzer_dictionaries/net_ntlm_ntlm_client_fuzzer.dict (right):
> > 
> >
>
https://codereview.chromium.org/2904633002/diff/690001/net/data/fuzzer_dictio...
> > net/data/fuzzer_dictionaries/net_ntlm_ntlm_client_fuzzer.dict:30:
> > "\x03\x00\x00\x00"
> > On 2017/07/20 19:48:27, mmenke wrote:
> > > May want to toss in \x08\x00 and \x00\x00, since they seem to be a common
> > > themes.
> > 
> > Done.
> > 
> >
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client.cc
> > File net/ntlm/ntlm_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client.c...
> > net/ntlm/ntlm_client.cc:136: // Read the flags and the servers random
> challenge
> > from the challenge
> > On 2017/07/20 19:44:50, mmenke wrote:
> > > nit:  server's
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_u...
> > File net/ntlm/ntlm_client_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_u...
> > net/ntlm/ntlm_client_unittest.cc:58: uint8_t raw[sec_buf.length];
> > On 2017/07/20 19:35:46, asanka wrote:
> > > I thought we weren't going to do this? Perhaps std::unique_ptr<uint8_t[]>
> > > instead?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_u...
> > net/ntlm/ntlm_client_unittest.cc:121: raw[16] = 0x00;
> > On 2017/07/20 19:35:46, asanka wrote:
> > > Just for giggles, let's assert that raw[16] wasn't already 0. Same comment
> > > applies to other tests that mutate a buffer.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_unittest.cc
> > File net/ntlm/ntlm_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_unittest...
> > net/ntlm/ntlm_unittest.cc:21: #include "net/ntlm/ntlm.h"
> > On 2017/07/20 19:35:46, asanka wrote:
> > > duplicate
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_unittest...
> > net/ntlm/ntlm_unittest.cc:23: #include "net/ntlm/ntlm_buffer_writer.h"
> > On 2017/07/20 19:35:46, asanka wrote:
> > > why are these necessary? There doesn't seem to be any
> > > NtlmBuffer{Reader,Writer}s?
> > 
> > Done.

[zentaro, 2017-08-02 14:31:23]

Description was changed from

==========
Replace NTLMv1 implementation with a functionally equivalent one.

- Replacement NTLMv1 implementation.
- In preparation for new NTLMv2 implementation.
- Passes all unit tests and has the same behavior.
- Deletes existing code.
- Changes interface to pass std::string instead of void*.
- Adds fuzzer for NtlmClient.


BUG=chromium:22532
TEST=All unit tests pass.
==========

to

==========
Add a functionally equivalent NTLMv1 implementation.

- Replacement NTLMv1 implementation.
- In preparation for new NTLMv2 implementation.
- Passes all unit tests and has the same behavior.
- Adds fuzzer for NtlmClient.
- Code is still not live.

BUG=chromium:22532
TEST=All unit tests pass.
==========

[zentaro, 2017-08-02 15:01:44]

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_f...
File net/ntlm/ntlm_client_fuzzer.cc (right):

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_f...
net/ntlm/ntlm_client_fuzzer.cc:20: net::ntlm::test::kClientChallenge,
net::ntlm::Buffer(data, size));
On 2017/07/20 19:44:50, mmenke wrote:
> Is there any benefit to fuzzing any of the other fields?  Differing challenge
> flags in particular may be interesting.
> 
> It's pretty easy to split up input - see base::FuzzedDataProvider.  It has a
> bunch of options fox fixed-size input.  For variable sized input, 
> ConsumeRandomLengthString is the best way (And if some inputs can't possible
be
> passed in, you can then just check for them and exit the test early, though
that
> will make the fuzzer explore the space of your input validator, too).

Done.

https://codereview.chromium.org/2904633002/diff/690001/net/ntlm/ntlm_client_f...
net/ntlm/ntlm_client_fuzzer.cc:22: }
On 2017/07/20 19:39:56, asanka wrote:
> Given that the library is for explicit credentials, and the credentials
> themselves are provided by extensions and other external sources (including
URL
> embedded credentials currently), we'd need to consider the username/password
to
> also be possibly unsafe inputs.
> 
> As such, we'd need to fuzz the credentials as well in another fuzzer. This one
> could respond to a known valid server challenge with fuzzed credentials.

Done. I think using the fuzzed data provider covers all the combinations of
challenges (and hence challenge flags) and other inputs.

[zentaro, 2017-08-02 15:30:46]

I had to move this to gerrit otherwise the dependency chain gets broken.

Please continue here - https://chromium-review.googlesource.com/c/598307