[PATCH] Some minor SMB fixes

Tim Beale timbeale at catalyst.net.nz
Thu Sep 27 02:48:44 UTC 2018


The main change (patch #2) is that in Samba v4.9 the smbd server has
started responding incorrectly if it can't negotiate an SMB protocol. In
the SMBnegprot response message, instead of responding with
DialectIndex=0xffff, it now responds with DialectIndex=0 (i.e. a valid
protocol choice). This misleads the client into continuing
(unsuccessfully) to establish the SMB connection. This was introduced by
a compiler warning fix (commit 06940155f31).

The other patches are minor changes to make sure the backup tool always
negotiates a signed SMB connection, and to add a better debug/error
message if an SMB protocol can't be negotiated.

CI link: https://gitlab.com/catalyst-samba/samba/pipelines/31315445

Review appreciated.

Thanks.

-------------- next part --------------
From a3e3f0017be09b46be83ae204bb48658f7450819 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 26 Sep 2018 17:01:03 +1200
Subject: [PATCH 1/3] netcmd: Make sure SMB connection is signed when backing
 up sysvol

i.e. protect the client against man-in-the-middle attacks by default.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 python/samba/netcmd/domain_backup.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py
index 5ddc1c1..cc90ed8 100644
--- a/python/samba/netcmd/domain_backup.py
+++ b/python/samba/netcmd/domain_backup.py
@@ -245,7 +245,7 @@ class cmd_domain_backup_online(samba.netcmd.Command):
 
         # Grab the remote DC's sysvol files and bundle them into a tar file
         sysvol_tar = os.path.join(tmpdir, 'sysvol.tar.gz')
-        smb_conn = smb.SMB(server, "sysvol", lp=lp, creds=creds)
+        smb_conn = smb.SMB(server, "sysvol", lp=lp, creds=creds, sign=True)
         backup_online(smb_conn, sysvol_tar, remote_sam.get_domain_sid())
 
         # remove the default sysvol files created by the clone (we want to
@@ -768,7 +768,7 @@ class cmd_domain_backup_rename(samba.netcmd.Command):
         # use the old realm) backed here, as well as default files generated
         # for the new realm as part of the clone/join.
         sysvol_tar = os.path.join(tmpdir, 'sysvol.tar.gz')
-        smb_conn = smb.SMB(server, "sysvol", lp=lp, creds=creds)
+        smb_conn = smb.SMB(server, "sysvol", lp=lp, creds=creds, sign=True)
         backup_online(smb_conn, sysvol_tar, remote_sam.get_domain_sid())
 
         # connect to the local DB (making sure we use the new/renamed config)
-- 
2.7.4


From ad92c9ebb8fdcdc5b265690556328909f29d2aa5 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 27 Sep 2018 09:46:41 +1200
Subject: [PATCH 2/3] s3/smbd: Server responds incorrectly if no SMB protocol
 chosen

The SMBnegprot response from the server contains the DialectIndex of the
selected protocol from the client's request message. Currently, if no
protocol is selected, the server is responding with a DialectIndex=zero,
which is a valid index (PROTOCOL_CORE by default). The Windows spec, and
historically the code, should return DialectIndex=0xffff if no protocol
is chosen. The following commit changed it recently (presumably
inadvertently), so that it now returns DialectIndex=zero.

06940155f315529c5b5 s3:smbd: Fix size types in reply_negprot()

This results in somewhat confusing error messages on the client side:
ERROR(runtime): uncaught exception - (3221225997, 'The transport
connection has been reset.')

or, when signing is configured as mandatory:
smbXcli_negprot: SMB signing is mandatory and the selected protocol
level (1) doesn't support it.
ERROR(runtime): uncaught exception - (3221225506, '{Access Denied} A
process has requested access to an object but has not been granted those
access rights.')

This patch restores the old behaviour of returning 0xffff.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 source3/smbd/negprot.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/source3/smbd/negprot.c b/source3/smbd/negprot.c
index 27366ea..6e4f7c0 100644
--- a/source3/smbd/negprot.c
+++ b/source3/smbd/negprot.c
@@ -28,6 +28,13 @@
 #include "auth/gensec/gensec.h"
 #include "../libcli/smb/smb_signing.h"
 
+/*
+ * MS-CIFS, 2.2.4.52.2 SMB_COM_NEGOTIATE Response:
+ * If the server does not support any of the listed dialects, it MUST return a
+ * DialectIndex of 0XFFFF
+ */
+#define NO_PROTOCOL_CHOSEN	0xffff
+
 extern fstring remote_proto;
 
 static void get_challenge(struct smbXsrv_connection *xconn, uint8_t buff[8])
@@ -557,7 +564,7 @@ static const struct {
 
 void reply_negprot(struct smb_request *req)
 {
-	size_t choice = 0;
+	size_t choice = NO_PROTOCOL_CHOSEN;
 	int chosen_level = -1;
 	bool choice_set = false;
 	int protocol;
-- 
2.7.4


From b1fc092afdfda071361bb44bd17992e70ac67390 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 27 Sep 2018 09:53:24 +1200
Subject: [PATCH 3/3] libcli: Add debug message if fail to negoatiate SMB
 protocol

Currently if the client and server can't negotiate an SMB protocol, you
just get the followiing error on the client-side, which doesn't tell you
much.
ERROR(runtime): uncaught exception - (3221225667, 'The network responded
incorrectly.')

This patch adds a debug message to help highlight what's actually going
wrong.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 libcli/smb/smbXcli_base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c
index ad1b67b..d94b4d8 100644
--- a/libcli/smb/smbXcli_base.c
+++ b/libcli/smb/smbXcli_base.c
@@ -4369,6 +4369,7 @@ static void smbXcli_negprot_smb1_done(struct tevent_req *subreq)
 	}
 
 	if (conn->protocol == PROTOCOL_NONE) {
+		DBG_ERR("No compatible protocol selected by server.\n");
 		tevent_req_nterror(req, NT_STATUS_INVALID_NETWORK_RESPONSE);
 		return;
 	}
-- 
2.7.4



More information about the samba-technical mailing list