From 6d74eec0a97171ef46fac7aa4017c235758a1f3d Mon Sep 17 00:00:00 2001 From: Phaedrus Leeds Date: Fri, 12 Nov 2021 20:11:15 -0800 Subject: [PATCH] dir: Verify subsummary checksum from disk cache Currently we verify the checksum of indexed summary files (which have .sub file names) before writing them to the on-disk cache, so in theory as long as the disk I/O is successful the data integrity should be intact when we use it via the flatpak-variant-impl-private.h helpers generated by variant-schema-compiler. However in practice people sometimes hit assertion failures which are what you would expect to see if the data is corrupt, since GVariant stores some metadata such as the "offset size" toward the end of the data, and if we read this from serialized user data instead it will obviously be incorrect. In one case I was able to acquire the flathub.idx, flathub.idx.sig, and flathub-x86_64-fad08cfb10713e749f02a0e894b5d577b7e9c4931fdf9d2fdc50364c002bc925.sub files which reproduce one of the assertion failures, and the sub file appears to be incomplete, like the writing of it was interrupted. We use g_file_replace_contents() when saving these to the disk, and when not replacing an existing file that function writes directly to the final filename, so if interrupted it would be expected to leave an incomplete file. This commit changes the summary file handling so that we verify the checksum of any indexed subsummary again after reading it from disk. If it doesn't match we delete the on-disk cache and try fetching from the network. Fixes #4127 --- common/flatpak-dir.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c index 1307f222..460945a5 100644 --- a/common/flatpak-dir.c +++ b/common/flatpak-dir.c @@ -11404,6 +11404,7 @@ flatpak_dir_remote_save_cached_summary (FlatpakDir *self, static gboolean flatpak_dir_remote_load_cached_summary (FlatpakDir *self, const char *basename, + const char *checksum, const char *main_ext, const char *sig_ext, GBytes **out_main, @@ -11417,6 +11418,8 @@ flatpak_dir_remote_load_cached_summary (FlatpakDir *self, g_autoptr(GFile) sig_cache_file = flatpak_build_file (self->cache_dir, "summaries", sig_file_name, NULL); g_autoptr(GMappedFile) mfile = NULL; g_autoptr(GMappedFile) sig_mfile = NULL; + g_autoptr(GBytes) mfile_bytes = NULL; + g_autofree char *sha256 = NULL; mfile = g_mapped_file_new (flatpak_file_get_path_cached (main_cache_file), FALSE, NULL); if (mfile == NULL) @@ -11429,7 +11432,29 @@ flatpak_dir_remote_load_cached_summary (FlatpakDir *self, if (out_sig) sig_mfile = g_mapped_file_new (flatpak_file_get_path_cached (sig_cache_file), FALSE, NULL); - *out_main = g_mapped_file_get_bytes (mfile); + mfile_bytes = g_mapped_file_get_bytes (mfile); + + /* The checksum would've already been verified before the file was written, + * but check again in case something went wrong during disk I/O. This is + * especially important since the variant-schema-compiler code assumes the + * GVariant data is well formed and asserts otherwise. + */ + if (checksum != NULL) + { + sha256 = g_compute_checksum_for_bytes (G_CHECKSUM_SHA256, mfile_bytes); + if (strcmp (sha256, checksum) != 0) + { + g_file_delete (main_cache_file, NULL, NULL); + if (sig_ext) + g_file_delete (sig_cache_file, NULL, NULL); + + return flatpak_fail_error (error, FLATPAK_ERROR_INVALID_DATA, + _("Invalid checksum for indexed summary %s read from %s"), + checksum, flatpak_file_get_path_cached (main_cache_file)); + } + } + + *out_main = g_steal_pointer (&mfile_bytes); if (sig_mfile) *out_sig = g_mapped_file_get_bytes (sig_mfile); @@ -11480,7 +11505,7 @@ flatpak_dir_remote_fetch_summary (FlatpakDir *self, { if (only_cached) { - if (!flatpak_dir_remote_load_cached_summary (self, name_or_uri, NULL, ".sig", + if (!flatpak_dir_remote_load_cached_summary (self, name_or_uri, NULL, NULL, ".sig", &summary, &summary_sig, cancellable, error)) return FALSE; g_debug ("Loaded summary from cache for remote ‘%s’", name_or_uri); @@ -11637,7 +11662,7 @@ flatpak_dir_remote_fetch_summary_index (FlatpakDir *self, if (error == NULL) error = &local_error; - flatpak_dir_remote_load_cached_summary (self, name_or_uri, ".idx", ".idx.sig", + flatpak_dir_remote_load_cached_summary (self, name_or_uri, NULL, ".idx", ".idx.sig", &cached_index, &cached_index_sig, cancellable, &cache_error); if (only_cached) @@ -11788,7 +11813,7 @@ flatpak_dir_remote_fetch_indexed_summary (FlatpakDir *self, cache_name = g_strconcat (name_or_uri, "-", arch, "-", checksum, NULL); /* First look for an on-disk cache */ - if (!flatpak_dir_remote_load_cached_summary (self, cache_name, ".sub", NULL, + if (!flatpak_dir_remote_load_cached_summary (self, cache_name, checksum, ".sub", NULL, &summary, NULL, cancellable, &cache_error)) { g_autofree char *old_checksum = NULL; @@ -11801,6 +11826,10 @@ flatpak_dir_remote_fetch_indexed_summary (FlatpakDir *self, return FALSE; } + /* Warn if the on-disk cache is corrupt; perhaps the write was interrupted? */ + if (g_error_matches (cache_error, FLATPAK_ERROR, FLATPAK_ERROR_INVALID_DATA)) + g_warning ("%s", cache_error->message); + /* Look for first applicable deltas */ VarArrayofChecksumRef history = var_subsummary_get_history (subsummary_info); gsize history_len = var_arrayof_checksum_get_length (history); @@ -11814,7 +11843,7 @@ flatpak_dir_remote_fetch_indexed_summary (FlatpakDir *self, old_checksum = ostree_checksum_from_bytes (var_checksum_peek (old)); old_cache_name = g_strconcat (name_or_uri, "-", arch, "-", old_checksum, NULL); - if (flatpak_dir_remote_load_cached_summary (self, old_cache_name, ".sub", NULL, + if (flatpak_dir_remote_load_cached_summary (self, old_cache_name, old_checksum, ".sub", NULL, &old_summary, NULL, cancellable, NULL)) break; }