[PATCH] Fix configuration reload in s3/loadparm
Ralph Böhme
slow at samba.org
Wed Nov 22 11:46:03 UTC 2017
Hi!
Attached patchset fixes reloading config in s3/loadparm. This popped up when
a user went through the following config changes:
1. Set [global] smb encrypt = required
2. Restart Samba
3. Remove the line smb encrypt = required
4. smbcontrol smbd reload-config
Expected result: unencryptd access allowed.
Actual result: unencryptd access denied.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13051
Took me some time to figure out, that this happens because in s3/loadparm we
have the defaults in the sDefault struct and any service option appearing in the
global section modifies it. As we have no copy of the original value, if the
user removes the config setting and reloads, the original value can't be
restored and will still be what was initially set. *ouch*
It looks like we have the same problem in the toplevel loadparm, but afacit all
the stuff that uses it doesn't support configuration reload, so I'm leaving that
as is.
Please review & push if happy. Thanks!
-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
-------------- next part --------------
From bd065e54d0f39fde5cd7bd0b57cf6556070160ee Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 22 Nov 2017 11:49:57 +0100
Subject: [PATCH 1/3] s3/loadparm: allocate a fresh sDefault object per lp_ctx
This is in preperation of preventing direct access to sDefault in all
places that currently modify it.
As currently s3/loadparm is afaict not accessing lp_ctx->sDefault, but
changes sDefault indirectly through lp_parm_ptr() this change is just a
safety measure to prevent future breakage.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13051
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/param/loadparm.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index 485d3f75b04..adf0921b757 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -961,7 +961,13 @@ static struct loadparm_context *setup_lp_context(TALLOC_CTX *mem_ctx)
return NULL;
}
- lp_ctx->sDefault = &sDefault;
+ lp_ctx->sDefault = talloc_zero(lp_ctx, struct loadparm_service);
+ if (lp_ctx->sDefault == NULL) {
+ DBG_ERR("talloc_zero failed\n");
+ return NULL;
+ }
+
+ *lp_ctx->sDefault = sDefault;
lp_ctx->services = NULL; /* We do not want to access this directly */
lp_ctx->bInGlobalSection = bInGlobalSection;
lp_ctx->flags = flags_list;
--
2.13.6
From 3584ff534e3881e04615c7c142fa0b5de271dc4e Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 21 Nov 2017 14:28:48 +0100
Subject: [PATCH 2/3] s3/loadparm: ensure default service options are not
changed
Rename sDefault to _sDefault and make it const. sDefault is make a copy
of _sDefault in in the initialisation function lp_load_ex().
As we may end up in setup_lp_context() without going through
lp_load_ex(), sDefault may still be uninitialized at that point, so I'm
initializing lp_ctx->sDefault from _sDefault.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13051
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/param/loadparm.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index adf0921b757..5a60177a626 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -111,7 +111,7 @@ static bool defaults_saved = false;
static struct loadparm_global Globals;
/* This is a default service used to prime a services structure */
-static struct loadparm_service sDefault =
+static const struct loadparm_service _sDefault =
{
.valid = true,
.autoloaded = false,
@@ -249,6 +249,12 @@ static struct loadparm_service sDefault =
.dummy = ""
};
+/*
+ * This is a copy of the default service structure. Service options in the
+ * global section would otherwise overwrite the initial default values.
+ */
+static struct loadparm_service sDefault;
+
/* local variables */
static struct loadparm_service **ServicePtrs = NULL;
static int iNumServices = 0;
@@ -967,7 +973,7 @@ static struct loadparm_context *setup_lp_context(TALLOC_CTX *mem_ctx)
return NULL;
}
- *lp_ctx->sDefault = sDefault;
+ *lp_ctx->sDefault = _sDefault;
lp_ctx->services = NULL; /* We do not want to access this directly */
lp_ctx->bInGlobalSection = bInGlobalSection;
lp_ctx->flags = flags_list;
@@ -3857,6 +3863,7 @@ static bool lp_load_ex(const char *pszFname,
bInGlobalSection = true;
bGlobalOnly = global_only;
bAllowIncludeRegistry = allow_include_registry;
+ sDefault = _sDefault;
lp_ctx = setup_lp_context(talloc_tos());
--
2.13.6
From 235d06ae1b4cd11e1f086f5810e3140ba14825b7 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 21 Nov 2017 14:34:28 +0100
Subject: [PATCH 3/3] s3/loadparm: don't mark IPC$ as autoloaded
A related problem that affects configuration for the hidden IPC$
share. This share is marked a "autoloaded" and such shares are not
reloaded when requested. That resulted in the tcon to IPC$ still using
encrpytion after running the following sequence of changes:
1. stop Samba
2. set [global] smb encrypt = required
3. start Samba
4. remove [global] smb encrypt = required
5. smbcontrol smbd reload-config
6a bin/smbclient -U slow%x //localhost/raw -c quit, or
6b bin/smbclient -U slow%x -mNT1 //localhost/raw -c ls
In 6a the client simply encrypted packets on the IPC$ tcon. In 6b the
client got a tcon failure with NT_STATUS_ACCESS_DENIED, but silently
ignore the error.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13051
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/param/loadparm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index 5a60177a626..c3f8b8338e5 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -1605,7 +1605,7 @@ static bool lp_add_ipc(const char *ipc_name, bool guest_ok)
ServicePtrs[i]->guest_ok = guest_ok;
ServicePtrs[i]->printable = false;
ServicePtrs[i]->browseable = sDefault.browseable;
- ServicePtrs[i]->autoloaded = true;
+ ServicePtrs[i]->autoloaded = false;
DEBUG(3, ("adding IPC service\n"));
--
2.13.6
More information about the samba-technical
mailing list