CVE-2023-0614 ldb: Make use of ldb_filter_attrs_in_place()

Change all uses of ldb_kv_filter_attrs() to use
ldb_filter_attrs_in_place() instead. This function does less work than
its predecessor, and no longer requires the allocation of a second ldb
message. Some of the work is able to be split out into separate
functions that each accomplish a single task, with a purpose to make the
code clearer.

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

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>

diff --git a/lib/ldb/ldb_key_value/ldb_kv.h b/lib/ldb/ldb_key_value/ldb_kv.h
index ac474b04b4cd65cb959d092ecd02f3404af14384..7d5a40e76e919e4b62f729e8c8846f5cb8519f31 100644 (file)
--- a/lib/ldb/ldb_key_value/ldb_kv.h
+++ b/lib/ldb/ldb_key_value/ldb_kv.h
@@ -301,10 +301,8 @@ int ldb_kv_search_key(struct ldb_module *module,
                      const struct ldb_val ldb_key,
                      struct ldb_message *msg,
                      unsigned int unpack_flags);
-int ldb_kv_filter_attrs(struct ldb_context *ldb,
-                       const struct ldb_message *msg,
-                       const char *const *attrs,
-                       struct ldb_message *filtered_msg);
+int ldb_kv_filter_attrs_in_place(struct ldb_message *msg,
+                                const char *const *attrs);
 int ldb_kv_search(struct ldb_kv_context *ctx);
 
 /*

diff --git a/lib/ldb/ldb_key_value/ldb_kv_index.c b/lib/ldb/ldb_key_value/ldb_kv_index.c
index d70e5f619efc5d95ee5a713d64adb8d3f286cd0f..203266ea8c90f20a4d805f4e3f88a1af069dbf1b 100644 (file)
--- a/lib/ldb/ldb_key_value/ldb_kv_index.c
+++ b/lib/ldb/ldb_key_value/ldb_kv_index.c
@@ -2264,7 +2264,6 @@ static int ldb_kv_index_filter(struct ldb_kv_private *ldb_kv,
 {
        struct ldb_context *ldb = ldb_module_get_ctx(ac->module);
        struct ldb_message *msg;
-       struct ldb_message *filtered_msg;
        unsigned int i;
        unsigned int num_keys = 0;
        uint8_t previous_guid_key[LDB_KV_GUID_KEY_SIZE] = {0};
@@ -2456,27 +2455,31 @@ static int ldb_kv_index_filter(struct ldb_kv_private *ldb_kv,
                        continue;
                }
 
-               filtered_msg = ldb_msg_new(ac);
-               if (filtered_msg == NULL) {
-                       TALLOC_FREE(keys);
-                       TALLOC_FREE(msg);
+               ret = ldb_msg_add_distinguished_name(msg);
+               if (ret == -1) {
+                       talloc_free(msg);
                        return LDB_ERR_OPERATIONS_ERROR;
                }
 
-               filtered_msg->dn = talloc_steal(filtered_msg, msg->dn);
-
                /* filter the attributes that the user wants */
-               ret = ldb_kv_filter_attrs(ldb, msg, ac->attrs, filtered_msg);
+               ret = ldb_kv_filter_attrs_in_place(msg, ac->attrs);
+               if (ret != LDB_SUCCESS) {
+                       talloc_free(keys);
+                       talloc_free(msg);
+                       return LDB_ERR_OPERATIONS_ERROR;
+               }
 
-               talloc_free(msg);
+               ldb_msg_shrink_to_fit(msg);
 
-               if (ret == -1) {
-                       TALLOC_FREE(filtered_msg);
+               /* Ensure the message elements are all talloc'd. */
+               ret = ldb_msg_elements_take_ownership(msg);
+               if (ret != LDB_SUCCESS) {
                        talloc_free(keys);
+                       talloc_free(msg);
                        return LDB_ERR_OPERATIONS_ERROR;
                }
 
-               ret = ldb_module_send_entry(ac->req, filtered_msg, NULL);
+               ret = ldb_module_send_entry(ac->req, msg, NULL);
                if (ret != LDB_SUCCESS) {
                        /* Regardless of success or failure, the msg
                         * is the callbacks responsiblity, and should

diff --git a/lib/ldb/ldb_key_value/ldb_kv_search.c b/lib/ldb/ldb_key_value/ldb_kv_search.c
index 46031b99c16c9410d85ed622de01f670801ccdce..f3333510eab0651953bf4711ebe2bb4ee2b96683 100644 (file)
--- a/lib/ldb/ldb_key_value/ldb_kv_search.c
+++ b/lib/ldb/ldb_key_value/ldb_kv_search.c
@@ -292,15 +292,13 @@ int ldb_kv_search_dn1(struct ldb_module *module,
 
 /*
  * filter the specified list of attributes from msg,
- * adding requested attributes, and perhaps all for *,
- * but not the DN to filtered_msg.
+ * adding requested attributes, and perhaps all for *.
+ * The DN will not be added if it is missing.
  */
-int ldb_kv_filter_attrs(struct ldb_context *ldb,
-                       const struct ldb_message *msg,
-                       const char *const *attrs,
-                       struct ldb_message *filtered_msg)
+int ldb_kv_filter_attrs_in_place(struct ldb_message *msg,
+                                const char *const *attrs)
 {
-       return ldb_filter_attrs(ldb, msg, attrs, filtered_msg);
+       return ldb_filter_attrs_in_place(msg, attrs);
 }
 
 /*
@@ -313,7 +311,7 @@ static int search_func(_UNUSED_ struct ldb_kv_private *ldb_kv,
 {
        struct ldb_context *ldb;
        struct ldb_kv_context *ac;
-       struct ldb_message *msg, *filtered_msg;
+       struct ldb_message *msg;
        struct timeval now;
        int ret, timeval_cmp;
        bool matched;
@@ -410,25 +408,31 @@ static int search_func(_UNUSED_ struct ldb_kv_private *ldb_kv,
                return 0;
        }
 
-       filtered_msg = ldb_msg_new(ac);
-       if (filtered_msg == NULL) {
-               TALLOC_FREE(msg);
+       ret = ldb_msg_add_distinguished_name(msg);
+       if (ret == -1) {
+               talloc_free(msg);
                return LDB_ERR_OPERATIONS_ERROR;
        }
 
-       filtered_msg->dn = talloc_steal(filtered_msg, msg->dn);
-
        /* filter the attributes that the user wants */
-       ret = ldb_kv_filter_attrs(ldb, msg, ac->attrs, filtered_msg);
-       talloc_free(msg);
+       ret = ldb_kv_filter_attrs_in_place(msg, ac->attrs);
+       if (ret != LDB_SUCCESS) {
+               talloc_free(msg);
+               ac->error = LDB_ERR_OPERATIONS_ERROR;
+               return -1;
+       }
 
-       if (ret == -1) {
-               TALLOC_FREE(filtered_msg);
+       ldb_msg_shrink_to_fit(msg);
+
+       /* Ensure the message elements are all talloc'd. */
+       ret = ldb_msg_elements_take_ownership(msg);
+       if (ret != LDB_SUCCESS) {
+               talloc_free(msg);
                ac->error = LDB_ERR_OPERATIONS_ERROR;
                return -1;
        }
 
-       ret = ldb_module_send_entry(ac->req, filtered_msg, NULL);
+       ret = ldb_module_send_entry(ac->req, msg, NULL);
        if (ret != LDB_SUCCESS) {
                ac->request_terminated = true;
                /* the callback failed, abort the operation */
@@ -491,7 +495,7 @@ static int ldb_kv_search_full(struct ldb_kv_context *ctx)
 static int ldb_kv_search_and_return_base(struct ldb_kv_private *ldb_kv,
                                         struct ldb_kv_context *ctx)
 {
-       struct ldb_message *msg, *filtered_msg;
+       struct ldb_message *msg;
        struct ldb_context *ldb = ldb_module_get_ctx(ctx->module);
        const char *dn_linearized;
        const char *msg_dn_linearized;
@@ -549,12 +553,6 @@ static int ldb_kv_search_and_return_base(struct ldb_kv_private *ldb_kv,
        dn_linearized = ldb_dn_get_linearized(ctx->base);
        msg_dn_linearized = ldb_dn_get_linearized(msg->dn);
 
-       filtered_msg = ldb_msg_new(ctx);
-       if (filtered_msg == NULL) {
-               talloc_free(msg);
-               return LDB_ERR_OPERATIONS_ERROR;
-       }
-
        if (strcmp(dn_linearized, msg_dn_linearized) == 0) {
                /*
                 * If the DN is exactly the same string, then
@@ -562,36 +560,42 @@ static int ldb_kv_search_and_return_base(struct ldb_kv_private *ldb_kv,
                 * returned result, as it has already been
                 * casefolded
                 */
-               filtered_msg->dn = ldb_dn_copy(filtered_msg, ctx->base);
+               struct ldb_dn *dn = ldb_dn_copy(msg, ctx->base);
+               if (dn != NULL) {
+                       msg->dn = dn;
+               }
        }
 
-       /*
-        * If the ldb_dn_copy() failed, or if we did not choose that
-        * optimisation (filtered_msg is zeroed at allocation),
-        * steal the one from the unpack
-        */
-       if (filtered_msg->dn == NULL) {
-               filtered_msg->dn = talloc_steal(filtered_msg, msg->dn);
+       ret = ldb_msg_add_distinguished_name(msg);
+       if (ret == -1) {
+               talloc_free(msg);
+               return LDB_ERR_OPERATIONS_ERROR;
        }
 
        /*
         * filter the attributes that the user wants.
         */
-       ret = ldb_kv_filter_attrs(ldb, msg, ctx->attrs, filtered_msg);
-       if (ret == -1) {
+       ret = ldb_kv_filter_attrs_in_place(msg, ctx->attrs);
+       if (ret != LDB_SUCCESS) {
+               talloc_free(msg);
+               return LDB_ERR_OPERATIONS_ERROR;
+       }
+
+       ldb_msg_shrink_to_fit(msg);
+
+       /* Ensure the message elements are all talloc'd. */
+       ret = ldb_msg_elements_take_ownership(msg);
+       if (ret != LDB_SUCCESS) {
                talloc_free(msg);
-               filtered_msg = NULL;
                return LDB_ERR_OPERATIONS_ERROR;
        }
 
        /*
-        * Remove any extended components possibly copied in from
-        * msg->dn, we just want the casefold components
+        * Remove any extended components, we just want the casefold components
         */
-       ldb_dn_remove_extended_components(filtered_msg->dn);
-       talloc_free(msg);
+       ldb_dn_remove_extended_components(msg->dn);
 
-       ret = ldb_module_send_entry(ctx->req, filtered_msg, NULL);
+       ret = ldb_module_send_entry(ctx->req, msg, NULL);
        if (ret != LDB_SUCCESS) {
                /* Regardless of success or failure, the msg
                 * is the callbacks responsiblity, and should

diff --git a/source4/torture/ldb/ldb.c b/source4/torture/ldb/ldb.c
index bd0ae3a382a00c35c3a09ddc272f1813151857fa..c170416bec42ca9dfe9569444fe337cd681f7461 100644 (file)
--- a/source4/torture/ldb/ldb.c
+++ b/source4/torture/ldb/ldb.c
@@ -1634,7 +1634,6 @@ static bool torture_ldb_unpack_and_filter(struct torture_context *torture,
        TALLOC_CTX *mem_ctx = talloc_new(torture);
        struct ldb_context *ldb;
        struct ldb_val data = *discard_const_p(struct ldb_val, data_p);
-       struct ldb_message *unpack_msg = ldb_msg_new(mem_ctx);
        struct ldb_message *msg = ldb_msg_new(mem_ctx);
        const char *lookup_names[] = {"instanceType", "nonexistent",
                                      "whenChanged", "objectClass",
@@ -1649,18 +1648,15 @@ static bool torture_ldb_unpack_and_filter(struct torture_context *torture,
                       "Failed to init samba");
 
        torture_assert_int_equal(torture,
-                                ldb_unpack_data(ldb, &data, unpack_msg),
+                                ldb_unpack_data(ldb, &data, msg),
                                 0, "ldb_unpack_data failed");
 
-       torture_assert_int_equal(torture, unpack_msg->num_elements, 13,
+       torture_assert_int_equal(torture, msg->num_elements, 13,
                                 "Got wrong count of elements");
 
-       msg->dn = talloc_steal(msg, unpack_msg->dn);
-
        torture_assert_int_equal(torture,
-                                ldb_filter_attrs(ldb, unpack_msg,
-                                                 lookup_names, msg),
-                                0, "ldb_kv_filter_attrs failed");
+                                ldb_filter_attrs_in_place(msg, lookup_names),
+                                0, "ldb_filter_attrs_in_place failed");
 
        /* Compare data in binary form */
        torture_assert_int_equal(torture, msg->num_elements, 6,