Fixes for HTTP proxy authentication with NTLM
Hello,
one of our customers reported that the NTLM authentication of OpenVPN doesn't work. I checked that and found that both NTLM versions didn't work against a Win2008 server. The configuration seemed a bit confusing because an 'auth-method' could only be set if an 'authfile' was given (ok, 'stdin' worked also).
When I inspected the code I noticed that both auto-modes for authentication and the retry mechanism were handled by one single variable (p->options.auth_retry). In order to fix the general working and to straighten the configuration and internal handling I created two patches:
[PATCH 1/2] Get NTLMv1 and NTLMv2 up and running
This patch fixes several problems found in the ntlm-code:
Apply UTF-16 conversion where needed, introduce a proper conversion from UTF-8 to UTF-16 (the former one worked only for ASCII chars), fix ntlm2 (the assumed length for the ntlm2-blob was wrong).
[PATCH 2/2] http-proxy: Separate settings for user interaction and authentication auto-behavior
Here a backward compatible re-structuring of the configuration for 'http-proxy' is
introduced: The interact setting (whether the user is prompted for the credentials again after an auth error) is put into a new keyword. This is now disjoined from the auth method. The new syntax is like:
http-proxy server port [authfile | 'interact'] [ auth-method ]
auth-method := auto | auto-nct | basic | digest | ntlm | ntlm2
The behavior of auth-method == auto or auto-nct works as follows:
If the server requests an authentication method the credentials are asked from the user.
The new behaviour treats the server request for 'NTLM' in the following way:
The NTLMv2 algorithm is tried first (in order to push higher security). Only if this fails NTLMv1 will be used. If this also fails the user is asked for the credentials again depending on the configuration.
Any opinions?
Best regards,
Holger
PATCH 1/2 Get NTLMv1 and NTLMv2 up and running
* Force conversion to UTF-16 of username and domain if server requires UTF-16.
* Rewrite conversion function to cleanly convert UTF-8 to UTF-16.
* Fix bug in length computation in NTLMv2-code.
* Architecture independent access to NTLM NegotiateFlags.
--- src/openvpn/ntlm.c | 156 +++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 134 insertions(+), 22 deletions(-)
src/openvpn/ntlm.c +134 -22
@@ -56,6 +56,11 @@#endif +#define NTLM_NEGOTIATE_UNICODE (1<<0) +#define NTLM_NEGOTIATE_OEM (1<<1) +#define NTLM_NEGOTIATE_NTLM2_KEY (1<<19) +#define NTLM_NEGOTIATE_TARGET_INFO (1<<23) + static void
@@ -79,8 +84,10 @@ gen_md4_hash (const char* data, int data_len, char *result)const md_kt_t *md4_kt =md_kt_get ("MD4" );char md[MD4_DIGEST_LENGTH ]; -md_full (md4_kt, data, data_len, md); -memcpy (result, md,MD4_DIGEST_LENGTH ); +if (data_len >=0 ) { +md_full (md4_kt, data, data_len, md); +memcpy (result, md,MD4_DIGEST_LENGTH ); + } } static void
@@ -140,18 +147,89 @@ unsigned char *my_strupr(unsigned char *str) }static int -unicodize (char *dst,const char *src) +utf8_to_utf16LE (unsigned char *dst,int dst_len,unsigned char *src) { -/* not really unicode... */ -int i =0 ; -do +/* Convert UTF-8 to UTF-16 little endian +* +* http://clang.llvm.org/doxygen/ConvertUTF_8c_source.html +*/ +unsigned char *end = src +strlen (src), + *dst_start = dst; +const char trailingBytesForUTF8[256 ] = { +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 , +2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,3 ,3 ,3 ,3 ,3 ,3 ,3 ,3 ,4 ,4 ,4 ,4 ,5 ,5 ,5 ,5 + }; +const unsigned int offsetsUtf8[6 ] = {0x00000000UL ,0x00003080UL ,0x000E2080UL , +0x03C82080UL ,0xFA082080UL ,0x82082080UL }; +const unsigned int uni_max_utf16 =0x0010FFFF , + uni_half_base =0x00010000 , + uni_half_mask =0x000003FF , + uni_half_shift =10 , + uni_sur_high_start =0xD800 , + uni_sur_low_start =0xDC00 , + uni_sur_low_end =0xDFFF ; + +while (src < end) { - dst[i++] = *src; - dst[i++] =0 ; +unsigned int ch =0 , + extraBytes = trailingBytesForUTF8[*src]; + +if (extraBytes >= end - src) +return -1 ; + +/* The cases all fall through. */ +switch (extraBytes) { +case 5 : ch += *src++; ch <<=6 ; +case 4 : ch += *src++; ch <<=6 ; +case 3 : ch += *src++; ch <<=6 ; +case 2 : ch += *src++; ch <<=6 ; +case 1 : ch += *src++; ch <<=6 ; +case 0 : ch += *src++; + } + ch -= offsetsUtf8[extraBytes];/* clean up UTF-8 control bits */ + +/* Now 'ch' contains the codepoint of the UTF-8-character. */ + +if (ch <=0xFFFF ) + { +if (ch >= uni_sur_high_start && ch <= uni_sur_low_end) { +msg (M_INFO ,"Warning: Illegal character value: %x is in reserved area of UTF-16 [%x, %x]" , uni_sur_high_start, uni_sur_low_end); +return -1 ; + } +else {/* normal case */ +if (dst_len - (dst - dst_start) <2 ) { +msg (M_INFO ,"Warning: To less space in destination buffer for conversion to UTF-16" ); +return -1 ; + } + *dst++ = ch &0xff ; + *dst++ = (ch >>8 ) &0xff ; + } + } +else if (ch > uni_max_utf16) { +msg (M_INFO ,"Warning: Illegal character value :%x is too large for UTF-16" , ch); +return -1 ; + } +else { +/* character is in range 0xFFFF - 0x10FFFF. */ +if (dst_len - (dst - dst_start) <4 ) { +msg (M_INFO ,"Warning: To less space in destination buffer for conversion to UTF-16" ); +return -1 ; + } + ch -= uni_half_base; + *dst++ = ( (ch >> uni_half_shift) + uni_sur_high_start) &0xff ; + *dst++ = (((ch >> uni_half_shift) + uni_sur_high_start) >>8 ) &0xff ; + *dst++ = ( (ch & uni_half_mask) + uni_sur_low_start) &0xff ; + *dst++ = (((ch & uni_half_mask) + uni_sur_low_start) >>8 ) &0xff ; + } } -while (*src++); -return i; +return dst - dst_start; } static void
@@ -195,10 +273,12 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_archar pwbuf[sizeof (p->up.password) *2 ];/* for unicode password */ char buf2[128 ];/* decoded reply from proxy */ unsigned char phase3[464 ]; +unsigned long ntlm_negotiate_flags;char md4_hash[MD4_DIGEST_LENGTH +5 ];char challenge[8 ], ntlm_response[24 ];int i, ret_val; +int ntlm_unicode;char ntlmv2_response[144 ];char userdomain_u[256 ];/* for uppercase unicode username and domain */
@@ -211,7 +291,9 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_arsize_t len;char domain[128 ]; -char username[128 ]; +char domain_u[256 ]; +char username[USER_PASS_LEN ]; +char username_u[USER_PASS_LEN *2 ];char *separator;bool ntlmv2_enabled = (p->auth_method ==HTTP_AUTH_NTLM2 );
@@ -238,7 +320,7 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar/* fill 1st 16 bytes with md4 hash, disregard terminating null */ -gen_md4_hash (pwbuf,unicodize (pwbuf, p->up.password) -2 , md4_hash); +gen_md4_hash (pwbuf,utf8_to_utf16LE (pwbuf,sizeof (p->up.password) *2 , (unsigned char *)p->up.password), md4_hash);/* pad to 21 bytes */ memset (md4_hash +MD4_DIGEST_LENGTH ,0 ,5 );
@@ -250,23 +332,36 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar/* we can be sure that phase_2 is less than 128 * therefore buf2 needs to be (3/4 * 128) */ +/* extract NegotiateFlags from bytes 20-23 */ + ntlm_negotiate_flags = (buf2[0x14 ] &0xff ) | (buf2[0x15 ] &0xff ) <<8 | (buf2[0x16 ] &0xff ) <<16 | (buf2[0x17 ] &0xff ) <<24 ; + + ntlm_unicode = ntlm_negotiate_flags &NTLM_NEGOTIATE_UNICODE ?1 :0 ; +/* extract the challenge from bytes 24-31 */ for (i=0 ; i<8 ; i++) { challenge[i] = buf2[i+24 ]; } +msg (M_DEBUG ,"Received NTLM negotiate flags 0x%08x: %s encoding, %sNTLM2sec, %sTargetInfo - doing NTLMv%s" , + ntlm_negotiate_flags, ntlm_unicode ?"Unicode" :"OEM" , + ntlm_negotiate_flags &NTLM_NEGOTIATE_NTLM2_KEY ?"" :"no " , + ntlm_negotiate_flags &NTLM_NEGOTIATE_TARGET_INFO ?"" :"no " , + ntlmv2_enabled ?"2" :"1" ); +if (ntlmv2_enabled){/* Generate NTLMv2 response */ int tib_len;/* NTLMv2 hash */ -my_strupr ((unsigned char *)strcpy (userdomain, username)); +my_strupr ((unsigned char *)strcpy (userdomain, username));if (strlen (username) +strlen (domain) <sizeof (userdomain))strcat (userdomain, domain);else msg (M_INFO ,"Warning: Username or domain too long" ); -unicodize (userdomain_u, userdomain); -gen_hmac_md5 (userdomain_u,2 *strlen (userdomain), md4_hash,MD5_DIGEST_LENGTH , ntlmv2_hash); + len =utf8_to_utf16LE (userdomain_u,sizeof (userdomain_u), userdomain); +if (len >=0 ) { +gen_hmac_md5 (userdomain_u, len, md4_hash,MD5_DIGEST_LENGTH , ntlmv2_hash); + }/* NTLMv2 Blob */ memset (ntlmv2_blob,0 ,128 );/* Clear blob buffer */
@@ -278,8 +373,8 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar ntlmv2_blob[0x18 ]=0 ;/* Unknown, zero should work */ /* Add target information block to the blob */ -if (( *((long *)&buf2[0x14 ]) &0x00800000 ) ==0x00800000 ){/* Check for Target Information block */ - tib_len = buf2[0x28 ];/* Get Target Information block size */ +if (ntlm_negotiate_flags &NTLM_NEGOTIATE_TARGET_INFO ){/* Check for Target Information block */ + tib_len = buf2[0x28 ] &0xff | (buf2[0x29 ] &0xff ) <<8 ;/* Get Target Information block size */ if (tib_len >96 ) tib_len =96 ; {char *tib_ptr = buf2 + buf2[0x2c ];/* Get Target Information block pointer */
@@ -292,7 +387,7 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar ntlmv2_blob[0x1c + tib_len] =0 ;/* Unknown, zero works */ /* Get blob length */ - ntlmv2_blob_size =0x20 + tib_len; + ntlmv2_blob_size =0x1c + tib_len;/* Add challenge from message 2 */ memcpy (&ntlmv2_response[8 ], challenge,8 );
@@ -328,11 +423,28 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_aradd_security_buffer (0x14 , ntlm_response,24 , phase3, &phase3_bufpos); } -/* username in ascii */ -add_security_buffer (0x24 , username,strlen (username), phase3, &phase3_bufpos); +/* Set username. */ +if (ntlm_unicode) { + len =utf8_to_utf16LE (username_u,sizeof (username_u), username); +if (len>=0 ) { +add_security_buffer (0x24 , username_u, len, phase3, &phase3_bufpos); + } + } +else { +add_security_buffer (0x24 , username,strlen (username), phase3, &phase3_bufpos); + } +/* Set domain. If <domain> is empty, default domain will be used (i.e. proxy's domain) */ -add_security_buffer (0x1c , domain,strlen (domain), phase3, &phase3_bufpos); +if (ntlm_unicode) { + len =utf8_to_utf16LE (domain_u,sizeof (domain_u), domain); +if (len>=0 ) { +add_security_buffer (0x1c , domain_u, len, phase3, &phase3_bufpos); + } + } +else { +add_security_buffer (0x1c , domain,strlen (domain), phase3, &phase3_bufpos); + }/* other security buffers will be empty */
@@ -341,7 +453,7 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar phase3[0x38 ] = phase3_bufpos;/* no session key */ /* flags */ - phase3[0x3c ] =0x02 ;/* negotiate oem */ + phase3[0x3c ] = ntlm_unicode ?0x01 :0x02 ;/* negotiate unicode/oem */ phase3[0x3d ] =0x02 ;/* negotiate ntlm */ return ((const char *)make_base64_string2 ((unsigned char *)phase3, phase3_bufpos, gc));
Signature · 1 line
1.8.4.5
Re: [Openvpn-devel] [PATCH 1/2] Get NTLMv1 and NTLMv2 up and running
Hi Holger,
sorry for drag this so long. I'm not trying to stare into this, but can't really say "this is correct" or "violating specs", lacking the NTLM auth background.
Is this used - and thus tested - by you only, or is it used by UTM customers? (This would give us confidence that it's tested by a larger user base, and those tend to uncover issues :-) ).
Can you explain which of these changes actually fix the "not working" bits, and which are just "make the code less ugly"?
More questions and some comments on style intermixed below...
Quoted 27 lines · Holger Kummert
* Force conversion to UTF-16 of username and domain if server requires UTF-16.
* Rewrite conversion function to cleanly convert UTF-8 to UTF-16.
* Fix bug in length computation in NTLMv2-code.
* Architecture independent access to NTLM NegotiateFlags.Signed-off-by Holger Kummert <[email protected]...>--- src/openvpn/ntlm.c | 156 +++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 134 insertions(+), 22 deletions(-)src/openvpn/ntlm.c +5
-0@@ -56,6 +56,11 @@#endif +#define NTLM_NEGOTIATE_UNICODE (1<<0) +#define NTLM_NEGOTIATE_OEM (1<<1) +#define NTLM_NEGOTIATE_NTLM2_KEY (1<<19) +#define NTLM_NEGOTIATE_TARGET_INFO (1<<23) + static void
src/openvpn/ntlm.c gen_md4_hash (const char* data, int data_len, char *result) · line 91
Quoted 10 lines
src/openvpn/ntlm.c +4
-2@@ -79,8 +84,10 @@ gen_md4_hash (const char* data, int data_len, char *result)const md_kt_t *md4_kt =md_kt_get ("MD4" );char md[MD4_DIGEST_LENGTH ]; -md_full (md4_kt, data, data_len, md); -memcpy (result, md,MD4_DIGEST_LENGTH ); +if (data_len >=0 ) { +md_full (md4_kt, data, data_len, md); +memcpy (result, md,MD4_DIGEST_LENGTH ); + } }
Can data_len be < 0 in any reasonable circumstances?
If not, I'd rather have an "ASSERT(data_len>=0)" here - it would be a calling convention violation, and just silently not doing anything is not how OpenVPN usually handles this (as it might lead to interesting effects later on, "result" being uninitialized). Better crash with a clear message pointing to this line of code in that case.
Quoted 5 lines
src/openvpn/ntlm.c +0
-0static void@@ -140,18 +147,89 @@ unsigned char *my_strupr(unsigned char *str) } static int
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 155
Quoted 10 lines
src/openvpn/ntlm.c +5
-4-unicodize (char *dst, const char *src) +utf8_to_utf16LE (unsigned char *dst, int dst_len, unsigned char *src) { -/* not really unicode... */ -int i =0 ; - do +/* Convert UTF-8 to UTF-16 little endian +* +* http://clang.llvm.org/doxygen/ConvertUTF_8c_source.html +*/
I can see why you would want to change this :-) - for clarification, is this a true bug in the code, preventing correct operation (... for user names / passwords with non-7bit characters in them), or is this just unrelated improvement?
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 176
Quoted 7 lines
src/openvpn/ntlm.c +7
-0+const unsigned int uni_max_utf16 =0x0010FFFF , + uni_half_base =0x00010000 , + uni_half_mask =0x000003FF , + uni_half_shift =10 , + uni_sur_high_start =0xD800 , + uni_sur_low_start =0xDC00 , + uni_sur_low_end =0xDFFF ;
I'm feeling slightly uneasy with these. If these are true constants, is there a good reason to use "const int" variables, and not follow the usual convention
#define UNI_MAX_UTF16 ...
#define UNI_HALF_BASE ...
so it's more obvious to the reader of the code that this is not something which can change at run time?
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 207
Quoted 9 lines
src/openvpn/ntlm.c +9
-0+if (ch <=0xFFFF ) + { +if (ch >= uni_sur_high_start && ch <= uni_sur_low_end) { +msg (M_INFO ,"Warning: Illegal character value: %x is in reserved area of UTF-16 [%x, %x]" , uni_sur_high_start, uni_sur_low_end); +return -1 ; + } +else {/* normal case */ +if (dst_len - (dst - dst_start) <2 ) { +msg (M_INFO ,"Warning: To less space in destination buffer for conversion to UTF-16" );
Grammar nit ;-) - "not enough space".
[..]
src/openvpn/ntlm.c ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar · line 323
Quoted 3 lines
src/openvpn/ntlm.c +1
-1/* fill 1st 16 bytes with md4 hash, disregard terminating null */ -gen_md4_hash (pwbuf,unicodize (pwbuf, p->up.password) -2 , md4_hash); +gen_md4_hash (pwbuf,utf8_to_utf16LE (pwbuf,sizeof (p->up.password) *2 , (unsigned char *)p->up.password), md4_hash);
I'm a bit confused (again). It's called "utf8_to_utf16LE()", but the stuff we have in OpenVPN variables, at least on unix systems, is not always UTF8 - it's "bytes as typed", and they might be ISO8859-1.
All my systems do *not* use UTF8.
So what happens if a user uses a German umlaut as part of his password, and types this into an password: prompt given by OpenVPN on an ISO8859-1 system?
(Yeah, this sucks, but this issue needs to be well understood before merging)
src/openvpn/ntlm.c ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar · line 336
Quoted 3 lines
src/openvpn/ntlm.c +2
-0+/* extract NegotiateFlags from bytes 20-23 */ + ntlm_negotiate_flags = (buf2[0x14 ] &0xff ) | (buf2[0x15 ] &0xff ) <<8 | (buf2[0x16 ] &0xff ) <<16 | (buf2[0x17 ] &0xff ) <<24 ;
We could make "buf2" an "unsigned char", and save ourselves the & 0xff orgy here... and further down setting tib_len.
src/openvpn/ntlm.c ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar · line 377
Quoted 4 lines
src/openvpn/ntlm.c +2
-2-if (( *((long *)&buf2[0x14 ]) &0x00800000 ) ==0x00800000 ){/* Check for Target Information block */ - tib_len = buf2[0x28 ];/* Get Target Information block size */ +if (ntlm_negotiate_flags &NTLM_NEGOTIATE_TARGET_INFO ){/* Check for Target Information block */ + tib_len = buf2[0x28 ] &0xff | (buf2[0x29 ] &0xff ) <<8 ;/* Get Target Information block size */
Yeah. Thanks for this. Whoever thought that accessing an arbitrary and potentially unaligned block of memory as a (long) on "all architectures that OpenVPN runs on" should propably be tasked with maintaining options.c for the next 10 years, or so.
src/openvpn/ntlm.c ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar · line 390
Quoted 3 lines
src/openvpn/ntlm.c +1
-1/* Get blob length */ - ntlmv2_blob_size =0x20 + tib_len; + ntlmv2_blob_size =0x1c + tib_len;
What are these magic numbers? Was 0x20 a bug, and 0x1c is right, or is the context different, so the magic number needs to be different?
src/openvpn/ntlm.c ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar · line 436
Quoted 9 lines
src/openvpn/ntlm.c +9
-0+ len =utf8_to_utf16LE (username_u,sizeof (username_u), username); +if (len>=0 ) { +add_security_buffer (0x24 , username_u, len, phase3, &phase3_bufpos); + } + } +else { +add_security_buffer (0x24 , username,strlen (username), phase3, &phase3_bufpos); + } +
I can see that you did not invent add_security_buffer(), but "copying arbitrary strings provided by other bits of OpenVPN into a buffer of limited length, with no overrun checking" is not *exactly* making me feel good at night. So maybe this should be extended to ensure that "*msg_bufpos + length" is not overring sizeof(phase3)...
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...Re: [Openvpn-devel] [PATCH 1/2] Get NTLMv1 and NTLMv2 up and running
Hello Gerd,
first of all thank you for doing a first review of the code. Please find my comments below.
Am 02.06.2014 20:20, schrieb Gert Doering:
Quoted 9 lines
Hi Holger,
sorry for drag this so long. I'm not trying to stare into this, but can't really say "this is correct" or "violating specs", lacking the NTLM auth background.
Is this used - and thus tested - by you only, or is it used by UTM customers? (This would give us confidence that it's tested by a larger user base, and those tend to uncover issues :-) ).
Well, I thought I let it pass through the review process of the group first before pushing it to customers.
But if it would help the review we could do it the other way round and give it to customers first.
No problem.
Quoted 3 lines
Can you explain which of these changes actually fix the "not working" bits, and which are just "make the code less ugly"?
Roughly: The changes in ntlm.c (PATCH 1) belong to "fix the not working", and all other files (PATCH 2) are related to "improve code and configuration".
Quoted 45 lines
More questions and some comments on style intermixed below...
Quoted 27 lines · Holger Kummert
* Force conversion to UTF-16 of username and domain if server requires UTF-16.
* Rewrite conversion function to cleanly convert UTF-8 to UTF-16.
* Fix bug in length computation in NTLMv2-code.
* Architecture independent access to NTLM NegotiateFlags.Signed-off-by Holger Kummert <[email protected]...>--- src/openvpn/ntlm.c | 156 +++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 134 insertions(+), 22 deletions(-)src/openvpn/ntlm.c +5
-0@@ -56,6 +56,11 @@#endif +#define NTLM_NEGOTIATE_UNICODE (1<<0) +#define NTLM_NEGOTIATE_OEM (1<<1) +#define NTLM_NEGOTIATE_NTLM2_KEY (1<<19) +#define NTLM_NEGOTIATE_TARGET_INFO (1<<23) + static voidsrc/openvpn/ntlm.c gen_md4_hash (const char* data, int data_len, char *result) · line 91
Quoted 10 lines
src/openvpn/ntlm.c +4
-2@@ -79,8 +84,10 @@ gen_md4_hash (const char* data, int data_len, char *result)const md_kt_t *md4_kt =md_kt_get ("MD4" );char md[MD4_DIGEST_LENGTH ]; -md_full (md4_kt, data, data_len, md); -memcpy (result, md,MD4_DIGEST_LENGTH ); +if (data_len >=0 ) { +md_full (md4_kt, data, data_len, md); +memcpy (result, md,MD4_DIGEST_LENGTH ); + } }Can
data_lenbe < 0 in any reasonable circumstances?
Quoted 5 lines
If not, I'd rather have an "
ASSERT(data_len>=0)" here - it would be a calling convention violation, and just silently not doing anything is not how OpenVPN usually handles this (as it might lead to interesting effects later on, "result" being uninitialized). Better crash with a clear message pointing to this line of code in that case.
Unfortunately it could be that 'data_len' <= 0 because it's the result of utf8_to_utf16LE() which returns -1 in case of a conversion error.
Quoted 6 lines
Quoted 6 lines
src/openvpn/ntlm.c +0
-0static void@@ -140,18 +147,89 @@ unsigned char *my_strupr(unsigned char *str) }
Quoted 16 lines
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 155
Quoted 11 lines
src/openvpn/ntlm.c +5
-4static int -unicodize (char *dst,const char *src) +utf8_to_utf16LE (unsigned char *dst,int dst_len,unsigned char *src) { -/* not really unicode... */ -int i =0 ; - do +/* Convert UTF-8 to UTF-16 little endian +* +* http://clang.llvm.org/doxygen/ConvertUTF_8c_source.html +*/ I can see why you would want to change this :-) - for clarification, is this a true bug in the code, preventing correct operation (... for user names / passwords with non-7bit characters in them), or is this just unrelated improvement?
This change/function is important because it provides a correct conversion of non-7bit-characters which are assumed to have utf-8 encoding.
The previous simpler function worked only for 7bit ascii characters.
Quoted 13 lines
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 176
Quoted 8 lines
src/openvpn/ntlm.c +7
-0+const unsigned int uni_max_utf16 =0x0010FFFF , + uni_half_base =0x00010000 , + uni_half_mask =0x000003FF , + uni_half_shift =10 , + uni_sur_high_start =0xD800 , + uni_sur_low_start =0xDC00 , + uni_sur_low_end =0xDFFF ;I'm feeling slightly uneasy with these. If these are true constants, is there a good reason to use "const int" variables, and not follow the usual convention
Quoted 5 lines
#define
UNI_MAX_UTF16...
#defineUNI_HALF_BASE...so it's more obvious to the reader of the code that this is not something which can change at run time?
Yes, you're right, they are actually constants.
The reason why I defined them as variables is because I wanted to keep things together that belong together. I'm a bit unhappy with defining a constant in a separate .h file that is used only once somewhere in the code.
So what about defining them as constants, but at the same location in the code as the variable definitions are now?
Quoted 12 lines
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 207
Quoted 11 lines
src/openvpn/ntlm.c +9
-0+if (ch <=0xFFFF ) + { +if (ch >= uni_sur_high_start && ch <= uni_sur_low_end) { +msg (M_INFO ,"Warning: Illegal character value: %x is in reserved area of UTF-16 [%x, %x]" , uni_sur_high_start, uni_sur_low_end); +return -1 ; + } +else {/* normal case */ +if (dst_len - (dst - dst_start) <2 ) { +msg (M_INFO ,"Warning: To less space in destination buffer for conversion to UTF-16" );
Quoted 1 line
Grammar nit ;-) - "not enough space".
Ok.
Quoted 11 lines
[..]
src/openvpn/ntlm.c ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar · line 323
Quoted 3 lines
/* fill 1st 16 bytes with md4 hash, disregard terminating null */src/openvpn/ntlm.c +1
-1-gen_md4_hash (pwbuf,unicodize (pwbuf, p->up.password) -2 , md4_hash); +gen_md4_hash (pwbuf,utf8_to_utf16LE (pwbuf,sizeof (p->up.password) *2 , (unsigned char *)p->up.password), md4_hash);I'm a bit confused (again). It's called "
utf8_to_utf16LE()", but the stuff we have in OpenVPN variables, at least on unix systems, is not always UTF8 - it's "bytes as typed", and they might be ISO8859-1.
Quoted 5 lines
All my systems do *not* use UTF8.
So what happens if a user uses a German umlaut as part of his password, and types this into an password: prompt given by OpenVPN on an ISO8859-1 system?
Well, Heiko told me that you guys decided that "we are now in 21st century and we can assume utf-8 internally" a couple of years ago (maybe at the first FOSDEM meeting, but he can't recall exactly). If other encodings than utf-8 should also be supported then the input filter of username and password should take the local encoding into account and provide a conversion to utf-8.
Quoted 3 lines
(Yeah, this sucks, but this issue needs to be well understood before merging)
The problem is - to some extend - well understood but we have to agree on what we can expect as input encoding on the different layers in the software.
Quoted 7 lines
src/openvpn/ntlm.c ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar · line 336
Quoted 4 lines
src/openvpn/ntlm.c +2
-0+/* extract NegotiateFlags from bytes 20-23 */ + ntlm_negotiate_flags = (buf2[0x14 ] &0xff ) | (buf2[0x15 ] &0xff ) <<8 | (buf2[0x16 ] &0xff ) <<16 | (buf2[0x17 ] &0xff ) <<24 ;We could make "buf2" an "unsigned char", and save ourselves the &
0xfforgy here... and further down settingtib_len.
Well, I'm unsure if that would work on all platforms.
In my experience the above orgy works always under all circumstances and doesn't give me restless nights ...
But if you're convinced that your proposal works (I just tried it in a separate test successfully) we could give it a try.
Quoted 10 lines
src/openvpn/ntlm.c ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar · line 377
Quoted 5 lines
src/openvpn/ntlm.c +2
-2-if (( *((long *)&buf2[0x14 ]) &0x00800000 ) ==0x00800000 ){/* Check for Target Information block */ - tib_len = buf2[0x28 ];/* Get Target Information block size */ +if (ntlm_negotiate_flags &NTLM_NEGOTIATE_TARGET_INFO ){/* Check for Target Information block */ + tib_len = buf2[0x28 ] &0xff | (buf2[0x29 ] &0xff ) <<8 ;/* Get Target Information block size */ Yeah. Thanks for this. Whoever thought that accessing an arbitrary and potentially unaligned block of memory as a (long) on "all architectures that OpenVPN runs on" should propably be tasked with maintaining
options.cfor the next 10 years, or so.
I think that the main error in the previous implementation lies in the assumption of a certain byte order which the new code eliminates.
Quoted 8 lines
src/openvpn/ntlm.c ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar · line 390
Quoted 5 lines
/* Get blob length */
src/openvpn/ntlm.c +1
-1- ntlmv2_blob_size =0x20 + tib_len; + ntlmv2_blob_size =0x1c + tib_len;What are these magic numbers? Was
0x20a bug, and0x1cis right, or is the context different, so the magic number needs to be different?
The 0x20 was a big, big bug which had cost me a long time to find out. The new number corresponds to the field descriptions in Microsofts' spec.
Quoted 11 lines
src/openvpn/ntlm.c ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar · line 436
Quoted 10 lines
src/openvpn/ntlm.c +9
-0+ len =utf8_to_utf16LE (username_u,sizeof (username_u), username); +if (len>=0 ) { +add_security_buffer (0x24 , username_u, len, phase3, &phase3_bufpos); + } + } +else { +add_security_buffer (0x24 , username,strlen (username), phase3, &phase3_bufpos); + } +
Quoted 5 lines
I can see that you did not invent
add_security_buffer(), but "copying arbitrary strings provided by other bits of OpenVPN into a buffer of limited length, with no overrun checking" is not *exactly* making me feel good at night. So maybe this should be extended to ensure that "*msg_bufpos+ length" is not overring sizeof(phase3)...
Agreed. What about adding an additional parameter to add_security_buffer() for the total length of 'msg_buf'?
Best regards,
Holger
Quoted 3 lines
gert
Re: [Openvpn-devel] [PATCH 1/2] Get NTLMv1 and NTLMv2 up and running
Hello Gert,
I reconsidered this point and found some lively discussions about it, e.g. http://stackoverflow.com/questions/1674032/static-const-vs-define-in-c
The "const int" way has some advantages (as pointed out in all the discussions I found) over the #define:
- It is type-safe
- It respects scope
- It eases (in some cases) debugging (entry in symbol table)
It is usually recommended as "static const", what we could also use.
Am 04.06.2014 16:05, schrieb Holger Kummert:
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
src/openvpn/ntlm.c +7
-0+const unsigned int uni_max_utf16 =0x0010FFFF , + uni_half_base =0x00010000 , + uni_half_mask =0x000003FF , + uni_half_shift =10 , + uni_sur_high_start =0xD800 , + uni_sur_low_start =0xDC00 , + uni_sur_low_end =0xDFFF ;
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
I'm feeling slightly uneasy with these. If these are true constants, is there a good reason to use "const int" variables, and not follow the usual convention
#define
UNI_MAX_UTF16...
#defineUNI_HALF_BASE...
In the original code both variants are used ... (see http://clang.llvm.org/doxygen/ConvertUTF_8c_source.html)
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
so it's more obvious to the reader of the code that this is not something which can change at run time?
Well, doesn't a "const" definition express this?
Quoted 8 lines
Yes, you're right, they are actually constants.
The reason why I defined them as variables is because I wanted to keep things together that belong together. I'm a bit unhappy with defining a constant in a separate .h file that is used only once somewhere in the code.So what about defining them as constants, but at the same location in the code as the variable definitions are now?
So I'm in favor of the "static const" - definition.
Opinions?
Holger
Re: [Openvpn-devel] [PATCH 1/2] Get NTLMv1 compatibilities and NTLMv2 up and running
Quoted 30 lines · Holger Kummert
Hello Gert,
I reconsidered this point and found some lively discussions about it, e.g. http://stackoverflow.com/questions/1674032/static-const-vs-define-in-c
The "const int" way has some advantages (as pointed out in all the discussions I found) over the #define:
- It is type-safe
- It respects scope
- It eases (in some cases) debugging (entry in symbol table)It is usually recommended as "static const", what we could also use.
Am 04.06.2014 16:05, schrieb Holger Kummert:
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
src/openvpn/ntlm.c +7
-0+const unsigned int uni_max_utf16 =0x0010FFFF , + uni_half_base =0x00010000 , + uni_half_mask =0x000003FF , + uni_half_shift =10 , + uni_sur_high_start =0xD800 , + uni_sur_low_start =0xDC00 , + uni_sur_low_end =0xDFFF ;Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
I'm feeling slightly uneasy with these. If these are true constants, is there a good reason to use "const int" variables, and not follow the usual convention
#define
UNI_MAX_UTF16...
#defineUNI_HALF_BASE...
Quoted 8 lines
In the original code both variants are used ... (see http://clang.llvm.org/doxygen/ConvertUTF_8c_source.html)
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
so it's more obvious to the reader of the code that this is not something which can change at run time?
Well, doesn't a "const" definition express this?
Not really, how I see it. Because most of the macros are defined in upper case in the OpenVPN code vase. So when you see upper case "variables" used, you can be quite sure it's a constant you're dealing with. You don't have to lookup where that variable is defined to see if it's a constant or not. But that's more a coding style thing.
Quoted 5 lines
Quoted 5 lines
Yes, you're right, they are actually constants.
The reason why I defined them as variables is because I wanted to keep things together that belong together. I'm a bit unhappy with defining a constant in a separate .h file that is used only once somewhere in the code.
Quoted 7 lines
Quoted 3 lines
So what about defining them as constants, but at the same location in the code as the variable definitions are now?
So I'm in favor of the "static const" - definition.
Opinions?
I see and understands the arguments for using static const, and it surely have it's advantages. However, the OpenVPN code base, which dates back over a decade, has used the #define approach. This is probably one of the really few things where the code base is quite consistent. I prefer that we try to be somewhat consistent with what most of the already existing code uses.
Even though the coding style can be quite inconsistent in parts of the code base, I prefer that we don't add more inconsistency.
If we at some point find the advantages with static const to really be superior to #define, then we should switch most of the code base to that style. But that's a different discussion, as this just as well requires compatibility testing with preprocessors and compilers across several platforms.
Signature · 3 lines
kind regards, David Sommerseth
Re: [Openvpn-devel] [PATCH 1/2] Get NTLMv1 compatibilities and NTLMv2 up and running
So the vote goes clearly to #defines, right?
I'm going to change the code and test it. If it works, the change will be included in the next patch.
Best regards,
Holger
Am 18.06.2014 23:07, schrieb David Sommerseth:
Quoted 17 lines
I see and understands the arguments for using static const, and it surely have it's advantages. However, the OpenVPN code base, which dates back over a decade, has used the #define approach. This is probably one of the really few things where the code base is quite consistent. I prefer that we try to be somewhat consistent with what most of the already existing code uses.
Even though the coding style can be quite inconsistent in parts of the code base, I prefer that we don't add more inconsistency.
If we at some point find the advantages with static const to really be superior to #define, then we should switch most of the code base to that style. But that's a different discussion, as this just as well requires compatibility testing with preprocessors and compilers across several platforms.
Re: [Openvpn-devel] [PATCH 1/2] Get NTLMv1 compatibilities and NTLMv2 up and running
Hi,
Quoted 1 line · Holger Kummert
So the vote goes clearly to #defines, right?
Yes, please. Not because it's necessarily "better", but because it's what the rest of the code currently uses. (As with all styleguides, personal taste will differ in places... I wouldn't *ever* use this particular indentation style, for example, but "as the code is what it is", we all stick to it... hard enough to maintain, even so).
Quoted 2 lines
I'm going to change the code and test it. If it works, the change will be included in the next patch.
Thanks. And I like the way this has become an actual discussion among people that understand NTLM auth :-)
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...Re: [Openvpn-devel] [PATCH 1/2] Get NTLMv1 and NTLMv2 up and running
In reply to Holger KummertHello,
Am 04.06.2014 16:05, schrieb Holger Kummert:
Quoted 9 lines
Quoted 3 lines
Is this used - and thus tested - by you only, or is it used by UTM customers? (This would give us confidence that it's tested by a larger user base, and those tend to uncover issues :-) ).
Well, I thought I let it pass through the review process of the group first before pushing it to customers.
But if it would help the review we could do it the other way round and give it to customers first.
No problem.
in the meantime I got feedback from a customer who successfully tested both patches. His environment is Win7 on client side, and Win2012R2 on server side.
I also tested the implementation against a Win2003R2 server successfully (this server used utf8-encoding and NTLMv2sec, too). The client was a Win7 Ultimate with SP1.
So we know now that the NTLM-patches work against Win2003R2, Win2008R2, Win2012R2 and squid (the latter was reported by David Woodhouse).
Regards,
Holger
PATCH v2 1/2 Get NTLMv1 and NTLMv2 up and running
In reply to Holger Kummert * Force conversion to UTF-16 of username and domain if server requires UTF-16.
* Rewrite conversion function to cleanly convert UTF-8 to UTF-16.
* Fix bug in length computation in NTLMv2-code.
* Architecture independent access to NTLM NegotiateFlags.
--- src/openvpn/ntlm.c | 169 +++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 144 insertions(+), 25 deletions(-)
src/openvpn/ntlm.c +144 -25
@@ -56,6 +56,20 @@#endif +#define UNI_MAX_UTF16 0x0010FFFF +#define UNI_HALF_BASE 0x00010000 +#define UNI_HALF_MASK 0x000003FF +#define UNI_HALF_SHIFT 10 +#define UNI_SUR_HIGH_START 0xD800 +#define UNI_SUR_LOW_START 0xDC00 +#define UNI_SUR_LOW_END 0xDFFF + + +#define NTLM_NEGOTIATE_UNICODE (1<<0) +#define NTLM_NEGOTIATE_OEM (1<<1) +#define NTLM_NEGOTIATE_NTLM2_KEY (1<<19) +#define NTLM_NEGOTIATE_TARGET_INFO (1<<23) + static void
@@ -79,8 +93,10 @@ gen_md4_hash (const char* data, int data_len, char *result)const md_kt_t *md4_kt =md_kt_get ("MD4" );char md[MD4_DIGEST_LENGTH ]; -md_full (md4_kt, data, data_len, md); -memcpy (result, md,MD4_DIGEST_LENGTH ); +if (data_len >=0 ) { +md_full (md4_kt, data, data_len, md); +memcpy (result, md,MD4_DIGEST_LENGTH ); + } } static void
@@ -140,23 +156,92 @@ unsigned char *my_strupr(unsigned char *str) }static int -unicodize (char *dst,const char *src) +utf8_to_utf16LE (unsigned char *dst,int dst_len,unsigned char *src) { -/* not really unicode... */ -int i =0 ; -do +/* Convert UTF-8 to UTF-16 little endian +* +* http://clang.llvm.org/doxygen/ConvertUTF_8c_source.html +*/ +unsigned char *end = src +strlen (src), + *dst_start = dst; +const char trailingBytesForUTF8[256 ] = { +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 , +2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,3 ,3 ,3 ,3 ,3 ,3 ,3 ,3 ,4 ,4 ,4 ,4 ,5 ,5 ,5 ,5 + }; +const unsigned int offsetsUtf8[6 ] = {0x00000000UL ,0x00003080UL ,0x000E2080UL , +0x03C82080UL ,0xFA082080UL ,0x82082080UL }; + +while (src < end) { - dst[i++] = *src; - dst[i++] =0 ; +unsigned int ch =0 , + extraBytes = trailingBytesForUTF8[*src]; + +if (extraBytes >= end - src) +return -1 ; + +/* The cases all fall through. */ +switch (extraBytes) { +case 5 : ch += *src++; ch <<=6 ; +case 4 : ch += *src++; ch <<=6 ; +case 3 : ch += *src++; ch <<=6 ; +case 2 : ch += *src++; ch <<=6 ; +case 1 : ch += *src++; ch <<=6 ; +case 0 : ch += *src++; + } + ch -= offsetsUtf8[extraBytes];/* clean up UTF-8 control bits */ + +/* Now 'ch' contains the codepoint of the UTF-8-character. */ + +if (ch <=0xFFFF ) + { +if (ch >=UNI_SUR_HIGH_START && ch <=UNI_SUR_LOW_END ) { +msg (M_INFO ,"Warning: Illegal character value: %x is in reserved area of UTF-16 [%x, %x]" ,UNI_SUR_HIGH_START ,UNI_SUR_LOW_END ); +return -1 ; + } +else {/* normal case */ +if (dst_len - (dst - dst_start) <2 ) { +msg (M_INFO ,"Warning: Not enough space in destination buffer for conversion to UTF-16" ); +return -1 ; + } + *dst++ = ch &0xff ; + *dst++ = (ch >>8 ) &0xff ; + } + } +else if (ch >UNI_MAX_UTF16 ) { +msg (M_INFO ,"Warning: Illegal character value :%x is too large for UTF-16" , ch); +return -1 ; + } +else { +/* character is in range 0xFFFF - 0x10FFFF. */ +if (dst_len - (dst - dst_start) <4 ) { +msg (M_INFO ,"Warning: Not enough space in destination buffer for conversion to UTF-16" ); +return -1 ; + } + ch -=UNI_HALF_BASE ; + *dst++ = ( (ch >>UNI_HALF_SHIFT ) +UNI_SUR_HIGH_START ) &0xff ; + *dst++ = (((ch >>UNI_HALF_SHIFT ) +UNI_SUR_HIGH_START ) >>8 ) &0xff ; + *dst++ = ( (ch &UNI_HALF_MASK ) +UNI_SUR_LOW_START ) &0xff ; + *dst++ = (((ch &UNI_HALF_MASK ) +UNI_SUR_LOW_START ) >>8 ) &0xff ; + } } -while (*src++); -return i; +return dst - dst_start; }static void -add_security_buffer (int sb_offset,void *data,int length,unsigned char *msg_buf,int *msg_bufpos) +add_security_buffer (int sb_offset,void *data,int length,unsigned char *msg_buf,int *msg_bufpos,int msg_buflen) { +if ((*msg_bufpos - *msg_buf) + length > msg_buflen) { +msg (M_INFO ,"Warning: Not enough space in destination buffer" ); +return ; + } +/* Adds security buffer data to a message and sets security buffer's offset and length */ msg_buf[sb_offset] = (unsigned char )length; msg_buf[sb_offset +2 ] = msg_buf[sb_offset];
@@ -195,10 +280,12 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_archar pwbuf[sizeof (p->up.password) *2 ];/* for unicode password */ char buf2[128 ];/* decoded reply from proxy */ unsigned char phase3[464 ]; +unsigned long ntlm_negotiate_flags;char md4_hash[MD4_DIGEST_LENGTH +5 ];char challenge[8 ], ntlm_response[24 ];int i, ret_val; +int ntlm_unicode;char ntlmv2_response[144 ];char userdomain_u[256 ];/* for uppercase unicode username and domain */
@@ -211,7 +298,9 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_arsize_t len;char domain[128 ]; -char username[128 ]; +char domain_u[256 ]; +char username[USER_PASS_LEN ]; +char username_u[USER_PASS_LEN *2 ];char *separator;bool ntlmv2_enabled = (p->auth_method ==HTTP_AUTH_NTLM2 );
@@ -238,7 +327,7 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar/* fill 1st 16 bytes with md4 hash, disregard terminating null */ -gen_md4_hash (pwbuf,unicodize (pwbuf, p->up.password) -2 , md4_hash); +gen_md4_hash (pwbuf,utf8_to_utf16LE (pwbuf,sizeof (p->up.password) *2 , (unsigned char *)p->up.password), md4_hash);/* pad to 21 bytes */ memset (md4_hash +MD4_DIGEST_LENGTH ,0 ,5 );
@@ -250,23 +339,36 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar/* we can be sure that phase_2 is less than 128 * therefore buf2 needs to be (3/4 * 128) */ +/* extract NegotiateFlags from bytes 20-23 */ + ntlm_negotiate_flags = (buf2[0x14 ] &0xff ) | (buf2[0x15 ] &0xff ) <<8 | (buf2[0x16 ] &0xff ) <<16 | (buf2[0x17 ] &0xff ) <<24 ; + + ntlm_unicode = ntlm_negotiate_flags &NTLM_NEGOTIATE_UNICODE ?1 :0 ; +/* extract the challenge from bytes 24-31 */ for (i=0 ; i<8 ; i++) { challenge[i] = buf2[i+24 ]; } +msg (M_DEBUG ,"Received NTLM negotiate flags 0x%08x: %s encoding, %sNTLM2sec, %sTargetInfo - doing NTLMv%s" , + ntlm_negotiate_flags, ntlm_unicode ?"Unicode" :"OEM" , + ntlm_negotiate_flags &NTLM_NEGOTIATE_NTLM2_KEY ?"" :"no " , + ntlm_negotiate_flags &NTLM_NEGOTIATE_TARGET_INFO ?"" :"no " , + ntlmv2_enabled ?"2" :"1" ); +if (ntlmv2_enabled){/* Generate NTLMv2 response */ int tib_len;/* NTLMv2 hash */ -my_strupr ((unsigned char *)strcpy (userdomain, username)); +my_strupr ((unsigned char *)strcpy (userdomain, username));if (strlen (username) +strlen (domain) <sizeof (userdomain))strcat (userdomain, domain);else msg (M_INFO ,"Warning: Username or domain too long" ); -unicodize (userdomain_u, userdomain); -gen_hmac_md5 (userdomain_u,2 *strlen (userdomain), md4_hash,MD5_DIGEST_LENGTH , ntlmv2_hash); + len =utf8_to_utf16LE (userdomain_u,sizeof (userdomain_u), userdomain); +if (len >=0 ) { +gen_hmac_md5 (userdomain_u, len, md4_hash,MD5_DIGEST_LENGTH , ntlmv2_hash); + }/* NTLMv2 Blob */ memset (ntlmv2_blob,0 ,128 );/* Clear blob buffer */
@@ -278,8 +380,8 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar ntlmv2_blob[0x18 ]=0 ;/* Unknown, zero should work */ /* Add target information block to the blob */ -if (( *((long *)&buf2[0x14 ]) &0x00800000 ) ==0x00800000 ){/* Check for Target Information block */ - tib_len = buf2[0x28 ];/* Get Target Information block size */ +if (ntlm_negotiate_flags &NTLM_NEGOTIATE_TARGET_INFO ){/* Check for Target Information block */ + tib_len = buf2[0x28 ] &0xff | (buf2[0x29 ] &0xff ) <<8 ;/* Get Target Information block size */ if (tib_len >96 ) tib_len =96 ; {char *tib_ptr = buf2 + buf2[0x2c ];/* Get Target Information block pointer */
@@ -292,7 +394,7 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar ntlmv2_blob[0x1c + tib_len] =0 ;/* Unknown, zero works */ /* Get blob length */ - ntlmv2_blob_size =0x20 + tib_len; + ntlmv2_blob_size =0x1c + tib_len;/* Add challenge from message 2 */ memcpy (&ntlmv2_response[8 ], challenge,8 );
@@ -323,16 +425,33 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar phase3[8 ] =3 ;/* type 3 */ if (ntlmv2_enabled){/* NTLMv2 response */ -add_security_buffer (0x14 , ntlmv2_response, ntlmv2_blob_size +16 , phase3, &phase3_bufpos); +add_security_buffer (0x14 , ntlmv2_response, ntlmv2_blob_size +16 , phase3, &phase3_bufpos,sizeof (phase3)); }else {/* NTLM response */ -add_security_buffer (0x14 , ntlm_response,24 , phase3, &phase3_bufpos); +add_security_buffer (0x14 , ntlm_response,24 , phase3, &phase3_bufpos,sizeof (phase3)); } -/* username in ascii */ -add_security_buffer (0x24 , username,strlen (username), phase3, &phase3_bufpos); +/* Set username. */ +if (ntlm_unicode) { + len =utf8_to_utf16LE (username_u,sizeof (username_u), username); +if (len>=0 ) { +add_security_buffer (0x24 , username_u, len, phase3, &phase3_bufpos,sizeof (phase3)); + } + } +else { +add_security_buffer (0x24 , username,strlen (username), phase3, &phase3_bufpos,sizeof (phase3)); + } +/* Set domain. If <domain> is empty, default domain will be used (i.e. proxy's domain) */ -add_security_buffer (0x1c , domain,strlen (domain), phase3, &phase3_bufpos); +if (ntlm_unicode) { + len =utf8_to_utf16LE (domain_u,sizeof (domain_u), domain); +if (len>=0 ) { +add_security_buffer (0x1c , domain_u, len, phase3, &phase3_bufpos,sizeof (phase3)); + } + } +else { +add_security_buffer (0x1c , domain,strlen (domain), phase3, &phase3_bufpos,sizeof (phase3)); + }/* other security buffers will be empty */
@@ -341,7 +460,7 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar phase3[0x38 ] = phase3_bufpos;/* no session key */ /* flags */ - phase3[0x3c ] =0x02 ;/* negotiate oem */ + phase3[0x3c ] = ntlm_unicode ?0x01 :0x02 ;/* negotiate unicode/oem */ phase3[0x3d ] =0x02 ;/* negotiate ntlm */ return ((const char *)make_base64_string2 ((unsigned char *)phase3, phase3_bufpos, gc));
Signature · 1 line
tg: (fce39ec..) t/0006/fix_ntlm_http_auth (depends on: t/0005/ignore_utf8_bom)
Re: [Openvpn-devel] [PATCHv2 1/2] Get NTLMv1 and NTLMv2 up and running
Hi Holgert,
Apologies for taking so long to respond. This patch came up again at the hackathon last weekend. Since we can not thoroughly test this ourselves, but the patch is coming from a known source and you report successful tests, we decided that 'stare at code' should suffice. Hereby the results of my staring.
First, ntlm.c was a type mess before your patch, and still is after. I'll not comment on that any further, but will send a follow-up patch to fix it later on. It would be highly appreciated if you could give that patch a test spin for me.
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 176
Quoted 10 lines · Holger Kummert
src/openvpn/ntlm.c +10
-0+const char trailingBytesForUTF8[256 ] = { +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 , +2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,3 ,3 ,3 ,3 ,3 ,3 ,3 ,3 ,4 ,4 ,4 ,4 ,5 ,5 ,5 ,5 + };
Not sure if the compiler will actually put this on the stack, but this can be static const.
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 206
Quoted 4 lines
src/openvpn/ntlm.c +4
-0+if (ch >=UNI_SUR_HIGH_START && ch <=UNI_SUR_LOW_END ) { +msg (M_INFO ,"Warning: Illegal character value: %x is in reserved area of UTF-16 [%x, %x]" ,UNI_SUR_HIGH_START ,UNI_SUR_LOW_END ); +return -1 ; + }
This msg() call is missing an argument (I think 'ch').
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 244
Quoted 9 lines
static void
src/openvpn/ntlm.c +6
-1-add_security_buffer(int sb_offset, void *data, int length, unsigned char *msg_buf, int *msg_bufpos) +add_security_buffer(int sb_offset, void *data, int length, unsigned char *msg_buf, int *msg_bufpos, int msg_buflen) { + if ((*msg_bufpos - *msg_buf) + length > msg_buflen) { + msg (M_INFO, "Warning: Not enough space in destination buffer"); +return ; + } +
I don't think this is what you intended, or does *msg_buf contain some magic length field? You probably want something like:
ASSERT (length > 0 && *msg_bufpos > 0 && msg_buflen > 0);
if (msg_buflen - length < msg_bufpos) { /* error */ }That should be an integer-overflow safe overflow check.
Since we're also writing to sb_offset to sb_offset+5, we should also add a check on sb_offset:
if (msg_buflen - 5 >= sb_offset) { /* error */ }
(And wow, this function was/is absolutely terrifying. It seems like a miracle that this code ever worked...)
-Steffan
Re: [Openvpn-devel] [PATCHv2 1/2] Get NTLMv1 and NTLMv2 up and running
Hello Steffan,
Am 14.10.2015 um 00:55 schrieb Steffan Karger:
Quoted 4 lines
Hi Holgert,
Apologies for taking so long to respond. This patch came up again at the hackathon last weekend. Since we can not thoroughly test this
no problem. I'm glad that someone picks this topic up.
Quoted 8 lines
ourselves, but the patch is coming from a known source and you report successful tests, we decided that 'stare at code' should suffice. Hereby the results of my staring.
First,
ntlm.cwas a type mess before your patch, and still is after. I'll not comment on that any further, but will send a follow-up patch to fix it later on. It would be highly appreciated if you could give that patch a test spin for me.
Hmm, setting up an environment for testing is a bit lengthy. But let's see ...
Quoted 16 lines · Holger Kummert
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 176
Quoted 10 lines
src/openvpn/ntlm.c +10
-0+const char trailingBytesForUTF8[256 ] = { +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 , +2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,3 ,3 ,3 ,3 ,3 ,3 ,3 ,3 ,4 ,4 ,4 ,4 ,5 ,5 ,5 ,5 + };Not sure if the compiler will actually put this on the stack, but this can be static const.
Quoted 6 lines
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 206
Quoted 4 lines
src/openvpn/ntlm.c +4
-0+if (ch >=UNI_SUR_HIGH_START && ch <=UNI_SUR_LOW_END ) { +msg (M_INFO ,"Warning: Illegal character value: %x is in reserved area of UTF-16 [%x, %x]" ,UNI_SUR_HIGH_START ,UNI_SUR_LOW_END ); +return -1 ; + }This
msg()call is missing an argument (I think 'ch').
Yes, obviously true.
Quoted 14 lines
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 244
Quoted 10 lines
static void
src/openvpn/ntlm.c +6
-1-add_security_buffer(int sb_offset, void *data, int length, unsigned char *msg_buf, int *msg_bufpos) +add_security_buffer(int sb_offset, void *data, int length, unsigned char *msg_buf, int *msg_bufpos, int msg_buflen) { + if ((*msg_bufpos - *msg_buf) + length > msg_buflen) { + msg (M_INFO, "Warning: Not enough space in destination buffer"); +return ; + } +I don't think this is what you intended, or does
*msg_bufcontain some magic length field? You probably want something like:
Quoted 4 lines
ASSERT (length > 0 && *msg_bufpos > 0 && msg_buflen > 0); if (msg_buflen - length < msg_bufpos) { /* error */ }That should be an integer-overflow safe overflow check.
Hmm, I think it won't work as msg_bufpos is a pointer into memory, right? What about improving the first version with
ASSERT (length > 0 && *msg_bufpos > 0 && msg_buflen > 0);
if ((msg_bufpos - msg_buf) + length > msg_buflen) {
...?
Quoted 5 lines
Since we're also writing to
sb_offsetto sb_offset+5, we should also add a check onsb_offset:if (msg_buflen - 5 >= sb_offset) { /* error */ }
Yes, makes sense.
Regards,
Holger
Quoted 12 lines
(And wow, this function was/is absolutely terrifying. It seems like a miracle that this code ever worked...)
-Steffan
------------------------------------------------------------------------------
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCHv2 1/2] Get NTLMv1 and NTLMv2 up and running
Hi Holger,
Quoted 16 lines · Holger Kummert
Quoted 16 lines
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 244
Quoted 9 lines
static void
src/openvpn/ntlm.c +6
-1-add_security_buffer(int sb_offset, void *data, int length, unsigned char *msg_buf, int *msg_bufpos) +add_security_buffer(int sb_offset, void *data, int length, unsigned char *msg_buf, int *msg_bufpos, int msg_buflen) { + if ((*msg_bufpos - *msg_buf) + length > msg_buflen) { + msg (M_INFO, "Warning: Not enough space in destination buffer"); +return ; + } +I don't think this is what you intended, or does
*msg_bufcontain some magic length field? You probably want something like:ASSERT (length > 0 && *msg_bufpos > 0 && msg_buflen > 0); if (msg_buflen - length < msg_bufpos) { /* error */ }
Quoted 8 lines
Quoted 1 line
That should be an integer-overflow safe overflow check.
Hmm, I think it won't work as
msg_bufposis a pointer into memory, right? What about improving the first version withASSERT (length > 0 && *msg_bufpos > 0 && msg_buflen > 0); if ((msg_bufpos - msg_buf) + length > msg_buflen) { ...
Seems I forgot the * before the second msg_bufpos, sorry. msg_bufpos itself is a pointer indeed, but *msg_bufpos is (used as) an offset into msg_buf:
memcpy(&msg_buf[*msg_bufpos], data, msg_buf[sb_offset]);
msg_bufpos (without the *) is not related to msg_buf.
-Steffan
Quoted 105 lines · Holger Kummert
Hello Steffan,
Am 14.10.2015 um 00:55 schrieb Steffan Karger:
Quoted 4 lines
Hi Holgert,
Apologies for taking so long to respond. This patch came up again at the hackathon last weekend. Since we can not thoroughly test this
no problem. I'm glad that someone picks this topic up.
Quoted 8 lines
ourselves, but the patch is coming from a known source and you report successful tests, we decided that 'stare at code' should suffice. Hereby the results of my staring.
First,
ntlm.cwas a type mess before your patch, and still is after. I'll not comment on that any further, but will send a follow-up patch to fix it later on. It would be highly appreciated if you could give that patch a test spin for me.Hmm, setting up an environment for testing is a bit lengthy. But let's see ...
Quoted 16 lines · Holger Kummert
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 176
Quoted 10 lines
src/openvpn/ntlm.c +10
-0+const char trailingBytesForUTF8[256 ] = { +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 , +2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,3 ,3 ,3 ,3 ,3 ,3 ,3 ,3 ,4 ,4 ,4 ,4 ,5 ,5 ,5 ,5 + };Not sure if the compiler will actually put this on the stack, but this can be static const.
Quoted 6 lines
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 206
Quoted 4 lines
src/openvpn/ntlm.c +4
-0+if (ch >=UNI_SUR_HIGH_START && ch <=UNI_SUR_LOW_END ) { +msg (M_INFO ,"Warning: Illegal character value: %x is in reserved area of UTF-16 [%x, %x]" ,UNI_SUR_HIGH_START ,UNI_SUR_LOW_END ); +return -1 ; + }This
msg()call is missing an argument (I think 'ch').Yes, obviously true.
Quoted 14 lines
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 244
Quoted 10 lines
static void
src/openvpn/ntlm.c +6
-1-add_security_buffer(int sb_offset, void *data, int length, unsigned char *msg_buf, int *msg_bufpos) +add_security_buffer(int sb_offset, void *data, int length, unsigned char *msg_buf, int *msg_bufpos, int msg_buflen) { + if ((*msg_bufpos - *msg_buf) + length > msg_buflen) { + msg (M_INFO, "Warning: Not enough space in destination buffer"); +return ; + } +I don't think this is what you intended, or does
*msg_bufcontain some magic length field? You probably want something like:Quoted 4 lines
ASSERT (length > 0 && *msg_bufpos > 0 && msg_buflen > 0); if (msg_buflen - length < msg_bufpos) { /* error */ }That should be an integer-overflow safe overflow check.
Hmm, I think it won't work as
msg_bufposis a pointer into memory, right? What about improving the first version withASSERT (length > 0 && *msg_bufpos > 0 && msg_buflen > 0); if ((msg_bufpos - msg_buf) + length > msg_buflen) { ...?
Quoted 5 lines
Since we're also writing to
sb_offsetto sb_offset+5, we should also add a check onsb_offset:if (msg_buflen - 5 >= sb_offset) { /* error */ }Yes, makes sense.
Regards,
HolgerQuoted 4 lines
(And wow, this function was/is absolutely terrifying. It seems like a miracle that this code ever worked...)
Quoted 8 lines
-Steffan
------------------------------------------------------------------------------
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel------------------------------------------------------------------------------
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCHv2 1/2] Get NTLMv1 and NTLMv2 up and running
Am 30.10.2015 um 14:58 schrieb Steffan Karger:
Quoted 29 lines
Hi Holger,
Quoted 16 lines · Holger Kummert
Quoted 16 lines
src/openvpn/ntlm.c unsigned char *my_strupr(unsigned char *str) · line 244
Quoted 9 lines
static void
src/openvpn/ntlm.c +6
-1-add_security_buffer(int sb_offset, void *data, int length, unsigned char *msg_buf, int *msg_bufpos) +add_security_buffer(int sb_offset, void *data, int length, unsigned char *msg_buf, int *msg_bufpos, int msg_buflen) { + if ((*msg_bufpos - *msg_buf) + length > msg_buflen) { + msg (M_INFO, "Warning: Not enough space in destination buffer"); +return ; + } +I don't think this is what you intended, or does
*msg_bufcontain some magic length field? You probably want something like:ASSERT (length > 0 && *msg_bufpos > 0 && msg_buflen > 0); if (msg_buflen - length < msg_bufpos) { /* error */ }Quoted 8 lines
Quoted 1 line
That should be an integer-overflow safe overflow check.
Hmm, I think it won't work as
msg_bufposis a pointer into memory, right? What about improving the first version withASSERT (length > 0 && *msg_bufpos > 0 && msg_buflen > 0); if ((msg_bufpos - msg_buf) + length > msg_buflen) { ...
Quoted 7 lines
Seems I forgot the * before the second
msg_bufpos, sorry.msg_bufpositself is a pointer indeed, but*msg_bufposis (used as) an offset intomsg_buf:memcpy(&msg_buf[*msg_bufpos], data, msg_buf[sb_offset]);
msg_bufpos(without the *) is not related tomsg_buf.
Ah, then your first proposal makes sense
(i.e. if (msg_buflen - length < *msg_bufpos) )
We could take that.
Sry, didn't look into the ntlm-code ...
Regards,
Holger
Re: [Openvpn-devel] [PATCHv2 1/2] Get NTLMv1 and NTLMv2 up and running
Hi,
Quoted 1 line · Holger Kummert
Am 30.10.2015 um 14:58 schrieb Steffan Karger:
[..]
Quoted 4 lines
Quoted 4 lines
Seems I forgot the * before the second
msg_bufpos, sorry.msg_bufpositself is a pointer indeed, but*msg_bufposis (used as) an offset intomsg_buf:
Quoted 8 lines
Quoted 3 lines
memcpy(&msg_buf[*msg_bufpos], data, msg_buf[sb_offset]);
msg_bufpos(without the *) is not related tomsg_buf.Ah, then your first proposal makes sense
(i.e. if (msg_buflen- length <*msg_bufpos) )We could take that.
So, how do we move onward here? Holger, can you send a new patch (set) incorporating Steffan's comments?
Since we found a proper reviewer now, merging this looks possible, after all the time... ;-)
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...Re: [Openvpn-devel] [PATCHv2 1/2] Get NTLMv1 and NTLMv2 up and running
Hi,
Quoted 14 lines · Gert Doering
Quoted 1 line · Holger Kummert
Am 30.10.2015 um 14:58 schrieb Steffan Karger:
[..]
Quoted 11 lines
Quoted 7 lines
Seems I forgot the * before the second
msg_bufpos, sorry.msg_bufpositself is a pointer indeed, but*msg_bufposis (used as) an offset intomsg_buf:memcpy(&msg_buf[*msg_bufpos], data, msg_buf[sb_offset]);
msg_bufpos(without the *) is not related tomsg_buf.Ah, then your first proposal makes sense
(i.e. if (msg_buflen- length <*msg_bufpos) )
Quoted 7 lines
Quoted 1 line
We could take that.
So, how do we move onward here? Holger, can you send a new patch (set) incorporating Steffan's comments?
Since we found a proper reviewer now, merging this looks possible, after all the time... ;-)
Would still be nice to get this patch in. I've had this patch (plus a type-fix patch) in my local repo for a long time now, so to get things started a bit, I rebased it onto the current (post-reformatting) master branch. The patch is attached.
Any news from the author's side?
-Steffan
From c5f5fc4278d34e8d99733672a6a324830907aa23 Mon Sep 17 00:00:00 2001
From: Holger Kummert <[email protected]...>
Date: Fri, 4 Jul 2014 09:35:24 +0200
Subject: [PATCH] Get NTLMv1 and NTLMv2 up and running
* Force conversion to UTF-16 of username and domain if server requires UTF-16.
* Rewrite conversion function to cleanly convert UTF-8 to UTF-16.
* Fix bug in length computation in NTLMv2-code.
* Architecture independent access to NTLM NegotiateFlags.
--- src/openvpn/ntlm.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 166 insertions(+), 24 deletions(-)
src/openvpn/ntlm.c +166 -24
@@ -56,6 +56,21 @@#endif +#define UNI_MAX_UTF16 0x0010FFFF +#define UNI_HALF_BASE 0x00010000 +#define UNI_HALF_MASK 0x000003FF +#define UNI_HALF_SHIFT 10 +#define UNI_SUR_HIGH_START 0xD800 +#define UNI_SUR_LOW_START 0xDC00 +#define UNI_SUR_LOW_END 0xDFFF + + +#define NTLM_NEGOTIATE_UNICODE (1<<0) +#define NTLM_NEGOTIATE_OEM (1<<1) +#define NTLM_NEGOTIATE_NTLM2_KEY (1<<19) +#define NTLM_NEGOTIATE_TARGET_INFO (1<<23) + +static void create_des_keys (const unsigned char *hash,unsigned char *key)
@@ -78,8 +93,11 @@ gen_md4_hash(const char *data, int data_len, char *result)const md_kt_t *md4_kt =md_kt_get ("MD4" );char md[MD4_DIGEST_LENGTH ]; -md_full (md4_kt, data, data_len, md); -memcpy (result, md,MD4_DIGEST_LENGTH ); +if (data_len >=0 ) + { +md_full (md4_kt, data, data_len, md); +memcpy (result, md,MD4_DIGEST_LENGTH ); + } } static void
@@ -140,23 +158,106 @@ my_strupr(unsigned char *str) }static int -unicodize (char *dst,const char *src) +utf8_to_utf16LE (unsigned char *dst,int dst_len,unsigned char *src) { -/* not really unicode... */ -int i =0 ; -do +/* Convert UTF-8 to UTF-16 little endian +* +* http://clang.llvm.org/doxygen/ConvertUTF_8c_source.html +*/ +unsigned char *end = src +strlen (src), + *dst_start = dst; +const char trailingBytesForUTF8[256 ] = { +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 , +1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 ,1 , +2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,2 ,3 ,3 ,3 ,3 ,3 ,3 ,3 ,3 ,4 ,4 ,4 ,4 ,5 ,5 ,5 ,5 + }; +const unsigned int offsetsUtf8[6 ] = {0x00000000UL ,0x00003080UL ,0x000E2080UL , +0x03C82080UL ,0xFA082080UL ,0x82082080UL }; + +while (src < end) { - dst[i++] = *src; - dst[i++] =0 ; +unsigned int ch =0 , + extraBytes = trailingBytesForUTF8[*src]; + +if (extraBytes >= end - src) + { +return -1 ; + } + +/* The cases all fall through. */ +switch (extraBytes) { +case 5 : ch += *src++; ch <<=6 ; + +case 4 : ch += *src++; ch <<=6 ; + +case 3 : ch += *src++; ch <<=6 ; + +case 2 : ch += *src++; ch <<=6 ; + +case 1 : ch += *src++; ch <<=6 ; + +case 0 : ch += *src++; + } + ch -= offsetsUtf8[extraBytes];/* clean up UTF-8 control bits */ + +/* Now 'ch' contains the codepoint of the UTF-8-character. */ + +if (ch <=0xFFFF ) + { +if (ch >=UNI_SUR_HIGH_START && ch <=UNI_SUR_LOW_END ) + { +msg (M_INFO ,"Warning: Illegal character value: %x is in reserved area of UTF-16 [%x, %x]" ,UNI_SUR_HIGH_START ,UNI_SUR_LOW_END ); +return -1 ; + } +else /* normal case */ + { +if (dst_len - (dst - dst_start) <2 ) + { +msg (M_INFO ,"Warning: Not enough space in destination buffer for conversion to UTF-16" ); +return -1 ; + } + *dst++ = ch &0xff ; + *dst++ = (ch >>8 ) &0xff ; + } + } +else if (ch >UNI_MAX_UTF16 ) + { +msg (M_INFO ,"Warning: Illegal character value :%x is too large for UTF-16" , ch); +return -1 ; + } +else + { +/* character is in range 0xFFFF - 0x10FFFF. */ +if (dst_len - (dst - dst_start) <4 ) + { +msg (M_INFO ,"Warning: Not enough space in destination buffer for conversion to UTF-16" ); +return -1 ; + } + ch -=UNI_HALF_BASE ; + *dst++ = ( (ch >>UNI_HALF_SHIFT ) +UNI_SUR_HIGH_START ) &0xff ; + *dst++ = (((ch >>UNI_HALF_SHIFT ) +UNI_SUR_HIGH_START ) >>8 ) &0xff ; + *dst++ = ( (ch &UNI_HALF_MASK ) +UNI_SUR_LOW_START ) &0xff ; + *dst++ = (((ch &UNI_HALF_MASK ) +UNI_SUR_LOW_START ) >>8 ) &0xff ; + } } -while (*src++); -return i; +return dst - dst_start; }static void -add_security_buffer (int sb_offset,void *data,int length,unsigned char *msg_buf,int *msg_bufpos) +add_security_buffer (int sb_offset,void *data,int length,unsigned char *msg_buf,int *msg_bufpos,int msg_buflen) { +if ((*msg_bufpos - *msg_buf) + length > msg_buflen) + { +msg (M_INFO ,"Warning: Not enough space in destination buffer" ); +return ; + } +/* Adds security buffer data to a message and sets security buffer's offset and length */ msg_buf[sb_offset] = (unsigned char )length; msg_buf[sb_offset +2 ] = msg_buf[sb_offset];
@@ -195,10 +296,12 @@ ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2, struct gc_arechar pwbuf[sizeof (p->up.password) *2 ];/* for unicode password */ char buf2[128 ];/* decoded reply from proxy */ unsigned char phase3[464 ]; +unsigned long ntlm_negotiate_flags;char md4_hash[MD4_DIGEST_LENGTH +5 ];char challenge[8 ], ntlm_response[24 ];int i, ret_val; +int ntlm_unicode;char ntlmv2_response[144 ];char userdomain_u[256 ];/* for uppercase unicode username and domain */
@@ -211,7 +314,9 @@ ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2, struct gc_aresize_t len;char domain[128 ]; -char username[128 ]; +char domain_u[256 ]; +char username[USER_PASS_LEN ]; +char username_u[USER_PASS_LEN *2 ];char *separator;bool ntlmv2_enabled = (p->auth_method ==HTTP_AUTH_NTLM2 );
@@ -244,7 +349,7 @@ ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2, struct gc_are/* fill 1st 16 bytes with md4 hash, disregard terminating null */ -gen_md4_hash (pwbuf,unicodize (pwbuf, p->up.password) -2 , md4_hash); +gen_md4_hash (pwbuf,utf8_to_utf16LE (pwbuf,sizeof (p->up.password) *2 , (unsigned char *)p->up.password), md4_hash);/* pad to 21 bytes */ memset (md4_hash +MD4_DIGEST_LENGTH ,0 ,5 );
@@ -258,12 +363,23 @@ ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2, struct gc_are/* we can be sure that phase_2 is less than 128 * therefore buf2 needs to be (3/4 * 128) */ +/* extract NegotiateFlags from bytes 20-23 */ + ntlm_negotiate_flags = (buf2[0x14 ] &0xff ) | (buf2[0x15 ] &0xff ) <<8 | (buf2[0x16 ] &0xff ) <<16 | (buf2[0x17 ] &0xff ) <<24 ; + + ntlm_unicode = ntlm_negotiate_flags &NTLM_NEGOTIATE_UNICODE ?1 :0 ; +/* extract the challenge from bytes 24-31 */ for (i =0 ; i<8 ; i++) { challenge[i] = buf2[i+24 ]; } +msg (M_DEBUG ,"Received NTLM negotiate flags 0x%08x: %s encoding, %sNTLM2sec, %sTargetInfo - doing NTLMv%s" , + ntlm_negotiate_flags, ntlm_unicode ?"Unicode" :"OEM" , + ntlm_negotiate_flags &NTLM_NEGOTIATE_NTLM2_KEY ?"" :"no " , + ntlm_negotiate_flags &NTLM_NEGOTIATE_TARGET_INFO ?"" :"no " , + ntlmv2_enabled ?"2" :"1" ); +if (ntlmv2_enabled)/* Generate NTLMv2 response */ {int tib_len;
@@ -278,8 +394,11 @@ ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2, struct gc_are {msg (M_INFO ,"Warning: Username or domain too long" ); } -unicodize (userdomain_u, userdomain); -gen_hmac_md5 (userdomain_u,2 *strlen (userdomain), md4_hash,MD5_DIGEST_LENGTH , ntlmv2_hash); + len =utf8_to_utf16LE (userdomain_u,sizeof (userdomain_u), userdomain); +if (len >=0 ) + { +gen_hmac_md5 (userdomain_u, len, md4_hash,MD5_DIGEST_LENGTH , ntlmv2_hash); + }/* NTLMv2 Blob */ memset (ntlmv2_blob,0 ,128 );/* Clear blob buffer */
@@ -291,9 +410,9 @@ ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2, struct gc_are ntlmv2_blob[0x18 ] =0 ;/* Unknown, zero should work */ /* Add target information block to the blob */ -if (( *((long *)&buf2[0x14 ]) &0x00800000 ) ==0x00800000 )/* Check for Target Information block */ +if (ntlm_negotiate_flags &NTLM_NEGOTIATE_TARGET_INFO )/* Check for Target Information block */ { - tib_len = buf2[0x28 ];/* Get Target Information block size */ + tib_len = buf2[0x28 ] &0xff | (buf2[0x29 ] &0xff ) <<8 ;/* Get Target Information block size */ if (tib_len >96 ) { tib_len =96 ;
@@ -311,7 +430,7 @@ ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2, struct gc_are ntlmv2_blob[0x1c + tib_len] =0 ;/* Unknown, zero works */ /* Get blob length */ - ntlmv2_blob_size =0x20 + tib_len; + ntlmv2_blob_size =0x1c + tib_len;/* Add challenge from message 2 */ memcpy (&ntlmv2_response[8 ], challenge,8 );
@@ -345,18 +464,41 @@ ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2, struct gc_areif (ntlmv2_enabled)/* NTLMv2 response */ { -add_security_buffer (0x14 , ntlmv2_response, ntlmv2_blob_size +16 , phase3, &phase3_bufpos); +add_security_buffer (0x14 , ntlmv2_response, ntlmv2_blob_size +16 , phase3, &phase3_bufpos,sizeof (phase3)); }else /* NTLM response */ { -add_security_buffer (0x14 , ntlm_response,24 , phase3, &phase3_bufpos); +add_security_buffer (0x14 , ntlm_response,24 , phase3, &phase3_bufpos,sizeof (phase3)); + } + +/* Set username. */ +if (ntlm_unicode) + { + len =utf8_to_utf16LE (username_u,sizeof (username_u), username); +if (len>=0 ) + { +add_security_buffer (0x24 , username_u, len, phase3, &phase3_bufpos,sizeof (phase3)); + } + } +else + { +add_security_buffer (0x24 , username,strlen (username), phase3, &phase3_bufpos,sizeof (phase3)); } -/* username in ascii */ -add_security_buffer (0x24 , username,strlen (username), phase3, &phase3_bufpos);/* Set domain. If <domain> is empty, default domain will be used (i.e. proxy's domain) */ -add_security_buffer (0x1c , domain,strlen (domain), phase3, &phase3_bufpos); +if (ntlm_unicode) + { + len =utf8_to_utf16LE (domain_u,sizeof (domain_u), domain); +if (len>=0 ) + { +add_security_buffer (0x1c , domain_u, len, phase3, &phase3_bufpos,sizeof (phase3)); + } + } +else + { +add_security_buffer (0x1c , domain,strlen (domain), phase3, &phase3_bufpos,sizeof (phase3)); + }/* other security buffers will be empty */
@@ -365,7 +507,7 @@ ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2, struct gc_are phase3[0x38 ] = phase3_bufpos;/* no session key */ /* flags */ - phase3[0x3c ] =0x02 ;/* negotiate oem */ + phase3[0x3c ] = ntlm_unicode ?0x01 :0x02 ;/* negotiate unicode/oem */ phase3[0x3d ] =0x02 ;/* negotiate ntlm */ return ((const char *)make_base64_string2 ((unsigned char *)phase3, phase3_bufpos, gc));
Signature · 1 line
2.7.4
PATCH 2/2 http-proxy: Separate settings for user interaction and authentication auto-behavior
In reply to Holger KummertUser interaction is set with 'stdin' (one try) and 'interact' (repeated tries)
Authentication 'auto' and 'auto-nct' examine auth request from server. If NTLM is requested, NTLMv2 is tried first. Only on failure NTLMv1 is tried with no additional user interaction.
The other modes remain at one round of interaction.
--- doc/openvpn.8 | 22 +++++++------ src/openvpn/init.c | 3 +- src/openvpn/options.c | 88 +++++++++++++++++++++++++++++++++++++-------------- src/openvpn/proxy.c | 56 +++++++++++++++++++++++--------- src/openvpn/proxy.h | 22 +++++++------ 5 files changed, 132 insertions(+), 59 deletions(-)
doc/openvpn.8 +12 -10
@@ -496,7 +496,7 @@ Show sensed HTTP or SOCKS proxy settings. Currently, only Windows clients support this option. .\"********************************************************* .TP -.B \-\-http-proxy server port [authfile|'auto'|'auto-nct'] [auth-method] +.B \-\-http-proxy server port [authfile | 'interact'] [ auth-method ] Connect to remote host through an HTTP proxy at address .B server and port
@@ -504,17 +504,12 @@ and port If HTTP Proxy-Authenticate is required, .B authfile is a file containing a username and password on 2 lines, or -"stdin" to prompt from console. +"stdin"/"interact" to prompt from console where "interact" +prompts again after an auth failure. .B auth-method -should be one of "none", "basic", or "ntlm". - -HTTP Digest authentication is supported as well, but only via -the -.B auto -or -.B auto-nct -flags (below). +should be one of "none", "auto", "auto-nct", "basic", "digest", +"ntlm" or "ntlm2". The .B auto
@@ -529,6 +524,13 @@ The flag (no clear-text auth) instructs OpenVPN to automatically determine the authentication method, but to reject weak authentication protocols such as HTTP Basic Authentication. + +For both +.B auto +and +.B auto-nct +the server requirement for NTLM authentication leads to try +NTLMv2 first before NTLMv1. .\"********************************************************* .TP .B \-\-http-proxy-retry
src/openvpn/init.c +2 -1
@@ -140,7 +140,8 @@ management_callback_proxy_cmd (void *arg, const char **p) ho->server =string_alloc (p[2 ], gc); ho->port =string_alloc (p[3 ], gc); ho->retry = true; - ho->auth_retry = (p[4 ] &&streq (p[4 ],"nct" ) ?PAR_NCT :PAR_ALL ); + ho->auth_retry =AUTH_INTERACT ; + ho->auth_method_string =string_alloc (p[4 ] &&streq (p[4 ],"nct" ) ?"auto-nct" :"auto" , gc); ret = true;#endif }
src/openvpn/options.c +64 -24
111 lines not on this page · raw message
src/openvpn/proxy.c +41 -15
117 lines not on this page · raw message
src/openvpn/proxy.h +13 -9
@@ -31,12 +31,16 @@ #ifdef ENABLE_HTTP_PROXY /* HTTP CONNECT authentication methods */ -#define HTTP_AUTH_NONE 0 -#define HTTP_AUTH_BASIC 1 -#define HTTP_AUTH_DIGEST 2 -#define HTTP_AUTH_NTLM 3 -#define HTTP_AUTH_NTLM2 4 -#define HTTP_AUTH_N 5 /* number of HTTP_AUTH methods */ +enum { + HTTP_AUTH_NONE = 0, + HTTP_AUTH_AUTO, + HTTP_AUTH_AUTO_NCT, + HTTP_AUTH_BASIC, + HTTP_AUTH_DIGEST, + HTTP_AUTH_NTLM, + HTTP_AUTH_NTLM2, + HTTP_AUTH_N /* number of HTTP_AUTH methods */ +}; struct http_custom_header { const char *name;
@@ -50,9 +54,8 @@ struct http_proxy_options {bool retry;int timeout; -# define PAR_NO 0/* don't support any auth retries */ -# define PAR_ALL 1/* allow all proxy auth protocols */ -# define PAR_NCT 2/* disable cleartext proxy auth protocols */ +# define AUTH_NO_RETRY 0 +# define AUTH_INTERACT 1/* allow auth retries */ int auth_retry;const char *auth_method_string;
@@ -70,6 +73,7 @@ struct http_proxy_options_simple {struct http_proxy_info {bool defined; +int auth_auto;int auth_method;struct http_proxy_options options;struct user_pass up;
Signature · 1 line
1.8.4.5
Hi,
Quoted 1 line · Holger Kummert
Any opinions?
First of all, a big thank you for taking this on :-) - there have been a couple of bugs in our trac related to NTLM, but since none of the developers really understand NTLM auth or have means to test it, nothing ever happend.
I'll give the patch a thorough review in the next days and come back with questions...
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...Hello,
Am 16.04.2014 15:56, schrieb Gert Doering:
Quoted 6 lines
Hi,
Quoted 1 line · Holger Kummert
Any opinions?
First of all, a big thank you for taking this on :-) - there have been
you're welcome!
Quoted 3 lines
a couple of bugs in our trac related to NTLM, but since none of the developers really understand NTLM auth or have means to test it, nothing ever happend.
It seems that at least one is related: #259. The patches are based on current git master branch. Don't know if #94 is touched.
Quoted 3 lines
I'll give the patch a thorough review in the next days and come back with questions...
I'm looking forward this :-)
Holger
Quoted 3 lines
gert
Hi,
Quoted 1 line · Holger Kummert
Any opinions?
Any easy way to test this, without having a Windows domain around?
(I already run a number of test cases from my t_client test sets using socks proxy, http proxy, http proxy to ipv4 hosts over ipv6, ..., and if I could "easily" add a proxy with NTML auth to that mix, it would help ensure that we're not accidentially breaking things)
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...Hi,
Quoted 10 lines · Gert Doering
Hi,
Quoted 1 line · Holger Kummert
Any opinions?
Any easy way to test this, without having a Windows domain around?
(I already run a number of test cases from my
t_clienttest sets using socks proxy, http proxy, http proxy to ipv4 hosts over ipv6, ..., and if I could "easily" add a proxy with NTML auth to that mix, it would help ensure that we're not accidentially breaking things)
actually: no ; when the original NTML problems were reported I tried to reproduce them in my Windows Domain less environment and couldn't ; the existing code was working quite nicely; even setting up a Samba domain did not help - that also worked, IIRC. For proper testing you'd have to have a Windows domain, and even worse, different version of the Domain Controller as well (Win2003, 2007, 2012 etc).
JM2CW,
JJK
Quoted 3 lines
gert
[Openvpn-devel] Wanted: NTLM-Testers
Hi,
due to the lack of a testing environment for the main developers we are looking for someone who is willing to test the two patches which make the NTLMv1 and v2-code running (again?) and offer a clearer user interface for the configuration of http proxy authentication.
The developer of the patches already tested them, but a second proof that they really work would be very helpful.
The patches, based on the master branch, can be found at:
http://thread.gmane.org/gmane.network.openvpn.devel/8531
Thanks,
Holger Kummert
Am 19.04.2014 12:56, schrieb Jan Just Keijser:
Quoted 28 lines
Hi,
Quoted 10 lines · Gert Doering
Hi,
Quoted 1 line · Holger Kummert
Any opinions?
Any easy way to test this, without having a Windows domain around?
(I already run a number of test cases from my
t_clienttest sets using socks proxy, http proxy, http proxy to ipv4 hosts over ipv6, ..., and if I could "easily" add a proxy with NTML auth to that mix, it would help ensure that we're not accidentially breaking things)actually: no ; when the original NTML problems were reported I tried to reproduce them in my Windows Domain less environment and couldn't ; the existing code was working quite nicely; even setting up a Samba domain did not help - that also worked, IIRC. For proper testing you'd have to have a Windows domain, and even worse, different version of the Domain Controller as well (Win2003, 2007, 2012 etc).
JM2CW,
JJK
Quoted 3 lines
gert
Re: [Openvpn-devel] Wanted: NTLM-Testers
Quoted 8 lines · Holger Kummert
Hi,
due to the lack of a testing environment for the main developers we are looking for someone who is willing to test the two patches which make the NTLMv1 and v2-code running (again?) and offer a clearer user interface for the configuration of http proxy authentication.
Quoted 6 lines
The developer of the patches already tested them, but a second proof that they really work would be very helpful.
The patches, based on the master branch, can be found at:
As it happens, I've just added proxy auth support to the OpenConnect VPN client, and have a local squid instance set up to accept all of GSSAPI, NTLM, Digest and Basic auth.
Looking over the patches first... they make the client work in OEM or Unicode mode according to what the server asks for... but then only ever admit to supporting Unicode mode in the 'phase 1' packet anyway. So a sane server is *never* going to ask you to do OEM, and you might as well ditch all the conditional code for supporting that. (Quite frankly, even if you add the OEM flag to your phase 1 packet, a sane server in 2014 is *still* never going to ask you to do OEM mode anyway, is it?)
Also, it is strictly correct to assume that the input strings are utf-8? It certainly *ought* to be — again, it's 2014 so who in their right mind would be using non-UTF8 locales? But some nutters insist on remaining stuck in the 20th century...
You also don't seem to include the NTLM_NEGOTIATE_TARGET_INFO flag in your phase 1 packet, which means I'd be surprised if you ever actually get the target info back from a server in its challenge... which in turn means I'd be surprised if you'd actually exercised that part of the NTLMv2 code path. Or do some servers send it 'unsolicited'? Samba apparently doesn't send it even if I do set the TARGET_INFO flag in the phase_1 packet...
Yours is actually the first implementation of NTLMv2 auth that I've seen
— other implementations seem to have a combination of NTLMv1 and 'NTLMv2
session' authentication — see
http://curl.cofman.dk/rfc/ntlm.html#theNtlm2SessionResponse and
http://git.infradead.org/users/dwmw2/openconnect.git/commitdiff/fe5cf5fd67 for example. We send just the single phase1 packet, and the server either takes us up on the offer of NTLMv2 session auth, or not.
All that said, these patches do seem to work for NTLMv1 and NTLMv2 authentication to my test server. Although I need to explicitly *ask* for NTLMv2; if I just say 'ntlm' then it uses NTLMv1. That isn't what I expected after reading the commit message on the second patch.
And if I say '--http-proxy localhost 3128 auto', it doesn't work at all. And doesn't really tell me why, which is naughty of it:
Enter HTTP Proxy Username:dwoodhou
Enter HTTP Proxy Password:
Mon Jun 23 14:36:03 2014 SIGTERM[soft,init_instance] received, process exiting
However, one of the things that NTLM is supposed to offer me is
single-sign-on. If I've logged in to my workstation using my domain
password (and I have), then single-sign-on ought to work by using
Samba's /usr/bin/ntlm_auth helper tool. That works for other things like
Firefox and the Evolution mail client... and for proxy auth with
OpenConnect too, of course. There's a simple implementation at
http://git.infradead.org/users/dwmw2/openconnect.git/commitdiff/583ee4b6c3 which you are welcome to copy into OpenVPN under GPLv2.
Of course, GSSAPI support would also give me single-sign-on. But you don't support that either. Again, you're welcome to use my code from OpenConnect if it's useful. RFC1928 says that SOCKS implementations MUST support GSSAPI auth too, FWIW :)
Signature · 2 lines
David Woodhouse Open Source Technology Centre [email protected]... Intel Corporation
Re: [Openvpn-devel] Wanted: NTLM-Testers
Hello David,
thanks for taking time and reviewing and testing the code.
Am 23.06.2014 16:23, schrieb David Woodhouse:
Quoted 7 lines
Looking over the patches first... they make the client work in OEM or Unicode mode according to what the server asks for... but then only ever admit to supporting Unicode mode in the 'phase 1' packet anyway. So a sane server is *never* going to ask you to do OEM, and you might as well ditch all the conditional code for supporting that. (Quite frankly, even if you add the OEM flag to your phase 1 packet, a sane server in 2014 is *still* never going to ask you to do OEM mode anyway, is it?)
Are you sure that a server will never ask to use the OEM charset? If yes, the OEM code could of course be elimintated. Or is this more a kind of assumption?
Quoted 5 lines
Also, it is strictly correct to assume that the input strings are utf-8? It certainly *ought* to be — again, it's 2014 so who in their right mind would be using non-UTF8 locales? But some nutters insist on remaining stuck in the 20th century...
Yes, probably ...
But as I've written to Gert 3 weeks ago, this group has decided
to presume UTF-8-encoding _internally_. If an environment doesn't support it, or is misconfigured, the "outer interfaces" of Openvpn should take care of this (Manager, config file, etc.).
Quoted 8 lines
You also don't seem to include the
NTLM_NEGOTIATE_TARGET_INFOflag in your phase 1 packet, which means I'd be surprised if you ever actually get the target info back from a server in its challenge... which in turn means I'd be surprised if you'd actually exercised that part of the NTLMv2 code path. Or do some servers send it 'unsolicited'? Samba apparently doesn't send it even if I do set theTARGET_INFOflag in thephase_1packet...
Well the Windows 2008 server I'm testing against actually sends Target-Info. It seems that this server ignores what options are set in the initial phase-1-packet and has a somehow preconfigured phase-2-answer. That stiffness makes me a bit cautious in trusting the negotiation parameters like UTF (s.above).
Quoted 11 lines
Yours is actually the first implementation of NTLMv2 auth that I've seen
— other implementations seem to have a combination of NTLMv1 and 'NTLMv2
session' authentication — see
http://curl.cofman.dk/rfc/ntlm.html#theNtlm2SessionResponse and
http://git.infradead.org/users/dwmw2/openconnect.git/commitdiff/fe5cf5fd67 for example. We send just the single phase1 packet, and the server either takes us up on the offer of NTLMv2 session auth, or not.All that said, these patches do seem to work for NTLMv1 and NTLMv2 authentication to my test server. Although I need to explicitly *ask* for NTLMv2; if I just say 'ntlm' then it uses NTLMv1. That isn't what I expected after reading the commit message on the second patch.
This is just backward compatibility: 'ntlm' refers to NTLMv1, and the new 'ntlm2' to NTLMv2.
Quoted 7 lines
And if I say '--http-proxy localhost 3128 auto', it doesn't work at all. And doesn't really tell me why, which is naughty of it:
Enter HTTP Proxy Username:dwoodhou
Enter HTTP Proxy Password:
Mon Jun 23 14:36:03 2014 SIGTERM[soft,init_instance] received, process exiting
This looks a bit weird. Are you sure you applied both patches? Maybe the logging level should be raised.
At least some parts of the http handshake should be seen.
Quoted 9 lines
However, one of the things that NTLM is supposed to offer me is
single-sign-on. If I've logged in to my workstation using my domain
password (and I have), then single-sign-on ought to work by using
Samba's/usr/bin/ntlm_authhelper tool. That works for other things like
Firefox and the Evolution mail client... and for proxy auth with
OpenConnect too, of course. There's a simple implementation at
http://git.infradead.org/users/dwmw2/openconnect.git/commitdiff/583ee4b6c3 which you are welcome to copy into OpenVPN under GPLv2.
Or what about if you try to include this feature in OpenVpn? You know for sure much more about that code than me ...
Quoted 6 lines
Of course, GSSAPI support would also give me single-sign-on. But you don't support that either. Again, you're welcome to use my code from OpenConnect if it's useful. RFC1928 says that SOCKS implementations MUST support GSSAPI auth too, FWIW :)
Best regards,
Holger
Re: [Openvpn-devel] Wanted: NTLM-Testers
Quoted 13 lines · Holger Kummert
Hello David,
thanks for taking time and reviewing and testing the code.
Am 23.06.2014 16:23, schrieb David Woodhouse:
Quoted 7 lines
Looking over the patches first... they make the client work in OEM or Unicode mode according to what the server asks for... but then only ever admit to supporting Unicode mode in the 'phase 1' packet anyway. So a sane server is *never* going to ask you to do OEM, and you might as well ditch all the conditional code for supporting that. (Quite frankly, even if you add the OEM flag to your phase 1 packet, a sane server in 2014 is *still* never going to ask you to do OEM mode anyway, is it?)
Quoted 3 lines
Are you sure that a server will never ask to use the OEM charset? If yes, the OEM code could of course be elimintated. Or is this more a kind of assumption?
My understanding is that the phase 1 packet should indicate which of {OEM, Unicode} you support, and the server shouldn't ask for something that you said you didn't support.
So if you don't set the OEM bit in your phase 1 packet, no server should ask you to do OEM mode.
Quoted 11 lines
Quoted 5 lines
Also, it is strictly correct to assume that the input strings are utf-8? It certainly *ought* to be — again, it's 2014 so who in their right mind would be using non-UTF8 locales? But some nutters insist on remaining stuck in the 20th century...
Yes, probably ...
But as I've written to Gert 3 weeks ago, this group has decided to presume UTF-8-encoding _internally_. If an environment doesn't support it, or is misconfigured, the "outer interfaces" of Openvpn should take care of this (Manager, config file, etc.).
Makes sense.
Quoted 9 lines
Quoted 8 lines
You also don't seem to include the
NTLM_NEGOTIATE_TARGET_INFOflag in your phase 1 packet, which means I'd be surprised if you ever actually get the target info back from a server in its challenge... which in turn means I'd be surprised if you'd actually exercised that part of the NTLMv2 code path. Or do some servers send it 'unsolicited'? Samba apparently doesn't send it even if I do set theTARGET_INFOflag in thephase_1packet...
Quoted 5 lines
Well the Windows 2008 server I'm testing against actually sends Target-Info. It seems that this server ignores what options are set in the initial phase-1-packet and has a somehow preconfigured phase-2-answer. That stiffness makes me a bit cautious in trusting the negotiation parameters like UTF (s.above).
Hm, fair enough. For OEM/Unicode other implementations certainly do seem to get away with simply specifying which mode they want in the phase 1 packet, then expecting the server to respond in kind. Other implementations have tended to start supporting Unicode over the years just by making the change in both places unconditionally. Like this:
https://git.gnome.org/browse/libsoup/commit/?id=67824e62
Quoted 6 lines
Quoted 6 lines
Yours is actually the first implementation of NTLMv2 auth that I've seen
— other implementations seem to have a combination of NTLMv1 and 'NTLMv2
session' authentication — see
http://curl.cofman.dk/rfc/ntlm.html#theNtlm2SessionResponse and
http://git.infradead.org/users/dwmw2/openconnect.git/commitdiff/fe5cf5fd67 for example. We send just the single phase1 packet, and the server either takes us up on the offer of NTLMv2 session auth, or not.
Quoted 7 lines
Quoted 4 lines
All that said, these patches do seem to work for NTLMv1 and NTLMv2 authentication to my test server. Although I need to explicitly *ask* for NTLMv2; if I just say 'ntlm' then it uses NTLMv1. That isn't what I expected after reading the commit message on the second patch.
This is just backward compatibility: 'ntlm' refers to NTLMv1, and the new 'ntlm2' to NTLMv2.
Ok. But don't we want to discourage the use of NTLMv1 wherever possible?
Quoted 10 lines
Quoted 6 lines
And if I say '--http-proxy localhost 3128 auto', it doesn't work at all. And doesn't really tell me why, which is naughty of it:
Enter HTTP Proxy Username:dwoodhou
Enter HTTP Proxy Password:
Mon Jun 23 14:36:03 2014 SIGTERM[soft,init_instance] received, process exitingThis looks a bit weird. Are you sure you applied both patches? Maybe the logging level should be raised.
At least some parts of the http handshake should be seen.
Oh, it was trying Digest auth. The username/password I gave it doesn't work with Digest auth; only NTLM or Basic. Perhaps that's just a weird setup on the part of my squid server, since I couldn't make Digest auth work against the Active Directory domain so it has a separate passwd file.
Either way, it ought to clearly state what happened, not just report SIGTERM after asking for the password. I shouldn't need to ask for debugging information to make it report an authentication failure.
Quoted 10 lines
Quoted 7 lines
However, one of the things that NTLM is supposed to offer me is
single-sign-on. If I've logged in to my workstation using my domain
password (and I have), then single-sign-on ought to work by using
Samba's/usr/bin/ntlm_authhelper tool. That works for other things like
Firefox and the Evolution mail client... and for proxy auth with
OpenConnect too, of course. There's a simple implementation at
http://git.infradead.org/users/dwmw2/openconnect.git/commitdiff/583ee4b6c3 which you are welcome to copy into OpenVPN under GPLv2.Or what about if you try to include this feature in OpenVpn? You know for sure much more about that code than me ...
Heh. Even implementing it in OpenConnect was something I was doing *instead* of my day job... :)
Signature · 1 line
dwmw2