https://chromium.googlesource.com/chromium/src/+/24f0eb870bf48611b6811354d1aa596a9c137c87 looks like the only relevant CL in range, although the uninitialized value was stored in the original CL from 2009... https://chromium.googlesource.com/chromium/src/+/3f918787e1073e17439e55ed34f23ffdc31f891f

Is the new code reachable yet?

I know very little about the NTLM code.

The new implementation is still not live but this code path was part of the old code too (so technically this particular code is reachable).

I took a quick look last night when I saw this and thought it may be a false positive. I'm not sure how exactly the tool checks this and how common that is. 

But I will take a closer look today to try and understand why. A "fix" is fairly obvious - just initialize the 'final' array to zero when it gets created, but I wanted to try and understand why the hash still is valid if it is based on uninitialized memory.

The old code was never fuzzed before though.

You can assign it to me. I don't seem to be able to do that.

I think I see where the uninitialized value is. I'm just trying to figure out why it is apparently not broken. 

The case is where the inputLen param to MD4Sum satisfies - inputLen % 64 >= 56.

The final array is 128 bytes long.

- [line 164] The first loop in MD4Sum takes as many 64 bit chunks as it can.
- [line 168] n = inputLen % 64
- [line 169] memcpy into final from index 0 -> n-1
- [line 170] write final[n] = 0x80 // indices 0 -> n are now initialized
- [line 171] memset to 0 from n+1 for 120 - (n + 1) bytes // indices 0 -> 119 are initialized
- [line 174] inputLen (4 bytes) are written either at indices 56-59 or 120-123
- [line 176] md4step reads at indices 0 -> 63 (all initialized)
- [line 178] when n >= 56 md4step reads at indices 64 -> 127 (but indices 124-127 are not initialized)

In the case where n < 56 bytes 60-63 were initialized by the memset. When n >= 56 the hash could be wrong based on the uninitialized memory.

I found where it got fixed in Firefox - https://bugzilla.mozilla.org/show_bug.cgi?id=496234

So this bug exists in the current code. It just results in the hash being wrong if the uninitialized memory was not 0.

I'm going to add the test suite from https://tools.ietf.org/html/rfc1320 and make the fix. 

Here's the fix and the test suite from the RFC is added. https://chromium-review.googlesource.com/c/602693

This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The impact:

- The bug has existed since the code was introduced (2009)
- The bug and uninitialized read is only triggered when the password used for NTLM is a multiple of 28, 29, 30, or 31 characters long (passwords are unicode) or technically also when longer than 268,435,456 characters (however valid domain passwords are never longer than 256 chars)
- The uninitialized memory is in one of the input blocks to the MD4 hash used only for NTLM authentication.
- The only read/use of the uninitialized memory is as an input to the hash.
- If those 4 bytes in the uninitialized memory were not password_length_in_bytes >> 29 (zero for all practical cases) then the hash would be incorrect.
- If the user had a password of those lengths NTLM authentication could fail.

The bug:

- The block length (in number of bits) is supposed to be written into the final input block as a 64 bit (8 byte) little endian value.
- The code only ever wrote the low 32 bits.
- 2 contiguous 64 byte blocks were used to handle the remaining (length mod 64) bytes of hash input
- The second block is only used when there are more than 56 bytes in the remaining input (since 8 extra bytes are needed to write the length)
- In the case where (length mod 64 < 56) the high 32 bits were always incidentally set to 0 (instead of password_length_in_bytes >> 29) by a memset intended to write the block padding.
- In the case where (length mod 64 >= 56) the high 32 bits are never written leaving 4 bytes of uninitialized memory in the hash input.

The fix:
- Write the length as a 64 bit little endian value.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4e3aae84b8396f6f2548dab5c2580570507b9359

commit 4e3aae84b8396f6f2548dab5c2580570507b9359
Author: Zentaro Kavanagh <zentaro@google.com>
Date: Mon Aug 07 22:35:30 2017

Fix bug in MD4 implementation that creates incorrect hash.

- Inputs with length % 64 >= 56 potentially generate the wrong hash.
- In MD4Sum function the last 4 bytes of the final array were never
  initialized.
- This would result in NTLM failing due to incorrect hash when the
  password length % 32 >= 28 (unicode password).
- Fixes fuzzer bug that was caught by new NTLM fuzzer.
- Added test suite from RFC 1320 for MD4.
- Md4Test.RfcTest6_Mod56CornerCase fails without this fix.

BUG= chromium:752491 

Change-Id: If7b718b8c80f00819200332a027bb2f430b8f68e
Reviewed-on: https://chromium-review.googlesource.com/602693
Commit-Queue: Zentaro Kavanagh <zentaro@google.com>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492432}
[modify] https://crrev.com/4e3aae84b8396f6f2548dab5c2580570507b9359/net/BUILD.gn
[modify] https://crrev.com/4e3aae84b8396f6f2548dab5c2580570507b9359/net/ntlm/md4.cc
[modify] https://crrev.com/4e3aae84b8396f6f2548dab5c2580570507b9359/net/ntlm/md4.h
[add] https://crrev.com/4e3aae84b8396f6f2548dab5c2580570507b9359/net/ntlm/md4_unittest.cc

ClusterFuzz has detected this issue as fixed in range 492405:492438.

Detailed report: https://clusterfuzz.com/testcase?key=5797014427402240

Fuzzer: libFuzzer_net_ntlm_ntlm_client_fuzzer
Job Type: libfuzzer_chrome_msan
Platform Id: linux

Crash Type: Use-of-uninitialized-value
Crash Address: 
Crash State:
  DES_set_key
  net::DESEncrypt
  net::ntlm::GenerateResponseDesl
  
Sanitizer: memory (MSAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=491562:491643
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=492405:492438

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5797014427402240


See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 5797014427402240 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot