From: Colin Walters Date: Fri, 31 Jul 2026 18:18:38 +0000 (-0400) Subject: static-delta: Account for the full part payload consistently X-Git-Tag: archive/raspbian/2026.4-1+rpi1^2~9^2~1^2~7^2 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=e2af53421b5e6acf54af7feb342fcf1da3d6319e;p=ostree.git static-delta: Account for the full part payload consistently The previous commit adding a limit here didn't account correctly for metadata overhead; in trying to do it as a private security fix we didn't run all of the tests, which was a procedural mistake. Assisted-by: AI Signed-off-by: Colin Walters --- diff --git a/src/libostree/ostree-repo-finder-mount.c b/src/libostree/ostree-repo-finder-mount.c index 3f92b278..7f964acd 100644 --- a/src/libostree/ostree-repo-finder-mount.c +++ b/src/libostree/ostree-repo-finder-mount.c @@ -339,8 +339,7 @@ ostree_repo_finder_mount_resolve_async (OstreeRepoFinder *finder, #if GLIB_CHECK_VERSION(2, 55, 0) G_GNUC_BEGIN_IGNORE_DEPRECATIONS /* remove once GLIB_VERSION_MAX_ALLOWED ≥ 2.56 */ - g_autoptr (GUnixMountEntry) mount_entry - = g_unix_mount_at (mount_root_path, NULL); + g_autoptr (GUnixMountEntry) mount_entry = g_unix_mount_at (mount_root_path, NULL); if (mount_entry != NULL && (g_unix_is_system_fs_type (g_unix_mount_get_fs_type (mount_entry)) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 69e6ed55..5f729e68 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -1270,9 +1270,12 @@ static_deltapart_fetch_on_complete (GObject *object, GAsyncResult *result, gpoin /* Transfer ownership of the fd */ in = g_unix_input_stream_new (g_steal_fd (&tmpf.fd), TRUE); + guint32 n_objects + = (guint32)(g_variant_get_size (fetch_data->objects) / OSTREE_STATIC_DELTA_OBJTYPE_CSUM_LEN); /* TODO - make async */ if (!_ostree_static_delta_part_open (in, NULL, 0, fetch_data->expected_checksum, - fetch_data->usize, &part, pull_data->cancellable, error)) + fetch_data->usize, n_objects, &part, pull_data->cancellable, + error)) goto out; _ostree_static_delta_part_execute_async (pull_data->repo, fetch_data->objects, part, @@ -2211,10 +2214,12 @@ process_one_static_delta (OtPullData *pull_data, const char *from_revision, cons g_autoptr (GInputStream) memin = g_memory_input_stream_new_from_bytes (inline_part_bytes); g_autoptr (GVariant) inline_delta_part = NULL; + guint32 n_objects + = (guint32)(g_variant_get_size (objects) / OSTREE_STATIC_DELTA_OBJTYPE_CSUM_LEN); /* For inline parts we are relying on per-commit GPG, so don't bother checksumming. */ - if (!_ostree_static_delta_part_open (memin, inline_part_bytes, - OSTREE_STATIC_DELTA_OPEN_FLAGS_SKIP_CHECKSUM, NULL, - usize, &inline_delta_part, cancellable, error)) + if (!_ostree_static_delta_part_open ( + memin, inline_part_bytes, OSTREE_STATIC_DELTA_OPEN_FLAGS_SKIP_CHECKSUM, NULL, + usize, n_objects, &inline_delta_part, cancellable, error)) { fetch_static_delta_data_free (fetch_data); return FALSE; diff --git a/src/libostree/ostree-repo-static-delta-compilation.c b/src/libostree/ostree-repo-static-delta-compilation.c index 25c77d06..7b986a17 100644 --- a/src/libostree/ostree-repo-static-delta-compilation.c +++ b/src/libostree/ostree-repo-static-delta-compilation.c @@ -56,6 +56,14 @@ typedef struct GPtrArray *modes; GHashTable *xattr_set; /* GVariant(ayay) -> offset */ GPtrArray *xattrs; + /* Running total of the serialized size of the unique entries in modes/ + * xattrs above. payload->len and operations->len track their own + * GStrings directly, but the mode/xattr tables are separate GVariant + * arrays only assembled into their final form in finish_part(), so this + * is tracked incrementally as entries are added; see + * write_unique_variant_chunk() and current_part_size_estimate(). + */ + guint64 tables_size; GLnxTmpfile part_tmpf; GVariant *header; } OstreeStaticDeltaPartBuilder; @@ -383,10 +391,25 @@ write_unique_variant_chunk (OstreeStaticDeltaPartBuilder *current_part, GHashTab target_offsetp = GUINT_TO_POINTER (offset); g_hash_table_insert (hash, g_variant_ref (key), target_offsetp); g_ptr_array_add (ordered, key); + current_part->tables_size += g_variant_get_size (key); return offset; } +/* Estimate the eventual serialized size of current_part's payload GVariant + * (see finish_part()), so callers deciding whether to start a new part can + * account for the mode/xattr tables and operations bytecode, not just the + * raw content bytes in ->payload. This doesn't need to be exact -- it's a + * lower bound (GVariant framing adds a little more) used only to decide + * when to proactively split a part; finish_part() enforces the real hard + * limit against the actual serialized size once a part is complete. + */ +static gsize +current_part_size_estimate (OstreeStaticDeltaPartBuilder *part) +{ + return part->payload->len + part->operations->len + part->tables_size; +} + static gboolean splice_stream_to_payload (OstreeStaticDeltaPartBuilder *current_part, GInputStream *istream, GCancellable *cancellable, GError **error) @@ -454,7 +477,7 @@ process_one_object (OstreeRepo *repo, OstreeStaticDeltaBuilder *builder, /* Check to see if this delta is maximum size */ if (current_part->objects->len > 0 - && current_part->payload->len + content_size > builder->max_chunk_size_bytes) + && current_part_size_estimate (current_part) + content_size > builder->max_chunk_size_bytes) { current_part = allocate_part (builder, error); if (current_part == NULL) @@ -668,7 +691,8 @@ process_one_rollsum (OstreeRepo *repo, OstreeStaticDeltaBuilder *builder, OstreeStaticDeltaPartBuilder *current_part = *current_part_val; /* Check to see if this delta has gone over maximum size */ - if (current_part->objects->len > 0 && current_part->payload->len > builder->max_chunk_size_bytes) + if (current_part->objects->len > 0 + && current_part_size_estimate (current_part) > builder->max_chunk_size_bytes) { current_part = allocate_part (builder, error); if (current_part == NULL) @@ -784,7 +808,8 @@ process_one_bsdiff (OstreeRepo *repo, OstreeStaticDeltaBuilder *builder, OstreeStaticDeltaPartBuilder *current_part = *current_part_val; /* Check to see if this delta has gone over maximum size */ - if (current_part->objects->len > 0 && current_part->payload->len > builder->max_chunk_size_bytes) + if (current_part->objects->len > 0 + && current_part_size_estimate (current_part) > builder->max_chunk_size_bytes) { current_part = allocate_part (builder, error); if (current_part == NULL) diff --git a/src/libostree/ostree-repo-static-delta-core.c b/src/libostree/ostree-repo-static-delta-core.c index cd974705..fee27de0 100644 --- a/src/libostree/ostree-repo-static-delta-core.c +++ b/src/libostree/ostree-repo-static-delta-core.c @@ -585,8 +585,10 @@ ostree_repo_static_delta_execute_offline_with_signature (OstreeRepo *self, GFile */ delta_open_flags |= OSTREE_STATIC_DELTA_OPEN_FLAGS_SKIP_CHECKSUM; + guint32 n_objects + = (guint32)(g_variant_get_size (objects) / OSTREE_STATIC_DELTA_OBJTYPE_CSUM_LEN); if (!_ostree_static_delta_part_open (part_in, inline_part_bytes, delta_open_flags, NULL, - usize, &part, cancellable, error)) + usize, n_objects, &part, cancellable, error)) return FALSE; } else @@ -598,8 +600,10 @@ ostree_repo_static_delta_execute_offline_with_signature (OstreeRepo *self, GFile part_in = g_unix_input_stream_new (part_fd, FALSE); + guint32 n_objects + = (guint32)(g_variant_get_size (objects) / OSTREE_STATIC_DELTA_OBJTYPE_CSUM_LEN); if (!_ostree_static_delta_part_open (part_in, NULL, delta_open_flags, checksum, usize, - &part, cancellable, error)) + n_objects, &part, cancellable, error)) return FALSE; } @@ -633,11 +637,38 @@ ostree_repo_static_delta_execute_offline (OstreeRepo *self, GFile *dir_or_file, self, dir_or_file, NULL, skip_validation, cancellable, error); } +/* Compute how much larger than the declared usize a part's decompressed + * payload is allowed to be. See the constants in + * ostree-repo-static-delta-private.h for the derivation of each term; + * all arithmetic saturates to G_MAXUINT64 on overflow rather than + * wrapping, since expected_usize comes from the (not yet fully + * validated) delta part header. + */ +static guint64 +_ostree_static_delta_compute_part_margin (guint64 expected_usize, guint32 expected_n_objects) +{ + guint64 margin = OSTREE_STATIC_DELTA_PART_FIXED_OVERHEAD_BYTES; + + guint64 per_object_overhead; + if (!g_uint64_checked_mul (&per_object_overhead, (guint64)expected_n_objects, + OSTREE_STATIC_DELTA_PART_OP_OVERHEAD_PER_OBJECT_BYTES + + OSTREE_STATIC_DELTA_PART_XATTR_ALLOWANCE_PER_OBJECT_BYTES) + || !g_uint64_checked_add (&margin, margin, per_object_overhead)) + return G_MAXUINT64; + + const guint64 rollsum_overhead + = expected_usize / OSTREE_STATIC_DELTA_PART_ROLLSUM_OVERHEAD_DIVISOR; + if (!g_uint64_checked_add (&margin, margin, rollsum_overhead)) + return G_MAXUINT64; + + return margin; +} + gboolean _ostree_static_delta_part_open (GInputStream *part_in, GBytes *inline_part_bytes, OstreeStaticDeltaOpenFlags flags, const char *expected_checksum, - guint64 expected_usize, GVariant **out_part, - GCancellable *cancellable, GError **error) + guint64 expected_usize, guint32 expected_n_objects, + GVariant **out_part, GCancellable *cancellable, GError **error) { const gboolean trusted = (flags & OSTREE_STATIC_DELTA_OPEN_FLAGS_VARIANT_TRUSTED) > 0; const gboolean skip_checksum = (flags & OSTREE_STATIC_DELTA_OPEN_FLAGS_SKIP_CHECKSUM) > 0; @@ -645,10 +676,25 @@ _ostree_static_delta_part_open (GInputStream *part_in, GBytes *inline_part_bytes /* Use the declared usize from the delta header as the decompression limit * when available, capped to the hard maximum. If the caller passes 0 * (e.g. the show/dump path) fall back to the hard cap alone. + * + * The declared usize only covers the final on-disk size of the objects + * a part produces, not the part payload itself (which additionally + * contains the mode/xattr tables and operations bytecode). Add a + * margin on top of usize, derived from the number of objects in the + * part, so legitimate parts aren't rejected; see + * _ostree_static_delta_compute_part_margin(). */ guint64 max_part_usize = OSTREE_STATIC_DELTA_PART_MAX_USIZE_BYTES; - if (expected_usize > 0 && expected_usize < max_part_usize) - max_part_usize = expected_usize; + if (expected_usize > 0) + { + const guint64 margin + = _ostree_static_delta_compute_part_margin (expected_usize, expected_n_objects); + guint64 bounded_usize; + if (!g_uint64_checked_add (&bounded_usize, expected_usize, margin)) + bounded_usize = G_MAXUINT64; + if (bounded_usize < max_part_usize) + max_part_usize = bounded_usize; + } /* We either take a fd or a GBytes reference */ g_return_val_if_fail (G_IS_FILE_DESCRIPTOR_BASED (part_in) || inline_part_bytes != NULL, FALSE); @@ -774,7 +820,7 @@ show_one_part (OstreeRepo *self, gboolean swap_endian, const char *from, const c g_autoptr (GVariant) part = NULL; if (!_ostree_static_delta_part_open (part_in, NULL, OSTREE_STATIC_DELTA_OPEN_FLAGS_SKIP_CHECKSUM, - NULL, 0, &part, cancellable, error)) + NULL, 0, 0, &part, cancellable, error)) return FALSE; { diff --git a/src/libostree/ostree-repo-static-delta-private.h b/src/libostree/ostree-repo-static-delta-private.h index 137ec60a..7d325448 100644 --- a/src/libostree/ostree-repo-static-delta-private.h +++ b/src/libostree/ostree-repo-static-delta-private.h @@ -34,6 +34,71 @@ G_BEGIN_DECLS * decompression bombs that would expand to gigabytes. */ #define OSTREE_STATIC_DELTA_PART_MAX_USIZE_BYTES (512ULL * 1024ULL * 1024ULL) + +/* The declared "usize" in a delta part header only accounts for the final + * on-disk size of the objects the part will produce; the part payload + * that actually gets decompressed is larger. The constants below bound + * that difference so a decompression limit can be derived from usize + * without either false-positiving on legitimate parts or degenerating + * into a check that never rejects anything (see + * _ostree_static_delta_compute_part_margin() in + * ostree-repo-static-delta-core.c for how they're combined). Each term + * is sized from the actual on-disk formats in + * ostree-repo-static-delta-compilation.c and ostree-varint.c rather than + * from a single round guess, so the resulting margin stays proportionate + * as the number of objects in a part grows. + */ + +/* Flat per-part overhead: GVariant framing for the part's outer tuple and + * the operations/payload byte arrays. A few KiB is generous here; this + * doesn't scale with content. + */ +#define OSTREE_STATIC_DELTA_PART_FIXED_OVERHEAD_BYTES (4ULL * 1024ULL) + +/* Per-object operations overhead: each object contributes a mode table + * entry ("(uuu)", 12 bytes, deduplicated but bounded per-object in the + * worst case) plus opcode bytecode. _ostree_write_varuint64() emits at + * most 10 bytes, and the largest per-object opcode sequence is the + * bsdiff path (SET_READ_SOURCE, OPEN, BSPATCH, CLOSE, UNSET_READ_SOURCE: + * 5 opcodes + 6 varints = 65 bytes) plus its embedded 32-byte source + * checksum, which isn't counted in usize at all. 109 bytes worst case; + * round up generously. + */ +#define OSTREE_STATIC_DELTA_PART_OP_OVERHEAD_PER_OBJECT_BYTES 256ULL + +/* Per-object xattr allowance: xattrs are written into the payload in full + * and aren't reflected in usize either. There's no way to derive a true + * worst-case bound for this from filesystem limits: ext4 caps total + * attribute bytes per inode at ~4 KiB (one external block plus a little + * in-inode space), but that bound doesn't hold in general -- XFS and + * Btrfs impose no total per-inode limit, and even ext4 with the + * (non-default) ea_inode feature can push individual values up to + * XATTR_SIZE_MAX (64 KiB) each across its ~100-250 max entries. A file + * could in principle carry many such values on some filesystem; fully + * covering that here would require a per-object allowance in the tens of + * MiB, which for parts with more than a handful of objects would swamp + * this margin and degenerate the check back into "always equal to the + * hard cap" -- the exact failure mode we moved away from a flat + * 1 MiB/object margin to avoid. So use a single XATTR_SIZE_MAX (64 KiB) + * as a generous but pragmatic allowance: real xattr sets (SELinux label, + * capabilities, ACLs, IMA/EVM signatures) total well under 2 KiB in + * practice, so this covers legitimate content with 30x+ headroom to + * spare. Pathological xattr counts beyond that are left to + * OSTREE_STATIC_DELTA_PART_MAX_USIZE_BYTES, the same hard-cap backstop + * that bounds this whole margin. + */ +#define OSTREE_STATIC_DELTA_PART_XATTR_ALLOWANCE_PER_OBJECT_BYTES (64ULL * 1024ULL) + +/* Rollsum overhead divisor: rollsum (bsdiff-like binary delta against a + * similar file) emits a WRITE op per matched/unmatched chunk, and chunk + * boundaries come from bupsplit's content-defined chunking, which + * averages BUP_BLOBSIZE (8 KiB) per chunk. At up to ~64 bytes of opcode + * overhead per chunk, that's an expected overhead of roughly usize/128; + * dividing by 32 instead bakes in a further 4x safety margin for content + * that chunks more finely than average. + */ +#define OSTREE_STATIC_DELTA_PART_ROLLSUM_OVERHEAD_DIVISOR 32ULL + /* 1 byte for object type, 32 bytes for checksum */ #define OSTREE_STATIC_DELTA_OBJTYPE_CSUM_LEN 33 @@ -153,8 +218,8 @@ typedef enum gboolean _ostree_static_delta_part_open (GInputStream *part_in, GBytes *inline_part_bytes, OstreeStaticDeltaOpenFlags flags, const char *expected_checksum, guint64 expected_usize, - GVariant **out_part, GCancellable *cancellable, - GError **error); + guint32 expected_n_objects, GVariant **out_part, + GCancellable *cancellable, GError **error); typedef struct { diff --git a/src/libostree/ostree-repo-static-delta-processing.c b/src/libostree/ostree-repo-static-delta-processing.c index c5445e7f..5fcec2de 100644 --- a/src/libostree/ostree-repo-static-delta-processing.c +++ b/src/libostree/ostree-repo-static-delta-processing.c @@ -421,8 +421,7 @@ dispatch_bspatch (OstreeRepo *repo, StaticDeltaExecutionState *state, GCancellab * the full 64-bit value, causing a heap buffer overflow. * (CVE / RHEL-189207, CWE-680, CWE-122) */ - if (G_UNLIKELY (state->content_size > G_MAXSIZE - || state->content_size > (guint64)G_MAXINT64)) + if (G_UNLIKELY (state->content_size > G_MAXSIZE || state->content_size > (guint64)G_MAXINT64)) { g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT, "Invalid content size %" G_GUINT64_FORMAT diff --git a/src/libotutil/ot-fs-utils.c b/src/libotutil/ot-fs-utils.c index 41aa944f..aa5709ae 100644 --- a/src/libotutil/ot-fs-utils.c +++ b/src/libotutil/ot-fs-utils.c @@ -187,7 +187,7 @@ ot_fd_readall_or_mmap (int fd, goffset start, GError **error) */ GBytes * ot_map_anonymous_tmpfile_from_content_with_limit (GInputStream *instream, guint64 max_bytes, - GCancellable *cancellable, GError **error) + GCancellable *cancellable, GError **error) { g_auto (GLnxTmpfile) tmpf = { 0, @@ -201,8 +201,7 @@ ot_map_anonymous_tmpfile_from_content_with_limit (GInputStream *instream, guint6 while (TRUE) { guchar buf[65536]; - gssize n_read - = g_input_stream_read (instream, buf, sizeof (buf), cancellable, error); + gssize n_read = g_input_stream_read (instream, buf, sizeof (buf), cancellable, error); if (n_read < 0) return NULL; if (n_read == 0) diff --git a/tests/test-bsdiff.c b/tests/test-bsdiff.c index 405ab20e..8d242d86 100644 --- a/tests/test-bsdiff.c +++ b/tests/test-bsdiff.c @@ -114,8 +114,7 @@ test_bspatch_content_size_guard (void) for (gsize i = 0; i < G_N_ELEMENTS (reject_values); i++) { guint64 content_size = reject_values[i]; - gboolean would_reject - = (content_size > G_MAXSIZE || content_size > (guint64)G_MAXINT64); + gboolean would_reject = (content_size > G_MAXSIZE || content_size > (guint64)G_MAXINT64); g_assert_true (would_reject); } @@ -127,13 +126,12 @@ test_bspatch_content_size_guard (void) { const guint64 reject_32bit[] = { (guint64)G_MAXUINT32 + 1ULL, /* 4 GiB — overflows gsize on 32-bit */ - 0x100001000ULL, /* 4 GiB + 4 KiB — the PoC value */ + 0x100001000ULL, /* 4 GiB + 4 KiB — the PoC value */ }; for (gsize i = 0; i < G_N_ELEMENTS (reject_32bit); i++) { guint64 content_size = reject_32bit[i]; - gboolean would_reject - = (content_size > G_MAXSIZE || content_size > (guint64)G_MAXINT64); + gboolean would_reject = (content_size > G_MAXSIZE || content_size > (guint64)G_MAXINT64); g_assert_true (would_reject); } } @@ -149,8 +147,7 @@ test_bspatch_content_size_guard (void) for (gsize i = 0; i < G_N_ELEMENTS (accept_values); i++) { guint64 content_size = accept_values[i]; - gboolean would_reject - = (content_size > G_MAXSIZE || content_size > (guint64)G_MAXINT64); + gboolean would_reject = (content_size > G_MAXSIZE || content_size > (guint64)G_MAXINT64); g_assert_false (would_reject); /* Also verify that the casts produce correct values when accepted */