[PATCH] Fix configuration reload in s3/loadparm

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix configuration reload in s3/loadparm

Samba - samba-technical mailing list
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/

bug13051-master.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix configuration reload in s3/loadparm

Samba - samba-technical mailing list
On Wed, Nov 22, 2017 at 12:46:03PM +0100, Ralph Böhme via samba-technical wrote:

> 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!

Happy. Pushed. Thanks, Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix configuration reload in s3/loadparm

Samba - samba-technical mailing list
On Wed, 2017-11-22 at 13:21 +0100, Volker Lendecke via samba-technical
wrote:

> On Wed, Nov 22, 2017 at 12:46:03PM +0100, Ralph Böhme via samba-
> technical wrote:
> > 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!
>
> Happy. Pushed. Thanks, Volker
>
Volker, Ralph

I'm new to the project so please excuse me if the following is total rubbish.
I was wondering if the code shouldn't talloc_free(lp_ctx) in case of an error for lp_ctx->sDefault = taloc_zero(...)
So like this

+ lp_ctx->sDefault = talloc_zero(lp_ctx, struct loadparm_service);
+ if (lp_ctx->sDefault == NULL) {
+ DBG_ERR("talloc_zero failed\n");
+ TALLOC_FREE(lp_ctx);
+ return NULL;
+ }

I'm not concerned about the first 2 caller lp_do_parameter(), lp_set_cmdline()
where TALLOC_FREE() is called if NULL is returned by setup_lp_context(),
but I'm not really sure about the next two and especially about dump_a_parameter()
where the memory for lp_ctx might sit there for ages.

Cheers Swen


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix configuration reload in s3/loadparm

Samba - samba-technical mailing list
Cancelled the autobuild.

Volker

On Wed, Nov 22, 2017 at 02:17:05PM +0100, Swen Schillig wrote:

> On Wed, 2017-11-22 at 13:21 +0100, Volker Lendecke via samba-technical
> wrote:
> > On Wed, Nov 22, 2017 at 12:46:03PM +0100, Ralph Böhme via samba-
> > technical wrote:
> > > 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!
> >
> > Happy. Pushed. Thanks, Volker
> >
> Volker, Ralph
>
> I'm new to the project so please excuse me if the following is total rubbish.
> I was wondering if the code shouldn't talloc_free(lp_ctx) in case of an error for lp_ctx->sDefault = taloc_zero(...)
> So like this
>
> + lp_ctx->sDefault = talloc_zero(lp_ctx, struct loadparm_service);
> + if (lp_ctx->sDefault == NULL) {
> + DBG_ERR("talloc_zero failed\n");
> + TALLOC_FREE(lp_ctx);
> + return NULL;
> + }
>
> I'm not concerned about the first 2 caller lp_do_parameter(), lp_set_cmdline()
> where TALLOC_FREE() is called if NULL is returned by setup_lp_context(),
> but I'm not really sure about the next two and especially about dump_a_parameter()
> where the memory for lp_ctx might sit there for ages.
>
> Cheers Swen
>

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix configuration reload in s3/loadparm

Samba - samba-technical mailing list


On Wed, Nov 22, 2017 at 02:31:33PM +0100, Volker Lendecke wrote:

> On Wed, Nov 22, 2017 at 02:17:05PM +0100, Swen Schillig wrote:
> > Volker, Ralph
> >
> > I'm new to the project so please excuse me if the following is total rubbish.
> > I was wondering if the code shouldn't talloc_free(lp_ctx) in case of an
> > error for lp_ctx->sDefault = taloc_zero(...)
> >
> > So like this
> >
> > + lp_ctx->sDefault = talloc_zero(lp_ctx, struct loadparm_service);
> > + if (lp_ctx->sDefault == NULL) {
> > + DBG_ERR("talloc_zero failed\n");
> > + TALLOC_FREE(lp_ctx);
> > + return NULL;
> > + }
good catch, thanks!

> Cancelled the autobuild.

Thanks! Attached is an updated patchset with the missing TALLOC_FREE(lp_ctx).

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

bug13051-master.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix configuration reload in s3/loadparm

Samba - samba-technical mailing list
On Wed, Nov 22, 2017 at 02:43:17PM +0100, Ralph Böhme via samba-technical wrote:

>
>
> On Wed, Nov 22, 2017 at 02:31:33PM +0100, Volker Lendecke wrote:
> > On Wed, Nov 22, 2017 at 02:17:05PM +0100, Swen Schillig wrote:
> > > Volker, Ralph
> > >
> > > I'm new to the project so please excuse me if the following is total rubbish.
> > > I was wondering if the code shouldn't talloc_free(lp_ctx) in case of an
> > > error for lp_ctx->sDefault = taloc_zero(...)
> > >
> > > So like this
> > >
> > > + lp_ctx->sDefault = talloc_zero(lp_ctx, struct loadparm_service);
> > > + if (lp_ctx->sDefault == NULL) {
> > > + DBG_ERR("talloc_zero failed\n");
> > > + TALLOC_FREE(lp_ctx);
> > > + return NULL;
> > > + }
>
> good catch, thanks!
>
> > Cancelled the autobuild.
>
> Thanks! Attached is an updated patchset with the missing TALLOC_FREE(lp_ctx).

Thanks for the update. LGTM. Pushed, thanks !

Jeremy.

> --
> Ralph Boehme, Samba Team       https://samba.org/
> Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

> From 6a75793debf90d7f9467877bddd7763043711849 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> 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 <[hidden email]>
> ---
>  source3/param/loadparm.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
> index 485d3f75b04..f09ef42dae6 100644
> --- a/source3/param/loadparm.c
> +++ b/source3/param/loadparm.c
> @@ -961,7 +961,14 @@ 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");
> + TALLOC_FREE(lp_ctx);
> + 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 db9b859340c7b84cbd4044faea415ec631ea971d Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> 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 <[hidden email]>
> ---
>  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 f09ef42dae6..cf938a60a75 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;
> @@ -968,7 +974,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;
> @@ -3858,6 +3864,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 29313b8f2956b39bb0377f1cf0ff4a69da38523b Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> 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 <[hidden email]>
> ---
>  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 cf938a60a75..c18e9c8e9e4 100644
> --- a/source3/param/loadparm.c
> +++ b/source3/param/loadparm.c
> @@ -1606,7 +1606,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
>