Update selection of type 3 response in ntlm.c #3287

huaraz · 2018-11-18T13:50:32Z

NTLM2 did not work i.e. no NTLMv2 response was created. Changing the check seems to work.

https://winprotocoldoc.blob.core.windows.net/productionwindowsarchives/MS-NLMP/[MS-NLMP].pdf

NTLM2 did not work i.e. no NTLMv2 response was created. Changing the check seems to work. https://winprotocoldoc.blob.core.windows.net/productionwindowsarchives/MS-NLMP/[MS-NLMP].pdf

remove space

huaraz · 2018-11-18T18:13:34Z

Does the Travis-CI log indicate NTLM failures ? It is hard to read/understand the result in the log.

Markus

bagder · 2018-11-19T07:41:26Z

These test cases failed: 2025 2028 2029 2030 2031

Yes, those tests seem to fail now and they all involve NTLM...

huaraz · 2018-11-19T08:10:32Z

I feared that be the case. Would it be possible to get the capture of the NTLM exchange i.e. what were the flags in type 1,2 and 3 for the successful and failed auth attempts ?

Is there an option to test against an AD server which has NTLMv1 / LM disabled ?

bagder · 2018-11-19T10:50:41Z

Is there an option to test against an AD server which has NTLMv1 / LM disabled ?

All curl tests are run either without a test server or with a "synthetic" test server that we build ourselves. This way everyone can build and run the tests and they can run in the CI etc without having to connect anywhere or need any magic accounts. (Also, most curl developers are on non-windows platforms.)

For this particular failure, you've updated bits that curl sends in its NTLM headers without updating the corresponding tests so this is not a surprise. The tests will verify that curl sends exactly what is expected of it!

huaraz · 2018-11-19T11:57:48Z

The change shouldn't break anything for a "old" "insecure" AD server, but add additional functionality to support AD server which have LM and NTLMv1 disabled.

So if the tests fail I need to understand what the synthetic setup looks like to adjust as I have a limited test environment. Is that possible ?

Thank you
Markus

bagder · 2018-11-20T14:05:41Z

Starting with the first test fail, test2025, I think the critical diff is this:

-Authorization: NTLM TlRMTVNTUAADAAAAGAAYAEAAAAAYABgAWAAAAAAAAABwAAAACAAIAHAAAAAIAAgAeAAAAAAAAAAAAAAABoIBAI+/Fp9IERAQ74OsdNPbBpg7o8CVwLSO4DtFyIcZHUMKVktWIu92s2892OVpd2JzqnRlc3R1c2VyY3VybGhvc3Q=[CR][LF]
+Authorization: NTLM TlRMTVNTUAADAAAAGAAYAEAAAAAYABgAWAAAAAAAAABwAAAACAAIAHAAAAAIAAgAeAAAAAAAAAAAAAAABoIBACSb60vUMLShAAAAAAAAAAAAAAAAAAAAAHSb9yQXjz3gIcUWePywK8aoyXJ4KokJ13Rlc3R1c2VyY3VybGhvc3Q=[CR][LF]

If you paste them into a text editor you can see how the headers start differ at position 105. Which isn't a surprise since the code has changed how the Authorization: header is created without updating the tests...

huaraz · 2018-11-20T20:25:38Z

I am sorry I may not have understood correctly before. Are you saying some tests are just string comparisons not functional ?

Markus

bagder · 2018-11-20T22:59:36Z

They do both, but yes I'm saying these five tests all report the string differences as failures. Since they get a different string than they expect!

huaraz · 2018-11-20T23:35:48Z

So that means basically the change works. The tests need to be only adjusted ?

bagder · 2018-11-21T07:54:34Z

If you say so. The custom test server we run really doesn't "know" NTLM, it just plays back whatever we tell it to.

If you want to verify that your code really works and speaks NTLMv2 I trust that you use it against a real server that implements and supports that protocol. Once you know your code works against such a server, then simply updating the strings in the corresponding curl tests should be fine and will make sure we don't regress this going forward.

mkauf · 2018-11-21T10:04:38Z

lib/vauth/ntlm.c

@@ -451,6 +451,7 @@ CURLcode Curl_auth_create_ntlm_type1_message(struct Curl_easy *data,
            LONGQUARTET(NTLMFLAG_NEGOTIATE_OEM |
                        NTLMFLAG_REQUEST_TARGET |
                        NTLMFLAG_NEGOTIATE_NTLM_KEY |
+                        NTLMFLAG_NEGOTIATE_NTLM2_KEY |
                        NTLM2FLAG |


NTLM2FLAG already contains NTLMFLAG_NEGOTIATE_NTLM2_KEY, see line ~408:

#if defined(USE_NTRESPONSES) && defined(USE_NTLM2SESSION) #define NTLM2FLAG NTLMFLAG_NEGOTIATE_NTLM2_KEY #else #define NTLM2FLAG 0 #endif

Oh I overlooked that. Thank you

mkauf · 2018-11-21T10:11:46Z

lib/vauth/ntlm.c

@@ -599,7 +600,7 @@ CURLcode Curl_auth_create_ntlm_type3_message(struct Curl_easy *data,

 #if defined(USE_NTRESPONSES) && defined(USE_NTLM2SESSION)
  /* We don't support NTLM2 if we don't have USE_NTRESPONSES */
-  if(ntlm->flags & NTLMFLAG_NEGOTIATE_NTLM2_KEY) {
+  if(ntlm->flags & NTLMFLAG_NEGOTIATE_NTLM_KEY) {


I don't understand this change. Does this mean that an NTLMv1 message is sent to servers that support NTLMv2 ?

That is the part of the code I am not sure about. The comment is about NTLM2, but the previous section is really the NTLMv2 section i.e. where the correct response is created. This one seem to create only the LM response. ( At least the was the result from the wireshark captures)

Markus

remove unnecessary flag

typing error

huaraz · 2018-11-22T13:06:52Z

Does anybody know which Negotiate flag combination reflects the 6 cases ?

There are six authentication levels.

Send LM & NTLM responses: Clients use LM and NTLM authentication, and never use NTLMv2 session security; DCs accept LM, NTLM, and NTLMv2 authentication.

Send LM & NTLM - use NTLMv2 session security if negotiated: Clients use LM and NTLM authentication, and use NTLMv2 session security if server supports it; DCs accept LM, NTLM, and NTLMv2 authentication.

Send NTLM response only: Clients use NTLM authentication only, and use NTLMv2 session security if server supports it; DCs accept LM, NTLM, and NTLMv2 authentication.

Send NTLMv2 response only: Clients use NTLMv2 authentication only, and use NTLMv2 session security if server supports it; DCs accept LM, NTLM, and NTLMv2 authentication.

Send NTLMv2 response only\refuse LM: Clients use NTLMv2 authentication only, and use NTLMv2 session security if server supports it; DCs refuse LM (accept only NTLM and NTLMv2 authentication).

Send NTLMv2 response only\refuse LM & NTLM: Clients use NTLMv2 authentication only, and use NTLMv2 session security if server supports it; DCs refuse LM and NTLM (accept only NTLMv2 authentication).

Markus

huaraz · 2018-12-01T14:29:27Z

Who can verify that this is correct and can go into a release ?

huaraz · 2018-12-16T21:52:13Z

How can I rerun the tests ? They don't seem to be related to my code changes

Thank you
Markus

MarcelRaad · 2018-12-17T10:19:13Z

I've restarted the failed Travis tests.

huaraz · 2018-12-17T12:32:05Z

It looks like the master branch is broken

make[2]: Entering directory /home/travis/build/curl/curl/tests/unit' CC unit1300-unit1300.o CC ../libtest/unit1300-first.o make[2]: *** No rule to make target ../../src/libcurltool.la', needed by unit1300'. Stop. make[2]: Leaving directory /home/travis/build/curl/curl/tests/unit'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/travis/build/curl/curl/tests'
make: *** [test-nonflaky] Error 2

MarcelRaad · 2018-12-18T08:59:40Z

Actually, at least the first failure is because of:

./vauth/ntlm.c:634:73: warning: Trailing whitespace (TRAILINGSPACE)
     /* NTLM v2 session security is a misnomer because it is not NTLM v2. 
                                                                         ^
./vauth/ntlm.c:635:84: warning: Longer than 79 columns (LONGLINE)
        It is NTLM v1 using the extended session security that is also in NTLM v2 */

Building with --enable-debug (or running checksrc explicitly) will show you these.

huaraz · 2018-12-18T12:31:58Z

Actually, at least the first failure is because of:

./vauth/ntlm.c:634:73: warning: Trailing whitespace (TRAILINGSPACE)
     /* NTLM v2 session security is a misnomer because it is not NTLM v2. 
                                                                         ^
./vauth/ntlm.c:635:84: warning: Longer than 79 columns (LONGLINE)
        It is NTLM v1 using the extended session security that is also in NTLM v2 */

Building with --enable-debug (or running checksrc explicitly) will show you these.

Thank you for the hint. I did not find this in the log.

huaraz · 2018-12-18T13:16:01Z

Not sure what the cause is of the unit1300 error. Looks more like an issue with the testing environment.
I will update against git master branch.

Markus

remove unnecessary flag

typing error

remove unnecessary flag

typing error

huaraz · 2018-12-28T14:16:52Z

Sorry I am too new to git and it seems I misunderstood how "squashing" works. I deleted the patch and recreated a new pull

Markus

NTLM2 did not work i.e. no NTLMv2 response was created. Changing the check seems to work. Ref: https://winprotocoldoc.blob.core.windows.net/productionwindowsarchives/MS-NLMP/[MS-NLMP].pdf Fixes #3286 Closes #3287 Closes #3415

huaraz added 2 commits November 18, 2018 13:48

Update ntlm.c

c0a8b86

NTLM2 did not work i.e. no NTLMv2 response was created. Changing the check seems to work. https://winprotocoldoc.blob.core.windows.net/productionwindowsarchives/MS-NLMP/[MS-NLMP].pdf

Update ntlm.c

b6c6cba

remove space

huaraz changed the title ~~Update ntlm.c~~ Update selection of type 3 response in ntlm.c Nov 18, 2018

huaraz mentioned this pull request Nov 18, 2018

NTLMv2 not fully functional #3286

Closed

mkauf reviewed Nov 21, 2018

View reviewed changes

huaraz added 2 commits November 21, 2018 21:20

Update ntlm.c

3ce207c

remove unnecessary flag

Update ntlm.c

d409b37

typing error

Add comment about NTLM2 session

ecf55a4

Markus Moeller added 3 commits December 16, 2018 14:35

Adjust tests

a214803

Merge remote-tracking branch 'upstream/master' into patch-1

31043d2

Adjust tests

9af4a60

Fix whitespace and long line

a1add86

Markus Moeller and others added 9 commits December 28, 2018 13:06

Adjust tests 2

63bc05c

Adjust tests round 3

585758b

Adjust tests round 4

7890158

remove debug lines

b2ba333

Merge branch 'patch-1' of github.com:huaraz/curl into patch-1

08bc4a4

Update ntlm.c

c484fd2

remove unnecessary flag

Update ntlm.c

18c2ede

typing error

remove debug

f009a41

remove debug

a323dca

huaraz force-pushed the patch-1 branch 3 times, most recently from b6fc986 to cfc5958 Compare December 28, 2018 13:18

Add some info debug

a11f9b5

huaraz force-pushed the patch-1 branch from cfc5958 to a11f9b5 Compare December 28, 2018 13:19

remove debug lines

32b2a11

huaraz force-pushed the patch-1 branch from d72ffc5 to 32b2a11 Compare December 28, 2018 13:21

Markus Moeller and others added 6 commits December 28, 2018 13:21

remove debug lines

78e8263

Merge branch 'patch-1' of github.com:huaraz/curl into patch-1

ea71d0e

Merge branch 'patch-1' of github.com:huaraz/curl into patch-1

a9eeb79

Merge branch 'patch-1' of github.com:huaraz/curl into patch-1

0b52b11

Update ntlm.c

0ed6073

remove unnecessary flag

Update ntlm.c

2afd234

typing error

huaraz force-pushed the patch-1 branch from 344e18f to 2afd234 Compare December 28, 2018 13:31

Markus Moeller added 2 commits December 28, 2018 13:38

Add comment about NTLM2 session

a2974f0

Fix my squash blunder

855c5f1

huaraz closed this Dec 28, 2018

huaraz deleted the patch-1 branch December 28, 2018 14:02

lock bot locked as resolved and limited conversation to collaborators Mar 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update selection of type 3 response in ntlm.c #3287

Update selection of type 3 response in ntlm.c #3287

huaraz commented Nov 18, 2018 •

edited by bagder

Loading

huaraz commented Nov 18, 2018

bagder commented Nov 19, 2018

huaraz commented Nov 19, 2018

bagder commented Nov 19, 2018

huaraz commented Nov 19, 2018

bagder commented Nov 20, 2018

huaraz commented Nov 20, 2018

bagder commented Nov 20, 2018

huaraz commented Nov 20, 2018

bagder commented Nov 21, 2018

mkauf Nov 21, 2018

huaraz Nov 21, 2018

mkauf Nov 21, 2018

huaraz Nov 21, 2018

huaraz commented Nov 22, 2018 •

edited

Loading

huaraz commented Dec 1, 2018

huaraz commented Dec 16, 2018

MarcelRaad commented Dec 17, 2018

huaraz commented Dec 17, 2018

MarcelRaad commented Dec 18, 2018

huaraz commented Dec 18, 2018

huaraz commented Dec 18, 2018 •

edited

Loading

huaraz commented Dec 28, 2018

Update selection of type 3 response in ntlm.c #3287

Update selection of type 3 response in ntlm.c #3287

Conversation

huaraz commented Nov 18, 2018 • edited by bagder Loading

huaraz commented Nov 18, 2018

bagder commented Nov 19, 2018

huaraz commented Nov 19, 2018

bagder commented Nov 19, 2018

huaraz commented Nov 19, 2018

bagder commented Nov 20, 2018

huaraz commented Nov 20, 2018

bagder commented Nov 20, 2018

huaraz commented Nov 20, 2018

bagder commented Nov 21, 2018

mkauf Nov 21, 2018

Choose a reason for hiding this comment

huaraz Nov 21, 2018

Choose a reason for hiding this comment

mkauf Nov 21, 2018

Choose a reason for hiding this comment

huaraz Nov 21, 2018

Choose a reason for hiding this comment

huaraz commented Nov 22, 2018 • edited Loading

huaraz commented Dec 1, 2018

huaraz commented Dec 16, 2018

MarcelRaad commented Dec 17, 2018

huaraz commented Dec 17, 2018

MarcelRaad commented Dec 18, 2018

huaraz commented Dec 18, 2018

huaraz commented Dec 18, 2018 • edited Loading

huaraz commented Dec 28, 2018

huaraz commented Nov 18, 2018 •

edited by bagder

Loading

huaraz commented Nov 22, 2018 •

edited

Loading

huaraz commented Dec 18, 2018 •

edited

Loading