[PATCH] Some minor SMB fixes
Ralph Böhme
slow at samba.org
Thu Sep 27 16:43:01 UTC 2018
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
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46
-------------- next part --------------
From 7d810752cb60b7bb1817f6f22e98f4849cc466f5 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/4] 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>
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 9c8457db4b8..683e7d2684b 100644
--- a/python/samba/netcmd/domain_backup.py
+++ b/python/samba/netcmd/domain_backup.py
@@ -245,7 +245,7 @@ def create_log_file(targetdir, lp, backup_type, server, include_secrets,
# 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 @@ def create_log_file(targetdir, lp, backup_type, server, include_secrets,
# 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.13.6
From 6159f7a18b23042a1817b966f7681d4370952c6a 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/4] 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 27366ea0013..6e4f7c0f0aa 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.13.6
From 19da727ad9dceb041a1bfcaf40612d23043ac4a6 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Thu, 27 Sep 2018 09:34:59 -0700
Subject: [PATCH 3/4] FIXUP: explicit NO_PROTOCOL_CHOSEN return
---
source3/smbd/negprot.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/source3/smbd/negprot.c b/source3/smbd/negprot.c
index 6e4f7c0f0aa..3c9f80dfd19 100644
--- a/source3/smbd/negprot.c
+++ b/source3/smbd/negprot.c
@@ -564,7 +564,7 @@ static const struct {
void reply_negprot(struct smb_request *req)
{
- size_t choice = NO_PROTOCOL_CHOSEN;
+ size_t choice;
int chosen_level = -1;
bool choice_set = false;
int protocol;
@@ -755,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.13.6
From 0f49516531803bdf9adf2634c8f58b740987379e 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 4/4] 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>
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 ad1b67b8476..d94b4d87f27 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.13.6
More information about the samba-technical
mailing list