From c133ef0ad22439bcd6f9fdb21d6b66572653f29d Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 2 Aug 2024 08:25:16 +0200 Subject: [PATCH 1/2] TODO: lib/util: let server_id_str_buf_unique() use '%' as delimiter instead of '/' This way the resulting buffer can be used as part of a file/directory name. TODO: impact on server_id_db_init() names.tdb??? BUG: https://bugzilla.samba.org/show_bug.cgi?id=15693 --- lib/util/server_id.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/util/server_id.c b/lib/util/server_id.c index 690b9ddfbdca..6ec40aa2c26d 100644 --- a/lib/util/server_id.c +++ b/lib/util/server_id.c @@ -90,7 +90,7 @@ size_t server_id_str_buf_unique(struct server_id id, char *buf, size_t buflen) if (buflen >= needed) { memcpy(buf, idbuf.buf, idlen); - buf[idlen] = '/'; + buf[idlen] = '%'; memcpy(buf + idlen + 1, unique_buf, unique_len+1); } @@ -114,7 +114,7 @@ struct server_id server_id_from_string(uint32_t local_vnn, */ result = templ; - ret = sscanf(pid_string, "%"SCNu32":%"SCNu64".%"SCNu32"/%"SCNu64, + ret = sscanf(pid_string, "%"SCNu32":%"SCNu64".%"SCNu32"%%%"SCNu64, &result.vnn, &result.pid, &result.task_id, &result.unique_id); if (ret == 4) { @@ -129,7 +129,7 @@ struct server_id server_id_from_string(uint32_t local_vnn, } result = templ; - ret = sscanf(pid_string, "%"SCNu32":%"SCNu64"/%"SCNu64, + ret = sscanf(pid_string, "%"SCNu32":%"SCNu64"%%%"SCNu64, &result.vnn, &result.pid, &result.unique_id); if (ret == 3) { return result; @@ -143,7 +143,7 @@ struct server_id server_id_from_string(uint32_t local_vnn, } result = templ; - ret = sscanf(pid_string, "%"SCNu64".%"SCNu32"/%"SCNu64, + ret = sscanf(pid_string, "%"SCNu64".%"SCNu32"%%%"SCNu64, &result.pid, &result.task_id, &result.unique_id); if (ret == 3) { result.vnn = local_vnn; @@ -159,7 +159,7 @@ struct server_id server_id_from_string(uint32_t local_vnn, } result = templ; - ret = sscanf(pid_string, "%"SCNu64"/%"SCNu64, + ret = sscanf(pid_string, "%"SCNu64"%%%"SCNu64, &result.pid, &result.unique_id); if (ret == 2) { result.vnn = local_vnn; -- 2.34.1 From 58b2b8ba28c40761b288f9a97046db6ef78f082b Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 1 Aug 2024 14:37:55 +0200 Subject: [PATCH 2/2] TODO: mkdir_internal tmp_atname We could at a test for it using source3/modules/vfs_delay_inject.c to make the race more likely to happen. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15693 --- source3/smbd/dir.c | 56 ++++++++++ source3/smbd/globals.h | 9 ++ source3/smbd/open.c | 222 ++++++++++++++++++++++++++++++++++++-- source3/smbd/proto.h | 1 + source3/smbd/smb2_reply.c | 2 + 5 files changed, 283 insertions(+), 7 deletions(-) diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c index 76eb5756dc87..147515fe2f5c 100644 --- a/source3/smbd/dir.c +++ b/source3/smbd/dir.c @@ -1186,11 +1186,67 @@ const char *ReadDirName(struct smb_Dir *dir_hnd, char **ptalloced) dir_hnd->fsp, dir_hnd->dir, &talloced))) { + int unlink_flags = INT_MAX; /* Ignore . and .. - we've already returned them. */ if (ISDOT(n) || ISDOTDOT(n)) { TALLOC_FREE(talloced); continue; } + /* + * ignore tmp directories, see mkdir_internals() + */ + if (IS_SMBD_TMPNAME(n, &unlink_flags)) { + struct smb_filename *atname = NULL; + const char *fp = NULL; + int ret; + + atname = synthetic_smb_fname(talloc_tos(), + n, + NULL, + NULL, + 0, + 0); + if (atname == NULL) { + TALLOC_FREE(talloced); + continue; + } + fp = full_path_from_dirfsp_at_basename(atname, + dir_hnd->fsp, + n); + if (fp == NULL) { + TALLOC_FREE(atname); + TALLOC_FREE(talloced); + continue; + } + + if (unlink_flags == INT_MAX) { + DBG_NOTICE("ignoring %s\n", fp); + TALLOC_FREE(atname); + TALLOC_FREE(talloced); + continue; + } + + /* + * We remove the stale tmpname + * as root and ignore any errors + */ + DBG_NOTICE("unlink stale %s\n", fp); + become_root(); + ret = SMB_VFS_UNLINKAT(conn, + dir_hnd->fsp, + atname, + unlink_flags); + unbecome_root(); + if (ret == 0) { + DBG_NOTICE("unlinked stale %s\n", fp); + } else { + DBG_WARNING("failed to unlink stale %s: %s\n", + fp, strerror(errno)); + } + TALLOC_FREE(atname); + TALLOC_FREE(talloced); + continue; + } *ptalloced = talloced; dir_hnd->file_number++; return n; diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h index 7d0d924dd739..8180f3368938 100644 --- a/source3/smbd/globals.h +++ b/source3/smbd/globals.h @@ -842,4 +842,13 @@ struct aio_extra { bool write_through; }; +#define SMBD_TMPNAME_PREFIX ".::TMPNAME:" +#define SMBD_TMPDIR_PREFIX SMBD_TMPNAME_PREFIX "D:" +#define IS_SMBD_TMPNAME_PREFIX(__n) \ + unlikely(__n[0] == '.' && __n[1] == ':' && \ + strncmp(__n, SMBD_TMPNAME_PREFIX, sizeof(SMBD_TMPNAME_PREFIX) -1) == 0) +#define IS_SMBD_TMPNAME(__n, __unlink_flags) \ + (IS_SMBD_TMPNAME_PREFIX(__n) && \ + smbd_is_tmpname(__n, __unlink_flags)) + #endif /* _SOURCE3_SMBD_GLOBALS_H_ */ diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 7754cb66607d..78dffc9e4fe7 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -4541,6 +4541,61 @@ unlock: return NT_STATUS_OK; } +bool smbd_is_tmpname(const char *n, int *_unlink_flags) +{ + const char *p = n; + int unlink_flags = INT_MAX; + struct server_id id; + bool exists; + + if (_unlink_flags != NULL) { + *_unlink_flags = INT_MAX; + } + + if (!IS_SMBD_TMPNAME_PREFIX(n)) { + return false; + } + p += sizeof(SMBD_TMPNAME_PREFIX) - 1; + switch (p[0]) { + case 'D': + unlink_flags = AT_REMOVEDIR; + break; + default: + return false; + } + p += 1; + if (p[0] != ':') { + return false; + } + p += 1; + + id = server_id_from_string(get_my_vnn(), p); + if (id.pid == UINT64_MAX) { + return false; + } + if (id.unique_id == 0) { + return false; + } + if (id.unique_id == SERVERID_UNIQUE_ID_NOT_TO_VERIFY) { + return false; + } + + if (_unlink_flags == NULL) { + return true; + } + + exists = serverid_exists(&id); + if (!exists) { + /* + * let the caller know it's stale + * and should be removed + */ + *_unlink_flags = unlink_flags; + } + + return true; +} + static NTSTATUS mkdir_internal(connection_struct *conn, struct smb_filename *parent_dir_fname, /* parent. */ struct smb_filename *smb_fname_atname, /* atname relative to parent. */ @@ -4548,6 +4603,7 @@ static NTSTATUS mkdir_internal(connection_struct *conn, uint32_t file_attributes, struct files_struct *fsp) { + TALLOC_CTX *frame = talloc_stackframe(); const struct loadparm_substitution *lp_sub = loadparm_s3_global_substitution(); mode_t mode; @@ -4555,12 +4611,18 @@ static NTSTATUS mkdir_internal(connection_struct *conn, bool posix_open = false; bool need_re_stat = false; uint32_t access_mask = SEC_DIR_ADD_SUBDIR; + struct smb_filename *tmp_atname = NULL; + char *orig_dname = NULL; + char *tmp_dname = NULL; + char tmp_pid_buf[64] = { 0, }; + SMB_STRUCT_STAT st; struct vfs_open_how how = { .flags = O_RDONLY|O_DIRECTORY, }; int ret; if (!CAN_WRITE(conn) || (access_mask & ~(conn->share_access))) { DEBUG(5,("mkdir_internal: failing share access " "%s\n", lp_servicename(talloc_tos(), lp_sub, SNUM(conn)))); + TALLOC_FREE(frame); return NT_STATUS_ACCESS_DENIED; } @@ -4581,6 +4643,7 @@ static NTSTATUS mkdir_internal(connection_struct *conn, smb_fname_str_dbg(parent_dir_fname), smb_dname->base_name, nt_errstr(status)); + TALLOC_FREE(frame); return status; } @@ -4590,12 +4653,78 @@ static NTSTATUS mkdir_internal(connection_struct *conn, } } + orig_dname = smb_dname->base_name; + + server_id_str_buf_unique(messaging_server_id(conn->sconn->msg_ctx), + tmp_pid_buf, sizeof(tmp_pid_buf)); + + tmp_atname = cp_smb_filename(frame, + smb_fname_atname); + if (tmp_atname == NULL) { + TALLOC_FREE(frame); + return NT_STATUS_NO_MEMORY; + } + TALLOC_FREE(tmp_atname->base_name); + tmp_atname->base_name = talloc_asprintf(tmp_atname, + "%s%s:%s", + SMBD_TMPDIR_PREFIX, + tmp_pid_buf, + smb_fname_atname->base_name); + if (tmp_atname == NULL) { + TALLOC_FREE(frame); + return NT_STATUS_NO_MEMORY; + } + if (!ISDOT(parent_dir_fname->base_name)) { + tmp_dname = talloc_asprintf(frame, + "%s/%s", + parent_dir_fname->base_name, + tmp_atname->base_name); + if (tmp_dname == NULL) { + TALLOC_FREE(frame); + return NT_STATUS_NO_MEMORY; + } + } else { + tmp_dname = talloc_strdup(frame, tmp_atname->base_name); + if (tmp_dname == NULL) { + TALLOC_FREE(frame); + return NT_STATUS_NO_MEMORY; + } + } + + DBG_DEBUG("%s: %s\n", __location__, "START"); + DBG_DEBUG("parent_dir_fname[%s][%p] VS[%u] VD[%u]\n", smb_fname_str_dbg(parent_dir_fname), parent_dir_fname, VALID_STAT(parent_dir_fname->st), VALID_STAT_OF_DIR(parent_dir_fname->st)); + DBG_DEBUG("parent_dir_fname->fsp[%p][%s] VS[%u] VD[%u]\n", parent_dir_fname->fsp, smb_fname_str_dbg(parent_dir_fname->fsp->fsp_name), VALID_STAT(parent_dir_fname->fsp->fsp_name->st), VALID_STAT_OF_DIR(parent_dir_fname->fsp->fsp_name->st)); + DBG_DEBUG("tmp_atname[%s] VS[%u] VD[%u]\n", smb_fname_str_dbg(tmp_atname), VALID_STAT(tmp_atname->st), VALID_STAT_OF_DIR(tmp_atname->st)); + DBG_DEBUG("smb_atname[%s] VS[%u] VD[%u]\n", smb_fname_str_dbg(smb_fname_atname), VALID_STAT(smb_fname_atname->st), VALID_STAT_OF_DIR(smb_fname_atname->st)); + DBG_DEBUG("fsp[%p]->name[%s][%p] VS[%u] VD[%u]\n", fsp, smb_fname_str_dbg(fsp->fsp_name), fsp->fsp_name, VALID_STAT(fsp->fsp_name->st), VALID_STAT_OF_DIR(fsp->fsp_name->st)); + DBG_DEBUG("smb_dname[%s][%p] VS[%u] VD[%u]\n", smb_fname_str_dbg(smb_dname), smb_dname, VALID_STAT(smb_dname->st), VALID_STAT_OF_DIR(smb_dname->st)); + DBG_DEBUG("smb_dname->fsp[%p][%s] VS[%u] VD[%u]\n", smb_dname->fsp, smb_fname_str_dbg(smb_dname->fsp->fsp_name), VALID_STAT(smb_dname->fsp->fsp_name->st), VALID_STAT_OF_DIR(smb_dname->fsp->fsp_name->st)); + + { + int unlink_flags = 0; + bool is = smbd_is_tmpname(tmp_atname->base_name, &unlink_flags); + SMB_ASSERT(is); + SMB_ASSERT(unlink_flags == INT_MAX); + } + + smb_dname->base_name = tmp_dname; + + DBG_DEBUG("%s: %s\n", __location__, "AFTER-replace"); + DBG_DEBUG("parent_dir_fname[%s][%p] VS[%u] VD[%u]\n", smb_fname_str_dbg(parent_dir_fname), parent_dir_fname, VALID_STAT(parent_dir_fname->st), VALID_STAT_OF_DIR(parent_dir_fname->st)); + DBG_DEBUG("parent_dir_fname->fsp[%p][%s] VS[%u] VD[%u]\n", parent_dir_fname->fsp, smb_fname_str_dbg(parent_dir_fname->fsp->fsp_name), VALID_STAT(parent_dir_fname->fsp->fsp_name->st), VALID_STAT_OF_DIR(parent_dir_fname->fsp->fsp_name->st)); + DBG_DEBUG("smb_atname[%s] VS[%u] VD[%u]\n", smb_fname_str_dbg(smb_fname_atname), VALID_STAT(smb_fname_atname->st), VALID_STAT_OF_DIR(smb_fname_atname->st)); + DBG_DEBUG("fsp[%p]->name[%s][%p] VS[%u] VD[%u]\n", fsp, smb_fname_str_dbg(fsp->fsp_name), fsp->fsp_name, VALID_STAT(fsp->fsp_name->st), VALID_STAT_OF_DIR(fsp->fsp_name->st)); + DBG_DEBUG("smb_dname[%s][%p] VS[%u] VD[%u]\n", smb_fname_str_dbg(smb_dname), smb_dname, VALID_STAT(smb_dname->st), VALID_STAT_OF_DIR(smb_dname->st)); + DBG_DEBUG("smb_dname->fsp[%p][%s] VS[%u] VD[%u]\n", smb_dname->fsp, smb_fname_str_dbg(smb_dname->fsp->fsp_name), VALID_STAT(smb_dname->fsp->fsp_name->st), VALID_STAT_OF_DIR(smb_dname->fsp->fsp_name->st)); + ret = SMB_VFS_MKDIRAT(conn, parent_dir_fname->fsp, - smb_fname_atname, + tmp_atname, mode); if (ret != 0) { - return map_nt_error_from_unix(errno); + status = map_nt_error_from_unix(errno); + DBG_ERR("%s: %s\n", __location__, nt_errstr(status)); + goto restore_orig; } /* @@ -4604,11 +4733,19 @@ static NTSTATUS mkdir_internal(connection_struct *conn, */ fsp->fsp_flags.is_pathref = true; - status = fd_openat(parent_dir_fname->fsp, smb_fname_atname, fsp, &how); + status = fd_openat(parent_dir_fname->fsp, tmp_atname, fsp, &how); if (!NT_STATUS_IS_OK(status)) { - return status; + DBG_ERR("%s: %s\n", __location__, nt_errstr(status)); + goto internal_error; } + DBG_DEBUG("%s: %s\n", __location__, "AFTER-fd_openat"); + DBG_DEBUG("tmp_atname[%s] VS[%u] VD[%u]\n", smb_fname_str_dbg(tmp_atname), VALID_STAT(tmp_atname->st), VALID_STAT_OF_DIR(tmp_atname->st)); + DBG_DEBUG("smb_atname[%s] VS[%u] VD[%u]\n", smb_fname_str_dbg(smb_fname_atname), VALID_STAT(smb_fname_atname->st), VALID_STAT_OF_DIR(smb_fname_atname->st)); + DBG_DEBUG("fsp[%p]->name[%s][%p] VS[%u] VD[%u]\n", fsp, smb_fname_str_dbg(fsp->fsp_name), fsp->fsp_name, VALID_STAT(fsp->fsp_name->st), VALID_STAT_OF_DIR(fsp->fsp_name->st)); + DBG_DEBUG("smb_dname[%s][%p] VS[%u] VD[%u]\n", smb_fname_str_dbg(smb_dname), smb_dname, VALID_STAT(smb_dname->st), VALID_STAT_OF_DIR(smb_dname->st)); + DBG_DEBUG("smb_dname->fsp[%p][%s] VS[%u] VD[%u]\n", smb_dname->fsp, smb_fname_str_dbg(smb_dname->fsp->fsp_name), VALID_STAT(smb_dname->fsp->fsp_name->st), VALID_STAT_OF_DIR(smb_dname->fsp->fsp_name->st)); + /* Ensure we're checking for a symlink here.... */ /* We don't want to get caught by a symlink racer. */ @@ -4616,13 +4753,14 @@ static NTSTATUS mkdir_internal(connection_struct *conn, if (!NT_STATUS_IS_OK(status)) { DEBUG(2, ("Could not stat directory '%s' just created: %s\n", smb_fname_str_dbg(smb_dname), nt_errstr(status))); - return status; + goto internal_error; } if (!S_ISDIR(smb_dname->st.st_ex_mode)) { DEBUG(0, ("Directory '%s' just created is not a directory !\n", smb_fname_str_dbg(smb_dname))); - return NT_STATUS_NOT_A_DIRECTORY; + status = NT_STATUS_NOT_A_DIRECTORY; + goto internal_error; } if (lp_store_dos_attributes(SNUM(conn))) { @@ -4667,14 +4805,84 @@ static NTSTATUS mkdir_internal(connection_struct *conn, if (!NT_STATUS_IS_OK(status)) { DEBUG(2, ("Could not stat directory '%s' just created: %s\n", smb_fname_str_dbg(smb_dname), nt_errstr(status))); - return status; + goto internal_error; } } + /* + * TODO: + * + * Instead of SMB_VFS_FSTATAT followed by + * SMB_VFS_RENAMEAT, we should use + * SMB_VFS_RENAMEAT(RENAME_NOREPLACE) with + * a fallback for EINVAL to SMB_VFS_RENAMEAT() without + * RENAME_NOREPLACE. + */ + ret = SMB_VFS_FSTATAT(conn, parent_dir_fname->fsp, + smb_fname_atname, &st, AT_SYMLINK_NOFOLLOW); + if (ret == 0) { + status = NT_STATUS_OBJECT_NAME_COLLISION; + DBG_ERR("%s: %s\n", __location__, nt_errstr(status)); + goto do_unlink; + } + if (ret != 0 && errno != ENOENT) { + status = map_nt_error_from_unix(errno); + DBG_ERR("%s: %s\n", __location__, nt_errstr(status)); + goto do_unlink; + } + ret = SMB_VFS_RENAMEAT(conn, + parent_dir_fname->fsp, + tmp_atname, + parent_dir_fname->fsp, + smb_fname_atname); + if (ret != 0) { + status = map_nt_error_from_unix(errno); + DBG_ERR("%s: %s\n", __location__, nt_errstr(status)); + goto do_unlink; + } + smb_fname_atname->st = tmp_atname->st; + smb_dname->base_name = orig_dname; + + DBG_DEBUG("%s: %s\n", __location__, "AFTER-RENAMEAT"); + DBG_DEBUG("tmp_atname[%s] VS[%u] VD[%u]\n", smb_fname_str_dbg(tmp_atname), VALID_STAT(tmp_atname->st), VALID_STAT_OF_DIR(tmp_atname->st)); + DBG_DEBUG("smb_atname[%s] VS[%u] VD[%u]\n", smb_fname_str_dbg(smb_fname_atname), VALID_STAT(smb_fname_atname->st), VALID_STAT_OF_DIR(smb_fname_atname->st)); + DBG_DEBUG("fsp[%p]->name[%s][%p] VS[%u] VD[%u]\n", fsp, smb_fname_str_dbg(fsp->fsp_name), fsp->fsp_name, VALID_STAT(fsp->fsp_name->st), VALID_STAT_OF_DIR(fsp->fsp_name->st)); + DBG_DEBUG("smb_dname[%s][%p] VS[%u] VD[%u]\n", smb_fname_str_dbg(smb_dname), smb_dname, VALID_STAT(smb_dname->st), VALID_STAT_OF_DIR(smb_dname->st)); + DBG_DEBUG("smb_dname->fsp[%p][%s] VS[%u] VD[%u]\n", smb_dname->fsp, smb_fname_str_dbg(smb_dname->fsp->fsp_name), VALID_STAT(smb_dname->fsp->fsp_name->st), VALID_STAT_OF_DIR(smb_dname->fsp->fsp_name->st)); + DBG_DEBUG("%s: %s\n", __location__, "DONE"); + notify_fname(conn, NOTIFY_ACTION_ADDED, FILE_NOTIFY_CHANGE_DIR_NAME, smb_dname->base_name); + TALLOC_FREE(frame); return NT_STATUS_OK; + +internal_error: + DBG_ERR("%s: %s\n", __location__, nt_errstr(status)); + goto restore_orig; + +do_unlink: + DBG_ERR("%s: %s\n", __location__, nt_errstr(status)); + ret = SMB_VFS_UNLINKAT(conn, + parent_dir_fname->fsp, + tmp_atname, + AT_REMOVEDIR); + if (ret == 0) { + status = map_nt_error_from_unix(errno); + DBG_ERR("%s: %s\n", __location__, nt_errstr(status)); + } else { + status = map_nt_error_from_unix(errno); + DBG_ERR("%s: %s\n", __location__, nt_errstr(status)); + } + goto restore_orig; + +restore_orig: + DBG_ERR("%s: %s\n", __location__, nt_errstr(status)); + SET_STAT_INVALID(smb_fname_atname->st); + smb_dname->base_name = orig_dname; + SET_STAT_INVALID(smb_dname->st); + TALLOC_FREE(frame); + return status; } /**************************************************************************** diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h index 90387aeb6c34..429d8875f0a9 100644 --- a/source3/smbd/proto.h +++ b/source3/smbd/proto.h @@ -657,6 +657,7 @@ NTSTATUS smbd_check_access_rights_fsp(struct files_struct *dirfsp, uint32_t access_mask); NTSTATUS check_parent_access_fsp(struct files_struct *fsp, uint32_t access_mask); +bool smbd_is_tmpname(const char *n, int *_unlink_flags); NTSTATUS fd_openat(const struct files_struct *dirfsp, struct smb_filename *smb_fname, files_struct *fsp, diff --git a/source3/smbd/smb2_reply.c b/source3/smbd/smb2_reply.c index 3f7bb7a1ea62..f093d4d13f2d 100644 --- a/source3/smbd/smb2_reply.c +++ b/source3/smbd/smb2_reply.c @@ -166,6 +166,8 @@ NTSTATUS check_path_syntax(char *path, bool posix_path) s++; continue; } + } else if (IS_SMBD_TMPNAME(s, NULL)) { + return NT_STATUS_OBJECT_NAME_INVALID; } } -- 2.34.1