From 559c4bfdc05b9cf3b6608a205668ca90e020f2a8 Mon Sep 17 00:00:00 2001 From: Armaan Sandhu Date: Tue, 13 Oct 2026 20:15:06 -0400 Subject: [PATCH] journal-file: refuse hash chains and field lists that loop back Data and field objects are only ever appended, and a new object is always linked after the current tail of its hash chain, so a valid next_hash_offset is either 0 or larger than the offset of the object itself. A new data object is also prepended to the list of data objects of its field, so a valid next_field_offset is always smaller than the offset of the object itself. Nothing checks either of these when an object is read, so if a chain loops back on itself, everything that walks it loops forever: - "journalctl --verify" spins in data_object_in_hash_table() instead of reporting the file as corrupted, - journal_file_punch_holes() never returns when the file is archived, so it is never offlined, - sd_journal_enumerate_fields() returns the same field over and over, so "journalctl --fields" never finishes, - sd_journal_enumerate_unique() returns the same value over and over, so "journalctl -F FIELD" never finishes. A single flipped bit in next_hash_offset or next_field_offset is enough for any of these. Check both in check_object(), like we already do for next_entry_array_offset, so that all readers are covered. (cherry picked from commit a1bf29afbeed5e4c702f58ea5a9038e433c99c11) --- src/libsystemd/sd-journal/journal-file.c | 21 +++ .../sd-journal/test-journal-verify.c | 87 +++++++++ src/libsystemd/sd-journal/test-journal.c | 177 ++++++++++++++++++ 3 files changed, 285 insertions(+) diff --git a/src/libsystemd/sd-journal/journal-file.c b/src/libsystemd/sd-journal/journal-file.c index 139da29620..bed0c812a6 100644 --- a/src/libsystemd/sd-journal/journal-file.c +++ b/src/libsystemd/sd-journal/journal-file.c @@ -979,6 +979,21 @@ static int check_object(JournalFile *f, Object *o, uint64_t offset) { le64toh(o->data.entry_array_offset), offset); + /* Objects are only ever appended, hence a hash chain has to move strictly forward. */ + if (le64toh(o->data.next_hash_offset) > 0 && le64toh(o->data.next_hash_offset) <= offset) + return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG), + "Invalid data object next_hash_offset: %" PRIu64 ": %" PRIu64, + le64toh(o->data.next_hash_offset), + offset); + + /* New data objects are prepended to the list of their field, hence that list has to move + * strictly backward. */ + if (le64toh(o->data.next_field_offset) >= offset) + return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG), + "Invalid data object next_field_offset: %" PRIu64 ": %" PRIu64, + le64toh(o->data.next_field_offset), + offset); + break; case OBJECT_FIELD: @@ -996,6 +1011,12 @@ static int check_object(JournalFile *f, Object *o, uint64_t offset) { le64toh(o->field.next_hash_offset), le64toh(o->field.head_data_offset), offset); + + if (le64toh(o->field.next_hash_offset) > 0 && le64toh(o->field.next_hash_offset) <= offset) + return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG), + "Invalid field object next_hash_offset: %" PRIu64 ": %" PRIu64, + le64toh(o->field.next_hash_offset), + offset); break; case OBJECT_ENTRY: { diff --git a/src/libsystemd/sd-journal/test-journal-verify.c b/src/libsystemd/sd-journal/test-journal-verify.c index 6a1401764b..fc459e3041 100644 --- a/src/libsystemd/sd-journal/test-journal-verify.c +++ b/src/libsystemd/sd-journal/test-journal-verify.c @@ -15,6 +15,7 @@ #include "log.h" #include "mmap-cache.h" #include "rm-rf.h" +#include "stdio-util.h" #include "tests.h" #include "time-util.h" @@ -171,6 +172,90 @@ static int run_test(const char *verification_key, ssize_t max_iterations) { return 0; } +static void test_hash_chain_cycle(void) { + _cleanup_(mmap_cache_unrefp) MMapCache *m = NULL; + char t[] = "/var/tmp/journal-XXXXXX", second[64]; + const char *first = "CYCLE=first"; + uint64_t buckets, bucket, head, tail; + struct dual_timestamp ts; + JournalFile *f; + le64_t back; + int fd; + + if (sd_id128_get_machine(NULL) < 0) + return (void) log_tests_skipped("No valid machine ID found"); + + test_setup_logging(LOG_DEBUG); + + ASSERT_NOT_NULL(m = mmap_cache_new()); + + ASSERT_NOT_NULL(mkdtemp(t)); + ASSERT_OK_ERRNO(chdir(t)); + + ASSERT_OK_ZERO(journal_file_open( + /* fd= */ -EBADF, + "test.journal", + O_RDWR|O_CREAT, + /* file_flags= */ 0, + 0666, + /* compress_threshold_bytes= */ UINT64_MAX, + /* metrics= */ NULL, + m, + /* template= */ NULL, + &f)); + dual_timestamp_now(&ts); + + /* We need two data objects in the same hash chain. journal_file_hash_data() is keyed per-file, so + * compute the colliding value against the open file. */ + buckets = le64toh(f->header->data_hash_table_size) / sizeof(HashItem); + bucket = journal_file_hash_data(f, first, strlen(first)) % buckets; + for (uint64_t i = 0;; i++) { + ASSERT_LT(i, UINT64_C(1000000)); + xsprintf(second, "CYCLE=%" PRIu64, i); + if (journal_file_hash_data(f, second, strlen(second)) % buckets == bucket) + break; + } + + const char *v; + FOREACH_ARGUMENT(v, first, second) { + struct iovec iovec = IOVEC_MAKE_STRING(v); + ASSERT_OK_ZERO(journal_file_append_entry( + f, + &ts, + /* boot_id= */ NULL, + &iovec, + /* n_iovec= */ 1, + /* seqnum= */ NULL, + /* seqnum_id= */ NULL, + /* ret_object= */ NULL, + /* ret_offset= */ NULL)); + } + + ASSERT_EQ(journal_file_find_data_object(f, first, strlen(first), NULL, &head), 1); + ASSERT_EQ(journal_file_find_data_object(f, second, strlen(second), NULL, &tail), 1); + ASSERT_LT(head, tail); + (void) journal_file_offline_close(f); + + /* The file has to be fine before we touch it, so that the failure below is really ours */ + ASSERT_OK(raw_verify("test.journal", /* verification_key= */ NULL)); + + /* Point the second data object of the chain back at the head. Looking up the second data object, + * which verifying the entry that references it does, would then run into the cycle. */ + back = htole64(head); + ASSERT_OK_ERRNO(fd = open("test.journal", O_RDWR|O_CLOEXEC)); + ASSERT_EQ(pwrite(fd, &back, sizeof(back), tail + offsetof(Object, data.next_hash_offset)), + (ssize_t) sizeof(back)); + safe_close(fd); + + /* If the cycle is not detected, verification never returns. Fail quickly and visibly then, instead + * of running into the timeout of the whole test. */ + alarm(60); + ASSERT_ERROR(raw_verify("test.journal", /* verification_key= */ NULL), EBADMSG); + alarm(0); + + ASSERT_OK(rm_rf(t, REMOVE_ROOT|REMOVE_PHYSICAL)); +} + int main(int argc, char *argv[]) { const char *verification_key = NULL; int max_iterations = 512; @@ -202,5 +287,7 @@ int main(int argc, char *argv[]) { run_test(verification_key, max_iterations); } + test_hash_chain_cycle(); + return 0; } diff --git a/src/libsystemd/sd-journal/test-journal.c b/src/libsystemd/sd-journal/test-journal.c index c58d30a4ff..586194d42e 100644 --- a/src/libsystemd/sd-journal/test-journal.c +++ b/src/libsystemd/sd-journal/test-journal.c @@ -3,6 +3,8 @@ #include #include +#include "sd-journal.h" + #include "argv-util.h" #include "chattr-util.h" #include "iovec-util.h" @@ -656,6 +658,181 @@ TEST(recover_truncated_hash_chain) { } } +static void test_punch_holes_hash_chain_cycle_one(void) { + _cleanup_(mmap_cache_unrefp) MMapCache *m = NULL; + char t[] = "/var/tmp/journal-XXXXXX"; + const char *field = "CYCLE=x"; + struct iovec iovec = IOVEC_MAKE_STRING(field); + dual_timestamp ts; + JournalFile *f; + uint64_t p; + le64_t self; + int fd; + + /* When a journal file is archived, holes are punched into the unused parts of the entry arrays of + * all data objects, which follows every data hash chain. A chain that loops back on itself must not + * make that spin forever. */ + + ASSERT_NOT_NULL(m = mmap_cache_new()); + mkdtemp_chdir_chattr(t); + + ASSERT_OK_ZERO(journal_file_open( + -EBADF, "test.journal", O_RDWR|O_CREAT, JOURNAL_COMPRESS, 0666, UINT64_MAX, + /* metrics= */ NULL, m, /* template= */ NULL, &f)); + dual_timestamp_now(&ts); + ASSERT_OK_ZERO(journal_file_append_entry( + f, &ts, /* boot_id= */ NULL, &iovec, 1, + /* seqnum= */ NULL, /* seqnum_id= */ NULL, + /* ret_object= */ NULL, /* ret_offset= */ NULL)); + ASSERT_EQ(journal_file_find_data_object(f, field, strlen(field), NULL, &p), 1); + (void) journal_file_offline_close(f); + + /* Make the data object its own successor in the hash chain */ + self = htole64(p); + ASSERT_OK_ERRNO(fd = open("test.journal", O_RDWR|O_CLOEXEC)); + ASSERT_EQ(pwrite(fd, &self, sizeof(self), p + offsetof(Object, data.next_hash_offset)), + (ssize_t) sizeof(self)); + ASSERT_OK_ERRNO(close(fd)); + + ASSERT_OK_ZERO(journal_file_open( + -EBADF, "test.journal", O_RDWR, JOURNAL_COMPRESS, 0666, UINT64_MAX, + /* metrics= */ NULL, m, /* template= */ NULL, &f)); + ASSERT_OK(journal_file_archive(f, /* ret_previous_path= */ NULL)); + + /* If the cycle is followed this never returns. Fail quickly and visibly then. */ + alarm(10); + (void) journal_file_offline_close(f); + alarm(0); + + if (arg_keep) + log_info("Not removing %s", t); + else + ASSERT_OK(rm_rf(t, REMOVE_ROOT | REMOVE_PHYSICAL)); +} + +TEST(punch_holes_hash_chain_cycle) { + const char *compact; + + FOREACH_ARGUMENT(compact, "0", "1") { + ASSERT_OK_ERRNO(setenv("SYSTEMD_JOURNAL_COMPACT", compact, 1)); + test_punch_holes_hash_chain_cycle_one(); + } +} + +static void test_enumerate_fields_hash_chain_cycle_one(void) { + _cleanup_(mmap_cache_unrefp) MMapCache *m = NULL; + _cleanup_(sd_journal_closep) sd_journal *j = NULL; + char t[] = "/var/tmp/journal-XXXXXX"; + const char *data = "CYCLE=x"; + struct iovec iovec = IOVEC_MAKE_STRING(data); + const char *field; + dual_timestamp ts; + JournalFile *f; + uint64_t p; + le64_t self; + int fd; + + /* sd_journal_enumerate_fields() follows the field hash chains. A chain that loops back on itself + * must be refused, instead of returning the same field over and over again. */ + + ASSERT_NOT_NULL(m = mmap_cache_new()); + mkdtemp_chdir_chattr(t); + + ASSERT_OK_ZERO(journal_file_open( + -EBADF, "test.journal", O_RDWR|O_CREAT, JOURNAL_COMPRESS, 0666, UINT64_MAX, + /* metrics= */ NULL, m, /* template= */ NULL, &f)); + dual_timestamp_now(&ts); + ASSERT_OK_ZERO(journal_file_append_entry( + f, &ts, /* boot_id= */ NULL, &iovec, 1, + /* seqnum= */ NULL, /* seqnum_id= */ NULL, + /* ret_object= */ NULL, /* ret_offset= */ NULL)); + ASSERT_EQ(journal_file_find_field_object(f, "CYCLE", STRLEN("CYCLE"), NULL, &p), 1); + (void) journal_file_offline_close(f); + + /* Make the field object its own successor in the hash chain */ + self = htole64(p); + ASSERT_OK_ERRNO(fd = open("test.journal", O_RDWR|O_CLOEXEC)); + ASSERT_EQ(pwrite(fd, &self, sizeof(self), p + offsetof(Object, field.next_hash_offset)), + (ssize_t) sizeof(self)); + ASSERT_OK_ERRNO(close(fd)); + + ASSERT_OK(sd_journal_open_directory(&j, t, SD_JOURNAL_ASSUME_IMMUTABLE)); + ASSERT_ERROR(sd_journal_enumerate_fields(j, &field), EBADMSG); + sd_journal_close(TAKE_PTR(j)); + + if (arg_keep) + log_info("Not removing %s", t); + else + ASSERT_OK(rm_rf(t, REMOVE_ROOT | REMOVE_PHYSICAL)); +} + +TEST(enumerate_fields_hash_chain_cycle) { + const char *compact; + + FOREACH_ARGUMENT(compact, "0", "1") { + ASSERT_OK_ERRNO(setenv("SYSTEMD_JOURNAL_COMPACT", compact, 1)); + test_enumerate_fields_hash_chain_cycle_one(); + } +} + +static void test_enumerate_unique_field_list_cycle_one(void) { + _cleanup_(mmap_cache_unrefp) MMapCache *m = NULL; + _cleanup_(sd_journal_closep) sd_journal *j = NULL; + char t[] = "/var/tmp/journal-XXXXXX"; + const char *data = "CYCLE=x"; + struct iovec iovec = IOVEC_MAKE_STRING(data); + const void *value; + dual_timestamp ts; + JournalFile *f; + size_t size; + uint64_t p; + le64_t self; + int fd; + + /* sd_journal_enumerate_unique() follows the list of data objects of a field. A list that loops back + * on itself must be refused, instead of returning the same value over and over again. */ + + ASSERT_NOT_NULL(m = mmap_cache_new()); + mkdtemp_chdir_chattr(t); + + ASSERT_OK_ZERO(journal_file_open( + -EBADF, "test.journal", O_RDWR|O_CREAT, JOURNAL_COMPRESS, 0666, UINT64_MAX, + /* metrics= */ NULL, m, /* template= */ NULL, &f)); + dual_timestamp_now(&ts); + ASSERT_OK_ZERO(journal_file_append_entry( + f, &ts, /* boot_id= */ NULL, &iovec, 1, + /* seqnum= */ NULL, /* seqnum_id= */ NULL, + /* ret_object= */ NULL, /* ret_offset= */ NULL)); + ASSERT_EQ(journal_file_find_data_object(f, data, strlen(data), NULL, &p), 1); + (void) journal_file_offline_close(f); + + /* Make the data object its own successor in the list of its field */ + self = htole64(p); + ASSERT_OK_ERRNO(fd = open("test.journal", O_RDWR|O_CLOEXEC)); + ASSERT_EQ(pwrite(fd, &self, sizeof(self), p + offsetof(Object, data.next_field_offset)), + (ssize_t) sizeof(self)); + ASSERT_OK_ERRNO(close(fd)); + + ASSERT_OK(sd_journal_open_directory(&j, t, SD_JOURNAL_ASSUME_IMMUTABLE)); + ASSERT_OK(sd_journal_query_unique(j, "CYCLE")); + ASSERT_ERROR(sd_journal_enumerate_unique(j, &value, &size), EBADMSG); + sd_journal_close(TAKE_PTR(j)); + + if (arg_keep) + log_info("Not removing %s", t); + else + ASSERT_OK(rm_rf(t, REMOVE_ROOT | REMOVE_PHYSICAL)); +} + +TEST(enumerate_unique_field_list_cycle) { + const char *compact; + + FOREACH_ARGUMENT(compact, "0", "1") { + ASSERT_OK_ERRNO(setenv("SYSTEMD_JOURNAL_COMPACT", compact, 1)); + test_enumerate_unique_field_list_cycle_one(); + } +} + static int intro(void) { arg_keep = saved_argc > 1;