[PATCH] Patch for bug 12520
Ralph Böhme
slow at samba.org
Fri Jan 27 15:33:00 UTC 2017
On Thu, Jan 26, 2017 at 02:48:37PM -0800, Jeremy Allison wrote:
> 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.
guess what happened... :)
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!
Cheerio!
-slow
-------------- next part --------------
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