From 38e7f81011990b52e67b025027500e76e083de56 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Tue, 19 Dec 2023 11:11:55 +0100 Subject: [PATCH 1/6] vfs_default: allow disabling /proc/fds and RESOLVE_NO_SYMLINK at compile time This will be used in CI to have a gitlab runner without all modern Linux features we make use of as part of path processing: - O_PATH - openat2() with RESOLVE_NO_SYMLINKS - somehow safely reopen an O_PATH file handle That gives what a classix UNIX like AIX or Solaris offers feature wise. Other OSes support other combinations of those features, but we leave the exersize of possibly adding more runners supporting those combinations to the reader. The following list shows which features are available and used by Samba on a few OSes: | O_PATH | RESOLVE_NO_SYMLINKS | Safe reopen | CI covered --------|----------------|---------------------|---------------------------- | Supported Used | Supported Used | Supported Used | ============================================================================ Linux | + + | + + | + + | + FreeBSD | + + | + [1] - | + [2] - | - AIX | - - | - - | - - | + [1] via open() flag O_RESOLVE_BENEATH [2] via open() flag O_EMPTY_PATH BUG: https://bugzilla.samba.org/show_bug.cgi?id=15549 Signed-off-by: Ralph Boehme Reviewed-by: Volker Lendecke (cherry picked from commit 5c2f96442a25a1725809a28b3719afbc0bd01830) --- source3/modules/vfs_default.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c index 89eec1146d74..218316867b80 100644 --- a/source3/modules/vfs_default.c +++ b/source3/modules/vfs_default.c @@ -52,6 +52,9 @@ static int vfswrap_connect(vfs_handle_struct *handle, const char *service, const bool bval; handle->conn->have_proc_fds = sys_have_proc_fds(); +#ifdef DISABLE_PROC_FDS + handle->conn->have_proc_fds = false; +#endif /* * assume the kernel will support openat2(), @@ -70,6 +73,9 @@ static int vfswrap_connect(vfs_handle_struct *handle, const char *service, const handle->conn->open_how_resolve |= VFS_OPEN_HOW_RESOLVE_NO_SYMLINKS; } +#ifdef DISABLE_VFS_OPEN_HOW_RESOLVE_NO_SYMLINKS + handle->conn->open_how_resolve &= ~VFS_OPEN_HOW_RESOLVE_NO_SYMLINKS; +#endif return 0; /* Return >= 0 for success */ } -- 2.43.0 From 454ab98f24774411ee44a98816b8877381d8755b Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Tue, 19 Dec 2023 11:12:49 +0100 Subject: [PATCH 2/6] CI: disable /proc/fds and RESOLVE_NO_SYMLINK in samba-no-opath-build runner This is a more sensible combination of missing Linux specific features: - O_PATH - openat2() with RESOLVE_NO_SYMLINKS - somehow safely reopen an O_PATH file handle Currently only O_PATH is disabled for these jobs, but that doesn't really match and know OS. The following list shows which features are available and used by Samba on a few OSes: | O_PATH | RESOLVE_NO_SYMLINKS | Safe reopen | CI covered --------|----------------|---------------------|---------------------------- | Supported Used | Supported Used | Supported Used | ============================================================================ Linux | + + | + + | + + | + FreeBSD | + + | + [1] - | + [2] - | - AIX | - - | - - | - - | + So by also disabling RESOLVE_NO_SYMLINKS and Safe Reopen, we cover classic UNIX systems like AIX. [1] via open() flag O_RESOLVE_BENEATH [2] via open() flag O_EMPTY_PATH BUG: https://bugzilla.samba.org/show_bug.cgi?id=15549 Signed-off-by: Ralph Boehme Reviewed-by: Volker Lendecke (cherry picked from commit 62cbe145c7e500c4759ed2005c78bd5056c87f43) --- script/autobuild.py | 2 +- selftest/skip.opath-required | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/script/autobuild.py b/script/autobuild.py index afa757491e06..577eaf13171f 100755 --- a/script/autobuild.py +++ b/script/autobuild.py @@ -288,7 +288,7 @@ tasks = { "samba-no-opath-build": { "git-clone-required": True, "sequence": [ - ("configure", "ADDITIONAL_CFLAGS='-DDISABLE_OPATH=1' ./configure.developer --without-ad-dc " + samba_configure_params), + ("configure", "ADDITIONAL_CFLAGS='-DDISABLE_OPATH=1 -DDISABLE_VFS_OPEN_HOW_RESOLVE_NO_SYMLINKS=1 -DDISABLE_PROC_FDS=1' ./configure.developer --without-ad-dc " + samba_configure_params), ("make", "make -j"), ("check-clean-tree", CLEAN_SOURCE_TREE_CMD), ("chmod-R-a-w", "chmod -R a-w ."), diff --git a/selftest/skip.opath-required b/selftest/skip.opath-required index 0faf0c4bd6c7..05ff76dcbabf 100644 --- a/selftest/skip.opath-required +++ b/selftest/skip.opath-required @@ -7,3 +7,9 @@ # These fail because become_root() doesn't work in make test ^samba3.blackbox.dropbox.* ^samba3.raw.samba3hide.* + +# These don't work without /proc/fd support +^samba3.blackbox.test_symlink_traversal.*\(fileserver\) +^samba3.blackbox.shadow_copy_torture.*\(fileserver\) +^samba3.blackbox.virus_scanner.*\(fileserver:local\) +^samba3.blackbox.shadow_copy2.*\(fileserver.*\) -- 2.43.0 From ea31a9b450a58cbc28bd840684895eaec9b12e0f Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Tue, 2 Jan 2024 12:49:14 +0100 Subject: [PATCH 3/6] smbd: pass symlink target path to safe_symlink_target_path() Moves processing the symlink error response to the caller filename_convert_dirfsp(). Prepares for using this in non_widelink_open(), where it will replace symlink_target_below_conn() with the same functionality. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15549 Signed-off-by: Ralph Boehme Reviewed-by: Volker Lendecke (back-ported from commit 0515dded4ddb49e5570ae7df51126af1a2d643de) --- source3/include/proto.h | 5 +++ source3/smbd/filename.c | 72 +++++++++++++++++++---------------------- 2 files changed, 38 insertions(+), 39 deletions(-) diff --git a/source3/include/proto.h b/source3/include/proto.h index f71796b57ae9..5e1e807b1adc 100644 --- a/source3/include/proto.h +++ b/source3/include/proto.h @@ -718,6 +718,11 @@ struct smb_filename *synthetic_smb_fname(TALLOC_CTX *mem_ctx, const SMB_STRUCT_STAT *psbuf, NTTIME twrp, uint32_t flags); +NTSTATUS safe_symlink_target_path(TALLOC_CTX *mem_ctx, + const char *connectpath, + const char *target, + size_t unparsed, + char **_relative); NTSTATUS filename_convert_dirfsp( TALLOC_CTX *ctx, connection_struct *conn, diff --git a/source3/smbd/filename.c b/source3/smbd/filename.c index dd0239777e89..606959d0805f 100644 --- a/source3/smbd/filename.c +++ b/source3/smbd/filename.c @@ -943,44 +943,34 @@ static char *symlink_target_path( return ret; } -static NTSTATUS safe_symlink_target_path( - TALLOC_CTX *mem_ctx, - const char *connectpath, - const char *name_in, - const char *substitute, - size_t unparsed, - char **_name_out) +NTSTATUS safe_symlink_target_path(TALLOC_CTX *mem_ctx, + const char *connectpath, + const char *target, + size_t unparsed, + char **_relative) { - char *target = NULL; char *abs_target = NULL; char *abs_target_canon = NULL; const char *relative = NULL; - char *name_out = NULL; - NTSTATUS status = NT_STATUS_NO_MEMORY; bool in_share; + NTSTATUS status = NT_STATUS_NO_MEMORY; - target = symlink_target_path(mem_ctx, name_in, substitute, unparsed); - if (target == NULL) { - goto fail; - } - - DBG_DEBUG("name_in: %s, substitute: %s, unparsed: %zu, target=%s\n", - name_in, - substitute, - unparsed, - target); + DBG_DEBUG("connectpath [%s] target [%s] unparsed [%zu]\n", + connectpath, target, unparsed); if (target[0] == '/') { - abs_target = target; + abs_target = talloc_strdup(mem_ctx, target); } else { - abs_target = talloc_asprintf( - target, "%s/%s", connectpath, target); - if (abs_target == NULL) { - goto fail; - } + abs_target = talloc_asprintf(mem_ctx, + "%s/%s", + connectpath, + target); + } + if (abs_target == NULL) { + goto fail; } - abs_target_canon = canonicalize_absolute_path(target, abs_target); + abs_target_canon = canonicalize_absolute_path(abs_target, abs_target); if (abs_target_canon == NULL) { goto fail; } @@ -995,15 +985,14 @@ static NTSTATUS safe_symlink_target_path( goto fail; } - name_out = talloc_strdup(mem_ctx, relative); - if (name_out == NULL) { + *_relative = talloc_strdup(mem_ctx, relative); + if (*_relative == NULL) { goto fail; } status = NT_STATUS_OK; - *_name_out = name_out; fail: - TALLOC_FREE(target); + TALLOC_FREE(abs_target); return status; } @@ -1456,6 +1445,7 @@ NTSTATUS filename_convert_dirfsp( size_t unparsed = 0; NTSTATUS status; char *target = NULL; + char *safe_target = NULL; size_t symlink_redirects = 0; next: @@ -1494,17 +1484,21 @@ NTSTATUS filename_convert_dirfsp( * resolve all symlinks locally. */ - status = safe_symlink_target_path( - mem_ctx, - conn->connectpath, - name_in, - substitute, - unparsed, - &target); + target = symlink_target_path(mem_ctx, name_in, substitute, unparsed); + if (target == NULL) { + return NT_STATUS_NO_MEMORY; + } + + status = safe_symlink_target_path(mem_ctx, + conn->connectpath, + target, + unparsed, + &safe_target); + TALLOC_FREE(target); if (!NT_STATUS_IS_OK(status)) { return status; } - name_in = target; + name_in = safe_target; symlink_redirects += 1; -- 2.43.0 From c8225344e493057101b18fdb25228937182145a6 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Tue, 2 Jan 2024 13:25:25 +0100 Subject: [PATCH 4/6] smbd: add a directory argument to safe_symlink_target_path() Existing caller passes NULL, no change in behaviour. Prepares for replacing symlink_target_below_conn() in open.c. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15549 Signed-off-by: Ralph Boehme Reviewed-by: Volker Lendecke (cherry picked from commit fc80c72d658a41fe4d93b24b793b52c91b350175) --- source3/include/proto.h | 1 + source3/smbd/filename.c | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/source3/include/proto.h b/source3/include/proto.h index 5e1e807b1adc..7df3c7a8e01a 100644 --- a/source3/include/proto.h +++ b/source3/include/proto.h @@ -720,6 +720,7 @@ struct smb_filename *synthetic_smb_fname(TALLOC_CTX *mem_ctx, uint32_t flags); NTSTATUS safe_symlink_target_path(TALLOC_CTX *mem_ctx, const char *connectpath, + const char *dir, const char *target, size_t unparsed, char **_relative); diff --git a/source3/smbd/filename.c b/source3/smbd/filename.c index 606959d0805f..1e07abacea68 100644 --- a/source3/smbd/filename.c +++ b/source3/smbd/filename.c @@ -945,6 +945,7 @@ static char *symlink_target_path( NTSTATUS safe_symlink_target_path(TALLOC_CTX *mem_ctx, const char *connectpath, + const char *dir, const char *target, size_t unparsed, char **_relative) @@ -960,10 +961,21 @@ NTSTATUS safe_symlink_target_path(TALLOC_CTX *mem_ctx, if (target[0] == '/') { abs_target = talloc_strdup(mem_ctx, target); - } else { + } else if (dir == NULL) { + abs_target = talloc_asprintf(mem_ctx, + "%s/%s", + connectpath, + target); + } else if (dir[0] == '/') { abs_target = talloc_asprintf(mem_ctx, "%s/%s", + dir, + target); + } else { + abs_target = talloc_asprintf(mem_ctx, + "%s/%s/%s", connectpath, + dir, target); } if (abs_target == NULL) { @@ -1491,6 +1503,7 @@ NTSTATUS filename_convert_dirfsp( status = safe_symlink_target_path(mem_ctx, conn->connectpath, + NULL, target, unparsed, &safe_target); -- 2.43.0 From a50cdb8b474e19f4d7ba5a7a1886d1ad75a4f05a Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Tue, 2 Jan 2024 14:34:26 +0100 Subject: [PATCH 5/6] smbd: use safe_symlink_target_path() in symlink_target_below_conn() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15549 Signed-off-by: Ralph Boehme Reviewed-by: Volker Lendecke (cherry picked from commit 1965fc77b3852a0593e13897af08f5304a1ce3a2) --- selftest/skip.opath-required | 2 - source3/smbd/open.c | 73 +++++++----------------------------- 2 files changed, 14 insertions(+), 61 deletions(-) diff --git a/selftest/skip.opath-required b/selftest/skip.opath-required index 05ff76dcbabf..b6758fdbcac0 100644 --- a/selftest/skip.opath-required +++ b/selftest/skip.opath-required @@ -9,7 +9,5 @@ ^samba3.raw.samba3hide.* # These don't work without /proc/fd support -^samba3.blackbox.test_symlink_traversal.*\(fileserver\) ^samba3.blackbox.shadow_copy_torture.*\(fileserver\) ^samba3.blackbox.virus_scanner.*\(fileserver:local\) -^samba3.blackbox.shadow_copy2.*\(fileserver.*\) diff --git a/source3/smbd/open.c b/source3/smbd/open.c index edc72bb03f07..4d7a103cbc0e 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -571,7 +571,6 @@ static NTSTATUS chdir_below_conn( static NTSTATUS symlink_target_below_conn( TALLOC_CTX *mem_ctx, const char *connection_path, - size_t connection_path_len, struct files_struct *fsp, struct files_struct *dirfsp, struct smb_filename *symlink_name, @@ -579,9 +578,7 @@ static NTSTATUS symlink_target_below_conn( { char *target = NULL; char *absolute = NULL; - const char *relative = NULL; NTSTATUS status; - bool ok; if (fsp_get_pathref_fd(fsp) != -1) { /* @@ -594,69 +591,28 @@ static NTSTATUS symlink_target_below_conn( talloc_tos(), dirfsp, symlink_name, &target); } + status = safe_symlink_target_path(talloc_tos(), + connection_path, + dirfsp->fsp_name->base_name, + target, + 0, + &absolute); if (!NT_STATUS_IS_OK(status)) { - DBG_DEBUG("readlink_talloc failed: %s\n", nt_errstr(status)); + DBG_DEBUG("safe_symlink_target_path() failed: %s\n", + nt_errstr(status)); return status; } - if (target[0] != '/') { - char *tmp = talloc_asprintf( - talloc_tos(), - "%s/%s/%s", - connection_path, - dirfsp->fsp_name->base_name, - target); - - TALLOC_FREE(target); - - if (tmp == NULL) { - return NT_STATUS_NO_MEMORY; - } - target = tmp; - } - - DBG_DEBUG("redirecting to %s\n", target); - - absolute = canonicalize_absolute_path(talloc_tos(), target); - TALLOC_FREE(target); - - if (absolute == NULL) { - return NT_STATUS_NO_MEMORY; - } - - /* - * We're doing the "below connection_path" here because it's - * cheap. It might be that we get a symlink out of the share, - * pointing to yet another symlink getting us back into the - * share. If we need that, we would have to remove the check - * here. - */ - ok = subdir_of( - connection_path, - connection_path_len, - absolute, - &relative); - if (!ok) { - DBG_NOTICE("Bad access attempt: %s is a symlink " - "outside the share path\n" - "conn_rootdir =%s\n" - "resolved_name=%s\n", - symlink_name->base_name, - connection_path, - absolute); - TALLOC_FREE(absolute); - return NT_STATUS_OBJECT_NAME_NOT_FOUND; - } - - if (relative[0] == '\0') { + if (absolute[0] == '\0') { /* * special case symlink to share root: "." is our * share root filename */ - absolute[0] = '.'; - absolute[1] = '\0'; - } else { - memmove(absolute, relative, strlen(relative)+1); + TALLOC_FREE(absolute); + absolute = talloc_strdup(talloc_tos(), "."); + if (absolute == NULL) { + return NT_STATUS_NO_MEMORY; + } } *_target = absolute; @@ -834,7 +790,6 @@ static NTSTATUS non_widelink_open(const struct files_struct *dirfsp, status = symlink_target_below_conn( talloc_tos(), connpath, - connpath_len, fsp, discard_const_p(files_struct, dirfsp), smb_fname_rel, -- 2.43.0 From a5f83c4f4358a527591a82690e78b48f7ff4a0d5 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Mon, 18 Dec 2023 12:35:58 +0100 Subject: [PATCH 6/6] smbd: use dirfsp and atname in open_directory() On systems without /proc/fd support this avoid the expensive chdir() logic in non_widelink_open(). open_file_ntcreate() already passes dirfsp and atname to reopen_from_fsp(), it was just missed in the conversion. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15549 Reviewed-by: Volker Lendecke Signed-off-by: Ralph Boehme Autobuild-User(master): Volker Lendecke Autobuild-Date(master): Mon Jan 22 12:00:56 UTC 2024 on atb-devel-224 (cherry picked from commit 2713023250f15cf9971d88620cab9dd4afd0dc73) --- source3/smbd/open.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 4d7a103cbc0e..3e7a8f45ebd2 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -4915,8 +4915,8 @@ static NTSTATUS open_directory(connection_struct *conn, if (access_mask & need_fd_access) { status = reopen_from_fsp( - fsp->conn->cwd_fsp, - fsp->fsp_name, + parent_dir_fname->fsp, + smb_fname_atname, fsp, O_RDONLY | O_DIRECTORY, 0, -- 2.43.0