[PATCH] Patch for bug 12520
Jeremy Allison
jra at samba.org
Thu Jan 26 22:48:37 UTC 2017
On Wed, Jan 25, 2017 at 12:13:22PM +0100, Ralph Böhme wrote:
> On Tue, Jan 24, 2017 at 02:48:11PM -0800, Jeremy Allison wrote:
> > On Thu, Jan 19, 2017 at 08:28:46AM +0100, Ralph Böhme wrote:
> > > 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 ?
>
> oh, right, this hunk is wrong, sorry for that!
>
> Iirc I had put that check there initially just after the test unveiled that the
> smb1 code has the same bug as the smb3 one. I then later realized that the
> proper fix is the check added in the other hunk (in make_connection_snum()) and
> then seem to have overlooked removing the other check in the end.
>
> Please review & push if happy. Thanks!
OK, went through these carefully and it looks good to me.
Reviewed-by: Jeremy Allison <jra at samba.org>
As this is changing server encryption (which has been a
source of issues in the past :-) can you run this past
metze for a quick sanity check before pushing ?
If he gives a +1 either you or I can push.
Cheers,
Jeremy.
> From 7f6c5c6d2c59f1a9e80f2d105e7add5ab85a8aac 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/service.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/source3/smbd/service.c b/source3/smbd/service.c
> index 3308e9d..22cc0af 100644
> --- a/source3/smbd/service.c
> +++ b/source3/smbd/service.c
> @@ -623,6 +623,18 @@ static NTSTATUS make_connection_snum(struct smbXsrv_connection *xconn,
> conn->short_case_preserve = lp_short_preserve_case(snum);
>
> conn->encrypt_level = lp_smb_encrypt(snum);
> + if (conn->encrypt_level > SMB_SIGNING_OFF) {
> + if (lp_smb_encrypt(-1) == SMB_SIGNING_OFF) {
> + if (conn->encrypt_level == SMB_SIGNING_REQUIRED) {
> + DBG_ERR("Service [%s] requires encrpytion, but "
> + "it is disabled globally!\n",
> + lp_servicename(talloc_tos(), snum));
> + status = NT_STATUS_ACCESS_DENIED;
> + goto err_root_exit;
> + }
> + conn->encrypt_level = SMB_SIGNING_OFF;
> + }
> + }
>
> conn->veto_list = NULL;
> conn->hide_list = NULL;
> --
> 2.7.4
>
>
> From 46fdb9d8d89546bdce875ecc02829fa85b140529 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Thu, 5 Jan 2017 12:14:35 +0100
> Subject: [PATCH 2/6] s3/smbd: ensure global "smb encrypt = off" is effective
> for SMB 3.1.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]
> smb encrypt = required
>
> must be to completely deny access to the share "share".
>
> This was working correctly for clients when using SMB 3 dialects <
> 3.1.1, but not for 3.1.1 with a negprot encryption context.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12520
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/smbd/smb2_negprot.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source3/smbd/smb2_negprot.c b/source3/smbd/smb2_negprot.c
> index 6cfa64f..d9ccdbe 100644
> --- a/source3/smbd/smb2_negprot.c
> +++ b/source3/smbd/smb2_negprot.c
> @@ -441,7 +441,7 @@ NTSTATUS smbd_smb2_request_process_negprot(struct smbd_smb2_request *req)
> req->preauth = &req->xconn->smb2.preauth;
> }
>
> - if (in_cipher != NULL) {
> + if ((capabilities & SMB2_CAP_ENCRYPTION) && (in_cipher != NULL)) {
> size_t needed = 2;
> uint16_t cipher_count;
> const uint8_t *p;
> --
> 2.7.4
>
>
> From 03fa02a76b5f1657e368cc4ac84d14527c10caeb Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Mon, 16 Jan 2017 12:56:10 +0100
> Subject: [PATCH 3/6] s3/smbd: ensure global "smb encrypt = off" is effective
> for share with "smb encrypt = desired"
>
> If encryption is disabled globally, per definition we shouldn't allow
> enabling encryption on individual shares.
>
> The behaviour of specifying
>
> [Global]
> smb encrypt = off
>
> [share]
> smb encrypt = desired
>
> must be an unecrypted tree connect to the share "share".
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12520
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/smbd/smb2_tcon.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/source3/smbd/smb2_tcon.c b/source3/smbd/smb2_tcon.c
> index 61e2a36..cc4b6b6 100644
> --- a/source3/smbd/smb2_tcon.c
> +++ b/source3/smbd/smb2_tcon.c
> @@ -268,6 +268,7 @@ static NTSTATUS smbd_smb2_tree_connect(struct smbd_smb2_request *req,
> }
>
> if ((lp_smb_encrypt(snum) >= SMB_SIGNING_DESIRED) &&
> + (conn->smb2.server.cipher != 0) &&
> (conn->smb2.client.capabilities & SMB2_CAP_ENCRYPTION)) {
> encryption_desired = true;
> }
> --
> 2.7.4
>
>
> From f358c73a506a0d92c9a091f750bd2b8fb3984b2d Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Mon, 16 Jan 2017 15:45:32 +0100
> Subject: [PATCH 4/6] docs: impact of a global "smb encrpyt=off" on a share
> with "smb encrpyt=required"
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12520
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> docs-xml/smbdotconf/security/smbencrypt.xml | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/docs-xml/smbdotconf/security/smbencrypt.xml b/docs-xml/smbdotconf/security/smbencrypt.xml
> index 0f08966..32a22cb 100644
> --- a/docs-xml/smbdotconf/security/smbencrypt.xml
> +++ b/docs-xml/smbdotconf/security/smbencrypt.xml
> @@ -180,7 +180,11 @@
> <listitem>
> <para>
> Setting it to <emphasis>off</emphasis> globally will
> - completely disable the encryption feature.
> + completely disable the encryption feature for all
> + connections. Setting <parameter>smb encrypt =
> + required</parameter> for individual shares (while it's
> + globally off) will deny access to this shares for all
> + clients.
> </para>
> </listitem>
>
> --
> 2.7.4
>
>
> From 644ef0e6fb643138ba4b2895b382d7c70a24c73f Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Tue, 17 Jan 2017 17:23:51 +0100
> Subject: [PATCH 5/6] selftest: disable SMB encryption in simpleserver
> environment
>
> Encryption is currently not tested in this env so we can safely turn it
> off. The next commit will add a blackbox tests that test combinations of
> having encryption globally turned off and enabled (desired/required) on
> a share.
>
> This also adds a new share "enc_desired" with "smb encrypt = desired"
> which will be used by the test in the next commit.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12520
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> selftest/target/Samba3.pm | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
> index 1ae270a..c708527 100755
> --- a/selftest/target/Samba3.pm
> +++ b/selftest/target/Samba3.pm
> @@ -579,6 +579,7 @@ sub setup_simpleserver($$)
> ntlm auth = yes
> vfs objects = xattr_tdb streams_depot time_audit full_audit
> change notify = no
> + smb encrypt = off
>
> full_audit:syslog = no
> full_audit:success = none
> @@ -596,6 +597,11 @@ sub setup_simpleserver($$)
> store dos attributes = yes
> hide files = /hidefile/
> hide dot files = yes
> +
> +[enc_desired]
> + path = $prefix_abs/share
> + vfs objects =
> + smb encrypt = desired
> ";
>
> my $vars = $self->provision($path,
> --
> 2.7.4
>
>
> From 093a56e42083b954ed8de536127d04fcfffaa648 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Wed, 18 Jan 2017 16:23:40 +0100
> Subject: [PATCH 6/6] selftest: add test for global "smb encrypt=off"
>
> Test various combinations of having encryption globally turned off and
> enabled (desired/required) on a share, with SMB1 UNIX Extensions and SMB3.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12520
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> .../script/tests/test_smbclient_encryption_off.sh | 47 ++++++++++++++++++++++
> source3/selftest/tests.py | 5 +++
> 2 files changed, 52 insertions(+)
> create mode 100755 source3/script/tests/test_smbclient_encryption_off.sh
>
> diff --git a/source3/script/tests/test_smbclient_encryption_off.sh b/source3/script/tests/test_smbclient_encryption_off.sh
> new file mode 100755
> index 0000000..6fdfac5
> --- /dev/null
> +++ b/source3/script/tests/test_smbclient_encryption_off.sh
> @@ -0,0 +1,47 @@
> +#!/bin/sh
> +
> +if [ $# -lt 4 ]; then
> +cat <<EOF
> +Usage: test_smbclient_encryption_off.sh USERNAME PASSWORD SERVER SMBCLIENT
> +EOF
> +exit 1;
> +fi
> +
> +USERNAME="$1"
> +PASSWORD="$2"
> +SERVER="$3"
> +SMBCLIENT="$VALGRIND $4"
> +
> +incdir=`dirname $0`/../../../testprogs/blackbox
> +. $incdir/subunit.sh
> +
> +failed=0
> +
> +#
> +# Let me introduce you to the shares used in this test:
> +#
> +# "tmp" has the default "smb encrpyt" (which is "enabled")
> +# "tmpenc" has "smb encrpyt = required"
> +# "enc_desired" has "smb encrpyt = desired"
> +#
> +
> +# These tests must fail, as encrpytion is globally off and in combination with "smb
> +# encrypt=required" on the share "tmpenc" the server *must* reject the tcon.
> +
> +# SMB1
> +testit_expect_failure "smbclient //$SERVER/tmpenc" $SMBCLIENT -U $USERNAME%$PASSWORD //$SERVER/tmpenc -c quit && failed=`expr $failed + 1`
> +testit_expect_failure "smbclient -e //$SERVER/tmpenc" $SMBCLIENT -e -U $USERNAME%$PASSWORD //$SERVER/tmpenc -c quit && failed=`expr $failed + 1`
> +# SMB3
> +testit_expect_failure "smbclient -m smb3 //$SERVER/tmpenc" $SMBCLIENT -m smb3 -U $USERNAME%$PASSWORD //$SERVER/tmpenc -c quit && failed=`expr $failed + 1`
> +testit_expect_failure "smbclient -e -m smb3 //$SERVER/tmpenc" $SMBCLIENT -e -m smb3 -U $USERNAME%$PASSWORD //$SERVER/tmpenc -c quit && failed=`expr $failed + 1`
> +
> +# These tests must fail, as the client requires encryption and it's off on the server
> +
> +# SMB1
> +testit_expect_failure "smbclient -e //$SERVER/enc_desired" $SMBCLIENT -e -U $USERNAME%$PASSWORD //$SERVER/enc_desired -c quit && failed=`expr $failed + 1`
> +testit_expect_failure "smbclient -e //$SERVER/tmp" $SMBCLIENT -e -U $USERNAME%$PASSWORD //$SERVER/tmp -c quit && failed=`expr $failed + 1`
> +# SMB3
> +testit_expect_failure "smbclient -e -m smb3 //$SERVER/enc_desired" $SMBCLIENT -e -m smb3 -U $USERNAME%$PASSWORD //$SERVER/enc_desired -c quit && failed=`expr $failed + 1`
> +testit_expect_failure "smbclient -e -m smb3 //$SERVER/tmp" $SMBCLIENT -e -m smb3 -U $USERNAME%$PASSWORD //$SERVER/tmp -c quit && failed=`expr $failed + 1`
> +
> +testok $0 $failed
> diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
> index 37cf1a4..e6ed2ec 100755
> --- a/source3/selftest/tests.py
> +++ b/source3/selftest/tests.py
> @@ -493,6 +493,11 @@ plantestsuite("samba3.blackbox.rpcclient.pw-nt-hash", "simpleserver",
> "$USERNAME", "$PASSWORD", "$SERVER",
> os.path.join(bindir(), "rpcclient")])
>
> +plantestsuite("samba3.blackbox.smbclient.encryption_off", "simpleserver",
> + [os.path.join(samba3srcdir, "script/tests/test_smbclient_encryption_off.sh"),
> + "$USERNAME", "$PASSWORD", "$SERVER",
> + smbclient3])
> +
> options_list = ["", "-e"]
> for options in options_list:
> plantestsuite("samba3.blackbox.smbclient_krb5 old ccache %s" % options, "ktest:local",
> --
> 2.7.4
>
More information about the samba-technical
mailing list