Attachment 14207 Details for Bug 13374 – patch for master

[patch] patch for master

dn-search-segfault.patch.txt (text/plain), 16.21 KB, created by Andrew Bartlett on 2018-05-21 03:47:15 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrew Bartlett

Created: 2018-05-21 03:47:15 UTC

Size: 16.21 KB

patch

obsolete

>From 594303d9d1e6c96e5b0942525f617db98b27163f Mon Sep 17 00:00:00 2001
>From: Andrej Gessel <Andrej.Gessel@janztec.com>
>Date: Fri, 6 Apr 2018 18:18:33 +0200
>Subject: [PATCH 1/6] Add NULL check for ldb_dn_get_casefold() in
> ltdb_index_dn_attr()
>
>Signed-off-by: Andrej Gessel <Andrej.Gessel@janztec.com>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13374
>---
> lib/ldb/ldb_tdb/ldb_index.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
>diff --git a/lib/ldb/ldb_tdb/ldb_index.c b/lib/ldb/ldb_tdb/ldb_index.c
>index cd64e6965f6..16215274f34 100644
>--- a/lib/ldb/ldb_tdb/ldb_index.c
>+++ b/lib/ldb/ldb_tdb/ldb_index.c
>@@ -1582,6 +1582,15 @@ static int ltdb_index_dn_attr(struct ldb_module *module,
> 
> 	/* work out the index key from the parent DN */
> 	val.data = (uint8_t *)((uintptr_t)ldb_dn_get_casefold(dn));
>+	if (val.data == NULL) {
>+		const char *dn_str = ldb_dn_get_linearized(dn);
>+		ldb_asprintf_errstring(ldb_module_get_ctx(module),
>+				       __location__
>+				       ": Failed to get casefold DN"
>+				       "from: %s",
>+				       dn_str);
>+		return LDB_ERR_OPERATIONS_ERROR;
>+	}
> 	val.length = strlen((char *)val.data);
> 	key = ltdb_index_key(ldb, ltdb, attr, &val, NULL, truncation);
> 	if (!key) {
>-- 
>2.11.0
>
>
>From e116f6257a06f1e621a717ab2a2abaf76a238c30 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Mon, 21 May 2018 14:50:50 +1200
>Subject: [PATCH 2/6] ldb: Check for ldb_dn_get_casefold() failure in
> ldb_sqlite
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13374
>---
> lib/ldb/ldb_sqlite3/ldb_sqlite3.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/lib/ldb/ldb_sqlite3/ldb_sqlite3.c b/lib/ldb/ldb_sqlite3/ldb_sqlite3.c
>index f94dc993904..0f5abf87547 100644
>--- a/lib/ldb/ldb_sqlite3/ldb_sqlite3.c
>+++ b/lib/ldb/ldb_sqlite3/ldb_sqlite3.c
>@@ -323,6 +323,9 @@ static char *parsetree_to_sql(struct ldb_module *module,
> 		 	const char *cdn = ldb_dn_get_casefold(
> 						ldb_dn_new(mem_ctx, ldb,
> 							      (const char *)value.data));
>+			if (cdn == NULL) {
>+				return NULL;
>+			}
> 
> 			return lsqlite3_tprintf(mem_ctx,
> 						"SELECT eid FROM ldb_entry "
>-- 
>2.11.0
>
>
>From 65293a44000b816c30a2908fe03e3f481d43594a Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Mon, 21 May 2018 15:20:26 +1200
>Subject: [PATCH 3/6] ldb_tdb: Ensure the dn in distinguishedName= is valid
> before use
>
>ldb_dn_from_ldb_val() does not validate this untrusted input, so a later
>call to ldb_dn_get_casefold() can fail if the input is not valid.
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13374
>---
> lib/ldb/ldb_tdb/ldb_index.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
>diff --git a/lib/ldb/ldb_tdb/ldb_index.c b/lib/ldb/ldb_tdb/ldb_index.c
>index 16215274f34..d8cc9dcebab 100644
>--- a/lib/ldb/ldb_tdb/ldb_index.c
>+++ b/lib/ldb/ldb_tdb/ldb_index.c
>@@ -1143,6 +1143,7 @@ static int ltdb_index_dn_leaf(struct ldb_module *module,
> 	}
> 	if (ldb_attr_dn(tree->u.equality.attr) == 0) {
> 		enum key_truncation truncation = KEY_NOT_TRUNCATED;
>+		bool valid_dn = false;
> 		struct ldb_dn *dn
> 			= ldb_dn_from_ldb_val(list,
> 					      ldb_module_get_ctx(module),
>@@ -1154,6 +1155,14 @@ static int ltdb_index_dn_leaf(struct ldb_module *module,
> 			return LDB_SUCCESS;
> 		}
> 
>+		valid_dn = ldb_dn_validate(dn);
>+		if (valid_dn == false) {
>+			/* If we can't parse it, no match */
>+			list->dn = NULL;
>+			list->count = 0;
>+			return LDB_SUCCESS;
>+		}
>+
> 		/*
> 		 * Re-use the same code we use for a SCOPE_BASE
> 		 * search
>-- 
>2.11.0
>
>
>From 7d607f888ec8fd2548e657298d777a3de7835790 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Mon, 21 May 2018 15:23:53 +1200
>Subject: [PATCH 4/6] ldb_tdb: Check for DN validity in add, rename and search
>
>This ensures we fail with a good error code before an eventual ldb_dn_get_casefold() which
>would otherwise fail.
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13374
>---
> lib/ldb/ldb_tdb/ldb_search.c | 16 ++++++++++++++++
> lib/ldb/ldb_tdb/ldb_tdb.c    | 27 ++++++++++++++++++++++++++-
> 2 files changed, 42 insertions(+), 1 deletion(-)
>
>diff --git a/lib/ldb/ldb_tdb/ldb_search.c b/lib/ldb/ldb_tdb/ldb_search.c
>index 35583a1eaa3..c904d9e32db 100644
>--- a/lib/ldb/ldb_tdb/ldb_search.c
>+++ b/lib/ldb/ldb_tdb/ldb_search.c
>@@ -294,6 +294,14 @@ int ltdb_search_dn1(struct ldb_module *module, struct ldb_dn *dn, struct ldb_mes
> 	};
> 	TALLOC_CTX *tdb_key_ctx = NULL;
> 
>+	bool valid_dn = ldb_dn_validate(dn);
>+	if (valid_dn == false) {
>+		ldb_asprintf_errstring(ldb_module_get_ctx(module),
>+				       "Invalid Base DN: %s",
>+				       ldb_dn_get_linearized(dn));
>+		return LDB_ERR_INVALID_DN_SYNTAX;
>+	}
>+
> 	if (ltdb->cache->GUID_index_attribute == NULL ||
> 		ldb_dn_is_special(dn)) {
> 
>@@ -787,6 +795,14 @@ int ltdb_search(struct ltdb_context *ctx)
> 					       ldb_dn_get_linearized(req->op.search.base));
> 		}
> 			
>+	} else if (ldb_dn_validate(req->op.search.base) == false) {
>+
>+		/* We don't want invalid base DNs here */
>+		ldb_asprintf_errstring(ldb,
>+				       "Invalid Base DN: %s",
>+				       ldb_dn_get_linearized(req->op.search.base));
>+		ret = LDB_ERR_INVALID_DN_SYNTAX;
>+
> 	} else {
> 		/* If we are not checking the base DN life is easy */
> 		ret = LDB_SUCCESS;
>diff --git a/lib/ldb/ldb_tdb/ldb_tdb.c b/lib/ldb/ldb_tdb/ldb_tdb.c
>index f9bc35ca0fb..48d0eab95cf 100644
>--- a/lib/ldb/ldb_tdb/ldb_tdb.c
>+++ b/lib/ldb/ldb_tdb/ldb_tdb.c
>@@ -565,6 +565,16 @@ static int ltdb_add_internal(struct ldb_module *module,
> 	struct ldb_context *ldb = ldb_module_get_ctx(module);
> 	int ret = LDB_SUCCESS;
> 	unsigned int i;
>+	bool valid_dn = false;
>+
>+	/* Check the new DN is reasonable */
>+	valid_dn = ldb_dn_validate(msg->dn);
>+	if (valid_dn == false) {
>+		ldb_asprintf_errstring(ldb_module_get_ctx(module),
>+				       "Invalid DN in ADD: %s",
>+				       ldb_dn_get_linearized(msg->dn));
>+		return LDB_ERR_INVALID_DN_SYNTAX;
>+	}
> 
> 	for (i=0;i<msg->num_elements;i++) {
> 		struct ldb_message_element *el = &msg->elements[i];
>@@ -1367,6 +1377,7 @@ static int ltdb_rename(struct ltdb_context *ctx)
> 	int ret = LDB_SUCCESS;
> 	TDB_DATA tdb_key, tdb_key_old;
> 	struct ldb_dn *db_dn;
>+	bool valid_dn = false;
> 
> 	ldb_request_set_state(req, LDB_ASYNC_PENDING);
> 
>@@ -1379,10 +1390,24 @@ static int ltdb_rename(struct ltdb_context *ctx)
> 		return LDB_ERR_OPERATIONS_ERROR;
> 	}
> 
>+	/* Check the new DN is reasonable */
>+	valid_dn = ldb_dn_validate(req->op.rename.newdn);
>+	if (valid_dn == false) {
>+		ldb_asprintf_errstring(ldb_module_get_ctx(module),
>+				       "Invalid New DN: %s",
>+				       ldb_dn_get_linearized(req->op.rename.newdn));
>+		return LDB_ERR_INVALID_DN_SYNTAX;
>+	}
>+
> 	/* we need to fetch the old record to re-add under the new name */
> 	ret = ltdb_search_dn1(module, req->op.rename.olddn, msg,
> 			      LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC);
>-	if (ret != LDB_SUCCESS) {
>+	if (ret == LDB_ERR_INVALID_DN_SYNTAX) {
>+		ldb_asprintf_errstring(ldb_module_get_ctx(module),
>+				       "Invalid Old DN: %s",
>+				       ldb_dn_get_linearized(req->op.rename.newdn));
>+		return ret;
>+	} else if (ret != LDB_SUCCESS) {
> 		/* not finding the old record is an error */
> 		return ret;
> 	}
>-- 
>2.11.0
>
>
>From b7f0529c251738fb46dc903ea3fd02eab6b69ab4 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Mon, 21 May 2018 15:25:33 +1200
>Subject: [PATCH 5/6] ldb_tdb: Remove pointless check of ldb_dn_is_valid()
>
>If the DN is not valid the ltdb_search_dn1() will catch it with ldb_dn_validate() which
>is the only safe way to check this.  ldb_dn_is_valid() does not actually check, but instead
>returns only the result of the previous checks, if there was one.
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13374
>---
> lib/ldb/ldb_tdb/ldb_search.c | 8 --------
> 1 file changed, 8 deletions(-)
>
>diff --git a/lib/ldb/ldb_tdb/ldb_search.c b/lib/ldb/ldb_tdb/ldb_search.c
>index c904d9e32db..6233548280a 100644
>--- a/lib/ldb/ldb_tdb/ldb_search.c
>+++ b/lib/ldb/ldb_tdb/ldb_search.c
>@@ -755,14 +755,6 @@ int ltdb_search(struct ltdb_context *ctx)
> 			/* We accept subtree searches from a NULL base DN, ie over the whole DB */
> 			ret = LDB_SUCCESS;
> 		}
>-	} else if (ldb_dn_is_valid(req->op.search.base) == false) {
>-
>-		/* We don't want invalid base DNs here */
>-		ldb_asprintf_errstring(ldb, 
>-				       "Invalid Base DN: %s", 
>-				       ldb_dn_get_linearized(req->op.search.base));
>-		ret = LDB_ERR_INVALID_DN_SYNTAX;
>-
> 	} else if (req->op.search.scope == LDB_SCOPE_BASE) {
> 
> 		/*
>-- 
>2.11.0
>
>
>From 24493934323694627992fef717a9f719fc7879e0 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Mon, 21 May 2018 15:25:58 +1200
>Subject: [PATCH 6/6] ldb: Add tests for search add and rename with a bad dn=
> DN
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13374
>---
> lib/ldb/tests/python/api.py | 156 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 156 insertions(+)
>
>diff --git a/lib/ldb/tests/python/api.py b/lib/ldb/tests/python/api.py
>index 657fa14b4ff..ef3723ed02a 100755
>--- a/lib/ldb/tests/python/api.py
>+++ b/lib/ldb/tests/python/api.py
>@@ -417,6 +417,19 @@ class SimpleLdb(LdbBaseTest):
>         finally:
>             l.delete(ldb.Dn(l, "dc=bar"))
> 
>+    def test_rename_bad_string_dns(self):
>+        l = ldb.Ldb(self.url(), flags=self.flags())
>+        m = ldb.Message()
>+        m.dn = ldb.Dn(l, "dc=foo8")
>+        m["bla"] = b"bla"
>+        m["objectUUID"] = b"0123456789abcdef"
>+        self.assertEqual(len(l.search()), 0)
>+        l.add(m)
>+        self.assertEqual(len(l.search()), 1)
>+        self.assertRaises(ldb.LdbError,lambda: l.rename("dcXfoo8", "dc=bar"))
>+        self.assertRaises(ldb.LdbError,lambda: l.rename("dc=foo8", "dcXbar"))
>+        l.delete(ldb.Dn(l, "dc=foo8"))
>+
>     def test_empty_dn(self):
>         l = ldb.Ldb(self.url(), flags=self.flags())
>         self.assertEqual(0, len(l.search()))
>@@ -1092,6 +1105,110 @@ class SearchTests(LdbBaseTest):
>         # At some point we should fix this, but it isn't trivial
>         self.assertEqual(len(res11), 1)
> 
>+    def test_distinguishedName_filter_one(self):
>+        """Testing that a distinguishedName= filter succeeds
>+        when the scope is SCOPE_ONELEVEL.
>+
>+        This should be made more consistent, but for now lock in
>+        the behaviour
>+
>+        """
>+
>+        res11 = self.l.search(base="DC=SAMBA,DC=ORG",
>+                              scope=ldb.SCOPE_ONELEVEL,
>+                              expression="(distinguishedName=OU=OU1,DC=SAMBA,DC=ORG)")
>+        self.assertEqual(len(res11), 1)
>+
>+    def test_distinguishedName_filter_subtree(self):
>+        """Testing that a distinguishedName= filter succeeds
>+        when the scope is SCOPE_SUBTREE"""
>+
>+        res11 = self.l.search(base="DC=SAMBA,DC=ORG",
>+                              scope=ldb.SCOPE_SUBTREE,
>+                              expression="(distinguishedName=OU=OU1,DC=SAMBA,DC=ORG)")
>+        self.assertEqual(len(res11), 1)
>+
>+    def test_distinguishedName_filter_base(self):
>+        """Testing that (incorrectly) a distinguishedName= filter works
>+        when the scope is SCOPE_BASE"""
>+
>+        res11 = self.l.search(base="OU=OU1,DC=SAMBA,DC=ORG",
>+                              scope=ldb.SCOPE_BASE,
>+                              expression="(distinguishedName=OU=OU1,DC=SAMBA,DC=ORG)")
>+
>+        # At some point we should fix this, but it isn't trivial
>+        self.assertEqual(len(res11), 1)
>+
>+    def test_bad_dn_filter_base(self):
>+        """Testing that a dn= filter on an invalid DN works
>+        when the scope is SCOPE_BASE but
>+        returns zero results"""
>+
>+        res11 = self.l.search(base="OU=OU1,DC=SAMBA,DC=ORG",
>+                              scope=ldb.SCOPE_BASE,
>+                              expression="(dn=OU=OU1,DC=SAMBA,DCXXXX)")
>+
>+        # At some point we should fix this, but it isn't trivial
>+        self.assertEqual(len(res11), 0)
>+
>+
>+    def test_bad_dn_filter_one(self):
>+        """Testing that a dn= filter succeeds but returns zero
>+        results when the DN is not valid on a SCOPE_ONELEVEL search
>+
>+        """
>+
>+        res11 = self.l.search(base="DC=SAMBA,DC=ORG",
>+                              scope=ldb.SCOPE_ONELEVEL,
>+                              expression="(dn=OU=OU1,DC=SAMBA,DCXXXX)")
>+        self.assertEqual(len(res11), 0)
>+
>+    def test_bad_dn_filter_subtree(self):
>+        """Testing that a dn= filter succeeds but returns zero
>+        results when the DN is not valid on a SCOPE_SUBTREE search
>+
>+        """
>+
>+        res11 = self.l.search(base="DC=SAMBA,DC=ORG",
>+                              scope=ldb.SCOPE_SUBTREE,
>+                              expression="(dn=OU=OU1,DC=SAMBA,DCXXXX)")
>+        self.assertEqual(len(res11), 0)
>+
>+    def test_bad_distinguishedName_filter_base(self):
>+        """Testing that a distinguishedName= filter on an invalid DN works
>+        when the scope is SCOPE_BASE but
>+        returns zero results"""
>+
>+        res11 = self.l.search(base="OU=OU1,DC=SAMBA,DC=ORG",
>+                              scope=ldb.SCOPE_BASE,
>+                              expression="(distinguishedName=OU=OU1,DC=SAMBA,DCXXXX)")
>+
>+        # At some point we should fix this, but it isn't trivial
>+        self.assertEqual(len(res11), 0)
>+
>+
>+    def test_bad_distinguishedName_filter_one(self):
>+        """Testing that a distinguishedName= filter succeeds but returns zero
>+        results when the DN is not valid on a SCOPE_ONELEVEL search
>+
>+        """
>+
>+        res11 = self.l.search(base="DC=SAMBA,DC=ORG",
>+                              scope=ldb.SCOPE_ONELEVEL,
>+                              expression="(distinguishedName=OU=OU1,DC=SAMBA,DCXXXX)")
>+        self.assertEqual(len(res11), 0)
>+
>+    def test_bad_distinguishedName_filter_subtree(self):
>+        """Testing that a distinguishedName= filter succeeds but returns zero
>+        results when the DN is not valid on a SCOPE_SUBTREE search
>+
>+        """
>+
>+        res11 = self.l.search(base="DC=SAMBA,DC=ORG",
>+                              scope=ldb.SCOPE_SUBTREE,
>+                              expression="(distinguishedName=OU=OU1,DC=SAMBA,DCXXXX)")
>+        self.assertEqual(len(res11), 0)
>+
> 
> class IndexedSearchTests(SearchTests):
>     """Test searches using the index, to ensure the index doesn't
>@@ -1227,6 +1344,17 @@ class AddModifyTests(LdbBaseTest):
>             enum = err.args[0]
>             self.assertEqual(enum, ldb.ERR_ENTRY_ALREADY_EXISTS)
> 
>+    def test_add_bad(self):
>+        try:
>+            self.l.add({"dn": "BAD,DC=SAMBA,DC=ORG",
>+                        "name": b"Admins",
>+                        "x": "z", "y": "a",
>+                        "objectUUID": b"0123456789abcde1"})
>+            self.fail("Should have failed adding entry with invalid DN")
>+        except ldb.LdbError as err:
>+            enum = err.args[0]
>+            self.assertEqual(enum, ldb.ERR_INVALID_DN_SYNTAX)
>+
>     def test_add_del_add(self):
>         self.l.add({"dn": "OU=DUP,DC=SAMBA,DC=ORG",
>                     "name": b"Admins",
>@@ -1321,6 +1449,34 @@ class AddModifyTests(LdbBaseTest):
>             enum = err.args[0]
>             self.assertEqual(enum, ldb.ERR_NO_SUCH_OBJECT)
> 
>+    def test_move_bad(self):
>+        self.l.add({"dn": "OU=DUP2,DC=SAMBA,DC=ORG",
>+                    "name": b"Admins",
>+                    "x": "z", "y": "a",
>+                    "objectUUID": b"0123456789abcde2"})
>+
>+        try:
>+            self.l.rename("OUXDUP,DC=SAMBA,DC=ORG",
>+                          "OU=DUP2,DC=SAMBA,DC=ORG")
>+            self.fail("Should have failed on invalid DN")
>+        except ldb.LdbError as err:
>+            enum = err.args[0]
>+            self.assertEqual(enum, ldb.ERR_INVALID_DN_SYNTAX)
>+
>+    def test_move_bad2(self):
>+        self.l.add({"dn": "OU=DUP2,DC=SAMBA,DC=ORG",
>+                    "name": b"Admins",
>+                    "x": "z", "y": "a",
>+                    "objectUUID": b"0123456789abcde2"})
>+
>+        try:
>+            self.l.rename("OU=DUP,DC=SAMBA,DC=ORG",
>+                          "OUXDUP2,DC=SAMBA,DC=ORG")
>+            self.fail("Should have failed on missing")
>+        except ldb.LdbError as err:
>+            enum = err.args[0]
>+            self.assertEqual(enum, ldb.ERR_INVALID_DN_SYNTAX)
>+
>     def test_move_fail_move_add(self):
>         self.l.add({"dn": "OU=DUP,DC=SAMBA,DC=ORG",
>                     "name": b"Admins",
>-- 
>2.11.0
>

Actions: View

Attachments on bug 13374: 14113 | 14206 | 14207 | 14208 | 14267 | 14270 | 14271 | 14272 | 14273 | 14274 | 14275 | 14276 | 14278 | 14279 | 14282 | 14283 | 14289 | 14290 | 14295 | 14296 | 14297 | 14316 | 14317 | 14418 | 14419