[PATCH] Some minor SMB fixes
Tim Beale
timbeale at catalyst.net.nz
Thu Sep 27 05:35:39 UTC 2018
Thanks. I updated the patches with the Bug number (I forgot earlier).
https://bugzilla.samba.org/show_bug.cgi?id=13621
Found when using the domain backup tool. The real problem behind the bug
is that the backup-tool only supports SMBv1 connections. But this issue
was causing some slightly cryptic errors when the SMB connection failed.
On 27/09/18 17:14, 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).
>>
>> 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.
>
> oh, good catch, thanks! I'll try to look at it tomorrow.
>
> This is the second (major) regression that I become aware of
> introduced by fixing compiler warning ons (iirc) newly introduced
> warning levels. Guess we have to be a little bit more careful here. :)
>
> -slow
>
-------------- next part --------------
From ec33e7f72d9f5deaf53aa3facc2b9d94612d183f 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>
---
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 914f9e2b56b259c68df9e3d19b22c735c7831ceb 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
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 d426c14ecc05e6cbe9df90dce005035ffb9fd6ad 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 negotiate 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>
---
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