samba.git
21 months agoCVE-2022-2031 tests/krb5: Test truncated forms of server principals
Joseph Sutton [Tue, 14 Jun 2022 03:23:55 +0000 (15:23 +1200)]
CVE-2022-2031 tests/krb5: Test truncated forms of server principals

We should not be able to use krb@REALM instead of krbtgt@REALM.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-32744 s4:kdc: Don't allow HDB keytab iteration
Joseph Sutton [Tue, 24 May 2022 05:52:05 +0000 (17:52 +1200)]
CVE-2022-32744 s4:kdc: Don't allow HDB keytab iteration

A fallback in krb5_rd_req_ctx() means that Samba's kpasswd service will
try many inappropriate keys to decrypt the ticket supplied to it. For
example, it will accept a ticket encrypted with the Administrator's key,
when it should rather accept only tickets encrypted with the krbtgt's
key (and not an RODC krbtgt). To fix this, declare the HDB keytab using
the HDBGET ops, which do not support iteration.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 s4:kdc: Reject tickets during the last two minutes of their life
Joseph Sutton [Mon, 30 May 2022 07:18:17 +0000 (19:18 +1200)]
CVE-2022-2031 s4:kdc: Reject tickets during the last two minutes of their life

For Heimdal, this now matches the behaviour of Windows. The object of
this requirement is to ensure we don't allow kpasswd tickets, not having
a lifetime of more than two minutes, to be passed off as TGTs.

An existing requirement for TGTs to contain a REQUESTER_SID PAC buffer
suffices to prevent kpasswd ticket misuse, so this is just an additional
precaution on top.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 third_party/heimdal: Add function to get current KDC time
Joseph Sutton [Wed, 22 Jun 2022 08:01:12 +0000 (20:01 +1200)]
CVE-2022-2031 third_party/heimdal: Add function to get current KDC time

This allows the plugin to check the endtime of a ticket against the
KDC's current time, to see if the ticket will expire in the next two
minutes.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-2031 s4:kdc: Limit kpasswd ticket lifetime to two minutes or less
Joseph Sutton [Tue, 24 May 2022 05:53:49 +0000 (17:53 +1200)]
CVE-2022-2031 s4:kdc: Limit kpasswd ticket lifetime to two minutes or less

This matches the behaviour of Windows.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 s4:kdc: Fix canonicalisation of kadmin/changepw principal
Joseph Sutton [Wed, 18 May 2022 04:56:01 +0000 (16:56 +1200)]
CVE-2022-2031 s4:kdc: Fix canonicalisation of kadmin/changepw principal

Since this principal goes through the samba_kdc_fetch_server() path,
setting the canonicalisation flag would cause the principal to be
replaced with the sAMAccountName; this meant requests to
kadmin/changepw@REALM would result in a ticket to krbtgt@REALM. Now we
properly handle canonicalisation for the kadmin/changepw principal.

View with 'git show -b'.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047

Pair-Programmed-With: Andreas Schneider <asn@samba.org>
Signed-off-by: Andreas Schneider <asn@samba.org>
Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 s4:kdc: Refactor samba_kdc_get_entry_principal()
Joseph Sutton [Wed, 25 May 2022 05:19:58 +0000 (17:19 +1200)]
CVE-2022-2031 s4:kdc: Refactor samba_kdc_get_entry_principal()

This eliminates some duplicate branches.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Pair-Programmed-With: Andreas Schneider <asn@samba.org>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 s4:kdc: Split out a samba_kdc_get_entry_principal() function
Joseph Sutton [Wed, 18 May 2022 04:56:01 +0000 (16:56 +1200)]
CVE-2022-2031 s4:kdc: Split out a samba_kdc_get_entry_principal() function

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 s4:kdc: Implement is_kadmin_changepw() helper function
Andreas Schneider [Tue, 24 May 2022 07:54:18 +0000 (09:54 +0200)]
CVE-2022-2031 s4:kdc: Implement is_kadmin_changepw() helper function

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047

Signed-off-by: Andreas Schneider <asn@samba.org>
Reviewed-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-2031 testprogs: Add kadmin/changepw canonicalization test with MIT kpasswd
Andreas Schneider [Thu, 19 May 2022 14:35:28 +0000 (16:35 +0200)]
CVE-2022-2031 testprogs: Add kadmin/changepw canonicalization test with MIT kpasswd

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047

Signed-off-by: Andreas Schneider <asn@samba.org>
Reviewed-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agos4:kpasswd: Restructure code for clarity
Joseph Sutton [Wed, 18 May 2022 05:11:49 +0000 (17:11 +1200)]
s4:kpasswd: Restructure code for clarity

View with 'git show -b'.

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 s4:kpasswd: Require an initial ticket
Joseph Sutton [Wed, 18 May 2022 04:52:41 +0000 (16:52 +1200)]
CVE-2022-2031 s4:kpasswd: Require an initial ticket

Ensure that for password changes the client uses an AS-REQ to get the
ticket to kpasswd, and not a TGS-REQ.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 gensec_krb5: Add helper function to check if client sent an initial...
Joseph Sutton [Wed, 18 May 2022 04:06:31 +0000 (16:06 +1200)]
CVE-2022-2031 gensec_krb5: Add helper function to check if client sent an initial ticket

This will be used in the kpasswd service to ensure that the client has
an initial ticket to kadmin/changepw, and not a service ticket.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 s4:kpasswd: Return a kpasswd error code in KRB-ERROR
Joseph Sutton [Wed, 18 May 2022 04:49:43 +0000 (16:49 +1200)]
CVE-2022-2031 s4:kpasswd: Return a kpasswd error code in KRB-ERROR

If we attempt to return an error code outside of Heimdal's allowed range
[KRB5KDC_ERR_NONE, KRB5_ERR_RCSID), it will be replaced with a GENERIC
error, and the error text will be set to the meaningless result of
krb5_get_error_message(). Avoid this by ensuring the error code is in
the correct range.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 lib:krb5_wrap: Generate valid error codes in smb_krb5_mk_error()
Joseph Sutton [Fri, 27 May 2022 07:29:34 +0000 (19:29 +1200)]
CVE-2022-2031 lib:krb5_wrap: Generate valid error codes in smb_krb5_mk_error()

The error code passed in will be an offset from ERROR_TABLE_BASE_krb5,
so we need to subtract that before creating the error. Heimdal does this
internally, so it isn't needed there.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 s4:kpasswd: Don't return AP-REP on failure
Joseph Sutton [Wed, 18 May 2022 04:48:59 +0000 (16:48 +1200)]
CVE-2022-2031 s4:kpasswd: Don't return AP-REP on failure

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 s4:kpasswd: Correctly generate error strings
Joseph Sutton [Fri, 27 May 2022 07:21:06 +0000 (19:21 +1200)]
CVE-2022-2031 s4:kpasswd: Correctly generate error strings

The error_data we create already has an explicit length, and should not
be zero-terminated, so we omit the trailing null byte. Previously,
Heimdal builds would leave a superfluous trailing null byte on error
strings, while MIT builds would omit the final character.

The two bytes added to the string's length are for the prepended error
code.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 tests/krb5: Add tests for kpasswd service
Joseph Sutton [Tue, 24 May 2022 07:59:16 +0000 (19:59 +1200)]
CVE-2022-2031 tests/krb5: Add tests for kpasswd service

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 tests/krb5: Consider kadmin/* principals as TGS for MIT KRB5 >= 1.20
Joseph Sutton [Tue, 31 May 2022 07:23:06 +0000 (19:23 +1200)]
CVE-2022-2031 tests/krb5: Consider kadmin/* principals as TGS for MIT KRB5 >= 1.20

With MIT Kerberos >= 1.20, we should not expect a ticket checksum in
tickets to principals such as kpasswd/changepw, as they are encrypted
with the krbtgt's key.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-32744 selftest: Specify Administrator kvno for Python krb5 tests
Joseph Sutton [Thu, 26 May 2022 04:35:03 +0000 (16:35 +1200)]
CVE-2022-32744 selftest: Specify Administrator kvno for Python krb5 tests

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 tests/krb5: Add kpasswd_exchange() method
Joseph Sutton [Tue, 24 May 2022 07:57:57 +0000 (19:57 +1200)]
CVE-2022-2031 tests/krb5: Add kpasswd_exchange() method

Now we can test the kpasswd service from Python.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 tests/krb5: Allow requesting a TGT to a different sname and realm
Joseph Sutton [Tue, 24 May 2022 07:34:59 +0000 (19:34 +1200)]
CVE-2022-2031 tests/krb5: Allow requesting a TGT to a different sname and realm

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agotests/krb5: Add option for creating accounts with expired passwords
Joseph Sutton [Tue, 24 May 2022 07:30:12 +0000 (19:30 +1200)]
tests/krb5: Add option for creating accounts with expired passwords

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agotests/krb5: Fix enum typo
Joseph Sutton [Tue, 24 May 2022 07:26:56 +0000 (19:26 +1200)]
tests/krb5: Fix enum typo

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 tests/krb5: Add methods to send and receive generic messages
Joseph Sutton [Tue, 24 May 2022 07:20:28 +0000 (19:20 +1200)]
CVE-2022-2031 tests/krb5: Add methods to send and receive generic messages

This allows us to send and receive kpasswd messages, while avoiding the
existing logic for encoding and decoding other Kerberos message types.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 tests/krb5: Add 'port' parameter to connect()
Joseph Sutton [Tue, 24 May 2022 07:21:37 +0000 (19:21 +1200)]
CVE-2022-2031 tests/krb5: Add 'port' parameter to connect()

This allows us to use the kpasswd port, 464.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 tests/krb5: Add methods to create ASN1 kpasswd structures
Joseph Sutton [Tue, 24 May 2022 07:17:45 +0000 (19:17 +1200)]
CVE-2022-2031 tests/krb5: Add methods to create ASN1 kpasswd structures

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 tests/krb5: Add new definitions for kpasswd
Joseph Sutton [Tue, 24 May 2022 07:13:54 +0000 (19:13 +1200)]
CVE-2022-2031 tests/krb5: Add new definitions for kpasswd

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-32744 tests/krb5: Correctly calculate salt for pre-existing accounts
Joseph Sutton [Tue, 24 May 2022 07:06:53 +0000 (19:06 +1200)]
CVE-2022-32744 tests/krb5: Correctly calculate salt for pre-existing accounts

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 tests/krb5: Split out _make_tgs_request()
Joseph Sutton [Thu, 26 May 2022 08:52:04 +0000 (20:52 +1200)]
CVE-2022-2031 tests/krb5: Split out _make_tgs_request()

This allows us to make use of it in other tests.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-32744 tests/krb5: Correctly handle specifying account kvno
Joseph Sutton [Thu, 26 May 2022 04:34:01 +0000 (16:34 +1200)]
CVE-2022-32744 tests/krb5: Correctly handle specifying account kvno

The environment variable is a string, but we expect an integer.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 s4:kpasswd: Add MIT fallback for decoding setpw structure
Joseph Sutton [Mon, 30 May 2022 07:17:41 +0000 (19:17 +1200)]
CVE-2022-2031 s4:kpasswd: Add MIT fallback for decoding setpw structure

The target principal and realm fields of the setpw structure are
supposed to be optional, but in MIT Kerberos they are mandatory. For
better compatibility and ease of testing, fall back to parsing the
simpler (containing only the new password) structure if the MIT function
fails to decode it.

Although the target principal and realm fields should be optional, one
is not supposed to specified without the other, so we don't have to deal
with the case where only one is specified.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 s4:kpasswd: Account for missing target principal
Joseph Sutton [Fri, 27 May 2022 07:17:02 +0000 (19:17 +1200)]
CVE-2022-2031 s4:kpasswd: Account for missing target principal

This field is supposed to be optional.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15049
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15074

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andreas Schneider <asn@samba.org>
21 months agoCVE-2022-2031 third_party/heimdal: Check generate_pac() return code
Joseph Sutton [Thu, 16 Jun 2022 03:32:49 +0000 (15:32 +1200)]
CVE-2022-2031 third_party/heimdal: Check generate_pac() return code

If the function fails, we should not issue a ticket missing the PAC.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15047

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32745 s4/dsdb/util: Correctly copy values into message element
Joseph Sutton [Fri, 3 Jun 2022 04:16:31 +0000 (16:16 +1200)]
CVE-2022-32745 s4/dsdb/util: Correctly copy values into message element

To use memcpy(), we need to specify the number of bytes to copy, rather
than the number of ldb_val structures.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15008

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32745 s4/dsdb/util: Don't call memcpy() with a NULL pointer
Joseph Sutton [Wed, 16 Feb 2022 22:13:38 +0000 (11:13 +1300)]
CVE-2022-32745 s4/dsdb/util: Don't call memcpy() with a NULL pointer

Doing so is undefined behaviour.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15008

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32745 s4/dsdb/util: Use correct value for loop count limit
Joseph Sutton [Wed, 16 Feb 2022 22:11:53 +0000 (11:11 +1300)]
CVE-2022-32745 s4/dsdb/util: Use correct value for loop count limit

Currently, we can crash the server by sending a large number of values
of a specific attribute (such as sAMAccountName) spread across a few
message elements. If val_count is larger than the total number of
elements, we get an access beyond the elements array.

Similarly, we can include unrelated message elements prior to the
message elements of the attribute in question, so that not all of the
attribute's values are copied into the returned elements values array.
This can cause the server to access uninitialised data, likely resulting
in a crash or unexpected behaviour.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15008

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32745 s4/dsdb/samldb: Check for empty values array
Joseph Sutton [Wed, 16 Feb 2022 04:03:10 +0000 (17:03 +1300)]
CVE-2022-32745 s4/dsdb/samldb: Check for empty values array

This avoids potentially trying to access the first element of an empty
array.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15008

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32746 ldb: Release LDB 2.6.1 ldb-2.6.1
Andrew Bartlett [Tue, 14 Jun 2022 03:43:26 +0000 (15:43 +1200)]
CVE-2022-32746 ldb: Release LDB 2.6.1

* CVE-2022-32746 Use-after-free occurring in database audit logging module (bug 15009)

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
21 months agoCVE-2022-32746 ldb: Make use of functions for appending to an ldb_message
Joseph Sutton [Mon, 21 Feb 2022 03:27:37 +0000 (16:27 +1300)]
CVE-2022-32746 ldb: Make use of functions for appending to an ldb_message

This aims to minimise usage of the error-prone pattern of searching for
a just-added message element in order to make modifications to it (and
potentially finding the wrong element).

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32746 ldb: Add functions for appending to an ldb_message
Joseph Sutton [Wed, 16 Feb 2022 03:30:03 +0000 (16:30 +1300)]
CVE-2022-32746 ldb: Add functions for appending to an ldb_message

Currently, there are many places where we use ldb_msg_add_empty() to add
an empty element to a message, and then call ldb_msg_add_value() or
similar to add values to that element. However, this performs an
unnecessary search of the message's elements to locate the new element.
Moreover, if an element with the same attribute name already exists
earlier in the message, the values will be added to that element,
instead of to the intended newly added element.

A similar pattern exists where we add values to a message, and then call
ldb_msg_find_element() to locate that message element and sets its flags
to (e.g.) LDB_FLAG_MOD_REPLACE. This also performs an unnecessary
search, and may locate the wrong message element for setting the flags.

To avoid these problems, add functions for appending a value to a
message, so that a particular value can be added to the end of a message
in a single operation.

For ADD requests, it is important that no two message elements share the
same attribute name, otherwise things will break. (Normally,
ldb_msg_normalize() is called before processing the request to help
ensure this.) Thus, we must be careful not to append an attribute to an
ADD message, unless we are sure (e.g. through ldb_msg_find_element())
that an existing element for that attribute is not present.

These functions will be used in the next commit.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32746 ldb: Ensure shallow copy modifications do not affect original message
Joseph Sutton [Tue, 15 Feb 2022 23:35:13 +0000 (12:35 +1300)]
CVE-2022-32746 ldb: Ensure shallow copy modifications do not affect original message

Using the newly added ldb flag, we can now detect when a message has
been shallow-copied so that its elements share their values with the
original message elements. Then when adding values to the copied
message, we now make a copy of the shared values array first.

This should prevent a use-after-free that occurred in LDB modules when
new values were added to a shallow copy of a message by calling
talloc_realloc() on the original values array, invalidating the 'values'
pointer in the original message element. The original values pointer can
later be used in the database audit logging module which logs database
requests, and potentially cause a crash.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32746 ldb: Add flag to mark message element values as shared
Joseph Sutton [Mon, 21 Feb 2022 03:10:32 +0000 (16:10 +1300)]
CVE-2022-32746 ldb: Add flag to mark message element values as shared

When making a shallow copy of an ldb message, mark the message elements
of the copy as sharing their values with the message elements in the
original message.

This flag value will be heeded in the next commit.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32746 s4/registry: Use LDB_FLAG_MOD_TYPE() for flags equality check
Joseph Sutton [Tue, 14 Jun 2022 09:12:39 +0000 (21:12 +1200)]
CVE-2022-32746 s4/registry: Use LDB_FLAG_MOD_TYPE() for flags equality check

Now unrelated flags will no longer affect the result.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32746 s4/dsdb/tombstone_reanimate: Use LDB_FLAG_MOD_TYPE() for flags equalit...
Joseph Sutton [Tue, 14 Jun 2022 09:11:33 +0000 (21:11 +1200)]
CVE-2022-32746 s4/dsdb/tombstone_reanimate: Use LDB_FLAG_MOD_TYPE() for flags equality check

Now unrelated flags will no longer affect the result.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32746 s4/dsdb/repl_meta_data: Use LDB_FLAG_MOD_TYPE() for flags equality...
Joseph Sutton [Tue, 14 Jun 2022 07:49:19 +0000 (19:49 +1200)]
CVE-2022-32746 s4/dsdb/repl_meta_data: Use LDB_FLAG_MOD_TYPE() for flags equality check

Now unrelated flags will no longer affect the result.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32746 ldb:rdn_name: Use LDB_FLAG_MOD_TYPE() for flags equality check
Joseph Sutton [Tue, 15 Feb 2022 23:43:52 +0000 (12:43 +1300)]
CVE-2022-32746 ldb:rdn_name: Use LDB_FLAG_MOD_TYPE() for flags equality check

Now unrelated flags will no longer affect the result.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32746 s4/dsdb/acl: Fix LDB flags comparison
Joseph Sutton [Tue, 21 Jun 2022 03:22:47 +0000 (15:22 +1200)]
CVE-2022-32746 s4/dsdb/acl: Fix LDB flags comparison

LDB_FLAG_MOD_* values are not actually flags, and the previous
comparison was equivalent to

(el->flags & LDB_FLAG_MOD_MASK) == 0

which is only true if none of the LDB_FLAG_MOD_* values are set, so we
would not successfully return if the element was a DELETE. Correct the
expression to what it was intended to be.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32746 s4:torture: Fix LDB flags comparison
Joseph Sutton [Tue, 21 Jun 2022 02:49:51 +0000 (14:49 +1200)]
CVE-2022-32746 s4:torture: Fix LDB flags comparison

LDB_FLAG_MOD_* values are not actually flags, and the previous
comparison was equivalent to

(el->flags & LDB_FLAG_MOD_MASK) == 0

which is only true if none of the LDB_FLAG_MOD_* values are set. Correct
the expression to what it was probably intended to be.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32746 s4/dsdb/partition: Fix LDB flags comparison
Joseph Sutton [Tue, 21 Jun 2022 02:41:02 +0000 (14:41 +1200)]
CVE-2022-32746 s4/dsdb/partition: Fix LDB flags comparison

LDB_FLAG_MOD_* values are not actually flags, and the previous
comparison was equivalent to

(req_msg->elements[el_idx].flags & LDB_FLAG_MOD_MASK) != 0

which is true whenever any of the LDB_FLAG_MOD_* values are set. Correct
the expression to what it was probably intended to be.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32746 s4:dsdb:tests: Add test for deleting a disallowed SPN
Joseph Sutton [Tue, 21 Jun 2022 03:37:15 +0000 (15:37 +1200)]
CVE-2022-32746 s4:dsdb:tests: Add test for deleting a disallowed SPN

If an account has an SPN that requires Write Property to set, we should
still be able to delete it with just Validated Write.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agoCVE-2022-32746 s4/dsdb/objectclass_attrs: Fix typo
Joseph Sutton [Tue, 14 Jun 2022 09:09:53 +0000 (21:09 +1200)]
CVE-2022-32746 s4/dsdb/objectclass_attrs: Fix typo

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
21 months agos3:dbwrap_watch: call dbwrap_watched_trigger_wakeup() outside of the low level record...
Stefan Metzmacher [Thu, 30 Jun 2022 10:39:18 +0000 (10:39 +0000)]
s3:dbwrap_watch: call dbwrap_watched_trigger_wakeup() outside of the low level record lock

This gives a nice speed up, as it's unlikely for the waiters to hit
contention.

The following test with 256 commections all looping with open/close
on the same inode (share root) is improved drastically:

  smbtorture //127.0.0.1/m -Uroot%test smb2.create.bench-path-contention-shared \
     --option='torture:bench_path=' \
     --option="torture:timelimit=60" \
     --option="torture:nprocs=256"

From some like this:

   open[num/s=8800,avslat=0.021445,minlat=0.000095,maxlat=0.179786]
   close[num/s=8800,avslat=0.021658,minlat=0.000044,maxlat=0.179819]

to:

   open[num/s=10223,avslat=0.017922,minlat=0.000083,maxlat=0.106759]
   close[num/s=10223,avslat=0.017694,minlat=0.000040,maxlat=0.107345]

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
Autobuild-User(master): Ralph Böhme <slow@samba.org>
Autobuild-Date(master): Tue Jul 26 14:32:35 UTC 2022 on sn-devel-184

21 months agos3:dbwrap_watch: only notify the first waiter
Stefan Metzmacher [Sun, 26 Jun 2022 12:57:06 +0000 (12:57 +0000)]
s3:dbwrap_watch: only notify the first waiter

In case of a highly contended record we will have a lot of watchers,
which will all race to get g_lock_lock() to finish.

If g_lock_unlock() wakes them all, e.g. 250 of them, we get a thundering
herd, were 249 will only find that one of them as able to get the lock
and re-add their watcher entry (not unlikely in a different order).

With this commit we only wake the first watcher and let it remove
itself once it no longer wants to monitor the record content
(at that time it will wake the new first watcher).

It means the woken watcher doesn't have to race with all others
and also means order of watchers is kept, which means that we
most likely get a fair latency distribution for all watchers.

The following test with 256 commections all looping with open/close
on the same inode (share root) is improved drastically:

  smbtorture //127.0.0.1/m -Uroot%test smb2.create.bench-path-contention-shared \
     --option='torture:bench_path=' \
     --option="torture:timelimit=60" \
     --option="torture:nprocs=256"

From some like this:

   open[num/s=80,avslat=2.793862,minlat=0.004097,maxlat=46.597053]
   close[num/s=80,avslat=2.387326,minlat=0.023875,maxlat=50.878165]

to:

   open[num/s=8800,avslat=0.021445,minlat=0.000095,maxlat=0.179786]
   close[num/s=8800,avslat=0.021658,minlat=0.000044,maxlat=0.179819]

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:smbXsrv_session: only change the dbwrap_watch instance when the record has changed
Stefan Metzmacher [Tue, 5 Jul 2022 14:05:15 +0000 (16:05 +0200)]
s3:smbXsrv_session: only change the dbwrap_watch instance when the record has changed

This will become important in the following commits when the
dbwrap_watch layer will only wake up one watcher at a time
and each woken watcher will wakeup the next one.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:smbXsrv_session: introduce smb2srv_session_close_previous_cleanup()
Stefan Metzmacher [Mon, 25 Jul 2022 20:28:27 +0000 (22:28 +0200)]
s3:smbXsrv_session: introduce smb2srv_session_close_previous_cleanup()

This makes sure we cleanup the locked record in all cases.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:smbXsrv_client: only change the dbwrap_watch instance when the record has changed
Stefan Metzmacher [Tue, 5 Jul 2022 14:04:09 +0000 (16:04 +0200)]
s3:smbXsrv_client: only change the dbwrap_watch instance when the record has changed

This will become important in the following commits when the
dbwrap_watch layer will only wake up one watcher at a time
and each woken watcher will wakeup the next one.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:g_lock: try to keep the watch instance during g_lock_watch_data()
Stefan Metzmacher [Sun, 26 Jun 2022 16:16:38 +0000 (16:16 +0000)]
s3:g_lock: try to keep the watch instance during g_lock_watch_data()

Unless the unique_lock_epoch changes via g_lock_lock()/g_lock_unlock()
we try to keep our existing watch instance alive while waiting
for unique_data_epoch to change.

This will become important in the following commits when the
dbwrap_watch layer will only wake up one watcher at a time
and each woken watcher will wakeup the next one. Without this
commit we would trigger an endless loop as none of the watchers
will ever change unique_data_epoch.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:g_lock: remember an unique_lock_epoch similar to unique_data_epoch
Stefan Metzmacher [Sun, 26 Jun 2022 16:16:38 +0000 (16:16 +0000)]
s3:g_lock: remember an unique_lock_epoch similar to unique_data_epoch

It changes with every lock and unlock.

This will be needed in future in order to differentiate between
lock and data changed.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:g_lock: avoid a lot of unused overhead using the new dbwrap_watch features
Stefan Metzmacher [Mon, 27 Jun 2022 13:40:55 +0000 (13:40 +0000)]
s3:g_lock: avoid a lot of unused overhead using the new dbwrap_watch features

The key points are:

1. We keep our position in the watcher queue until we got what
   we were waiting for. It means the order is now fair and stable.

2. We only wake up other during g_lock_unlock() and only if
   we detect that an pending exclusive lock is able to make progress.
   (Note: read lock holders are never waiters on their own)

This reduced the contention on locking.tdb records drastically,
as waiters are no longer woken 3 times (where the first 2 times were completely useless).

The following test with 256 commections all looping with open/close
on the same inode (share root) is improved drastically:

  smbtorture //127.0.0.1/m -Uroot%test smb2.create.bench-path-contention-shared \
     --option='torture:bench_path=' \
     --option="torture:timelimit=60" \
     --option="torture:nprocs=256"

From some like this:

   open[num/s=50,avslat=6.455775,minlat=0.000157,maxlat=55.683846]
   close[num/s=50,avslat=4.563605,minlat=0.000128,maxlat=53.585839]

to:

   open[num/s=80,avslat=2.793862,minlat=0.004097,maxlat=46.597053]
   close[num/s=80,avslat=2.387326,minlat=0.023875,maxlat=50.878165]

Note the real effect of this commit will releaved together
with a following commit that only wakes one waiter at a time.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:g_lock: always call g_lock_cleanup_shared() before getting stuck on lck.num_shared...
Stefan Metzmacher [Thu, 30 Jun 2022 14:42:54 +0000 (16:42 +0200)]
s3:g_lock: always call g_lock_cleanup_shared() before getting stuck on lck.num_shared != 0

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:g_lock: avoid calling g_lock_store() from g_lock_cleanup_dead()
Stefan Metzmacher [Mon, 27 Jun 2022 13:39:18 +0000 (13:39 +0000)]
s3:g_lock: avoid calling g_lock_store() from g_lock_cleanup_dead()

This matches the behavior of g_lock_cleanup_shared(), which also
only operates on the in memory struct g_lock.

We do a g_lock_store() later during g_lock_trylock() anyway
when we make any progress.

In the case we where a pending exclusive lock holder
we now force a g_lock_store() if g_lock_cleanup_dead()
removed the dead blocker.

This will be useful for the following changes...

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: allow callers of dbwrap_watched_watch_send/recv() to manage the...
Stefan Metzmacher [Thu, 30 Jun 2022 13:53:47 +0000 (15:53 +0200)]
s3:dbwrap_watch: allow callers of dbwrap_watched_watch_send/recv() to manage the watcher instances

The destructor triggered by dbwrap_watched_watch_recv() will
remove the watcher instance via a dedicated dbwrap_do_locked(),
just calling dbwrap_watched_watch_remove_instance() inside.

But the typical caller triggers a dbwrap_do_locked() again after
dbwrap_watched_watch_recv() returned. Which means we call
dbwrap_do_locked() twice.

We now allow dbwrap_watched_watch_recv() to return the existing
instance id (if it still exists) and removes the destructor.
That way the caller can pass the given instance id to
dbwrap_watched_watch_remove_instance() from within its own dbwrap_do_locked(),
when it decides to leave the queue, because it's happy with the new
state of the record. In order to get the best performance
dbwrap_watched_watch_remove_instance() should be called before any
dbwrap_record_storev() or dbwrap_record_delete(),
because that will only trigger a single low level storev/delete.

If the caller found out that the state of the record doesn't meet the
expectations and the callers wants to continue watching the
record (from its current position, most likely the first one),
dbwrap_watched_watch_remove_instance() can be skipped and the
instance id can be passed to dbwrap_watched_watch_send() again,
in order to resume waiting on the existing instance.
Currently the watcher instance were always removed (most likely from
the first position) and re-added (to the last position), which may
cause unfair latencies.

In order to improve the overhead of adding a new watcher instance
the caller can call dbwrap_watched_watch_add_instance() before
any dbwrap_record_storev() or dbwrap_record_delete(), which
will only result in a single low level storev/delete.
The returned instance id is then passed to dbwrap_watched_watch_send(),
within the same dbwrap_do_locked() run.

It also adds a way to avoid alerting any callers during
the current dbwrap_do_locked() run.

Layers above may only want to wake up watchers
during specific situations and while it's useless to wake
others in other situations.

This will soon be used to add more fairness to the g_lock code.

Note that this commit only prepares the api for the above to be useful,
the instance returned by dbwrap_watched_watch_recv() is most likely 0,
which means the watcher entry was already removed, but that will change
in the following commits.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: remove a watcher via db_watched_record_fini()
Stefan Metzmacher [Sun, 26 Jun 2022 12:57:06 +0000 (12:57 +0000)]
s3:dbwrap_watch: remove a watcher via db_watched_record_fini()

The new dbwrap_watched_watch_remove_instance() will just remove ourself
from the in memory array and let db_watched_record_fini() call
dbwrap_watched_record_storev() in order to write the modified version
into the low level backend record.

For now there's no change in behavior, but it allows us to change it
soon....

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: use dbwrap_watched_record_storev() to add a new watcher
Stefan Metzmacher [Sun, 26 Jun 2022 12:57:06 +0000 (12:57 +0000)]
s3:dbwrap_watch: use dbwrap_watched_record_storev() to add a new watcher

It means we only have one code path storing the low level record
and that's dbwrap_watched_record_storev on the main record.

It avoids the nested dbwrap_do_locked() and only uses
dbwrap_parse_record() and talloc_memdup() when needed.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: let dbwrap_watched_delete() call dbwrap_watched_record_storev(num_db...
Stefan Metzmacher [Sun, 26 Jun 2022 12:57:06 +0000 (12:57 +0000)]
s3:dbwrap_watch: let dbwrap_watched_delete() call dbwrap_watched_record_storev(num_dbufs=0)

dbwrap_watched_record_storev() will handle the high level storev and
delete, it will find out if we can remove the record as there's no value
and also no watchers to be stored.

This is no real change for now as dbwrap_watched_record_wakeup() will
always exits with wrec->watchers.count = 0, but that will change in the next
commits.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: filter out records with empty payload during traverse
Stefan Metzmacher [Mon, 25 Jul 2022 20:19:13 +0000 (22:19 +0200)]
s3:dbwrap_watch: filter out records with empty payload during traverse

We will soon have records with just a number of watchers, but without
payload. These records should not be visible during traverse.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: prepare dbwrap_watched_record_storev() to store watchers if requested
Stefan Metzmacher [Sun, 26 Jun 2022 12:57:06 +0000 (12:57 +0000)]
s3:dbwrap_watch: prepare dbwrap_watched_record_storev() to store watchers if requested

It will also delete the low level record in case there are no watchers
should be stored and no data buffers are given.

This is no real change for now as dbwrap_watched_record_wakeup() will
always exit with wrec->watchers.count = 0, but that will change in the next
commits.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: define/use DBWRAP_MAX_WATCHERS
Stefan Metzmacher [Sun, 26 Jun 2022 12:57:06 +0000 (12:57 +0000)]
s3:dbwrap_watch: define/use DBWRAP_MAX_WATCHERS

dbwrap backends are unlikely to be able to store
UINT32_MAX*DBWRAP_WATCHER_BUF_LENGTH in a single record
and most likely also not with the whole database!

DBWRAP_MAX_WATCHERS = INT32_MAX/DBWRAP_WATCHER_BUF_LENGTH should be
enough and makes further changes easier as we don't need to care
about size_t overflows.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: remove unused dbwrap_watched_do_locked_state.status
Stefan Metzmacher [Sun, 26 Jun 2022 12:57:06 +0000 (12:57 +0000)]
s3:dbwrap_watch: remove unused dbwrap_watched_do_locked_state.status

This is never set...

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: let dbwrap_watched_watch_recv() use tevent_req_received()
Stefan Metzmacher [Sun, 26 Jun 2022 12:57:06 +0000 (12:57 +0000)]
s3:dbwrap_watch: let dbwrap_watched_watch_recv() use tevent_req_received()

At the end of the dbwrap_watched_watch_recv() all temporary state should
be destroyed. It also means dbwrap_watched_watch_state_destructor() was
triggered.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: don't use talloc_tos() for messaging_filtered_read_recv()
Stefan Metzmacher [Sun, 26 Jun 2022 12:57:06 +0000 (12:57 +0000)]
s3:dbwrap_watch: don't use talloc_tos() for messaging_filtered_read_recv()

Async function always have their 'state' context for temporary memory.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: move db_record and db_watched_record to dbwrap_watched_do_locked()
Stefan Metzmacher [Thu, 30 Jun 2022 17:30:39 +0000 (19:30 +0200)]
s3:dbwrap_watch: move db_record and db_watched_record to dbwrap_watched_do_locked()

This will help in the next commits.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: split out a dbwrap_watched_watch_add_instance() helper
Stefan Metzmacher [Thu, 30 Jun 2022 12:05:43 +0000 (14:05 +0200)]
s3:dbwrap_watch: split out a dbwrap_watched_watch_add_instance() helper

This will be used in other places soon.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: remove dbwrap_watched_record_wakeup_fn() indirection
Stefan Metzmacher [Fri, 24 Jun 2022 15:48:54 +0000 (15:48 +0000)]
s3:dbwrap_watch: remove dbwrap_watched_record_wakeup_fn() indirection

This reduces quite some complexity and will make further changes
(which will follow soon) easier.

Review with git show --patience

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: also the fetch_locked case only needs to wake waiters just once
Stefan Metzmacher [Fri, 24 Jun 2022 15:33:30 +0000 (15:33 +0000)]
s3:dbwrap_watch: also the fetch_locked case only needs to wake waiters just once

This is no change in behavior, because:

- The first dbwrap_do_locked(dbwrap_watched_record_wakeup_fn), is
  called at the start of dbwrap_watched_record_{storev,delete}().
  That means the nested dbwrap_do_locked() will pass the
  exact value same (unchanged) value to dbwrap_watched_record_wakeup_fn.

- After the first change we have either removed the whole backend
  record in dbwrap_watched_record_delete or dbwrap_watched_record_storev()
  removed all watchers and store num_watchers = 0.

- With that any further updates will have no watchers in the backend
  record, so dbwrap_do_locked(dbwrap_watched_record_wakeup_fn) will
  never do anything useful. It only burns cpu time any may cause memory
  fragmentation.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: split out db_watched_record_fini() from db_watched_record_destructor()
Stefan Metzmacher [Sun, 26 Jun 2022 08:58:21 +0000 (10:58 +0200)]
s3:dbwrap_watch: split out db_watched_record_fini() from db_watched_record_destructor()

That makes it easier to understand that db_watched_record_init() and
db_watched_record_fini() wrap any caller activity on the record,
either during do_locked or between fetch_locked and the related
destructor.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: split out a db_watched_record_init() helper function
Stefan Metzmacher [Fri, 24 Jun 2022 15:07:43 +0000 (15:07 +0000)]
s3:dbwrap_watch: split out a db_watched_record_init() helper function

The code to construct a struct db_watched_record is mostly common
between dbwrap_watched_fetch_locked() and dbwrap_watched_do_locked_fn().

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: remove unused dbwrap_watched_do_locked_{storev,delete}()
Stefan Metzmacher [Fri, 24 Jun 2022 14:45:58 +0000 (14:45 +0000)]
s3:dbwrap_watch: remove unused dbwrap_watched_do_locked_{storev,delete}()

dbwrap_watched_do_locked_{storev,delete}() was now exactly the
same as dbwrap_watched_{storev,delete}().

We only need to know if dbwrap_watched_record_wakeup() is called from
within dbwrap_watched_do_locked_fn().

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: move the do_locked optimization to dbwrap_watched_record_wakeup()
Stefan Metzmacher [Fri, 24 Jun 2022 14:38:50 +0000 (14:38 +0000)]
s3:dbwrap_watch: move the do_locked optimization to dbwrap_watched_record_wakeup()

Both dbwrap_watched_record_storev() and dbwrap_watched_record_delete()
call dbwrap_watched_record_wakeup() as their first action.

So the behavior stays the same, but dbwrap_watched_do_locked_storev()
and dbwrap_watched_do_locked_delete() are not trivial and we
have the wakeup logic isolated in dbwrap_watched_record_wakeup() only.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: add db_record_get_watched_record() helper
Stefan Metzmacher [Fri, 24 Jun 2022 13:41:12 +0000 (13:41 +0000)]
s3:dbwrap_watch: add db_record_get_watched_record() helper

This allows safe casting off rec->private_data to get
struct db_watched_record. And that works fetch_locked and do_locked

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: use backend.{rec,initial_value} instead of subrec[_value]
Stefan Metzmacher [Fri, 24 Jun 2022 11:16:37 +0000 (11:16 +0000)]
s3:dbwrap_watch: use backend.{rec,initial_value} instead of subrec[_value]

This makes it much clearer to me what it actually is.

Keeping the initial_value with struct db_watched_record will also
simplify further changes.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: only pass struct db_watched_record to dbwrap_watched_record_*()...
Stefan Metzmacher [Fri, 24 Jun 2022 11:05:40 +0000 (11:05 +0000)]
s3:dbwrap_watch: only pass struct db_watched_record to dbwrap_watched_record_*() functions

We get to the main 'struct db_record' via wrec->rec where needed.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: use dbwrap_record_get_key() to access the key
Stefan Metzmacher [Fri, 24 Jun 2022 11:59:21 +0000 (11:59 +0000)]
s3:dbwrap_watch: use dbwrap_record_get_key() to access the key

We should avoid doing shortcuts if not needed.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: move 'wrec' from dbwrap_watched_do_locked_state to dbwrap_watched_do...
Stefan Metzmacher [Fri, 24 Jun 2022 13:00:06 +0000 (13:00 +0000)]
s3:dbwrap_watch: move 'wrec' from dbwrap_watched_do_locked_state to dbwrap_watched_do_locked_fn

We can use a local variable in dbwrap_watched_do_locked_fn.
As 'wrec' should have the same lifetime as 'rec'.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: use struct db_watched_record as rec->private_data for do_locked too
Stefan Metzmacher [Fri, 24 Jun 2022 12:54:40 +0000 (12:54 +0000)]
s3:dbwrap_watch: use struct db_watched_record as rec->private_data for do_locked too

There's no real reason to pass struct dbwrap_watched_do_locked_state
anymore. The only difference is that we can't use
talloc_get_type_abort().

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: use dbwrap_record_get_db(rec) instead of state->db
Stefan Metzmacher [Fri, 24 Jun 2022 12:51:49 +0000 (12:51 +0000)]
s3:dbwrap_watch: use dbwrap_record_get_db(rec) instead of state->db

We should try to avoid using dbwrap_watched_do_locked_state in low
level code.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: move wakeup_value to struct db_watched_record
Stefan Metzmacher [Fri, 24 Jun 2022 12:49:36 +0000 (12:49 +0000)]
s3:dbwrap_watch: move wakeup_value to struct db_watched_record

For the do_locked case they have the same scope, but having
it on db_watched_record will simplify further changes.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: rename struct dbwrap_watched_record variables to 'wrec'
Stefan Metzmacher [Fri, 24 Jun 2022 10:23:21 +0000 (10:23 +0000)]
s3:dbwrap_watch: rename struct dbwrap_watched_record variables to 'wrec'

This makes it much easier to understand...

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: s/dbwrap_watched_subrec/dbwrap_watched_record
Stefan Metzmacher [Fri, 24 Jun 2022 10:23:21 +0000 (10:23 +0000)]
s3:dbwrap_watch: s/dbwrap_watched_subrec/dbwrap_watched_record

These functions operate on struct db_watched_record.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: s/db_watched_subrec/db_watched_record
Stefan Metzmacher [Fri, 24 Jun 2022 10:23:21 +0000 (10:23 +0000)]
s3:dbwrap_watch: s/db_watched_subrec/db_watched_record

struct db_watched_record is the private data of
the struct db_record produced by the struct db_context that
uses struct db_watched_ctx.

db_watched_subrec had nothing really todo with the
sub record we got back from db_watched_ctx->backend.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: use value_valid = false during dbwrap_watched_do_locked_fn()
Stefan Metzmacher [Fri, 24 Jun 2022 09:57:05 +0000 (09:57 +0000)]
s3:dbwrap_watch: use value_valid = false during dbwrap_watched_do_locked_fn()

This matches db_tdb_do_locked() and the fetch_locked based fallback in
dbwrap_do_locked().

Calling dbwrap_record_get_value() is not allowed from within
dbwrap_do_locked()!

Now that rec.value is only internal, use it to remember the initial
payload value. This will simplify further code changes as it
makes the fetch_locked case.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agos3:dbwrap_watch: let dbwrap_watched_watch_state_destructor() use DBG_WARNING()
Stefan Metzmacher [Sun, 26 Jun 2022 12:57:06 +0000 (12:57 +0000)]
s3:dbwrap_watch: let dbwrap_watched_watch_state_destructor() use DBG_WARNING()

When we (need) to ignore an error from dbwrap_do_locked() within
dbwrap_watched_watch_state_destructor(), we better print this
with log level 1 instead of 10.

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
21 months agosmbd: split out smbd_check_access_rights_fname and call it before SMB_VFS_FGET_NT_ACL
Stefan Metzmacher [Wed, 13 Jul 2022 11:15:39 +0000 (11:15 +0000)]
smbd: split out smbd_check_access_rights_fname and call it before SMB_VFS_FGET_NT_ACL

commit 8e3798dd22276bc1ac8e96004d0e5e974240a7b9 actually came with a
change in behavior..., as SMB_VFS_GET_NT_ACL_AT() (at the time) and
now SMB_VFS_FGET_NT_ACL() is always called even if it's not needed.
E.g. access by root.

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Autobuild-User(master): Jeremy Allison <jra@samba.org>
Autobuild-Date(master): Tue Jul 26 00:31:29 UTC 2022 on sn-devel-184

21 months agos3:profile: make use of tevent_cached_getpid() in performance critical code
Stefan Metzmacher [Mon, 25 Jul 2022 12:29:35 +0000 (14:29 +0200)]
s3:profile: make use of tevent_cached_getpid() in performance critical code

This avoids wasting getpid() during profiling.

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
Autobuild-User(master): Jeremy Allison <jra@samba.org>
Autobuild-Date(master): Mon Jul 25 18:32:18 UTC 2022 on sn-devel-184

21 months agolib/util: make use of tevent_cached_getpid() in performance critical code
Stefan Metzmacher [Mon, 25 Jul 2022 12:29:35 +0000 (14:29 +0200)]
lib/util: make use of tevent_cached_getpid() in performance critical code

This avoids wasting getpid() calls in a lot of places...

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
21 months agos3:lib/messages*: s/getpid/tevent_cached_getpid
Stefan Metzmacher [Mon, 25 Jul 2022 12:29:35 +0000 (14:29 +0200)]
s3:lib/messages*: s/getpid/tevent_cached_getpid

Our messaging code is very performance critical and
we should note waste time in getpid() syscalls...

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
21 months agolib/messaging: s/getpid/tevent_cached_getpid
Stefan Metzmacher [Mon, 25 Jul 2022 12:29:35 +0000 (14:29 +0200)]
lib/messaging: s/getpid/tevent_cached_getpid

Our messaging code is very performance critical and
we should note waste time in getpid() syscalls...

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
21 months agotevent: version 0.13.0 tevent-0.13.0
Stefan Metzmacher [Mon, 25 Jul 2022 12:16:43 +0000 (14:16 +0200)]
tevent: version 0.13.0

- add tevent_cached_getpid()

Note the changes to ABI/tevent-0.12.1.sigs only
revert the temporary changes made there...

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>
21 months agotevent: tevent_cached_getpid() tests
Stefan Metzmacher [Mon, 25 Jul 2022 12:13:34 +0000 (14:13 +0200)]
tevent: tevent_cached_getpid() tests

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>