[PATCH] Remove size limit of client-side parsing of dirsync control
Garming Sam
garming at catalyst.net.nz
Sun Nov 18 22:50:06 UTC 2018
I've changed the handling to use talloc, but I also noticed some talloc
handling issues which I've fixed. One other thing is that the dirsync
cookie seems to be logged far too often and I've removed one of the log
lines because inspecting the actual logs for normal operation isn't very
useful with these large cookies inside.
CI running for LDB control patches (minus the log line patch):
https://gitlab.com/samba-team/devel/samba/pipelines/36519578
Cheers,
Garming
On 30/10/18 1:23 PM, Garming Sam wrote:
> Hi metze,
>
> In a domain with 200 cursor entries in the dirsync cookie, while I was
> looking at syncing passwords, I noticed that the server failed to get a
> valid cookie returned and it turned out that it was truncated by the
> client (at 1024 base64 characters or ~670 bytes). Attached is the patch
> where I just did the simplest thing to get it to work. I don't know if
> we should have further validation, but I didn't know if there were
> actual limitations to this cookie, or what's done in Windows either.
>
> Cheers,
>
> Garming
>
-------------- next part --------------
From a39d9d2ae535ab80f2eee7b21132d5c51ee1f7a7 Mon Sep 17 00:00:00 2001
From: Garming Sam <garming at catalyst.net.nz>
Date: Fri, 26 Oct 2018 13:38:02 +1300
Subject: [PATCH 1/3] dirsync: Allow arbitrary length cookies
The length of the cookie is proportional to the number of DCs ever in
the domain (as it stores the uptodateness vector which has stale
invocationID).
Signed-off-by: Garming Sam <garming at catalyst.net.nz>
---
lib/ldb/common/ldb_controls.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/lib/ldb/common/ldb_controls.c b/lib/ldb/common/ldb_controls.c
index a83768a..f07f3c5 100644
--- a/lib/ldb/common/ldb_controls.c
+++ b/lib/ldb/common/ldb_controls.c
@@ -534,13 +534,20 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
if (LDB_CONTROL_CMP(control_strings, LDB_CONTROL_DIRSYNC_NAME) == 0) {
struct ldb_dirsync_control *control;
const char *p;
- char cookie[1024];
+ char *cookie = NULL;
int crit, max_attrs, ret;
uint32_t flags;
- cookie[0] = '\0';
+ cookie = talloc_zero_array(ctrl, char,
+ strlen(control_strings) + 1);
+ if (cookie == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
+
p = &(control_strings[sizeof(LDB_CONTROL_DIRSYNC_NAME)]);
- ret = sscanf(p, "%d:%u:%d:%1023[^$]", &crit, &flags, &max_attrs, cookie);
+ ret = sscanf(p, "%d:%u:%d:%[^$]", &crit, &flags, &max_attrs, cookie);
if ((ret < 3) || (crit < 0) || (crit > 1) || (max_attrs < 0)) {
ldb_set_errstring(ldb,
@@ -582,17 +589,25 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
control->cookie_len = 0;
}
ctrl->data = control;
+ TALLOC_FREE(cookie);
return ctrl;
}
if (LDB_CONTROL_CMP(control_strings, LDB_CONTROL_DIRSYNC_EX_NAME) == 0) {
struct ldb_dirsync_control *control;
const char *p;
- char cookie[1024];
+ char *cookie = NULL;
int crit, max_attrs, ret;
uint32_t flags;
- cookie[0] = '\0';
+ cookie = talloc_zero_array(ctrl, char,
+ strlen(control_strings) + 1);
+ if (cookie == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
+
p = &(control_strings[sizeof(LDB_CONTROL_DIRSYNC_EX_NAME)]);
ret = sscanf(p, "%d:%u:%d:%1023[^$]", &crit, &flags, &max_attrs, cookie);
@@ -637,6 +652,7 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
control->cookie_len = 0;
}
ctrl->data = control;
+ TALLOC_FREE(cookie);
return ctrl;
}
--
1.9.1
From ebc4a5c4eebc2618f0e7f72d8817c638b4ff7b4f Mon Sep 17 00:00:00 2001
From: Garming Sam <garming at catalyst.net.nz>
Date: Mon, 19 Nov 2018 11:05:59 +1300
Subject: [PATCH 2/3] sync_passwords: Remove dirsync cookie logging for
continuous operation
Under normal operation, users shouldn't see giant cookies in their logs.
We still log the initial cookie retrieved from the cache database, which
should still be helpful for identifying corrupt cookies.
Signed-off-by: Garming Sam <garming at catalyst.net.nz>
---
python/samba/netcmd/user.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/python/samba/netcmd/user.py b/python/samba/netcmd/user.py
index 0b8314d..cb46a95 100644
--- a/python/samba/netcmd/user.py
+++ b/python/samba/netcmd/user.py
@@ -2121,7 +2121,8 @@ samba-tool user syncpasswords --terminate \\
assert res_controls[0].oid == "1.2.840.113556.1.4.841"
res_controls[0].critical = True
self.dirsync_controls = [str(res_controls[0]), "extended_dn:1:0"]
- log_msg("dirsyncControls: %r\n" % self.dirsync_controls)
+ # This cookie can be extremely long
+ # log_msg("dirsyncControls: %r\n" % self.dirsync_controls)
modify_ldif = "dn: %s\n" % (self.cache_dn)
modify_ldif += "changetype: modify\n"
--
1.9.1
From 34e4ef0b46b9caac5af563606a73ec12ce956dc6 Mon Sep 17 00:00:00 2001
From: Garming Sam <garming at catalyst.net.nz>
Date: Wed, 14 Nov 2018 10:29:01 +1300
Subject: [PATCH 3/3] ldb_controls: Add some talloc error checking for controls
Signed-off-by: Garming Sam <garming at catalyst.net.nz>
---
lib/ldb/common/ldb_controls.c | 82 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 80 insertions(+), 2 deletions(-)
diff --git a/lib/ldb/common/ldb_controls.c b/lib/ldb/common/ldb_controls.c
index f07f3c5..e0f0eb4 100644
--- a/lib/ldb/common/ldb_controls.c
+++ b/lib/ldb/common/ldb_controls.c
@@ -520,6 +520,7 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
control->ctxid_len);
if (control->contextId == NULL) {
ldb_oom(ldb);
+ talloc_free(ctrl);
return NULL;
}
} else {
@@ -568,6 +569,11 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
ctrl->oid = LDB_CONTROL_DIRSYNC_OID;
ctrl->critical = crit;
control = talloc(ctrl, struct ldb_dirsync_control);
+ if (control == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
control->flags = flags;
control->max_attributes = max_attrs;
if (*cookie) {
@@ -582,6 +588,7 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
control->cookie = (char *)talloc_memdup(control, cookie, control->cookie_len);
if (control->cookie == NULL) {
ldb_oom(ldb);
+ talloc_free(ctrl);
return NULL;
}
} else {
@@ -630,6 +637,11 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
ctrl->oid = LDB_CONTROL_DIRSYNC_EX_OID;
ctrl->critical = crit;
control = talloc(ctrl, struct ldb_dirsync_control);
+ if (control == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
control->flags = flags;
control->max_attributes = max_attrs;
if (*cookie) {
@@ -645,6 +657,7 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
control->cookie = (char *)talloc_memdup(control, cookie, control->cookie_len);
if (control->cookie == NULL) {
ldb_oom(ldb);
+ talloc_free(ctrl);
return NULL;
}
} else {
@@ -678,6 +691,11 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
ctrl->oid = LDB_CONTROL_ASQ_OID;
ctrl->critical = crit;
control = talloc(ctrl, struct ldb_asq_control);
+ if (control == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
control->request = 1;
control->source_attribute = talloc_strdup(control, attr);
control->src_attr_len = strlen(attr);
@@ -709,6 +727,11 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
control = NULL;
} else {
control = talloc(ctrl, struct ldb_extended_dn_control);
+ if (control == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
control->type = type;
}
@@ -739,6 +762,12 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
ctrl->oid = LDB_CONTROL_SD_FLAGS_OID;
ctrl->critical = crit;
control = talloc(ctrl, struct ldb_sd_flags_control);
+ if (control == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
+
control->secinfo_flags = secinfo_flags;
ctrl->data = control;
@@ -765,6 +794,12 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
ctrl->oid = LDB_CONTROL_SEARCH_OPTIONS_OID;
ctrl->critical = crit;
control = talloc(ctrl, struct ldb_search_options_control);
+ if (control == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
+
control->search_options = search_options;
ctrl->data = control;
@@ -881,6 +916,12 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
ctrl->oid = LDB_CONTROL_PAGED_RESULTS_OID;
ctrl->critical = crit;
control = talloc(ctrl, struct ldb_paged_control);
+ if (control == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
+
control->size = size;
if (cookie[0] != '\0') {
int len = ldb_base64_decode(cookie);
@@ -895,6 +936,7 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
control->cookie = talloc_memdup(control, cookie, control->cookie_len);
if (control->cookie == NULL) {
ldb_oom(ldb);
+ talloc_free(ctrl);
return NULL;
}
} else {
@@ -928,12 +970,36 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
ctrl->oid = LDB_CONTROL_SERVER_SORT_OID;
ctrl->critical = crit;
control = talloc_array(ctrl, struct ldb_server_sort_control *, 2);
+ if (control == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
+
control[0] = talloc(control, struct ldb_server_sort_control);
+ if (control[0] == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
+
control[0]->attributeName = talloc_strdup(control, attr);
- if (rule[0])
+ if (control[0]->attributeName == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
+
+ if (rule[0]) {
control[0]->orderingRule = talloc_strdup(control, rule);
- else
+ if (control[0]->orderingRule == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
+ } else {
control[0]->orderingRule = NULL;
+ }
control[0]->reverse = rev;
control[1] = NULL;
ctrl->data = control;
@@ -1195,7 +1261,19 @@ struct ldb_control *ldb_parse_control_from_string(struct ldb_context *ldb, TALLO
ctrl->oid = LDB_CONTROL_VERIFY_NAME_OID;
ctrl->critical = crit;
control = talloc(ctrl, struct ldb_verify_name_control);
+ if (control == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
+
control->gc = talloc_strdup(control, gc);
+ if (control->gc == NULL) {
+ ldb_oom(ldb);
+ talloc_free(ctrl);
+ return NULL;
+ }
+
control->gc_len = strlen(gc);
control->flags = flags;
ctrl->data = control;
--
1.9.1
More information about the samba-technical
mailing list