diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 730731b..c4e1ba4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -121,6 +121,8 @@ jobs: run: make CC=${{ matrix.cc }} V=1 WITH_SDK=0 WITH_PQBOX=0 check - name: Extended upstream tests run: make CC=${{ matrix.cc }} V=1 WITH_SDK=0 WITH_PQBOX=0 test-all + - name: In-tree SDK atomic key-save regression + run: make CC=${{ matrix.cc }} V=1 sdk-test - name: Functional test of the built CLI run: bash scripts/test-installed-zupt.sh "$PWD/zupt" diff --git a/Makefile b/Makefile index da95b79..5b6ec4e 100644 --- a/Makefile +++ b/Makefile @@ -540,6 +540,7 @@ test-all: check # Release-only gates need a committed Git checkout and packaging metadata. # Keep them out of downstream %check, which intentionally has no dist rebuild. release-check: test-all audit-licenses + $(Q)$(MAKE) sdk-test $(Q)bash tests/test_static_analysis.sh $(Q)bash tests/test_packaging_syntax.sh $(Q)bash scripts/test-installed-zupt.sh ./$(TARGET) diff --git a/sdk/src/zuptsdk.c b/sdk/src/zuptsdk.c index d886aa0..f90245e 100644 --- a/sdk/src/zuptsdk.c +++ b/sdk/src/zuptsdk.c @@ -556,20 +556,46 @@ void zuptsdk_keypair_destroy(zuptsdk_keypair_t *kp) { static int zsdk_copy_file(const char *src, const char *dst, mode_t mode) { FILE *fi = fopen(src, "rb"); if (!fi) return ZSDK_FAIL(ZUPTSDK_ERR_IO, "open %s", src); - FILE *fo = fopen(dst, "wb"); - if (!fo) { fclose(fi); return ZSDK_FAIL(ZUPTSDK_ERR_IO, "create %s", dst); } - uint8_t buf[4096]; - size_t n; - int rc = ZUPTSDK_OK; - while ((n = fread(buf, 1, sizeof(buf), fi)) > 0) - if (fwrite(buf, 1, n, fo) != n) { rc = ZSDK_FAIL(ZUPTSDK_ERR_IO, "write %s", dst); break; } - zuptsdk_secure_zero(buf, sizeof(buf)); - fclose(fi); fclose(fo); + FILE *fo = NULL; + zupt_atomic_output_t *output = zupt_atomic_output_open(dst, &fo); + if (!output) { + int saved_errno = errno; + fclose(fi); + errno = saved_errno; + return ZSDK_FAIL(ZUPTSDK_ERR_IO, "create %s", dst); + } + #ifndef _WIN32 - if (rc == ZUPTSDK_OK) chmod(dst, mode); + /* Apply permissions to the private temporary object, never to a + * re-resolved destination path. */ + if (fchmod(fileno(fo), mode) != 0) { + int saved_errno = errno; + fclose(fi); + (void)zupt_atomic_output_finish(output, 0); + errno = saved_errno; + return ZSDK_FAIL(ZUPTSDK_ERR_IO, "set permissions on %s", dst); + } #else (void)mode; #endif + + uint8_t buf[4096]; + size_t n; + int rc = ZUPTSDK_OK; + while ((n = fread(buf, 1, sizeof(buf), fi)) > 0) { + if (fwrite(buf, 1, n, fo) != n) { + rc = ZSDK_FAIL(ZUPTSDK_ERR_IO, "write %s", dst); + break; + } + } + if (rc == ZUPTSDK_OK && ferror(fi)) + rc = ZSDK_FAIL(ZUPTSDK_ERR_IO, "read %s", src); + zuptsdk_secure_zero(buf, sizeof(buf)); + if (fclose(fi) != 0 && rc == ZUPTSDK_OK) + rc = ZSDK_FAIL(ZUPTSDK_ERR_IO, "close %s", src); + if (zupt_atomic_output_finish(output, rc == ZUPTSDK_OK) != 0 && + rc == ZUPTSDK_OK) + rc = ZSDK_FAIL(ZUPTSDK_ERR_IO, "publish %s", dst); return rc; } diff --git a/sdk/tests/test_sdk_roundtrip.c b/sdk/tests/test_sdk_roundtrip.c index 2176a60..8ef87a2 100644 --- a/sdk/tests/test_sdk_roundtrip.c +++ b/sdk/tests/test_sdk_roundtrip.c @@ -12,6 +12,9 @@ #include #include #include +#ifndef _WIN32 +#include +#endif #include static int g_pass = 0, g_fail = 0; @@ -47,6 +50,79 @@ static const uint8_t TEST_DATA[] = "Lorem ipsum dolor sit amet consectetur adipiscing elit sed do eiusmod. " "End of test data.\n"; +#ifndef _WIN32 +static int file_matches(const char *path, const void *expected, + size_t expected_size) { + struct stat info; + char observed[128]; + if (expected_size > sizeof(observed) || stat(path, &info) != 0 || + info.st_size < 0 || (uint64_t)info.st_size != (uint64_t)expected_size) + return 0; + FILE *stream = fopen(path, "rb"); + if (!stream) return 0; + size_t got = fread(observed, 1, expected_size, stream); + int read_error = ferror(stream); + int close_rc = fclose(stream); + return got == expected_size && !read_error && close_rc == 0 && + memcmp(observed, expected, expected_size) == 0; +} + +static int private_key_save_avoids_link_targets(const zuptsdk_keypair_t *kp) { + static const char sentinel[] = "do not replace through a symlink\n"; + char workspace[] = "/tmp/zupt-sdk-link-save.XXXXXX"; + char target[192]; + char symlink_path[192]; + char hardlink_path[192]; + FILE *stream; + struct stat target_st; + struct stat output_st; + int ok = 0; + + if (!mkdtemp(workspace)) return 0; + snprintf(target, sizeof(target), "%s/target", workspace); + snprintf(symlink_path, sizeof(symlink_path), "%s/symlink-output", + workspace); + snprintf(hardlink_path, sizeof(hardlink_path), "%s/hardlink-output", + workspace); + + stream = fopen(target, "wb"); + if (!stream) goto cleanup; + size_t written = fwrite(sentinel, 1, sizeof(sentinel) - 1, stream); + int close_rc = fclose(stream); + if (written != sizeof(sentinel) - 1 || close_rc != 0) + goto cleanup; + + if (symlink(target, symlink_path) != 0 || + zuptsdk_keypair_save_private(kp, symlink_path) != ZUPTSDK_OK || + !file_matches(target, sentinel, sizeof(sentinel) - 1) || + stat(target, &target_st) != 0 || lstat(symlink_path, &output_st) != 0 || + (target_st.st_dev == output_st.st_dev && + target_st.st_ino == output_st.st_ino) || + !S_ISREG(output_st.st_mode) || output_st.st_size <= 0 || + (output_st.st_mode & 0777) != 0600) + goto cleanup; + + if (link(target, hardlink_path) != 0 || + zuptsdk_keypair_save_private(kp, hardlink_path) != ZUPTSDK_OK || + !file_matches(target, sentinel, sizeof(sentinel) - 1) || + stat(target, &target_st) != 0 || stat(hardlink_path, &output_st) != 0 || + (target_st.st_dev == output_st.st_dev && + target_st.st_ino == output_st.st_ino) || + !S_ISREG(output_st.st_mode) || output_st.st_size <= 0 || + (output_st.st_mode & 0777) != 0600) + goto cleanup; + + ok = 1; + +cleanup: + unlink(symlink_path); + unlink(hardlink_path); + unlink(target); + rmdir(workspace); + return ok; +} +#endif + static void test_version(void) { TEST("version_string returns non-NULL"); const char *v = zuptsdk_version_string(); @@ -250,6 +326,15 @@ cleanup: static void test_keypair_pq(void) { TEST("keypair_generate + compress_pq + extract_pq"); + char saved_priv[160]; + char saved_pub[160]; + snprintf(saved_priv, sizeof(saved_priv), "/tmp/_zsdk_priv_%ld.key", + (long)getpid()); + snprintf(saved_pub, sizeof(saved_pub), "/tmp/_zsdk_pub_%ld.key", + (long)getpid()); + unlink(saved_priv); + unlink(saved_pub); + zuptsdk_ctx_t *ctx = NULL; CHECK(zuptsdk_ctx_create(&ctx), "ctx"); @@ -257,17 +342,35 @@ static void test_keypair_pq(void) { int rc = zuptsdk_keypair_generate(ctx, &kp); if (rc != ZUPTSDK_OK) { FAIL("keygen"); zuptsdk_ctx_destroy(ctx); return; } +#ifndef _WIN32 + if (!private_key_save_avoids_link_targets(kp)) { + FAIL("private key save followed a symlink or hardlink target"); + goto err; + } +#endif + /* Save and load to exercise that path too */ - rc = zuptsdk_keypair_save_private(kp, "/tmp/_zsdk_priv.key"); + rc = zuptsdk_keypair_save_private(kp, saved_priv); if (rc != ZUPTSDK_OK) { FAIL("save priv"); goto err; } - rc = zuptsdk_keypair_save_public(kp, "/tmp/_zsdk_pub.key"); + rc = zuptsdk_keypair_save_public(kp, saved_pub); if (rc != ZUPTSDK_OK) { FAIL("save pub"); goto err; } +#ifndef _WIN32 + struct stat private_st; + struct stat public_st; + if (stat(saved_priv, &private_st) != 0 || + stat(saved_pub, &public_st) != 0 || + (private_st.st_mode & 0777) != 0600 || + (public_st.st_mode & 0777) != 0644) { + FAIL("saved key permissions do not match the requested modes"); + goto err; + } +#endif zuptsdk_pubkey_t *pub = NULL; zuptsdk_privkey_t *priv = NULL; - rc = zuptsdk_pubkey_load("/tmp/_zsdk_pub.key", &pub); + rc = zuptsdk_pubkey_load(saved_pub, &pub); if (rc != ZUPTSDK_OK) { FAIL("load pub"); goto err; } - rc = zuptsdk_privkey_load("/tmp/_zsdk_priv.key", &priv); + rc = zuptsdk_privkey_load(saved_priv, &priv); if (rc != ZUPTSDK_OK) { FAIL("load priv"); zuptsdk_pubkey_destroy(pub); goto err; } zuptsdk_options_t *opts = NULL; @@ -295,8 +398,8 @@ static void test_keypair_pq(void) { zuptsdk_privkey_destroy(priv); zuptsdk_options_destroy(opts); - unlink("/tmp/_zsdk_priv.key"); - unlink("/tmp/_zsdk_pub.key"); + unlink(saved_priv); + unlink(saved_pub); if (!ok) { FAIL("byte mismatch or rc != OK"); zuptsdk_keypair_destroy(kp); zuptsdk_ctx_destroy(ctx); return; } zuptsdk_keypair_destroy(kp); @@ -305,6 +408,8 @@ static void test_keypair_pq(void) { return; err: + unlink(saved_priv); + unlink(saved_pub); zuptsdk_keypair_destroy(kp); zuptsdk_ctx_destroy(ctx); } diff --git a/src/zupt_disk.c b/src/zupt_disk.c index b7a2570..d6bc090 100644 --- a/src/zupt_disk.c +++ b/src/zupt_disk.c @@ -1220,23 +1220,35 @@ zupt_error_t zupt_disk_restore(const char *archive_path, const char *target_path #else int tgt_fd = -1; int is_block_dev = 0; - struct stat target_st; - - if (lstat(target_path, &target_st) == 0) { - if (S_ISLNK(target_st.st_mode)) { - fprintf(stderr, "Error: refusing a symbolic-link restore target.\n"); - fclose(f); - return ZUPT_ERR_INVALID; - } - if (S_ISREG(target_st.st_mode)) { - if (target_st.st_dev == archive_identity.device && - target_st.st_ino == archive_identity.inode) { + /* Resolve the target exactly once before making any type or identity + * decision. The open is non-truncating, O_NOFOLLOW rejects a final + * symlink, and fstat classifies the kernel object that was actually + * opened. Device restores retain this same descriptor through the final + * write, so a concurrent pathname exchange cannot redirect the restore. */ + tgt_fd = open(target_path, O_WRONLY | O_NOFOLLOW | O_CLOEXEC | + O_NONBLOCK | O_SYNC); + if (tgt_fd >= 0) { + struct stat opened_st; + if (fstat(tgt_fd, &opened_st) != 0) { + int saved_errno = errno; + close(tgt_fd); + tgt_fd = -1; + errno = saved_errno; + } else if (S_ISREG(opened_st.st_mode)) { + int close_result = close(tgt_fd); + tgt_fd = -1; + if (close_result != 0) { + fclose(f); + return ZUPT_ERR_IO; + } + if (opened_st.st_dev == archive_identity.device && + opened_st.st_ino == archive_identity.inode) { fprintf(stderr, "Error: archive and restore target are the same file.\n"); fclose(f); return ZUPT_ERR_INVALID; } - if (target_st.st_nlink != 1) { + if (opened_st.st_nlink != 1) { fprintf(stderr, "Error: refusing a multiply-linked restore target.\n"); fclose(f); @@ -1244,59 +1256,46 @@ zupt_error_t zupt_disk_restore(const char *archive_path, const char *target_path } target_atomic = zupt_atomic_output_open(target_path, &target_stream); - } else if (S_ISBLK(target_st.st_mode) || - S_ISCHR(target_st.st_mode)) { - tgt_fd = open(target_path, O_WRONLY | O_NOFOLLOW | O_CLOEXEC | - O_NONBLOCK | O_SYNC); - if (tgt_fd >= 0) { - struct stat opened_st; - if (fstat(tgt_fd, &opened_st) != 0 || - opened_st.st_dev != target_st.st_dev || - opened_st.st_ino != target_st.st_ino || - !(S_ISBLK(opened_st.st_mode) || - S_ISCHR(opened_st.st_mode))) { + } else if (S_ISBLK(opened_st.st_mode) || + S_ISCHR(opened_st.st_mode)) { + int flags = fcntl(tgt_fd, F_GETFL); + if (flags < 0 || + fcntl(tgt_fd, F_SETFL, flags & ~O_NONBLOCK) != 0) { + close(tgt_fd); + tgt_fd = -1; + } else { +#if defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__) + uint64_t target_capacity = 0; + if (!disk_restore_target_capacity( + tgt_fd, &opened_st, &target_capacity)) { + fprintf(stderr, + "Error: cannot determine restore device " + "capacity safely.\n"); close(tgt_fd); tgt_fd = -1; - errno = EAGAIN; + } else if (expected_size > target_capacity) { + fprintf(stderr, + "Error: disk image (%llu bytes) exceeds " + "restore device capacity (%llu bytes).\n", + (unsigned long long)expected_size, + (unsigned long long)target_capacity); + close(tgt_fd); + tgt_fd = -1; + errno = EFBIG; } else { - int flags = fcntl(tgt_fd, F_GETFL); - if (flags < 0 || - fcntl(tgt_fd, F_SETFL, flags & ~O_NONBLOCK) != 0) { - close(tgt_fd); - tgt_fd = -1; - } else { -#if defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__) - uint64_t target_capacity = 0; - if (!disk_restore_target_capacity( - tgt_fd, &opened_st, &target_capacity)) { - fprintf(stderr, - "Error: cannot determine restore device " - "capacity safely.\n"); - close(tgt_fd); - tgt_fd = -1; - } else if (expected_size > target_capacity) { - fprintf(stderr, - "Error: disk image (%llu bytes) exceeds " - "restore device capacity (%llu bytes).\n", - (unsigned long long)expected_size, - (unsigned long long)target_capacity); - close(tgt_fd); - tgt_fd = -1; - errno = EFBIG; - } else { - is_block_dev = 1; - } -#else - fprintf(stderr, - "Error: restore-device capacity queries are " - "not supported on this platform.\n"); - close(tgt_fd); - tgt_fd = -1; -#endif - } + is_block_dev = 1; } +#else + fprintf(stderr, + "Error: restore-device capacity queries are " + "not supported on this platform.\n"); + close(tgt_fd); + tgt_fd = -1; +#endif } } else { + close(tgt_fd); + tgt_fd = -1; fprintf(stderr, "Error: restore target is not a regular file or device.\n"); fclose(f); @@ -1304,8 +1303,12 @@ zupt_error_t zupt_disk_restore(const char *archive_path, const char *target_path } } else if (errno == ENOENT) { target_atomic = zupt_atomic_output_open(target_path, &target_stream); + } else if (errno == ELOOP) { + fprintf(stderr, "Error: refusing a symbolic-link restore target.\n"); + fclose(f); + return ZUPT_ERR_INVALID; } else { - fprintf(stderr, "Error: Cannot inspect target '%s': %s\n", + fprintf(stderr, "Error: Cannot open target '%s': %s\n", target_path, strerror(errno)); fclose(f); return ZUPT_ERR_IO; diff --git a/src/zupt_main.c b/src/zupt_main.c index f2b5b3b..a0d4a00 100644 --- a/src/zupt_main.c +++ b/src/zupt_main.c @@ -25,7 +25,9 @@ #ifdef _WIN32 #include #include + #include #else + #include #include #include #endif @@ -100,9 +102,11 @@ static int zupt_create_private_temp_directory(char *output, size_t capacity) { } return 0; #else - static const char pattern[] = "/tmp/zupt-bench-XXXXXX"; - if (sizeof(pattern) > capacity) return 0; - memcpy(output, pattern, sizeof(pattern)); + char temp_root[ZUPT_MAX_PATH]; + if (!realpath("/tmp", temp_root)) return 0; + int written = snprintf(output, capacity, "%s/zupt-bench-XXXXXX", + temp_root); + if (written < 0 || (size_t)written >= capacity) return 0; if (!mkdtemp(output)) return 0; if (chmod(output, 0700) != 0) { rmdir(output); @@ -114,7 +118,97 @@ static int zupt_create_private_temp_directory(char *output, size_t capacity) { } #ifdef _WIN32 -static int zupt_remove_tree_wide(const wchar_t *directory) { +static void zupt_win_set_cleanup_errno(NTSTATUS status) { + if (status == (NTSTATUS)0xC0000034L || /* STATUS_OBJECT_NAME_NOT_FOUND */ + status == (NTSTATUS)0xC000003AL) { /* STATUS_OBJECT_PATH_NOT_FOUND */ + errno = ENOENT; + } else { + errno = EACCES; + } +} + +/* Open one entry relative to a pinned parent. Omitting FILE_SHARE_DELETE + * keeps the name bound to this handle until cleanup finishes; opening the + * reparse point itself prevents a junction or symlink from redirecting the + * recursive walk. */ +static HANDLE zupt_win_open_cleanup_entry(HANDLE parent, + const wchar_t *name, + int directory_only, + int delete_access) { + size_t name_length = wcslen(name); + if (name_length == 0 || + name_length > (size_t)USHRT_MAX / sizeof(wchar_t)) { + errno = ENAMETOOLONG; + return INVALID_HANDLE_VALUE; + } + UNICODE_STRING object_name; + object_name.Buffer = (PWSTR)name; + object_name.Length = (USHORT)(name_length * sizeof(wchar_t)); + object_name.MaximumLength = object_name.Length + sizeof(wchar_t); + OBJECT_ATTRIBUTES attributes; + InitializeObjectAttributes(&attributes, &object_name, + OBJ_CASE_INSENSITIVE, parent, NULL); + IO_STATUS_BLOCK status_block; + HANDLE handle = INVALID_HANDLE_VALUE; + ACCESS_MASK access = FILE_LIST_DIRECTORY | FILE_TRAVERSE | + FILE_READ_ATTRIBUTES | SYNCHRONIZE; + if (delete_access) access |= DELETE; + ULONG share = FILE_SHARE_READ | FILE_SHARE_WRITE; + if (delete_access) share |= FILE_SHARE_DELETE; + ULONG options = FILE_OPEN_REPARSE_POINT | FILE_SYNCHRONOUS_IO_NONALERT; + if (directory_only) options |= FILE_DIRECTORY_FILE; + NTSTATUS status = NtCreateFile( + &handle, access, &attributes, &status_block, NULL, + FILE_ATTRIBUTE_NORMAL, share, FILE_OPEN, + options, NULL, 0); + if (status < 0 || handle == INVALID_HANDLE_VALUE) { + zupt_win_set_cleanup_errno(status); + return INVALID_HANDLE_VALUE; + } + return handle; +} + +/* Mark the exact object held by an identity-checked deletion handle. */ +static int zupt_win_delete_cleanup_handle(HANDLE handle) { + FILE_DISPOSITION_INFO disposition; + disposition.DeleteFile = TRUE; + if (SetFileInformationByHandle(handle, FileDispositionInfo, + &disposition, sizeof(disposition))) + return 1; + errno = EACCES; + return 0; +} + +/* Reopen an emptied child only after closing its no-delete-sharing traversal + * handle. Comparing the filesystem identity before marking the new handle + * for deletion makes a close/reopen name exchange fail safely. */ +static int zupt_win_delete_cleanup_entry( + HANDLE parent, const wchar_t *name, + const BY_HANDLE_FILE_INFORMATION *expected) { + HANDLE handle = zupt_win_open_cleanup_entry(parent, name, 1, 1); + if (handle == INVALID_HANDLE_VALUE) return 0; + BY_HANDLE_FILE_INFORMATION current; + int same = GetFileInformationByHandle(handle, ¤t) && + (current.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0 && + (current.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) == 0 && + current.dwVolumeSerialNumber == expected->dwVolumeSerialNumber && + current.nFileIndexHigh == expected->nFileIndexHigh && + current.nFileIndexLow == expected->nFileIndexLow; + int deleted = same && zupt_win_delete_cleanup_handle(handle); + int closed = CloseHandle(handle) != 0; + if (!same) errno = EBUSY; + return deleted && closed; +} + +static int zupt_win_plain_directory(HANDLE handle) { + BY_HANDLE_FILE_INFORMATION info; + return GetFileInformationByHandle(handle, &info) && + (info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0 && + (info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) == 0; +} + +static int zupt_remove_tree_wide(HANDLE directory_handle, + const wchar_t *directory) { size_t directory_length = wcslen(directory); wchar_t *pattern = (wchar_t *)calloc(directory_length + 3u, sizeof(*pattern)); @@ -125,6 +219,8 @@ static int zupt_remove_tree_wide(const wchar_t *directory) { WIN32_FIND_DATAW data; HANDLE search = FindFirstFileW(pattern, &data); + DWORD search_error = search == INVALID_HANDLE_VALUE + ? GetLastError() : ERROR_SUCCESS; free(pattern); int failed = 0; if (search != INVALID_HANDLE_VALUE) { @@ -143,23 +239,217 @@ static int zupt_remove_tree_wide(const wchar_t *directory) { child[directory_length] = L'\\'; memcpy(child + directory_length + 1u, data.cFileName, (name_length + 1u) * sizeof(*child)); - if ((data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0) { - if ((data.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0) { - if (!RemoveDirectoryW(child)) failed = 1; - } else if (zupt_remove_tree_wide(child) != 0) { - failed = 1; - } - } else { - SetFileAttributesW(child, FILE_ATTRIBUTE_NORMAL); - if (!DeleteFileW(child)) failed = 1; + if (DeleteFileW(child) || RemoveDirectoryW(child)) { + free(child); + continue; } + DWORD delete_error = GetLastError(); + if (delete_error == ERROR_FILE_NOT_FOUND || + delete_error == ERROR_PATH_NOT_FOUND) { + free(child); + continue; + } + HANDLE child_handle = zupt_win_open_cleanup_entry( + directory_handle, data.cFileName, 1, 0); + if (child_handle == INVALID_HANDLE_VALUE) { + if (errno != ENOENT) failed = 1; + free(child); + continue; + } + int child_failed = 0; + BY_HANDLE_FILE_INFORMATION child_identity; + if (!GetFileInformationByHandle(child_handle, &child_identity) || + (child_identity.dwFileAttributes & + FILE_ATTRIBUTE_DIRECTORY) == 0 || + (child_identity.dwFileAttributes & + FILE_ATTRIBUTE_REPARSE_POINT) != 0 || + zupt_remove_tree_wide(child_handle, child) != 0) + child_failed = 1; + if (!CloseHandle(child_handle)) child_failed = 1; + if (!child_failed && !zupt_win_delete_cleanup_entry( + directory_handle, data.cFileName, &child_identity)) + child_failed = 1; + if (child_failed) failed = 1; free(child); } while (FindNextFileW(search, &data)); + if (GetLastError() != ERROR_NO_MORE_FILES) failed = 1; if (!FindClose(search)) failed = 1; - } else if (GetLastError() != ERROR_FILE_NOT_FOUND) { + } else if (search_error != ERROR_FILE_NOT_FOUND) { failed = 1; } - if (!RemoveDirectoryW(directory)) failed = 1; + return failed ? -1 : 0; +} + +/* Resolve the absolute temporary path one component at a time and retain + * every ancestor handle. This makes the pathname used for enumeration + * stable even if another process tries to exchange an ancestor directory. */ +static int zupt_win_open_cleanup_path( + const wchar_t *directory, wchar_t full[ZUPT_MAX_PATH + 256], + HANDLE **handles_out, size_t *handle_count_out) { + if (!_wfullpath(full, directory, ZUPT_MAX_PATH + 256)) { + errno = EINVAL; + return 0; + } + for (wchar_t *p = full; *p; p++) if (*p == L'/') *p = L'\\'; + if ((full[0] == L'\\' && full[1] == L'\\') || + !(full[0] && full[1] == L':' && full[2] == L'\\')) { + errno = EINVAL; + return 0; + } + + size_t capacity = wcslen(full) + 1u; + HANDLE *handles = (HANDLE *)calloc(capacity, sizeof(*handles)); + if (!handles) return 0; + wchar_t drive_root[4] = {full[0], L':', L'\\', L'\0'}; + HANDLE current = CreateFileW( + drive_root, + FILE_LIST_DIRECTORY | FILE_TRAVERSE | FILE_READ_ATTRIBUTES | + SYNCHRONIZE, + FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, + FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT, NULL); + if (current == INVALID_HANDLE_VALUE || + !zupt_win_plain_directory(current)) { + DWORD open_error = current == INVALID_HANDLE_VALUE + ? GetLastError() : ERROR_ACCESS_DENIED; + if (current != INVALID_HANDLE_VALUE) CloseHandle(current); + free(handles); + errno = open_error == ERROR_FILE_NOT_FOUND || + open_error == ERROR_PATH_NOT_FOUND + ? ENOENT : EACCES; + return 0; + } + size_t count = 0; + handles[count++] = current; + + wchar_t *scan = full + 3; + while (*scan) { + wchar_t *separator = wcschr(scan, L'\\'); + if (separator) *separator = L'\0'; + HANDLE next = zupt_win_open_cleanup_entry( + current, scan, 1, 0); + if (separator) *separator = L'\\'; + if (next == INVALID_HANDLE_VALUE || + !zupt_win_plain_directory(next)) { + if (next != INVALID_HANDLE_VALUE) CloseHandle(next); + while (count > 0) CloseHandle(handles[--count]); + free(handles); + if (next != INVALID_HANDLE_VALUE) errno = EACCES; + return 0; + } + handles[count++] = next; + current = next; + if (!separator) break; + scan = separator + 1; + } + *handles_out = handles; + *handle_count_out = count; + return 1; +} +#endif + +#ifndef _WIN32 +/* Resolve every component without following symlinks and return both the + * pinned target and its pinned parent. The caller can therefore remove the + * final directory with unlinkat() instead of resolving its pathname again. */ +static int zupt_open_temp_tree(const char *path, int *parent_out, + int *directory_out, char *leaf, + size_t leaf_capacity) { + if (!path || !*path || !parent_out || !directory_out || !leaf || + leaf_capacity == 0) { + errno = EINVAL; + return 0; + } + int current = open(path[0] == '/' ? "/" : ".", + O_RDONLY | O_DIRECTORY | O_CLOEXEC); + if (current < 0) return 0; + + const char *cursor = path; + while (*cursor == '/') cursor++; + while (*cursor) { + const char *start = cursor; + while (*cursor && *cursor != '/') cursor++; + size_t component_length = (size_t)(cursor - start); + while (*cursor == '/') cursor++; + int final_component = *cursor == '\0'; + if ((component_length == 1u && start[0] == '.') || + component_length == 0u) { + if (final_component) { + close(current); + errno = EINVAL; + return 0; + } + continue; + } + if (component_length == 2u && start[0] == '.' && start[1] == '.') { + close(current); + errno = EINVAL; + return 0; + } + if (component_length >= leaf_capacity) { + close(current); + errno = ENAMETOOLONG; + return 0; + } + memcpy(leaf, start, component_length); + leaf[component_length] = '\0'; + int next = openat(current, leaf, + O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + if (next < 0) { + int saved_errno = errno; + close(current); + errno = saved_errno; + return 0; + } + if (final_component) { + *parent_out = current; + *directory_out = next; + return 1; + } + close(current); + current = next; + } + close(current); + errno = EINVAL; + return 0; +} + +/* Delete leaves before attempting to open them as directories. unlinkat() + * never follows a symlink; a directory is recursively visited only through + * an O_NOFOLLOW descriptor returned by openat(). */ +static int zupt_remove_temp_tree_fd(int directory_fd) { + DIR *stream = fdopendir(directory_fd); + if (!stream) { + close(directory_fd); + return -1; + } + int failed = 0; + int parent_fd = dirfd(stream); + for (;;) { + errno = 0; + struct dirent *entry = readdir(stream); + if (!entry) { + if (errno != 0) failed = 1; + break; + } + if (strcmp(entry->d_name, ".") == 0 || + strcmp(entry->d_name, "..") == 0) + continue; + if (unlinkat(parent_fd, entry->d_name, 0) == 0 || errno == ENOENT) + continue; + + int child_fd = openat(parent_fd, entry->d_name, + O_RDONLY | O_DIRECTORY | O_NOFOLLOW | + O_CLOEXEC); + if (child_fd < 0) { + if (errno != ENOENT) failed = 1; + continue; + } + if (zupt_remove_temp_tree_fd(child_fd) != 0) failed = 1; + if (unlinkat(parent_fd, entry->d_name, AT_REMOVEDIR) != 0 && + errno != ENOENT) + failed = 1; + } + if (closedir(stream) != 0) failed = 1; return failed ? -1 : 0; } #endif @@ -169,41 +459,43 @@ static int zupt_remove_temp_tree(const char *directory) { #ifdef _WIN32 wchar_t *wide = zupt_win_utf8_to_wide_alloc(directory); if (!wide) return -1; - int result = zupt_remove_tree_wide(wide); + wchar_t full[ZUPT_MAX_PATH + 256]; + HANDLE *handles = NULL; + size_t handle_count = 0; + if (!zupt_win_open_cleanup_path(wide, full, &handles, &handle_count)) { + int result = errno == ENOENT ? 0 : -1; + free(wide); + return result; + } + HANDLE root_handle = handles[handle_count - 1u]; + int result = zupt_remove_tree_wide(root_handle, full); + BY_HANDLE_FILE_INFORMATION root_identity; + if (result == 0 && !GetFileInformationByHandle(root_handle, + &root_identity)) + result = -1; + const wchar_t *root_name = wcsrchr(full, L'\\'); + if (!root_name || root_name[1] == L'\0') result = -1; + else root_name++; + if (!CloseHandle(handles[--handle_count])) result = -1; + if (result == 0 && !zupt_win_delete_cleanup_entry( + handles[handle_count - 1u], root_name, &root_identity)) + result = -1; + while (handle_count > 0) + if (!CloseHandle(handles[--handle_count])) result = -1; + free(handles); free(wide); return result; #else - DIR *stream = opendir(directory); - if (!stream) return errno == ENOENT ? 0 : -1; - int failed = 0; - struct dirent *entry; - while ((entry = readdir(stream)) != NULL) { - if (strcmp(entry->d_name, ".") == 0 || - strcmp(entry->d_name, "..") == 0) - continue; - size_t needed = strlen(directory) + strlen(entry->d_name) + 2u; - char *child = (char *)malloc(needed); - if (!child) { - failed = 1; - continue; - } - if (!zupt_join_temp_path(child, needed, directory, entry->d_name)) { - free(child); - failed = 1; - continue; - } - struct stat info; - if (lstat(child, &info) != 0) { - failed = 1; - } else if (S_ISDIR(info.st_mode)) { - if (zupt_remove_temp_tree(child) != 0) failed = 1; - } else if (unlink(child) != 0) { - failed = 1; - } - free(child); - } - if (closedir(stream) != 0) failed = 1; - if (rmdir(directory) != 0) failed = 1; + int parent_fd = -1; + int directory_fd = -1; + char leaf[ZUPT_MAX_PATH]; + if (!zupt_open_temp_tree(directory, &parent_fd, &directory_fd, + leaf, sizeof(leaf))) + return errno == ENOENT ? 0 : -1; + int failed = zupt_remove_temp_tree_fd(directory_fd) != 0; + if (unlinkat(parent_fd, leaf, AT_REMOVEDIR) != 0 && errno != ENOENT) + failed = 1; + if (close(parent_fd) != 0) failed = 1; return failed ? -1 : 0; #endif } @@ -429,7 +721,11 @@ static int prompt_password(const char *prompt, char *buf, size_t cap) { if (!buf || cap < 2) return 0; buf[0] = '\0'; #ifdef _WIN32 - if (!_isatty(_fileno(stdin))) { + HANDLE input_handle = GetStdHandle(STD_INPUT_HANDLE); + DWORD input_mode = 0; + if (input_handle == NULL || input_handle == INVALID_HANDLE_VALUE || + GetFileType(input_handle) != FILE_TYPE_CHAR || + !GetConsoleMode(input_handle, &input_mode)) { fprintf(stderr, "Error: password prompt requires a terminal.\n"); return 0; } @@ -445,6 +741,11 @@ static int prompt_password(const char *prompt, char *buf, size_t cap) { int too_long = 0; for (;;) { int c = _getch(); + if (c == EOF) { + zupt_secure_wipe(buf, cap); + fprintf(stderr, "\nError: cannot read password prompt.\n"); + return 0; + } if (c == '\r' || c == '\n') break; if (c == 0 || c == 0xe0) { (void)_getch(); diff --git a/tests/test_benchmark_temp_safety.sh b/tests/test_benchmark_temp_safety.sh index 470f545..954b5fb 100755 --- a/tests/test_benchmark_temp_safety.sh +++ b/tests/test_benchmark_temp_safety.sh @@ -3,6 +3,11 @@ set -Eeuo pipefail bin=${1:-./zupt} +repo_root=$(CDPATH='' cd -- "$(dirname -- "$0")/.." && pwd -P) +case $bin in + /*) ;; + *) bin="$(pwd -P)/${bin#./}" ;; +esac tmp=$(mktemp -d "${TMPDIR:-/tmp}/zupt-bench-safety.XXXXXXXX") trap 'rm -rf -- "$tmp"' EXIT HUP INT TERM @@ -11,9 +16,36 @@ fail() { exit 1 } +# CodeQL #7 reported the old lstat(child) -> recursive pathname operation as +# cpp/toctou-race-condition. Keep the platform-specific cleanup primitives in +# the source gate as well as exercising the runtime symlink boundary below. +cleanup_source=$repo_root/src/zupt_main.c +grep -Fq 'static int zupt_remove_temp_tree_fd(int directory_fd)' \ + "$cleanup_source" || fail 'POSIX descriptor-relative cleanup is missing' +grep -Fq 'unlinkat(parent_fd, entry->d_name, 0)' "$cleanup_source" || + fail 'POSIX leaf cleanup is not unlinkat-relative' +grep -Fq 'directory_handle, data.cFileName, 1, 0)' "$cleanup_source" || + fail 'Windows recursive cleanup is not handle-relative' +grep -Fq 'FILE_OPEN_REPARSE_POINT' "$cleanup_source" || + fail 'Windows cleanup no longer opens reparse points without following' +grep -Fq 'zupt_win_delete_cleanup_entry(' "$cleanup_source" || + fail 'Windows cleanup lacks identity-checked handle deletion' +grep -Fq 'current.nFileIndexLow == expected->nFileIndexLow' "$cleanup_source" || + fail 'Windows cleanup no longer rejects a close/reopen name exchange' +if grep -Fq 'RemoveDirectoryW(full)' "$cleanup_source"; then + fail 'Windows root cleanup restored post-handle pathname deletion' +fi +if grep -Fq 'lstat(child' "$cleanup_source" || + grep -Fq 'zupt_remove_temp_tree(child' "$cleanup_source"; then + fail 'temporary cleanup restored a check-then-use pathname traversal' +fi + case $(uname -s 2>/dev/null || printf unknown) in MINGW*|MSYS*|CYGWIN*) - printf 'SKIP: historical POSIX /tmp symlink benchmark test is not native on Windows\n' + "$bin" bench --compare >/dev/null 2>&1 || + fail 'native Windows handle-relative benchmark cleanup failed' + printf 'SKIP: adversarial POSIX symlink injection is not native on Windows\n' + printf 'private Windows handle-relative benchmark workspace: PASS\n' exit 0 ;; esac @@ -46,4 +78,50 @@ if [[ -d $old_directory ]]; then mv "$old_directory" "$tmp/historical-remnant" fi -printf 'private benchmark workspace: PASS\n' +# Inject a directory symlink into the private workspace while a real benchmark +# is active. Cleanup must remove the link itself and never visit its target. +mkdir "$tmp/symlink-target" +printf 'cleanup sentinel must survive\n' > "$tmp/symlink-target/sentinel" +cp "$tmp/symlink-target/sentinel" "$tmp/symlink-target.expected" +dd if=/dev/urandom of="$tmp/injection-input" bs=65536 count=128 2>/dev/null + +physical_tmp=$(CDPATH='' cd -P -- /tmp && pwd -P) +: > "$tmp/preexisting-workspaces" +for candidate in "$physical_tmp"/zupt-bench-*; do + if [[ -d $candidate && ! -L $candidate ]]; then + printf '%s\n' "$candidate" >> "$tmp/preexisting-workspaces" + fi +done + +(cd "$tmp" && "$bin" bench injection-input >/dev/null 2>&1) & +bench_pid=$! +injected=0 +injected_workspace= +attempt=0 +while (( attempt < 1000 )); do + for candidate in "$physical_tmp"/zupt-bench-*; do + [[ -d $candidate && ! -L $candidate ]] || continue + if grep -Fqx -- "$candidate" "$tmp/preexisting-workspaces"; then + continue + fi + if ln -s "$tmp/symlink-target" "$candidate/attacker-link" \ + 2>/dev/null; then + injected=1 + injected_workspace=$candidate + break + fi + done + (( injected == 1 )) && break + kill -0 "$bench_pid" 2>/dev/null || break + sleep 0.01 + attempt=$((attempt + 1)) +done +wait "$bench_pid" || fail 'benchmark with injected symlink failed' +(( injected == 1 )) || fail 'could not observe the private benchmark workspace' +if [[ -e $injected_workspace || -L $injected_workspace ]]; then + fail 'injected workspace was not the benchmark tree that was removed' +fi +cmp "$tmp/symlink-target.expected" "$tmp/symlink-target/sentinel" || + fail 'temporary cleanup followed an injected directory symlink' + +printf 'private descriptor/handle-relative benchmark workspace: PASS\n' diff --git a/tests/test_password_sources.sh b/tests/test_password_sources.sh index 014ad1c..2261eb6 100644 --- a/tests/test_password_sources.sh +++ b/tests/test_password_sources.sh @@ -56,9 +56,30 @@ if "$binary" test --pass-fd not-a-number archive.zupt >/dev/null 2>&1; then exit 1 fi -if "$binary" test --password-prompt archive.zupt /dev/null 2>&1; then +prompt_log=$test_root/non-interactive-prompt.log +if command -v timeout >/dev/null 2>&1; then + set +e + timeout 10 "$binary" test --password-prompt archive.zupt \ + "$prompt_log" 2>&1 + prompt_status=$? + set -e +else + set +e + "$binary" test --password-prompt archive.zupt \ + "$prompt_log" 2>&1 + prompt_status=$? + set -e +fi +if ((prompt_status == 124)); then + printf '%s\n' 'FAIL: non-interactive password prompt timed out' >&2 + exit 1 +elif ((prompt_status == 0)); then printf '%s\n' 'FAIL: non-interactive password prompt unexpectedly succeeded' >&2 exit 1 +elif ! grep -Fq 'password prompt requires a terminal.' "$prompt_log"; then + printf 'FAIL: non-interactive password prompt returned status %d without a terminal rejection\n' \ + "$prompt_status" >&2 + exit 1 fi case $(uname -s) in diff --git a/tests/test_source_only.sh b/tests/test_source_only.sh index 9243386..3ed42fb 100755 --- a/tests/test_source_only.sh +++ b/tests/test_source_only.sh @@ -154,16 +154,19 @@ case "$(uname -s)" in tree=$(fresh_tree raw-c1-path) control_name=$'raw-\200.txt' - printf '\177ELF\002\001\001\000compiled' >"$tree/$control_name" - if "$SCANNER" --tree "$tree" >"$TEST_TMP/output" 2>&1; then - printf 'not ok - raw C1 path was not rejected\n' - exit 1 - elif ! grep -Fq 'raw-\x80.txt' "$TEST_TMP/output" || - LC_ALL=C grep -q $'\200' "$TEST_TMP/output"; then - printf 'not ok - raw C1 path was not rendered safely\n' - exit 1 + if { printf '\177ELF\002\001\001\000compiled' >"$tree/$control_name"; } 2>/dev/null; then + if "$SCANNER" --tree "$tree" >"$TEST_TMP/output" 2>&1; then + printf 'not ok - raw C1 path was not rejected\n' + exit 1 + elif ! grep -Fq 'raw-\x80.txt' "$TEST_TMP/output" || + LC_ALL=C grep -q $'\200' "$TEST_TMP/output"; then + printf 'not ok - raw C1 path was not rendered safely\n' + exit 1 + else + pass 'scanner escapes invalid raw C1 bytes in reported paths' + fi else - pass 'scanner escapes invalid raw C1 bytes in reported paths' + skip 'raw C1 filenames are forbidden by this filesystem' fi tree=$(fresh_tree utf8-c1-path) diff --git a/tests/test_static_analysis.sh b/tests/test_static_analysis.sh index 49af6ba..690898e 100755 --- a/tests/test_static_analysis.sh +++ b/tests/test_static_analysis.sh @@ -187,6 +187,31 @@ else F "ECHO bit-clear missing the explicit (tcflag_t) cast" fi +# A restore to a device is irreversible. Classify the already-open descriptor +# rather than checking target_path and resolving that mutable name again. +if grep -Fq 'lstat(target_path' src/zupt_disk.c; then + F "disk restore has a path-check/open TOCTOU pattern" +elif grep -Fq 'tgt_fd = open(target_path' src/zupt_disk.c && + grep -Fq 'fstat(tgt_fd, &opened_st)' src/zupt_disk.c; then + P "disk restore classifies the opened target descriptor" +else + F "disk restore descriptor-first target guard is missing" +fi + +# CodeQL #5 reported chmod(dst, mode) after reopening/resolving the SDK save +# path. Key copies must use the core's handle/descriptor-relative atomic +# publisher and apply POSIX permissions to its already-open temporary stream. +if grep -Fq 'chmod(dst, mode)' sdk/src/zuptsdk.c; then + F "SDK key save has a path-based chmod TOCTOU pattern" +elif grep -Fq 'zupt_atomic_output_open(dst, &fo)' sdk/src/zuptsdk.c && + grep -Fq 'fchmod(fileno(fo), mode)' sdk/src/zuptsdk.c && + grep -Fq 'zupt_atomic_output_finish(output, rc == ZUPTSDK_OK)' \ + sdk/src/zuptsdk.c; then + P "SDK key save uses descriptor-relative atomic publication" +else + F "SDK key save atomic publication guard is missing" +fi + echo "" echo " ───────────────────────────────────────" echo " Static analysis: $PASS passed, $FAIL failed"