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: support version 2 on 32-bit platforms and fix negotiated flags usage #6849

Closed
wants to merge 3 commits into from

Conversation

@monnerat
Copy link
Contributor

@monnerat monnerat commented Apr 4, 2021

This does not affect sspi builds.

I successfully performed as many tests as my non-M$ environment allows to, but this must be tested against a genuine M$ server, although I'm quite confident in it.

Thanks for considering it.

@monnerat monnerat force-pushed the monnerat:ntlm branch 2 times, most recently from 5dc6fda to ea6fd4f Apr 4, 2021
unsigned char ntbuffer[0x18];
unsigned char tmp[0x18];
unsigned char md5sum[CURL_MD5_DIGEST_LENGTH];
unsigned char md5sum[MD5_DIGEST_LENGTH];

This comment has been minimized.

@bagder

bagder Apr 5, 2021
Member

Why this change? We provide our own define to make sure it exists under a single name. There's no guarantee there's a MD5_DIGEST_LENGTH define provided in systems! See d80d419 that introduced our name.

This comment has been minimized.

@monnerat

monnerat Apr 5, 2021
Author Contributor

You're right, thanks for pointing it.
I intended to replace by MD5_DIGEST_LEN which is defined in curl_md5.h. Will fix it.

This comment has been minimized.

@monnerat

monnerat Apr 5, 2021
Author Contributor

Fixed by using MD5_DIGEST_LEN.

This comment has been minimized.

@bagder

bagder Apr 5, 2021
Member

Do you remove the CURL_MD5_DIGEST_LENGTH define as well then?

This comment has been minimized.

@monnerat

monnerat Apr 5, 2021
Author Contributor

Yes, because it was local to vauth/ntlm.c.

@bagder
Copy link
Member

@bagder bagder commented Apr 5, 2021

this must be tested against a genuine M$ server

We need to figure out how to get someone to run such tests! I don't have any such servers/services to test against and I don't know who does! 🤔

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Apr 5, 2021

We need to figure out how to get someone to run such tests! I don't have any such servers/services to test against and I don't know who does!

We're both linux guys, that's why we are in the same situation.
If the initial ntlm developer is still around, maybe we should ask him/her for a review. Else a W$ contributor.

I would prefer not to have it committed for "testing by the community".
One thing is sure: current flag usage is wrong!

@monnerat monnerat force-pushed the monnerat:ntlm branch from ea6fd4f to 9135bd5 Apr 5, 2021
@bagder
Copy link
Member

@bagder bagder commented Apr 5, 2021

If the initial ntlm developer is still around, maybe we should ask him/her for a review. Else a W$ contributor.

I wrote the initial NTLM implementation in curl...

One thing is sure: current flag usage is wrong!

... I believe you, but still it has been right enough to work pretty well for a lot of users over the years.

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Apr 5, 2021

I wrote the initial NTLM implementation in curl...

Congratulations! I guess it was not that easy when no official documentation was released.
And how did you test it?

@bagder
Copy link
Member

@bagder bagder commented Apr 5, 2021

Other users at the time tested it against actual NTLM-using servers before I wrote up the test cases that verifiy that we maintain the same behavior.

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Apr 6, 2021

Other users at the time tested it against actual NTLM-using servers before I wrote up the test cases that verifiy that we maintain the same behavior.

I had to adapt the tests as the NTLM response in type 3 message changes. I did it manually, checking that only the targeted message part was affected.

I guess NTLM is not much used nowadays.

@bagder
Copy link
Member

@bagder bagder commented Apr 6, 2021

I guess NTLM is not much used nowadays.

According to our user survey 2020, 9.9% of our users use it, down just a little from 10.3% the year before. I think that's a pretty sizable share after all.

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Apr 6, 2021

9.9% of our users use it

Maybe we should call them for evaluation, via the mailing list for example ?

@MarcelRaad
Copy link
Member

@MarcelRaad MarcelRaad commented Apr 6, 2021

FWIW, I'm one of those who use NTLM (via SSPI though), but NTLMv2 only would suffice for me.

I can try to find a test server and test this patch.

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Apr 6, 2021

Many thanks Marcel,
In the meantime, I managed to setup a minimal IIS7 for testing.
The first tests results with it are not satisfactory, thus I think we should put it on hold for a few days.
I'll come back to your kind proposal when I feel it more mature.

monnerat added 3 commits Apr 2, 2021
... as !defined(CURL_DISABLE_CRYPTO_AUTH) is a prerequisite for the
whole NTLM.
According to Microsoft document MS-NLMP, current flags usage is not
accurate: flag NTLMFLAG_NEGOTIATE_NTLM2_KEY controls the use of
extended security in an NTLM authentication message and NTLM version 2
cannot be negotiated within the protocol.

The solution implemented here is: if the extended security flag is set,
prefer using NTLM version 2 (as a server featuring extended security
should also support version 2). If version 2 has been disabled at
compile time, use extended security.

Tests involving NTLM are adjusted to this new behavior.

Fixes #6813
@monnerat monnerat force-pushed the monnerat:ntlm branch from 9135bd5 to 100e096 Apr 6, 2021
@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Apr 7, 2021

OK, I performed additional test with IIS7 and it works.

Here is a table of what is used depending on curl and server capabilities:

curl \ server V1ext V1ext + V2 V2 V1 only
V1ext V1ext V1ext V1 V1
V1ext + V2 V2 ! V2 V1 V1
V2 V2 ! V2 V1 V1
V1 only V1 V1 V1 V1

Entries marked with ! are wrong and will fail, but these situations will probably never occur.
V2 cannot be negotiated: this explains oddities in this table.
By default curls supports V2 and V1ext (this can be changed by altering defines in lib/curl_ntlm_core.h).
Of course, this can fail if the security level used is not high enough for the server.
This should at least be compatible with M$ servers, samba and saslauthd.

It should be ready now.

@MarcelRaad
Copy link
Member

@MarcelRaad MarcelRaad commented Apr 7, 2021

The V2 column looks strange. Shouldn't the V1ext + V2 and V2 rows contain V2 instead of V1? And probably the V1ext row V1ext?

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Apr 7, 2021

Shouldn't the V1ext + V2 and V2 rows contain V2 instead of V1?

No because as V2 cannot be negotiated, curl does not see server supports V2. We thus assume that V2 is supported only if V1ext is. IMHO this is the safest way.

@MarcelRaad
Copy link
Member

@MarcelRaad MarcelRaad commented Apr 7, 2021

OK, makes sense. I thought that V1ext + V2 looked the same to the client instead of V1 + V2.

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Apr 7, 2021

I thought that V1ext + V2 looked the same to the client instead of V1 + V2.

No. In fact a V2-aware server does not care about the extended security flag when determining V2 is used: it only checks the NTLM response length.
The unknown here is: is the server V2-aware ?

The only other solution to the problem is to never use V2 (this will also remove a lot of code), but I don't think you will feel satisfied with it :-)

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Apr 8, 2021

Thanks for your review and work on this, Marcel.

@bagder
Copy link
Member

@bagder bagder commented Apr 9, 2021

Thanks!

@bagder bagder closed this in 10514d0 Apr 9, 2021
bagder added a commit that referenced this pull request Apr 9, 2021
Closes #6849
bagder added a commit that referenced this pull request Apr 9, 2021
According to Microsoft document MS-NLMP, current flags usage is not
accurate: flag NTLMFLAG_NEGOTIATE_NTLM2_KEY controls the use of
extended security in an NTLM authentication message and NTLM version 2
cannot be negotiated within the protocol.

The solution implemented here is: if the extended security flag is set,
prefer using NTLM version 2 (as a server featuring extended security
should also support version 2). If version 2 has been disabled at
compile time, use extended security.

Tests involving NTLM are adjusted to this new behavior.

Fixes #6813
Closes #6849
@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Apr 9, 2021

Thanks for pulling.

@monnerat monnerat deleted the monnerat:ntlm branch Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants