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

Centralise the TLS specific MD4 code away from the NTLM code #3780

Closed
wants to merge 9 commits into from

Conversation

@captain-caveman2k
Copy link
Member

captain-caveman2k commented Apr 14, 2019

Following curl://up 2019 I've finally started the task of making the NTLM code TLS backend agnostic. As this is a fairly chunky piece of work I have split it up into the following sub tasks:

  • Move the TLS specific MD4 code out of curl_ntlm_core.c
  • Move the TLS specific MD5 code out of curl_ntlm_core.c
  • Move the TLS specific DES implementation out of curl_ntlm_core.c
  • Add our own DES implementation

As you will appreciate, this will probably (more than likely) be spanned across several releases - especially as it has taken me 4 years and several discussion with folk on the curl://team to actually start this :-P

In summary this patch set centralises the MD4 code for the TLS libraries into md4.c but it also:

  • Adds support for MD4, by calling our local implementation, when OpenSSL does not support it
  • Adds support for MD4 in the NTLM code when no crypto libraries are used (No effect yet, given that we still rely on these libraries for MD5 and DES)
  • Allows the NTLM type-3 response message to always include the extended (and more secure) NT response as opposed to just the LM response.
  • Simplifies the MD4 code in curl_ntlm_core.cpp as it replaces the complex pre-processor block with one line of code.
  • Maintains similar code between md4.c and md5.c.

The downsides:

Two TLS backends (SecureTransport and mbed TLS) support a single line function call when creating the MD4 hash. As the Curl_md4it() function implements an OpenSSL style API (calling multiple functions) we have to store the data to be encrypted and as such store that in a temporary buffer. The buffer is malloc'ed and as such will be slower for these two backend libraries.

Concerns:

md4.c and md5.c are a little different, from my point of view, in two areas:

  • The SecureTransport pre-processor directives are different. md4.c (now) uses the USE_SECTRANSP directive, as that is what was used in the NTLM code which has now moved to md4.c. md5.c, on the other hand, uses a few Mac and IOS specific directives. @nickzman
  • The MD5 code, when using OpenSSL, has an Amiga OS specific pre-processor directive present (USE_AMISSL) from #3677. Do we need one in the MD4 code as well? The original NTLM code didn't have this so I'm not sure. @chris-y

Notes:

Whilst I have compiled this on Windows using both OpenSSL and Schannel I cannot compile it for the other TLS, non OpenSSL style, backends (GNU TLS, mbed, SecureTransport, NSS, OS/400). As such I am relying on the automated build system and tests, as well as AppVeyor and Travis CI to let me know if there are any problems. I will of course update this patch set if and when they fail.

@captain-caveman2k captain-caveman2k requested review from bagder and dfandrich Apr 14, 2019
@captain-caveman2k captain-caveman2k force-pushed the captain-caveman2k:md4 branch 2 times, most recently from 9842a91 to ccfdb2b Apr 15, 2019
@chris-y

This comment has been minimized.

Copy link
Contributor

chris-y commented Apr 16, 2019

"The MD5 code, when using OpenSSL, has an Amiga OS specific pre-processor directive present (USE_AMISSL) from #3677. Do we need one in the MD4 code as well? The original NTLM code didn't have this so I'm not sure."

As long as it doesn't try to assign an OpenSSL function to a variable (and, as far as I can tell, it doesn't), you don't need to worry about this.

@captain-caveman2k

This comment has been minimized.

Copy link
Member Author

captain-caveman2k commented Apr 16, 2019

Thank you @chris-y.

bagder added a commit that referenced this pull request Apr 18, 2019
... and disconnect too old ones. The limit is right now set hardcoded to
120 seconds. We can consider offering a configurable value in the
future.

Ref: #3722
Closes #3780
@captain-caveman2k captain-caveman2k force-pushed the captain-caveman2k:md4 branch 6 times, most recently from 547001b to fe13092 Apr 19, 2019
@captain-caveman2k captain-caveman2k force-pushed the captain-caveman2k:md4 branch from fe13092 to 598870c May 4, 2019
@captain-caveman2k captain-caveman2k force-pushed the captain-caveman2k:md4 branch 5 times, most recently from 084c4c7 to bd086c2 May 5, 2019
@captain-caveman2k captain-caveman2k force-pushed the captain-caveman2k:md4 branch 2 times, most recently from aaeaeea to 6fed9aa May 19, 2019
@captain-caveman2k captain-caveman2k force-pushed the captain-caveman2k:md4 branch from 6fed9aa to acd7cdc May 23, 2019
@captain-caveman2k captain-caveman2k force-pushed the captain-caveman2k:md4 branch from acd7cdc to af70997 May 31, 2019
As the NTLM code no longer calls any of TLS libraries' specific MD4
functions, there is no need to call this function for each #ifdef.
…lable
@captain-caveman2k captain-caveman2k force-pushed the captain-caveman2k:md4 branch from af70997 to a010f8b Aug 4, 2019
captain-caveman2k added a commit to captain-caveman2k/curl that referenced this pull request Aug 4, 2019
…lable

Closes curl#3780
@captain-caveman2k captain-caveman2k deleted the captain-caveman2k:md4 branch Aug 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants
You can’t perform that action at this time.