diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c05a177d..358aab0e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ - Native/macOS: honor the `system_crash_reporter_enabled` option. ([#1743](https://github.com/getsentry/sentry-native/pull/1743)) - Cap rate-limit retry-after values at 24 hours to prevent a MITM-provided response from disabling event delivery for the process lifetime. ([#1744](https://github.com/getsentry/sentry-native/pull/1744)) - Native: validate ELF header entry sizes. ([#1746](https://github.com/getsentry/sentry-native/pull/1746)) +- Prevent database cleanup from following symlinks in run and cache directories. ([#1751](https://github.com/getsentry/sentry-native/pull/1751)) - Structured logs: respect printf argument widths when extracting log parameters to avoid stack-data disclosure and corrupted attributes on 32-bit platforms. ([#1752](https://github.com/getsentry/sentry-native/pull/1752)) ## 0.14.2 diff --git a/src/path/sentry_path.c b/src/path/sentry_path.c index 2ec8631d4..2f47e550c 100644 --- a/src/path/sentry_path.c +++ b/src/path/sentry_path.c @@ -69,7 +69,7 @@ sentry__path_eq(const sentry_path_t *path_a, const sentry_path_t *path_b) int sentry__path_remove_all(const sentry_path_t *path) { - if (sentry__path_is_dir(path)) { + if (sentry__path_is_dir(path) && !sentry__path_is_symlink(path)) { sentry_pathiter_t *piter = sentry__path_iter_directory(path); if (piter) { const sentry_path_t *p; diff --git a/src/path/sentry_path_unix.c b/src/path/sentry_path_unix.c index 752025bf1..e6442d8f1 100644 --- a/src/path/sentry_path_unix.c +++ b/src/path/sentry_path_unix.c @@ -229,6 +229,18 @@ sentry__path_is_file(const sentry_path_t *path) return stat(path->path, &buf) == 0 && S_ISREG(buf.st_mode); } +bool +sentry__path_is_symlink(const sentry_path_t *path) +{ +#if defined(SENTRY_PLATFORM_NX) || defined(SENTRY_PLATFORM_PS) + (void)path; + return false; +#else + struct stat buf; + return lstat(path->path, &buf) == 0 && S_ISLNK(buf.st_mode); +#endif +} + size_t sentry__path_get_size(const sentry_path_t *path) { @@ -309,7 +321,7 @@ int sentry__path_remove(const sentry_path_t *path) { int status; - if (!sentry__path_is_dir(path)) { + if (!sentry__path_is_dir(path) || sentry__path_is_symlink(path)) { EINTR_RETRY(unlink(path->path), &status); if (status == 0) { return 0; diff --git a/src/path/sentry_path_windows.c b/src/path/sentry_path_windows.c index 5ccda3301..5d62b3b46 100644 --- a/src/path/sentry_path_windows.c +++ b/src/path/sentry_path_windows.c @@ -351,6 +351,18 @@ sentry__path_is_file(const sentry_path_t *path) return _wstat(path_w, &buf) == 0 && S_ISREG(buf.st_mode); } +bool +sentry__path_is_symlink(const sentry_path_t *path) +{ + wchar_t *path_w = path->path_w; + if (!path_w) { + return false; + } + const DWORD attributes = GetFileAttributesW(path_w); + return attributes != INVALID_FILE_ATTRIBUTES + && (attributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0; +} + size_t sentry__path_get_size(const sentry_path_t *path) { diff --git a/src/sentry_database.c b/src/sentry_database.c index ced802ebf..278fa7013 100644 --- a/src/sentry_database.c +++ b/src/sentry_database.c @@ -609,7 +609,7 @@ sentry__process_old_runs(const sentry_options_t *options, uint64_t last_crash) while ((run_dir = sentry__pathiter_next(db_iter)) != NULL) { // skip over other files such as the saved consent or the last_crash // timestamp - if (!sentry__path_is_dir(run_dir)) { + if (!sentry__path_is_dir(run_dir) || sentry__path_is_symlink(run_dir)) { continue; } diff --git a/src/sentry_path.h b/src/sentry_path.h index f8140cd1e..f21d7c66f 100644 --- a/src/sentry_path.h +++ b/src/sentry_path.h @@ -148,6 +148,11 @@ bool sentry__path_ends_with(const sentry_path_t *path, const char *suffix); */ bool sentry__path_is_dir(const sentry_path_t *path); +/** + * Return whether the path refers to a symlink. + */ +bool sentry__path_is_symlink(const sentry_path_t *path); + /** * Return whether the path refers to a regular file. */ diff --git a/tests/unit/test_cache.c b/tests/unit/test_cache.c index 265e6c133..8d078a5b0 100644 --- a/tests/unit/test_cache.c +++ b/tests/unit/test_cache.c @@ -12,6 +12,7 @@ #ifdef SENTRY_PLATFORM_WINDOWS # include #elif !defined(SENTRY_PLATFORM_NX) && !defined(SENTRY_PLATFORM_PS) +# include # include #endif @@ -439,6 +440,57 @@ SENTRY_TEST(cache_prune_siblings) sentry_close(); } +SENTRY_TEST(cache_symlink_run) +{ +#if !defined(SENTRY_PLATFORM_UNIX) || defined(SENTRY_PLATFORM_NX) \ + || defined(SENTRY_PLATFORM_PS) + SKIP_TEST(); +#else + SENTRY_TEST_OPTIONS_NEW(options); + sentry_options_set_dsn(options, "https://foo@sentry.invalid/42"); + sentry_init(options); + + sentry_path_t *target_dir + = sentry__path_from_str(SENTRY_TEST_PATH_PREFIX ".old-run-target"); + TEST_ASSERT(!!target_dir); + sentry_path_t *target_file = sentry__path_join_str(target_dir, "important"); + TEST_ASSERT(!!target_file); + sentry_path_t *linked_run_dir + = sentry__path_join_str(options->database_path, "malicious.run"); + TEST_ASSERT(!!linked_run_dir); + sentry_path_t *linked_run_lock + = sentry__path_append_str(linked_run_dir, ".lock"); + TEST_ASSERT(!!linked_run_lock); + + // Model an old run entry like: + // /malicious.run -> /.old-run-target + // The target stays outside the database so following the link would remove + // files outside SDK-owned cache directories. + sentry__path_remove(linked_run_dir); + sentry__path_remove(linked_run_lock); + TEST_ASSERT(sentry__path_remove_all(target_dir) == 0); + TEST_ASSERT(sentry__path_create_dir_all(target_dir) == 0); + TEST_ASSERT(sentry__path_write_buffer(target_file, "keep", 4) == 0); + sentry_path_t *target_dir_abs = sentry__path_absolute(target_dir); + TEST_ASSERT(!!target_dir_abs); + TEST_ASSERT(symlink(target_dir_abs->path, linked_run_dir->path) == 0); + + sentry__process_old_runs(options, 0); + + TEST_CHECK(sentry__path_is_file(target_file)); + + sentry__path_remove(linked_run_dir); + sentry__path_remove(linked_run_lock); + sentry__path_remove_all(target_dir); + sentry__path_free(linked_run_lock); + sentry__path_free(linked_run_dir); + sentry__path_free(target_dir_abs); + sentry__path_free(target_file); + sentry__path_free(target_dir); + sentry_close(); +#endif +} + SENTRY_TEST(cache_consent_revoked) { #if defined(SENTRY_PLATFORM_NX) || defined(SENTRY_PLATFORM_PS) diff --git a/tests/unit/test_path.c b/tests/unit/test_path.c index 2f0ff06df..6b782e828 100644 --- a/tests/unit/test_path.c +++ b/tests/unit/test_path.c @@ -2,6 +2,11 @@ #include "sentry_string.h" #include "sentry_testsupport.h" +#if defined(SENTRY_PLATFORM_UNIX) && !defined(SENTRY_PLATFORM_NX) \ + && !defined(SENTRY_PLATFORM_PS) +# include +#endif + SENTRY_TEST(recursive_paths) { sentry_path_t *base = sentry__path_from_str(SENTRY_TEST_PATH_PREFIX ".foo"); @@ -247,6 +252,47 @@ SENTRY_TEST(path_directory) sentry__path_free(path_2); } +SENTRY_TEST(path_remove_all_symlink) +{ +#if !defined(SENTRY_PLATFORM_UNIX) || defined(SENTRY_PLATFORM_NX) \ + || defined(SENTRY_PLATFORM_PS) + SKIP_TEST(); +#else + sentry_path_t *base + = sentry__path_from_str(SENTRY_TEST_PATH_PREFIX ".remove-all"); + TEST_ASSERT(!!base); + sentry_path_t *target = sentry__path_join_str(base, "target"); + TEST_ASSERT(!!target); + sentry_path_t *target_file = sentry__path_join_str(target, "important"); + TEST_ASSERT(!!target_file); + sentry_path_t *link = sentry__path_join_str(base, "link"); + TEST_ASSERT(!!link); + + // Model a recursive deletion target like: + // .remove-all/link -> .remove-all/target + // Removing the link must not recurse into the target directory. + TEST_ASSERT(sentry__path_remove_all(base) == 0); + TEST_ASSERT(sentry__path_create_dir_all(target) == 0); + TEST_ASSERT(sentry__path_write_buffer(target_file, "keep", 4) == 0); + sentry_path_t *target_abs = sentry__path_absolute(target); + TEST_ASSERT(!!target_abs); + TEST_ASSERT(symlink(target_abs->path, link->path) == 0); + TEST_CHECK(sentry__path_is_symlink(link)); + + TEST_ASSERT(sentry__path_remove_all(link) == 0); + + TEST_CHECK(sentry__path_is_file(target_file)); + TEST_CHECK(!sentry__path_is_dir(link)); + + sentry__path_remove_all(base); + sentry__path_free(link); + sentry__path_free(target_file); + sentry__path_free(target_abs); + sentry__path_free(target); + sentry__path_free(base); +#endif +} + SENTRY_TEST(path_mtime) { #if defined(SENTRY_PLATFORM_NX) diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc index 3529c6845..d81b04467 100644 --- a/tests/unit/tests.inc +++ b/tests/unit/tests.inc @@ -66,6 +66,7 @@ XX(cache_max_size) XX(cache_max_size_and_age) XX(cache_prune_siblings) XX(cache_remove_siblings) +XX(cache_symlink_run) XX(cache_write_minidump) XX(cache_write_raw_with_minidump) XX(capture_minidump_basic) @@ -227,6 +228,7 @@ XX(path_joining_unix) XX(path_joining_windows) XX(path_mtime) XX(path_relative_filename) +XX(path_remove_all_symlink) XX(path_rename) XX(path_unique) XX(percent_decode_basic)