[PATCH] Patch for bug 12520
Jeremy Allison
jra at samba.org
Fri Jan 27 20:28:02 UTC 2017
On Fri, Jan 27, 2017 at 04:33:00PM +0100, Ralph Böhme wrote:
>
> Attached is a slightly improved patch with the following modifications:
>
> - patch 3/6: simplied check, no need to check client caps, it's sufficient to
> check server.cipher
>
> - path 6/6: test with explicit SMB3 protocol levels 3.1.1 and 3.0.2, add tests
> that demonstrate and test what is expected to work
>
> Please re-review and push happy. Thanks!
Pushed, with the edit:
s/encrpyt/encrypt/g
in the commit messages and comments :-).
I think you have problems with that word Ralph :-).
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.9.3
>
>
> 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.9.3
>
>
> From eac58797bbe411b3b8dcfcda40743de7484d7963 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 | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/source3/smbd/smb2_tcon.c b/source3/smbd/smb2_tcon.c
> index 61e2a36..5330fc3 100644
> --- a/source3/smbd/smb2_tcon.c
> +++ b/source3/smbd/smb2_tcon.c
> @@ -268,7 +268,8 @@ static NTSTATUS smbd_smb2_tree_connect(struct smbd_smb2_request *req,
> }
>
> if ((lp_smb_encrypt(snum) >= SMB_SIGNING_DESIRED) &&
> - (conn->smb2.client.capabilities & SMB2_CAP_ENCRYPTION)) {
> + (conn->smb2.server.cipher != 0))
> + {
> encryption_desired = true;
> }
>
> --
> 2.9.3
>
>
> From d34630a7746f5120dc7ecf86f660fc040c84b32c 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.9.3
>
>
> From 727fa9a39b5fae8e04f1297ffaa1256d5629d853 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.9.3
>
>
> From 6484b4d2368731d8f279bb95017ce29620d6898e 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 | 65 ++++++++++++++++++++++
> source3/selftest/tests.py | 5 ++
> 2 files changed, 70 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..efbbf65
> --- /dev/null
> +++ b/source3/script/tests/test_smbclient_encryption_off.sh
> @@ -0,0 +1,65 @@
> +#!/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"
> +#
> +
> +# Unencrypted connections should work of course, let's test em to be sure...
> +
> +# SMB1
> +testit "smbclient //$SERVER/enc_desired" $SMBCLIENT -U $USERNAME%$PASSWORD //$SERVER/enc_desired -c quit || failed=`expr $failed + 1`
> +testit "smbclient //$SERVER/tmp" $SMBCLIENT -U $USERNAME%$PASSWORD //$SERVER/tmp -c quit || failed=`expr $failed + 1`
> +# SMB3_02
> +testit "smbclient -m smb3_02 //$SERVER/enc_desired" $SMBCLIENT -m smb3_02 -U $USERNAME%$PASSWORD //$SERVER/enc_desired -c quit || failed=`expr $failed + 1`
> +testit "smbclient -m smb3_02 //$SERVER/tmp" $SMBCLIENT -m smb3_02 -U $USERNAME%$PASSWORD //$SERVER/tmp -c quit || failed=`expr $failed + 1`
> +# SMB3_11
> +testit "smbclient -m smb3_11 //$SERVER/enc_desired" $SMBCLIENT -m smb3_11 -U $USERNAME%$PASSWORD //$SERVER/enc_desired -c quit || failed=`expr $failed + 1`
> +testit "smbclient -m smb3_11 //$SERVER/tmp" $SMBCLIENT -m smb3_11 -U $USERNAME%$PASSWORD //$SERVER/tmp -c quit || failed=`expr $failed + 1`
> +
> +# 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_02
> +testit_expect_failure "smbclient -m smb3_02 //$SERVER/tmpenc" $SMBCLIENT -m smb3_02 -U $USERNAME%$PASSWORD //$SERVER/tmpenc -c quit && failed=`expr $failed + 1`
> +testit_expect_failure "smbclient -e -m smb3_02 //$SERVER/tmpenc" $SMBCLIENT -e -m smb3_02 -U $USERNAME%$PASSWORD //$SERVER/tmpenc -c quit && failed=`expr $failed + 1`
> +# SMB3_11
> +testit_expect_failure "smbclient -m smb3_11 //$SERVER/tmpenc" $SMBCLIENT -m smb3_11 -U $USERNAME%$PASSWORD //$SERVER/tmpenc -c quit && failed=`expr $failed + 1`
> +testit_expect_failure "smbclient -e -m smb3_11 //$SERVER/tmpenc" $SMBCLIENT -e -m smb3_11 -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_02
> +testit_expect_failure "smbclient -e -m smb3_02 //$SERVER/enc_desired" $SMBCLIENT -e -m smb3_02 -U $USERNAME%$PASSWORD //$SERVER/enc_desired -c quit && failed=`expr $failed + 1`
> +testit_expect_failure "smbclient -e -m smb3_02 //$SERVER/tmp" $SMBCLIENT -e -m smb3_02 -U $USERNAME%$PASSWORD //$SERVER/tmp -c quit && failed=`expr $failed + 1`
> +# SMB3_11
> +testit_expect_failure "smbclient -e -m smb3_11 //$SERVER/enc_desired" $SMBCLIENT -e -m smb3_11 -U $USERNAME%$PASSWORD //$SERVER/enc_desired -c quit && failed=`expr $failed + 1`
> +testit_expect_failure "smbclient -e -m smb3_11 //$SERVER/tmp" $SMBCLIENT -e -m smb3_11 -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.9.3
>
More information about the samba-technical
mailing list