libcli/auth/ntlmssp Be clear about talloc parents for session keys

The previous API was not clear as to who owned the returned session key.
This fixes a valgrind-found use-after-free in the NTLMSSP key derivation code,
and avoids making allocations - we steal and zero instead.

Andrew Bartlett

Signed-off-by: Andrew Tridgell <tridge@samba.org>

3 files changed, 24 insertions, 14 deletions

diff --git a/source3/auth/auth_ntlmssp.c b/source3/auth/auth_ntlmssp.c
index aa7998cf63..af3a6f3827 100644
--- a/source3/auth/auth_ntlmssp.c
+++ b/source3/auth/auth_ntlmssp.c
@@ -26,12 +26,13 @@
 #include "../librpc/gen_ndr/netlogon.h"
 
 NTSTATUS auth_ntlmssp_steal_server_info(TALLOC_CTX *mem_ctx,
-				struct auth_ntlmssp_state *auth_ntlmssp_state,
-				struct auth_serversupplied_info **server_info)
+					struct auth_ntlmssp_state *auth_ntlmssp_state,
+					struct auth_serversupplied_info **server_info)
 {
 	/* Free the current server_info user_session_key and reset it from the
 	 * current ntlmssp_state session_key */
 	data_blob_free(&auth_ntlmssp_state->server_info->user_session_key);
+	/* Set up the final session key for the connection */
 	auth_ntlmssp_state->server_info->user_session_key =
 		data_blob_talloc(
 			auth_ntlmssp_state->server_info,
@@ -105,7 +106,8 @@ static NTSTATUS auth_ntlmssp_set_challenge(struct ntlmssp_state *ntlmssp_state,
  * Return the session keys used on the connection.
  */
 
-static NTSTATUS auth_ntlmssp_check_password(struct ntlmssp_state *ntlmssp_state, DATA_BLOB *user_session_key, DATA_BLOB *lm_session_key) 
+static NTSTATUS auth_ntlmssp_check_password(struct ntlmssp_state *ntlmssp_state, TALLOC_CTX *mem_ctx,
+					    DATA_BLOB *user_session_key, DATA_BLOB *lm_session_key)
 {
 	struct auth_ntlmssp_state *auth_ntlmssp_state =
 		(struct auth_ntlmssp_state *)ntlmssp_state->callback_private;
@@ -160,19 +162,24 @@ static NTSTATUS auth_ntlmssp_check_password(struct ntlmssp_state *ntlmssp_state,
 		return nt_status;
 	}
 
+	/* Clear out the session keys, and pass them to the caller.
+	 * They will not be used in this form again - instead the
+	 * NTLMSSP code will decide on the final correct session key,
+	 * and put it back here at the end of
+	 * auth_ntlmssp_steal_server_info */
 	if (auth_ntlmssp_state->server_info->user_session_key.length) {
 		DEBUG(10, ("Got NT session key of length %u\n",
 			(unsigned int)auth_ntlmssp_state->server_info->user_session_key.length));
-		*user_session_key = data_blob_talloc(auth_ntlmssp_state,
-						   auth_ntlmssp_state->server_info->user_session_key.data,
-						   auth_ntlmssp_state->server_info->user_session_key.length);
+		*user_session_key = auth_ntlmssp_state->server_info->user_session_key;
+		talloc_steal(mem_ctx, auth_ntlmssp_state->server_info->user_session_key.data);
+		auth_ntlmssp_state->server_info->user_session_key = data_blob_null;
 	}
 	if (auth_ntlmssp_state->server_info->lm_session_key.length) {
 		DEBUG(10, ("Got LM session key of length %u\n",
 			(unsigned int)auth_ntlmssp_state->server_info->lm_session_key.length));
-		*lm_session_key = data_blob_talloc(auth_ntlmssp_state,
-						   auth_ntlmssp_state->server_info->lm_session_key.data,
-						   auth_ntlmssp_state->server_info->lm_session_key.length);
+		*lm_session_key = auth_ntlmssp_state->server_info->lm_session_key;
+		talloc_steal(mem_ctx, auth_ntlmssp_state->server_info->lm_session_key.data);
+		auth_ntlmssp_state->server_info->lm_session_key = data_blob_null;
 	}
 	return nt_status;
 }
diff --git a/source3/smbd/sesssetup.c b/source3/smbd/sesssetup.c
index 58b446da17..0b999b348a 100644
--- a/source3/smbd/sesssetup.c
+++ b/source3/smbd/sesssetup.c
@@ -405,6 +405,7 @@ static void reply_spnego_kerberos(struct smb_request *req,
 	}
 
 	data_blob_free(&server_info->user_session_key);
+	/* Set the kerberos-derived session key onto the server_info */
 	server_info->user_session_key = session_key;
 	talloc_steal(server_info, session_key.data);
 
diff --git a/source3/utils/ntlm_auth.c b/source3/utils/ntlm_auth.c
index 82819cf145..38ed9f7c9b 100644
--- a/source3/utils/ntlm_auth.c
+++ b/source3/utils/ntlm_auth.c
@@ -565,7 +565,8 @@ static NTSTATUS contact_winbind_change_pswd_auth_crap(const char *username,
     return nt_status;
 }
 
-static NTSTATUS winbind_pw_check(struct ntlmssp_state *ntlmssp_state, DATA_BLOB *user_session_key, DATA_BLOB *lm_session_key) 
+static NTSTATUS winbind_pw_check(struct ntlmssp_state *ntlmssp_state, TALLOC_CTX *mem_ctx,
+				 DATA_BLOB *user_session_key, DATA_BLOB *lm_session_key)
 {
 	static const char zeros[16] = { 0, };
 	NTSTATUS nt_status;
@@ -585,13 +586,13 @@ static NTSTATUS winbind_pw_check(struct ntlmssp_state *ntlmssp_state, DATA_BLOB
 
 	if (NT_STATUS_IS_OK(nt_status)) {
 		if (memcmp(lm_key, zeros, 8) != 0) {
-			*lm_session_key = data_blob_talloc(ntlmssp_state, NULL, 16);
+			*lm_session_key = data_blob_talloc(mem_ctx, NULL, 16);
 			memcpy(lm_session_key->data, lm_key, 8);
 			memset(lm_session_key->data+8, '\0', 8);
 		}
 
 		if (memcmp(user_sess_key, zeros, 16) != 0) {
-			*user_session_key = data_blob_talloc(ntlmssp_state, user_sess_key, 16);
+			*user_session_key = data_blob_talloc(mem_ctx, user_sess_key, 16);
 		}
 		ntlmssp_state->callback_private = talloc_strdup(ntlmssp_state,
 								unix_name);
@@ -609,14 +610,15 @@ static NTSTATUS winbind_pw_check(struct ntlmssp_state *ntlmssp_state, DATA_BLOB
 	return nt_status;
 }
 
-static NTSTATUS local_pw_check(struct ntlmssp_state *ntlmssp_state, DATA_BLOB *user_session_key, DATA_BLOB *lm_session_key) 
+static NTSTATUS local_pw_check(struct ntlmssp_state *ntlmssp_state, TALLOC_CTX *mem_ctx,
+			       DATA_BLOB *user_session_key, DATA_BLOB *lm_session_key)
 {
 	NTSTATUS nt_status;
 	struct samr_Password lm_pw, nt_pw;
 
 	nt_lm_owf_gen (opt_password, nt_pw.hash, lm_pw.hash);
 
-	nt_status = ntlm_password_check(ntlmssp_state,
+	nt_status = ntlm_password_check(mem_ctx,
 					true, true, 0,
 					&ntlmssp_state->chal,
 					&ntlmssp_state->lm_resp,