[SCM] Samba Shared Repository - branch master updated
Andrew Bartlett
abartlet at samba.org
Wed Mar 10 09:52:02 UTC 2021
The branch, master has been updated
via bb17b4e1bbd ldb: dn tests use cmocka print functions
via fa933399780 ldb_match: remove redundant check
via 33a95a1e75b ldb: add tests for ldb_wildcard_compare
via cc098f1cad0 ldb_match: trailing chunk must match end of string
from d7e620ff41d lib/util: Replace buggy string_sub_talloc() with talloc_string_sub() in lib/util
https://git.samba.org/?p=samba.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit bb17b4e1bbd1f03445bb3ef8cfd5f33d5e49bccc
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Fri Mar 5 15:49:56 2021 +1300
ldb: dn tests use cmocka print functions
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14044
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Björn Jacke <bjacke at samba.org>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
Autobuild-User(master): Andrew Bartlett <abartlet at samba.org>
Autobuild-Date(master): Wed Mar 10 09:51:25 UTC 2021 on sn-devel-184
commit fa93339978040eab52b2722c1716028b48d8d084
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed Mar 3 19:54:37 2021 +1300
ldb_match: remove redundant check
We already ensure the no-trailing-asterisk case ends at the end of the
string.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14044
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Björn Jacke <bjacke at samba.org>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
commit 33a95a1e75b85e9795c4490b78ead2162e2a1f47
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Fri Mar 5 15:47:56 2021 +1300
ldb: add tests for ldb_wildcard_compare
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14044
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Björn Jacke <bjacke at samba.org>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
commit cc098f1cad04b2cfec4ddd6b2511cd5a600f31c6
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed Mar 3 19:17:36 2021 +1300
ldb_match: trailing chunk must match end of string
A wildcard search is divided into chunks by the asterisks. While most
chunks match the first suitable string, the last chunk matches the
last possible string (unless there is a trailing asterisk, in which
case this distinction is moot).
We always knew this in our hearts, but we tried to do it in a funny
complicated way that stepped through the string, comparing here and
there, leading to CVE-2019-3824 and missed matches (bug 14044).
With this patch, we just jump to the end of the string and compare it.
As well as being correct, this should also improve performance, as the
previous algorithm involved a quadratic loop of erroneous memmem()s.
See https://tools.ietf.org/html/rfc4517
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14044
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Björn Jacke <bjacke at samba.org>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
-----------------------------------------------------------------------
Summary of changes:
lib/ldb/common/ldb_match.c | 82 +++++++++++--------------
lib/ldb/tests/ldb_match_test.c | 134 ++++++++++++++++++++++++++++++++++++++---
lib/ldb/tests/test_ldb_dn.c | 5 +-
3 files changed, 161 insertions(+), 60 deletions(-)
Changeset truncated at 500 lines:
diff --git a/lib/ldb/common/ldb_match.c b/lib/ldb/common/ldb_match.c
index 829afa77e71..2f4d41f3441 100644
--- a/lib/ldb/common/ldb_match.c
+++ b/lib/ldb/common/ldb_match.c
@@ -295,8 +295,9 @@ static int ldb_wildcard_compare(struct ldb_context *ldb,
uint8_t *p;
chunk = tree->u.substring.chunks[c];
- if(a->syntax->canonicalise_fn(ldb, ldb, chunk, &cnk) != 0) goto mismatch;
-
+ if(a->syntax->canonicalise_fn(ldb, ldb, chunk, &cnk) != 0) {
+ goto mismatch;
+ }
/*
* Empty strings are returned as length 0. Ensure
* we can cope with this.
@@ -304,56 +305,43 @@ static int ldb_wildcard_compare(struct ldb_context *ldb,
if (cnk.length == 0) {
goto mismatch;
}
- /*
- * Values might be binary blobs. Don't use string
- * search, but memory search instead.
- */
- p = memmem((const void *)val.data,val.length,
- (const void *)cnk.data, cnk.length);
- if (p == NULL) goto mismatch;
-
- /*
- * At this point we know cnk.length <= val.length as
- * otherwise there could be no match
- */
+ if (cnk.length > val.length) {
+ goto mismatch;
+ }
- if ( (! tree->u.substring.chunks[c + 1]) && (! tree->u.substring.end_with_wildcard) ) {
- uint8_t *g;
- uint8_t *end = val.data + val.length;
- do { /* greedy */
-
- /*
- * haystack is a valid pointer in val
- * because the memmem() can only
- * succeed if the needle (cnk.length)
- * is <= haystacklen
- *
- * p will be a pointer at least
- * cnk.length from the end of haystack
- */
- uint8_t *haystack
- = p + cnk.length;
- size_t haystacklen
- = end - (haystack);
-
- g = memmem(haystack,
- haystacklen,
- (const uint8_t *)cnk.data,
- cnk.length);
- if (g) {
- p = g;
- }
- } while(g);
+ if ( (tree->u.substring.chunks[c + 1]) == NULL &&
+ (! tree->u.substring.end_with_wildcard) ) {
+ /*
+ * The last bit, after all the asterisks, must match
+ * exactly the last bit of the string.
+ */
+ int cmp;
+ p = val.data + val.length - cnk.length;
+ cmp = memcmp(p,
+ cnk.data,
+ cnk.length);
+ if (cmp != 0) {
+ goto mismatch;
+ }
+ } else {
+ /*
+ * Values might be binary blobs. Don't use string
+ * search, but memory search instead.
+ */
+ p = memmem((const void *)val.data, val.length,
+ (const void *)cnk.data, cnk.length);
+ if (p == NULL) {
+ goto mismatch;
+ }
+ /* move val to the end of the match */
+ p += cnk.length;
+ val.length -= (p - val.data);
+ val.data = p;
}
- val.length = val.length - (p - (uint8_t *)(val.data)) - cnk.length;
- val.data = (uint8_t *)(p + cnk.length);
c++;
- talloc_free(cnk.data);
- cnk.data = NULL;
+ TALLOC_FREE(cnk.data);
}
- /* last chunk may not have reached end of string */
- if ( (! tree->u.substring.end_with_wildcard) && (val.length != 0) ) goto mismatch;
talloc_free(save_p);
*matched = true;
return LDB_SUCCESS;
diff --git a/lib/ldb/tests/ldb_match_test.c b/lib/ldb/tests/ldb_match_test.c
index e09f50c86ba..3028aed072c 100644
--- a/lib/ldb/tests/ldb_match_test.c
+++ b/lib/ldb/tests/ldb_match_test.c
@@ -91,6 +91,33 @@ static int teardown(void **state)
return 0;
}
+static void escape_string(uint8_t *buf, size_t buflen,
+ const uint8_t *s, size_t len)
+{
+ size_t i;
+ size_t j = 0;
+ for (i = 0; i < len; i++) {
+ if (j == buflen - 1) {
+ goto fin;
+ }
+ if (s[i] >= 0x20) {
+ buf[j] = s[i];
+ j++;
+ } else {
+ if (j >= buflen - 4) {
+ goto fin;
+ }
+ /* utf-8 control char representation */
+ buf[j] = 0xE2;
+ buf[j + 1] = 0x90;
+ buf[j + 2] = 0x80 + s[i];
+ j+= 3;
+ }
+ }
+fin:
+ buf[j] = 0;
+}
+
/*
* The wild card pattern "attribute=*" is parsed as an LDB_OP_PRESENT operation
@@ -122,23 +149,110 @@ static void test_wildcard_match_star(void **state)
* Test basic wild card matching
*
*/
+struct wildcard_test {
+ uint8_t *val;
+ size_t val_size;
+ const char *search;
+ bool should_match;
+ bool fold;
+};
+
+/*
+ * Q: Why this macro rather than plain struct values?
+ * A: So we can get the size of the const char[] value while it is still a
+ * true array, not a pointer.
+ *
+ * Q: but why not just use strlen?
+ * A: so values can contain '\0', which we supposedly allow.
+ */
+
+#define TEST_ENTRY(val, search, should_match, fold) \
+ { \
+ (uint8_t*)discard_const(val), \
+ sizeof(val) - 1, \
+ search, \
+ should_match, \
+ fold \
+ }
+
static void test_wildcard_match(void **state)
{
struct ldbtest_ctx *ctx = *state;
- bool matched = false;
-
- uint8_t value[] = "The value.......end";
- struct ldb_val val = {
- .data = value,
- .length = (sizeof(value))
+ size_t failed = 0;
+ size_t i;
+ struct wildcard_test tests[] = {
+ TEST_ENTRY("The value.......end", "*end", true, true),
+ TEST_ENTRY("The value.......end", "*fend", false, true),
+ TEST_ENTRY("The value.......end", "*eel", false, true),
+ TEST_ENTRY("The value.......end", "*d", true, true),
+ TEST_ENTRY("The value.......end", "*D*", true, true),
+ TEST_ENTRY("The value.......end", "*e*d*", true, true),
+ TEST_ENTRY("end", "*e*d*", true, true),
+ TEST_ENTRY("end", " *e*d*", true, true),
+ TEST_ENTRY("1.0.0.0.0.0.0.0aaaaaaaaaaaa", "*aaaaa", true, true),
+ TEST_ENTRY("1.0..0.0.0.0.0.0.0aAaaaAAAAAAA", "*a", true, true),
+ TEST_ENTRY("1.0.0.0.0.0.0.0.0.0.0aaaa", "*aaaaa", false, true),
+ TEST_ENTRY("1.0.0.0.0.0.0.0.0.0.0", "*0.0", true, true),
+ TEST_ENTRY("1.0.0.0.0.0.0.0.0.0.0", "*0.0.0", true, true),
+ TEST_ENTRY("1.0.0.0.0.0.0.0.0.0", "1*0*0*0*0*0*0*0*0*0", true,
+ true),
+ TEST_ENTRY("1.0.0.0.0.0.0.0.0", "1*0*0*0*0*0*0*0*0*0", false,
+ true),
+ TEST_ENTRY("1.0.0.0.000.0.0.0.0", "1*0*0*0*0*0*0*0*0*0", true,
+ true),
+ TEST_ENTRY("1\n0\r0\t000.0.0.0.0", "1*0*0*0*0*0*0*0*0", true,
+ true),
+ /*
+ * We allow NUL bytes in non-casefolding syntaxes.
+ */
+ TEST_ENTRY("1\x00 x", "1*x", true, false),
+ TEST_ENTRY("1\x00 x", "*x", true, false),
+ TEST_ENTRY("1\x00 x", "*x*", true, false),
+ TEST_ENTRY("1\x00 x", "* *", true, false),
+ TEST_ENTRY("1\x00 x", "1*", true, false),
+ TEST_ENTRY("1\x00 b* x", "1*b*", true, false),
+ TEST_ENTRY("1.0..0.0.0.0.0.0.0aAaaaAAAAAAA", "*a", false, false),
};
- struct ldb_parse_tree *tree = ldb_parse_tree(ctx, "objectClass=*end");
- assert_non_null(tree);
- ldb_wildcard_compare(ctx->ldb, tree, val, &matched);
- assert_true(matched);
+ for (i = 0; i < ARRAY_SIZE(tests); i++) {
+ bool matched;
+ int ret;
+ struct ldb_val val = {
+ .data = (uint8_t *)tests[i].val,
+ .length = tests[i].val_size
+ };
+ const char *attr = tests[i].fold ? "objectclass" : "birthLocation";
+ const char *s = talloc_asprintf(ctx, "%s=%s",
+ attr, tests[i].search);
+ struct ldb_parse_tree *tree = ldb_parse_tree(ctx, s);
+ assert_non_null(tree);
+ ret = ldb_wildcard_compare(ctx->ldb, tree, val, &matched);
+ if (ret != LDB_SUCCESS) {
+ uint8_t buf[100];
+ escape_string(buf, sizeof(buf),
+ tests[i].val, tests[i].val_size);
+ print_error("%zu val: «%s», search «%s» FAILED with %d\n",
+ i, buf, tests[i].search, ret);
+ failed++;
+ }
+ if (matched != tests[i].should_match) {
+ uint8_t buf[100];
+ escape_string(buf, sizeof(buf),
+ tests[i].val, tests[i].val_size);
+ print_error("%zu val: «%s», search «%s» should %s\n",
+ i, buf, tests[i].search,
+ matched ? "not match" : "match");
+ failed++;
+ }
+ }
+ if (failed != 0) {
+ fail_msg("wrong results for %zu/%zu wildcard searches\n",
+ failed, ARRAY_SIZE(tests));
+ }
}
+#undef TEST_ENTRY
+
/*
* ldb_handler_copy and ldb_val_dup over allocate by one and add a trailing '\0'
diff --git a/lib/ldb/tests/test_ldb_dn.c b/lib/ldb/tests/test_ldb_dn.c
index 6faff9b7de7..d863e84766b 100644
--- a/lib/ldb/tests/test_ldb_dn.c
+++ b/lib/ldb/tests/test_ldb_dn.c
@@ -190,7 +190,6 @@ static void test_ldb_dn_explode(void **state)
* special, invalid, linear, and ext_linear are set before
* explode
*/
- fprintf(stderr, "%zu «%s»: ", i, tests[i].strdn);
linear = ldb_dn_get_linearized(dn);
assert_true((linear == NULL) == (tests[i].linearized == NULL));
assert_string_equal(linear,
@@ -209,8 +208,8 @@ static void test_ldb_dn_explode(void **state)
/* comp nums are set by explode */
result = ldb_dn_validate(dn);
- fprintf(stderr, "res %i lin «%s» ext «%s»\n",
- result, linear, ext_linear);
+ print_error("test %zu «%s»: res %i lin «%s» ext «%s»\n",
+ i, tests[i].strdn, result, linear, ext_linear);
assert_true(result == tests[i].explode_result);
assert_int_equal(ldb_dn_get_comp_num(dn),
--
Samba Shared Repository
More information about the samba-cvs
mailing list