CVE-2021-43860: Incorrect Default Permissions (CWE-276)
value: Flatpak is a Linux application sandboxing and distribution framework. Prior to versions 1.12.3 and 1.10.6, Flatpak doesn't properly validate that the permissions displayed to the user for an app at install time match the actual permissions granted to the app at runtime, in the case that there's a null byte in the metadata file of an app. Therefore apps can grant themselves permissions without the consent of the user. Flatpak shows permissions to the user during install by reading them from the "xa.metadata" key in the commit metadata. This cannot contain a null terminator, because it is an untrusted GVariant. Flatpak compares these permissions to the *actual* metadata, from the "metadata" file to ensure it wasn't lied to. However, the actual metadata contents are loaded in several places where they are read as simple C-style strings. That means that, if the metadata file includes a null terminator, only the content of the file from *before* the terminator gets compared to xa.metadata. Thus, any permissions that appear in the metadata file after a null terminator are applied at runtime but not shown to the user. So maliciously crafted apps can give themselves hidden permissions. Users who have Flatpaks installed from untrusted sources are at risk in case the Flatpak has a maliciously crafted metadata file, either initially or in an update. This issue is patched in versions 1.12.3 and 1.10.6. As a workaround, users can manually check the permissions of installed apps by checking the metadata file or the xa.metadata key on the commit metadata.
CWE description: During installation, installed file permissions are set to allow anyone to modify those files.
CVSS v3 base score: 8.6
Published: 2022-01-12T22:15Z
Repository: https://github.com/flatpak/flatpak
Language: C
Files changed: common/flatpak-dir.c, common/flatpak-transaction.c, common/flatpak-utils.c
Commit date: 2022-01-12 19:48:16 +0100
Commit message: Fix metadata file contents after null terminators being ignored

In particular, if a null terminator is placed inside the metadata file,
Flatpak will only compare the text *before* it to the value of
xa.metadata, but the full file will be parsed when permissions are set
at runtime. This means that any app can include a null terminator in its
permissions metadata, and Flatpak will only show the user the
permissions *preceding* the terminator during install, but the
permissions *after* the terminator are applied at runtime.

Fixes GHSA-qpjc-vq3c-572j / CVE-2021-43860

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

Diff: diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c
index 8c5ab6701..05aa91a11 100644
--- a/common/flatpak-dir.c
+++ b/common/flatpak-dir.c
@@ -1792,23 +1792,33 @@ flatpak_get_user_base_dir_location (void)
 
 static gboolean
 validate_commit_metadata (GVariant   *commit_data,
                           const char *ref,
                           const char *required_metadata,
+                          gsize       required_metadata_size,
                           gboolean   require_xa_metadata,
                           GError   **error)
 {
   g_autoptr(GVariant) commit_metadata = NULL;
+  g_autoptr(GVariant) xa_metadata_v = NULL;
   const char *xa_metadata = NULL;
+  gsize xa_metadata_size = 0;
 
   commit_metadata = g_variant_get_child_value (commit_data, 0);
 
   if (commit_metadata != NULL)
-    g_variant_lookup (commit_metadata, "xa.metadata", "&s", &xa_metadata);
+    {
+      xa_metadata_v = g_variant_lookup_value (commit_metadata,
+                                              "xa.metadata",
+                                              G_VARIANT_TYPE_STRING);
+      if (xa_metadata_v)
+        xa_metadata = g_variant_get_string (xa_metadata_v, &xa_metadata_size);
+    }
 
   if ((xa_metadata == NULL && require_xa_metadata) ||
-      (xa_metadata != NULL && g_strcmp0 (required_metadata, xa_metadata) != 0))
+      (xa_metadata != NULL && (xa_metadata_size != required_metadata_size ||
+                               memcmp (xa_metadata, required_metadata, xa_metadata_size) != 0)))
     {
       g_set_error (error, G_IO_ERROR, G_IO_ERROR_PERMISSION_DENIED,
                    _("Commit metadata for %s not matching expected metadata"), ref);
       return FALSE;
     }
@@ -3513,10 +3523,11 @@ upgrade_deploy_data (GBytes             *deploy_data,
       g_autoptr(GVariant) commit_data = NULL;
       g_autoptr(GVariant) commit_metadata = NULL;
       g_autoptr(GKeyFile) keyfile = NULL;
       g_autoptr(GFile) metadata_file = NULL;
       g_autofree char *metadata_contents = NULL;
+      gsize metadata_size = 0;
       g_autofree char *id = flatpak_decomposed_dup_id (ref);
 
       /* Add fields from commit metadata to deploy */
       commit = flatpak_deploy_data_get_commit (deploy_data);
       if (!ostree_repo_load_commit (repo, commit, &commit_data, NULL, error))
@@ -3526,13 +3537,13 @@ upgrade_deploy_data (GBytes             *deploy_data,
 
       /* Add fields from metadata file to deploy */
       keyfile = g_key_file_new ();
       metadata_file = g_file_resolve_relative_path (deploy_dir, "metadata");
       if (!g_file_load_contents (metadata_file, cancellable,
-                                 &metadata_contents, NULL, NULL, error))
+                                 &metadata_contents, &metadata_size, NULL, error))
         return NULL;
-      if (!g_key_file_load_from_data (keyfile, metadata_contents, -1, 0, error))
+      if (!g_key_file_load_from_data (keyfile, metadata_contents, metadata_size, 0, error))
         return NULL;
       add_metadata_to_deploy_data (&metadata_dict, keyfile);
 
       /* Add fields from appdata to deploy, since appdata-content-rating wasn't
        * added when upgrading from version 2 as it should have been
@@ -5831,12 +5842,17 @@ flatpak_dir_pull (FlatpakDir                           *self,
 
   if (require_metadata)
     {
       g_autoptr(GVariant) commit_data = NULL;
       if (!ostree_repo_load_commit (repo, rev, &commit_data, NULL, error) ||
-          !validate_commit_metadata (commit_data, ref, (const char *)g_bytes_get_data (require_metadata, NULL), TRUE, error))
-        return FALSE;
+          !validate_commit_metadata (commit_data,
+                                     ref,
+                                     (const char *)g_bytes_get_data (require_metadata, NULL),
+                                     g_bytes_get_size (require_metadata),
+                                     TRUE,
+                                     error))
+        goto out;
     }
 
   if (!flatpak_dir_pull_extra_data (self, repo,
                                     state->remote_name,
                                     ref, rev,
@@ -8154,10 +8170,11 @@ flatpak_dir_deploy (FlatpakDir          *self,
   gboolean created_extra_data = FALSE;
   g_autoptr(GVariant) commit_metadata = NULL;
   g_auto(GLnxLockFile) lock = { 0, };
   g_autoptr(GFile) metadata_file = NULL;
   g_autofree char *metadata_contents = NULL;
+  gsize metadata_size = 0;
   gboolean is_oci;
   const char *flatpak;
 
   if (!flatpak_dir_ensure_repo (self, cancellable, error))
     return FALSE;
@@ -8364,15 +8381,16 @@ flatpak_dir_deploy (FlatpakDir          *self,
     }
 
   keyfile = g_key_file_new ();
   metadata_file = g_file_resolve_relative_path (checkoutdir, "metadata");
   if (g_file_load_contents (metadata_file, NULL,
-                            &metadata_contents, NULL, NULL, NULL))
+                            &metadata_contents,
+                            &metadata_size, NULL, NULL))
     {
       if (!g_key_file_load_from_data (keyfile,
                                       metadata_contents,
-                                      -1,
+                                      metadata_size,
                                       0, error))
         return FALSE;
 
       if (!flatpak_check_required_version (flatpak_decomposed_get_ref (ref), keyfile, error))
         return FALSE;
@@ -8384,11 +8402,11 @@ flatpak_dir_deploy (FlatpakDir          *self,
    * Note: For historical reason we don't enforce commits to contain xa.metadata
    * since this was lacking in fedora builds.
    */
   is_oci = flatpak_dir_get_remote_oci (self, origin);
   if (!validate_commit_metadata (commit_data, flatpak_decomposed_get_ref (ref),
-                                 metadata_contents, !is_oci, error))
+                                 metadata_contents, metadata_size, !is_oci, error))
     return FALSE;
 
   dotref = g_file_resolve_relative_path (checkoutdir, "files/.ref");
   if (!g_file_replace_contents (dotref, "", 0, NULL, FALSE,
                                 G_FILE_CREATE_REPLACE_DESTINATION, NULL, cancellable, error))
diff --git a/common/flatpak-transaction.c b/common/flatpak-transaction.c
index 551da495e..a7e24d8f7 100644
--- a/common/flatpak-transaction.c
+++ b/common/flatpak-transaction.c
@@ -2537,11 +2537,11 @@ flatpak_transaction_add_ref (FlatpakTransaction             *self,
                                    commit, bundle, kind, pin_on_deploy, error);
   if (op == NULL)
     return FALSE;
 
   if (external_metadata)
-    op->external_metadata = g_bytes_new (external_metadata, strlen (external_metadata) + 1);
+    op->external_metadata = g_bytes_new (external_metadata, strlen (external_metadata));
 
   return TRUE;
 }
 
 /**
@@ -2948,11 +2948,11 @@ load_deployed_metadata (FlatpakTransaction *self, FlatpakDecomposed *ref, char *
     {
       g_debug ("No metadata in local deploy of %s", flatpak_decomposed_get_ref (ref));
       return NULL;
     }
 
-  return g_bytes_new_take (g_steal_pointer (&metadata_contents), metadata_contents_length + 1);
+  return g_bytes_new_take (g_steal_pointer (&metadata_contents), metadata_contents_length);
 }
 
 static void
 emit_eol_and_maybe_skip (FlatpakTransaction          *self,
                          FlatpakTransactionOperation *op)
@@ -3049,11 +3049,11 @@ resolve_op_from_commit (FlatpakTransaction *self,
   commit_metadata = g_variant_get_child_value (commit_data, 0);
   g_variant_lookup (commit_metadata, "xa.metadata", "&s", &xa_metadata);
   if (xa_metadata == NULL)
     g_message ("Warning: No xa.metadata in local commit %s ref %s", checksum, flatpak_decomposed_get_ref (op->ref));
   else
-    metadata_bytes = g_bytes_new (xa_metadata, strlen (xa_metadata) + 1);
+    metadata_bytes = g_bytes_new (xa_metadata, strlen (xa_metadata));
 
   if (g_variant_lookup (commit_metadata, "xa.download-size", "t", &download_size))
     op->download_size = GUINT64_FROM_BE (download_size);
   if (g_variant_lookup (commit_metadata, "xa.installed-size", "t", &installed_size))
     op->installed_size = GUINT64_FROM_BE (installed_size);
@@ -3089,11 +3089,11 @@ try_resolve_op_from_metadata (FlatpakTransaction *self,
   /* And, we must have the actual cached data in the summary */
   if (!flatpak_remote_state_lookup_cache (state, flatpak_decomposed_get_ref (op->ref),
                                           &download_size, &installed_size, &metadata, NULL))
       return FALSE;
 
-  metadata_bytes = g_bytes_new (metadata, strlen (metadata) + 1);
+  metadata_bytes = g_bytes_new (metadata, strlen (metadata));
 
   if (flatpak_remote_state_lookup_ref (state, flatpak_decomposed_get_ref (op->ref),
                                        NULL, NULL, &info, NULL, NULL))
     op->summary_metadata = var_metadata_dup_to_gvariant (var_ref_info_get_metadata (info));
 
diff --git a/common/flatpak-utils.c b/common/flatpak-utils.c
index 5532e7b70..3259188ec 100644
--- a/common/flatpak-utils.c
+++ b/common/flatpak-utils.c
@@ -6665,10 +6665,11 @@ flatpak_pull_from_bundle (OstreeRepo   *repo,
                           const char   *ref,
                           gboolean      require_gpg_signature,
                           GCancellable *cancellable,
                           GError      **error)
 {
+  gsize metadata_size = 0;
   g_autofree char *metadata_contents = NULL;
   g_autofree char *to_checksum = NULL;
   g_autoptr(GFile) root = NULL;
   g_autoptr(GFile) metadata_file = NULL;
   g_autoptr(GInputStream) in = NULL;
@@ -6681,10 +6682,12 @@ flatpak_pull_from_bundle (OstreeRepo   *repo,
 
   metadata = flatpak_bundle_load (file, &to_checksum, NULL, NULL, NULL, &metadata_contents, NULL, NULL, &collection_id, error);
   if (metadata == NULL)
     return FALSE;
 
+  metadata_size = strlen (metadata_contents);
+
   if (!ostree_repo_get_remote_option (repo, remote, "collection-id", NULL,
                                       &remote_collection_id, NULL))
     remote_collection_id = NULL;
 
   if (remote_collection_id != NULL && collection_id != NULL &&
@@ -6750,16 +6753,14 @@ flatpak_pull_from_bundle (OstreeRepo   *repo,
       if (g_output_stream_splice (G_OUTPUT_STREAM (data_stream), in,
                                   G_OUTPUT_STREAM_SPLICE_CLOSE_SOURCE,
                                   cancellable, error) < 0)
         return FALSE;
 
-      /* Null terminate */
-      g_output_stream_write (G_OUTPUT_STREAM (data_stream), "\0", 1, NULL, NULL);
-
       metadata_valid =
         metadata_contents != NULL &&
-        strcmp (metadata_contents, g_memory_output_stream_get_data (data_stream)) == 0;
+        metadata_size == g_memory_output_stream_get_data_size (data_stream) &&
+        memcmp (metadata_contents, g_memory_output_stream_get_data (data_stream), metadata_size) == 0;
     }
   else
     {
       metadata_valid = (metadata_contents == NULL);
     }

Vulnerable code:     g_variant_lookup (commit_metadata, "xa.metadata", "&s", &xa_metadata);
      (xa_metadata != NULL && g_strcmp0 (required_metadata, xa_metadata) != 0))
                                 &metadata_contents, NULL, NULL, error))
      if (!g_key_file_load_from_data (keyfile, metadata_contents, -1, 0, error))
          !validate_commit_metadata (commit_data, ref, (const char *)g_bytes_get_data (require_metadata, NULL), TRUE, error))
        return FALSE;
                            &metadata_contents, NULL, NULL, NULL))
                                      -1,
                                 metadata_contents, !is_oci, error))
    op->external_metadata = g_bytes_new (external_metadata, strlen (external_metadata) + 1);
  return g_bytes_new_take (g_steal_pointer (&metadata_contents), metadata_contents_length + 1);
    metadata_bytes = g_bytes_new (xa_metadata, strlen (xa_metadata) + 1);
  metadata_bytes = g_bytes_new (metadata, strlen (metadata) + 1);
      /* Null terminate */
      g_output_stream_write (G_OUTPUT_STREAM (data_stream), "\0", 1, NULL, NULL);

        strcmp (metadata_contents, g_memory_output_stream_get_data (data_stream)) == 0;
Fixed code:                           gsize       required_metadata_size,
  g_autoptr(GVariant) xa_metadata_v = NULL;
  gsize xa_metadata_size = 0;
    {
      xa_metadata_v = g_variant_lookup_value (commit_metadata,
                                              "xa.metadata",
                                              G_VARIANT_TYPE_STRING);
      if (xa_metadata_v)
        xa_metadata = g_variant_get_string (xa_metadata_v, &xa_metadata_size);
    }
      (xa_metadata != NULL && (xa_metadata_size != required_metadata_size ||
                               memcmp (xa_metadata, required_metadata, xa_metadata_size) != 0)))
      gsize metadata_size = 0;
                                 &metadata_contents, &metadata_size, NULL, error))
      if (!g_key_file_load_from_data (keyfile, metadata_contents, metadata_size, 0, error))
          !validate_commit_metadata (commit_data,
                                     ref,
                                     (const char *)g_bytes_get_data (require_metadata, NULL),
                                     g_bytes_get_size (require_metadata),
                                     TRUE,
                                     error))
        goto out;
  gsize metadata_size = 0;
                            &metadata_contents,
                            &metadata_size, NULL, NULL))
                                      metadata_size,
                                 metadata_contents, metadata_size, !is_oci, error))
    op->external_metadata = g_bytes_new (external_metadata, strlen (external_metadata));
  return g_bytes_new_take (g_steal_pointer (&metadata_contents), metadata_contents_length);
    metadata_bytes = g_bytes_new (xa_metadata, strlen (xa_metadata));
  metadata_bytes = g_bytes_new (metadata, strlen (metadata));
  gsize metadata_size = 0;
  metadata_size = strlen (metadata_contents);

        metadata_size == g_memory_output_stream_get_data_size (data_stream) &&
        memcmp (metadata_contents, g_memory_output_stream_get_data (data_stream), metadata_size) == 0;