From ab79ecf317bb86f5b44886f59f860d86a0aca405 Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 Reviewed-by: Volker Lendecke (cherry picked from commit e7bf94b4e3a7f994aa6f0b859089c5add2ad380f) --- 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 78dd09199aa..785c9d5da13 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 cb485070395a6d1c4d76dbc16d2168a71b5ab596 Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 Reviewed-by: Volker Lendecke (back-ported from commit 963fd8aa9b76361ab9aeb63307773f2498b17879) --- 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 360351336a2..dbf70bbe715 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -203,6 +203,17 @@ plantestsuite("samba3.smbtorture_s3.hidenewfiles(fileserver_smb1)", "", "-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 cd32156ae42..e6b34c4a43c 100644 --- a/source3/torture/torture.c +++ b/source3/torture/torture.c @@ -14638,6 +14638,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) { @@ -14714,6 +14890,7 @@ static bool run_ign_bad_negprot(int dummy) return true; } + static double create_procs(bool (*fn)(int), bool *result) { int i, status; @@ -15366,6 +15543,10 @@ static struct { .name = "OPLOCK-CANCEL", .fn = run_oplock_cancel, }, + { + .name = "SMB1-TRUNCATED-SESSSETUP", + .fn = run_smb1_truncated_sesssetup, + }, { .name = "PIDHIGH", .fn = run_pidhigh, -- 2.34.1 From 4d637a6cf4613b4800f9307143e62b36e719289e Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 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 Reviewed-by: Volker Lendecke (cherry picked from commit 9220c45cc191b34e293190f6a923ba463edd5db9) --- 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 abd9b928b7e..16d132e0acf 100644 --- a/source3/smbd/smb2_reply.c +++ b/source3/smbd/smb2_reply.c @@ -548,6 +548,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 dd1eae67d81b8d3db6911f0fba845eaa90f03f7f Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 Reviewed-by: Volker Lendecke Signed-off-by: Jeremy Allison Reviewed-by: Volker Lendecke (cherry picked from commit 5bc50d2ea4444244721e72b4264311c7005d2f3c) --- 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 785c9d5da13..29302f9c56b 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 c52f2b8542630015e3659100f19ba1766e6ff269 Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 Reviewed-by: Volker Lendecke Autobuild-User(master): Volker Lendecke Autobuild-Date(master): Mon Aug 14 15:55:43 UTC 2023 on atb-devel-224 (cherry picked from commit 5379b8d557a9a16b81eafb87b60b81debc4bfccb) --- 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 1f289e4fc3a..66e58e6c099 100644 --- a/source3/smbd/smb1_ipc.c +++ b/source3/smbd/smb1_ipc.c @@ -688,7 +688,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 6894aa52ec0..edce398dd7e 100644 --- a/source3/smbd/smb1_message.c +++ b/source3/smbd/smb1_message.c @@ -161,7 +161,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 29302f9c56b..a812d375d63 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