CVE-2021-20251 auth4: Avoid reading the database twice by precaculating 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 <abartlet@samba.org>
Reviewed-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>

diff --git a/selftest/knownfail.d/auth-sam b/selftest/knownfail.d/auth-sam
deleted file mode 100644 (file)
index 438cea4..0000000
--- a/selftest/knownfail.d/auth-sam
+++ /dev/null
@@ -1,12 +0,0 @@
-^samba.unittests.auth.sam.test_success_accounting_add_control_failed.none
-^samba.unittests.auth.sam.test_success_accounting_build_mod_req_failed.none
-^samba.unittests.auth.sam.test_success_accounting_commit_failed.none
-^samba.unittests.auth.sam.test_success_accounting_ldb_msg_new_failed.none
-^samba.unittests.auth.sam.test_success_accounting_ldb_request_failed.none
-^samba.unittests.auth.sam.test_success_accounting_ldb_wait_failed.none
-^samba.unittests.auth.sam.test_success_accounting_reread_failed.none
-^samba.unittests.auth.sam.test_success_accounting_rollback_failed.none
-^samba.unittests.auth.sam.test_success_accounting_samdb_rodc_failed.none
-^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

diff --git a/source4/auth/sam.c b/source4/auth/sam.c
index 698324e5cc0d184f81077adb7c71dca123428385..219ee10d5bd3f3262701e5d3acb22274e82cc2ee 100644 (file)
--- 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 @@ get_transaction:
        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 @@ get_transaction:
                }
        }
 
-       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,