From c5faaa68be81382b44a1e9222d7230fe5ea7e692 Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Thu, 21 Jul 2022 14:55:31 +0200 Subject: [PATCH 01/37] third_party: Update socket_wrapper to version 1.3.4 Signed-off-by: Andreas Schneider Reviewed-by: Jeremy Allison (cherry picked from commit 5dcb49bbd80abf6f3f082ef9c1d5452991c74c87) --- buildtools/wafsamba/samba_third_party.py | 2 +- third_party/socket_wrapper/socket_wrapper.c | 18 ++++++++++-------- third_party/socket_wrapper/wscript | 2 +- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/buildtools/wafsamba/samba_third_party.py b/buildtools/wafsamba/samba_third_party.py index f046ebc96da..10635a3d46b 100644 --- a/buildtools/wafsamba/samba_third_party.py +++ b/buildtools/wafsamba/samba_third_party.py @@ -24,7 +24,7 @@ Build.BuildContext.CHECK_CMOCKA = CHECK_CMOCKA @conf def CHECK_SOCKET_WRAPPER(conf): - return conf.CHECK_BUNDLED_SYSTEM_PKG('socket_wrapper', minversion='1.3.3') + return conf.CHECK_BUNDLED_SYSTEM_PKG('socket_wrapper', minversion='1.3.4') Build.BuildContext.CHECK_SOCKET_WRAPPER = CHECK_SOCKET_WRAPPER @conf diff --git a/third_party/socket_wrapper/socket_wrapper.c b/third_party/socket_wrapper/socket_wrapper.c index 44cfad8c6cf..5804e936ff7 100644 --- a/third_party/socket_wrapper/socket_wrapper.c +++ b/third_party/socket_wrapper/socket_wrapper.c @@ -3815,7 +3815,6 @@ static int swrap_auto_bind(int fd, struct socket_info *si, int family) char type; int ret; int port; - struct stat st; char *swrap_dir = NULL; swrap_mutex_lock(&autobind_start_mutex); @@ -3916,10 +3915,12 @@ static int swrap_auto_bind(int fd, struct socket_info *si, int family) type, socket_wrapper_default_iface(), port); - if (stat(un_addr.sa.un.sun_path, &st) == 0) continue; ret = libc_bind(fd, &un_addr.sa.s, un_addr.sa_socklen); if (ret == -1) { + if (errno == EALREADY || errno == EADDRINUSE) { + continue; + } goto done; } @@ -6285,9 +6286,11 @@ static void swrap_sendmsg_after(int fd, for (i = 0; i < (size_t)msg->msg_iovlen; i++) { size_t this_time = MIN(remain, (size_t)msg->msg_iov[i].iov_len); - memcpy(buf + ofs, - msg->msg_iov[i].iov_base, - this_time); + if (this_time > 0) { + memcpy(buf + ofs, + msg->msg_iov[i].iov_base, + this_time); + } ofs += this_time; remain -= this_time; } @@ -7849,8 +7852,8 @@ void swrap_destructor(void) * related syscalls also with the '_' prefix. * * This is tested in Samba's 'make test', - * there we noticed that providing '_read' - * and '_open' would cause errors, which + * there we noticed that providing '_read', + * '_open' and '_close' would cause errors, which * means we skip '_read', '_write' and * all non socket related calls without * further analyzing the problem. @@ -7863,7 +7866,6 @@ SWRAP_SYMBOL_ALIAS(accept4, _accept4); #endif SWRAP_SYMBOL_ALIAS(accept, _accept); SWRAP_SYMBOL_ALIAS(bind, _bind); -SWRAP_SYMBOL_ALIAS(close, _close); SWRAP_SYMBOL_ALIAS(connect, _connect); SWRAP_SYMBOL_ALIAS(dup, _dup); SWRAP_SYMBOL_ALIAS(dup2, _dup2); diff --git a/third_party/socket_wrapper/wscript b/third_party/socket_wrapper/wscript index 15ceefd168d..b87072038f8 100644 --- a/third_party/socket_wrapper/wscript +++ b/third_party/socket_wrapper/wscript @@ -2,7 +2,7 @@ import os -VERSION="1.3.3" +VERSION="1.3.4" def configure(conf): if conf.CHECK_SOCKET_WRAPPER(): -- 2.35.0 From e208147de96d8b71a62aa7e9f12dedaeb3e5c94a Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Thu, 28 Jul 2022 10:13:45 +1200 Subject: [PATCH 02/37] CVE-2021-20251 tests/krb5: Add PasswordKey_from_creds() This is needed for generating a key when we don't have ETYPE-INFO2. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton --- python/samba/tests/krb5/raw_testcase.py | 26 +++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/python/samba/tests/krb5/raw_testcase.py b/python/samba/tests/krb5/raw_testcase.py index 00c590a34c0..d95361c7f35 100644 --- a/python/samba/tests/krb5/raw_testcase.py +++ b/python/samba/tests/krb5/raw_testcase.py @@ -1247,18 +1247,28 @@ class RawKerberosTest(TestCaseInTempDir): def PasswordKey_from_etype_info2(self, creds, etype_info2, kvno=None): e = etype_info2['etype'] - salt = etype_info2.get('salt') - - if e == kcrypto.Enctype.RC4: - nthash = creds.get_nt_hash() - return self.SessionKey_create(etype=e, contents=nthash, kvno=kvno) - params = etype_info2.get('s2kparams') + return self.PasswordKey_from_etype(creds, e, + kvno=kvno, + salt=salt, + params=params) - password = creds.get_password() + def PasswordKey_from_creds(self, creds, etype): + kvno = creds.get_kvno() + salt = creds.get_salt() + return self.PasswordKey_from_etype(creds, etype, + kvno=kvno, + salt=salt) + + def PasswordKey_from_etype(self, creds, etype, kvno=None, salt=None, params=None): + if etype == kcrypto.Enctype.RC4: + nthash = creds.get_nt_hash() + return self.SessionKey_create(etype=etype, contents=nthash, kvno=kvno) + + password = creds.get_password().encode('utf-8') return self.PasswordKey_create( - etype=e, pwd=password, salt=salt, kvno=kvno, params=params) + etype=etype, pwd=password, salt=salt, kvno=kvno) def TicketDecryptionKey_from_creds(self, creds, etype=None): -- 2.35.0 From 628f97b24bd228aca409f5e142d8ab34e6421c7b Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Wed, 6 Jul 2022 15:36:26 +1200 Subject: [PATCH 03/37] CVE-2021-20251 lib:crypto: Add des_crypt_blob_16() for encrypting data with DES This lets us access single-DES from Python. This function is used in a following commit for encrypting an NT hash to obtain the verifier for a SAMR password change. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit b27a67af0216811d330d8a4c52390cf4fc04b5fd) [jsutton@samba.org Fixed wscript conflict introduced by commit 61aeb7740764b202db0ddba559e83c3b2953ae36] --- lib/crypto/py_crypto.c | 65 ++++++++++++++++++++++++++++++++++++++++ lib/crypto/wscript_build | 2 +- 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/lib/crypto/py_crypto.c b/lib/crypto/py_crypto.c index ad18d3ada0f..6753d3d8e9c 100644 --- a/lib/crypto/py_crypto.c +++ b/lib/crypto/py_crypto.c @@ -25,6 +25,7 @@ #include #include #include "lib/crypto/gnutls_helpers.h" +#include "libcli/auth/libcli_auth.h" static PyObject *py_crypto_arcfour_crypt_blob(PyObject *module, PyObject *args) { @@ -100,13 +101,77 @@ static PyObject *py_crypto_set_strict_mode(PyObject *module) Py_RETURN_NONE; } +static PyObject *py_crypto_des_crypt_blob_16(PyObject *self, PyObject *args) +{ + PyObject *py_data = NULL; + uint8_t *data = NULL; + Py_ssize_t data_size; + + PyObject *py_key = NULL; + uint8_t *key = NULL; + Py_ssize_t key_size; + + uint8_t result[16]; + + bool ok; + int ret; + + ok = PyArg_ParseTuple(args, "SS", + &py_data, &py_key); + if (!ok) { + return NULL; + } + + ret = PyBytes_AsStringAndSize(py_data, + (char **)&data, + &data_size); + if (ret != 0) { + return NULL; + } + + ret = PyBytes_AsStringAndSize(py_key, + (char **)&key, + &key_size); + if (ret != 0) { + return NULL; + } + + if (data_size != 16) { + return PyErr_Format(PyExc_ValueError, + "Expected data size of 16 bytes; got %zd", + data_size); + } + + if (key_size != 14) { + return PyErr_Format(PyExc_ValueError, + "Expected key size of 14 bytes; got %zd", + key_size); + } + + ret = des_crypt112_16(result, data, key, + SAMBA_GNUTLS_ENCRYPT); + if (ret != 0) { + return PyErr_Format(PyExc_RuntimeError, + "des_crypt112_16() failed: %d", + ret); + } + + return PyBytes_FromStringAndSize((const char *)result, + sizeof(result)); +} + static const char py_crypto_arcfour_crypt_blob_doc[] = "arcfour_crypt_blob(data, key)\n" "Encrypt the data with RC4 algorithm using the key"; +static const char py_crypto_des_crypt_blob_16_doc[] = "des_crypt_blob_16(data, key) -> bytes\n" + "Encrypt the 16-byte data with DES using " + "the 14-byte key"; + static PyMethodDef py_crypto_methods[] = { { "arcfour_crypt_blob", (PyCFunction)py_crypto_arcfour_crypt_blob, METH_VARARGS, py_crypto_arcfour_crypt_blob_doc }, { "set_relax_mode", (PyCFunction)py_crypto_set_relax_mode, METH_NOARGS, "Set fips to relax mode" }, { "set_strict_mode", (PyCFunction)py_crypto_set_strict_mode, METH_NOARGS, "Set fips to strict mode" }, + { "des_crypt_blob_16", (PyCFunction)py_crypto_des_crypt_blob_16, METH_VARARGS, py_crypto_des_crypt_blob_16_doc }, {0}, }; diff --git a/lib/crypto/wscript_build b/lib/crypto/wscript_build index e5766042541..0d5b548e7f8 100644 --- a/lib/crypto/wscript_build +++ b/lib/crypto/wscript_build @@ -50,5 +50,5 @@ bld.SAMBA_SUBSYSTEM('TORTURE_LIBCRYPTO', bld.SAMBA_PYTHON('python_crypto', source='py_crypto.c', - deps='gnutls talloc', + deps='gnutls talloc LIBCLI_AUTH', realname='samba/crypto.so') -- 2.35.0 From 09c1945896df667788ac42a426484a18e4077dd6 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Wed, 13 Jul 2022 14:20:59 +1200 Subject: [PATCH 04/37] CVE-2021-20251 lib:crypto: Add md4_hash_blob() for hashing data with MD4 This lets us access MD4, which might not be available in hashlib, from Python. This function is used in a following commit for hashing a password to obtain the verifier for a SAMR password change. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit 17b8d164f69a5ed79d9b7b7fc2f3f84f8ea534c8) --- lib/crypto/py_crypto.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/lib/crypto/py_crypto.c b/lib/crypto/py_crypto.c index 6753d3d8e9c..40b0cb9e9c0 100644 --- a/lib/crypto/py_crypto.c +++ b/lib/crypto/py_crypto.c @@ -25,6 +25,7 @@ #include #include #include "lib/crypto/gnutls_helpers.h" +#include "lib/crypto/md4.h" #include "libcli/auth/libcli_auth.h" static PyObject *py_crypto_arcfour_crypt_blob(PyObject *module, PyObject *args) @@ -160,6 +161,36 @@ static PyObject *py_crypto_des_crypt_blob_16(PyObject *self, PyObject *args) sizeof(result)); } +static PyObject *py_crypto_md4_hash_blob(PyObject *self, PyObject *args) +{ + PyObject *py_data = NULL; + uint8_t *data = NULL; + Py_ssize_t data_size; + + uint8_t result[16]; + + bool ok; + int ret; + + ok = PyArg_ParseTuple(args, "S", + &py_data); + if (!ok) { + return NULL; + } + + ret = PyBytes_AsStringAndSize(py_data, + (char **)&data, + &data_size); + if (ret != 0) { + return NULL; + } + + mdfour(result, data, data_size); + + return PyBytes_FromStringAndSize((const char *)result, + sizeof(result)); +} + static const char py_crypto_arcfour_crypt_blob_doc[] = "arcfour_crypt_blob(data, key)\n" "Encrypt the data with RC4 algorithm using the key"; @@ -167,11 +198,15 @@ static const char py_crypto_des_crypt_blob_16_doc[] = "des_crypt_blob_16(data, k "Encrypt the 16-byte data with DES using " "the 14-byte key"; +static const char py_crypto_md4_hash_blob_doc[] = "md4_hash_blob(data) -> bytes\n" + "Hash the data with MD4 algorithm"; + static PyMethodDef py_crypto_methods[] = { { "arcfour_crypt_blob", (PyCFunction)py_crypto_arcfour_crypt_blob, METH_VARARGS, py_crypto_arcfour_crypt_blob_doc }, { "set_relax_mode", (PyCFunction)py_crypto_set_relax_mode, METH_NOARGS, "Set fips to relax mode" }, { "set_strict_mode", (PyCFunction)py_crypto_set_strict_mode, METH_NOARGS, "Set fips to strict mode" }, { "des_crypt_blob_16", (PyCFunction)py_crypto_des_crypt_blob_16, METH_VARARGS, py_crypto_des_crypt_blob_16_doc }, + { "md4_hash_blob", (PyCFunction)py_crypto_md4_hash_blob, METH_VARARGS, py_crypto_md4_hash_blob_doc }, {0}, }; -- 2.35.0 From aec3fcc2adcc3aa97ff008e494e23b3d438dce0d Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Mon, 4 Jul 2022 20:48:48 +1200 Subject: [PATCH 05/37] CVE-2021-20251 tests/krb5: Add tests for password lockout race BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit 91e2e5616ccd507fcaf097533c5fc25974119c1e) [jsutton@samba.org Fixed conflicts in usage.py, knownfails, and tests.py due to not having claims tests] [jsutton@samba.org Removed tests for unsupported SAMR AES password change, removed related GNUTLS_PBKDF2_SUPPORT environment variable, and fixed knownfail conflicts; marked all password lockout tests as flapping due to sporadic failures seen with Fedora 35] --- python/samba/tests/krb5/lockout_tests.py | 975 +++++++++++++++++++ python/samba/tests/krb5/raw_testcase.py | 3 +- python/samba/tests/krb5/rfc4120_constants.py | 1 + python/samba/tests/usage.py | 1 + selftest/flapping.d/ldap-pwd-change-race | 6 + selftest/knownfail_heimdal_kdc | 10 + selftest/knownfail_mit_kdc | 14 + source4/selftest/tests.py | 4 + 8 files changed, 1013 insertions(+), 1 deletion(-) create mode 100755 python/samba/tests/krb5/lockout_tests.py create mode 100644 selftest/flapping.d/ldap-pwd-change-race diff --git a/python/samba/tests/krb5/lockout_tests.py b/python/samba/tests/krb5/lockout_tests.py new file mode 100755 index 00000000000..dd0ca9db378 --- /dev/null +++ b/python/samba/tests/krb5/lockout_tests.py @@ -0,0 +1,975 @@ +#!/usr/bin/env python3 +# Unix SMB/CIFS implementation. +# Copyright (C) Stefan Metzmacher 2020 +# Copyright (C) Catalyst.Net Ltd +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# + +from concurrent import futures +from enum import Enum +from functools import partial +from multiprocessing import Pipe +import os +import sys +import time + +from cryptography.hazmat.backends import default_backend +from cryptography.hazmat.primitives.ciphers.base import Cipher +from cryptography.hazmat.primitives.ciphers import algorithms + +import ldb + +from samba import ( + NTSTATUSError, + dsdb, + generate_random_password, + ntstatus, + unix2nttime, + werror, +) +from samba.credentials import DONT_USE_KERBEROS, MUST_USE_KERBEROS +from samba.crypto import ( + des_crypt_blob_16, + md4_hash_blob, +) +from samba.dcerpc import lsa, samr +from samba.samdb import SamDB + +from samba.tests import connect_samdb, env_get_var_value, env_loadparm + +from samba.tests.krb5.as_req_tests import AsReqBaseTest +from samba.tests.krb5 import kcrypto +from samba.tests.krb5.kdc_base_test import KDCBaseTest +from samba.tests.krb5.raw_testcase import KerberosCredentials +import samba.tests.krb5.rfc4120_pyasn1 as krb5_asn1 +from samba.tests.krb5.rfc4120_constants import ( + KDC_ERR_CLIENT_REVOKED, + KDC_ERR_PREAUTH_FAILED, + KRB_AS_REP, + KRB_ERROR, + NT_PRINCIPAL, + NT_SRV_INST, +) + +sys.path.insert(0, 'bin/python') +os.environ['PYTHONUNBUFFERED'] = '1' + +global_asn1_print = False +global_hexdump = False + + +class ConnectionResult(Enum): + LOCKED_OUT = 1 + WRONG_PASSWORD = 2 + SUCCESS = 3 + + +def connect_kdc(pipe, + url, + hostname, + username, + password, + domain, + realm, + workstation, + dn, + expect_error=True): + AsReqBaseTest.setUpClass() + as_req_base = AsReqBaseTest() + as_req_base.setUp() + + user_creds = KerberosCredentials() + user_creds.set_username(username) + user_creds.set_password(password) + user_creds.set_domain(domain) + user_creds.set_realm(realm) + user_creds.set_workstation(workstation) + user_creds.set_kerberos_state(DONT_USE_KERBEROS) + + user_name = user_creds.get_username() + cname = as_req_base.PrincipalName_create(name_type=NT_PRINCIPAL, + names=user_name.split('/')) + + krbtgt_creds = as_req_base.get_krbtgt_creds() + krbtgt_supported_etypes = krbtgt_creds.tgs_supported_enctypes + realm = krbtgt_creds.get_realm() + + krbtgt_account = krbtgt_creds.get_username() + sname = as_req_base.PrincipalName_create(name_type=NT_SRV_INST, + names=[krbtgt_account, realm]) + + expected_salt = user_creds.get_salt() + + till = as_req_base.get_KerberosTime(offset=36000) + + kdc_options = krb5_asn1.KDCOptions('postdated') + + preauth_key = as_req_base.PasswordKey_from_creds(user_creds, + kcrypto.Enctype.AES256) + + ts_enc_padata = as_req_base.get_enc_timestamp_pa_data_from_key(preauth_key) + padata = [ts_enc_padata] + + krbtgt_decryption_key = ( + as_req_base.TicketDecryptionKey_from_creds(krbtgt_creds)) + + etypes = as_req_base.get_default_enctypes() + + if expect_error: + expected_error_modes = (KDC_ERR_CLIENT_REVOKED, + KDC_ERR_PREAUTH_FAILED) + else: + expected_error_modes = 0 + + # Remove the LDAP connection. + del type(as_req_base)._ldb + + # Indicate that we're ready. This ensures we hit the right transaction + # lock. + pipe.send_bytes(b'0') + + # Wait for the main process to take out a transaction lock. + if not pipe.poll(timeout=5): + raise AssertionError('main process failed to indicate readiness') + + # Try making a Kerberos AS-REQ to the KDC. This should fail, either due to + # the user's account being locked out or due to using the wrong password. + as_rep, kdc_exchange_dict = as_req_base._test_as_exchange( + cname=cname, + realm=realm, + sname=sname, + till=till, + client_as_etypes=etypes, + expected_error_mode=expected_error_modes, + expected_crealm=realm, + expected_cname=cname, + expected_srealm=realm, + expected_sname=sname, + expected_salt=expected_salt, + etypes=etypes, + padata=padata, + kdc_options=kdc_options, + expected_supported_etypes=krbtgt_supported_etypes, + expected_account_name=user_name, + preauth_key=preauth_key, + ticket_decryption_key=krbtgt_decryption_key, + pac_request=True) + as_req_base.assertIsNotNone(as_rep) + + msg_type = as_rep['msg-type'] + if expect_error and msg_type != KRB_ERROR or ( + not expect_error and msg_type != KRB_AS_REP): + raise AssertionError(f'wrong message type {msg_type}') + + if not expect_error: + return ConnectionResult.SUCCESS + + error_code = as_rep['error-code'] + if error_code == KDC_ERR_CLIENT_REVOKED: + return ConnectionResult.LOCKED_OUT + elif error_code == KDC_ERR_PREAUTH_FAILED: + return ConnectionResult.WRONG_PASSWORD + else: + raise AssertionError(f'wrong error code {error_code}') + + +def connect_ntlm(pipe, + url, + hostname, + username, + password, + domain, + realm, + workstation, + dn): + user_creds = KerberosCredentials() + user_creds.set_username(username) + user_creds.set_password(password) + user_creds.set_domain(domain) + user_creds.set_workstation(workstation) + user_creds.set_kerberos_state(DONT_USE_KERBEROS) + + # Indicate that we're ready. This ensures we hit the right transaction + # lock. + pipe.send_bytes(b'0') + + # Wait for the main process to take out a transaction lock. + if not pipe.poll(timeout=5): + raise AssertionError('main process failed to indicate readiness') + + try: + # Try connecting to SamDB. This should fail, either due to our + # account being locked out or due to using the wrong password. + SamDB(url=url, + credentials=user_creds, + lp=env_loadparm()) + except ldb.LdbError as err: + num, estr = err.args + + if num != ldb.ERR_INVALID_CREDENTIALS: + raise AssertionError(f'connection raised wrong error code ' + f'({err})') + + if f'data {werror.WERR_ACCOUNT_LOCKED_OUT:x},' in estr: + return ConnectionResult.LOCKED_OUT + elif f'data {werror.WERR_LOGON_FAILURE:x},' in estr: + return ConnectionResult.WRONG_PASSWORD + else: + raise AssertionError(f'connection raised wrong error code ' + f'({estr})') + else: + return ConnectionResult.SUCCESS + + +def connect_samr(pipe, + url, + hostname, + username, + password, + domain, + realm, + workstation, + dn): + # Get the user's NT hash. + user_creds = KerberosCredentials() + user_creds.set_password(password) + nt_hash = user_creds.get_nt_hash() + + # Generate a new UTF-16 password. + new_password = generate_random_password(32, 32) + new_password = new_password.encode('utf-16le') + + # Generate the MD4 hash of the password. + new_password_md4 = md4_hash_blob(new_password) + + # Prefix the password with padding so it is 512 bytes long. + new_password_len = len(new_password) + remaining_len = 512 - new_password_len + new_password = bytes(remaining_len) + new_password + + # Append the 32-bit length of the password.. + new_password += int.to_bytes(new_password_len, + length=4, + byteorder='little') + + # Encrypt the password with RC4 and the existing NT hash. + encryptor = Cipher(algorithms.ARC4(nt_hash), + None, + default_backend()).encryptor() + new_password = encryptor.update(new_password) + + # Create a key from the MD4 hash of the new password. + key = new_password_md4[:14] + + # Encrypt the old NT hash with DES to obtain the verifier. + verifier = des_crypt_blob_16(nt_hash, key) + + server = lsa.String() + server.string = hostname + + account = lsa.String() + account.string = username + + nt_password = samr.CryptPassword() + nt_password.data = list(new_password) + + nt_verifier = samr.Password() + nt_verifier.hash = list(verifier) + + conn = samr.samr(f'ncacn_np:{hostname}[krb5,seal,smb2]') + + # Indicate that we're ready. This ensures we hit the right transaction + # lock. + pipe.send_bytes(b'0') + + # Wait for the main process to take out a transaction lock. + if not pipe.poll(timeout=5): + raise AssertionError('main process failed to indicate readiness') + + try: + # Try changing the password. This should fail, either due to our + # account being locked out or due to using the wrong password. + conn.ChangePasswordUser3(server=server, + account=account, + nt_password=nt_password, + nt_verifier=nt_verifier, + lm_change=True, + lm_password=None, + lm_verifier=None, + password3=None) + except NTSTATUSError as err: + num, estr = err.args + + if num == ntstatus.NT_STATUS_ACCOUNT_LOCKED_OUT: + return ConnectionResult.LOCKED_OUT + elif num == ntstatus.NT_STATUS_WRONG_PASSWORD: + return ConnectionResult.WRONG_PASSWORD + else: + raise AssertionError(f'pwd change raised wrong error code ' + f'({num:08X})') + else: + return ConnectionResult.SUCCESS + + +def ldap_pwd_change(pipe, + url, + hostname, + username, + password, + domain, + realm, + workstation, + dn): + lp = env_loadparm() + + admin_creds = KerberosCredentials() + admin_creds.guess(lp) + admin_creds.set_username(env_get_var_value('ADMIN_USERNAME')) + admin_creds.set_password(env_get_var_value('ADMIN_PASSWORD')) + admin_creds.set_kerberos_state(MUST_USE_KERBEROS) + + samdb = SamDB(url=url, + credentials=admin_creds, + lp=lp) + + old_utf16pw = f'"{password}"'.encode('utf-16le') + + new_password = generate_random_password(32, 32) + new_utf16pw = f'"{new_password}"'.encode('utf-16le') + + msg = ldb.Message(ldb.Dn(samdb, dn)) + msg['0'] = ldb.MessageElement(old_utf16pw, + ldb.FLAG_MOD_DELETE, + 'unicodePwd') + msg['1'] = ldb.MessageElement(new_utf16pw, + ldb.FLAG_MOD_ADD, + 'unicodePwd') + + # Indicate that we're ready. This ensures we hit the right transaction + # lock. + pipe.send_bytes(b'0') + + # Wait for the main process to take out a transaction lock. + if not pipe.poll(timeout=5): + raise AssertionError('main process failed to indicate readiness') + + # Try changing the user's password. This should fail, either due to the + # user's account being locked out or due to specifying the wrong password. + try: + samdb.modify(msg) + except ldb.LdbError as err: + num, estr = err.args + if num != ldb.ERR_CONSTRAINT_VIOLATION: + raise AssertionError(f'pwd change raised wrong error code ({err})') + + if f'<{werror.WERR_ACCOUNT_LOCKED_OUT:08X}:' in estr: + return ConnectionResult.LOCKED_OUT + elif f'<{werror.WERR_INVALID_PASSWORD:08X}:' in estr: + return ConnectionResult.WRONG_PASSWORD + else: + raise AssertionError(f'pwd change raised wrong error code ' + f'({estr})') + else: + return ConnectionResult.SUCCESS + + +class LockoutTests(KDCBaseTest): + + def setUp(self): + super().setUp() + self.do_asn1_print = global_asn1_print + self.do_hexdump = global_hexdump + + samdb = self.get_samdb() + base_dn = ldb.Dn(samdb, samdb.domain_dn()) + + def modify_attr(attr, value): + if value is None: + value = [] + flag = ldb.FLAG_MOD_DELETE + else: + value = str(value) + flag = ldb.FLAG_MOD_REPLACE + + msg = ldb.Message(base_dn) + msg[attr] = ldb.MessageElement( + value, flag, attr) + samdb.modify(msg) + + res = samdb.search(base_dn, + scope=ldb.SCOPE_BASE, + attrs=['lockoutDuration', + 'lockoutThreshold', + 'msDS-LogonTimeSyncInterval']) + self.assertEqual(1, len(res)) + + # Reset the lockout duration as it was before. + lockout_duration = res[0].get('lockoutDuration', idx=0) + self.addCleanup(modify_attr, 'lockoutDuration', lockout_duration) + + # Set the new lockout duration: locked out accounts now stay locked + # out. + modify_attr('lockoutDuration', 0) + + # Reset the lockout threshold as it was before. + lockout_threshold = res[0].get('lockoutThreshold', idx=0) + self.addCleanup(modify_attr, 'lockoutThreshold', lockout_threshold) + + # Set the new lockout threshold. + self.lockout_threshold = 3 + modify_attr('lockoutThreshold', self.lockout_threshold) + + # Reset the logon time sync interval as it was before. + sync_interval = res[0].get('msDS-LogonTimeSyncInterval', idx=0) + self.addCleanup(modify_attr, + 'msDS-LogonTimeSyncInterval', + sync_interval) + + # Set the new logon time sync interval. Setting it to 0 eliminates the + # need for this attribute to be updated on logon, and thus the + # requirement to take out a transaction. + modify_attr('msDS-LogonTimeSyncInterval', 0) + + # Get the old 'minPwdAge'. + minPwdAge = samdb.get_minPwdAge() + + # Reset the 'minPwdAge' as it was before. + self.addCleanup(samdb.set_minPwdAge, minPwdAge) + + # Set it temporarily to '0'. + samdb.set_minPwdAge('0') + + def assertLocalSamDB(self, samdb): + if samdb.url.startswith('tdb://'): + return + if samdb.url.startswith('mdb://'): + return + + self.fail(f'connection to {samdb.url} is not local!') + + def wait_for_ready(self, pipe, future): + if pipe.poll(timeout=5): + return + + # We failed to read a response from the pipe, so see if the test raised + # an exception with more information. + if future.done(): + exception = future.exception(timeout=0) + if exception is not None: + raise exception + + self.fail('test failed to indicate readiness') + + def test_lockout_transaction_kdc(self): + self.do_lockout_transaction(connect_kdc) + + def test_lockout_transaction_ntlm(self): + self.do_lockout_transaction(connect_ntlm) + + def test_lockout_transaction_samr(self): + self.do_lockout_transaction(connect_samr) + + def test_lockout_transaction_ldap_pw_change(self): + self.do_lockout_transaction(ldap_pwd_change) + + # Tests to ensure we can handle the account being renamed. We do not test + # renames with SAMR password changes, because in that case the entire + # process happens inside a transaction, and the password change method only + # receives the account username. By the time it searches for the account, + # it will have already been renamed, and so it will always fail to find the + # account. + + def test_lockout_transaction_rename_kdc(self): + self.do_lockout_transaction(connect_kdc, rename=True) + + def test_lockout_transaction_rename_ntlm(self): + self.do_lockout_transaction(connect_ntlm, rename=True) + + def test_lockout_transaction_rename_ldap_pw_change(self): + self.do_lockout_transaction(ldap_pwd_change, rename=True) + + def test_lockout_transaction_bad_pwd_kdc(self): + self.do_lockout_transaction(connect_kdc, correct_pw=False) + + def test_lockout_transaction_bad_pwd_ntlm(self): + self.do_lockout_transaction(connect_ntlm, correct_pw=False) + + def test_lockout_transaction_bad_pwd_samr(self): + self.do_lockout_transaction(connect_samr, correct_pw=False) + + def test_lockout_transaction_bad_pwd_ldap_pw_change(self): + self.do_lockout_transaction(ldap_pwd_change, correct_pw=False) + + def test_bad_pwd_count_transaction_kdc(self): + self.do_bad_pwd_count_transaction(connect_kdc) + + def test_bad_pwd_count_transaction_ntlm(self): + self.do_bad_pwd_count_transaction(connect_ntlm) + + def test_bad_pwd_count_transaction_samr(self): + self.do_bad_pwd_count_transaction(connect_samr) + + def test_bad_pwd_count_transaction_ldap_pw_change(self): + self.do_bad_pwd_count_transaction(ldap_pwd_change) + + def test_bad_pwd_count_transaction_rename_kdc(self): + self.do_bad_pwd_count_transaction(connect_kdc, rename=True) + + def test_bad_pwd_count_transaction_rename_ntlm(self): + self.do_bad_pwd_count_transaction(connect_ntlm, rename=True) + + def test_bad_pwd_count_transaction_rename_ldap_pw_change(self): + self.do_bad_pwd_count_transaction(ldap_pwd_change, rename=True) + + def test_lockout_race_kdc(self): + self.do_lockout_race(connect_kdc) + + def test_lockout_race_ntlm(self): + self.do_lockout_race(connect_ntlm) + + def test_lockout_race_samr(self): + self.do_lockout_race(connect_samr) + + def test_lockout_race_ldap_pw_change(self): + self.do_lockout_race(ldap_pwd_change) + + def test_logon_without_transaction_ntlm(self): + self.do_logon_without_transaction(connect_ntlm) + + # Tests to ensure that the connection functions work correctly in the happy + # path. + + def test_logon_kdc(self): + self.do_logon(partial(connect_kdc, expect_error=False)) + + def test_logon_ntlm(self): + self.do_logon(connect_ntlm) + + def test_logon_samr(self): + self.do_logon(connect_samr) + + def test_logon_ldap_pw_change(self): + self.do_logon(ldap_pwd_change) + + # Test that connection without a correct password works. + def do_logon(self, connect_fn): + # Create the user account for testing. + user_creds = self.get_cached_creds(account_type=self.AccountType.USER, + use_cache=False) + user_dn = user_creds.get_dn() + + admin_creds = self.get_admin_creds() + lp = self.get_lp() + + # Get a connection to our local SamDB. + samdb = connect_samdb(samdb_url=lp.samdb_url(), lp=lp, + credentials=admin_creds) + self.assertLocalSamDB(samdb) + + password = user_creds.get_password() + + # Prepare to connect to the server with a valid password. + our_pipe, their_pipe = Pipe(duplex=True) + + # Inform the test function that it may proceed. + our_pipe.send_bytes(b'0') + + result = connect_fn(pipe=their_pipe, + url=f'ldap://{samdb.host_dns_name()}', + hostname=samdb.host_dns_name(), + username=user_creds.get_username(), + password=password, + domain=user_creds.get_domain(), + realm=user_creds.get_realm(), + workstation=user_creds.get_workstation(), + dn=str(user_dn)) + + # The connection should succeed. + self.assertEqual(result, ConnectionResult.SUCCESS) + + # Lock out the account while holding a transaction lock, then release the + # lock. A logon attempt already in progress should reread the account + # details and recognise the account is locked out. The account can + # additionally be renamed within the transaction to ensure that, by using + # the GUID, rereading the account's details still succeeds. + def do_lockout_transaction(self, connect_fn, + rename=False, + correct_pw=True): + # Create the user account for testing. + user_creds = self.get_cached_creds(account_type=self.AccountType.USER, + use_cache=False) + user_dn = user_creds.get_dn() + + admin_creds = self.get_admin_creds() + lp = self.get_lp() + + # Get a connection to our local SamDB. + samdb = connect_samdb(samdb_url=lp.samdb_url(), lp=lp, + credentials=admin_creds) + self.assertLocalSamDB(samdb) + + password = user_creds.get_password() + if not correct_pw: + password = password[:-1] + + # Prepare to connect to the server. + with futures.ProcessPoolExecutor(max_workers=1) as executor: + our_pipe, their_pipe = Pipe(duplex=True) + connect_future = executor.submit( + connect_fn, + pipe=their_pipe, + url=f'ldap://{samdb.host_dns_name()}', + hostname=samdb.host_dns_name(), + username=user_creds.get_username(), + password=password, + domain=user_creds.get_domain(), + realm=user_creds.get_realm(), + workstation=user_creds.get_workstation(), + dn=str(user_dn)) + + # Wait until the test process indicates it's ready. + self.wait_for_ready(our_pipe, connect_future) + + # Take out a transaction. + samdb.transaction_start() + try: + # Lock out the account. We must do it using an actual password + # check like so, rather than directly with a database + # modification, so that the account is also added to the + # auxiliary bad password database. + + old_utf16pw = f'"Secret007"'.encode('utf-16le') # invalid pwd + new_utf16pw = f'"Secret008"'.encode('utf-16le') + + msg = ldb.Message(user_dn) + msg['0'] = ldb.MessageElement(old_utf16pw, + ldb.FLAG_MOD_DELETE, + 'unicodePwd') + msg['1'] = ldb.MessageElement(new_utf16pw, + ldb.FLAG_MOD_ADD, + 'unicodePwd') + + for i in range(self.lockout_threshold): + try: + samdb.modify(msg) + except ldb.LdbError as err: + num, estr = err.args + + # We get an error, but the bad password count should + # still be updated. + self.assertEqual(num, ldb.ERR_OPERATIONS_ERROR) + self.assertEqual('Failed to obtain remote address for ' + 'the LDAP client while changing the ' + 'password', + estr) + else: + self.fail('pwd change should have failed') + + # Ensure the account is locked out. + + res = samdb.search( + user_dn, scope=ldb.SCOPE_BASE, + attrs=['msDS-User-Account-Control-Computed']) + self.assertEqual(1, len(res)) + + uac = int(res[0].get('msDS-User-Account-Control-Computed', + idx=0)) + self.assertTrue(uac & dsdb.UF_LOCKOUT) + + # Now the bad password database has been updated, inform the + # test process that it may proceed. + our_pipe.send_bytes(b'0') + + # Wait one second to ensure the test process hits the + # transaction lock. + time.sleep(1) + + if rename: + # While we're at it, rename the account to ensure that is + # also safe if a race occurs. + msg = ldb.Message(user_dn) + new_username = self.get_new_username() + msg['sAMAccountName'] = ldb.MessageElement( + new_username, + ldb.FLAG_MOD_REPLACE, + 'sAMAccountName') + samdb.modify(msg) + + except Exception: + samdb.transaction_cancel() + raise + + # Commit the local transaction. + samdb.transaction_commit() + + result = connect_future.result(timeout=5) + self.assertEqual(result, ConnectionResult.LOCKED_OUT) + + # Update the bad password count while holding a transaction lock, then + # release the lock. A logon attempt already in progress should reread the + # account details and ensure the bad password count is atomically + # updated. The account can additionally be renamed within the transaction + # to ensure that, by using the GUID, rereading the account's details still + # succeeds. + def do_bad_pwd_count_transaction(self, connect_fn, rename=False): + # Create the user account for testing. + user_creds = self.get_cached_creds(account_type=self.AccountType.USER, + use_cache=False) + user_dn = user_creds.get_dn() + + admin_creds = self.get_admin_creds() + lp = self.get_lp() + + # Get a connection to our local SamDB. + samdb = connect_samdb(samdb_url=lp.samdb_url(), lp=lp, + credentials=admin_creds) + self.assertLocalSamDB(samdb) + + # Prepare to connect to the server with an invalid password. + with futures.ProcessPoolExecutor(max_workers=1) as executor: + our_pipe, their_pipe = Pipe(duplex=True) + connect_future = executor.submit( + connect_fn, + pipe=their_pipe, + url=f'ldap://{samdb.host_dns_name()}', + hostname=samdb.host_dns_name(), + username=user_creds.get_username(), + password=user_creds.get_password()[:-1], # invalid password + domain=user_creds.get_domain(), + realm=user_creds.get_realm(), + workstation=user_creds.get_workstation(), + dn=str(user_dn)) + + # Wait until the test process indicates it's ready. + self.wait_for_ready(our_pipe, connect_future) + + # Take out a transaction. + samdb.transaction_start() + try: + # Inform the test process that it may proceed. + our_pipe.send_bytes(b'0') + + # Wait one second to ensure the test process hits the + # transaction lock. + time.sleep(1) + + # Set badPwdCount to 1. + msg = ldb.Message(user_dn) + now = int(time.time()) + bad_pwd_time = unix2nttime(now) + msg['badPwdCount'] = ldb.MessageElement( + '1', + ldb.FLAG_MOD_REPLACE, + 'badPwdCount') + msg['badPasswordTime'] = ldb.MessageElement( + str(bad_pwd_time), + ldb.FLAG_MOD_REPLACE, + 'badPasswordTime') + if rename: + # While we're at it, rename the account to ensure that is + # also safe if a race occurs. + new_username = self.get_new_username() + msg['sAMAccountName'] = ldb.MessageElement( + new_username, + ldb.FLAG_MOD_REPLACE, + 'sAMAccountName') + samdb.modify(msg) + + # Ensure the account is not yet locked out. + + res = samdb.search( + user_dn, scope=ldb.SCOPE_BASE, + attrs=['msDS-User-Account-Control-Computed']) + self.assertEqual(1, len(res)) + + uac = int(res[0].get('msDS-User-Account-Control-Computed', + idx=0)) + self.assertFalse(uac & dsdb.UF_LOCKOUT) + except Exception: + samdb.transaction_cancel() + raise + + # Commit the local transaction. + samdb.transaction_commit() + + result = connect_future.result(timeout=5) + self.assertEqual(result, ConnectionResult.WRONG_PASSWORD, result) + + # Check that badPwdCount has now increased to 2. + + res = samdb.search(user_dn, + scope=ldb.SCOPE_BASE, + attrs=['badPwdCount']) + self.assertEqual(1, len(res)) + + bad_pwd_count = int(res[0].get('badPwdCount', idx=0)) + self.assertEqual(2, bad_pwd_count) + + # Attempt to log in to the account with an incorrect password, using + # lockoutThreshold+1 simultaneous attempts. We should get three 'wrong + # password' errors and one 'locked out' error, showing that the bad + # password count is checked and incremented atomically. + def do_lockout_race(self, connect_fn): + # Create the user account for testing. + user_creds = self.get_cached_creds(account_type=self.AccountType.USER, + use_cache=False) + user_dn = user_creds.get_dn() + + admin_creds = self.get_admin_creds() + lp = self.get_lp() + + # Get a connection to our local SamDB. + samdb = connect_samdb(samdb_url=lp.samdb_url(), lp=lp, + credentials=admin_creds) + self.assertLocalSamDB(samdb) + + # Prepare to connect to the server with an invalid password, using four + # simultaneous requests. Only three of those attempts should get + # through before the account is locked out. + num_attempts = self.lockout_threshold + 1 + with futures.ProcessPoolExecutor(max_workers=num_attempts) as executor: + connect_futures = [] + our_pipes = [] + for i in range(num_attempts): + our_pipe, their_pipe = Pipe(duplex=True) + our_pipes.append(our_pipe) + + connect_future = executor.submit( + connect_fn, + pipe=their_pipe, + url=f'ldap://{samdb.host_dns_name()}', + hostname=samdb.host_dns_name(), + username=user_creds.get_username(), + password=user_creds.get_password()[:-1], # invalid pw + domain=user_creds.get_domain(), + realm=user_creds.get_realm(), + workstation=user_creds.get_workstation(), + dn=str(user_dn)) + connect_futures.append(connect_future) + + # Wait until the test process indicates it's ready. + self.wait_for_ready(our_pipe, connect_future) + + # Take out a transaction. + samdb.transaction_start() + try: + # Inform the test processes that they may proceed. + for our_pipe in our_pipes: + our_pipe.send_bytes(b'0') + + # Wait one second to ensure the test processes hit the + # transaction lock. + time.sleep(1) + except Exception: + samdb.transaction_cancel() + raise + + # Commit the local transaction. + samdb.transaction_commit() + + lockouts = 0 + wrong_passwords = 0 + for i, connect_future in enumerate(connect_futures): + result = connect_future.result(timeout=5) + if result == ConnectionResult.LOCKED_OUT: + lockouts += 1 + elif result == ConnectionResult.WRONG_PASSWORD: + wrong_passwords += 1 + else: + self.fail(f'process {i} gave an unexpected result ' + f'{result}') + + self.assertEqual(wrong_passwords, self.lockout_threshold) + self.assertEqual(lockouts, num_attempts - self.lockout_threshold) + + # Ensure the account is now locked out. + + res = samdb.search( + user_dn, scope=ldb.SCOPE_BASE, + attrs=['badPwdCount', + 'msDS-User-Account-Control-Computed']) + self.assertEqual(1, len(res)) + + bad_pwd_count = int(res[0].get('badPwdCount', idx=0)) + self.assertEqual(self.lockout_threshold, bad_pwd_count) + + uac = int(res[0].get('msDS-User-Account-Control-Computed', + idx=0)) + self.assertTrue(uac & dsdb.UF_LOCKOUT) + + # Test that logon is possible even while we locally hold a transaction + # lock. This test only works with NTLM authentication; Kerberos + # authentication must take out a transaction to update the logonCount + # attribute, and LDAP and SAMR password changes both take out a transaction + # to effect the password change. NTLM is the only logon method that does + # not require a transaction, and can thus be performed while we're holding + # the lock. + def do_logon_without_transaction(self, connect_fn): + # Create the user account for testing. + user_creds = self.get_cached_creds(account_type=self.AccountType.USER, + use_cache=False) + user_dn = user_creds.get_dn() + + admin_creds = self.get_admin_creds() + lp = self.get_lp() + + # Get a connection to our local SamDB. + samdb = connect_samdb(samdb_url=lp.samdb_url(), lp=lp, + credentials=admin_creds) + self.assertLocalSamDB(samdb) + + password = user_creds.get_password() + + # Prepare to connect to the server with a valid password. + with futures.ProcessPoolExecutor(max_workers=1) as executor: + our_pipe, their_pipe = Pipe(duplex=True) + connect_future = executor.submit( + connect_fn, + pipe=their_pipe, + url=f'ldap://{samdb.host_dns_name()}', + hostname=samdb.host_dns_name(), + username=user_creds.get_username(), + password=password, + domain=user_creds.get_domain(), + realm=user_creds.get_realm(), + workstation=user_creds.get_workstation(), + dn=str(user_dn)) + + # Wait until the test process indicates it's ready. + self.wait_for_ready(our_pipe, connect_future) + + # Take out a transaction. + samdb.transaction_start() + try: + # Inform the test process that it may proceed. + our_pipe.send_bytes(b'0') + + # The connection should succeed, despite our holding a + # transaction. + result = connect_future.result(timeout=5) + self.assertEqual(result, ConnectionResult.SUCCESS) + except Exception: + samdb.transaction_cancel() + raise + + # Commit the local transaction. + samdb.transaction_commit() + + +if __name__ == '__main__': + global_asn1_print = False + global_hexdump = False + import unittest + unittest.main() diff --git a/python/samba/tests/krb5/raw_testcase.py b/python/samba/tests/krb5/raw_testcase.py index d95361c7f35..b1113e6d4ea 100644 --- a/python/samba/tests/krb5/raw_testcase.py +++ b/python/samba/tests/krb5/raw_testcase.py @@ -50,6 +50,7 @@ from samba.tests.krb5.rfc4120_constants import ( AD_IF_RELEVANT, AD_WIN2K_PAC, FX_FAST_ARMOR_AP_REQUEST, + KDC_ERR_CLIENT_REVOKED, KDC_ERR_GENERIC, KDC_ERR_POLICY, KDC_ERR_PREAUTH_FAILED, @@ -3432,7 +3433,7 @@ class RawKerberosTest(TestCaseInTempDir): expected_patypes += (PADATA_ETYPE_INFO2,) if error_code not in (KDC_ERR_PREAUTH_FAILED, KDC_ERR_SKEW, - KDC_ERR_POLICY): + KDC_ERR_POLICY, KDC_ERR_CLIENT_REVOKED): if sent_fast: expected_patypes += (PADATA_ENCRYPTED_CHALLENGE,) else: diff --git a/python/samba/tests/krb5/rfc4120_constants.py b/python/samba/tests/krb5/rfc4120_constants.py index 8d8340e4711..f7e2daff09b 100644 --- a/python/samba/tests/krb5/rfc4120_constants.py +++ b/python/samba/tests/krb5/rfc4120_constants.py @@ -80,6 +80,7 @@ KDC_ERR_POLICY = 12 KDC_ERR_BADOPTION = 13 KDC_ERR_ETYPE_NOSUPP = 14 KDC_ERR_SUMTYPE_NOSUPP = 15 +KDC_ERR_CLIENT_REVOKED = 18 KDC_ERR_TGT_REVOKED = 20 KDC_ERR_PREAUTH_FAILED = 24 KDC_ERR_PREAUTH_REQUIRED = 25 diff --git a/python/samba/tests/usage.py b/python/samba/tests/usage.py index 037c4ab9faf..77c12bd01af 100644 --- a/python/samba/tests/usage.py +++ b/python/samba/tests/usage.py @@ -111,6 +111,7 @@ EXCLUDE_USAGE = { 'python/samba/tests/krb5/test_idmap_nss.py', 'python/samba/tests/krb5/pac_align_tests.py', 'python/samba/tests/krb5/kpasswd_tests.py', + 'python/samba/tests/krb5/lockout_tests.py', } EXCLUDE_HELP = { diff --git a/selftest/flapping.d/ldap-pwd-change-race b/selftest/flapping.d/ldap-pwd-change-race new file mode 100644 index 00000000000..443f9662a1a --- /dev/null +++ b/selftest/flapping.d/ldap-pwd-change-race @@ -0,0 +1,6 @@ +# This test currently depends on a race. The password_hash dsdb module +# relinquishes and immediately reacquires a transaction lock, and another +# process may be able to acquire it during the short period of time in which it +# is not held. +^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_lockout_race_ldap_pw_change.ad_dc:local +^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.* diff --git a/selftest/knownfail_heimdal_kdc b/selftest/knownfail_heimdal_kdc index a5a995d92f0..be4af0d4bed 100644 --- a/selftest/knownfail_heimdal_kdc +++ b/selftest/knownfail_heimdal_kdc @@ -47,3 +47,13 @@ ^samba.tests.krb5.kdc_tgs_tests.samba.tests.krb5.kdc_tgs_tests.KdcTgsTests.test_tgs_rodc_not_revealed ^samba.tests.krb5.kdc_tgs_tests.samba.tests.krb5.kdc_tgs_tests.KdcTgsTests.test_user2user_rodc_not_revealed ^samba.tests.krb5.kdc_tgs_tests.samba.tests.krb5.kdc_tgs_tests.KdcTgsTests.test_validate_rodc_not_revealed +# +# 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 6d07ca4efb6..def7d6a985e 100644 --- a/selftest/knownfail_mit_kdc +++ b/selftest/knownfail_mit_kdc @@ -550,3 +550,17 @@ samba.tests.krb5.as_canonicalization_tests.samba.tests.krb5.as_canonicalization_ ^samba.tests.krb5.kpasswd_tests.samba.tests.krb5.kpasswd_tests.KpasswdTests.test_kpasswd_canonicalize_realm_case.ad_dc ^samba.tests.krb5.kpasswd_tests.samba.tests.krb5.kpasswd_tests.KpasswdTests.test_kpasswd_no_canonicalize_realm_case.ad_dc ^samba.tests.krb5.kpasswd_tests.samba.tests.krb5.kpasswd_tests.KpasswdTests.test_kpasswd_ticket_requester_sid_tgs.ad_dc +# +# 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 +^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_lockout_transaction_kdc.ad_dc:local +^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_lockout_transaction_rename_kdc.ad_dc:local +^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_logon_kdc.ad_dc:local diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index 576834bfd91..d415f270701 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -1667,6 +1667,10 @@ planoldpythontestsuite( 'ad_dc', 'samba.tests.krb5.kpasswd_tests', environ=krb5_environ) +planoldpythontestsuite( + 'ad_dc:local', + 'samba.tests.krb5.lockout_tests', + environ=krb5_environ) for env in [ 'vampire_dc', -- 2.35.0 From f48691470ad37c24a311e8f848e1ed30d844edb0 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Thu, 15 Sep 2022 16:18:10 +1200 Subject: [PATCH 06/37] CVE-2021-20251 tests/krb5: Convert password lockout tests to use os.fork() and os.pipe() Running the password lockout tests on Fedora 35 occasionally results in errors similar to the following: [1(0)/1 at 0s] samba.tests.krb5.lockout_tests(ad_dc:local) EPOLL_CTL_DEL EBADF for fde[0x5569dc76c670] mpx_fde[(nil)] fd[14] - disabling EPOLL_CTL_DEL EBADF for fde[0x5569dc6089c0] mpx_fde[(nil)] fd[14] - disabling EPOLL_CTL_DEL EBADF for fde[0x5569dbbe58e0] mpx_fde[(nil)] fd[14] - disabling UNEXPECTED(error): samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_lockout_race_kdc(ad_dc:local) REASON: Exception: Exception: concurrent.futures.process._RemoteTraceback: """ Traceback (most recent call last): File "/usr/lib64/python3.10/concurrent/futures/process.py", line 243, in _process_worker r = call_item.fn(*call_item.args, **call_item.kwargs) File "/home/samba/src/bin/python/samba/tests/krb5/lockout_tests.py", line 141, in connect_kdc pipe.send_bytes(b'0') File "/usr/lib64/python3.10/multiprocessing/connection.py", line 205, in send_bytes self._send_bytes(m[offset:offset + size]) File "/usr/lib64/python3.10/multiprocessing/connection.py", line 416, in _send_bytes self._send(header + buf) File "/usr/lib64/python3.10/multiprocessing/connection.py", line 373, in _send n = write(self._handle, buf) OSError: [Errno 9] Bad file descriptor """ The above exception was the direct cause of the following exception: Traceback (most recent call last): File "/home/samba/src/bin/python/samba/tests/krb5/lockout_tests.py", line 537, in test_lockout_race_kdc self.do_lockout_race(connect_kdc) File "/home/samba/src/bin/python/samba/tests/krb5/lockout_tests.py", line 863, in do_lockout_race self.wait_for_ready(our_pipe, connect_future) File "/home/samba/src/bin/python/samba/tests/krb5/lockout_tests.py", line 471, in wait_for_ready raise exception OSError: [Errno 9] Bad file descriptor Such messages can be seen to come from epoll_del_event(). By resorting to lower-level facilites such as fork() and OS pipes, we lose helpful features such as timeouts and propagation of exceptions from child processes, but we may avoid interactions with the event system that lead to failures. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton --- python/samba/tests/krb5/lockout_tests.py | 579 +++++++++++++---------- selftest/flapping.d/ldap-pwd-change-race | 1 - 2 files changed, 336 insertions(+), 244 deletions(-) diff --git a/python/samba/tests/krb5/lockout_tests.py b/python/samba/tests/krb5/lockout_tests.py index dd0ca9db378..19131feb543 100755 --- a/python/samba/tests/krb5/lockout_tests.py +++ b/python/samba/tests/krb5/lockout_tests.py @@ -17,10 +17,8 @@ # along with this program. If not, see . # -from concurrent import futures from enum import Enum from functools import partial -from multiprocessing import Pipe import os import sys import time @@ -74,9 +72,39 @@ class ConnectionResult(Enum): LOCKED_OUT = 1 WRONG_PASSWORD = 2 SUCCESS = 3 + ERROR = 4 -def connect_kdc(pipe, +def fork_connect_fn(connect_fn, + read_pipe, + write_pipe, + *args, + **kwargs): + pid = os.fork() + if pid == 0: + result = ConnectionResult.ERROR + msg = b'done' + try: + result = connect_fn( + read_pipe=read_pipe, + write_pipe=write_pipe, + *args, + **kwargs) + except Exception as err: + msg = repr(err).encode('utf8') + finally: + # Write something to the pipe to provide some information if we + # fail, and so the main process won't wait forever. + os.write(write_pipe, msg) + + # Return result via the exit status. + os._exit(result.value) + + return pid + + +def connect_kdc(read_pipe, + write_pipe, url, hostname, username, @@ -138,10 +166,11 @@ def connect_kdc(pipe, # Indicate that we're ready. This ensures we hit the right transaction # lock. - pipe.send_bytes(b'0') + if os.write(write_pipe, b'ready') != 5: + raise AssertionError('we failed to indicate readiness') # Wait for the main process to take out a transaction lock. - if not pipe.poll(timeout=5): + if os.read(read_pipe, 5) != b'ready': raise AssertionError('main process failed to indicate readiness') # Try making a Kerberos AS-REQ to the KDC. This should fail, either due to @@ -185,7 +214,8 @@ def connect_kdc(pipe, raise AssertionError(f'wrong error code {error_code}') -def connect_ntlm(pipe, +def connect_ntlm(read_pipe, + write_pipe, url, hostname, username, @@ -203,10 +233,11 @@ def connect_ntlm(pipe, # Indicate that we're ready. This ensures we hit the right transaction # lock. - pipe.send_bytes(b'0') + if os.write(write_pipe, b'ready') != 5: + raise AssertionError('we failed to indicate readiness') # Wait for the main process to take out a transaction lock. - if not pipe.poll(timeout=5): + if os.read(read_pipe, 5) != b'ready': raise AssertionError('main process failed to indicate readiness') try: @@ -233,7 +264,8 @@ def connect_ntlm(pipe, return ConnectionResult.SUCCESS -def connect_samr(pipe, +def connect_samr(read_pipe, + write_pipe, url, hostname, username, @@ -292,10 +324,11 @@ def connect_samr(pipe, # Indicate that we're ready. This ensures we hit the right transaction # lock. - pipe.send_bytes(b'0') + if os.write(write_pipe, b'ready') != 5: + raise AssertionError('we failed to indicate readiness') # Wait for the main process to take out a transaction lock. - if not pipe.poll(timeout=5): + if os.read(read_pipe, 5) != b'ready': raise AssertionError('main process failed to indicate readiness') try: @@ -323,7 +356,8 @@ def connect_samr(pipe, return ConnectionResult.SUCCESS -def ldap_pwd_change(pipe, +def ldap_pwd_change(read_pipe, + write_pipe, url, hostname, username, @@ -359,10 +393,11 @@ def ldap_pwd_change(pipe, # Indicate that we're ready. This ensures we hit the right transaction # lock. - pipe.send_bytes(b'0') + if os.write(write_pipe, b'ready') != 5: + raise AssertionError('we failed to indicate readiness') # Wait for the main process to take out a transaction lock. - if not pipe.poll(timeout=5): + if os.read(read_pipe, 5) != b'ready': raise AssertionError('main process failed to indicate readiness') # Try changing the user's password. This should fail, either due to the @@ -459,18 +494,12 @@ class LockoutTests(KDCBaseTest): self.fail(f'connection to {samdb.url} is not local!') - def wait_for_ready(self, pipe, future): - if pipe.poll(timeout=5): - return + def indicate_ready(self, write_pipe): + self.assertEqual(5, os.write(write_pipe, b'ready')) - # We failed to read a response from the pipe, so see if the test raised - # an exception with more information. - if future.done(): - exception = future.exception(timeout=0) - if exception is not None: - raise exception - - self.fail('test failed to indicate readiness') + def wait_for_ready(self, read_pipe): + self.assertEqual(b'ready', os.read(read_pipe, 256), + 'test failed to indicate readiness') def test_lockout_transaction_kdc(self): self.do_lockout_transaction(connect_kdc) @@ -581,12 +610,14 @@ class LockoutTests(KDCBaseTest): password = user_creds.get_password() # Prepare to connect to the server with a valid password. - our_pipe, their_pipe = Pipe(duplex=True) + our_read_pipe, their_write_pipe = os.pipe() + their_read_pipe, our_write_pipe = os.pipe() # Inform the test function that it may proceed. - our_pipe.send_bytes(b'0') + self.indicate_ready(our_write_pipe) - result = connect_fn(pipe=their_pipe, + result = connect_fn(read_pipe=their_read_pipe, + write_pipe=their_write_pipe, url=f'ldap://{samdb.host_dns_name()}', hostname=samdb.host_dns_name(), username=user_creds.get_username(), @@ -624,98 +655,113 @@ class LockoutTests(KDCBaseTest): if not correct_pw: password = password[:-1] + url = f'ldap://{samdb.host_dns_name()}' + hostname = samdb.host_dns_name() + username = user_creds.get_username() + domain = user_creds.get_domain() + realm = user_creds.get_realm() + workstation = user_creds.get_workstation() + dn = str(user_dn) + # Prepare to connect to the server. - with futures.ProcessPoolExecutor(max_workers=1) as executor: - our_pipe, their_pipe = Pipe(duplex=True) - connect_future = executor.submit( - connect_fn, - pipe=their_pipe, - url=f'ldap://{samdb.host_dns_name()}', - hostname=samdb.host_dns_name(), - username=user_creds.get_username(), - password=password, - domain=user_creds.get_domain(), - realm=user_creds.get_realm(), - workstation=user_creds.get_workstation(), - dn=str(user_dn)) + our_read_pipe, their_write_pipe = os.pipe() + their_read_pipe, our_write_pipe = os.pipe() - # Wait until the test process indicates it's ready. - self.wait_for_ready(our_pipe, connect_future) + pid = fork_connect_fn( + connect_fn=connect_fn, + read_pipe=their_read_pipe, + write_pipe=their_write_pipe, + url=url, + hostname=hostname, + username=username, + password=password, + domain=domain, + realm=realm, + workstation=workstation, + dn=dn) - # Take out a transaction. - samdb.transaction_start() - try: - # Lock out the account. We must do it using an actual password - # check like so, rather than directly with a database - # modification, so that the account is also added to the - # auxiliary bad password database. + # Wait until the test process indicates it's ready. + self.wait_for_ready(our_read_pipe) - old_utf16pw = f'"Secret007"'.encode('utf-16le') # invalid pwd - new_utf16pw = f'"Secret008"'.encode('utf-16le') + # Take out a transaction. + samdb.transaction_start() + try: + # Lock out the account. We must do it using an actual password + # check like so, rather than directly with a database + # modification, so that the account is also added to the + # auxiliary bad password database. - msg = ldb.Message(user_dn) - msg['0'] = ldb.MessageElement(old_utf16pw, - ldb.FLAG_MOD_DELETE, - 'unicodePwd') - msg['1'] = ldb.MessageElement(new_utf16pw, - ldb.FLAG_MOD_ADD, - 'unicodePwd') + old_utf16pw = f'"Secret007"'.encode('utf-16le') # invalid pwd + new_utf16pw = f'"Secret008"'.encode('utf-16le') - for i in range(self.lockout_threshold): - try: - samdb.modify(msg) - except ldb.LdbError as err: - num, estr = err.args + msg = ldb.Message(user_dn) + msg['0'] = ldb.MessageElement(old_utf16pw, + ldb.FLAG_MOD_DELETE, + 'unicodePwd') + msg['1'] = ldb.MessageElement(new_utf16pw, + ldb.FLAG_MOD_ADD, + 'unicodePwd') - # We get an error, but the bad password count should - # still be updated. - self.assertEqual(num, ldb.ERR_OPERATIONS_ERROR) - self.assertEqual('Failed to obtain remote address for ' - 'the LDAP client while changing the ' - 'password', - estr) - else: - self.fail('pwd change should have failed') - - # Ensure the account is locked out. - - res = samdb.search( - user_dn, scope=ldb.SCOPE_BASE, - attrs=['msDS-User-Account-Control-Computed']) - self.assertEqual(1, len(res)) - - uac = int(res[0].get('msDS-User-Account-Control-Computed', - idx=0)) - self.assertTrue(uac & dsdb.UF_LOCKOUT) - - # Now the bad password database has been updated, inform the - # test process that it may proceed. - our_pipe.send_bytes(b'0') - - # Wait one second to ensure the test process hits the - # transaction lock. - time.sleep(1) - - if rename: - # While we're at it, rename the account to ensure that is - # also safe if a race occurs. - msg = ldb.Message(user_dn) - new_username = self.get_new_username() - msg['sAMAccountName'] = ldb.MessageElement( - new_username, - ldb.FLAG_MOD_REPLACE, - 'sAMAccountName') + for i in range(self.lockout_threshold): + try: samdb.modify(msg) + except ldb.LdbError as err: + num, estr = err.args - except Exception: - samdb.transaction_cancel() - raise + # We get an error, but the bad password count should + # still be updated. + self.assertEqual(num, ldb.ERR_OPERATIONS_ERROR) + self.assertEqual('Failed to obtain remote address for ' + 'the LDAP client while changing the ' + 'password', + estr) + else: + self.fail('pwd change should have failed') - # Commit the local transaction. - samdb.transaction_commit() + # Ensure the account is locked out. - result = connect_future.result(timeout=5) - self.assertEqual(result, ConnectionResult.LOCKED_OUT) + res = samdb.search( + user_dn, scope=ldb.SCOPE_BASE, + attrs=['msDS-User-Account-Control-Computed']) + self.assertEqual(1, len(res)) + + uac = int(res[0].get('msDS-User-Account-Control-Computed', + idx=0)) + self.assertTrue(uac & dsdb.UF_LOCKOUT) + + # Now the bad password database has been updated, inform the + # test process that it may proceed. + self.indicate_ready(our_write_pipe) + + # Wait one second to ensure the test process hits the + # transaction lock. + time.sleep(1) + + if rename: + # While we're at it, rename the account to ensure that is + # also safe if a race occurs. + msg = ldb.Message(user_dn) + new_username = self.get_new_username() + msg['sAMAccountName'] = ldb.MessageElement( + new_username, + ldb.FLAG_MOD_REPLACE, + 'sAMAccountName') + samdb.modify(msg) + + except Exception: + samdb.transaction_cancel() + raise + + # Commit the local transaction. + samdb.transaction_commit() + + got_pid, status = os.waitpid(pid, 0) + self.assertEqual(pid, got_pid) + + self.assertTrue(os.WIFEXITED(status)) + result = ConnectionResult(os.WEXITSTATUS(status)) + + self.assertEqual(result, ConnectionResult.LOCKED_OUT) # Update the bad password count while holding a transaction lock, then # release the lock. A logon attempt already in progress should reread the @@ -737,75 +783,91 @@ class LockoutTests(KDCBaseTest): credentials=admin_creds) self.assertLocalSamDB(samdb) + url = f'ldap://{samdb.host_dns_name()}' + hostname = samdb.host_dns_name() + username = user_creds.get_username() + password = user_creds.get_password()[:-1] # invalid password + domain = user_creds.get_domain() + realm = user_creds.get_realm() + workstation = user_creds.get_workstation() + dn = str(user_dn) + # Prepare to connect to the server with an invalid password. - with futures.ProcessPoolExecutor(max_workers=1) as executor: - our_pipe, their_pipe = Pipe(duplex=True) - connect_future = executor.submit( - connect_fn, - pipe=their_pipe, - url=f'ldap://{samdb.host_dns_name()}', - hostname=samdb.host_dns_name(), - username=user_creds.get_username(), - password=user_creds.get_password()[:-1], # invalid password - domain=user_creds.get_domain(), - realm=user_creds.get_realm(), - workstation=user_creds.get_workstation(), - dn=str(user_dn)) + our_read_pipe, their_write_pipe = os.pipe() + their_read_pipe, our_write_pipe = os.pipe() - # Wait until the test process indicates it's ready. - self.wait_for_ready(our_pipe, connect_future) + pid = fork_connect_fn( + connect_fn=connect_fn, + read_pipe=their_read_pipe, + write_pipe=their_write_pipe, + url=url, + hostname=hostname, + username=username, + password=password, + domain=domain, + realm=realm, + workstation=workstation, + dn=dn) - # Take out a transaction. - samdb.transaction_start() - try: - # Inform the test process that it may proceed. - our_pipe.send_bytes(b'0') + # Wait until the test process indicates it's ready. + self.wait_for_ready(our_read_pipe) - # Wait one second to ensure the test process hits the - # transaction lock. - time.sleep(1) + # Take out a transaction. + samdb.transaction_start() + try: + # Inform the test process that it may proceed. + self.indicate_ready(our_write_pipe) - # Set badPwdCount to 1. - msg = ldb.Message(user_dn) - now = int(time.time()) - bad_pwd_time = unix2nttime(now) - msg['badPwdCount'] = ldb.MessageElement( - '1', + # Wait one second to ensure the test process hits the + # transaction lock. + time.sleep(1) + + # Set badPwdCount to 1. + msg = ldb.Message(user_dn) + now = int(time.time()) + bad_pwd_time = unix2nttime(now) + msg['badPwdCount'] = ldb.MessageElement( + '1', + ldb.FLAG_MOD_REPLACE, + 'badPwdCount') + msg['badPasswordTime'] = ldb.MessageElement( + str(bad_pwd_time), + ldb.FLAG_MOD_REPLACE, + 'badPasswordTime') + if rename: + # While we're at it, rename the account to ensure that is + # also safe if a race occurs. + new_username = self.get_new_username() + msg['sAMAccountName'] = ldb.MessageElement( + new_username, ldb.FLAG_MOD_REPLACE, - 'badPwdCount') - msg['badPasswordTime'] = ldb.MessageElement( - str(bad_pwd_time), - ldb.FLAG_MOD_REPLACE, - 'badPasswordTime') - if rename: - # While we're at it, rename the account to ensure that is - # also safe if a race occurs. - new_username = self.get_new_username() - msg['sAMAccountName'] = ldb.MessageElement( - new_username, - ldb.FLAG_MOD_REPLACE, - 'sAMAccountName') - samdb.modify(msg) + 'sAMAccountName') + samdb.modify(msg) - # Ensure the account is not yet locked out. + # Ensure the account is not yet locked out. - res = samdb.search( - user_dn, scope=ldb.SCOPE_BASE, - attrs=['msDS-User-Account-Control-Computed']) - self.assertEqual(1, len(res)) + res = samdb.search( + user_dn, scope=ldb.SCOPE_BASE, + attrs=['msDS-User-Account-Control-Computed']) + self.assertEqual(1, len(res)) - uac = int(res[0].get('msDS-User-Account-Control-Computed', - idx=0)) - self.assertFalse(uac & dsdb.UF_LOCKOUT) - except Exception: - samdb.transaction_cancel() - raise + uac = int(res[0].get('msDS-User-Account-Control-Computed', + idx=0)) + self.assertFalse(uac & dsdb.UF_LOCKOUT) + except Exception: + samdb.transaction_cancel() + raise - # Commit the local transaction. - samdb.transaction_commit() + # Commit the local transaction. + samdb.transaction_commit() - result = connect_future.result(timeout=5) - self.assertEqual(result, ConnectionResult.WRONG_PASSWORD, result) + got_pid, status = os.waitpid(pid, 0) + self.assertEqual(pid, got_pid) + + self.assertTrue(os.WIFEXITED(status)) + result = ConnectionResult(os.WEXITSTATUS(status)) + + self.assertEqual(result, ConnectionResult.WRONG_PASSWORD, result) # Check that badPwdCount has now increased to 2. @@ -835,64 +897,80 @@ class LockoutTests(KDCBaseTest): credentials=admin_creds) self.assertLocalSamDB(samdb) + url = f'ldap://{samdb.host_dns_name()}' + hostname = samdb.host_dns_name() + username = user_creds.get_username() + password = user_creds.get_password()[:-1] # invalid pw + domain = user_creds.get_domain() + realm = user_creds.get_realm() + workstation = user_creds.get_workstation() + dn = str(user_dn) + # Prepare to connect to the server with an invalid password, using four # simultaneous requests. Only three of those attempts should get # through before the account is locked out. num_attempts = self.lockout_threshold + 1 - with futures.ProcessPoolExecutor(max_workers=num_attempts) as executor: - connect_futures = [] - our_pipes = [] - for i in range(num_attempts): - our_pipe, their_pipe = Pipe(duplex=True) - our_pipes.append(our_pipe) + our_write_pipes = [] + pids = [] + for i in range(num_attempts): + our_read_pipe, their_write_pipe = os.pipe() + their_read_pipe, our_write_pipe = os.pipe() + our_write_pipes.append(our_write_pipe) - connect_future = executor.submit( - connect_fn, - pipe=their_pipe, - url=f'ldap://{samdb.host_dns_name()}', - hostname=samdb.host_dns_name(), - username=user_creds.get_username(), - password=user_creds.get_password()[:-1], # invalid pw - domain=user_creds.get_domain(), - realm=user_creds.get_realm(), - workstation=user_creds.get_workstation(), - dn=str(user_dn)) - connect_futures.append(connect_future) + pid = fork_connect_fn( + connect_fn=connect_fn, + read_pipe=their_read_pipe, + write_pipe=their_write_pipe, + url=url, + hostname=hostname, + username=username, + password=password, + domain=domain, + realm=realm, + workstation=workstation, + dn=dn) - # Wait until the test process indicates it's ready. - self.wait_for_ready(our_pipe, connect_future) + # Wait until the test process indicates it's ready. + self.wait_for_ready(our_read_pipe) - # Take out a transaction. - samdb.transaction_start() - try: - # Inform the test processes that they may proceed. - for our_pipe in our_pipes: - our_pipe.send_bytes(b'0') + pids.append(pid) - # Wait one second to ensure the test processes hit the - # transaction lock. - time.sleep(1) - except Exception: - samdb.transaction_cancel() - raise + # Take out a transaction. + samdb.transaction_start() + try: + # Inform the test processes that they may proceed. + for our_write_pipe in our_write_pipes: + self.indicate_ready(our_write_pipe) - # Commit the local transaction. - samdb.transaction_commit() + # Wait one second to ensure the test processes hit the + # transaction lock. + time.sleep(1) + except Exception: + samdb.transaction_cancel() + raise - lockouts = 0 - wrong_passwords = 0 - for i, connect_future in enumerate(connect_futures): - result = connect_future.result(timeout=5) - if result == ConnectionResult.LOCKED_OUT: - lockouts += 1 - elif result == ConnectionResult.WRONG_PASSWORD: - wrong_passwords += 1 - else: - self.fail(f'process {i} gave an unexpected result ' - f'{result}') + # Commit the local transaction. + samdb.transaction_commit() - self.assertEqual(wrong_passwords, self.lockout_threshold) - self.assertEqual(lockouts, num_attempts - self.lockout_threshold) + lockouts = 0 + wrong_passwords = 0 + for i, pid in enumerate(pids): + got_pid, status = os.waitpid(pid, 0) + self.assertEqual(pid, got_pid) + + self.assertTrue(os.WIFEXITED(status)) + result = ConnectionResult(os.WEXITSTATUS(status)) + + if result == ConnectionResult.LOCKED_OUT: + lockouts += 1 + elif result == ConnectionResult.WRONG_PASSWORD: + wrong_passwords += 1 + else: + self.fail(f'process {i} gave an unexpected result ' + f'{result}') + + self.assertEqual(wrong_passwords, self.lockout_threshold) + self.assertEqual(lockouts, num_attempts - self.lockout_threshold) # Ensure the account is now locked out. @@ -932,40 +1010,55 @@ class LockoutTests(KDCBaseTest): password = user_creds.get_password() + url = f'ldap://{samdb.host_dns_name()}' + hostname = samdb.host_dns_name() + username = user_creds.get_username() + domain = user_creds.get_domain() + realm = user_creds.get_realm() + workstation = user_creds.get_workstation() + dn = str(user_dn) + # Prepare to connect to the server with a valid password. - with futures.ProcessPoolExecutor(max_workers=1) as executor: - our_pipe, their_pipe = Pipe(duplex=True) - connect_future = executor.submit( - connect_fn, - pipe=their_pipe, - url=f'ldap://{samdb.host_dns_name()}', - hostname=samdb.host_dns_name(), - username=user_creds.get_username(), - password=password, - domain=user_creds.get_domain(), - realm=user_creds.get_realm(), - workstation=user_creds.get_workstation(), - dn=str(user_dn)) + our_read_pipe, their_write_pipe = os.pipe() + their_read_pipe, our_write_pipe = os.pipe() - # Wait until the test process indicates it's ready. - self.wait_for_ready(our_pipe, connect_future) + pid = fork_connect_fn( + connect_fn=connect_fn, + read_pipe=their_read_pipe, + write_pipe=their_write_pipe, + url=url, + hostname=hostname, + username=username, + password=password, + domain=domain, + realm=realm, + workstation=workstation, + dn=dn) - # Take out a transaction. - samdb.transaction_start() - try: - # Inform the test process that it may proceed. - our_pipe.send_bytes(b'0') + # Wait until the test process indicates it's ready. + self.wait_for_ready(our_read_pipe) - # The connection should succeed, despite our holding a - # transaction. - result = connect_future.result(timeout=5) - self.assertEqual(result, ConnectionResult.SUCCESS) - except Exception: - samdb.transaction_cancel() - raise + # Take out a transaction. + samdb.transaction_start() + try: + # Inform the test process that it may proceed. + self.indicate_ready(our_write_pipe) - # Commit the local transaction. - samdb.transaction_commit() + # The connection should succeed, despite our holding a + # transaction. + got_pid, status = os.waitpid(pid, 0) + self.assertEqual(pid, got_pid) + + self.assertTrue(os.WIFEXITED(status)) + result = ConnectionResult(os.WEXITSTATUS(status)) + + self.assertEqual(result, ConnectionResult.SUCCESS) + except Exception: + samdb.transaction_cancel() + raise + + # Commit the local transaction. + samdb.transaction_commit() if __name__ == '__main__': diff --git a/selftest/flapping.d/ldap-pwd-change-race b/selftest/flapping.d/ldap-pwd-change-race index 443f9662a1a..54ed56c1134 100644 --- a/selftest/flapping.d/ldap-pwd-change-race +++ b/selftest/flapping.d/ldap-pwd-change-race @@ -3,4 +3,3 @@ # process may be able to acquire it during the short period of time in which it # is not held. ^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_lockout_race_ldap_pw_change.ad_dc:local -^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.* -- 2.35.0 From 3709de2f611839f7cc974919307e0eb9339b6d64 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 30 Mar 2021 10:51:26 +1300 Subject: [PATCH 07/37] CVE-2021-20251 s4-rpc_server: Use authsam_search_account() to find the user This helps the bad password and audit log handling code as it allows assumptions to be made about the attributes found in the variable "msg", such as that DSDB_SEARCH_SHOW_EXTENDED_DN was used. This ensures we can re-search on the DN via the embedded GUID, which in in turn rename-proof. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Andrew Bartlett Reviewed-by: Joseph Sutton Reviewed-by: Andreas Schneider (cherry picked from commit 439f96a2cfe77f6cbf331d965a387512c2db91c6) [jsutton@samba.org Adapted to LM hash still being present] --- source4/rpc_server/samr/samr_password.c | 38 +++++++++++-------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/source4/rpc_server/samr/samr_password.c b/source4/rpc_server/samr/samr_password.c index c0fbaacd4d8..9e99822cd54 100644 --- a/source4/rpc_server/samr/samr_password.c +++ b/source4/rpc_server/samr/samr_password.c @@ -337,13 +337,7 @@ NTSTATUS dcesrv_samr_ChangePasswordUser3(struct dcesrv_call_state *dce_call, struct ldb_context *sam_ctx = NULL; struct ldb_dn *user_dn = NULL; int ret; - struct ldb_message **res; - const char * const attrs[] = { "unicodePwd", "dBCSPwd", - "userAccountControl", - "msDS-ResultantPSO", - "msDS-User-Account-Control-Computed", - "badPwdCount", "badPasswordTime", - "objectSid", NULL }; + struct ldb_message *msg = NULL; struct samr_Password *nt_pwd, *lm_pwd; struct samr_DomInfo1 *dominfo = NULL; struct userPwdChangeFailureInformation *reject = NULL; @@ -380,24 +374,26 @@ NTSTATUS dcesrv_samr_ChangePasswordUser3(struct dcesrv_call_state *dce_call, return NT_STATUS_INVALID_SYSTEM_SERVICE; } - /* we need the users dn and the domain dn (derived from the - user SID). We also need the current lm and nt password hashes - in order to decrypt the incoming passwords */ - ret = gendb_search(sam_ctx, - mem_ctx, NULL, &res, attrs, - "(&(sAMAccountName=%s)(objectclass=user))", - ldb_binary_encode_string(mem_ctx, r->in.account->string)); - if (ret != 1) { - status = NT_STATUS_NO_SUCH_USER; /* Converted to WRONG_PASSWORD below */ + /* + * We use authsam_search_account() to be consistent with the + * other callers in the bad password and audit log handling + * systems. It ensures we get DSDB_SEARCH_SHOW_EXTENDED_DN. + */ + status = authsam_search_account(mem_ctx, + sam_ctx, + r->in.account->string, + ldb_get_default_basedn(sam_ctx), + &msg); + if (!NT_STATUS_IS_OK(status)) { goto failed; } - user_dn = res[0]->dn; - user_samAccountName = ldb_msg_find_attr_as_string(res[0], "samAccountName", NULL); - user_objectSid = samdb_result_dom_sid(res, res[0], "objectSid"); + user_dn = msg->dn; + user_samAccountName = ldb_msg_find_attr_as_string(msg, "samAccountName", NULL); + user_objectSid = samdb_result_dom_sid(mem_ctx, msg, "objectSid"); status = samdb_result_passwords(mem_ctx, dce_call->conn->dce_ctx->lp_ctx, - res[0], &lm_pwd, &nt_pwd); + msg, &lm_pwd, &nt_pwd); if (!NT_STATUS_IS_OK(status) ) { goto failed; } @@ -538,7 +534,7 @@ failed: /* Only update the badPwdCount if we found the user */ if (NT_STATUS_EQUAL(status, NT_STATUS_WRONG_PASSWORD)) { - authsam_update_bad_pwd_count(sam_ctx, res[0], ldb_get_default_basedn(sam_ctx)); + authsam_update_bad_pwd_count(sam_ctx, msg, ldb_get_default_basedn(sam_ctx)); } else if (NT_STATUS_EQUAL(status, NT_STATUS_NO_SUCH_USER)) { /* Don't give the game away: (don't allow anonymous users to prove the existence of usernames) */ status = NT_STATUS_WRONG_PASSWORD; -- 2.35.0 From 52103fe067e9dc142005ac43fa69f7e9cd43df59 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Tue, 16 Mar 2021 10:52:58 +1300 Subject: [PATCH 08/37] CVE-2021-20251 auth4: split samdb_result_msds_LockoutObservationWindow() out samdb_result_msds_LockoutObservationWindow() is split out of samdb_result_effective_badPwdCount() BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Gary Lockyer Reviewed-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit 2087b0cd986b8959b2a402b9a1891472e47ca0b0) --- source4/dsdb/common/util.c | 45 +++++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c index 551857a526c..e354731375e 100644 --- a/source4/dsdb/common/util.c +++ b/source4/dsdb/common/util.c @@ -5388,9 +5388,9 @@ int dsdb_create_partial_replica_NC(struct ldb_context *ldb, struct ldb_dn *dn) * This also requires that the domain_msg have (if present): * - lockOutObservationWindow */ -static int dsdb_effective_badPwdCount(const struct ldb_message *user_msg, - int64_t lockOutObservationWindow, - NTTIME now) +int dsdb_effective_badPwdCount(const struct ldb_message *user_msg, + int64_t lockOutObservationWindow, + NTTIME now) { int64_t badPasswordTime; badPasswordTime = ldb_msg_find_attr_as_int64(user_msg, "badPasswordTime", 0); @@ -5437,25 +5437,24 @@ static struct ldb_result *lookup_user_pso(struct ldb_context *sam_ldb, } /* - * Return the effective badPwdCount + * Return the msDS-LockoutObservationWindow for a user message * * This requires that the user_msg have (if present): - * - badPasswordTime - * - badPwdCount * - msDS-ResultantPSO */ -int samdb_result_effective_badPwdCount(struct ldb_context *sam_ldb, - TALLOC_CTX *mem_ctx, - struct ldb_dn *domain_dn, - const struct ldb_message *user_msg) +int64_t samdb_result_msds_LockoutObservationWindow( + struct ldb_context *sam_ldb, + TALLOC_CTX *mem_ctx, + struct ldb_dn *domain_dn, + const struct ldb_message *user_msg) { - struct timeval tv_now = timeval_current(); - NTTIME now = timeval_to_nttime(&tv_now); int64_t lockOutObservationWindow; struct ldb_result *res = NULL; const char *attrs[] = { "msDS-LockoutObservationWindow", NULL }; - + if (domain_dn == NULL) { + smb_panic("domain dn is NULL"); + } res = lookup_user_pso(sam_ldb, mem_ctx, user_msg, attrs); if (res != NULL) { @@ -5471,7 +5470,27 @@ int samdb_result_effective_badPwdCount(struct ldb_context *sam_ldb, samdb_search_int64(sam_ldb, mem_ctx, 0, domain_dn, "lockOutObservationWindow", NULL); } + return lockOutObservationWindow; +} +/* + * Return the effective badPwdCount + * + * This requires that the user_msg have (if present): + * - badPasswordTime + * - badPwdCount + * - msDS-ResultantPSO + */ +int samdb_result_effective_badPwdCount(struct ldb_context *sam_ldb, + TALLOC_CTX *mem_ctx, + struct ldb_dn *domain_dn, + const struct ldb_message *user_msg) +{ + struct timeval tv_now = timeval_current(); + NTTIME now = timeval_to_nttime(&tv_now); + int64_t lockOutObservationWindow = + samdb_result_msds_LockoutObservationWindow( + sam_ldb, mem_ctx, domain_dn, user_msg); return dsdb_effective_badPwdCount(user_msg, lockOutObservationWindow, now); } -- 2.35.0 From 74c5805eb9a498b99cce2cdafbdcf95fb7d8bbb4 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Wed, 27 Jan 2021 14:24:58 +1300 Subject: [PATCH 09/37] CVE-2021-20251 s4 auth: Prepare to make bad password count increment atomic To ensure that the bad password count is incremented atomically, and that the successful logon accounting data is updated atomically, without always opening a transaction, we will need to make a note of all bad and successful passwords in a side-DB outside the transaction lock. This provides the functions needed for that and hooks them in (future commits will handle errors and use the results). 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 (cherry picked from commit 408717242aad8adf4551f2394eee2d80a06c7e63) --- source4/auth/sam.c | 187 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 187 insertions(+) diff --git a/source4/auth/sam.c b/source4/auth/sam.c index 7c609655fcb..fb480947569 100644 --- a/source4/auth/sam.c +++ b/source4/auth/sam.c @@ -31,6 +31,8 @@ #include "libcli/ldap/ldap_ndr.h" #include "param/param.h" #include "librpc/gen_ndr/ndr_winbind_c.h" +#include "lib/dbwrap/dbwrap.h" +#include "cluster/cluster.h" #undef DBGC_CLASS #define DBGC_CLASS DBGC_AUTH @@ -825,6 +827,177 @@ static int authsam_get_user_pso(struct ldb_context *sam_ctx, return LDB_SUCCESS; } +static struct db_context *authsam_get_bad_password_db( + TALLOC_CTX *mem_ctx, + struct ldb_context *sam_ctx) +{ + struct loadparm_context *lp_ctx = NULL; + const char *db_name = "bad_password"; + struct db_context *db_ctx = NULL; + + lp_ctx = ldb_get_opaque(sam_ctx, "loadparm"); + if (lp_ctx == NULL) { + DBG_ERR("Unable to get loadparm_context\n"); + return NULL; + } + + db_ctx = cluster_db_tmp_open(mem_ctx, lp_ctx, db_name, TDB_DEFAULT); + if (db_ctx == NULL) { + DBG_ERR("Unable to open bad password attempts database\n"); + return NULL; + } + return db_ctx; +} + +static NTSTATUS get_object_sid_as_tdb_data( + TALLOC_CTX *mem_ctx, + const struct ldb_message *msg, + struct dom_sid_buf *buf, + TDB_DATA *key) +{ + struct dom_sid *objectsid = NULL; + + /* + * Convert the objectSID to a human readable form to + * make debugging easier + */ + objectsid = samdb_result_dom_sid(mem_ctx, msg, "objectSID"); + if (objectsid == NULL) { + DBG_ERR("Unable to extract objectSID\n"); + return NT_STATUS_INTERNAL_ERROR; + } + dom_sid_str_buf(objectsid, buf); + key->dptr = (unsigned char *)buf->buf; + key->dsize = strlen(buf->buf); + + talloc_free(objectsid); + + return NT_STATUS_OK; +} + +/* + * Add the users objectSID to the bad password attempt database + * to indicate that last authentication failed due to a bad password + */ +static NTSTATUS authsam_set_bad_password_indicator( + struct ldb_context *sam_ctx, + TALLOC_CTX *mem_ctx, + const struct ldb_message *msg) +{ + NTSTATUS status = NT_STATUS_OK; + struct dom_sid_buf buf; + TDB_DATA key = {0}; + TDB_DATA value = {0}; + struct db_context *db = NULL; + + TALLOC_CTX *ctx = talloc_new(mem_ctx); + if (ctx == NULL) { + return NT_STATUS_NO_MEMORY; + } + + db = authsam_get_bad_password_db(ctx, sam_ctx); + if (db == NULL) { + status = NT_STATUS_INTERNAL_ERROR; + goto exit; + } + + status = get_object_sid_as_tdb_data(ctx, msg, &buf, &key); + if (!NT_STATUS_IS_OK(status)) { + goto exit; + } + + status = dbwrap_store(db, key, value, 0); + if (!NT_STATUS_IS_OK(status)) { + DBG_ERR("Unable to store bad password indicator\n"); + } +exit: + talloc_free(ctx); + return status; +} + +/* + * see if the users objectSID is in the bad password attempt database + */ +static NTSTATUS authsam_check_bad_password_indicator( + struct ldb_context *sam_ctx, + TALLOC_CTX *mem_ctx, + bool *exists, + const struct ldb_message *msg) +{ + NTSTATUS status = NT_STATUS_OK; + struct dom_sid_buf buf; + TDB_DATA key = {0}; + struct db_context *db = NULL; + + TALLOC_CTX *ctx = talloc_new(mem_ctx); + if (ctx == NULL) { + return NT_STATUS_NO_MEMORY; + } + + db = authsam_get_bad_password_db(ctx, sam_ctx); + if (db == NULL) { + status = NT_STATUS_INTERNAL_ERROR; + goto exit; + } + + status = get_object_sid_as_tdb_data(ctx, msg, &buf, &key); + if (!NT_STATUS_IS_OK(status)) { + goto exit; + } + + *exists = dbwrap_exists(db, key); +exit: + talloc_free(ctx); + return status; +} + +/* + * Remove the users objectSID to the bad password attempt database + * to indicate that last authentication succeeded. + */ +static NTSTATUS authsam_clear_bad_password_indicator( + struct ldb_context *sam_ctx, + TALLOC_CTX *mem_ctx, + const struct ldb_message *msg) +{ + NTSTATUS status = NT_STATUS_OK; + struct dom_sid_buf buf; + TDB_DATA key = {0}; + struct db_context *db = NULL; + + TALLOC_CTX *ctx = talloc_new(mem_ctx); + if (ctx == NULL) { + return NT_STATUS_NO_MEMORY; + } + + db = authsam_get_bad_password_db(ctx, sam_ctx); + if (db == NULL) { + status = NT_STATUS_INTERNAL_ERROR; + goto exit; + } + + status = get_object_sid_as_tdb_data(ctx, msg, &buf, &key); + if (!NT_STATUS_IS_OK(status)) { + goto exit; + } + + status = dbwrap_delete(db, key); + if (NT_STATUS_EQUAL(NT_STATUS_NOT_FOUND, status)) { + /* + * Ok there was no bad password indicator this is expected + */ + status = NT_STATUS_OK; + } + if (NT_STATUS_IS_ERR(status)) { + DBG_ERR("Unable to delete bad password indicator, %s %s\n", + nt_errstr(status), + get_friendly_nt_error_msg(status)); + } +exit: + talloc_free(ctx); + return status; +} + NTSTATUS authsam_update_bad_pwd_count(struct ldb_context *sam_ctx, struct ldb_message *msg, struct ldb_dn *domain_dn) @@ -894,6 +1067,10 @@ NTSTATUS authsam_update_bad_pwd_count(struct ldb_context *sam_ctx, ret = dsdb_autotransaction_request(sam_ctx, req); talloc_free(req); + + status = authsam_set_bad_password_indicator( + sam_ctx, mem_ctx, msg); + /* Failure is ignored for now */ } done: @@ -1057,12 +1234,19 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx, NTTIME now; NTTIME lastLogonTimestamp; bool am_rodc = false; + bool need_db_reread; mem_ctx = talloc_new(msg); if (mem_ctx == NULL) { return NT_STATUS_NO_MEMORY; } + status = authsam_check_bad_password_indicator( + sam_ctx, mem_ctx, &need_db_reread, msg); + if (!NT_STATUS_IS_OK(status)) { + return status; + } + lockoutTime = ldb_msg_find_attr_as_int64(msg, "lockoutTime", 0); dbBadPwdCount = ldb_msg_find_attr_as_int(msg, "badPwdCount", 0); if (interactive_or_kerberos) { @@ -1197,6 +1381,9 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx, talloc_free(req); } + status = authsam_clear_bad_password_indicator(sam_ctx, mem_ctx, msg); + /* Failure is ignored for now */ + done: if (ret != LDB_SUCCESS) { DEBUG(0, ("Failed to set badPwdCount and lockoutTime " -- 2.35.0 From 6e84ff6c09d243a09f214c1d6b9803533fb904f4 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 30 Mar 2021 17:57:10 +1300 Subject: [PATCH 10/37] CVE-2021-20251 auth4: Reread the user record if a bad password is noticed. As is, this is pointless, as we need a transaction to make this any less of a race, but this provides the steps towards that goal. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Andrew Bartlett Reviewed-by: Joseph Sutton Reviewed-by: Andreas Schneider (cherry picked from commit 7b8e32efc336fb728e0c7e3dd6fbe2ed54122124) --- source4/auth/sam.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/source4/auth/sam.c b/source4/auth/sam.c index fb480947569..26ca35a601d 100644 --- a/source4/auth/sam.c +++ b/source4/auth/sam.c @@ -827,6 +827,68 @@ static int authsam_get_user_pso(struct ldb_context *sam_ctx, return LDB_SUCCESS; } +/* + * Re-read the bad password and successful logon data for a user. + * + * The DN in the passed user record should contain the "objectGUID" in case the + * object DN has changed. + */ +NTSTATUS authsam_reread_user_logon_data( + struct ldb_context *sam_ctx, + TALLOC_CTX *mem_ctx, + const struct ldb_message *user_msg, + struct ldb_message **current) +{ + const struct ldb_val *v = NULL; + struct ldb_result *res = NULL; + uint16_t acct_flags = 0; + const char *attr_name = "msDS-User-Account-Control-Computed"; + + int ret; + + /* + * Re-read the account details, using the GUID in case the DN + * is being changed (this is automatic in LDB because the + * original search also used DSDB_SEARCH_SHOW_EXTENDED_DN) + * + * We re read all the attributes in user_attrs, rather than using a + * subset to ensure that we can reuse existing validation code. + */ + ret = dsdb_search_dn(sam_ctx, + mem_ctx, + &res, + user_msg->dn, + user_attrs, + DSDB_SEARCH_SHOW_EXTENDED_DN); + if (ret != LDB_SUCCESS) { + DBG_ERR("Unable to re-read account control data for %s\n", + ldb_dn_get_linearized(user_msg->dn)); + return NT_STATUS_INTERNAL_ERROR; + } + + /* + * Ensure the account has not been locked out by another request + */ + v = ldb_msg_find_ldb_val(res->msgs[0], attr_name); + if (v == NULL || v->data == NULL) { + DBG_ERR("No %s attribute for %s\n", + attr_name, + ldb_dn_get_linearized(user_msg->dn)); + TALLOC_FREE(res); + return NT_STATUS_INTERNAL_ERROR; + } + acct_flags = samdb_result_acct_flags(res->msgs[0], attr_name); + if (acct_flags & ACB_AUTOLOCK) { + DBG_WARNING( + "Account for user %s was locked out.\n", + ldb_dn_get_linearized(user_msg->dn)); + TALLOC_FREE(res); + return NT_STATUS_ACCOUNT_LOCKED_OUT; + } + *current = res->msgs[0]; + return NT_STATUS_OK; +} + static struct db_context *authsam_get_bad_password_db( TALLOC_CTX *mem_ctx, struct ldb_context *sam_ctx) @@ -1247,6 +1309,26 @@ NTSTATUS authsam_logon_success_accounting(struct ldb_context *sam_ctx, return status; } + if (need_db_reread) { + struct ldb_message *current = NULL; + + /* + * Re-read the account details, using the GUID + * embedded in DN so this is safe against a race where + * it is being renamed. + */ + status = authsam_reread_user_logon_data( + sam_ctx, mem_ctx, msg, ¤t); + if (!NT_STATUS_IS_OK(status)) { + /* + * The re-read can return account locked out, as well + * as an internal error + */ + return status; + } + msg = current; + } + lockoutTime = ldb_msg_find_attr_as_int64(msg, "lockoutTime", 0); dbBadPwdCount = ldb_msg_find_attr_as_int(msg, "badPwdCount", 0); if (interactive_or_kerberos) { -- 2.35.0 From 7706d48960b563bac6516e4db9c4730eef6c83d2 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Tue, 9 Feb 2021 11:59:05 +1300 Subject: [PATCH 11/37] CVE-2021-20251 s4 auth test: Unit tests for source4/auth/sam.c cmocka unit tests for the authsam_reread_user_logon_data in source4/auth/sam.c BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Gary Lockyer Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit d6cf245b96fb02edb3bcc52733d040d5f03fb918) --- selftest/knownfail.d/auth-sam | 25 + selftest/tests.py | 2 + source4/auth/tests/sam.c | 2746 +++++++++++++++++++++++++++++++++ source4/auth/wscript_build | 11 + 4 files changed, 2784 insertions(+) create mode 100644 selftest/knownfail.d/auth-sam create mode 100644 source4/auth/tests/sam.c diff --git a/selftest/knownfail.d/auth-sam b/selftest/knownfail.d/auth-sam new file mode 100644 index 00000000000..e87e43d509b --- /dev/null +++ b/selftest/knownfail.d/auth-sam @@ -0,0 +1,25 @@ +^samba.unittests.auth.sam.test_reread_account_not_locked.none +^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 +^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/tests.py b/selftest/tests.py index e35c3fff3c1..06517f6cfba 100644 --- a/selftest/tests.py +++ b/selftest/tests.py @@ -432,6 +432,8 @@ plantestsuite("samba.unittests.test_registry_regfio", "none", [os.path.join(bindir(), "default/source3/test_registry_regfio")]) plantestsuite("samba.unittests.test_oLschema2ldif", "none", [os.path.join(bindir(), "default/source4/utils/oLschema2ldif/test_oLschema2ldif")]) +plantestsuite("samba.unittests.auth.sam", "none", + [os.path.join(bindir(), "test_auth_sam")]) if with_elasticsearch_backend: plantestsuite("samba.unittests.mdsparser_es", "none", [os.path.join(bindir(), "default/source3/test_mdsparser_es")] + [configuration]) diff --git a/source4/auth/tests/sam.c b/source4/auth/tests/sam.c new file mode 100644 index 00000000000..b39408c3699 --- /dev/null +++ b/source4/auth/tests/sam.c @@ -0,0 +1,2746 @@ +/* + * Unit tests for source4/auth/sam.c + * + * Copyright (C) Catalyst.NET Ltd 2021 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +/* + * from cmocka.c: + * These headers or their equivalents should be included prior to + * including + * this header file. + * + * #include + * #include + * #include + * + * This allows test applications to use custom definitions of C standard + * library functions and types. + * + */ + +#include +#include +#include +#include +#include +#include +#include + +#include "includes.h" +#include "auth/sam.c" +#include "ldb.h" +#include "ntstatus.h" +#include "librpc/gen_ndr/ndr_security.h" + +/***************************************************************************** + * wrapped functions + * + *****************************************************************************/ +int __wrap_samdb_msg_add_int64( + struct ldb_context *sam_ldb, + TALLOC_CTX *mem_ctx, + struct ldb_message *msg, + const char *attr_name, + int64_t v); +int __real_samdb_msg_add_int64( + struct ldb_context *sam_ldb, + TALLOC_CTX *mem_ctx, + struct ldb_message *msg, + const char *attr_name, + int64_t v); +int __wrap_samdb_msg_add_int64( + struct ldb_context *sam_ldb, + TALLOC_CTX *mem_ctx, + struct ldb_message *msg, + const char *attr_name, + int64_t v) +{ + + int ret; + ret = (int)mock(); + if (ret != LDB_SUCCESS) { + return ret; + } + return __real_samdb_msg_add_int64(sam_ldb, mem_ctx, msg, attr_name, v); +} +/***************************************************************************** + * Mock implementations + *****************************************************************************/ + +static int check_dn(const LargestIntegralType left_value, + const LargestIntegralType right_value) +{ + /* + * We have to cast away const so we can get the linearized form with + * ldb_dn_get_extended_linearized(). + */ + struct ldb_dn *left_dn = (void *)left_value; + struct ldb_dn *right_dn = (void *)right_value; + char *left_dn_string = NULL; + char *right_dn_string = NULL; + bool ok; + + if (left_dn == NULL && right_dn == NULL) { + return true; + } + + if (left_dn != NULL) { + left_dn_string = ldb_dn_get_extended_linearized(NULL, left_dn, 1); + assert_non_null(left_dn_string); + } + + if (right_dn != NULL) { + right_dn_string = ldb_dn_get_extended_linearized(NULL, right_dn, 1); + assert_non_null(right_dn_string); + } + + if (left_dn_string == NULL || right_dn_string == NULL) { + ok = false; + print_error("\"%s\" != \"%s\"\n", + left_dn_string != NULL ? left_dn_string : "", + right_dn_string != NULL ? right_dn_string : ""); + } else { + ok = (strcmp(left_dn_string, right_dn_string) == 0); + if (!ok) { + print_error("\"%s\" != \"%s\"\n", + left_dn_string, + right_dn_string); + } + + } + + TALLOC_FREE(right_dn_string); + TALLOC_FREE(left_dn_string); + + return ok; +} + +int __wrap_dsdb_search_dn(struct ldb_context *ldb, + TALLOC_CTX *mem_ctx, + struct ldb_result **_result, + struct ldb_dn *basedn, + const char * const *attrs, + uint32_t dsdb_flags); +int __wrap_dsdb_search_dn(struct ldb_context *ldb, + TALLOC_CTX *mem_ctx, + struct ldb_result **_result, + struct ldb_dn *basedn, + const char * const *attrs, + uint32_t dsdb_flags) +{ + check_expected(basedn); + + *_result = talloc_steal(mem_ctx, mock_ptr_type(struct ldb_result *)); + + return mock(); +} + +int ldb_transaction_start_ret = LDB_SUCCESS; +bool in_transaction = false; +int ldb_transaction_start(struct ldb_context *ldb) { + assert_false(in_transaction); + if (ldb_transaction_start_ret == LDB_SUCCESS) { + in_transaction = true; + } + return ldb_transaction_start_ret; +} + +int ldb_transaction_cancel_ret = LDB_SUCCESS; +bool transaction_cancelled = false; +int ldb_transaction_cancel(struct ldb_context *ldb) { + assert_true(in_transaction); + if (ldb_transaction_cancel_ret == LDB_SUCCESS) { + in_transaction = false; + transaction_cancelled = true; + } + return ldb_transaction_cancel_ret; +} + +int ldb_transaction_commit_ret = LDB_SUCCESS; +bool transaction_committed = false; +int ldb_transaction_commit(struct ldb_context *ldb) { + assert_true(in_transaction); + if (ldb_transaction_commit_ret == LDB_SUCCESS) { + in_transaction = false; + transaction_committed = true; + } + return ldb_transaction_commit_ret; +} + +NTSTATUS dsdb_update_bad_pwd_count_ret = NT_STATUS_OK; +struct ldb_message *dsdb_update_bad_pwd_count_res = NULL; +NTSTATUS dsdb_update_bad_pwd_count(TALLOC_CTX *mem_ctx, + struct ldb_context *sam_ctx, + struct ldb_message *user_msg, + struct ldb_message *domain_msg, + struct ldb_message *pso_msg, + struct ldb_message **_mod_msg) { + + *_mod_msg = talloc_move(mem_ctx, &dsdb_update_bad_pwd_count_res); + return dsdb_update_bad_pwd_count_ret; +} + +int ldb_build_mod_req_ret = LDB_SUCCESS; +struct ldb_request *ldb_build_mod_req_res = NULL; +int ldb_build_mod_req(struct ldb_request **ret_req, + struct ldb_context *ldb, + TALLOC_CTX *mem_ctx, + const struct ldb_message *message, + struct ldb_control **controls, + void *context, + ldb_request_callback_t callback, + struct ldb_request *parent) +{ + *ret_req = talloc_move(mem_ctx, &ldb_build_mod_req_res); + return ldb_build_mod_req_ret; +} + +int ldb_request_add_control_ret = LDB_SUCCESS; +int ldb_request_add_control(struct ldb_request *req, + const char *oid, + bool critical, + void *data) +{ + return ldb_request_add_control_ret; +} + +int ldb_request_ret = LDB_SUCCESS; +int ldb_request(struct ldb_context *ldb, + struct ldb_request *req) +{ + return ldb_request_ret; +} + +int ldb_wait_ret = LDB_SUCCESS; +int ldb_wait(struct ldb_handle *handle, + enum ldb_wait_type type) +{ + return ldb_wait_ret; +} +bool ldb_msg_new_fail = false; +struct ldb_message *ldb_msg_new(TALLOC_CTX *mem_ctx) +{ + if (ldb_msg_new_fail) { + return NULL; + } else { + return talloc_zero(mem_ctx, struct ldb_message); + } +} + +int samdb_rodc_ret = LDB_SUCCESS; +bool samdb_rodc_res = false; + +int samdb_rodc( + struct ldb_context *sam_ctx, + bool *am_rodc) +{ + + *am_rodc = samdb_rodc_res; + return samdb_rodc_ret; +} + +struct loadparm_context *ldb_get_opaque_ret = NULL; +void *ldb_get_opaque(struct ldb_context *ldb, const char *name) +{ + return ldb_get_opaque_ret; +} + +struct db_context {}; +struct db_context *cluster_db_tmp_open_ret = NULL; +struct db_context *cluster_db_tmp_open( + TALLOC_CTX *mem_ctx, + struct loadparm_context *lp_ctx, + const char *dbbase, + int flags) +{ + return cluster_db_tmp_open_ret; +} + +NTSTATUS dbwrap_store_ret = NT_STATUS_OK; +NTSTATUS dbwrap_store(struct db_context *db, TDB_DATA key, + TDB_DATA data, int flags) +{ + return dbwrap_store_ret; +} +bool dbwrap_exists_ret = true; + +bool dbwrap_exists(struct db_context *db, TDB_DATA key) +{ + return dbwrap_exists_ret; +} + +NTSTATUS dbwrap_delete_ret = NT_STATUS_OK; +NTSTATUS dbwrap_delete(struct db_context *db, TDB_DATA key) +{ + return dbwrap_delete_ret; +} + +/* + * Set the globals used by the mocked functions to a known and consistent state + * + */ +static void init_mock_results(TALLOC_CTX *mem_ctx) +{ + ldb_transaction_start_ret = LDB_SUCCESS; + in_transaction = false; + + ldb_transaction_cancel_ret = LDB_SUCCESS; + transaction_cancelled = false; + + ldb_transaction_commit_ret = LDB_SUCCESS; + transaction_committed = false; + + dsdb_update_bad_pwd_count_ret = NT_STATUS_OK; + dsdb_update_bad_pwd_count_res = NULL; + + ldb_build_mod_req_ret = LDB_SUCCESS; + ldb_build_mod_req_res = NULL; + + ldb_request_add_control_ret = LDB_SUCCESS; + ldb_request_ret = LDB_SUCCESS; + ldb_wait_ret = LDB_SUCCESS; + + ldb_msg_new_fail = false; + + samdb_rodc_ret = LDB_SUCCESS; + samdb_rodc_res = false; + + ldb_get_opaque_ret = loadparm_init(mem_ctx); + + cluster_db_tmp_open_ret = talloc_zero(mem_ctx, struct db_context); + + dbwrap_store_ret = NT_STATUS_OK; + + dbwrap_exists_ret = true; + + dbwrap_delete_ret = NT_STATUS_OK; + +} + +/***************************************************************************** + * Unit test set up and tear down + *****************************************************************************/ +struct context { +}; + +static int setup(void **state) { + struct context *ctx = talloc_zero(NULL, struct context); + init_mock_results(ctx); + + *state = ctx; + return 0; +} + +static int teardown(void **state) { + struct context *ctx = *state; + TALLOC_FREE(ctx); + return 0; +} + +/****************************************************************************** + * Helper functions + ******************************************************************************/ + +/* + * Build the "Original" user details record, i.e. the user being + * authenticated + */ +static struct ldb_message *create_message(TALLOC_CTX *ctx) +{ + + int ret; + struct timeval tv_now = timeval_current(); + NTTIME now = timeval_to_nttime(&tv_now); + + struct ldb_message *msg = ldb_msg_new(ctx); + + assert_non_null(msg); + ret = samdb_msg_add_int(ctx, msg, msg, "badPwdCount", 10); + assert_int_equal(LDB_SUCCESS, ret); + ret = __real_samdb_msg_add_int64(ctx, msg, msg, "badPasswordTime", now); + assert_int_equal(LDB_SUCCESS, ret); + ret = __real_samdb_msg_add_int64(ctx, msg, msg, "lockoutTime", now); + assert_int_equal(LDB_SUCCESS, ret); + return msg; +} + +/* + * Add a binary objectSID from string form to the supplied message + * + * + */ +static void add_sid( + struct ldb_message *msg, + const char *sid_str) +{ + struct ldb_val v; + enum ndr_err_code ndr_err; + struct dom_sid *sid = NULL; + + sid = talloc_zero(msg, struct dom_sid); + assert_non_null(sid); + assert_true(string_to_sid(sid, sid_str)); + ndr_err = ndr_push_struct_blob( + &v, msg, sid, (ndr_push_flags_fn_t)ndr_push_dom_sid); + assert_true(NDR_ERR_CODE_IS_SUCCESS(ndr_err)); + assert_int_equal(0, ldb_msg_add_value(msg, "objectSID", &v, NULL)); +} + +/* + * Build an ldb_result, for the re-reading of a user record + * + * if account_control < 0 then the msDS-User-Account-Control-Computed + * element is not included + * otherwise it is set to the value passed in account_control. + * + */ +static struct ldb_result *build_reread_result( + struct ldb_context *ldb, + TALLOC_CTX *ctx, + int account_control) +{ + struct ldb_message *msg = NULL; + int ret; + + struct ldb_result *res = talloc_zero(ctx, struct ldb_result); + + assert_non_null(res); + res->count = 1; + res->msgs = talloc_array(res, struct ldb_message *, 1); + + msg = create_message(res); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + if (account_control >= 0) { + ret = samdb_msg_add_int( + ldb, + msg, + msg, + "msDS-User-Account-Control-Computed", + account_control); + assert_int_equal(LDB_SUCCESS, ret); + } + + res->msgs[0] = msg; + return res; +} + +/* + * Build a mock domain pso ldb_result + */ +static struct ldb_result *build_domain_pso_result( + struct ldb_context *ldb, + TALLOC_CTX *ctx) +{ + struct ldb_message *msg = NULL; + struct ldb_result *res = talloc_zero(ctx, struct ldb_result); + + assert_non_null(res); + res->count = 1; + res->msgs = talloc_array(res, struct ldb_message *, 1); + assert_non_null(res->msgs); + msg = talloc_zero(res, struct ldb_message); + assert_non_null(msg); + res->msgs[0] = msg; + return res; +} + +/***************************************************************************** + * authsam_reread_user_logon_data unit tests + *****************************************************************************/ +/* + * authsam_reread_user_logon_data unable to re-read the user record. + * + */ +static void test_reread_read_failure(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_message *cur = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, NULL); + will_return(__wrap_dsdb_search_dn, LDB_ERR_NO_SUCH_OBJECT); + + status = authsam_reread_user_logon_data(ldb, ctx, msg, &cur); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_reread_user_logon_data account control flags missing from + * re-read data + * + */ +static void test_reread_missing_account_control(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_message *cur = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, build_reread_result(ldb, ctx, -1)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + status = authsam_reread_user_logon_data(ldb, ctx, msg, &cur); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_reread_user_logon_data account locked + * re-read data + * + */ +static void test_reread_account_locked(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_message *cur = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, build_reread_result(ldb, ctx, UF_LOCKOUT)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + status = authsam_reread_user_logon_data(ldb, ctx, msg, &cur); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_ACCOUNT_LOCKED_OUT)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_reread_user_logon_data account is not locked + * re-read data + * + */ +static void test_reread_account_not_locked(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_message *cur = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + size_t result_size = 0; + NTSTATUS status; + struct ldb_result *res = NULL; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + /* + * authsam_reread_user_logon_data returns the ldb_message portion + * of the ldb_result created by build_reread_result. + * So the tests for memory leaks will need to adjust for that + */ + res = build_reread_result(ldb, ctx, 0); + will_return(__wrap_dsdb_search_dn, res); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + result_size = talloc_total_size(res) - + talloc_total_size(res->msgs[0]); + before = talloc_total_size(ctx) - result_size; + + status = authsam_reread_user_logon_data(ldb, ctx, msg, &cur); + assert_true(NT_STATUS_IS_OK(status)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + + +/***************************************************************************** + * authsam_update_bad_pwd_count unit tests + *****************************************************************************/ + +/* + * authsam_update_bad_pwd_account + * + * Unable to read the domain_dn record + * + */ +static void test_update_bad_domain_dn_search_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = talloc_zero(ctx, struct ldb_message); + assert_non_null(msg); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, NULL); + will_return(__wrap_dsdb_search_dn, LDB_ERR_NO_SUCH_OBJECT); + + before = talloc_total_size(ctx); + + status = authsam_update_bad_pwd_count(ldb, msg, domain_dn); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_DB_CORRUPTION)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_update_bad_pwd_account + * + * authsam_get_user_pso failure + * + */ +static void test_update_bad_get_pso_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + struct ldb_dn *pso_dn = NULL; + const char *pso_dn_str = "CN=PSO"; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + int ret; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + pso_dn = ldb_dn_new(ctx, ldb, pso_dn_str); + assert_non_null(pso_dn); + + msg = talloc_zero(ctx, struct ldb_message); + assert_non_null(msg); + ret = ldb_msg_add_string(msg, "msDS-ResultantPSO", pso_dn_str); + assert_int_equal(LDB_SUCCESS, ret); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, pso_dn); + will_return(__wrap_dsdb_search_dn, NULL); + will_return(__wrap_dsdb_search_dn, LDB_ERR_NO_SUCH_OBJECT); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, build_reread_result(ldb, ctx, 0)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + status = authsam_update_bad_pwd_count(ldb, msg, domain_dn); + assert_true(NT_STATUS_IS_OK(status)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + + +/* + * authsam_update_bad_pwd_account + * + * start_transaction failure + * + */ +static void test_update_bad_start_txn_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = talloc_zero(ctx, struct ldb_message); + assert_non_null(msg); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + ldb_transaction_start_ret = LDB_ERR_OPERATIONS_ERROR; + + status = authsam_update_bad_pwd_count(ldb, msg, domain_dn); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_update_bad_pwd_account + * + * User details re-read failed + * + */ +static void test_update_bad_reread_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = talloc_zero(ctx, struct ldb_message); + assert_non_null(msg); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, NULL); + will_return(__wrap_dsdb_search_dn, LDB_ERR_NO_SUCH_OBJECT); + + status = authsam_update_bad_pwd_count(ldb, msg, domain_dn); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(transaction_cancelled); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_update_bad_pwd_account + * + * User details re-read reported locked out. + * + */ +static void test_update_bad_reread_locked_out(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, build_reread_result(ldb, ctx, UF_LOCKOUT)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + status = authsam_update_bad_pwd_count(ldb, msg, domain_dn); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_ACCOUNT_LOCKED_OUT)); + assert_false(transaction_cancelled); + assert_true(transaction_committed); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_update_bad_pwd_account + * + * Transaction cancel failure + */ +static void test_update_bad_txn_cancel_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = talloc_zero(ctx, struct ldb_message); + assert_non_null(msg); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, NULL); + will_return(__wrap_dsdb_search_dn, LDB_ERR_NO_SUCH_OBJECT); + + ldb_transaction_cancel_ret = LDB_ERR_OPERATIONS_ERROR; + + status = authsam_update_bad_pwd_count(ldb, msg, domain_dn); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(in_transaction); + assert_false(transaction_cancelled); + assert_false(transaction_committed); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * The following tests all expect the same setup, that is a normal + * good user object and empty domain object. + * + * returns the talloc size after result array setup for leak tests + */ +static size_t setup_bad_password_search_results(TALLOC_CTX *ctx, + struct ldb_context *ldb, + struct ldb_dn *domain_dn, + struct ldb_dn *user_dn) +{ + size_t before = 0; + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, user_dn); + will_return(__wrap_dsdb_search_dn, build_reread_result(ldb, ctx, 0)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + return before; +} + + +/* + * authsam_update_bad_pwd_account + * + * dsdb_update_bad_pwd_count failure + * + */ +static void test_update_bad_update_count_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = setup_bad_password_search_results(ctx, ldb, + domain_dn, + msg->dn); + + dsdb_update_bad_pwd_count_ret = NT_STATUS_INTERNAL_ERROR; + + status = authsam_update_bad_pwd_count(ldb, msg, domain_dn); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(transaction_cancelled); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_update_bad_pwd_account + * + * No need to update the bad password stats + * + */ +static void test_update_bad_no_update_required(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = setup_bad_password_search_results(ctx, ldb, + domain_dn, + msg->dn); + + status = authsam_update_bad_pwd_count(ldb, msg, domain_dn); + assert_true(NT_STATUS_IS_OK(status)); + assert_true(transaction_committed); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_update_bad_pwd_account + * + * Transaction commit failure + * + */ +static void test_update_bad_commit_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = setup_bad_password_search_results(ctx, ldb, + domain_dn, + msg->dn); + + ldb_transaction_commit_ret = LDB_ERR_OPERATIONS_ERROR; + + status = authsam_update_bad_pwd_count(ldb, msg, domain_dn); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(in_transaction); + assert_false(transaction_cancelled); + assert_false(transaction_committed); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_update_bad_pwd_account + * + * ldb_build_mod_req failed building the user update details + * + */ +static void test_update_bad_build_mod_request_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = setup_bad_password_search_results(ctx, ldb, + domain_dn, + msg->dn); + + dsdb_update_bad_pwd_count_res = talloc_zero(ctx, struct ldb_message); + ldb_build_mod_req_ret = LDB_ERR_OPERATIONS_ERROR; + + status = authsam_update_bad_pwd_count(ldb, msg, domain_dn); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(transaction_cancelled); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_update_bad_pwd_account + * + * ldb_request_add_control failed to add DSDB_CONTROL_FORCE_RODC_LOCAL_CHANGE + * to the user update record. + * + */ +static void test_update_bad_add_control_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = setup_bad_password_search_results(ctx, ldb, + domain_dn, + msg->dn); + + dsdb_update_bad_pwd_count_res = talloc_zero(ctx, struct ldb_message); + ldb_build_mod_req_res = talloc_zero(ctx, struct ldb_request); + ldb_request_add_control_ret = LDB_ERR_OPERATIONS_ERROR; + + status = authsam_update_bad_pwd_count(ldb, msg, domain_dn); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(transaction_cancelled); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_update_bad_pwd_account + * + * call to ldb_request failed + * + */ +static void test_update_bad_ldb_request_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = setup_bad_password_search_results(ctx, ldb, + domain_dn, + msg->dn); + + dsdb_update_bad_pwd_count_res = talloc_zero(ctx, struct ldb_message); + ldb_build_mod_req_res = talloc_zero(ctx, struct ldb_request); + ldb_request_ret = LDB_ERR_OPERATIONS_ERROR; + + status = authsam_update_bad_pwd_count(ldb, msg, domain_dn); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(transaction_cancelled); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_update_bad_pwd_account + * + * call to ldb_wait failed + * + */ +static void test_update_bad_ldb_wait_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = setup_bad_password_search_results(ctx, ldb, + domain_dn, + msg->dn); + + dsdb_update_bad_pwd_count_res = talloc_zero(ctx, struct ldb_message); + ldb_build_mod_req_res = talloc_zero(ctx, struct ldb_request); + ldb_wait_ret = LDB_ERR_OPERATIONS_ERROR; + + status = authsam_update_bad_pwd_count(ldb, msg, domain_dn); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(transaction_cancelled); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/***************************************************************************** + * authsam_logon_success_accounting unit tests + *****************************************************************************/ +/* + * authsam_logon_success_accounting + * + * start_transaction failure + * + */ +static void test_success_accounting_start_txn_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + ldb_transaction_start_ret = LDB_ERR_OPERATIONS_ERROR; + + status = authsam_logon_success_accounting( + ldb, msg, domain_dn, true, NULL); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_logon_success_accounting + * + * User details re-read failed + * + */ +static void test_success_accounting_reread_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, NULL); + will_return(__wrap_dsdb_search_dn, LDB_ERR_NO_SUCH_OBJECT); + + status = authsam_logon_success_accounting( + ldb, msg, domain_dn, true, NULL); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(transaction_cancelled); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_logon_success_accounting + * + * ldb_msg_new failed + * + */ +static void test_success_accounting_ldb_msg_new_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, build_reread_result(ldb, ctx, 0)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + ldb_msg_new_fail = true; + + status = authsam_logon_success_accounting( + ldb, msg, domain_dn, true, NULL); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_NO_MEMORY)); + assert_true(transaction_cancelled); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_logon_success_accounting + * + * samdb_rodc failed + * + */ +static void test_success_accounting_samdb_rodc_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + samdb_rodc_ret = LDB_ERR_OPERATIONS_ERROR; + + status = authsam_logon_success_accounting( + ldb, msg, domain_dn, true, NULL); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_false(in_transaction); + assert_false(transaction_cancelled); + assert_false(transaction_committed); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_logon_success_accounting + * + * authsam_update_lastlogon_timestamp failed + * + */ +static void test_success_accounting_update_lastlogon_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + ldb_build_mod_req_res = talloc_zero(ctx, struct ldb_request); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, build_reread_result(ldb, ctx, 0)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + will_return(__wrap_samdb_msg_add_int64, LDB_ERR_OPERATIONS_ERROR); + + status = authsam_logon_success_accounting( + ldb, msg, domain_dn, true, NULL); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_NO_MEMORY)); + assert_true(transaction_cancelled); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_logon_success_accounting + * + * ldb_build_mod_req failed + * + */ +static void test_success_accounting_build_mod_req_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, build_reread_result(ldb, ctx, 0)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + ldb_build_mod_req_ret = LDB_ERR_OPERATIONS_ERROR; + + will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); + will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); + + status = authsam_logon_success_accounting( + ldb, msg, domain_dn, true, NULL); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(transaction_cancelled); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_logon_success_accounting + * + * ldb_request_add_control failed + * + */ +static void test_success_accounting_add_control_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, build_reread_result(ldb, ctx, 0)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + ldb_build_mod_req_res = talloc_zero(ldb, struct ldb_request); + ldb_request_add_control_ret = LDB_ERR_OPERATIONS_ERROR; + + will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); + will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); + + status = authsam_logon_success_accounting( + ldb, msg, domain_dn, true, NULL); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(transaction_cancelled); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_logon_success_accounting + * + * ldb_request failed + * + */ +static void test_success_accounting_ldb_request_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, build_reread_result(ldb, ctx, 0)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + ldb_build_mod_req_res = talloc_zero(ldb, struct ldb_request); + ldb_request_ret = LDB_ERR_OPERATIONS_ERROR; + + will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); + will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); + + status = authsam_logon_success_accounting( + ldb, msg, domain_dn, true, NULL); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(transaction_cancelled); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_logon_success_accounting + * + * ldb_wait failed + * + */ +static void test_success_accounting_ldb_wait_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, build_reread_result(ldb, ctx, 0)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + ldb_build_mod_req_res = talloc_zero(ldb, struct ldb_request); + ldb_wait_ret = LDB_ERR_OPERATIONS_ERROR; + + will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); + will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); + + status = authsam_logon_success_accounting( + ldb, msg, domain_dn, true, NULL); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(transaction_cancelled); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_logon_success_accounting + * + * ldb_transaction_commit failed + * + */ +static void test_success_accounting_commit_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, build_reread_result(ldb, ctx, 0)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + ldb_build_mod_req_res = talloc_zero(ldb, struct ldb_request); + ldb_transaction_commit_ret = LDB_ERR_OPERATIONS_ERROR; + + will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); + will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); + + status = authsam_logon_success_accounting( + ldb, msg, domain_dn, true, NULL); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(in_transaction); + assert_false(transaction_cancelled); + assert_false(transaction_committed); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_logon_success_accounting + * + * ldb_wait failed and then ldb_transaction_cancel failed + * + */ +static void test_success_accounting_rollback_failed(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, build_reread_result(ldb, ctx, 0)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + ldb_build_mod_req_res = talloc_zero(ldb, struct ldb_request); + ldb_wait_ret = LDB_ERR_OPERATIONS_ERROR; + ldb_transaction_cancel_ret = LDB_ERR_OPERATIONS_ERROR; + + will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); + will_return(__wrap_samdb_msg_add_int64, LDB_SUCCESS); + + status = authsam_logon_success_accounting( + ldb, msg, domain_dn, true, NULL); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_INTERNAL_ERROR)); + assert_true(in_transaction); + assert_false(transaction_cancelled); + assert_false(transaction_committed); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * authsam_logon_success_accounting + * + * The bad password indicator is set, but the account is not locked out. + * + */ +static void test_success_accounting_spurious_bad_pwd_indicator(void **state) { + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + struct ldb_dn *domain_dn = NULL; + TALLOC_CTX *ctx = NULL; + size_t before = 0; + size_t after = 0; + NTSTATUS status; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + domain_dn = ldb_dn_new(ctx, ldb, "CN=Domain"); + assert_non_null(domain_dn); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1000"); + + msg->dn = ldb_dn_new(ctx, ldb, "CN=User"); + assert_non_null(msg->dn); + + before = talloc_total_size(ctx); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, domain_dn); + will_return(__wrap_dsdb_search_dn, build_domain_pso_result(ldb, ctx)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + expect_check(__wrap_dsdb_search_dn, basedn, check_dn, msg->dn); + will_return(__wrap_dsdb_search_dn, build_reread_result(ldb, ctx, 0)); + will_return(__wrap_dsdb_search_dn, LDB_SUCCESS); + + will_return_count(__wrap_samdb_msg_add_int64, LDB_SUCCESS, 2); + + /* + * Set the bad password indicator. + */ + status = authsam_set_bad_password_indicator(ldb, ctx, msg); + assert_true(NT_STATUS_EQUAL(NT_STATUS_OK, status)); + + ldb_build_mod_req_res = talloc_zero(ctx, struct ldb_request); + + status = authsam_logon_success_accounting( + ldb, msg, domain_dn, true, NULL); + assert_true(NT_STATUS_EQUAL(status, NT_STATUS_OK)); + assert_false(in_transaction); + assert_false(transaction_cancelled); + assert_true(transaction_committed); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * get_bad_password_db + * + * ldb_get_opaque failure. + */ +static void test_get_bad_password_get_opaque_failed(void **state) { + struct ldb_context *ldb = NULL; + TALLOC_CTX *ctx = NULL; + struct db_context *db = NULL; + size_t before = 0; + size_t after = 0; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + /* + * clear the mock ldb_get_opaque return value, so that we get a null + * response. + */ + TALLOC_FREE(ldb_get_opaque_ret); + + before = talloc_total_size(ctx); + + db = authsam_get_bad_password_db(ctx, ldb); + assert_null(db); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * get_bad_password_db + * + * cluster_db_tmp_open failure. + */ +static void test_get_bad_password_db_open_failed(void **state) { + struct ldb_context *ldb = NULL; + TALLOC_CTX *ctx = NULL; + struct db_context *db = NULL; + size_t before = 0; + size_t after = 0; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + /* + * Clear the mock cluster_db_tmp_open return value so that + * it returns NULL + */ + TALLOC_FREE(cluster_db_tmp_open_ret); + before = talloc_total_size(ctx); + + db = authsam_get_bad_password_db(ctx, ldb); + assert_null(db); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * set_bad_password_indicator + * + * set_bad_password_indicator failure. + */ +static void test_set_bad_password_indicator_get_db_failed(void **state) { + struct ldb_context *ldb = NULL; + TALLOC_CTX *ctx = NULL; + NTSTATUS status; + size_t before = 0; + size_t after = 0; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + /* + * Clear the mock cluster_db_tmp_open return value so that + * it returns NULL + */ + TALLOC_FREE(cluster_db_tmp_open_ret); + before = talloc_total_size(ctx); + + status = authsam_set_bad_password_indicator(ldb, ctx, NULL); + assert_true(NT_STATUS_EQUAL(NT_STATUS_INTERNAL_ERROR, status)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * set_bad_password_indicator + * + * get_object_sid_as_tdb_data failure. + */ +static void test_set_bad_password_indicator_get_object_sid_failed( + void **state) +{ + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + TALLOC_CTX *ctx = NULL; + NTSTATUS status; + size_t before = 0; + size_t after = 0; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + /* + * The created message does not contain an objectSid, so + * get_object_sid_as_tdb_data will fail. + */ + msg = create_message(ctx); + + before = talloc_total_size(ctx); + + status = authsam_set_bad_password_indicator(ldb, ctx, msg); + assert_true(NT_STATUS_EQUAL(NT_STATUS_INTERNAL_ERROR, status)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * set_bad_password_indicator + * + * dbwrap_store failure. + */ +static void test_set_bad_password_indicator_dbwrap_store_failed( + void **state) +{ + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + TALLOC_CTX *ctx = NULL; + NTSTATUS status; + size_t before = 0; + size_t after = 0; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1010"); + + dbwrap_store_ret = NT_STATUS_INTERNAL_DB_CORRUPTION; + + before = talloc_total_size(ctx); + + status = authsam_set_bad_password_indicator(ldb, ctx, msg); + assert_true(NT_STATUS_EQUAL(NT_STATUS_INTERNAL_DB_CORRUPTION, status)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * check_bad_password_indicator + * + * set_bad_password_indicator failure. + */ +static void test_check_bad_password_indicator_get_db_failed(void **state) { + struct ldb_context *ldb = NULL; + TALLOC_CTX *ctx = NULL; + NTSTATUS status; + size_t before = 0; + size_t after = 0; + bool exists = false; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + /* + * Clear the mock cluster_db_tmp_open return value so that + * it returns NULL + */ + TALLOC_FREE(cluster_db_tmp_open_ret); + before = talloc_total_size(ctx); + + status = authsam_check_bad_password_indicator(ldb, ctx, &exists, NULL); + assert_true(NT_STATUS_EQUAL(NT_STATUS_INTERNAL_ERROR, status)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * check_bad_password_indicator + * + * get_object_sid_as_tdb_data failure. + */ +static void test_check_bad_password_indicator_get_object_sid_failed( + void **state) +{ + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + TALLOC_CTX *ctx = NULL; + NTSTATUS status; + size_t before = 0; + size_t after = 0; + bool exists = false; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + /* + * The created message does not contain an objectSid, so + * get_object_sid_as_tdb_data will fail. + */ + msg = create_message(ctx); + + before = talloc_total_size(ctx); + + status = authsam_check_bad_password_indicator(ldb, ctx, &exists, msg); + assert_true(NT_STATUS_EQUAL(NT_STATUS_INTERNAL_ERROR, status)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * clear_bad_password_indicator + * + * set_bad_password_indicator failure. + */ +static void test_clear_bad_password_indicator_get_db_failed(void **state) { + struct ldb_context *ldb = NULL; + TALLOC_CTX *ctx = NULL; + NTSTATUS status; + size_t before = 0; + size_t after = 0; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + /* + * Clear the mock cluster_db_tmp_open return value so that + * it returns NULL + */ + TALLOC_FREE(cluster_db_tmp_open_ret); + before = talloc_total_size(ctx); + + status = authsam_clear_bad_password_indicator(ldb, ctx, NULL); + assert_true(NT_STATUS_EQUAL(NT_STATUS_INTERNAL_ERROR, status)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * clear_bad_password_indicator + * + * get_object_sid_as_tdb_data failure. + */ +static void test_clear_bad_password_indicator_get_object_sid_failed( + void **state) +{ + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + TALLOC_CTX *ctx = NULL; + NTSTATUS status; + size_t before = 0; + size_t after = 0; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + /* + * The created message does not contain an objectSid, so + * get_object_sid_as_tdb_data will fail. + */ + msg = create_message(ctx); + + before = talloc_total_size(ctx); + + status = authsam_clear_bad_password_indicator(ldb, ctx, msg); + assert_true(NT_STATUS_EQUAL(NT_STATUS_INTERNAL_ERROR, status)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * clear_bad_password_indicator + * + * dbwrap_delete failure. + */ +static void test_clear_bad_password_indicator_dbwrap_store_failed( + void **state) +{ + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + TALLOC_CTX *ctx = NULL; + NTSTATUS status; + size_t before = 0; + size_t after = 0; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1010"); + + dbwrap_delete_ret = NT_STATUS_INTERNAL_DB_CORRUPTION; + + before = talloc_total_size(ctx); + + status = authsam_clear_bad_password_indicator(ldb, ctx, msg); + assert_true(NT_STATUS_EQUAL(NT_STATUS_INTERNAL_DB_CORRUPTION, status)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +/* + * clear_bad_password_indicator + * + * dbwrap_delete returns NT_STATUS_NOT_FOUND. + */ +static void test_clear_bad_pwd_indicator_dbwrap_store_not_found( + void **state) +{ + struct ldb_context *ldb = NULL; + struct ldb_message *msg = NULL; + TALLOC_CTX *ctx = NULL; + NTSTATUS status; + size_t before = 0; + size_t after = 0; + + ctx = talloc_new(*state); + assert_non_null(ctx); + + ldb = ldb_init(ctx, NULL); + assert_non_null(ldb); + + msg = create_message(ctx); + add_sid(msg, "S-1-5-21-2470180966-3899876309-2637894779-1010"); + + dbwrap_delete_ret = NT_STATUS_NOT_FOUND; + + before = talloc_total_size(ctx); + + status = authsam_clear_bad_password_indicator(ldb, ctx, msg); + assert_true(NT_STATUS_IS_OK(status)); + + /* + * Check that all allocated memory was freed + */ + after = talloc_total_size(ctx); + assert_int_equal(before, after); + + /* + * Clean up + */ + TALLOC_FREE(ctx); +} + +int main(int argc, const char **argv) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test_setup_teardown( + test_reread_read_failure, setup, teardown), + cmocka_unit_test_setup_teardown( + test_reread_missing_account_control, setup, teardown), + cmocka_unit_test_setup_teardown( + test_reread_account_locked, setup, teardown), + cmocka_unit_test_setup_teardown( + test_reread_account_not_locked, setup, teardown), + cmocka_unit_test_setup_teardown( + test_update_bad_domain_dn_search_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_update_bad_get_pso_failed, setup, teardown), + cmocka_unit_test_setup_teardown( + test_update_bad_start_txn_failed, setup, teardown), + cmocka_unit_test_setup_teardown( + test_update_bad_reread_failed, setup, teardown), + cmocka_unit_test_setup_teardown( + test_update_bad_reread_locked_out, setup, teardown), + cmocka_unit_test_setup_teardown( + test_update_bad_update_count_failed, setup, teardown), + cmocka_unit_test_setup_teardown( + test_update_bad_no_update_required, setup, teardown), + cmocka_unit_test_setup_teardown( + test_update_bad_build_mod_request_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_update_bad_add_control_failed, setup, teardown), + cmocka_unit_test_setup_teardown( + test_update_bad_ldb_request_failed, setup, teardown), + cmocka_unit_test_setup_teardown( + test_update_bad_ldb_wait_failed, setup, teardown), + cmocka_unit_test_setup_teardown( + test_update_bad_txn_cancel_failed, setup, teardown), + cmocka_unit_test_setup_teardown( + test_update_bad_commit_failed, setup, teardown), + cmocka_unit_test_setup_teardown( + test_success_accounting_start_txn_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_success_accounting_reread_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_success_accounting_ldb_msg_new_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_success_accounting_samdb_rodc_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_success_accounting_update_lastlogon_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_success_accounting_build_mod_req_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_success_accounting_add_control_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_success_accounting_ldb_request_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_success_accounting_ldb_wait_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_success_accounting_commit_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_success_accounting_rollback_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_success_accounting_spurious_bad_pwd_indicator, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_get_bad_password_get_opaque_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_get_bad_password_db_open_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_set_bad_password_indicator_get_db_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_set_bad_password_indicator_get_object_sid_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_set_bad_password_indicator_dbwrap_store_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_check_bad_password_indicator_get_db_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_check_bad_password_indicator_get_object_sid_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_clear_bad_password_indicator_get_db_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_clear_bad_password_indicator_get_object_sid_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_clear_bad_password_indicator_dbwrap_store_failed, + setup, + teardown), + cmocka_unit_test_setup_teardown( + test_clear_bad_pwd_indicator_dbwrap_store_not_found, + setup, + teardown), + }; + + cmocka_set_message_output(CM_OUTPUT_SUBUNIT); + return cmocka_run_group_tests(tests, NULL, NULL); +} diff --git a/source4/auth/wscript_build b/source4/auth/wscript_build index 381a7b19bf0..ff1a61a9566 100644 --- a/source4/auth/wscript_build +++ b/source4/auth/wscript_build @@ -49,6 +49,17 @@ bld.SAMBA_BINARY('test_kerberos', for_selftest=True ) +bld.SAMBA_BINARY('test_auth_sam', + source='tests/sam.c', + deps='cmocka samdb samba-security ldb tevent', + local_include=False, + for_selftest=True, + ldflags=''' + -Wl,--wrap,dsdb_search_dn + -Wl,--wrap,samdb_msg_add_int64 + ''' + ) + pytalloc_util = bld.pyembed_libname('pytalloc-util') pyparam_util = bld.pyembed_libname('pyparam_util') pyldb_util = bld.pyembed_libname('pyldb-util') -- 2.35.0 From 9712e371539e4e8d8f458eeaa24d8743e47f3d5e Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 5 Jul 2022 20:17:49 +1200 Subject: [PATCH 12/37] CVE-2021-20251 auth4: Detect ACCOUNT_LOCKED_OUT error for password change This is more specific than NT_STATUS_UNSUCCESSFUL, and for the SAMR password change, matches the result the call to samdb_result_passwords() would give. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit 336e303cf1962b56b64c0d9d2b05ac15d00e8692) --- source4/dsdb/common/util.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c index e354731375e..679daa0348b 100644 --- a/source4/dsdb/common/util.c +++ b/source4/dsdb/common/util.c @@ -2320,7 +2320,8 @@ int samdb_set_password_callback(struct ldb_request *req, struct ldb_reply *ares) * change failed. * * Results: NT_STATUS_OK, NT_STATUS_INVALID_PARAMETER, NT_STATUS_UNSUCCESSFUL, - * NT_STATUS_WRONG_PASSWORD, NT_STATUS_PASSWORD_RESTRICTION + * NT_STATUS_WRONG_PASSWORD, NT_STATUS_PASSWORD_RESTRICTION, + * NT_STATUS_ACCESS_DENIED, NT_STATUS_ACCOUNT_LOCKED_OUT, NT_STATUS_NO_MEMORY */ static NTSTATUS samdb_set_password_internal(struct ldb_context *ldb, TALLOC_CTX *mem_ctx, struct ldb_dn *user_dn, struct ldb_dn *domain_dn, @@ -2502,6 +2503,9 @@ static NTSTATUS samdb_set_password_internal(struct ldb_context *ldb, TALLOC_CTX if (W_ERROR_EQUAL(werr, WERR_PASSWORD_RESTRICTION)) { status = NT_STATUS_PASSWORD_RESTRICTION; } + if (W_ERROR_EQUAL(werr, WERR_ACCOUNT_LOCKED_OUT)) { + status = NT_STATUS_ACCOUNT_LOCKED_OUT; + } } } else if (ret == LDB_ERR_NO_SUCH_OBJECT) { /* don't let the caller know if an account doesn't exist */ @@ -2553,6 +2557,7 @@ NTSTATUS samdb_set_password(struct ldb_context *ldb, TALLOC_CTX *mem_ctx, * Results: NT_STATUS_OK, NT_STATUS_INTERNAL_DB_CORRUPTION, * NT_STATUS_INVALID_PARAMETER, NT_STATUS_UNSUCCESSFUL, * NT_STATUS_WRONG_PASSWORD, NT_STATUS_PASSWORD_RESTRICTION, + * NT_STATUS_ACCESS_DENIED, NT_STATUS_ACCOUNT_LOCKED_OUT, NT_STATUS_NO_MEMORY * NT_STATUS_TRANSACTION_ABORTED, NT_STATUS_NO_SUCH_USER */ NTSTATUS samdb_set_password_sid(struct ldb_context *ldb, TALLOC_CTX *mem_ctx, -- 2.35.0 From 3d39ced39f8a24c0e94395b0a75d84a8025ff9f3 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 30 Mar 2021 18:01:39 +1300 Subject: [PATCH 13/37] 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 (cherry picked from commit de4cc0a3dae89f3e51a099282615cf80c8539e11) --- selftest/knownfail.d/auth-sam | 12 -- selftest/knownfail_heimdal_kdc | 5 - selftest/knownfail_mit_kdc | 3 - source4/auth/sam.c | 296 +++++++++++++++++++++++++++------ 4 files changed, 246 insertions(+), 70 deletions(-) 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 be4af0d4bed..48de0a520c7 100644 --- a/selftest/knownfail_heimdal_kdc +++ b/selftest/knownfail_heimdal_kdc @@ -50,10 +50,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 def7d6a985e..793ba33e696 100644 --- a/selftest/knownfail_mit_kdc +++ b/selftest/knownfail_mit_kdc @@ -554,10 +554,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 26ca35a601d..f740419c7e4 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; } - - ret = dsdb_autotransaction_request(sam_ctx, req); - talloc_free(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_clear_bad_password_indicator(sam_ctx, mem_ctx, msg); + if (!NT_STATUS_IS_OK(status)) { + goto error; } - status = authsam_clear_bad_password_indicator(sam_ctx, mem_ctx, msg); - /* Failure is ignored for now */ + /* + * 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); -done: + 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; } -- 2.35.0 From 070efbca91b60670a421a8d2e710a6bd1dc5dbc7 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 30 Mar 2021 16:35:44 +1300 Subject: [PATCH 14/37] CVE-2021-20251 auth4: Add missing newline to debug message on PSO read failure BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Andrew Bartlett Reviewed-by: Joseph Sutton Reviewed-by: Andreas Schneider (cherry picked from commit 4a9e0fdccfa218fbb2c3eb87e1a955ade0364b98) --- source4/auth/sam.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source4/auth/sam.c b/source4/auth/sam.c index f740419c7e4..2a63238d1b9 100644 --- a/source4/auth/sam.c +++ b/source4/auth/sam.c @@ -1096,7 +1096,7 @@ NTSTATUS authsam_update_bad_pwd_count(struct ldb_context *sam_ctx, * fallback to using the domain defaults so that we still * record the bad password attempt */ - DBG_ERR("Error (%d) checking PSO for %s", + DBG_ERR("Error (%d) checking PSO for %s\n", ret, ldb_dn_get_linearized(msg->dn)); } -- 2.35.0 From fe989c2e39d3c97e44d771df01c2976a1a5619d9 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Thu, 25 Mar 2021 11:30:59 +1300 Subject: [PATCH 15/37] CVE-2021-20251 auth4: Return only the result message and free the surrounding result BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Andrew Bartlett Reviewed-by: Joseph Sutton Reviewed-by: Andreas Schneider (cherry picked from commit b954acfde258a1909ed60c1c3e1015701582719f) --- selftest/knownfail.d/auth-sam | 1 - source4/auth/sam.c | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/selftest/knownfail.d/auth-sam b/selftest/knownfail.d/auth-sam index 048459e6555..438cea46415 100644 --- a/selftest/knownfail.d/auth-sam +++ b/selftest/knownfail.d/auth-sam @@ -1,4 +1,3 @@ -^samba.unittests.auth.sam.test_reread_account_not_locked.none ^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 diff --git a/source4/auth/sam.c b/source4/auth/sam.c index 2a63238d1b9..3190577818c 100644 --- a/source4/auth/sam.c +++ b/source4/auth/sam.c @@ -885,7 +885,8 @@ NTSTATUS authsam_reread_user_logon_data( TALLOC_FREE(res); return NT_STATUS_ACCOUNT_LOCKED_OUT; } - *current = res->msgs[0]; + *current = talloc_steal(mem_ctx, res->msgs[0]); + TALLOC_FREE(res); return NT_STATUS_OK; } -- 2.35.0 From c2c65d16b96e016e3c1f01732e200bbf38dc1278 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Thu, 25 Mar 2021 14:42:39 +1300 Subject: [PATCH 16/37] CVE-2021-20251 auth4: Split authsam_calculate_lastlogon_sync_interval() out authsam_calculate_lastlogon_sync_interval() is split out of authsam_update_lastlogon_timestamp() Based on work 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 (cherry picked from commit 55147335aec8194b6439169b040556a96db22e95) --- source4/auth/sam.c | 115 ++++++++++++++++++++++++++++----------------- 1 file changed, 73 insertions(+), 42 deletions(-) diff --git a/source4/auth/sam.c b/source4/auth/sam.c index 3190577818c..52f23a08aa3 100644 --- a/source4/auth/sam.c +++ b/source4/auth/sam.c @@ -1247,40 +1247,36 @@ error: } -static NTSTATUS authsam_update_lastlogon_timestamp(struct ldb_context *sam_ctx, - struct ldb_message *msg_mod, - struct ldb_dn *domain_dn, - NTTIME old_timestamp, - NTTIME now) +/* + * msDS-LogonTimeSyncInterval is an int32_t number of days. + * + * The docs say: "the initial update, after the domain functional + * level is raised to DS_BEHAVIOR_WIN2003 or higher, is calculated as + * 14 days minus a random percentage of 5 days", but we aren't doing + * that. The blogosphere seems to think that this randomised update + * happens everytime, but [MS-ADA1] doesn't agree. + * + * Dochelp referred us to the following blog post: + * http://blogs.technet.com/b/askds/archive/2009/04/15/the-lastlogontimestamp-attribute-what-it-was-designed-for-and-how-it-works.aspx + * + * when msDS-LogonTimeSyncInterval is zero, the lastLogonTimestamp is + * not changed. + */ + +static NTSTATUS authsam_calculate_lastlogon_sync_interval( + struct ldb_context *sam_ctx, + TALLOC_CTX *ctx, + struct ldb_dn *domain_dn, + NTTIME *sync_interval_nt) { - /* - * We only set lastLogonTimestamp if the current value is older than - * now - msDS-LogonTimeSyncInterval days. - * - * msDS-LogonTimeSyncInterval is an int32_t number of days, while - * lastLogonTimestamp is in the 64 bit 100ns NTTIME format. - * - * The docs say: "the initial update, after the domain functional - * level is raised to DS_BEHAVIOR_WIN2003 or higher, is calculated as - * 14 days minus a random percentage of 5 days", but we aren't doing - * that. The blogosphere seems to think that this randomised update - * happens everytime, but [MS-ADA1] doesn't agree. - * - * Dochelp referred us to the following blog post: - * http://blogs.technet.com/b/askds/archive/2009/04/15/the-lastlogontimestamp-attribute-what-it-was-designed-for-and-how-it-works.aspx - * - * en msDS-LogonTimeSyncInterval is zero, the lastLogonTimestamp is - * not changed. - */ static const char *attrs[] = { "msDS-LogonTimeSyncInterval", NULL }; int ret; struct ldb_result *domain_res = NULL; TALLOC_CTX *mem_ctx = NULL; - int32_t sync_interval; - NTTIME sync_interval_nt; + uint32_t sync_interval; - mem_ctx = talloc_new(msg_mod); + mem_ctx = talloc_new(ctx); if (mem_ctx == NULL) { return NT_STATUS_NO_MEMORY; } @@ -1296,15 +1292,7 @@ static NTSTATUS authsam_update_lastlogon_timestamp(struct ldb_context *sam_ctx, "msDS-LogonTimeSyncInterval", 14); DEBUG(5, ("sync interval is %d\n", sync_interval)); - if (sync_interval == 0){ - /* - * Setting msDS-LogonTimeSyncInterval to zero is how you ask - * that nothing happens here. - */ - TALLOC_FREE(mem_ctx); - return NT_STATUS_OK; - } - else if (sync_interval >= 5){ + if (sync_interval >= 5){ /* * Subtract "a random percentage of 5" days. Presumably this * percentage is between 0 and 100, and modulus is accurate @@ -1312,17 +1300,47 @@ static NTSTATUS authsam_update_lastlogon_timestamp(struct ldb_context *sam_ctx, */ uint32_t r = generate_random() % 6; sync_interval -= r; - DEBUG(5, ("randomised sync interval is %d (-%d)\n", sync_interval, r)); + DBG_INFO("randomised sync interval is %d (-%d)\n", sync_interval, r); } /* In the case where sync_interval < 5 there is no randomisation */ - sync_interval_nt = sync_interval * 24LL * 3600LL * 10000000LL; + /* + * msDS-LogonTimeSyncInterval is an int32_t number of days, + * while lastLogonTimestamp (to be updated) is in the 64 bit + * 100ns NTTIME format so we must convert. + */ + *sync_interval_nt = sync_interval * 24LL * 3600LL * 10000000LL; + TALLOC_FREE(mem_ctx); + return NT_STATUS_OK; +} + +/* + * We only set lastLogonTimestamp if the current value is older than + * now - msDS-LogonTimeSyncInterval days. + * + * lastLogonTimestamp is in the 64 bit 100ns NTTIME format + */ +static NTSTATUS authsam_update_lastlogon_timestamp(struct ldb_context *sam_ctx, + struct ldb_message *msg_mod, + struct ldb_dn *domain_dn, + NTTIME old_timestamp, + NTTIME now, + NTTIME sync_interval_nt) +{ + int ret; DEBUG(5, ("old timestamp is %lld, threshold %lld, diff %lld\n", (long long int)old_timestamp, (long long int)(now - sync_interval_nt), (long long int)(old_timestamp - now + sync_interval_nt))); + if (sync_interval_nt == 0){ + /* + * Setting msDS-LogonTimeSyncInterval to zero is how you ask + * that nothing happens here. + */ + return NT_STATUS_OK; + } if (old_timestamp > now){ DEBUG(0, ("lastLogonTimestamp is in the future! (%lld > %lld)\n", (long long int)old_timestamp, (long long int)now)); @@ -1337,11 +1355,9 @@ static NTSTATUS authsam_update_lastlogon_timestamp(struct ldb_context *sam_ctx, "lastLogonTimestamp", now); if (ret != LDB_SUCCESS) { - TALLOC_FREE(mem_ctx); return NT_STATUS_NO_MEMORY; } } - TALLOC_FREE(mem_ctx); return NT_STATUS_OK; } @@ -1550,8 +1566,23 @@ get_transaction: } if (!am_rodc) { - status = authsam_update_lastlogon_timestamp(sam_ctx, msg_mod, domain_dn, - lastLogonTimestamp, now); + 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, + domain_dn, + lastLogonTimestamp, + now, + sync_interval_nt); if (!NT_STATUS_IS_OK(status)) { status = NT_STATUS_NO_MEMORY; goto error; -- 2.35.0 From ac1970fa760f9cb3653f8d013e96976803a1e38d Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Thu, 25 Mar 2021 15:33:08 +1300 Subject: [PATCH 17/37] CVE-2021-20251 auth4: Inline samdb_result_effective_badPwdCount() in authsam_logon_success_accounting() By bringing this function inline it can then be split out in a subsequent commit. Based on work 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 (cherry picked from commit 712181032a47318576ef35f6a6cf0f958aa538fb) --- source4/auth/sam.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/source4/auth/sam.c b/source4/auth/sam.c index 52f23a08aa3..6ef22182b8f 100644 --- a/source4/auth/sam.c +++ b/source4/auth/sam.c @@ -1479,11 +1479,17 @@ get_transaction: lockoutTime = ldb_msg_find_attr_as_int64(msg, "lockoutTime", 0); dbBadPwdCount = ldb_msg_find_attr_as_int(msg, "badPwdCount", 0); + tv_now = timeval_current(); + now = timeval_to_nttime(&tv_now); + if (interactive_or_kerberos) { badPwdCount = dbBadPwdCount; } else { - badPwdCount = samdb_result_effective_badPwdCount(sam_ctx, mem_ctx, - domain_dn, msg); + int64_t lockOutObservationWindow = + samdb_result_msds_LockoutObservationWindow( + sam_ctx, mem_ctx, domain_dn, msg); + badPwdCount = dsdb_effective_badPwdCount( + msg, lockOutObservationWindow, now); } lastLogonTimestamp = ldb_msg_find_attr_as_int64(msg, "lastLogonTimestamp", 0); @@ -1521,9 +1527,6 @@ get_transaction: } } - tv_now = timeval_current(); - now = timeval_to_nttime(&tv_now); - if (interactive_or_kerberos || (badPwdCount != 0 && lockoutTime == 0)) { ret = samdb_msg_add_int64(sam_ctx, msg_mod, msg_mod, -- 2.35.0 From d0ff61f5910bf87f830e5afb6a79b4c166557406 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 30 Mar 2021 16:48:31 +1300 Subject: [PATCH 18/37] 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 Reviewed-by: Joseph Sutton Reviewed-by: Andreas Schneider (cherry picked from commit b5f78b7b895a6b92cfdc9221b18d67ab18bc2a24) --- selftest/knownfail.d/auth-sam | 12 -------- source4/auth/sam.c | 55 +++++++++++++++++++++++------------ 2 files changed, 36 insertions(+), 31 deletions(-) delete mode 100644 selftest/knownfail.d/auth-sam diff --git a/selftest/knownfail.d/auth-sam b/selftest/knownfail.d/auth-sam deleted file mode 100644 index 438cea46415..00000000000 --- 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 6ef22182b8f..8b575a9bc51 100644 --- 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, -- 2.35.0 From d874615c8f4eef5a429ebdb518594ade37bbae0f Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Sat, 9 Jul 2022 15:53:51 +1200 Subject: [PATCH 19/37] CVE-2021-20251 s4-auth: Pass through error code from badPwdCount update The error code may be NT_STATUS_ACCOUNT_LOCKED_OUT, which we use in preference to NT_STATUS_WRONG_PASSWORD. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit d8a862cb811489abb67d4cf3a7fbd83d05c7e5cb) --- selftest/knownfail_heimdal_kdc | 1 - selftest/knownfail_mit_kdc | 1 - source4/auth/ntlm/auth_sam.c | 6 +++++- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/selftest/knownfail_heimdal_kdc b/selftest/knownfail_heimdal_kdc index 48de0a520c7..645f6d671f9 100644 --- a/selftest/knownfail_heimdal_kdc +++ b/selftest/knownfail_heimdal_kdc @@ -51,4 +51,3 @@ # Lockout tests # ^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 793ba33e696..a560941de1b 100644 --- a/selftest/knownfail_mit_kdc +++ b/selftest/knownfail_mit_kdc @@ -556,7 +556,6 @@ samba.tests.krb5.as_canonicalization_tests.samba.tests.krb5.as_canonicalization_ ^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_rename_kdc.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 ^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_lockout_transaction_kdc.ad_dc:local ^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_lockout_transaction_rename_kdc.ad_dc:local diff --git a/source4/auth/ntlm/auth_sam.c b/source4/auth/ntlm/auth_sam.c index cf0656ae0da..9c4790c7c3f 100644 --- a/source4/auth/ntlm/auth_sam.c +++ b/source4/auth/ntlm/auth_sam.c @@ -518,7 +518,11 @@ static NTSTATUS authsam_password_check_and_record(struct auth4_context *auth_con } TALLOC_FREE(tmp_ctx); - return NT_STATUS_WRONG_PASSWORD; + + if (NT_STATUS_IS_OK(nt_status)) { + nt_status = NT_STATUS_WRONG_PASSWORD; + } + return nt_status; } static NTSTATUS authsam_authenticate(struct auth4_context *auth_context, -- 2.35.0 From 8f6ad1482641e46d1d0771b398ca20c8642206b7 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Sat, 9 Jul 2022 15:44:21 +1200 Subject: [PATCH 20/37] CVE-2021-20251 s4:dsdb: Update bad password count inside transaction Previously, there was a gap between calling dsdb_update_bad_pwd_count() and dsdb_module_modify() where no transaction was in effect. Another process could slip in and modify badPwdCount, only for our update to immediately overwrite it. Doing the update inside the transaction will help for the following commit when we make it atomic. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit a65147a9e98ead70869cdfa20ffcc9c167dbf535) [jsutton@samba.org Fixed conflicts due to lack of dbg_ret variable] --- .../dsdb/samdb/ldb_modules/password_hash.c | 37 ++++++++++--------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/password_hash.c b/source4/dsdb/samdb/ldb_modules/password_hash.c index 3978d82a79a..f44fa418e81 100644 --- a/source4/dsdb/samdb/ldb_modules/password_hash.c +++ b/source4/dsdb/samdb/ldb_modules/password_hash.c @@ -2695,24 +2695,6 @@ static int make_error_and_update_badPwdCount(struct setup_password_fields_io *io NTSTATUS status; int ret; - /* PSO search result is optional (NULL if no PSO applies) */ - if (io->ac->pso_res != NULL) { - pso_msg = io->ac->pso_res->message; - } - - status = dsdb_update_bad_pwd_count(io->ac, ldb, - io->ac->search_res->message, - io->ac->dom_res->message, - pso_msg, - &mod_msg); - if (!NT_STATUS_IS_OK(status)) { - goto done; - } - - if (mod_msg == NULL) { - goto done; - } - /* * OK, horrible semantics ahead. * @@ -2755,6 +2737,24 @@ static int make_error_and_update_badPwdCount(struct setup_password_fields_io *io goto done; } + /* PSO search result is optional (NULL if no PSO applies) */ + if (io->ac->pso_res != NULL) { + pso_msg = io->ac->pso_res->message; + } + + status = dsdb_update_bad_pwd_count(io->ac, ldb, + io->ac->search_res->message, + io->ac->dom_res->message, + pso_msg, + &mod_msg); + if (!NT_STATUS_IS_OK(status)) { + goto end_transaction; + } + + if (mod_msg == NULL) { + goto end_transaction; + } + ret = dsdb_module_modify(io->ac->module, mod_msg, DSDB_FLAG_NEXT_MODULE, io->ac->req); @@ -2768,6 +2768,7 @@ static int make_error_and_update_badPwdCount(struct setup_password_fields_io *io */ } +end_transaction: ret = ldb_next_end_trans(io->ac->module); if (ret != LDB_SUCCESS) { ldb_debug(ldb, LDB_DEBUG_ERROR, -- 2.35.0 From 2451d80d18510f7fb9edea4106f1898efe15cc0b Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Sat, 9 Jul 2022 15:54:12 +1200 Subject: [PATCH 21/37] CVE-2021-20251 s4:dsdb: Make badPwdCount update atomic We reread the account details inside the transaction in case the account has been locked out in the meantime. If it has, we return the appropriate error code. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit 96479747bdb5bc5f33d903085f5f69793f369e3a) [jsutton@samba.org Fixed conflict due to lack of dbg_ret variable] --- selftest/flapping.d/ldap-pwd-change-race | 5 ---- .../dsdb/samdb/ldb_modules/password_hash.c | 27 ++++++++++++++++--- 2 files changed, 24 insertions(+), 8 deletions(-) delete mode 100644 selftest/flapping.d/ldap-pwd-change-race diff --git a/selftest/flapping.d/ldap-pwd-change-race b/selftest/flapping.d/ldap-pwd-change-race deleted file mode 100644 index 54ed56c1134..00000000000 --- a/selftest/flapping.d/ldap-pwd-change-race +++ /dev/null @@ -1,5 +0,0 @@ -# This test currently depends on a race. The password_hash dsdb module -# relinquishes and immediately reacquires a transaction lock, and another -# process may be able to acquire it during the short period of time in which it -# is not held. -^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_lockout_race_ldap_pw_change.ad_dc:local diff --git a/source4/dsdb/samdb/ldb_modules/password_hash.c b/source4/dsdb/samdb/ldb_modules/password_hash.c index f44fa418e81..fb4deeae9f5 100644 --- a/source4/dsdb/samdb/ldb_modules/password_hash.c +++ b/source4/dsdb/samdb/ldb_modules/password_hash.c @@ -45,6 +45,7 @@ #include "lib/crypto/md4.h" #include "param/param.h" #include "lib/krb5_wrap/krb5_samba.h" +#include "auth/auth_sam.h" #include "auth/common_auth.h" #include "lib/messaging/messaging.h" #include "lib/param/loadparm.h" @@ -2692,7 +2693,8 @@ static int make_error_and_update_badPwdCount(struct setup_password_fields_io *io struct ldb_context *ldb = ldb_module_get_ctx(io->ac->module); struct ldb_message *mod_msg = NULL; struct ldb_message *pso_msg = NULL; - NTSTATUS status; + struct ldb_message *current = NULL; + NTSTATUS status = NT_STATUS_OK; int ret; /* @@ -2737,13 +2739,28 @@ static int make_error_and_update_badPwdCount(struct setup_password_fields_io *io goto done; } + /* + * Re-read the account details, using the GUID in case the DN + * is being changed. + */ + status = authsam_reread_user_logon_data( + ldb, io->ac, + io->ac->search_res->message, + ¤t); + if (!NT_STATUS_IS_OK(status)) { + /* The re-read can return account locked out, as well + * as an internal error + */ + goto end_transaction; + } + /* PSO search result is optional (NULL if no PSO applies) */ if (io->ac->pso_res != NULL) { pso_msg = io->ac->pso_res->message; } status = dsdb_update_bad_pwd_count(io->ac, ldb, - io->ac->search_res->message, + current, io->ac->dom_res->message, pso_msg, &mod_msg); @@ -2793,7 +2810,11 @@ end_transaction: done: ret = LDB_ERR_CONSTRAINT_VIOLATION; - *werror = WERR_INVALID_PASSWORD; + if (NT_STATUS_EQUAL(status, NT_STATUS_ACCOUNT_LOCKED_OUT)) { + *werror = WERR_ACCOUNT_LOCKED_OUT; + } else { + *werror = WERR_INVALID_PASSWORD; + } ldb_asprintf_errstring(ldb, "%08X: %s - check_password_restrictions: " "The old password specified doesn't match!", -- 2.35.0 From 3c1c9228ba4577cea83c6ff2a87b6df93d2cc7a1 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Wed, 6 Jul 2022 11:11:43 +1200 Subject: [PATCH 22/37] CVE-2021-20251 s4:kdc: Move logon success accounting code into existing branch This simplifies the code for the following commit. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit 2b593c34c4f5cb82440b940766e53626c1cbec5b) --- source4/kdc/hdb-samba4.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/source4/kdc/hdb-samba4.c b/source4/kdc/hdb-samba4.c index 3705a086bb2..1f88ad68e60 100644 --- a/source4/kdc/hdb-samba4.c +++ b/source4/kdc/hdb-samba4.c @@ -600,26 +600,6 @@ static krb5_error_code hdb_samba4_audit(krb5_context context, } switch (hdb_auth_status) { - case KDC_AUTH_EVENT_CLIENT_AUTHORIZED: - { - TALLOC_CTX *frame = talloc_stackframe(); - struct samba_kdc_entry *p = talloc_get_type(entry->context, - struct samba_kdc_entry); - struct netr_SendToSamBase *send_to_sam = NULL; - - /* - * TODO: We could log the AS-REQ authorization success here as - * well. However before we do that, we need to pass - * in the PAC here or re-calculate it. - */ - authsam_logon_success_accounting(kdc_db_ctx->samdb, p->msg, - domain_dn, true, &send_to_sam); - if (kdc_db_ctx->rodc && send_to_sam != NULL) { - reset_bad_password_netlogon(frame, kdc_db_ctx, send_to_sam); - } - talloc_free(frame); - } - FALL_THROUGH; default: { TALLOC_CTX *frame = talloc_stackframe(); @@ -661,6 +641,19 @@ static krb5_error_code hdb_samba4_audit(krb5_context context, ui.auth_description = auth_description; if (hdb_auth_status == KDC_AUTH_EVENT_CLIENT_AUTHORIZED) { + struct netr_SendToSamBase *send_to_sam = NULL; + + /* + * TODO: We could log the AS-REQ authorization success here as + * well. However before we do that, we need to pass + * in the PAC here or re-calculate it. + */ + authsam_logon_success_accounting(kdc_db_ctx->samdb, p->msg, + domain_dn, true, &send_to_sam); + if (kdc_db_ctx->rodc && send_to_sam != NULL) { + reset_bad_password_netlogon(frame, kdc_db_ctx, send_to_sam); + } + /* This is the final sucess */ status = NT_STATUS_OK; } else if (hdb_auth_status == KDC_AUTH_EVENT_VALIDATED_LONG_TERM_KEY) { -- 2.35.0 From 5753d8fef7f3513d29bc327cc2d8a4e5567892ab Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Fri, 1 Jul 2022 15:04:41 +1200 Subject: [PATCH 23/37] CVE-2021-20251 s4:kdc: Check return status of authsam_logon_success_accounting() If we find that the user has been locked out sometime during the request (due to a race), we will now return an error code. Note that we cannot avoid the MIT KDC aspect of the issue by checking the return status of mit_samba_zero_bad_password_count(), because kdb_vftabl::audit_as_req() returning void means we cannot pass on the result. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit b1e740896ebae14ba64250da2f718e1d707e9eed) --- source4/kdc/hdb-samba4.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/source4/kdc/hdb-samba4.c b/source4/kdc/hdb-samba4.c index 1f88ad68e60..4882f19e760 100644 --- a/source4/kdc/hdb-samba4.c +++ b/source4/kdc/hdb-samba4.c @@ -648,14 +648,21 @@ static krb5_error_code hdb_samba4_audit(krb5_context context, * well. However before we do that, we need to pass * in the PAC here or re-calculate it. */ - authsam_logon_success_accounting(kdc_db_ctx->samdb, p->msg, - domain_dn, true, &send_to_sam); - if (kdc_db_ctx->rodc && send_to_sam != NULL) { + status = authsam_logon_success_accounting(kdc_db_ctx->samdb, p->msg, + domain_dn, true, &send_to_sam); + if (NT_STATUS_EQUAL(status, NT_STATUS_ACCOUNT_LOCKED_OUT)) { + final_ret = KRB5KDC_ERR_CLIENT_REVOKED; + r->error_code = final_ret; + rwdc_fallback = kdc_db_ctx->rodc; + } else if (!NT_STATUS_IS_OK(status)) { + final_ret = KRB5KRB_ERR_GENERIC; + r->error_code = final_ret; + rwdc_fallback = kdc_db_ctx->rodc; + } else if (kdc_db_ctx->rodc && send_to_sam != NULL) { reset_bad_password_netlogon(frame, kdc_db_ctx, send_to_sam); } /* This is the final sucess */ - status = NT_STATUS_OK; } else if (hdb_auth_status == KDC_AUTH_EVENT_VALIDATED_LONG_TERM_KEY) { /* * This was only a pre-authentication success, -- 2.35.0 From be79610525d87adee3b0e8296ab3995929b0eaa6 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Sat, 9 Jul 2022 15:54:52 +1200 Subject: [PATCH 24/37] CVE-2021-20251 s4:kdc: Check badPwdCount update return status If the account has been locked out in the meantime (indicated by NT_STATUS_ACCOUNT_LOCKED_OUT), we should return the appropriate error code. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit bdfc9d96f8fe5070ab8a189bbf42ccb7e77afb73) [jsutton@samba.org Fixed knownfail conflicts due to not having claims tests] [jsutton@samba.org Fixed knownfail conflicts] --- selftest/knownfail_heimdal_kdc | 4 ---- source4/kdc/hdb-samba4.c | 9 +++++++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/selftest/knownfail_heimdal_kdc b/selftest/knownfail_heimdal_kdc index 645f6d671f9..a5a995d92f0 100644 --- a/selftest/knownfail_heimdal_kdc +++ b/selftest/knownfail_heimdal_kdc @@ -47,7 +47,3 @@ ^samba.tests.krb5.kdc_tgs_tests.samba.tests.krb5.kdc_tgs_tests.KdcTgsTests.test_tgs_rodc_not_revealed ^samba.tests.krb5.kdc_tgs_tests.samba.tests.krb5.kdc_tgs_tests.KdcTgsTests.test_user2user_rodc_not_revealed ^samba.tests.krb5.kdc_tgs_tests.samba.tests.krb5.kdc_tgs_tests.KdcTgsTests.test_validate_rodc_not_revealed -# -# Lockout tests -# -^samba.tests.krb5.lockout_tests.samba.tests.krb5.lockout_tests.LockoutTests.test_lockout_race_kdc.ad_dc:local diff --git a/source4/kdc/hdb-samba4.c b/source4/kdc/hdb-samba4.c index 4882f19e760..4c431712dff 100644 --- a/source4/kdc/hdb-samba4.c +++ b/source4/kdc/hdb-samba4.c @@ -702,8 +702,13 @@ static krb5_error_code hdb_samba4_audit(krb5_context context, } else if (hdb_auth_status == KDC_AUTH_EVENT_CLIENT_TIME_SKEW) { status = NT_STATUS_TIME_DIFFERENCE_AT_DC; } else if (hdb_auth_status == KDC_AUTH_EVENT_WRONG_LONG_TERM_KEY) { - authsam_update_bad_pwd_count(kdc_db_ctx->samdb, p->msg, domain_dn); - status = NT_STATUS_WRONG_PASSWORD; + status = authsam_update_bad_pwd_count(kdc_db_ctx->samdb, p->msg, domain_dn); + if (NT_STATUS_EQUAL(status, NT_STATUS_ACCOUNT_LOCKED_OUT)) { + final_ret = KRB5KDC_ERR_CLIENT_REVOKED; + r->error_code = final_ret; + } else { + status = NT_STATUS_WRONG_PASSWORD; + } rwdc_fallback = kdc_db_ctx->rodc; } else if (hdb_auth_status == KDC_AUTH_EVENT_CLIENT_LOCKED_OUT) { status = NT_STATUS_ACCOUNT_LOCKED_OUT; -- 2.35.0 From c20bab7318317e02875066c5fc04eabb26143c1a Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Sat, 9 Jul 2022 15:55:02 +1200 Subject: [PATCH 25/37] CVE-2021-20251 s4-rpc_server: Check badPwdCount update return status If the account has been locked out in the meantime (indicated by NT_STATUS_ACCOUNT_LOCKED_OUT), we should return the appropriate error code. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit a268a1a0e304d0702469e4ac146d8af5e7384c39) --- source4/rpc_server/samr/samr_password.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/source4/rpc_server/samr/samr_password.c b/source4/rpc_server/samr/samr_password.c index 9e99822cd54..fa323c1a49a 100644 --- a/source4/rpc_server/samr/samr_password.c +++ b/source4/rpc_server/samr/samr_password.c @@ -534,7 +534,11 @@ failed: /* Only update the badPwdCount if we found the user */ if (NT_STATUS_EQUAL(status, NT_STATUS_WRONG_PASSWORD)) { - authsam_update_bad_pwd_count(sam_ctx, msg, ldb_get_default_basedn(sam_ctx)); + NTSTATUS bad_pwd_status = authsam_update_bad_pwd_count( + sam_ctx, msg, ldb_get_default_basedn(sam_ctx)); + if (NT_STATUS_EQUAL(bad_pwd_status, NT_STATUS_ACCOUNT_LOCKED_OUT)) { + status = bad_pwd_status; + } } else if (NT_STATUS_EQUAL(status, NT_STATUS_NO_SUCH_USER)) { /* Don't give the game away: (don't allow anonymous users to prove the existence of usernames) */ status = NT_STATUS_WRONG_PASSWORD; -- 2.35.0 From c2ed83fe0462af76b56aa4f0a992b8d36975bd32 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Mon, 4 Jul 2022 20:51:38 +1200 Subject: [PATCH 26/37] CVE-2021-20251 s4:auth_winbind: Check return status of authsam_logon_success_accounting() This may return an error if we find the account is locked out. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit 268ea7bef5af4b9c8a02f4f5856113ff0664d9e8) --- source4/auth/ntlm/auth_winbind.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/source4/auth/ntlm/auth_winbind.c b/source4/auth/ntlm/auth_winbind.c index d7879966603..6381f866667 100644 --- a/source4/auth/ntlm/auth_winbind.c +++ b/source4/auth/ntlm/auth_winbind.c @@ -252,11 +252,14 @@ static void winbind_check_password_done(struct tevent_req *subreq) status = authsam_search_account(state, ctx->auth_ctx->sam_ctx, nt4_account, domain_dn, &msg); if (NT_STATUS_IS_OK(status)) { - authsam_logon_success_accounting( + status = authsam_logon_success_accounting( ctx->auth_ctx->sam_ctx, msg, domain_dn, user_info->flags & USER_INFO_INTERACTIVE_LOGON, NULL); + if (tevent_req_nterror(req, status)) { + return; + } } } -- 2.35.0 From e33836470d875f81278b4b8faf933a85c2f2daed Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 11 Jan 2021 12:11:35 -0800 Subject: [PATCH 27/37] CVE-2021-20251 s3: ensure bad password count atomic updates The bad password count is supposed to limit the number of failed login attempt a user can make before being temporarily locked out, but race conditions between processes have allowed determined attackers to make many more than the specified number of attempts. This is especially bad on constrained or overcommitted hardware. To fix this, once a bad password is detected, we reload the sam account information under a user-specific mutex, ensuring we have an up to date bad password count. Discovered by Nathaniel W. Turner. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Jeremy Allison Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit 8587734bf989aeaafa9d09d78d0f381caf52d285) --- source3/auth/check_samsec.c | 77 +++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/source3/auth/check_samsec.c b/source3/auth/check_samsec.c index b9563c958a9..bd8ca8de2f0 100644 --- a/source3/auth/check_samsec.c +++ b/source3/auth/check_samsec.c @@ -379,6 +379,8 @@ NTSTATUS check_sam_security(const DATA_BLOB *challenge, const uint8_t *nt_pw; const uint8_t *lm_pw; uint32_t acct_ctrl; + char *mutex_name_by_user = NULL; + struct named_mutex *mtx = NULL; /* the returned struct gets kept on the server_info, by means of a steal further down */ @@ -418,6 +420,79 @@ NTSTATUS check_sam_security(const DATA_BLOB *challenge, challenge, lm_pw, nt_pw, user_info, &user_sess_key, &lm_sess_key); + /* + * We must re-load the sam acount information under a mutex + * lock to ensure we don't miss any concurrent account lockout + * changes. + */ + + /* Clear out old sampass info. */ + TALLOC_FREE(sampass); + acct_ctrl = 0; + username = NULL; + nt_pw = NULL; + lm_pw = NULL; + + sampass = samu_new(mem_ctx); + if (sampass == NULL) { + return NT_STATUS_NO_MEMORY; + } + + mutex_name_by_user = talloc_asprintf(mem_ctx, + "check_sam_security_mutex_%s", + user_info->mapped.account_name); + if (mutex_name_by_user == NULL) { + nt_status = NT_STATUS_NO_MEMORY; + goto done; + } + + /* Grab the named mutex under root with 30 second timeout. */ + become_root(); + mtx = grab_named_mutex(mem_ctx, mutex_name_by_user, 30); + if (mtx != NULL) { + /* Re-load the account information if we got the mutex. */ + ret = pdb_getsampwnam(sampass, user_info->mapped.account_name); + } + unbecome_root(); + + /* Everything from here on until mtx is freed is done under the mutex.*/ + + if (mtx == NULL) { + DBG_ERR("Acquisition of mutex %s failed " + "for user %s\n", + mutex_name_by_user, + user_info->mapped.account_name); + nt_status = NT_STATUS_INTERNAL_ERROR; + goto done; + } + + if (!ret) { + /* + * Re-load of account failed. This could only happen if the + * user was deleted in the meantime. + */ + DBG_NOTICE("reload of user '%s' in passdb failed.\n", + user_info->mapped.account_name); + nt_status = NT_STATUS_NO_SUCH_USER; + goto done; + } + + /* Re-load the account control info. */ + acct_ctrl = pdb_get_acct_ctrl(sampass); + username = pdb_get_username(sampass); + + /* + * Check if the account is now locked out - now under the mutex. + * This can happen if the server is under + * a password guess attack and the ACB_AUTOLOCK is set by + * another process. + */ + if (acct_ctrl & ACB_AUTOLOCK) { + DBG_NOTICE("Account for user %s was locked out.\n", username); + nt_status = NT_STATUS_ACCOUNT_LOCKED_OUT; + goto done; + } + /* Notify passdb backend of login success/failure. If not NT_STATUS_OK the backend doesn't like the login */ @@ -510,6 +585,8 @@ done: TALLOC_FREE(sampass); data_blob_free(&user_sess_key); data_blob_free(&lm_sess_key); + TALLOC_FREE(mutex_name_by_user); + TALLOC_FREE(mtx); return nt_status; } -- 2.35.0 From 1e0b482753426713e740bf8fa19175163abffeec Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 5 Jul 2022 20:17:33 +1200 Subject: [PATCH 28/37] CVE-2021-20251 s3: Ensure bad password count atomic updates for SAMR password change The bad password count is supposed to limit the number of failed login attempt a user can make before being temporarily locked out, but race conditions between processes have allowed determined attackers to make many more than the specified number of attempts. This is especially bad on constrained or overcommitted hardware. To fix this, once a bad password is detected, we reload the sam account information under a user-specific mutex, ensuring we have an up to date bad password count. Derived from a similar patch to source3/auth/check_samsec.c by Jeremy Allison BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit 65c473d4a53fc8a22a0d531aff45203ea3a4d99b) --- source3/rpc_server/samr/srv_samr_chgpasswd.c | 81 ++++++++++++++++++-- 1 file changed, 75 insertions(+), 6 deletions(-) diff --git a/source3/rpc_server/samr/srv_samr_chgpasswd.c b/source3/rpc_server/samr/srv_samr_chgpasswd.c index e326745169e..798993bb542 100644 --- a/source3/rpc_server/samr/srv_samr_chgpasswd.c +++ b/source3/rpc_server/samr/srv_samr_chgpasswd.c @@ -1201,6 +1201,8 @@ NTSTATUS pass_oem_change(char *user, const char *rhost, bool ret = false; bool updated_badpw = false; NTSTATUS update_login_attempts_status; + char *mutex_name_by_user = NULL; + struct named_mutex *mtx = NULL; if (!(sampass = samu_new(NULL))) { return NT_STATUS_NO_MEMORY; @@ -1212,15 +1214,15 @@ NTSTATUS pass_oem_change(char *user, const char *rhost, if (ret == false) { DEBUG(0,("pass_oem_change: getsmbpwnam returned NULL\n")); - TALLOC_FREE(sampass); - return NT_STATUS_NO_SUCH_USER; + nt_status = NT_STATUS_NO_SUCH_USER; + goto done; } /* Quit if the account was locked out. */ if (pdb_get_acct_ctrl(sampass) & ACB_AUTOLOCK) { DEBUG(3,("check_sam_security: Account for user %s was locked out.\n", user)); - TALLOC_FREE(sampass); - return NT_STATUS_ACCOUNT_LOCKED_OUT; + nt_status = NT_STATUS_ACCOUNT_LOCKED_OUT; + goto done; } nt_status = check_oem_password(user, @@ -1231,6 +1233,71 @@ NTSTATUS pass_oem_change(char *user, const char *rhost, sampass, &new_passwd); + /* + * We must re-load the sam acount information under a mutex + * lock to ensure we don't miss any concurrent account lockout + * changes. + */ + + /* Clear out old sampass info. */ + TALLOC_FREE(sampass); + + sampass = samu_new(NULL); + if (sampass == NULL) { + return NT_STATUS_NO_MEMORY; + } + + mutex_name_by_user = talloc_asprintf(NULL, + "check_sam_security_mutex_%s", + user); + if (mutex_name_by_user == NULL) { + nt_status = NT_STATUS_NO_MEMORY; + goto done; + } + + /* Grab the named mutex under root with 30 second timeout. */ + become_root(); + mtx = grab_named_mutex(NULL, mutex_name_by_user, 30); + if (mtx != NULL) { + /* Re-load the account information if we got the mutex. */ + ret = pdb_getsampwnam(sampass, user); + } + unbecome_root(); + + /* Everything from here on until mtx is freed is done under the mutex.*/ + + if (mtx == NULL) { + DBG_ERR("Acquisition of mutex %s failed " + "for user %s\n", + mutex_name_by_user, + user); + nt_status = NT_STATUS_INTERNAL_ERROR; + goto done; + } + + if (!ret) { + /* + * Re-load of account failed. This could only happen if the + * user was deleted in the meantime. + */ + DBG_NOTICE("reload of user '%s' in passdb failed.\n", + user); + nt_status = NT_STATUS_NO_SUCH_USER; + goto done; + } + + /* + * Check if the account is now locked out - now under the mutex. + * This can happen if the server is under + * a password guess attack and the ACB_AUTOLOCK is set by + * another process. + */ + if (pdb_get_acct_ctrl(sampass) & ACB_AUTOLOCK) { + DBG_NOTICE("Account for user %s was locked out.\n", user); + nt_status = NT_STATUS_ACCOUNT_LOCKED_OUT; + goto done; + } + /* * Notify passdb backend of login success/failure. If not * NT_STATUS_OK the backend doesn't like the login @@ -1278,8 +1345,7 @@ NTSTATUS pass_oem_change(char *user, const char *rhost, } if (!NT_STATUS_IS_OK(nt_status)) { - TALLOC_FREE(sampass); - return nt_status; + goto done; } /* We've already checked the old password here.... */ @@ -1290,7 +1356,10 @@ NTSTATUS pass_oem_change(char *user, const char *rhost, memset(new_passwd, 0, strlen(new_passwd)); +done: TALLOC_FREE(sampass); + TALLOC_FREE(mutex_name_by_user); + TALLOC_FREE(mtx); return nt_status; } -- 2.35.0 From 288763062c0f5379c51b07f60ac17666545da76c Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 2 Aug 2022 14:35:33 +1200 Subject: [PATCH 29/37] lib:util: Check memset_s() error code in talloc_keep_secret_destructor() Panic if memset_s() fails. Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit 03a50d8f7d872b6ef701d1207061c88b73d171bb) --- lib/util/talloc_keep_secret.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/util/talloc_keep_secret.c b/lib/util/talloc_keep_secret.c index d6aa38265f6..c427406cbba 100644 --- a/lib/util/talloc_keep_secret.c +++ b/lib/util/talloc_keep_secret.c @@ -20,13 +20,26 @@ static int talloc_keep_secret_destructor(void *ptr) { + int ret; size_t size = talloc_get_size(ptr); if (unlikely(size == 0)) { return 0; } - memset_s(ptr, size, 0, size); + ret = memset_s(ptr, size, 0, size); + if (unlikely(ret != 0)) { + char *msg = NULL; + int ret2; + ret2 = asprintf(&msg, + "talloc_keep_secret_destructor: memset_s() failed: %s", + strerror(ret)); + if (ret2 != -1) { + smb_panic(msg); + } else { + smb_panic("talloc_keep_secret_destructor: memset_s() failed"); + } + } return 0; } -- 2.35.0 From ce41eeab3a662f2457e4223fe8c822968c02b5b2 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 2 Aug 2022 14:35:50 +1200 Subject: [PATCH 30/37] libcli:auth: Keep passwords from convert_string_talloc() secret Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit 6edf88f5c40421b9881666a2e78038ea9c547c24) [jsutton@samba.org Removed change to decode_pwd_string_from_buffer514() that is not present in 4.16] --- libcli/auth/smbencrypt.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libcli/auth/smbencrypt.c b/libcli/auth/smbencrypt.c index fe20fe4e2a3..c4339272fb6 100644 --- a/libcli/auth/smbencrypt.c +++ b/libcli/auth/smbencrypt.c @@ -937,6 +937,7 @@ bool decode_pw_buffer(TALLOC_CTX *ctx, DEBUG(0, ("decode_pw_buffer: failed to convert incoming password\n")); return false; } + talloc_keep_secret(*pp_new_pwrd); #ifdef DEBUG_PASSWORD DEBUG(100,("decode_pw_buffer: new_pwrd: ")); -- 2.35.0 From a8d7b00d9d11d85472e34a552d8cf4a4a6f176a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20Filipensk=C3=BD?= Date: Mon, 8 Aug 2022 17:47:28 +0200 Subject: [PATCH 31/37] lib:replace: Add macro BURN_STR() to zero memory of a string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Pavel Filipenský Reviewed-by: Andreas Schneider (cherry picked from commit 8564380346ace981b957bb8464f2ecf007032062) [jsutton@samba.org Fixed conflict] --- lib/replace/replace.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/replace/replace.h b/lib/replace/replace.h index 8609d84322c..3c938f26240 100644 --- a/lib/replace/replace.h +++ b/lib/replace/replace.h @@ -846,6 +846,17 @@ typedef unsigned long long ptrdiff_t ; */ #define ZERO_ARRAY_LEN(x, l) memset_s((char *)(x), (l), 0, (l)) +/** + * Explicitly zero data in string. This is guaranteed to be not optimized + * away. + */ +#define BURN_STR(x) do { \ + if ((x) != NULL) { \ + size_t s = strlen(x); \ + memset_s((x), s, 0, s); \ + } \ + } while(0) + /** * Work out how many elements there are in a static array. */ -- 2.35.0 From 34159e5b0c990a602ccd631318567de63d98f71e Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 6 Sep 2022 14:54:08 +1200 Subject: [PATCH 32/37] s3:rpc_server: Use BURN_STR() to zero password This ensures these calls are not optimised away. Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit 1258746ba85b8702628f95a19aba9afea96eab8b) --- source3/rpc_server/samr/srv_samr_chgpasswd.c | 2 +- source3/rpc_server/samr/srv_samr_nt.c | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/source3/rpc_server/samr/srv_samr_chgpasswd.c b/source3/rpc_server/samr/srv_samr_chgpasswd.c index 798993bb542..a4029f6e50a 100644 --- a/source3/rpc_server/samr/srv_samr_chgpasswd.c +++ b/source3/rpc_server/samr/srv_samr_chgpasswd.c @@ -1354,7 +1354,7 @@ NTSTATUS pass_oem_change(char *user, const char *rhost, True, reject_reason); unbecome_root(); - memset(new_passwd, 0, strlen(new_passwd)); + BURN_STR(new_passwd); done: TALLOC_FREE(sampass); diff --git a/source3/rpc_server/samr/srv_samr_nt.c b/source3/rpc_server/samr/srv_samr_nt.c index 7f2c0a861f5..a84125d93c2 100644 --- a/source3/rpc_server/samr/srv_samr_nt.c +++ b/source3/rpc_server/samr/srv_samr_nt.c @@ -4945,9 +4945,7 @@ static NTSTATUS set_user_info_23(TALLOC_CTX *mem_ctx, } } - if (plaintext_buf) { - memset(plaintext_buf, '\0', strlen(plaintext_buf)); - } + BURN_STR(plaintext_buf); if (IS_SAM_CHANGED(pwd, PDB_GROUPSID) && (!NT_STATUS_IS_OK(status = pdb_set_unix_primary_group(mem_ctx, @@ -5017,7 +5015,7 @@ static bool set_user_info_pw(uint8_t *pass, const char *rhost, struct samu *pwd) } } - memset(plaintext_buf, '\0', strlen(plaintext_buf)); + BURN_STR(plaintext_buf); DEBUG(5,("set_user_info_pw: pdb_update_pwd()\n")); -- 2.35.0 From 7d260b94df9af439b6d46fe58aebeee628b30897 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 2 Aug 2022 14:39:43 +1200 Subject: [PATCH 33/37] CVE-2021-20251 s4-rpc_server: Extend scope of transaction for ChangePasswordUser3 Now the initial account search is performed under the transaction, ensuring the overall password change is atomic. We set DSDB_SESSION_INFO to drop our privileges to those of the user before we perform the actual password change, and restore them afterwards if we need to update the bad password count. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit fcabcb326d385c1e1daaa8dae9820e33a3868f56) [jsutton@samba.org Included dsdb/common/util.h header for DSDB_SESSION_INFO define] --- source4/rpc_server/samr/samr_password.c | 43 +++++++++++++++++++------ 1 file changed, 34 insertions(+), 9 deletions(-) diff --git a/source4/rpc_server/samr/samr_password.c b/source4/rpc_server/samr/samr_password.c index fa323c1a49a..cc0cf869b8d 100644 --- a/source4/rpc_server/samr/samr_password.c +++ b/source4/rpc_server/samr/samr_password.c @@ -26,6 +26,7 @@ #include "rpc_server/samr/dcesrv_samr.h" #include "system/time.h" #include "lib/crypto/md4.h" +#include "dsdb/common/util.h" #include "dsdb/samdb/samdb.h" #include "auth/auth.h" #include "libcli/auth/libcli_auth.h" @@ -350,6 +351,8 @@ NTSTATUS dcesrv_samr_ChangePasswordUser3(struct dcesrv_call_state *dce_call, = lpcfg_ntlm_auth(dce_call->conn->dce_ctx->lp_ctx); gnutls_cipher_hd_t cipher_hnd = NULL; gnutls_datum_t nt_session_key; + struct auth_session_info *call_session_info = NULL; + struct auth_session_info *old_session_info = NULL; int rc; *r->out.dominfo = NULL; @@ -374,6 +377,12 @@ NTSTATUS dcesrv_samr_ChangePasswordUser3(struct dcesrv_call_state *dce_call, return NT_STATUS_INVALID_SYSTEM_SERVICE; } + ret = ldb_transaction_start(sam_ctx); + if (ret != LDB_SUCCESS) { + DEBUG(1, ("Failed to start transaction: %s\n", ldb_errstring(sam_ctx))); + return NT_STATUS_TRANSACTION_ABORTED; + } + /* * We use authsam_search_account() to be consistent with the * other callers in the bad password and audit log handling @@ -385,6 +394,7 @@ NTSTATUS dcesrv_samr_ChangePasswordUser3(struct dcesrv_call_state *dce_call, ldb_get_default_basedn(sam_ctx), &msg); if (!NT_STATUS_IS_OK(status)) { + ldb_transaction_cancel(sam_ctx); goto failed; } @@ -395,11 +405,13 @@ NTSTATUS dcesrv_samr_ChangePasswordUser3(struct dcesrv_call_state *dce_call, status = samdb_result_passwords(mem_ctx, dce_call->conn->dce_ctx->lp_ctx, msg, &lm_pwd, &nt_pwd); if (!NT_STATUS_IS_OK(status) ) { + ldb_transaction_cancel(sam_ctx); goto failed; } if (!nt_pwd) { status = NT_STATUS_WRONG_PASSWORD; + ldb_transaction_cancel(sam_ctx); goto failed; } @@ -415,6 +427,7 @@ NTSTATUS dcesrv_samr_ChangePasswordUser3(struct dcesrv_call_state *dce_call, NULL); if (rc < 0) { status = gnutls_error_to_ntstatus(rc, NT_STATUS_CRYPTO_SYSTEM_INVALID); + ldb_transaction_cancel(sam_ctx); goto failed; } @@ -424,17 +437,20 @@ NTSTATUS dcesrv_samr_ChangePasswordUser3(struct dcesrv_call_state *dce_call, gnutls_cipher_deinit(cipher_hnd); if (rc < 0) { status = gnutls_error_to_ntstatus(rc, NT_STATUS_CRYPTO_SYSTEM_INVALID); + ldb_transaction_cancel(sam_ctx); goto failed; } if (!extract_pw_from_buffer(mem_ctx, r->in.nt_password->data, &new_password)) { DEBUG(3,("samr: failed to decode password buffer\n")); status = NT_STATUS_WRONG_PASSWORD; + ldb_transaction_cancel(sam_ctx); goto failed; } if (r->in.nt_verifier == NULL) { status = NT_STATUS_WRONG_PASSWORD; + ldb_transaction_cancel(sam_ctx); goto failed; } @@ -444,10 +460,12 @@ NTSTATUS dcesrv_samr_ChangePasswordUser3(struct dcesrv_call_state *dce_call, E_old_pw_hash(new_nt_hash, nt_pwd->hash, nt_verifier.hash); if (rc != 0) { status = gnutls_error_to_ntstatus(rc, NT_STATUS_ACCESS_DISABLED_BY_POLICY_OTHER); + ldb_transaction_cancel(sam_ctx); goto failed; } if (memcmp(nt_verifier.hash, r->in.nt_verifier->hash, 16) != 0) { status = NT_STATUS_WRONG_PASSWORD; + ldb_transaction_cancel(sam_ctx); goto failed; } @@ -476,16 +494,16 @@ NTSTATUS dcesrv_samr_ChangePasswordUser3(struct dcesrv_call_state *dce_call, } } - /* Connect to a SAMDB with user privileges for the password change */ - sam_ctx = dcesrv_samdb_connect_as_user(mem_ctx, dce_call); - if (sam_ctx == NULL) { - return NT_STATUS_INVALID_SYSTEM_SERVICE; - } + /* Drop to user privileges for the password change */ - ret = ldb_transaction_start(sam_ctx); + old_session_info = ldb_get_opaque(sam_ctx, DSDB_SESSION_INFO); + call_session_info = dcesrv_call_session_info(dce_call); + + ret = ldb_set_opaque(sam_ctx, DSDB_SESSION_INFO, call_session_info); if (ret != LDB_SUCCESS) { - DEBUG(1, ("Failed to start transaction: %s\n", ldb_errstring(sam_ctx))); - return NT_STATUS_TRANSACTION_ABORTED; + status = NT_STATUS_INVALID_SYSTEM_SERVICE; + ldb_transaction_cancel(sam_ctx); + goto failed; } /* Performs the password modification. We pass the old hashes read out @@ -499,6 +517,11 @@ NTSTATUS dcesrv_samr_ChangePasswordUser3(struct dcesrv_call_state *dce_call, &reason, &dominfo); + /* Restore our privileges to system level */ + if (old_session_info != NULL) { + ldb_set_opaque(sam_ctx, DSDB_SESSION_INFO, old_session_info); + } + if (!NT_STATUS_IS_OK(status)) { ldb_transaction_cancel(sam_ctx); goto failed; @@ -534,7 +557,9 @@ failed: /* Only update the badPwdCount if we found the user */ if (NT_STATUS_EQUAL(status, NT_STATUS_WRONG_PASSWORD)) { - NTSTATUS bad_pwd_status = authsam_update_bad_pwd_count( + NTSTATUS bad_pwd_status; + + bad_pwd_status = authsam_update_bad_pwd_count( sam_ctx, msg, ldb_get_default_basedn(sam_ctx)); if (NT_STATUS_EQUAL(bad_pwd_status, NT_STATUS_ACCOUNT_LOCKED_OUT)) { status = bad_pwd_status; -- 2.35.0 From 423d98094355031f60def9de9fedc0f8a8fc3d00 Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Tue, 26 Jul 2022 10:57:19 +0200 Subject: [PATCH 34/37] s3:rpc_server: Use a done goto label for dcesrv_samr_SetUserInfo() This will be used in the following commits. Signed-off-by: Andreas Schneider Reviewed-by: Stefan Metzmacher (cherry picked from commit a246ae993fd8553bf66aa8ee1700eb68b85f2857) --- source4/rpc_server/samr/dcesrv_samr.c | 84 +++++++++++++++++---------- 1 file changed, 54 insertions(+), 30 deletions(-) diff --git a/source4/rpc_server/samr/dcesrv_samr.c b/source4/rpc_server/samr/dcesrv_samr.c index 10c53b19e0d..ccb4e37bf31 100644 --- a/source4/rpc_server/samr/dcesrv_samr.c +++ b/source4/rpc_server/samr/dcesrv_samr.c @@ -3713,13 +3713,14 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL r->in.info->info18.lm_pwd_active ? r->in.info->info18.lm_pwd.hash : NULL, r->in.info->info18.nt_pwd_active ? r->in.info->info18.nt_pwd.hash : NULL); if (!NT_STATUS_IS_OK(status)) { - return status; + goto done; } if (r->in.info->info18.password_expired > 0) { struct ldb_message_element *set_el; if (samdb_msg_add_uint64(sam_ctx, mem_ctx, msg, "pwdLastSet", 0) != LDB_SUCCESS) { - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto done; } set_el = ldb_msg_find_element(msg, "pwdLastSet"); set_el->flags = LDB_FLAG_MOD_REPLACE; @@ -3731,8 +3732,10 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL break; case 21: - if (r->in.info->info21.fields_present == 0) - return NT_STATUS_INVALID_PARAMETER; + if (r->in.info->info21.fields_present == 0) { + status = NT_STATUS_INVALID_PARAMETER; + goto done; + } #define IFSET(bit) if (bit & r->in.info->info21.fields_present) IFSET(SAMR_FIELD_LAST_LOGON) @@ -3777,8 +3780,10 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL SET_UINT (msg, info21.code_page, "codePage"); /* password change fields */ - IFSET(SAMR_FIELD_LAST_PWD_CHANGE) - return NT_STATUS_ACCESS_DENIED; + IFSET(SAMR_FIELD_LAST_PWD_CHANGE) { + status = NT_STATUS_ACCESS_DENIED; + goto done; + } IFSET((SAMR_FIELD_LM_PASSWORD_PRESENT | SAMR_FIELD_NT_PASSWORD_PRESENT)) { @@ -3787,7 +3792,8 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL if (r->in.info->info21.lm_password_set) { if ((r->in.info->info21.lm_owf_password.length != 16) || (r->in.info->info21.lm_owf_password.size != 16)) { - return NT_STATUS_INVALID_PARAMETER; + status = NT_STATUS_INVALID_PARAMETER; + goto done; } lm_pwd_hash = (uint8_t *) r->in.info->info21.lm_owf_password.array; @@ -3795,7 +3801,8 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL if (r->in.info->info21.nt_password_set) { if ((r->in.info->info21.nt_owf_password.length != 16) || (r->in.info->info21.nt_owf_password.size != 16)) { - return NT_STATUS_INVALID_PARAMETER; + status = NT_STATUS_INVALID_PARAMETER; + goto done; } nt_pwd_hash = (uint8_t *) r->in.info->info21.nt_owf_password.array; @@ -3808,7 +3815,7 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL lm_pwd_hash, nt_pwd_hash); if (!NT_STATUS_IS_OK(status)) { - return status; + goto done; } } @@ -3821,7 +3828,8 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL t = "-1"; } if (ldb_msg_add_string(msg, "pwdLastSet", t) != LDB_SUCCESS) { - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto done; } set_el = ldb_msg_find_element(msg, "pwdLastSet"); set_el->flags = LDB_FLAG_MOD_REPLACE; @@ -3830,8 +3838,10 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL break; case 23: - if (r->in.info->info23.info.fields_present == 0) - return NT_STATUS_INVALID_PARAMETER; + if (r->in.info->info23.info.fields_present == 0) { + status = NT_STATUS_INVALID_PARAMETER; + goto done; + } #define IFSET(bit) if (bit & r->in.info->info23.info.fields_present) IFSET(SAMR_FIELD_LAST_LOGON) @@ -3877,8 +3887,10 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL SET_UINT (msg, info23.info.code_page, "codePage"); /* password change fields */ - IFSET(SAMR_FIELD_LAST_PWD_CHANGE) - return NT_STATUS_ACCESS_DENIED; + IFSET(SAMR_FIELD_LAST_PWD_CHANGE) { + status = NT_STATUS_ACCESS_DENIED; + goto done; + } IFSET(SAMR_FIELD_NT_PASSWORD_PRESENT) { status = samr_set_password(dce_call, @@ -3896,7 +3908,7 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL &r->in.info->info23.password); } if (!NT_STATUS_IS_OK(status)) { - return status; + goto done; } IFSET(SAMR_FIELD_EXPIRED_FLAG) { @@ -3907,7 +3919,8 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL t = "-1"; } if (ldb_msg_add_string(msg, "pwdLastSet", t) != LDB_SUCCESS) { - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto done; } set_el = ldb_msg_find_element(msg, "pwdLastSet"); set_el->flags = LDB_FLAG_MOD_REPLACE; @@ -3924,13 +3937,14 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL mem_ctx, &r->in.info->info24.password); if (!NT_STATUS_IS_OK(status)) { - return status; + goto done; } if (r->in.info->info24.password_expired > 0) { struct ldb_message_element *set_el; if (samdb_msg_add_uint64(sam_ctx, mem_ctx, msg, "pwdLastSet", 0) != LDB_SUCCESS) { - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto done; } set_el = ldb_msg_find_element(msg, "pwdLastSet"); set_el->flags = LDB_FLAG_MOD_REPLACE; @@ -3938,8 +3952,10 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL break; case 25: - if (r->in.info->info25.info.fields_present == 0) - return NT_STATUS_INVALID_PARAMETER; + if (r->in.info->info25.info.fields_present == 0) { + status = NT_STATUS_INVALID_PARAMETER; + goto done; + } #define IFSET(bit) if (bit & r->in.info->info25.info.fields_present) IFSET(SAMR_FIELD_LAST_LOGON) @@ -3984,8 +4000,10 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL SET_UINT (msg, info25.info.code_page, "codePage"); /* password change fields */ - IFSET(SAMR_FIELD_LAST_PWD_CHANGE) - return NT_STATUS_ACCESS_DENIED; + IFSET(SAMR_FIELD_LAST_PWD_CHANGE) { + status = NT_STATUS_ACCESS_DENIED; + goto done; + } IFSET(SAMR_FIELD_NT_PASSWORD_PRESENT) { status = samr_set_password_ex(dce_call, @@ -4003,7 +4021,7 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL &r->in.info->info25.password); } if (!NT_STATUS_IS_OK(status)) { - return status; + goto done; } IFSET(SAMR_FIELD_EXPIRED_FLAG) { @@ -4014,7 +4032,8 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL t = "-1"; } if (ldb_msg_add_string(msg, "pwdLastSet", t) != LDB_SUCCESS) { - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto done; } set_el = ldb_msg_find_element(msg, "pwdLastSet"); set_el->flags = LDB_FLAG_MOD_REPLACE; @@ -4031,7 +4050,7 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL mem_ctx, &r->in.info->info26.password); if (!NT_STATUS_IS_OK(status)) { - return status; + goto done; } if (r->in.info->info26.password_expired > 0) { @@ -4042,7 +4061,8 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL t = "-1"; } if (ldb_msg_add_string(msg, "pwdLastSet", t) != LDB_SUCCESS) { - return NT_STATUS_NO_MEMORY; + status = NT_STATUS_NO_MEMORY; + goto done; } set_el = ldb_msg_find_element(msg, "pwdLastSet"); set_el->flags = LDB_FLAG_MOD_REPLACE; @@ -4051,11 +4071,12 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL default: /* many info classes are not valid for SetUserInfo */ - return NT_STATUS_INVALID_INFO_CLASS; + status = NT_STATUS_INVALID_INFO_CLASS; + goto done; } if (!NT_STATUS_IS_OK(status)) { - return status; + goto done; } /* modify the samdb record */ @@ -4066,11 +4087,14 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL ldb_dn_get_linearized(a_state->account_dn), ldb_errstring(a_state->sam_ctx))); - return dsdb_ldb_err_to_ntstatus(ret); + status = dsdb_ldb_err_to_ntstatus(ret); + goto done; } } - return NT_STATUS_OK; + status = NT_STATUS_OK; +done: + return status; } -- 2.35.0 From 3b90e9ca2233087f5582ed3678b07347acc14682 Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Tue, 26 Jul 2022 10:59:13 +0200 Subject: [PATCH 35/37] s4:rpc_server: Use sam_ctx consistently in dcesrv_samr_SetUserInfo() Signed-off-by: Andreas Schneider Reviewed-by: Stefan Metzmacher (cherry picked from commit 1b3d7f811680f9ac66ca5822950b3eee081a06b0) --- source4/rpc_server/samr/dcesrv_samr.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/source4/rpc_server/samr/dcesrv_samr.c b/source4/rpc_server/samr/dcesrv_samr.c index ccb4e37bf31..15ca7cf039a 100644 --- a/source4/rpc_server/samr/dcesrv_samr.c +++ b/source4/rpc_server/samr/dcesrv_samr.c @@ -3706,7 +3706,7 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL case 18: status = samr_set_password_buffers(dce_call, - a_state->sam_ctx, + sam_ctx, a_state->account_dn, a_state->domain_state->domain_dn, mem_ctx, @@ -3808,7 +3808,7 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL nt_pwd_hash = (uint8_t *) r->in.info->info21.nt_owf_password.array; } status = samr_set_password_buffers(dce_call, - a_state->sam_ctx, + sam_ctx, a_state->account_dn, a_state->domain_state->domain_dn, mem_ctx, @@ -3894,14 +3894,14 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL IFSET(SAMR_FIELD_NT_PASSWORD_PRESENT) { status = samr_set_password(dce_call, - a_state->sam_ctx, + sam_ctx, a_state->account_dn, a_state->domain_state->domain_dn, mem_ctx, &r->in.info->info23.password); } else IFSET(SAMR_FIELD_LM_PASSWORD_PRESENT) { status = samr_set_password(dce_call, - a_state->sam_ctx, + sam_ctx, a_state->account_dn, a_state->domain_state->domain_dn, mem_ctx, @@ -3931,7 +3931,7 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL /* the set password levels are handled separately */ case 24: status = samr_set_password(dce_call, - a_state->sam_ctx, + sam_ctx, a_state->account_dn, a_state->domain_state->domain_dn, mem_ctx, @@ -4007,14 +4007,14 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL IFSET(SAMR_FIELD_NT_PASSWORD_PRESENT) { status = samr_set_password_ex(dce_call, - a_state->sam_ctx, + sam_ctx, a_state->account_dn, a_state->domain_state->domain_dn, mem_ctx, &r->in.info->info25.password); } else IFSET(SAMR_FIELD_LM_PASSWORD_PRESENT) { status = samr_set_password_ex(dce_call, - a_state->sam_ctx, + sam_ctx, a_state->account_dn, a_state->domain_state->domain_dn, mem_ctx, @@ -4044,7 +4044,7 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL /* the set password levels are handled separately */ case 26: status = samr_set_password_ex(dce_call, - a_state->sam_ctx, + sam_ctx, a_state->account_dn, a_state->domain_state->domain_dn, mem_ctx, @@ -4081,11 +4081,11 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL /* modify the samdb record */ if (msg->num_elements > 0) { - ret = ldb_modify(a_state->sam_ctx, msg); + ret = ldb_modify(sam_ctx, msg); if (ret != LDB_SUCCESS) { DEBUG(1,("Failed to modify record %s: %s\n", ldb_dn_get_linearized(a_state->account_dn), - ldb_errstring(a_state->sam_ctx))); + ldb_errstring(sam_ctx))); status = dsdb_ldb_err_to_ntstatus(ret); goto done; -- 2.35.0 From f0c0f57a6168ff6d5ff4198343ddbbb18cb97631 Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Tue, 26 Jul 2022 11:04:29 +0200 Subject: [PATCH 36/37] s4:rpc_server: Add transaction for dcesrv_samr_SetUserInfo() Signed-off-by: Andreas Schneider Reviewed-by: Stefan Metzmacher (cherry picked from commit 1aa403517ffc0d43df72ddc9fa2ce86ab5c33873) --- source4/rpc_server/samr/dcesrv_samr.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/source4/rpc_server/samr/dcesrv_samr.c b/source4/rpc_server/samr/dcesrv_samr.c index 15ca7cf039a..74edb32d773 100644 --- a/source4/rpc_server/samr/dcesrv_samr.c +++ b/source4/rpc_server/samr/dcesrv_samr.c @@ -3647,6 +3647,13 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL return NT_STATUS_NO_MEMORY; } + ret = ldb_transaction_start(sam_ctx); + if (ret != LDB_SUCCESS) { + DBG_ERR("Failed to start a transaction: %s\n", + ldb_errstring(sam_ctx)); + return NT_STATUS_LOCK_NOT_GRANTED; + } + switch (r->in.level) { case 2: SET_STRING(msg, info2.comment, "comment"); @@ -4092,8 +4099,21 @@ static NTSTATUS dcesrv_samr_SetUserInfo(struct dcesrv_call_state *dce_call, TALL } } + ret = ldb_transaction_commit(sam_ctx); + if (ret != LDB_SUCCESS) { + DBG_ERR("Failed to commit transaction modifying account record " + "%s: %s\n", + ldb_dn_get_linearized(msg->dn), + ldb_errstring(sam_ctx)); + return NT_STATUS_INTERNAL_DB_CORRUPTION; + } + status = NT_STATUS_OK; done: + if (!NT_STATUS_IS_OK(status)) { + ldb_transaction_cancel(sam_ctx); + } + return status; } -- 2.35.0 From 014e4521be98b9e3ffed1fc7dff87e40c8f08096 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 2 Aug 2022 14:40:01 +1200 Subject: [PATCH 37/37] CVE-2021-20251 dsdb/common: Remove transaction logic from samdb_set_password() All of its callers, where necessary, take out a transaction covering the entire password set or change operation, so a transaction is no longer needed here. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14611 Signed-off-by: Joseph Sutton Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett (cherry picked from commit 7981cba87e3a7256b12bfc5fdd89b136c12979ff) --- source4/dsdb/common/util.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c index 679daa0348b..02472f8a89c 100644 --- a/source4/dsdb/common/util.c +++ b/source4/dsdb/common/util.c @@ -2442,7 +2442,10 @@ static NTSTATUS samdb_set_password_internal(struct ldb_context *ldb, TALLOC_CTX return NT_STATUS_NO_MEMORY; } - ret = dsdb_autotransaction_request(ldb, req); + ret = ldb_request(ldb, req); + if (ret == LDB_SUCCESS) { + ret = ldb_wait(req->handle, LDB_WAIT_ALL); + } if (req->context != NULL) { struct ldb_control *control = talloc_get_type_abort(req->context, -- 2.35.0