From ebdb9135bcf19056484e89e5d7551f436883383d Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Wed, 16 Feb 2022 17:03:10 +1300 Subject: [PATCH 1/4] 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 --- source4/dsdb/samdb/ldb_modules/samldb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/samldb.c b/source4/dsdb/samdb/ldb_modules/samldb.c index 24971d521aa..116c3ec1f00 100644 --- a/source4/dsdb/samdb/ldb_modules/samldb.c +++ b/source4/dsdb/samdb/ldb_modules/samldb.c @@ -751,7 +751,7 @@ static int samldb_schema_add_handle_linkid(struct samldb_ctx *ac) return ret; } - if (el == NULL) { + if (el == NULL || el->num_values == 0) { return LDB_SUCCESS; } @@ -919,7 +919,7 @@ static int samldb_schema_add_handle_mapiid(struct samldb_ctx *ac) return ret; } - if (el == NULL) { + if (el == NULL || el->num_values == 0) { return LDB_SUCCESS; } -- 2.35.0 From ad5099332f959f28b34dabc9205e75680016e6e4 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Thu, 17 Feb 2022 11:11:53 +1300 Subject: [PATCH 2/4] 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 --- source4/dsdb/samdb/ldb_modules/util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source4/dsdb/samdb/ldb_modules/util.c b/source4/dsdb/samdb/ldb_modules/util.c index 405febf0b3d..14947746837 100644 --- a/source4/dsdb/samdb/ldb_modules/util.c +++ b/source4/dsdb/samdb/ldb_modules/util.c @@ -1546,7 +1546,7 @@ int dsdb_get_expected_new_values(TALLOC_CTX *mem_ctx, v = _el->values; - for (i = 0; i < val_count; i++) { + for (i = 0; i < msg->num_elements; i++) { if (ldb_attr_cmp(msg->elements[i].name, attr_name) == 0) { if ((operation == LDB_MODIFY) && (LDB_FLAG_MOD_TYPE(msg->elements[i].flags) -- 2.35.0 From 2b1a31460f1cae4772f5622c137c6f51f3778cc8 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Thu, 17 Feb 2022 11:13:38 +1300 Subject: [PATCH 3/4] 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 --- source4/dsdb/samdb/ldb_modules/util.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/util.c b/source4/dsdb/samdb/ldb_modules/util.c index 14947746837..35ae110b5ef 100644 --- a/source4/dsdb/samdb/ldb_modules/util.c +++ b/source4/dsdb/samdb/ldb_modules/util.c @@ -1548,15 +1548,19 @@ int dsdb_get_expected_new_values(TALLOC_CTX *mem_ctx, for (i = 0; i < msg->num_elements; i++) { if (ldb_attr_cmp(msg->elements[i].name, attr_name) == 0) { + const struct ldb_message_element *tmp_el = &msg->elements[i]; if ((operation == LDB_MODIFY) && - (LDB_FLAG_MOD_TYPE(msg->elements[i].flags) + (LDB_FLAG_MOD_TYPE(tmp_el->flags) == LDB_FLAG_MOD_DELETE)) { continue; } + if (tmp_el->values == NULL || tmp_el->num_values == 0) { + continue; + } memcpy(v, - msg->elements[i].values, - msg->elements[i].num_values); - v += msg->elements[i].num_values; + tmp_el->values, + tmp_el->num_values); + v += tmp_el->num_values; } } -- 2.35.0 From 821ee9a90d5acaca2c785aa790c8ade88a7267c4 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Fri, 3 Jun 2022 16:16:31 +1200 Subject: [PATCH 4/4] 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. Signed-off-by: Joseph Sutton --- source4/dsdb/samdb/ldb_modules/util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source4/dsdb/samdb/ldb_modules/util.c b/source4/dsdb/samdb/ldb_modules/util.c index 35ae110b5ef..e7fe8f855df 100644 --- a/source4/dsdb/samdb/ldb_modules/util.c +++ b/source4/dsdb/samdb/ldb_modules/util.c @@ -1559,7 +1559,7 @@ int dsdb_get_expected_new_values(TALLOC_CTX *mem_ctx, } memcpy(v, tmp_el->values, - tmp_el->num_values); + tmp_el->num_values * sizeof(*v)); v += tmp_el->num_values; } } -- 2.35.0