From 8ee6dd5cbd6bbbb856ce8e983262d0a3b6c72638 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 16 Jun 2021 10:47:33 -0700 Subject: [PATCH 1/2] s3: torture: Add POSIX-SYMLINK-SETPATHINFO regression test. This ensure we never blunder into indirecting a NULL fsp pointer in the server. Currently this crashes the server in several info levels. Add knownfail. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14742 Back-ported from ac10058d7f6b4605157f508189a448310f5f18da Signed-off-by: Jeremy Allison Reviewed-by: Noel Power --- selftest/knownfail.d/setpathsymlink | 2 + source3/selftest/tests.py | 1 + source3/torture/proto.h | 1 + source3/torture/test_posix.c | 219 ++++++++++++++++++++++++++++ source3/torture/torture.c | 4 + 5 files changed, 227 insertions(+) create mode 100644 selftest/knownfail.d/setpathsymlink diff --git a/selftest/knownfail.d/setpathsymlink b/selftest/knownfail.d/setpathsymlink new file mode 100644 index 00000000000..9d7ded388c2 --- /dev/null +++ b/selftest/knownfail.d/setpathsymlink @@ -0,0 +1,2 @@ +^samba3.smbtorture_s3.crypt.POSIX-SYMLINK-SETPATHINFO.smbtorture\(nt4_dc_smb1\) +^samba3.smbtorture_s3.plain.POSIX-SYMLINK-SETPATHINFO.smbtorture\(nt4_dc_smb1\) diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index 533b58d6d9c..ce450a20ffe 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -247,6 +247,7 @@ posix_tests = ["POSIX", "POSIX-APPEND", "POSIX-SYMLINK-ACL", "POSIX-SYMLINK-EA", "POSIX-LS-SINGLE", "POSIX-READLINK", "POSIX-STAT", + "POSIX-SYMLINK-SETPATHINFO", ] for t in posix_tests: diff --git a/source3/torture/proto.h b/source3/torture/proto.h index 521c662e2fb..e6d00b290a6 100644 --- a/source3/torture/proto.h +++ b/source3/torture/proto.h @@ -90,6 +90,7 @@ bool run_posix_ls_single_test(int dummy); bool run_posix_readlink_test(int dummy); bool run_posix_stat_test(int dummy); bool run_case_insensitive_create(int dummy); +bool run_posix_symlink_setpathinfo_test(int dummy); bool run_nbench2(int dummy); bool run_async_echo(int dummy); diff --git a/source3/torture/test_posix.c b/source3/torture/test_posix.c index 3ccb51d222b..4776e40e0ff 100644 --- a/source3/torture/test_posix.c +++ b/source3/torture/test_posix.c @@ -23,6 +23,7 @@ #include "libsmb/clirap.h" #include "libsmb/proto.h" #include "../libcli/smb/smbXcli_base.h" +#include "trans2.h" extern struct cli_credentials *torture_creds; extern fstring host, workgroup, share, password, username, myname; @@ -734,3 +735,221 @@ out: TALLOC_FREE(frame); return correct; } + +/* List of info levels to try with a POSIX symlink path. */ + +static struct { + uint32_t level; + const char *name; + uint32_t data_len; +} posix_smb1_setpath_array[] = { + { SMB_SET_FILE_UNIX_BASIC, "SMB_SET_FILE_UNIX_BASIC", 100}, + { SMB_SET_FILE_UNIX_INFO2, "SMB_SET_FILE_UNIX_INFO2", 116}, + { SMB_SET_FILE_UNIX_LINK, "SMB_SET_FILE_UNIX_LINK", 8}, + { SMB_SET_FILE_UNIX_HLINK, "SMB_SET_FILE_UNIX_HLINK", 8}, + { SMB_SET_POSIX_ACL, "SMB_SET_POSIX_ACL", 6}, + { SMB_SET_POSIX_LOCK, "SMB_SET_POSIX_LOCK", 24}, + { SMB_INFO_STANDARD, "SMB_INFO_STANDARD", 12}, + { SMB_INFO_SET_EA, "SMB_INFO_SET_EA", 10}, + { SMB_FILE_BASIC_INFORMATION, "SMB_FILE_BASIC_INFORMATION", 36}, + { SMB_SET_FILE_ALLOCATION_INFO, "SMB_SET_FILE_ALLOCATION_INFO", 8}, + { SMB_SET_FILE_END_OF_FILE_INFO,"SMB_SET_FILE_END_OF_FILE_INFO",8}, + { SMB_SET_FILE_DISPOSITION_INFO,"SMB_SET_FILE_DISPOSITION_INFO",1}, + { SMB_FILE_POSITION_INFORMATION,"SMB_FILE_POSITION_INFORMATION",8}, + { SMB_FILE_FULL_EA_INFORMATION, "SMB_FILE_FULL_EA_INFORMATION",10}, + { SMB_FILE_MODE_INFORMATION, "SMB_FILE_MODE_INFORMATION", 4}, + { SMB_FILE_SHORT_NAME_INFORMATION,"SMB_FILE_SHORT_NAME_INFORMATION",12}, + { SMB_FILE_RENAME_INFORMATION,"SMB_FILE_RENAME_INFORMATION", 20}, + { SMB_FILE_LINK_INFORMATION, "SMB_FILE_LINK_INFORMATION", 20}, +}; + +static NTSTATUS do_setpath(TALLOC_CTX *ctx, + struct cli_state *cli_unix, + const char *fname, + size_t i) +{ + NTSTATUS status; + uint8_t *data = NULL; + + data = talloc_zero_array(ctx, + uint8_t, + posix_smb1_setpath_array[i].data_len); + if (data == NULL) { + return NT_STATUS_NO_MEMORY; + } + + status = cli_setpathinfo(cli_unix, + posix_smb1_setpath_array[i].level, + fname, + data, + posix_smb1_setpath_array[i].data_len); + TALLOC_FREE(data); + + /* + * We don't care what came back, so long as the + * server didn't crash. + */ + if (NT_STATUS_EQUAL(status, + NT_STATUS_CONNECTION_DISCONNECTED)) { + printf("cli_setpathinfo info %x (%s) of %s failed" + "error NT_STATUS_CONNECTION_DISCONNECTED\n", + (unsigned int)posix_smb1_setpath_array[i].level, + posix_smb1_setpath_array[i].name, + fname); + return status; + } + + printf("cli_setpathinfo info %x (%s) of %s got %s " + "(this is not an error)\n", + (unsigned int)posix_smb1_setpath_array[i].level, + posix_smb1_setpath_array[i].name, + fname, + nt_errstr(status)); + + return NT_STATUS_OK; +} + +/* + Ensure we can call SMB1 setpathinfo in a symlink, + pointing to a real object or dangling. We mostly + expect errors, but the server must not crash. + */ +bool run_posix_symlink_setpathinfo_test(int dummy) +{ + TALLOC_CTX *frame = NULL; + struct cli_state *cli_unix = NULL; + NTSTATUS status; + uint16_t fnum = (uint16_t)-1; + const char *fname_real = "file_setpath_real"; + const char *fname_real_symlink = "file_real_setpath_symlink"; + const char *nonexist = "nonexist_setpath"; + const char *nonexist_symlink = "dangling_setpath_symlink"; + bool correct = false; + size_t i; + + frame = talloc_stackframe(); + + printf("Starting POSIX-SYMLINK-SETPATHINFO test\n"); + + if (!torture_open_connection(&cli_unix, 0)) { + TALLOC_FREE(frame); + return false; + } + + torture_conn_set_sockopt(cli_unix); + + status = torture_setup_unix_extensions(cli_unix); + if (!NT_STATUS_IS_OK(status)) { + TALLOC_FREE(frame); + return false; + } + + /* Start with a clean slate. */ + cli_unlink(cli_unix, + fname_real, + FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN); + cli_unlink(cli_unix, + fname_real_symlink, + FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN); + cli_unlink(cli_unix, + nonexist, + FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN); + cli_unlink(cli_unix, + nonexist_symlink, + FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN); + cli_posix_unlink(cli_unix, fname_real); + cli_posix_unlink(cli_unix, fname_real_symlink); + cli_posix_unlink(cli_unix, nonexist); + cli_posix_unlink(cli_unix, nonexist_symlink); + + /* Create a real file. */ + status = cli_openx(cli_unix, + fname_real, + O_RDWR|O_CREAT|O_EXCL, + DENY_NONE, + &fnum); + if (!NT_STATUS_IS_OK(status)) { + printf("cli_openx of %s failed (%s)\n", + fname_real, + nt_errstr(status)); + goto out; + } + status = cli_close(cli_unix, fnum); + if (!NT_STATUS_IS_OK(status)) { + printf("cli_close failed %s\n", nt_errstr(status)); + goto out; + } + fnum = (uint16_t)-1; + + /* Create symlink to real target. */ + status = cli_posix_symlink(cli_unix, + fname_real, + fname_real_symlink); + if (!NT_STATUS_IS_OK(status)) { + printf("cli_posix_symlink of %s -> %s failed error %s\n", + fname_real_symlink, + fname_real, + nt_errstr(status)); + goto out; + } + + /* Now create symlink to non-existing target. */ + status = cli_posix_symlink(cli_unix, + nonexist, + nonexist_symlink); + if (!NT_STATUS_IS_OK(status)) { + printf("cli_posix_symlink of %s -> %s failed error %s\n", + nonexist_symlink, + nonexist, + nt_errstr(status)); + goto out; + } + + for (i = 0; i < ARRAY_SIZE(posix_smb1_setpath_array); i++) { + status = do_setpath(frame, + cli_unix, + fname_real_symlink, + i); + if (!NT_STATUS_IS_OK(status)) { + goto out; + } + status = do_setpath(frame, + cli_unix, + nonexist_symlink, + i); + if (!NT_STATUS_IS_OK(status)) { + goto out; + } + } + + printf("POSIX-SYMLINK-SETPATHINFO test passed\n"); + correct = true; + +out: + if (fnum != (uint16_t)-1) { + cli_close(cli_unix, fnum); + } + cli_unlink(cli_unix, + fname_real, + FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN); + cli_unlink(cli_unix, + fname_real_symlink, + FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN); + cli_unlink(cli_unix, + nonexist, + FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN); + cli_unlink(cli_unix, + nonexist_symlink, + FILE_ATTRIBUTE_SYSTEM | FILE_ATTRIBUTE_HIDDEN); + cli_posix_unlink(cli_unix, fname_real); + cli_posix_unlink(cli_unix, fname_real_symlink); + cli_posix_unlink(cli_unix, nonexist); + cli_posix_unlink(cli_unix, nonexist_symlink); + + if (!torture_close_connection(cli_unix)) { + correct = false; + } + + TALLOC_FREE(frame); + return correct; +} diff --git a/source3/torture/torture.c b/source3/torture/torture.c index d1ea9b85a72..a56a73150fb 100644 --- a/source3/torture/torture.c +++ b/source3/torture/torture.c @@ -14967,6 +14967,10 @@ static struct { .name = "ASYNC-ECHO", .fn = run_async_echo, }, + { + .name = "POSIX-SYMLINK-SETPATHINFO", + .fn = run_posix_symlink_setpathinfo_test, + }, { .name = "UID-REGRESSION-TEST", .fn = run_uid_regression_test, -- 2.30.2 From 218786df62642acaeda4fe3e177f517b71937014 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 15 Jun 2021 15:42:33 -0700 Subject: [PATCH 2/2] s3: smbd: Fix smbd crash on dangling symlink with posix connection calling several non-posix info levels. Tidy up fsp == NULL checks. Remove knownfail. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14742 Signed-off-by: Jeremy Allison Reviewed-by: Noel Power Autobuild-User(master): Noel Power Autobuild-Date(master): Wed Jun 16 11:58:00 UTC 2021 on sn-devel-184 (cherry picked from commit 263c95aee38c9198ad9a30c4d960d72f46b7c27a) --- selftest/knownfail.d/setpathsymlink | 2 -- source3/smbd/trans2.c | 14 +++++++++++++- 2 files changed, 13 insertions(+), 3 deletions(-) delete mode 100644 selftest/knownfail.d/setpathsymlink diff --git a/selftest/knownfail.d/setpathsymlink b/selftest/knownfail.d/setpathsymlink deleted file mode 100644 index 9d7ded388c2..00000000000 --- a/selftest/knownfail.d/setpathsymlink +++ /dev/null @@ -1,2 +0,0 @@ -^samba3.smbtorture_s3.crypt.POSIX-SYMLINK-SETPATHINFO.smbtorture\(nt4_dc_smb1\) -^samba3.smbtorture_s3.plain.POSIX-SYMLINK-SETPATHINFO.smbtorture\(nt4_dc_smb1\) diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c index fac45df586e..70a492a96a8 100644 --- a/source3/smbd/trans2.c +++ b/source3/smbd/trans2.c @@ -785,6 +785,10 @@ NTSTATUS set_ea(connection_struct *conn, files_struct *fsp, return NT_STATUS_EAS_NOT_SUPPORTED; } + if (fsp == NULL) { + return NT_STATUS_INVALID_HANDLE; + } + posix_pathnames = (fsp->fsp_name->flags & SMB_FILENAME_POSIX_PATH); status = refuse_symlink(conn, fsp, fsp->fsp_name); @@ -6860,7 +6864,7 @@ static NTSTATUS smb_set_file_full_ea_info(connection_struct *conn, struct ea_list *ea_list = NULL; NTSTATUS status; - if (!fsp) { + if (fsp == NULL) { return NT_STATUS_INVALID_HANDLE; } @@ -7899,6 +7903,10 @@ static NTSTATUS smb_set_file_basic_info(connection_struct *conn, return NT_STATUS_INVALID_PARAMETER; } + if (fsp == NULL) { + return NT_STATUS_INVALID_HANDLE; + } + status = check_access_fsp(fsp, FILE_WRITE_ATTRIBUTES); if (!NT_STATUS_IS_OK(status)) { return status; @@ -7956,6 +7964,10 @@ static NTSTATUS smb_set_info_standard(connection_struct *conn, return NT_STATUS_INVALID_PARAMETER; } + if (fsp == NULL) { + return NT_STATUS_INVALID_HANDLE; + } + /* create time */ ft.create_time = time_t_to_full_timespec(srv_make_unix_date2(pdata)); /* access time */ -- 2.30.2