Attachment 18044 Details for Bug 15420 – WIP patches for master.

[patch] WIP patches for master.

bug-15420-master (text/plain), 12.49 KB, created by Jeremy Allison on 2023-08-11 22:23:53 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2023-08-11 22:23:53 UTC

Size: 12.49 KB

patch

obsolete

>From e346f0f6543d82b7b6eec89bf634bfc3f1e816ad Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Fri, 11 Aug 2023 10:38:23 -0700
>Subject: [PATCH 1/5] s3: smbd: Deliberately currupt an uninitialized pointer.
>
>We will need this to show smbd crashing in the test code.
>This will be removed once we're passing the test.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15420
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source3/smbd/smb1_sesssetup.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/source3/smbd/smb1_sesssetup.c b/source3/smbd/smb1_sesssetup.c
>index fe4519aef20..fe59a4781fb 100644
>--- a/source3/smbd/smb1_sesssetup.c
>+++ b/source3/smbd/smb1_sesssetup.c
>@@ -581,7 +581,7 @@ void reply_sesssetup_and_X(struct smb_request *req)
> 	struct reply_sesssetup_and_X_state *state = NULL;
> 	uint64_t sess_vuid;
> 	uint16_t smb_bufsize;
>-	char *tmp;
>+	char *tmp = (char *)0xDEADBEEF;
> 	fstring sub_user; /* Sanitised username for substitution */
> 	const char *native_os;
> 	const char *native_lanman;
>-- 
>2.34.1
>
>
>From d79dc9795f3bbce91ac4f059ff89aa34019c50cd Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Fri, 11 Aug 2023 10:39:36 -0700
>Subject: [PATCH 2/5] s3: torture: Add SMB1-TRUNCATED-SESSSETUP test.
>
>Shows that we indirect through an uninitialized pointer and the client crashes
>it's own smbd.
>
>Add knownfail.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15420
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> selftest/knownfail.d/smb1_truncated_sessetup |   1 +
> source3/selftest/tests.py                    |  11 ++
> source3/torture/torture.c                    | 181 +++++++++++++++++++
> 3 files changed, 193 insertions(+)
> create mode 100644 selftest/knownfail.d/smb1_truncated_sessetup
>
>diff --git a/selftest/knownfail.d/smb1_truncated_sessetup b/selftest/knownfail.d/smb1_truncated_sessetup
>new file mode 100644
>index 00000000000..2ecdbd867a9
>--- /dev/null
>+++ b/selftest/knownfail.d/smb1_truncated_sessetup
>@@ -0,0 +1 @@
>+^samba3.smbtorture_s3.smb1.SMB1-TRUNCATED-SESSSETUP.smbtorture$fileserver_smb1$
>diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
>index 461e09be87b..579ed87656d 100755
>--- a/source3/selftest/tests.py
>+++ b/source3/selftest/tests.py
>@@ -219,6 +219,17 @@ plantestsuite("samba3.smbtorture_s3.hidenewfiles_showdirs",
>                "",
>                "-l $LOCAL_PATH"])
> 
>+plantestsuite("samba3.smbtorture_s3.smb1.SMB1-TRUNCATED-SESSSETUP",
>+                "fileserver_smb1",
>+                [os.path.join(samba3srcdir,
>+                              "script/tests/test_smbtorture_s3.sh"),
>+                'SMB1-TRUNCATED-SESSSETUP',
>+                '//$SERVER_IP/tmp',
>+                '$USERNAME',
>+                '$PASSWORD',
>+                smbtorture3,
>+                "-mNT1"])
>+
> #
> # MSDFS attribute tests.
> #
>diff --git a/source3/torture/torture.c b/source3/torture/torture.c
>index ab992665323..59ef401197d 100644
>--- a/source3/torture/torture.c
>+++ b/source3/torture/torture.c
>@@ -14645,6 +14645,182 @@ static bool run_local_canonicalize_path(int dummy)
> 	}
> 	return true;
> }
>+struct session_setup_nt1_truncated_state {
>+	uint16_t vwv[13];
>+	uint8_t bytes[20];
>+};
>+
>+static void smb1_session_setup_nt1_truncated_done(struct tevent_req *subreq);
>+
>+static struct tevent_req *smb1_session_setup_nt1_truncated_send(
>+		TALLOC_CTX *mem_ctx,
>+		struct tevent_context *ev,
>+		struct smbXcli_conn *conn)
>+{
>+	uint16_t *vwv = NULL;
>+	uint8_t *bytes = NULL;
>+	const char *pass = "12345678";
>+	const char *uname = "z";
>+	struct session_setup_nt1_truncated_state *state = NULL;
>+	struct tevent_req *req = NULL;
>+	struct tevent_req *subreq = NULL;
>+
>+	req = tevent_req_create(mem_ctx,
>+				&state,
>+				struct session_setup_nt1_truncated_state);
>+	if (req == NULL) {
>+		return NULL;
>+	}
>+	vwv = &state->vwv[0];
>+	bytes = &state->bytes[0];
>+
>+	SCVAL(vwv+0,  0, 0xff);
>+	SCVAL(vwv+0,  1, 0);
>+	SSVAL(vwv+1,  0, 0);
>+	SSVAL(vwv+2,  0, 8192);
>+	SSVAL(vwv+3,  0, 2);
>+	SSVAL(vwv+4,  0, 1);
>+	SIVAL(vwv+5,  0, 0);
>+	SSVAL(vwv+7,  0, strlen(pass)); /* OEMPasswordLen */
>+	SSVAL(vwv+8,  0, 0); /* UnicodePasswordLen */
>+	SSVAL(vwv+9,  0, 0); /* reserved */
>+	SSVAL(vwv+10, 0, 0); /* reserved */
>+	SIVAL(vwv+11, 0, CAP_STATUS32);
>+
>+	memcpy(bytes, pass, strlen(pass));
>+	bytes += strlen(pass);
>+	memcpy(bytes, uname, strlen(uname)+1);
>+
>+	subreq = smb1cli_req_send(state, ev, conn,
>+				  SMBsesssetupX,
>+				  0, /*  additional_flags */
>+				  0, /*  clear_flags */
>+				  0, /*  additional_flags2 */
>+				  0, /*  clear_flags2 */
>+				  10000, /* timeout_msec */
>+				  getpid(),
>+				  NULL, /* tcon */
>+				  NULL, /* session */
>+				  13, /* wct */
>+				  state->vwv,
>+				  strlen(pass), /* Truncate length at password. */
>+				  state->bytes);
>+	if (tevent_req_nomem(subreq, req)) {
>+		return tevent_req_post(req, ev);
>+	}
>+	tevent_req_set_callback(subreq,
>+				smb1_session_setup_nt1_truncated_done,
>+				req);
>+	return req;
>+}
>+
>+static void smb1_session_setup_nt1_truncated_done(struct tevent_req *subreq)
>+{
>+	struct tevent_req *req =
>+		tevent_req_callback_data(subreq,
>+		struct tevent_req);
>+	struct session_setup_nt1_truncated_state *state =
>+		tevent_req_data(req,
>+		struct session_setup_nt1_truncated_state);
>+	NTSTATUS status;
>+	struct smb1cli_req_expected_response expected[] = {
>+	{
>+		.status = NT_STATUS_OK,
>+		.wct    = 3,
>+	},
>+	};
>+
>+	status = smb1cli_req_recv(subreq, state,
>+				  NULL,
>+				  NULL,
>+				  NULL,
>+				  NULL,
>+				  NULL, /* pvwv_offset */
>+				  NULL,
>+				  NULL,
>+				  NULL, /* pbytes_offset */
>+				  NULL,
>+				  expected, ARRAY_SIZE(expected));
>+	TALLOC_FREE(subreq);
>+	if (tevent_req_nterror(req, status)) {
>+		return;
>+	}
>+	tevent_req_done(req);
>+}
>+
>+static NTSTATUS smb1_session_setup_nt1_truncated_recv(struct tevent_req *req)
>+{
>+	return tevent_req_simple_recv_ntstatus(req);
>+}
>+
>+static bool run_smb1_truncated_sesssetup(int dummy)
>+{
>+	struct tevent_context *ev;
>+	struct tevent_req *req;
>+	struct smbXcli_conn *conn;
>+	struct sockaddr_storage ss;
>+	NTSTATUS status;
>+	int fd;
>+	bool ok;
>+
>+	printf("Starting send truncated SMB1 sesssetup.\n");
>+
>+	ok = resolve_name(host, &ss, 0x20, true);
>+	if (!ok) {
>+		d_fprintf(stderr, "Could not resolve name %s\n", host);
>+		return false;
>+	}
>+
>+	status = open_socket_out(&ss, 445, 10000, &fd);
>+	if (!NT_STATUS_IS_OK(status)) {
>+		d_fprintf(stderr, "open_socket_out failed: %s\n",
>+			  nt_errstr(status));
>+		return false;
>+	}
>+
>+	conn = smbXcli_conn_create(talloc_tos(), fd, host, SMB_SIGNING_OFF, 0,
>+				   NULL, 0, NULL);
>+	if (conn == NULL) {
>+		d_fprintf(stderr, "smbXcli_conn_create failed\n");
>+		return false;
>+	}
>+
>+	status = smbXcli_negprot(conn, 0, PROTOCOL_NT1, PROTOCOL_NT1);
>+	if (!NT_STATUS_IS_OK(status)) {
>+		d_fprintf(stderr, "smbXcli_negprot failed!\n");
>+		return false;
>+	}
>+
>+	ev = samba_tevent_context_init(talloc_tos());
>+	if (ev == NULL) {
>+		d_fprintf(stderr, "samba_tevent_context_init failed\n");
>+		return false;
>+	}
>+
>+	req = smb1_session_setup_nt1_truncated_send(ev, ev, conn);
>+	if (req == NULL) {
>+		d_fprintf(stderr, "smb1_session_setup_nt1_truncated_send failed\n");
>+		return false;
>+	}
>+
>+	ok = tevent_req_poll_ntstatus(req, ev, &status);
>+	if (!ok) {
>+		d_fprintf(stderr, "tevent_req_poll failed with status %s\n",
>+			nt_errstr(status));
>+		return false;
>+	}
>+
>+	status = smb1_session_setup_nt1_truncated_recv(req);
>+	if (!NT_STATUS_IS_OK(status)) {
>+		d_fprintf(stderr, "smb1_session_setup_nt1_truncated_recv returned "
>+			  "%s, expected NT_STATUS_OK\n",
>+			  nt_errstr(status));
>+		return false;
>+	}
>+
>+	TALLOC_FREE(conn);
>+	return true;
>+}
> 
> static bool run_ign_bad_negprot(int dummy)
> {
>@@ -14721,6 +14897,7 @@ static bool run_ign_bad_negprot(int dummy)
> 	return true;
> }
> 
>+
> static double create_procs(bool (*fn)(int), bool *result)
> {
> 	int i, status;
>@@ -15373,6 +15550,10 @@ static struct {
> 		.name  = "SMB2-DFS-FILENAME-LEADING-BACKSLASH",
> 		.fn    = run_smb2_dfs_filename_leading_backslash,
> 	},
>+	{
>+		.name  = "SMB1-TRUNCATED-SESSSETUP",
>+		.fn    = run_smb1_truncated_sesssetup,
>+	},
> 	{
> 		.name  = "SMB1-DFS-PATHS",
> 		.fn    = run_smb1_dfs_paths,
>-- 
>2.34.1
>
>
>From 1c78d7605049afd1cd2bcd70b795155e2ad64c9c Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Fri, 11 Aug 2023 10:42:41 -0700
>Subject: [PATCH 3/5] s3: smbd: Ensure srvstr_pull_req_talloc() always NULLs
> out *dest.
>
>Robert Morris <rtm@lcs.mit.edu> noticed that in the case
>where srvstr_pull_req_talloc() is being called with
>buffer remaining == 0, we don't NULL out the destination
>pointed which is *always* done in the codepaths inside
>pull_string_talloc(). This prevents a crash in the caller.
>
>Remove knownfail.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15420
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> selftest/knownfail.d/smb1_truncated_sessetup | 1 -
> source3/smbd/smb2_reply.c                    | 1 +
> 2 files changed, 1 insertion(+), 1 deletion(-)
> delete mode 100644 selftest/knownfail.d/smb1_truncated_sessetup
>
>diff --git a/selftest/knownfail.d/smb1_truncated_sessetup b/selftest/knownfail.d/smb1_truncated_sessetup
>deleted file mode 100644
>index 2ecdbd867a9..00000000000
>--- a/selftest/knownfail.d/smb1_truncated_sessetup
>+++ /dev/null
>@@ -1 +0,0 @@
>-^samba3.smbtorture_s3.smb1.SMB1-TRUNCATED-SESSSETUP.smbtorture$fileserver_smb1$
>diff --git a/source3/smbd/smb2_reply.c b/source3/smbd/smb2_reply.c
>index 66b735e0b75..dfcd05d2cae 100644
>--- a/source3/smbd/smb2_reply.c
>+++ b/source3/smbd/smb2_reply.c
>@@ -533,6 +533,7 @@ size_t srvstr_pull_req_talloc(TALLOC_CTX *ctx, struct smb_request *req,
> 	ssize_t bufrem = smbreq_bufrem(req, src);
> 
> 	if (bufrem == 0) {
>+		*dest = NULL;
> 		return 0;
> 	}
> 
>-- 
>2.34.1
>
>
>From daa669629fd8f0e5acee6a42b1b80b1b21b9ef30 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Fri, 11 Aug 2023 10:47:28 -0700
>Subject: [PATCH 4/5] s3: smbd: Uncorrupt the pointer we were using to prove a
> crash.
>
>Rather than restore to uninitialized, set to NULL as per
>modern coding practices.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15420
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source3/smbd/smb1_sesssetup.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/source3/smbd/smb1_sesssetup.c b/source3/smbd/smb1_sesssetup.c
>index fe59a4781fb..e0c601c34c7 100644
>--- a/source3/smbd/smb1_sesssetup.c
>+++ b/source3/smbd/smb1_sesssetup.c
>@@ -581,7 +581,7 @@ void reply_sesssetup_and_X(struct smb_request *req)
> 	struct reply_sesssetup_and_X_state *state = NULL;
> 	uint64_t sess_vuid;
> 	uint16_t smb_bufsize;
>-	char *tmp = (char *)0xDEADBEEF;
>+	char *tmp = NULL;
> 	fstring sub_user; /* Sanitised username for substitution */
> 	const char *native_os;
> 	const char *native_lanman;
>-- 
>2.34.1
>
>
>From 97cf955b36c51af41437051ffa26204d5e2da776 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Fri, 11 Aug 2023 10:52:31 -0700
>Subject: [PATCH 5/5] s3: smbd: Ensure all callers to srvstr_pull_req_talloc()
> pass a zeroed-out dest pointer.
>
>Now we've fixed srvstr_pull_req_talloc() this isn't
>strictly needed, but ensuring pointers are initialized
>is best practice to avoid future bugs.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15420
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source3/smbd/smb1_ipc.c       | 2 +-
> source3/smbd/smb1_message.c   | 2 +-
> source3/smbd/smb1_sesssetup.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/source3/smbd/smb1_ipc.c b/source3/smbd/smb1_ipc.c
>index 3f9958fece0..716b67b40ea 100644
>--- a/source3/smbd/smb1_ipc.c
>+++ b/source3/smbd/smb1_ipc.c
>@@ -695,7 +695,7 @@ void reply_trans(struct smb_request *req)
> 		return;
> 	}
> 
>-	if ((state = talloc(conn, struct trans_state)) == NULL) {
>+	if ((state = talloc_zero(conn, struct trans_state)) == NULL) {
> 		DEBUG(0, ("talloc failed\n"));
> 		reply_nterror(req, NT_STATUS_NO_MEMORY);
> 		END_PROFILE(SMBtrans);
>diff --git a/source3/smbd/smb1_message.c b/source3/smbd/smb1_message.c
>index 928be77f854..ca7201e2e7f 100644
>--- a/source3/smbd/smb1_message.c
>+++ b/source3/smbd/smb1_message.c
>@@ -159,7 +159,7 @@ void reply_sends(struct smb_request *req)
> 		return;
> 	}
> 
>-	state = talloc(talloc_tos(), struct msg_state);
>+	state = talloc_zero(talloc_tos(), struct msg_state);
> 
> 	p = req->buf + 1;
> 	p += srvstr_pull_req_talloc(
>diff --git a/source3/smbd/smb1_sesssetup.c b/source3/smbd/smb1_sesssetup.c
>index e0c601c34c7..6c668fffa7b 100644
>--- a/source3/smbd/smb1_sesssetup.c
>+++ b/source3/smbd/smb1_sesssetup.c
>@@ -86,7 +86,7 @@ static void reply_sesssetup_and_X_spnego(struct smb_request *req)
> 	DATA_BLOB in_blob;
> 	DATA_BLOB out_blob = data_blob_null;
> 	size_t bufrem;
>-	char *tmp;
>+	char *tmp = NULL;
> 	const char *native_os;
> 	const char *native_lanman;
> 	const char *primary_domain;
>-- 
>2.34.1
>

Actions: View

Attachments on bug 15420: 17984 | 18042 | 18044 | 18046 | 18047