[PATCH] Patch for bug 12520

[Jeremy Allison]

On Thu, Jan 19, 2017 at 08:28:46AM +0100, Ralph Böhme wrote:
> On Wed, Jan 18, 2017 at 06:41:45AM +1100, Andrew Bartlett wrote:
> > On Mon, 2017-01-16 at 15:54 +0100, Ralph Böhme wrote:
> > > Hi!
> > > 
> > > Attached is a fix for:
> > > <https://bugzilla.samba.org/show_bug.cgi?id=12520>
> > > 
> > > I didn't add a test because that would require an additional server
> > > in selftest
> > > just for that. Shall I?
> > 
> > Given the issue we had around smb signing not being enabled for years
> > in the AD DC because we didn't actually test it correctly in the
> > default, let along all the reasonable configurations, I would really
> > prefer the costs of the extra environments.  Particularly for the
> > standalone fileserver, they are not expensive.
> 
> instead of adding a new env I got away with disabling it in the simpleserver
> env. :)
> 
> The test then pointed me at our SMB1 UNIX extensions encrpytion code that got
> this wrog as well.
> 
> Updated patchset attached. Please review & push if ok. Thanks!
> 
> Cheerio!
> -slow

> From 91d803050bcf1765a496f3e2210dfb796680ebf7 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Wed, 18 Jan 2017 16:19:15 +0100
> Subject: [PATCH 1/6] s3/smbd: ensure global "smb encrypt = off" is effective
>  for SMB 1 clients
> 
> If encryption is disabled globally, per definition we shouldn't allow
> enabling encryption on individual shares.
> 
> The behaviour of setting
> 
> [Global]
>   smb encrypt = off
> 
> [share_required]
>   smb encrypt = required
> 
> [share_desired]
>   smb encrypt = desired
> 
> must be to completely deny access to the share "share_required" and an
> unencrypted connection to "share_desired".
> 
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12520
> 
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
>  source3/smbd/process.c |  4 ++++
>  source3/smbd/service.c | 12 ++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/source3/smbd/process.c b/source3/smbd/process.c
> index 8f097ec..3b6ff4c 100644
> --- a/source3/smbd/process.c
> +++ b/source3/smbd/process.c
> @@ -1636,6 +1636,10 @@ static connection_struct *switch_message(uint8_t type, struct smb_request *req)
>  	/* load service specific parameters */
>  	if (conn) {
>  		if (req->encrypted) {
> +			if (lp_smb_encrypt(-1) == SMB_SIGNING_OFF) {
> +				reply_nterror(req, NT_STATUS_ACCESS_DENIED);
> +				return conn;
> +			}

So do we need the lp_smb_encrypt() call in the above ?

It's executed in every single call, and this is in a
call path that takes a valid conn struct.

Inside make_connection_snum() we have:

conn->encrypt_level = lp_smb_encrypt(snum);

If you want lp_smb_encrypt(-1) == SMB_SIGNING_OFF
to override, is it possible to put the logic inside
make_connection_snum() instead ?

Or am I not understanding what you're fixing here ?