[PATCH] Some minor SMB fixes
Tim Beale
timbeale at catalyst.net.nz
Fri Sep 28 02:44:59 UTC 2018
Yeah, that's a much better idea. I squashed your change into my patch,
updated patch-set attached.
Can I get another reviewer?
Thanks.
On 28/09/18 04:43, Ralph Böhme wrote:
> On Thu, Sep 27, 2018 at 02:48:44PM +1200, Tim Beale via
> samba-technical wrote:
>> 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).
>
> generelly looks good, however, to avoid having this happen again, we
> may want to make the returned choice explicit instead, instead of
> initializing the variable to the error return value and relying on the
> next 30 lines of code before the error return doesn't munge it. What
> do you think?
>
> Patchset with fixup attached. With this change, +1.
>
> -slow
>
-------------- next part --------------
From 4912cad8a4ce2472436b38a4c567af4e9011c710 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.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13621
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
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 9c8457d..683e7d2 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 e53954d2ec9add98e4ee46ec0828e829c447515b 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.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13621
Pair-Programmed-With: Ralph Boehme <slow at samba.org>
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..2d5edf1 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])
@@ -748,7 +755,7 @@ void reply_negprot(struct smb_request *req)
DBG_NOTICE("No protocol supported !\n");
reply_outbuf(req, 1, 0);
- SSVAL(req->outbuf, smb_vwv0, choice);
+ SSVAL(req->outbuf, smb_vwv0, NO_PROTOCOL_CHOSEN);
ok = srv_send_smb(xconn, (char *)req->outbuf,
false, 0, false, NULL);
--
2.7.4
From bd6052013985b7d7e0d0b853a476a721b2218ce9 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.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13621
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
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