From 3c7db0fb3c116da025839c82afc5835c453f9ee0 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Mon, 7 Nov 2022 17:21:44 +0100 Subject: [PATCH] CVE-2021-20251: s4:auth: fix use after free in authsam_logon_success_accounting() This fixes a use after free problem introduced by commit 7b8e32efc336fb728e0c7e3dd6fbe2ed54122124, which has msg = current; which means the lifetime of the 'msg' memory is no longer in the scope of th caller. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 BUG: https://bugzilla.samba.org/show_bug.cgi?id=15253 Signed-off-by: Stefan Metzmacher Reviewed-by: Andrew Bartlett (cherry picked from commit 1414269dccfd7cb831889cc92df35920b034457c) --- source4/auth/ntlm/auth_sam.c | 1 + source4/auth/ntlm/auth_winbind.c | 2 +- source4/auth/sam.c | 9 ++++++++- source4/auth/tests/sam.c | 24 ++++++++++++------------ source4/kdc/hdb-samba4.c | 2 +- source4/kdc/mit_samba.c | 4 ++-- 6 files changed, 25 insertions(+), 17 deletions(-) diff --git a/source4/auth/ntlm/auth_sam.c b/source4/auth/ntlm/auth_sam.c index 882d92e26ed3..0d5043124aab 100644 --- a/source4/auth/ntlm/auth_sam.c +++ b/source4/auth/ntlm/auth_sam.c @@ -787,6 +787,7 @@ static NTSTATUS authsam_authenticate(struct auth4_context *auth_context, nt_status = authsam_logon_success_accounting(auth_context->sam_ctx, msg, domain_dn, interactive, + tmp_ctx, &send_to_sam); if (send_to_sam != NULL) { diff --git a/source4/auth/ntlm/auth_winbind.c b/source4/auth/ntlm/auth_winbind.c index 6381f866667e..719d877a1703 100644 --- a/source4/auth/ntlm/auth_winbind.c +++ b/source4/auth/ntlm/auth_winbind.c @@ -256,7 +256,7 @@ static void winbind_check_password_done(struct tevent_req *subreq) ctx->auth_ctx->sam_ctx, msg, domain_dn, user_info->flags & USER_INFO_INTERACTIVE_LOGON, - NULL); + NULL, NULL); if (tevent_req_nterror(req, status)) { return; } diff --git a/source4/auth/sam.c b/source4/auth/sam.c index 219ee10d5bd3..f2e5ced6caf8 100644 --- a/source4/auth/sam.c +++ b/source4/auth/sam.c @@ -1396,6 +1396,7 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx, const struct ldb_message *msg, struct ldb_dn *domain_dn, bool interactive_or_kerberos, + TALLOC_CTX *send_to_sam_mem_ctx, struct netr_SendToSamBase **send_to_sam) { int ret; @@ -1612,7 +1613,13 @@ get_transaction: if (dbBadPwdCount != 0 && send_to_sam != NULL) { struct netr_SendToSamBase *base_msg; struct GUID guid = samdb_result_guid(msg, "objectGUID"); - base_msg = talloc_zero(msg, struct netr_SendToSamBase); + + base_msg = talloc_zero(send_to_sam_mem_ctx, + struct netr_SendToSamBase); + if (base_msg == NULL) { + status = NT_STATUS_NO_MEMORY; + goto error; + } base_msg->message_type = SendToSamResetBadPasswordCount; base_msg->message_size = 16; diff --git a/source4/auth/tests/sam.c b/source4/auth/tests/sam.c index b39408c3699a..e1e2c69b8630 100644 --- a/source4/auth/tests/sam.c +++ b/source4/auth/tests/sam.c @@ -1446,7 +1446,7 @@ static void test_success_accounting_start_txn_failed(void **state) { ldb_transaction_start_ret = LDB_ERR_OPERATIONS_ERROR; status = authsam_logon_success_accounting( - ldb, msg, domain_dn, true, NULL); + ldb, msg, domain_dn, true, NULL, NULL); assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); /* @@ -1502,7 +1502,7 @@ static void test_success_accounting_reread_failed(void **state) { will_return(__wrap_dsdb_search_dn, LDB_ERR_NO_SUCH_OBJECT); status = authsam_logon_success_accounting( - ldb, msg, domain_dn, true, NULL); + ldb, msg, domain_dn, true, NULL, NULL); assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); assert_true(transaction_cancelled); @@ -1561,7 +1561,7 @@ static void test_success_accounting_ldb_msg_new_failed(void **state) { ldb_msg_new_fail = true; status = authsam_logon_success_accounting( - ldb, msg, domain_dn, true, NULL); + ldb, msg, domain_dn, true, NULL, NULL); assert_true(NT_STATUS_EQUAL(status, NT_STATUS_NO_MEMORY)); assert_true(transaction_cancelled); @@ -1612,7 +1612,7 @@ static void test_success_accounting_samdb_rodc_failed(void **state) { samdb_rodc_ret = LDB_ERR_OPERATIONS_ERROR; status = authsam_logon_success_accounting( - ldb, msg, domain_dn, true, NULL); + ldb, msg, domain_dn, true, NULL, NULL); assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); assert_false(in_transaction); assert_false(transaction_cancelled); @@ -1675,7 +1675,7 @@ static void test_success_accounting_update_lastlogon_failed(void **state) { will_return(__wrap_samdb_msg_add_int64, LDB_ERR_OPERATIONS_ERROR); status = authsam_logon_success_accounting( - ldb, msg, domain_dn, true, NULL); + ldb, msg, domain_dn, true, NULL, NULL); assert_true(NT_STATUS_EQUAL(status, NT_STATUS_NO_MEMORY)); assert_true(transaction_cancelled); @@ -1737,7 +1737,7 @@ static void test_success_accounting_build_mod_req_failed(void **state) { will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); status = authsam_logon_success_accounting( - ldb, msg, domain_dn, true, NULL); + ldb, msg, domain_dn, true, NULL, NULL); assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); assert_true(transaction_cancelled); @@ -1800,7 +1800,7 @@ static void test_success_accounting_add_control_failed(void **state) { will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); status = authsam_logon_success_accounting( - ldb, msg, domain_dn, true, NULL); + ldb, msg, domain_dn, true, NULL, NULL); assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); assert_true(transaction_cancelled); @@ -1863,7 +1863,7 @@ static void test_success_accounting_ldb_request_failed(void **state) { will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); status = authsam_logon_success_accounting( - ldb, msg, domain_dn, true, NULL); + ldb, msg, domain_dn, true, NULL, NULL); assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); assert_true(transaction_cancelled); @@ -1926,7 +1926,7 @@ static void test_success_accounting_ldb_wait_failed(void **state) { will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); status = authsam_logon_success_accounting( - ldb, msg, domain_dn, true, NULL); + ldb, msg, domain_dn, true, NULL, NULL); assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); assert_true(transaction_cancelled); @@ -1989,7 +1989,7 @@ static void test_success_accounting_commit_failed(void **state) { will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); status = authsam_logon_success_accounting( - ldb, msg, domain_dn, true, NULL); + ldb, msg, domain_dn, true, NULL, NULL); assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); assert_true(in_transaction); assert_false(transaction_cancelled); @@ -2055,7 +2055,7 @@ static void test_success_accounting_rollback_failed(void **state) { will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); status = authsam_logon_success_accounting( - ldb, msg, domain_dn, true, NULL); + ldb, msg, domain_dn, true, NULL, NULL); assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); assert_true(in_transaction); assert_false(transaction_cancelled); @@ -2124,7 +2124,7 @@ static void test_success_accounting_spurious_bad_pwd_indicator(void **state) { ldb_build_mod_req_res = talloc_zero(ctx, struct ldb_request); status = authsam_logon_success_accounting( - ldb, msg, domain_dn, true, NULL); + ldb, msg, domain_dn, true, NULL, NULL); assert_true(NT_STATUS_EQUAL(status, NT_STATUS_OK)); assert_false(in_transaction); assert_false(transaction_cancelled); diff --git a/source4/kdc/hdb-samba4.c b/source4/kdc/hdb-samba4.c index 96bb3d858f04..e465a288ca02 100644 --- a/source4/kdc/hdb-samba4.c +++ b/source4/kdc/hdb-samba4.c @@ -650,7 +650,7 @@ static krb5_error_code hdb_samba4_audit(krb5_context context, * in the PAC here or re-calculate it. */ status = authsam_logon_success_accounting(kdc_db_ctx->samdb, p->msg, - domain_dn, true, &send_to_sam); + domain_dn, true, frame, &send_to_sam); if (NT_STATUS_EQUAL(status, NT_STATUS_ACCOUNT_LOCKED_OUT)) { final_ret = KRB5KDC_ERR_CLIENT_REVOKED; r->error_code = final_ret; diff --git a/source4/kdc/mit_samba.c b/source4/kdc/mit_samba.c index e316c57ee31b..bfe9748a068b 100644 --- a/source4/kdc/mit_samba.c +++ b/source4/kdc/mit_samba.c @@ -1031,7 +1031,7 @@ out: void mit_samba_zero_bad_password_count(krb5_db_entry *db_entry) { - struct netr_SendToSamBase *send_to_sam = NULL; + /* struct netr_SendToSamBase *send_to_sam = NULL; */ struct samba_kdc_entry *p = talloc_get_type_abort(db_entry->e_data, struct samba_kdc_entry); struct ldb_dn *domain_dn; @@ -1042,7 +1042,7 @@ void mit_samba_zero_bad_password_count(krb5_db_entry *db_entry) p->msg, domain_dn, true, - &send_to_sam); + NULL, NULL); /* TODO: RODC support */ } -- 2.34.1