From 8b82a27982333cf31ea4fcff3ac81d2b04b23c12 Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 --- 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 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 . Adds knownfail. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15432 Signed-off-by: Jeremy Allison --- 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 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 --- 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