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

1 files 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;
         }