[PATCHES] add some safeguards in idmap_hash module
Michael Adam
obnox at samba.org
Wed Mar 16 07:48:24 UTC 2016
On 2016-03-15 at 21:37 -0700, Jeremy Allison wrote:
> On Tue, Mar 15, 2016 at 12:59:03PM +0100, Günther Deschner wrote:
> > Hi,
> >
> > the idmap_hash module can behave pretty badly in more advanced idmap
> > configurations, add some more safety when using this module.
> >
> > Resolves https://bugzilla.samba.org/show_bug.cgi?id=11786.
> >
> > Please review and push,
>
> In this patch:
>
> Subject: [PATCH 3/5] s3:winbindd:idmap: check loadparm in
> domain_has_idmap_config() helper as well.
>
> You have:
>
> @@ -123,6 +123,9 @@ static bool idmap_init(void)
> bool domain_has_idmap_config(const char *domname)
> {
> int i;
> + const char *config_option;
> + const char *range = NULL;
> + const char *backend = NULL;
>
> idmap_init();
>
> @@ -132,6 +135,23 @@ bool domain_has_idmap_config(const char *domname)
> }
> }
>
> + /* fallback: also check loadparm */
> +
> + config_option = talloc_asprintf(talloc_tos(), "idmap config %s",
> + domname);
> + if (config_option == NULL) {
> + DEBUG(0, ("out of memory\n"));
> + return false;
> + }
> +
> + range = lp_parm_const_string(-1, config_option, "range", NULL);
> + backend = lp_parm_const_string(-1, config_option, "backend", NULL);
> + if (range != NULL && backend != NULL) {
> + DEBUG(5, ("idmap configuration specified for domain '%s'\n",
> + domname));
> + return true;
> + }
> +
> return false;
>
> This leaves config_option allocated on talloc_tos() on
> exit of the function. It won't leak memory (because, talloc :-),
> but it's untidy. Can you make config option a char * not a
> const char * and talloc_free on all exit paths please ?
Thanks for spotting this.
Updated patchset attached.
Also removed some left-over "fixup" from a rebase
from one commit message which Andreas had spottet.
Thanks - Michael
-------------- next part --------------
From e7cb8d31a6aaf094b71c8ed855a1060bd624d1da Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Thu, 10 Mar 2016 10:38:29 +0100
Subject: [PATCH 1/5] s3:winbindd:idmap: add domain_has_idmap_config() helper
function.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11786
Pair-Programmed-With: Guenther Deschner <gd at samba.org>
Signed-off-by: Michael Adam <obnox at samba.org>
Signed-off-by: Guenther Deschner <gd at samba.org>
---
source3/winbindd/idmap.c | 15 +++++++++++++++
source3/winbindd/winbindd_proto.h | 1 +
2 files changed, 16 insertions(+)
diff --git a/source3/winbindd/idmap.c b/source3/winbindd/idmap.c
index 4012e70..39ee230 100644
--- a/source3/winbindd/idmap.c
+++ b/source3/winbindd/idmap.c
@@ -120,6 +120,21 @@ static bool idmap_init(void)
return true;
}
+bool domain_has_idmap_config(const char *domname)
+{
+ int i;
+
+ idmap_init();
+
+ for (i=0; i<num_domains; i++) {
+ if (strequal(idmap_domains[i]->name, domname)) {
+ return true;
+ }
+ }
+
+ return false;
+}
+
static bool idmap_found_domain_backend(
const char *string, regmatch_t matches[], void *private_data)
{
diff --git a/source3/winbindd/winbindd_proto.h b/source3/winbindd/winbindd_proto.h
index dd389c2..12629ff 100644
--- a/source3/winbindd/winbindd_proto.h
+++ b/source3/winbindd/winbindd_proto.h
@@ -330,6 +330,7 @@ void init_idmap_child(void);
struct winbindd_child *idmap_child(void);
struct idmap_domain *idmap_find_domain_with_sid(const char *domname,
const struct dom_sid *sid);
+bool domain_has_idmap_config(const char *domname);
/* The following definitions come from winbindd/winbindd_locator.c */
--
2.5.0
From 6a8e09b270ce67ab6966df73541d98ed7770076e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=BCnther=20Deschner?= <gd at samba.org>
Date: Thu, 10 Mar 2016 10:39:15 +0100
Subject: [PATCH 2/5] s3:winbindd:idmap_hash: skip domains that already have
their own idmap configuration.
Check if the domain from the list is not already configured to use another idmap
backend. Not checking this makes the idmap_hash module map IDs for *all* domains
implicitly. This is quite dangeorous in multi-idmap-config setups.
Guenther
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11786
Pair-Programmed-With: Michael Adam <obnox at samba.org>
Signed-off-by: Guenther Deschner <gd at samba.org>
Signed-off-by: Michael Adam <obnox at samba.org>
---
source3/winbindd/idmap_hash/idmap_hash.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/source3/winbindd/idmap_hash/idmap_hash.c b/source3/winbindd/idmap_hash/idmap_hash.c
index 51bbf5b..818d102 100644
--- a/source3/winbindd/idmap_hash/idmap_hash.c
+++ b/source3/winbindd/idmap_hash/idmap_hash.c
@@ -137,6 +137,19 @@ static NTSTATUS be_init(struct idmap_domain *dom)
if (is_null_sid(&dom_list[i].sid))
continue;
+
+ /*
+ * Check if the domain from the list is not already configured
+ * to use another idmap backend. Not checking this makes the
+ * idmap_hash module map IDs for *all* domains implicitly. This
+ * is quite dangerous in setups that use multiple idmap
+ * configurations.
+ */
+
+ if (domain_has_idmap_config(dom_list[i].domain_name)) {
+ continue;
+ }
+
if ((hash = hash_domain_sid(&dom_list[i].sid)) == 0)
continue;
--
2.5.0
From 62206e01cc3a9c2accc17d84adaa447a451647fc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=BCnther=20Deschner?= <gd at samba.org>
Date: Thu, 10 Mar 2016 12:21:52 +0100
Subject: [PATCH 3/5] s3:winbindd:idmap: check loadparm in
domain_has_idmap_config() helper as well.
Guenther
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11786
Pair-Programmed-With: Michael Adam <obnox at samba.org>
Signed-off-by: Guenther Deschner <gd at samba.org>
Signed-off-by: Michael Adam <obnox at samba.org>
---
source3/winbindd/idmap.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/source3/winbindd/idmap.c b/source3/winbindd/idmap.c
index 39ee230..faf0df2 100644
--- a/source3/winbindd/idmap.c
+++ b/source3/winbindd/idmap.c
@@ -123,6 +123,9 @@ static bool idmap_init(void)
bool domain_has_idmap_config(const char *domname)
{
int i;
+ char *config_option;
+ const char *range = NULL;
+ const char *backend = NULL;
idmap_init();
@@ -132,6 +135,25 @@ bool domain_has_idmap_config(const char *domname)
}
}
+ /* fallback: also check loadparm */
+
+ config_option = talloc_asprintf(talloc_tos(), "idmap config %s",
+ domname);
+ if (config_option == NULL) {
+ DEBUG(0, ("out of memory\n"));
+ return false;
+ }
+
+ range = lp_parm_const_string(-1, config_option, "range", NULL);
+ backend = lp_parm_const_string(-1, config_option, "backend", NULL);
+ if (range != NULL && backend != NULL) {
+ DEBUG(5, ("idmap configuration specified for domain '%s'\n",
+ domname));
+ TALLOC_FREE(config_option);
+ return true;
+ }
+
+ TALLOC_FREE(config_option);
return false;
}
--
2.5.0
From fb5b02de0d56b2aaeb45d527f98362d685b90a4c Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Mon, 14 Mar 2016 17:06:34 +0100
Subject: [PATCH 4/5] idmap_hash: rename be_init() --> idmap_hash_initialize()
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11786
Pair-Programmed-With: Guenther Deschner <gd at samba.org>
Signed-off-by: Michael Adam <obnox at samba.org>
Signed-off-by: Guenther Deschner <gd at samba.org>
---
source3/winbindd/idmap_hash/idmap_hash.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/source3/winbindd/idmap_hash/idmap_hash.c b/source3/winbindd/idmap_hash/idmap_hash.c
index 818d102..ed9cc20 100644
--- a/source3/winbindd/idmap_hash/idmap_hash.c
+++ b/source3/winbindd/idmap_hash/idmap_hash.c
@@ -104,7 +104,7 @@ static void separate_hashes(uint32_t id,
/*********************************************************************
********************************************************************/
-static NTSTATUS be_init(struct idmap_domain *dom)
+static NTSTATUS idmap_hash_initialize(struct idmap_domain *dom)
{
struct sid_hash_table *hashed_domains;
NTSTATUS nt_status = NT_STATUS_UNSUCCESSFUL;
@@ -153,10 +153,10 @@ static NTSTATUS be_init(struct idmap_domain *dom)
if ((hash = hash_domain_sid(&dom_list[i].sid)) == 0)
continue;
- DEBUG(5,("hash:be_init() Adding %s (%s) -> %d\n",
+ DBG_INFO("Adding %s (%s) -> %d\n",
dom_list[i].domain_name,
sid_string_dbg(&dom_list[i].sid),
- hash));
+ hash);
hashed_domains[hash].sid = talloc(hashed_domains, struct dom_sid);
sid_copy(hashed_domains[hash].sid, &dom_list[i].sid);
@@ -189,7 +189,7 @@ static NTSTATUS unixids_to_sids(struct idmap_domain *dom,
ids[i]->status = ID_UNKNOWN;
}
- nt_status = be_init(dom);
+ nt_status = idmap_hash_initialize(dom);
BAIL_ON_NTSTATUS_ERROR(nt_status);
for (i=0; ids[i]; i++) {
@@ -239,7 +239,7 @@ static NTSTATUS sids_to_unixids(struct idmap_domain *dom,
ids[i]->status = ID_UNKNOWN;
}
- nt_status = be_init(dom);
+ nt_status = idmap_hash_initialize(dom);
BAIL_ON_NTSTATUS_ERROR(nt_status);
for (i=0; ids[i]; i++) {
@@ -360,7 +360,7 @@ static NTSTATUS nss_hash_close(void)
********************************************************************/
static struct idmap_methods hash_idmap_methods = {
- .init = be_init,
+ .init = idmap_hash_initialize,
.unixids_to_sids = unixids_to_sids,
.sids_to_unixids = sids_to_unixids,
};
--
2.5.0
From bb6e04c37e444e588462546fc0c2dbb615283b8a Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Mon, 14 Mar 2016 17:07:34 +0100
Subject: [PATCH 5/5] idmap_hash: only allow the hash module for default idmap
config.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11786
This module only makes sense as the default idmap config
("idmap config * : backend = hash" ...)
Pair-Programmed-With: Guenther Deschner <gd at samba.org>
Signed-off-by: Michael Adam <obnox at samba.org>
Signed-off-by: Guenther Deschner <gd at samba.org>
---
source3/winbindd/idmap_hash/idmap_hash.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/source3/winbindd/idmap_hash/idmap_hash.c b/source3/winbindd/idmap_hash/idmap_hash.c
index ed9cc20..0aba36c 100644
--- a/source3/winbindd/idmap_hash/idmap_hash.c
+++ b/source3/winbindd/idmap_hash/idmap_hash.c
@@ -112,6 +112,13 @@ static NTSTATUS idmap_hash_initialize(struct idmap_domain *dom)
size_t num_domains = 0;
int i;
+ if (!strequal(dom->name, "*")) {
+ DBG_ERR("Error: idmap_hash configured for domain '%s'. "
+ "But the hash module can only be used for the default "
+ "idmap configuration.\n", dom->name);
+ return NT_STATUS_INVALID_PARAMETER;
+ }
+
/* If the domain SID hash table has been initialized, assume
that we completed this function previously */
--
2.5.0
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160316/e9a212bd/signature.sig>
More information about the samba-technical
mailing list