Attachment 18045 Details for Bug 15432 – WIP patches for master.

[patch] WIP patches for master.

bug-15432.patch (text/plain), 6.52 KB, created by Jeremy Allison on 2023-08-12 00:32:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2023-08-12 00:32:00 UTC

Size: 6.52 KB

patch

obsolete

>From 8b82a27982333cf31ea4fcff3ac81d2b04b23c12 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Fri, 11 Aug 2023 17:14:38 -0700
>Subject: [PATCH 1/3] s3: smbd: init_smb1_request() isn't being passed zero'ed
> memory from any codepath.
>
>If a client does a SMB1 NEGPROT followed by SMB1 TCON
>then req->session is left uninitialized.
>
>Show this causes a crash be deliberately initializing
>req->session to an invalid pointer. This will be removed
>once the test shows the crash, and the fix is added to
>cause init_smb1_request() to zero the memory passed in.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15432
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source3/smbd/smb2_process.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/source3/smbd/smb2_process.c b/source3/smbd/smb2_process.c
>index 923810eeff6..99e30481491 100644
>--- a/source3/smbd/smb2_process.c
>+++ b/source3/smbd/smb2_process.c
>@@ -760,6 +760,7 @@ bool init_smb1_request(struct smb_request *req,
> 	req->smb2req = NULL;
> 	req->chain = NULL;
> 	req->posix_pathnames = lp_posix_pathnames();
>+	req->session = (void *)0xDEADBEEF;
> 
> 	/* Ensure we have at least wct words and 2 bytes of bcc. */
> 	if (smb_size + req->wct*2 > req_size) {
>-- 
>2.34.1
>
>
>From 4d86874931c799771012b84cbb65876508a9ff26 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Fri, 11 Aug 2023 17:18:26 -0700
>Subject: [PATCH 2/3] s3: torture: Add SMB1-NEGOTIATE-TCON that shows the SMB1
> server crashes on the uninitialized req->session.
>
>Found by Robert Morris <rtm@lcs.mit.edu>.
>
>Adds knownfail.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15432
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> selftest/knownfail.d/smb1_negprot_tcon |  1 +
> source3/selftest/tests.py              | 11 +++++++
> source3/torture/torture.c              | 40 ++++++++++++++++++++++++++
> 3 files changed, 52 insertions(+)
> create mode 100644 selftest/knownfail.d/smb1_negprot_tcon
>
>diff --git a/selftest/knownfail.d/smb1_negprot_tcon b/selftest/knownfail.d/smb1_negprot_tcon
>new file mode 100644
>index 00000000000..4f620948c31
>--- /dev/null
>+++ b/selftest/knownfail.d/smb1_negprot_tcon
>@@ -0,0 +1 @@
>+^samba3.smbtorture_s3.smb1.SMB1-NEGOTIATE-TCON.smbtorture$fileserver_smb1$
>diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
>index 8c9ed71c6f7..9d77c49ed48 100755
>--- a/source3/selftest/tests.py
>+++ b/source3/selftest/tests.py
>@@ -241,6 +241,17 @@ plantestsuite("samba3.smbtorture_s3.smb1.SMB1-NEGOTIATE-EXIT",
>                 smbtorture3,
>                 "-mNT1"])
> 
>+plantestsuite("samba3.smbtorture_s3.smb1.SMB1-NEGOTIATE-TCON",
>+                "fileserver_smb1",
>+                [os.path.join(samba3srcdir,
>+                              "script/tests/test_smbtorture_s3.sh"),
>+                'SMB1-NEGOTIATE-TCON',
>+                '//$SERVER_IP/tmp',
>+                '$USERNAME',
>+                '$PASSWORD',
>+                smbtorture3,
>+                "-mNT1"])
>+
> #
> # MSDFS attribute tests.
> #
>diff --git a/source3/torture/torture.c b/source3/torture/torture.c
>index 1e496e74456..2a8b8dec22f 100644
>--- a/source3/torture/torture.c
>+++ b/source3/torture/torture.c
>@@ -14999,6 +14999,42 @@ static bool run_smb1_negotiate_exit(int dummy)
> 	return true;
> }
> 
>+static bool run_smb1_negotiate_tcon(int dummy)
>+{
>+	struct cli_state *cli = NULL;
>+	uint16_t cnum = 0;
>+	uint16_t max_xmit = 0;
>+	NTSTATUS status;
>+
>+	printf("Starting send SMB1 negotiate+tcon.\n");
>+	cli = open_nbt_connection();
>+	if (cli == NULL) {
>+		d_fprintf(stderr, "open_nbt_connection failed!\n");
>+		return false;
>+	}
>+	smbXcli_conn_set_sockopt(cli->conn, sockops);
>+
>+	status = smbXcli_negprot(cli->conn, 0, PROTOCOL_NT1, PROTOCOL_NT1);
>+	if (!NT_STATUS_IS_OK(status)) {
>+		d_fprintf(stderr, "smbXcli_negprot failed %s!\n",
>+			nt_errstr(status));
>+		return false;
>+	}
>+        status = cli_raw_tcon(cli,
>+			      share,
>+			      "",
>+			      "?????",
>+			      &max_xmit,
>+			      &cnum);
>+	if (!NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED)) {
>+		d_fprintf(stderr, "cli_raw_tcon failed - got %s "
>+			"(should get NT_STATUS_ACCESS_DENIED)!\n",
>+			nt_errstr(status));
>+		return false;
>+	}
>+	return true;
>+}
>+
> static bool run_ign_bad_negprot(int dummy)
> {
> 	struct tevent_context *ev;
>@@ -15735,6 +15771,10 @@ static struct {
> 		.name  = "SMB1-NEGOTIATE-EXIT",
> 		.fn    = run_smb1_negotiate_exit,
> 	},
>+	{
>+		.name  = "SMB1-NEGOTIATE-TCON",
>+		.fn    = run_smb1_negotiate_tcon,
>+	},
> 	{
> 		.name  = "SMB1-DFS-PATHS",
> 		.fn    = run_smb1_dfs_paths,
>-- 
>2.34.1
>
>
>From 243bfc2d6d3a884129c35c1735693cae45d0fc99 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Fri, 11 Aug 2023 17:28:53 -0700
>Subject: [PATCH 3/3] s3: smbd: Ensure init_smb1_request() calls ZERO_STRUCTP
> on incoming pointer.
>
>Remove the now unneeded req->xxx = NULL assignments (and the
>deliberately bogus req->session = (void *)0xDEADBEEF one
>used to demonstrate the bug).
>
>Remove knownfail.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15432
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> selftest/knownfail.d/smb1_negprot_tcon | 1 -
> source3/smbd/smb2_process.c            | 8 ++------
> 2 files changed, 2 insertions(+), 7 deletions(-)
> delete mode 100644 selftest/knownfail.d/smb1_negprot_tcon
>
>diff --git a/selftest/knownfail.d/smb1_negprot_tcon b/selftest/knownfail.d/smb1_negprot_tcon
>deleted file mode 100644
>index 4f620948c31..00000000000
>--- a/selftest/knownfail.d/smb1_negprot_tcon
>+++ /dev/null
>@@ -1 +0,0 @@
>-^samba3.smbtorture_s3.smb1.SMB1-NEGOTIATE-TCON.smbtorture$fileserver_smb1$
>diff --git a/source3/smbd/smb2_process.c b/source3/smbd/smb2_process.c
>index 99e30481491..8bbca1815b0 100644
>--- a/source3/smbd/smb2_process.c
>+++ b/source3/smbd/smb2_process.c
>@@ -731,6 +731,8 @@ bool init_smb1_request(struct smb_request *req,
> 		return false;
> 	}
> 
>+	ZERO_STRUCTP(req);
>+
> 	req->request_time = timeval_current();
> 	now = timeval_to_nttime(&req->request_time);
> 
>@@ -749,18 +751,13 @@ bool init_smb1_request(struct smb_request *req,
> 	req->encrypted = encrypted;
> 	req->sconn = sconn;
> 	req->xconn = xconn;
>-	req->conn = NULL;
> 	if (xconn != NULL) {
> 		status = smb1srv_tcon_lookup(xconn, req->tid, now, &tcon);
> 		if (NT_STATUS_IS_OK(status)) {
> 			req->conn = tcon->compat;
> 		}
> 	}
>-	req->chain_fsp = NULL;
>-	req->smb2req = NULL;
>-	req->chain = NULL;
> 	req->posix_pathnames = lp_posix_pathnames();
>-	req->session = (void *)0xDEADBEEF;
> 
> 	/* Ensure we have at least wct words and 2 bytes of bcc. */
> 	if (smb_size + req->wct*2 > req_size) {
>@@ -779,7 +776,6 @@ bool init_smb1_request(struct smb_request *req,
> 		return false;
> 	}
> 
>-	req->outbuf = NULL;
> 	return true;
> }
> 
>-- 
>2.34.1
>

Actions: View

Attachments on bug 15432: 17997 | 18045 | 18054 | 18055 | 18056