From 6832d5e9334f93d2b41fa50580379a2381311748 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Thu, 16 Sep 2010 14:37:20 +1000 Subject: [PATCH] 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 --- libcli/auth/ntlmssp.h | 4 +++- libcli/auth/ntlmssp_server.c | 12 ++++-------- source3/auth/auth_ntlmssp.c | 25 ++++++++++++++++--------- source3/smbd/sesssetup.c | 1 + source3/utils/ntlm_auth.c | 12 +++++++----- source4/auth/ntlmssp/ntlmssp_server.c | 5 +++++ 6 files changed, 36 insertions(+), 23 deletions(-) diff --git a/libcli/auth/ntlmssp.h b/libcli/auth/ntlmssp.h index d0a282c3509..dead412c99c 100644 --- a/libcli/auth/ntlmssp.h +++ b/libcli/auth/ntlmssp.h @@ -129,11 +129,13 @@ struct ntlmssp_state * * The callback must reads the feilds of this structure for the information it needs on the user * @param ntlmssp_state This structure + * @param mem_ctx Talloc context for LM and NT session key to be returned on * @param nt_session_key If an NT session key is returned by the authentication process, return it here * @param lm_session_key If an LM session key is returned by the authentication process, return it here * */ - NTSTATUS (*check_password)(struct ntlmssp_state *ntlmssp_state, DATA_BLOB *nt_session_key, DATA_BLOB *lm_session_key); + NTSTATUS (*check_password)(struct ntlmssp_state *ntlmssp_state, TALLOC_CTX *mem_ctx, + DATA_BLOB *nt_session_key, DATA_BLOB *lm_session_key); union ntlmssp_crypt_state *crypt; }; diff --git a/libcli/auth/ntlmssp_server.c b/libcli/auth/ntlmssp_server.c index 3627c4d7aed..f78698af1f9 100644 --- a/libcli/auth/ntlmssp_server.c +++ b/libcli/auth/ntlmssp_server.c @@ -478,7 +478,6 @@ static NTSTATUS ntlmssp_server_postauth(struct ntlmssp_state *ntlmssp_state, if (ntlmssp_state->neg_flags & NTLMSSP_NEGOTIATE_KEY_EXCH) { if (!state->encrypted_session_key.data || state->encrypted_session_key.length != 16) { - data_blob_free(&state->encrypted_session_key); DEBUG(1, ("Client-supplied KEY_EXCH session key was of invalid length (%u)!\n", (unsigned)state->encrypted_session_key.length)); return NT_STATUS_INVALID_PARAMETER; @@ -486,6 +485,7 @@ static NTSTATUS ntlmssp_server_postauth(struct ntlmssp_state *ntlmssp_state, DEBUG(5, ("server session key is invalid (len == %u), cannot do KEY_EXCH!\n", (unsigned int)session_key.length)); ntlmssp_state->session_key = session_key; + talloc_steal(ntlmssp_state, session_key.data); } else { dump_data_pw("KEY_EXCH session key (enc):\n", state->encrypted_session_key.data, @@ -499,10 +499,10 @@ static NTSTATUS ntlmssp_server_postauth(struct ntlmssp_state *ntlmssp_state, dump_data_pw("KEY_EXCH session key:\n", state->encrypted_session_key.data, state->encrypted_session_key.length); - talloc_free(session_key.data); } } else { ntlmssp_state->session_key = session_key; + talloc_steal(ntlmssp_state, session_key.data); } if (ntlmssp_state->session_key.length) { @@ -555,6 +555,7 @@ NTSTATUS ntlmssp_server_auth(struct ntlmssp_state *ntlmssp_state, /* Finally, actually ask if the password is OK */ nt_status = ntlmssp_state->check_password(ntlmssp_state, + state, &state->user_session_key, &state->lm_session_key); if (!NT_STATUS_IS_OK(nt_status)) { @@ -567,11 +568,6 @@ NTSTATUS ntlmssp_server_auth(struct ntlmssp_state *ntlmssp_state, can be done in a callback */ nt_status = ntlmssp_server_postauth(ntlmssp_state, state); - if (!NT_STATUS_IS_OK(nt_status)) { - TALLOC_FREE(state); - return nt_status; - } - TALLOC_FREE(state); - return NT_STATUS_OK; + return nt_status; } diff --git a/source3/auth/auth_ntlmssp.c b/source3/auth/auth_ntlmssp.c index aa7998cf639..af3a6f38278 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 58b446da172..0b999b348a8 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 82819cf1455..38ed9f7c9bb 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, diff --git a/source4/auth/ntlmssp/ntlmssp_server.c b/source4/auth/ntlmssp/ntlmssp_server.c index 6e3cf8a8fff..8623c1da8ed 100644 --- a/source4/auth/ntlmssp/ntlmssp_server.c +++ b/source4/auth/ntlmssp/ntlmssp_server.c @@ -149,6 +149,7 @@ static NTSTATUS auth_ntlmssp_set_challenge(struct ntlmssp_state *ntlmssp_state, */ 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 gensec_ntlmssp_context *gensec_ntlmssp = @@ -188,11 +189,15 @@ static NTSTATUS auth_ntlmssp_check_password(struct ntlmssp_state *ntlmssp_state, DEBUG(10, ("Got NT session key of length %u\n", (unsigned)gensec_ntlmssp->server_info->user_session_key.length)); *user_session_key = gensec_ntlmssp->server_info->user_session_key; + talloc_steal(mem_ctx, user_session_key->data); + gensec_ntlmssp->server_info->user_session_key = data_blob_null; } if (gensec_ntlmssp->server_info->lm_session_key.length) { DEBUG(10, ("Got LM session key of length %u\n", (unsigned)gensec_ntlmssp->server_info->lm_session_key.length)); *lm_session_key = gensec_ntlmssp->server_info->lm_session_key; + talloc_steal(mem_ctx, lm_session_key->data); + gensec_ntlmssp->server_info->lm_session_key = data_blob_null; } return nt_status; } -- 2.34.1