From 6c22338b196c5d350d784dd91e961c2501fc4b23 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Mon, 3 Aug 2026 16:19:55 +0200 Subject: [PATCH] fix(pipeline): create the publication staging file exclusively MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cbm_pipeline_publish_generation built its staging database name by hand as ".stage..", unlinked it, then wrote it. Any local process can compute that name in advance, so a symlink planted between the unlink and the write redirects the write to a target of the attacker's choosing — an arbitrary-file clobber when the database sits in a world-writable directory. The same file already solves this correctly elsewhere: create_staging_path() mints the name with mkstemp, so the file is created O_EXCL and we only ever write one we made ourselves. Publication now shares it. The unlink-first step goes away with the predictable name — it existed to clear a leftover at a name we might reuse, and a freshly minted name cannot collide, nor can its sidecars pre-exist. SCOPE, stated precisely because the PR description overstates it: the only caller of cbm_pipeline_publish_generation sits behind CBM_INCREMENTAL_TEST_API, which is set in CFLAGS_TEST and never in CFLAGS_PROD. The predictable name was therefore not reachable in a shipped binary — production publication already went through create_staging_path. This is removing a bad pattern from a test-only path before it can be promoted, not patching a live user-facing vulnerability. The regression test calls the function directly, because no pipeline entry point reaches it in a production build. It does not try to win the race — a test that has to win a race is a coin flip, not a gate. It asserts the property that removes the race: canaries occupy every name the old scheme could have chosen and all must survive publication. Verified both ways rather than green-only: against the old code it reports "survived == 31, expected PREDICTABLE_CANARIES == 32", exactly one canary consumed; with the fix the suite goes 18 passed/1 failed -> 19 passed. Signed-off-by: Martin Vogel --- src/pipeline/pipeline.c | 26 ++++++++------- tests/test_pipeline.c | 73 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 11 deletions(-) diff --git a/src/pipeline/pipeline.c b/src/pipeline/pipeline.c index 6b5e94d47..7decf6aa2 100644 --- a/src/pipeline/pipeline.c +++ b/src/pipeline/pipeline.c @@ -1429,7 +1429,9 @@ int cbm_pipeline_refresh_artifact(cbm_pipeline_t *p, const char *db_path) { return 0; } -static atomic_ullong g_generation_stage_counter = 0; + +/* Defined below, next to the other publication helpers. */ +static char *create_staging_path(const char *final_path); static void discard_generation_stage(const char *stage_path) { if (!stage_path) { @@ -1598,19 +1600,21 @@ int cbm_pipeline_publish_generation(const cbm_pipeline_generation_t *generation) return CBM_PIPELINE_ABORT_PRESERVE_DB; } - size_t stage_size = strlen(generation->final_db_path) + 96; - char *stage_path = malloc(stage_size); + /* The staging name must be unpredictable and created exclusively. It used + * to be ".stage..", which any other process can compute + * in advance; this path is then unlinked and written, so in a + * world-writable database directory an attacker could land a symlink in + * the window between the two and have us clobber the target. Sharing the + * mkstemp-based helper the other staging site already uses closes that: + * O_EXCL creation means we only ever write a file we made ourselves. + * + * The old unlink-first step goes with it. It existed to clear a leftover + * file at a name we might reuse; a freshly minted name cannot collide, + * and its sidecars cannot pre-exist either. */ + char *stage_path = create_staging_path(generation->final_db_path); if (!stage_path) { return CBM_PIPELINE_PERSIST_FAILED; } - unsigned long long serial = atomic_fetch_add(&g_generation_stage_counter, 1) + 1; - int path_n = snprintf(stage_path, stage_size, "%s.stage.%ld.%llu", generation->final_db_path, - (long)cbm_pipeline_getpid(), serial); - if (path_n < 0 || (size_t)path_n >= stage_size) { - free(stage_path); - return CBM_PIPELINE_PERSIST_FAILED; - } - discard_generation_stage(stage_path); int dump_rc = cbm_gbuf_dump_to_sqlite(generation->gbuf, stage_path); if (dump_rc != 0) { diff --git a/tests/test_pipeline.c b/tests/test_pipeline.c index 84ab0b8ee..b9f44f113 100644 --- a/tests/test_pipeline.c +++ b/tests/test_pipeline.c @@ -2580,6 +2580,78 @@ TEST(pipeline_source_mutation_before_publication_preserves_previous_generation) PASS(); } +/* Publication must never write through a name another local process can + * predict. The staging database used to be ".stage..", + * which anyone can compute in advance; publication then unlinked that name + * and wrote it, so a symlink planted in the gap redirects the write to a + * target of the attacker's choosing. + * + * Winning that timing window is not something a test should attempt -- a + * test that has to win a race is a coin flip, not a gate. The property that + * REMOVES the window is deterministic: publication must not touch any + * predictable name at all. Canaries occupy every name the old scheme could + * have picked, and all must survive. Against the old code exactly one is + * consumed, whichever serial the counter had reached. + * + * This calls cbm_pipeline_publish_generation directly. The only in-tree + * caller sits behind CBM_INCREMENTAL_TEST_API, so going through the pipeline + * would never reach the code under test. */ +TEST(pipeline_publication_never_uses_a_predictable_staging_path) { + char tmp[256]; + snprintf(tmp, sizeof(tmp), "/tmp/cbm_publish_predictable_stage_XXXXXX"); + ASSERT_NOT_NULL(cbm_mkdtemp(tmp)); + char db_path[512]; + snprintf(db_path, sizeof(db_path), "%s/generation.db", tmp); + + enum { PREDICTABLE_CANARIES = 32 }; + static const char canary[] = "canary-must-survive\n"; + char canary_path[PREDICTABLE_CANARIES][640]; + for (int i = 0; i < PREDICTABLE_CANARIES; i++) { + snprintf(canary_path[i], sizeof(canary_path[i]), "%s.stage.%ld.%d", db_path, + (long)getpid(), i + 1); + ASSERT_EQ(th_write_file(canary_path[i], canary), 0); + } + + cbm_gbuf_t *gb = cbm_gbuf_new("predictable-stage-proj", tmp); + ASSERT_NOT_NULL(gb); + cbm_pipeline_generation_t generation = { + .gbuf = gb, + .final_db_path = db_path, + .project = "predictable-stage-proj", + .cancelled = NULL, + .manifest = NULL, + .manifest_count = 0, + .adr_content = NULL, + .coverage = NULL, + .coverage_count = 0, + }; + int publish_rc = cbm_pipeline_publish_generation(&generation); + cbm_gbuf_free(gb); + + int survived = 0; + int intact = 0; + for (int i = 0; i < PREDICTABLE_CANARIES; i++) { + FILE *f = cbm_fopen(canary_path[i], "rb"); + if (!f) { + continue; + } + survived++; + char buf[64] = {0}; + size_t n = fread(buf, 1, sizeof(buf) - 1, f); + (void)fclose(f); + if (n == strlen(canary) && memcmp(buf, canary, n) == 0) { + intact++; + } + } + th_rmtree(tmp); + + ASSERT_EQ(publish_rc, 0); + /* Not one may be unlinked, truncated, or written through. */ + ASSERT_EQ(survived, PREDICTABLE_CANARIES); + ASSERT_EQ(intact, PREDICTABLE_CANARIES); + PASS(); +} + /* Discovery and extraction must describe the same immutable generation. A * source file created after extraction is not present in the original file * list, so merely re-hashing that list cannot detect the race. Publication @@ -11434,6 +11506,7 @@ SUITE(pipeline_semantic_manifest_repro) { #if defined(CBM_INCREMENTAL_TEST_API) && CBM_INCREMENTAL_TEST_API RUN_TEST(pipeline_git_context_change_forces_full_and_refreshes_branch); RUN_TEST(pipeline_global_extension_config_change_forces_full); + RUN_TEST(pipeline_publication_never_uses_a_predictable_staging_path); RUN_TEST(pipeline_source_mutation_before_publication_preserves_previous_generation); RUN_TEST(pipeline_source_addition_before_publication_preserves_previous_generation); RUN_TEST(pipeline_tsconfig_mutation_before_publication_preserves_previous_generation);