CVE-2021-20251 auth4: Avoid reading the database twice by precaculati…

…ng some variables These variables are not important to protect against a race with and a double-read can easily be avoided by moving them up the file a little. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Andrew Bartlett <[email protected]> Reviewed-by: Joseph Sutton <[email protected]> Reviewed-by: Andreas Schneider <[email protected]>
CynCYX · Sep 12, 2022 · b5f78b7 · b5f78b7
1 parent 7121810
commit b5f78b7
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 31 deletions.
diff --git a/selftest/knownfail.d/auth-sam b/selftest/knownfail.d/auth-sam
diff --git a/source4/auth/sam.c b/source4/auth/sam.c
@@ -1408,6 +1408,8 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx,
 	struct timeval tv_now;
 	NTTIME now;
 	NTTIME lastLogonTimestamp;
+	int64_t lockOutObservationWindow;
+	NTTIME sync_interval_nt = 0;
 	bool am_rodc = false;
 	bool txn_active = false;
 	bool need_db_reread;
@@ -1436,6 +1438,36 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx,
 		return status;
 	}
 
+	if (interactive_or_kerberos == false) {
+		/*
+		 * Avoid calculating this twice, it reads the PSO.  A
+		 * race on this is unimportant.
+		 */
+		lockOutObservationWindow
+			= samdb_result_msds_LockoutObservationWindow(
+				sam_ctx, mem_ctx, domain_dn, msg);
+	}
+
+	ret = samdb_rodc(sam_ctx, &am_rodc);
+	if (ret != LDB_SUCCESS) {
+		status = NT_STATUS_INTERNAL_ERROR;
+		goto error;
+	}
+
+	if (!am_rodc) {
+		/*
+		 * Avoid reading the main domain DN twice.  A race on
+		 * this is unimportant.
+		 */
+		status = authsam_calculate_lastlogon_sync_interval(
+			sam_ctx, mem_ctx, domain_dn, &sync_interval_nt);
+
+		if (!NT_STATUS_IS_OK(status)) {
+			status = NT_STATUS_INTERNAL_ERROR;
+			goto error;
+		}
+	}
+
 get_transaction:
 
 	if (need_db_reread) {
@@ -1485,9 +1517,10 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx,
 	if (interactive_or_kerberos) {
 		badPwdCount = dbBadPwdCount;
 	} else {
-		int64_t lockOutObservationWindow =
-			samdb_result_msds_LockoutObservationWindow(
-				sam_ctx, mem_ctx, domain_dn, msg);
+		/*
+		 * We get lockOutObservationWindow above, before the
+		 * transaction
+		 */
 		badPwdCount = dsdb_effective_badPwdCount(
 			msg, lockOutObservationWindow, now);
 	}
@@ -1562,23 +1595,7 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx,
 		}
 	}
 
-	ret = samdb_rodc(sam_ctx, &am_rodc);
-	if (ret != LDB_SUCCESS) {
-		status = NT_STATUS_INTERNAL_ERROR;
-		goto error;
-	}
-
 	if (!am_rodc) {
-		NTTIME sync_interval_nt;
-
-		status = authsam_calculate_lastlogon_sync_interval(
-			sam_ctx, mem_ctx, domain_dn, &sync_interval_nt);
-
-		if (!NT_STATUS_IS_OK(status)) {
-			status = NT_STATUS_INTERNAL_ERROR;
-			goto error;
-		}
-
 		status = authsam_update_lastlogon_timestamp(
 			sam_ctx,
 			msg_mod,