Attachment 16411 Details for Bug 14611 – Proposed patch for bad password path

[patch] Proposed patch for bad password path

bug-14611-bad-password-proposed-fix.patch (text/plain), 4.53 KB, created by Gary Lockyer on 2021-01-28 02:30:32 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Gary Lockyer

Created: 2021-01-28 02:30:32 UTC

Size: 4.53 KB

patch

obsolete

>From bd847e40448a0046c731dcdc02596df269ba2c87 Mon Sep 17 00:00:00 2001
>From: Gary Lockyer <gary@catalyst.net.nz>
>Date: Wed, 27 Jan 2021 14:24:58 +1300
>Subject: [PATCH] s4 auth: make bad password count increment atomic
>
>Ensure that the bad password count is incremented atomically.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611
>
>Signed-off-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> source4/auth/sam.c | 121 +++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 101 insertions(+), 20 deletions(-)
>
>diff --git a/source4/auth/sam.c b/source4/auth/sam.c
>index 39e48c26b52..75e03aa3e08 100644
>--- a/source4/auth/sam.c
>+++ b/source4/auth/sam.c
>@@ -823,10 +823,12 @@ NTSTATUS authsam_update_bad_pwd_count(struct ldb_context *sam_ctx,
> 				"pwdProperties",
> 				NULL };
> 	int ret;
>-	NTSTATUS status;
>-	struct ldb_result *domain_res;
>+	NTSTATUS status = NT_STATUS_OK;
>+	struct ldb_result *domain_res = NULL;
> 	struct ldb_message *msg_mod = NULL;
> 	struct ldb_message *pso_msg = NULL;
>+	struct ldb_result *res = NULL;
>+	uint16_t acct_flags = 0;
> 	TALLOC_CTX *mem_ctx;
> 
> 	mem_ctx = talloc_new(msg);
>@@ -851,14 +853,68 @@ 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
>+	 *       update the bad password count
>+	 *    commit the transaction.
>+	 */
>+
>+	/*
>+	 * Start a new transaction
>+	 */
>+	ret = ldb_transaction_start(sam_ctx);
>+	if (ret != LDB_SUCCESS) {
>+		goto error;
>+	}
>+
>+	/*
>+	 * Re-read the account details
>+	 */
>+	ret = dsdb_search_dn(
>+		sam_ctx,
>+		mem_ctx,
>+		&res,
>+		msg->dn,
>+		user_attrs,
>+		0);
>+	if (ret != LDB_SUCCESS) {
>+		goto error;
>+	}
>+
>+	/*
>+	 * Ensure the account has not been locked out by another request
>+	 */
>+	acct_flags = samdb_result_acct_flags(
>+		res->msgs[0],
>+		"msDS-User-Account-Control-Computed");
>+	if (acct_flags & ACB_AUTOLOCK) {
>+		DBG_WARNING(
>+			"Account for user %s was locked out.\n",
>+			ldb_dn_get_linearized(msg->dn));
>+		status = NT_STATUS_ACCOUNT_LOCKED_OUT;
>+		goto cleanup;
>+	}
>+
>+	/*
>+	 * Update the bad password count and if required lock the account
>+	 */
>+	status = dsdb_update_bad_pwd_count(
>+		mem_ctx,
>+		sam_ctx,
>+		res->msgs[0],
>+		domain_res->msgs[0],
>+		pso_msg,
>+		&msg_mod);
> 	if (!NT_STATUS_IS_OK(status)) {
>-		TALLOC_FREE(mem_ctx);
>-		return status;
>+		goto error;
> 	}
> 
>+	/*
>+	 * Write the data back to disk if required.
>+	 */
> 	if (msg_mod != NULL) {
> 		struct ldb_request *req;
> 
>@@ -869,35 +925,60 @@ NTSTATUS authsam_update_bad_pwd_count(struct ldb_context *sam_ctx,
> 					ldb_op_default_callback,
> 					NULL);
> 		if (ret != LDB_SUCCESS) {
>-			goto done;
>+			goto error;
> 		}
> 
>-		ret = ldb_request_add_control(req,
>-					      DSDB_CONTROL_FORCE_RODC_LOCAL_CHANGE,
>-					      false, NULL);
>+		ret = ldb_request_add_control(
>+			req,
>+			DSDB_CONTROL_FORCE_RODC_LOCAL_CHANGE,
>+			false,
>+			NULL);
> 		if (ret != LDB_SUCCESS) {
> 			talloc_free(req);
>-			goto done;
>+			goto error;
> 		}
> 
> 		ret = dsdb_autotransaction_request(sam_ctx, req);
> 		talloc_free(req);
>+		if (ret != LDB_SUCCESS) {
>+			goto error;
>+		}
> 	}
> 
>-done:
>+	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 NT_STATUS_OK;
> 
>+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));
>+cleanup:
>+	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;
>-}
>+	if (NT_STATUS_EQUAL(NT_STATUS_ACCOUNT_LOCKED_OUT, status)) {
>+		return NT_STATUS_ACCOUNT_LOCKED_OUT;
>+	}
>+	return NT_STATUS_INTERNAL_ERROR;
> 
>+}
> 
> static NTSTATUS authsam_update_lastlogon_timestamp(struct ldb_context *sam_ctx,
> 					    struct ldb_message *msg_mod,
>-- 
>2.25.1
>

Flags:

gary: ci-passed+

Actions: View

Attachments on bug 14611: 16386 | 16390 | 16391 | 16392 | 16393 | 16411 | 16474 | 16483 | 16484 | 16485 | 16486 | 16487 | 16573 | 17410 | 17411 | 17413 | 17436 | 17452 | 17453 | 17456 | 17513 | 17521 | 17733 | 17734