Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix - NTLMv1 DES key expansion #2

Merged
merged 6 commits into from
Sep 28, 2015

Conversation

Rican7
Copy link
Contributor

@Rican7 Rican7 commented Sep 26, 2015

After 10+ hours of debugging, I finally found why NTLMv1 seemed to be failing... and reason is incredibly stupid and frustrating.

The DES encryption algorithm that is used in the NTLM protocol is a non-standard algorithm, in that it uses a special process to "expand" 56-bit (7-byte) keys to 64-bit (8-byte) keys. While implementing the LM hashing algorithm originally, I made sure to include that expansion process. Unfortunately, I had made the original LM hasher way before putting together all of the other pieces that integrate to create a full NTLMv1 "Authenticate Message" encoding process. Because I had spent so much time in between those implementations, I didn't remember to include that special key expansion and normalization process when using the DES encrypter to generate part of the LM/NT responses.

This is one of those "bugs" that is caused by oversight when fleshing out an implementation. Damn.

Anyway, in order to fix this bug, I've simply abstracted the key expansion bits into a common key processing piece that gets optionally called for all DES encryption processes, since that's the way that DES encryption is always used in the NTLM protocol.

Also, I had forgotten a small part of the processing originally that is intended to normalize keys by setting a parity bit for each byte in a DES key. Thanks to some prior art (referenced in the method doc-block), that is now implemented.

Finally, thanks to some integration testing, I another oversight in the design of the LM hashing and DES encryption usage: I was generating a random "initialization vector" and providing it to the DES encrypter when using an ECB cipher mode... which doesn't make any sense, as the DES-ECB encryption cipher doesn't use a provided initialization vector (oops!). So, in order to "fix" that unnecessary operation, I've simply removed the old initialization vector generation when using DES-ECB. When doing so, I realized that the LmHasher no longer needed the RandomByteGeneratorInterface dependency, as it was no longer being used, so I removed it! The nice thing about this removal is that we'll no longer be generating random bytes when we don't need to, which will allow us to remove unnecessary calls that were actually taking up I/O time and entropy, so this is an optimization all around too. 😄

Do to these new abstractions and dependency removals, the changes in this PR are actually breaking backwards compatibility. Luckily the library is still in a pre-1.0 "unstable" version phase, so a BC break like this is allowed (and somewhat expected).

Anyway, I'm beat now. My past several hours have had so much reverse engineering through message decoding that I'm absolutely toast. 🍞 So I'm out for tonight.

PS: I've done some integration tests, and now both the NTLMv1 and NTLMv2 processes work against an Exchange 2010 server running on Windows Server 2008. 😃

@jstruzik
Copy link
Member

👍 This can be merged. Good work on this, it can be quite frustrating when something so minor can make a complex system unravel.

@Rican7
Copy link
Contributor Author

Rican7 commented Sep 28, 2015

Thanks @jstruzik!

Rican7 added a commit that referenced this pull request Sep 28, 2015
@Rican7 Rican7 merged commit 5da8f81 into robinpowered:master Sep 28, 2015
@Rican7 Rican7 deleted the bugfix/ntlmv1-des-key-expansion branch September 28, 2015 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants