From: Andrew Bartlett Date: Tue, 30 Mar 2021 05:01:39 +0000 (+1300) Subject: CVE-2021-20251 s4 auth: make bad password count increment atomic X-Git-Tag: talloc-2.4.0~1081 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=de4cc0a3dae89f3e51a099282615cf80c8539e11;p=thirdparty%2Fsamba.git CVE-2021-20251 s4 auth: make bad password count increment atomic Ensure that the bad password count is incremented atomically, and that the successful logon accounting data is updated atomically. Use bad password indicator (in a distinct TDB) to determine if to open a transaction We open a transaction when we have seen the hint that this user has recorded a bad password. This allows us to avoid always needing one, while not missing a possible lockout. We also go back and get a transation if we did not take out one out but we chose to do a write (eg for lastLogonTimestamp) Based on patches by Gary Lockyer BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Andrew Bartlett Reviewed-by: Joseph Sutton Reviewed-by: Andreas Schneider --- diff --git a/selftest/knownfail.d/auth-sam b/selftest/knownfail.d/auth-sam index e87e43d509b..048459e6555 100644 --- a/selftest/knownfail.d/auth-sam +++ b/selftest/knownfail.d/auth-sam @@ -11,15 +11,3 @@ ^samba.unittests.auth.sam.test_success_accounting_spurious_bad_pwd_indicator.none ^samba.unittests.auth.sam.test_success_accounting_start_txn_failed.none ^samba.unittests.auth.sam.test_success_accounting_update_lastlogon_failed.none -^samba.unittests.auth.sam.test_update_bad_add_control_failed.none -^samba.unittests.auth.sam.test_update_bad_build_mod_request_failed.none -^samba.unittests.auth.sam.test_update_bad_commit_failed.none -^samba.unittests.auth.sam.test_update_bad_get_pso_failed.none -^samba.unittests.auth.sam.test_update_bad_ldb_request_failed.none -^samba.unittests.auth.sam.test_update_bad_ldb_wait_failed.none -^samba.unittests.auth.sam.test_update_bad_no_update_required.none -^samba.unittests.auth.sam.test_update_bad_reread_failed.none -^samba.unittests.auth.sam.test_update_bad_reread_locked_out.none -^samba.unittests.auth.sam.test_update_bad_start_txn_failed.none -^samba.unittests.auth.sam.test_update_bad_txn_cancel_failed.none -^samba.unittests.auth.sam.test_update_bad_update_count_failed.none diff --git a/selftest/knownfail_heimdal_kdc b/selftest/knownfail_heimdal_kdc index 13bdb9691a7..dad59c2684e 100644 --- a/selftest/knownfail_heimdal_kdc +++ b/selftest/knownfail_heimdal_kdc @@ -145,10 +145,5 @@ # # Lockout tests # -^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_bad_pwd_count_transaction_kdc.ad_dc:local -^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_bad_pwd_count_transaction_ntlm.ad_dc:local -^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_bad_pwd_count_transaction_rename_kdc.ad_dc:local -^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_bad_pwd_count_transaction_rename_ntlm.ad_dc:local -^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_bad_pwd_count_transaction_samr.ad_dc:local ^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_lockout_race_kdc.ad_dc:local ^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_lockout_race_ntlm.ad_dc:local diff --git a/selftest/knownfail_mit_kdc b/selftest/knownfail_mit_kdc index a905af24892..38ffac70762 100644 --- a/selftest/knownfail_mit_kdc +++ b/selftest/knownfail_mit_kdc @@ -532,10 +532,7 @@ samba.tests.krb5.as_canonicalization_tests.samba.tests.krb5.as_canonicalization_ # Lockout tests # ^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_bad_pwd_count_transaction_kdc.ad_dc:local -^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_bad_pwd_count_transaction_ntlm.ad_dc:local ^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_bad_pwd_count_transaction_rename_kdc.ad_dc:local -^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_bad_pwd_count_transaction_rename_ntlm.ad_dc:local -^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_bad_pwd_count_transaction_samr.ad_dc:local ^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_lockout_race_kdc.ad_dc:local ^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_lockout_race_ntlm.ad_dc:local ^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_lockout_transaction_bad_pwd_kdc.ad_dc:local diff --git a/source4/auth/sam.c b/source4/auth/sam.c index 92c0480ba6a..19199701465 100644 --- a/source4/auth/sam.c +++ b/source4/auth/sam.c @@ -1073,7 +1073,9 @@ NTSTATUS authsam_update_bad_pwd_count(struct ldb_context *sam_ctx, NTSTATUS status; struct ldb_result *domain_res; struct ldb_message *msg_mod = NULL; + struct ldb_message *current = NULL; struct ldb_message *pso_msg = NULL; + bool txn_active = false; TALLOC_CTX *mem_ctx; mem_ctx = talloc_new(msg); @@ -1098,14 +1100,65 @@ NTSTATUS authsam_update_bad_pwd_count(struct ldb_context *sam_ctx, ret, ldb_dn_get_linearized(msg->dn)); } - status = dsdb_update_bad_pwd_count(mem_ctx, sam_ctx, - msg, domain_res->msgs[0], pso_msg, - &msg_mod); + /* + * To ensure that the bad password count is updated atomically, + * we need to: + * begin a transaction + * re-read the account details, + * using the msgs[0], + pso_msg, + &msg_mod); + if (!NT_STATUS_IS_OK(status)) { + status = NT_STATUS_INTERNAL_ERROR; + goto error; } + /* + * Write the data back to disk if required. + */ if (msg_mod != NULL) { struct ldb_request *req; @@ -1116,7 +1169,9 @@ NTSTATUS authsam_update_bad_pwd_count(struct ldb_context *sam_ctx, ldb_op_default_callback, NULL); if (ret != LDB_SUCCESS) { - goto done; + TALLOC_FREE(msg_mod); + status = NT_STATUS_INTERNAL_ERROR; + goto error; } ret = ldb_request_add_control(req, @@ -1124,31 +1179,72 @@ NTSTATUS authsam_update_bad_pwd_count(struct ldb_context *sam_ctx, false, NULL); if (ret != LDB_SUCCESS) { talloc_free(req); - goto done; + status = NT_STATUS_INTERNAL_ERROR; + goto error; } - ret = dsdb_autotransaction_request(sam_ctx, req); + /* + * As we're in a transaction, make the ldb request directly + * to avoid the nested transaction that would result if we + * called dsdb_autotransaction_request + */ + ret = ldb_request(sam_ctx, req); + if (ret == LDB_SUCCESS) { + ret = ldb_wait(req->handle, LDB_WAIT_ALL); + } talloc_free(req); - + if (ret != LDB_SUCCESS) { + status = NT_STATUS_INTERNAL_ERROR; + goto error; + } status = authsam_set_bad_password_indicator( sam_ctx, mem_ctx, msg); - /* Failure is ignored for now */ + if (!NT_STATUS_IS_OK(status)) { + goto error; + } } - -done: + /* + * Note that we may not have updated the user record, but + * committing the transaction in that case is still the correct + * thing to do. + * If the transaction was cancelled, this would be logged by + * the dsdb audit log as a failure. When in fact it is expected + * behaviour. + */ +exit: + TALLOC_FREE(mem_ctx); + ret = ldb_transaction_commit(sam_ctx); if (ret != LDB_SUCCESS) { - DBG_ERR("Failed to update badPwdCount, badPasswordTime or " - "set lockoutTime on %s: %s\n", - ldb_dn_get_linearized(msg->dn), - ldb_errstring(sam_ctx)); - TALLOC_FREE(mem_ctx); + DBG_ERR("Error (%d) %s, committing transaction," + " while updating bad password count" + " for (%s)\n", + ret, + ldb_errstring(sam_ctx), + ldb_dn_get_linearized(msg->dn)); return NT_STATUS_INTERNAL_ERROR; } + return status; +error: + DBG_ERR("Failed to update badPwdCount, badPasswordTime or " + "set lockoutTime on %s: %s\n", + ldb_dn_get_linearized(msg->dn), + ldb_errstring(sam_ctx) != NULL ? + ldb_errstring(sam_ctx) :nt_errstr(status)); + if (txn_active) { + ret = ldb_transaction_cancel(sam_ctx); + if (ret != LDB_SUCCESS) { + DBG_ERR("Error rolling back transaction," + " while updating bad password count" + " on %s: %s\n", + ldb_dn_get_linearized(msg->dn), + ldb_errstring(sam_ctx)); + } + } TALLOC_FREE(mem_ctx); - return NT_STATUS_OK; -} + return status; +} static NTSTATUS authsam_update_lastlogon_timestamp(struct ldb_context *sam_ctx, struct ldb_message *msg_mod, @@ -1296,6 +1392,7 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx, NTTIME now; NTTIME lastLogonTimestamp; bool am_rodc = false; + bool txn_active = false; bool need_db_reread; mem_ctx = talloc_new(msg); @@ -1303,15 +1400,41 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx, return NT_STATUS_NO_MEMORY; } + /* + * Any update of the last logon data, needs to be done inside a + * transaction. + * And the user data needs to be re-read, and the account re-checked + * for lockout. + * + * Howevver we have long-running transactions like replication + * that could otherwise grind the system to a halt so we first + * determine if *this* account has seen a bad password, + * otherwise we only start a transaction if there was a need + * (because a change was to be made). + */ + status = authsam_check_bad_password_indicator( sam_ctx, mem_ctx, &need_db_reread, msg); if (!NT_STATUS_IS_OK(status)) { return status; } +get_transaction: + if (need_db_reread) { struct ldb_message *current = NULL; + /* + * Start a new transaction + */ + ret = ldb_transaction_start(sam_ctx); + if (ret != LDB_SUCCESS) { + status = NT_STATUS_INTERNAL_ERROR; + goto error; + } + + txn_active = true; + /* * Re-read the account details, using the GUID * embedded in DN so this is safe against a race where @@ -1324,7 +1447,15 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx, * The re-read can return account locked out, as well * as an internal error */ - return status; + if (NT_STATUS_EQUAL(status, NT_STATUS_ACCOUNT_LOCKED_OUT)) { + /* + * For NT_STATUS_ACCOUNT_LOCKED_OUT we want to commit + * the transaction. Again to avoid cluttering the + * audit logs with spurious errors + */ + goto exit; + } + goto error; } msg = current; } @@ -1345,9 +1476,15 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx, msg_mod = ldb_msg_new(mem_ctx); if (msg_mod == NULL) { - TALLOC_FREE(mem_ctx); - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto error; } + + /* + * By using the DN from msg->dn directly, we allow LDB to + * prefer the embedded GUID form, so this is actually quite + * safe even in the case where DN has been changed + */ msg_mod->dn = msg->dn; if (lockoutTime != 0) { @@ -1356,14 +1493,14 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx, */ ret = samdb_msg_add_int(sam_ctx, msg_mod, msg_mod, "lockoutTime", 0); if (ret != LDB_SUCCESS) { - TALLOC_FREE(mem_ctx); - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto error; } } else if (badPwdCount != 0) { ret = samdb_msg_add_int(sam_ctx, msg_mod, msg_mod, "badPwdCount", 0); if (ret != LDB_SUCCESS) { - TALLOC_FREE(mem_ctx); - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto error; } } @@ -1375,8 +1512,8 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx, ret = samdb_msg_add_int64(sam_ctx, msg_mod, msg_mod, "lastLogon", now); if (ret != LDB_SUCCESS) { - TALLOC_FREE(mem_ctx); - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto error; } } @@ -1390,8 +1527,8 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx, ret = samdb_msg_add_int(sam_ctx, msg_mod, msg_mod, "logonCount", logonCount); if (ret != LDB_SUCCESS) { - TALLOC_FREE(mem_ctx); - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto error; } } else { /* Set an unset logonCount to 0 on first successful login */ @@ -1407,16 +1544,16 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx, ret = samdb_rodc(sam_ctx, &am_rodc); if (ret != LDB_SUCCESS) { - TALLOC_FREE(mem_ctx); - return NT_STATUS_INTERNAL_ERROR; + status = NT_STATUS_INTERNAL_ERROR; + goto error; } if (!am_rodc) { status = authsam_update_lastlogon_timestamp(sam_ctx, msg_mod, domain_dn, lastLogonTimestamp, now); if (!NT_STATUS_IS_OK(status)) { - TALLOC_FREE(mem_ctx); - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto error; } } else { /* Perform the (async) SendToSAM calls for MS-SAMS */ @@ -1436,6 +1573,16 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx, unsigned int i; struct ldb_request *req; + /* + * If it turns out we are going to update the DB, go + * back to the start, get a transaction and the + * current DB state and try again + */ + if (txn_active == false) { + need_db_reread = true; + goto get_transaction; + } + /* mark all the message elements as LDB_FLAG_MOD_REPLACE */ for (i=0;inum_elements;i++) { msg_mod->elements[i].flags = LDB_FLAG_MOD_REPLACE; @@ -1448,35 +1595,84 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx, ldb_op_default_callback, NULL); if (ret != LDB_SUCCESS) { - goto done; + status = NT_STATUS_INTERNAL_ERROR; + goto error; } ret = ldb_request_add_control(req, DSDB_CONTROL_FORCE_RODC_LOCAL_CHANGE, false, NULL); if (ret != LDB_SUCCESS) { - talloc_free(req); - goto done; + TALLOC_FREE(req); + status = NT_STATUS_INTERNAL_ERROR; + goto error; + } + /* + * As we're in a transaction, make the ldb request directly + * to avoid the nested transaction that would result if we + * called dsdb_autotransaction_request + */ + ret = ldb_request(sam_ctx, req); + if (ret == LDB_SUCCESS) { + ret = ldb_wait(req->handle, LDB_WAIT_ALL); + } + TALLOC_FREE(req); + if (ret != LDB_SUCCESS) { + status = NT_STATUS_INTERNAL_ERROR; + goto error; } - - ret = dsdb_autotransaction_request(sam_ctx, req); - talloc_free(req); } - status = authsam_clear_bad_password_indicator(sam_ctx, mem_ctx, msg); - /* Failure is ignored for now */ + if (!NT_STATUS_IS_OK(status)) { + goto error; + } -done: + /* + * Note that we may not have updated the user record, but + * committing the transaction in that case is still the correct + * thing to do. + * If the transaction was cancelled, this would be logged by + * the dsdb audit log as a failure. When in fact it is expected + * behaviour. + * + * Thankfully both TDB and LMDB seem to optimise for the empty + * transaction case + */ +exit: + TALLOC_FREE(mem_ctx); + + if (txn_active == false) { + return status; + } + + ret = ldb_transaction_commit(sam_ctx); if (ret != LDB_SUCCESS) { - DEBUG(0, ("Failed to set badPwdCount and lockoutTime " - "to 0 and/or lastlogon to now (%lld) " - "%s: %s\n", (long long int)now, - ldb_dn_get_linearized(msg_mod->dn), - ldb_errstring(sam_ctx))); - TALLOC_FREE(mem_ctx); + DBG_ERR("Error (%d) %s, committing transaction," + " while updating successful logon accounting" + " for (%s)\n", + ret, + ldb_errstring(sam_ctx), + ldb_dn_get_linearized(msg->dn)); return NT_STATUS_INTERNAL_ERROR; } + return status; +error: + DBG_ERR("Failed to update badPwdCount, badPasswordTime or " + "set lockoutTime on %s: %s\n", + ldb_dn_get_linearized(msg->dn), + ldb_errstring(sam_ctx) != NULL ? + ldb_errstring(sam_ctx) :nt_errstr(status)); + if (txn_active) { + ret = ldb_transaction_cancel(sam_ctx); + if (ret != LDB_SUCCESS) { + DBG_ERR("Error rolling back transaction," + " while updating bad password count" + " on %s: %s\n", + ldb_dn_get_linearized(msg->dn), + ldb_errstring(sam_ctx)); + } + } TALLOC_FREE(mem_ctx); - return NT_STATUS_OK; + return status; }