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

NTLM: Incorrect MIC offset calculation #1340

Closed
filipnavara opened this issue Feb 23, 2022 · 8 comments
Closed

NTLM: Incorrect MIC offset calculation #1340

filipnavara opened this issue Feb 23, 2022 · 8 comments
Labels
bug Something isn't working

Comments

@filipnavara
Copy link

filipnavara commented Feb 23, 2022

Describe the bug

The NTLM implementation suffers from the same bug as gss-ntlmssp and Heimdal. It incorrectly assumes that the presence, or lack thereof, of the NtlmFlags.NegotiateVersion affects the location of the MIC field in the AUTHENTICATE_MESSAGE packet. That disagrees both with the official specification and the the Wireshark traces when testing against Windows Server 2022.

The code in question:

Notably, I am working on a unit test here and in some subsequent PRs to dotnet/runtime that emulate the server behaviour to unit test the whole authentication exchange. Once the code stabilises a bit it would likely make sense to adapt it in MailKit too.

@filipnavara
Copy link
Author

filipnavara commented Feb 23, 2022

Side notes:

  • SaslMechanismNtlm seems to specify OSVersion on Windows, this propagates into the NTLM NegotiateMessage and upon successful exchange all the way into AuthenticationMessage. As a result it masks the bug on Windows.
  • Unlike gss-ntlmssp if the server unilaterally sets the NegotiateVersion bit in ChallengeMessage without the client asking for it in NegotiateMessage the version is not propagated into AuthenticationMessage. This is in line with the Microsoft specification but it also means that it can actually result in failed authentication in practical scenarios.

@jstedfast
Copy link
Owner

Ah, thanks. I would love to adopt any unit tests you have into MailKit for NTLM.

It was one of the bigger challenges and I never managed to get access to a server that supported NTLM, so I pretty much implemented it blind 😅

@jstedfast
Copy link
Owner

Oh, you work for eM Client? That's awesome!

Do you guys use MimeKit or MailKit at all?

I'm guessing you have your own MIME parser/IMAP/POP3/SMTP implementations, but figured I'd ask.

I was originally thinking of writing a mail client based on MimeKit/MailKit, but I just don't have the time. Would LOVE to be able to point to a mail client that used my libraries 😁

@filipnavara
Copy link
Author

Ah, thanks. I would love to adopt any unit tests you have into MailKit for NTLM.

We were in the same boat. Last week I snapped and implemented a fake server-side part in C# to test against. It's not perfect but it already uncovered bugs in every major NTLM implementation [outside Windows] including buffer overruns. So I started filing bugs...

It was one of the bigger challenges and I never managed to get access to a server that supported NTLM, so I pretty much implemented it blind 😅

We run two test servers in Azure that I would be happy to share access to. The problem is that it doesn't cover all scenarios (domain vs. server authentication) and one of the servers routinely crashes (and needs manual restart). Moreover, at the moment they only serve HTTP requests and not the full Exchange installation. The Exchange machines bit rot way too easily and we didn't have anyone to maintain them. I cannot promise anything at the moment but I will try to get the infrastructure more reliable so it can be used for testing by external projects. Ideally, we should figure out a way to share the credentials for private use or use in GitHub Actions CI (through the GitHub Secrets).

Oh, you work for eM Client? That's awesome!
Do you guys use MimeKit or MailKit at all?
I'm guessing you have your own MIME parser/IMAP/POP3/SMTP implementations, but figured I'd ask.

We use our own libraries, mostly because the original code predates MimeKit and MailKit by a couple of years. That said, we do use MimeKit/MailKit in some ancillary tools and we run some compatibility tests to ensure we don't break on emails produced by MimeKit or vice-versa.

@jstedfast
Copy link
Owner

It's not perfect but it already uncovered bugs in every major NTLM implementation [outside Windows] including buffer overruns. So I started filing bugs...

Thanks for doing that work! I'm sure all of these open source projects appreciate it. I know I do!

Getting NTLM correct was not trivial (and it looks like I still got stuff wrong!).

We run two test servers in Azure that I would be happy to share access to.

What I currently do for my unit tests is to run the code against a dummy set of server responses. Where a real server comes in handy, though, is testing that initial implementation and creating those initial "dummy" server responses to make sure the implementation works against a real-world server. The dev-loop cycle is so much faster when you can test yourself rather than having to create a package, asking someone with a server that supports NTLM to test it out, and then trying to diagnose the issue based on "it doesn't work" :-)

I cannot promise anything at the moment but I will try to get the infrastructure more reliable so it can be used for testing by external projects.

No worries if you can't.

We use our own libraries, mostly because the original code predates MimeKit and MailKit by a couple of years.

I just looked up eM Client on WIkipedia and it looks like eM Client was started back in 2007. So yea, a "few" years! MimeKit wasn't born until fall of 2013 and MailKit wasn't even started until early 2014 or so, iirc, so that's a good 7 years.

I should have gotten off my butt sooner to write MimeKit/MailKit :-)

That said, we do use MimeKit/MailKit in some ancillary tools and we run some compatibility tests to ensure we don't break on emails produced by MimeKit or vice-versa.

That's still a good consolation prize :-)

@filipnavara
Copy link
Author

Getting NTLM correct was not trivial (and it looks like I still got stuff wrong!).

I got it wrong too, twice. 😅 First in one implementation that I wrote years ago and now again in a one that I based on someone else's code.

The MIC offset calculation seems to be broken in every implementation I checked so far (5 of them + Wireshark analyzer). I am even starting to doubt myself whether I performed the check correctly. However, I did Wireshark trace against real Windows installation and double-checked the specification (+ errata). In most cases that bug is masked by some other implementation detail though.

Given the fact that I could have possibly missed something, I recommend proceeding with extra scrutiny. The safe way to fix the problem is to always start with negotiating with the NtlmFlags.NegotiateVersion flag. Servers that support MIC also support this flag and will reply with appropriate challenge. The authenticate message will then contain the version field as well and the MIC field will be at the correct offset [per the specification] regardless of calculation used.

@jstedfast jstedfast added the bug Something isn't working label Feb 23, 2022
jstedfast added a commit that referenced this issue Feb 25, 2022
Based on issue #1340 (and my re-reading of the spec)
@jstedfast
Copy link
Owner

I re-read the spec the other night and it does look like you are correct about always including the block of 8 bytes for the version field (even if not populated).

I've made a commit to do that, but I'm going to keep this open for a bit longer to remind me to check up on the dotnet/runtime link unit test discussion here: dotnet/runtime#65611

Looks like there's now discussion of setting up a SMTP server for testing purposes - whether I try to hop onto that train or take your unit tests and modify them for inclusion in MailKit, I'm not sure yet - but either way, I want better tests for MailKit's NTLM support.

@filipnavara
Copy link
Author

Thanks, as of today gss-ntlmssp and Wireshark were also fixed.

I'll notify you when we move forward with the testing infrastructure.

Elanis pushed a commit to Elanis/portfolio that referenced this issue Dec 13, 2022
Bumps [MailKit](https://github.com/jstedfast/MailKit) from 3.1.1 to 3.2.0.
<details>
<summary>Changelog</summary>

*Sourced from [MailKit's changelog](https://github.com/jstedfast/MailKit/blob/master/ReleaseNotes.md).*

> ### MailKit 3.2.0 (2022-03-26)
>
> * Do not use ApplicationProtocols with SSL. (issue [#1352](jstedfast/MailKit#1352))
> * Updated GMail, Yahoo, and Outlook.com certificates.
> * Lazy-initialize MessageSummary.Keywords. This reduces memory usage when the client isn't requesting Flags/Keywords.
> * Hard-cache some IMAP FETCH-related tokens in order to relieve GC pressure for commands like FETCH where there can
>   be a LOT of responses containing the same tokens over and over again.
> * Converted some IMAP async Task methods to use ValueTask to reduce GC pressure.
> * Reduced string allocations in the IMAP logic by avoiding use of ToUpperInvariant().
> * Added non-async implementations for ImapStream APIs to be used by the synchronous public APIs to avoid some async overhead.
> * Reduce MemoryStream (and thus byte[]) allocations by using a new ByteArrayBuilder.
> * Rewrote the IMAP CAPABILITY parser to avoid allocating strings.
> * Fixed some cases where IMAP NIL tokens were not compared case insensitively.
> * Always include the VERSION block in NTLM messages. (issue [#1340](jstedfast/MailKit#1340))
> * Target .NET Framework v4.6.1 instead of v4.6 to match the changes in MimeKit.
> * Capture the Socket timeout value in Read/WriteAsync() to have it in case of exceptions.
>   (issue [#1327](jstedfast/MailKit#1327))
</details>
<details>
<summary>Commits</summary>

- [`77eb925`](jstedfast/MailKit@77eb925) Bumped version to 3.2.0
- [`1af5110`](jstedfast/MailKit@1af5110) Do not use ApplicationProtocols with SSL
- [`73c6f67`](jstedfast/MailKit@73c6f67) Updated GMail and Yahoo certificates
- [`c3a48ac`](jstedfast/MailKit@c3a48ac) Bump nunit from 3.13.2 to 3.13.3 ([#1350](jstedfast/MailKit#1350))
- [`a736d88`](jstedfast/MailKit@a736d88) Lazy-initialize MessageSummary.Keywords
- [`ab1e087`](jstedfast/MailKit@ab1e087) Use token.Value.ToString() in case the token is a char token
- [`16adde1`](jstedfast/MailKit@16adde1) Updated GMail certificates
- [`d284a2d`](jstedfast/MailKit@d284a2d) Hard-cache some IMAP FETCH-related tokens
- [`cfe6dba`](jstedfast/MailKit@cfe6dba) Always include the VERSION block in NTLM messages.
- [`cfa66e4`](jstedfast/MailKit@cfa66e4) Convert more async IMAP methods to ValueTask
- Additional commits viewable in [compare view](jstedfast/MailKit@3.1.1...3.2.0)
</details>

<br />

Reviewed-on: https://gitea.dysnomia.studio/elanis/portfolio/pulls/31
Co-authored-by: elanis <elanis@noreply.example.org>
Co-committed-by: elanis <elanis@noreply.example.org>
Elanis added a commit to Dysnomia-Studio/dysnomia-website that referenced this issue Jul 14, 2023
Bumps [MailKit](https://github.com/jstedfast/MailKit) from 3.1.1 to 3.2.0.
<details>
<summary>Changelog</summary>

*Sourced from [MailKit's changelog](https://github.com/jstedfast/MailKit/blob/master/ReleaseNotes.md).*

> ### MailKit 3.2.0 (2022-03-26)
>
> * Do not use ApplicationProtocols with SSL. (issue [#1352](jstedfast/MailKit#1352))
> * Updated GMail, Yahoo, and Outlook.com certificates.
> * Lazy-initialize MessageSummary.Keywords. This reduces memory usage when the client isn't requesting Flags/Keywords.
> * Hard-cache some IMAP FETCH-related tokens in order to relieve GC pressure for commands like FETCH where there can
>   be a LOT of responses containing the same tokens over and over again.
> * Converted some IMAP async Task methods to use ValueTask to reduce GC pressure.
> * Reduced string allocations in the IMAP logic by avoiding use of ToUpperInvariant().
> * Added non-async implementations for ImapStream APIs to be used by the synchronous public APIs to avoid some async overhead.
> * Reduce MemoryStream (and thus byte[]) allocations by using a new ByteArrayBuilder.
> * Rewrote the IMAP CAPABILITY parser to avoid allocating strings.
> * Fixed some cases where IMAP NIL tokens were not compared case insensitively.
> * Always include the VERSION block in NTLM messages. (issue [#1340](jstedfast/MailKit#1340))
> * Target .NET Framework v4.6.1 instead of v4.6 to match the changes in MimeKit.
> * Capture the Socket timeout value in Read/WriteAsync() to have it in case of exceptions.
>   (issue [#1327](jstedfast/MailKit#1327))
</details>
<details>
<summary>Commits</summary>

- [`77eb925`](jstedfast/MailKit@77eb925) Bumped version to 3.2.0
- [`1af5110`](jstedfast/MailKit@1af5110) Do not use ApplicationProtocols with SSL
- [`73c6f67`](jstedfast/MailKit@73c6f67) Updated GMail and Yahoo certificates
- [`c3a48ac`](jstedfast/MailKit@c3a48ac) Bump nunit from 3.13.2 to 3.13.3 ([#1350](jstedfast/MailKit#1350))
- [`a736d88`](jstedfast/MailKit@a736d88) Lazy-initialize MessageSummary.Keywords
- [`ab1e087`](jstedfast/MailKit@ab1e087) Use token.Value.ToString() in case the token is a char token
- [`16adde1`](jstedfast/MailKit@16adde1) Updated GMail certificates
- [`d284a2d`](jstedfast/MailKit@d284a2d) Hard-cache some IMAP FETCH-related tokens
- [`cfe6dba`](jstedfast/MailKit@cfe6dba) Always include the VERSION block in NTLM messages.
- [`cfa66e4`](jstedfast/MailKit@cfa66e4) Convert more async IMAP methods to ValueTask
- Additional commits viewable in [compare view](jstedfast/MailKit@3.1.1...3.2.0)
</details>

<br />

Co-authored-by: Elanis <elanis@hotmail.com>
Reviewed-on: https://gitea.dysnomia.studio/elanis/dysnomia-website/pulls/35
Co-authored-by: elanis <elanis@noreply.example.org>
Co-committed-by: elanis <elanis@noreply.example.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants