GNOME Bugzilla – Bug 739192
NTLMv2 Session Security support
Last modified: 2015-02-28 14:55:31 UTC
Overview: libsoup fails to authenticate to servers that require the NTLMv2 Session Security protocol to be used for the authentication handshake. (Note that "NTLMv2 Session Security" is not actually a part of NTLMv2; it's effectively a backport of a subset of NTLMv2 to NTLMv1.) Steps to Reproduce: 1) Configure a server to require NTLM2 Session auth 2) Using libsoup, and with valid credentials, attempt to request a URL that requires authentication Actual Results: Authentication fails; the server rejects the connection. Expected Results: Authentication should succeed. Libsoup should successfully retrieve the content at the specified URL. Build Date & Platform: The "libsoup2.4" package as included with Ubuntu 14.04 and 14.10 as of today's date, October 25, 2014. (2.44.0 and 2.46.0, respectively.) Unless I'm mis-reading the source code (?), this is still present as of the current revision in git. Additional Information: Any other useful information. Protocol is as documented here: http://davenport.sourceforge.net/ntlm.html#theNtlm2SessionResponse http://msdn.microsoft.com/en-us/library/cc236650.aspx (the "P" bit)
Created attachment 289331 [details] [review] Draft patch implementing this functionality I've implemented this functionality for my own use. Not the best code, but it seems to work. Feedback welcome. I'd certainly appreciate having this functionality integrated into the core libsoup release.
Comment on attachment 289331 [details] [review] Draft patch implementing this functionality > http://msdn.microsoft.com/en-us/library/cc236650.aspx oh wow... that didn't exist back when we first implemented this... In general the code looks great. A few comments: > #define NTLM_RESPONSE_FLAGS 0x8201 >+#define NTLM2_RESPONSE_FLAGS 0x00080000 That makes it sound like we're going to send *just* 0x00080000 for NTLM2, rather than adding that value to NTLM_RESPONSE_FLAGS. Maybe just call that define "NTLMSSP_NEGOTIATE_EXTENDED_SESSIONSECURITY" like in the MSDN docs? Also, it would probably be clearer at this point to make NTLM_RESPONSE_FLAGS be a 32-bit constant rather than 16-bit too. >+ int flags; ... >+ memcpy(&flags, chall + NTLM_CHALLENGE_FLAGS_OFFSET, sizeof(flags)); I wonder if it would simplify things at this point if we had an NTLMChallenge struct like the NTLMResponse struct, rather than a bunch of offset defines? Either way, you need to use a guint32 for "flags" rather than assuming that "int" will be the right size, and you need to call GUINT32_FROM_LE() on it. (Since the value in the challenge is little-endian but the NTLM_FLAGS_NEGOTIATE_NTLMV2 constant will be the host's native endianness.) >+static void >+calc_ntlm2_session_response(const char *nonce, style consistency nitpick: always put a space between function names and "("s (in both declarations and calls). >+ int client_nonce[2]; again, use an explicitly-sized int type. Actually, despite its name, it looks like g_random_int() returns guint32, not a signed int, so you should use guint32 here. >+ client_nonce[0] = g_random_int(); >+ client_nonce[1] = g_random_int(); GRandom isn't a cryptographically-secure RNG, so this is not awesome... Maybe for now just add a comment: /* FIXME: if GLib ever gets a more secure random number generator, use it here */ >+ // Compute nt_hash as usual but with a new nonce >+ calc_response (nt_hash, (guchar *)ntlmv2_hash, nt_resp); That cast shouldn't be needed? > resp.flags = GUINT32_TO_LE (NTLM_RESPONSE_FLAGS); >+ if (ntlmv2_session) >+ resp.flags |= NTLM2_RESPONSE_FLAGS; endianness issues here again
Created attachment 292284 [details] [review] Draft patch v2 Finally got back to looking at this. Attaching an updated patch. Thanks for catching the endianness and int-size issues. I've gotten too comfortable with x86-land; missed those. If I missed anything or fixed anything incorrectly, please let me know. I think you're probably right that an NTLMChallenge struct would make sense. But I'm lazy, and this works :-) Will make the change if I end up doing much more work on this code. (If full NTLMv2 support ever gets added, it'd probably be worth re-visiting a bunch of this structure at that point anyway.) I'm more concerned about the RNG issue. I added the comment as suggested. You're right that it's not secure, though. What are the compatibility expectations for this code? I'm more than a little tempted to just open /dev/random and read a few bytes; how bad an idea would that be? I also added a basic test for the new code. The test turns up something funny with winbind external auth. But I don't think it's related to my code -- that block of test code always errors out the second time it's run, whether it's using the new auth method or the old/existing one. Thoughts welcome, on this issue and/or on the test more generally.
As documented in bug 736554, I can confirm that the patch is working with 2.46.0.
Created attachment 293419 [details] [review] Draft patch v3 Uploading a third patch. Some changes: - Modify the patch to use "/dev/urandom", rather than g_random_int(), for the client nonce on platforms that support it. (Continue to use g_random_int() as a fallback.) - Update unit test. That part of the patch now applies to libsoup 2.46. (Previously it was based on 2.44.) Also, I think, generally made it cleaner. I'm not obsoleting the previous version of the patch because it's not clear to me that these changes are strictly better. There should be no functional difference between these two patches in terms of the resulting compiled .so file, other than that it would be slightly harder for an attacker to predict the client nonce.
I can confirm that the patch is also working with 2.48.1
*bump* -- is there anything I could do to help this patch get considered and/or accepted? Any thoughts?
[mass-moving all "UNCONFIRMED" libsoup bugs to "NEW" after disabling the "UNCONFIRMED" status for this product now that bugzilla.gnome.org allows that. bugspam-libsoup-20150210]
works well and makes my day. Can we push it and get it released in libsoup.?
Committed to master, and will be in next week's (unstable) release. (I removed the /dev/urandom parts; it just seemed weird to be using that directly...)
Thanks Dan! Hopefully GLib will gain a better RNG sometime soon.