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>

diff --git a/libcli/auth/ntlmssp.h b/libcli/auth/ntlmssp.h
index d0a282c35096759b9d152e618e1aa2f2c7b8ebe1..dead412c99ce5316a393feadc8a81f50812140b8 100644 (file)
--- 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 3627c4d7aed0e05947be7056e08a0f135a0a805d..f78698af1f97bf73835432efe8c6c6f6e0ea121e 100644 (file)
--- 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 aa7998cf639508d4bd55ff90d37d9fcf167622cb..af3a6f382787390c726e37e77a0a86455545f551 100644 (file)
--- 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 58b446da17294f33481ecd744543c9bd90075462..0b999b348a8fee5f8f9ed5d60095eb0e0249cea3 100644 (file)
--- 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 82819cf1455f02282061dbaadcb947142a8a3777..38ed9f7c9bbf5084e3032a62ecfe73f8350aa4fa 100644 (file)
--- 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 6e3cf8a8ffff32eec82f9b56066e3375a6303aa9..8623c1da8ed3e92793cf159c1592bf258e2fb7fa 100644 (file)
--- 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;
 }