From ba818f504c926baaf6e362be8159cfacf994310e Mon Sep 17 00:00:00 2001 From: Ryan Gonzalez Date: Thu, 23 Dec 2021 18:30:17 -0600 Subject: [PATCH] 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 Conflict:NA Reference:https://github.com/flatpak/flatpak/commit/ba818f504c926baaf6e362be8159cfacf994310e --- common/flatpak-dir.c | 36 +++++++++++++++++++++++++++--------- common/flatpak-transaction.c | 8 ++++---- common/flatpak-utils.c | 9 +++++---- 3 files changed, 36 insertions(+), 17 deletions(-) diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c index ddc5ee9..e6c8046 100644 --- a/common/flatpak-dir.c +++ b/common/flatpak-dir.c @@ -1762,19 +1762,29 @@ 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); @@ -3478,6 +3488,7 @@ upgrade_deploy_data (GBytes *deploy_data, 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 */ @@ -3491,9 +3502,9 @@ upgrade_deploy_data (GBytes *deploy_data, 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); @@ -5799,8 +5810,13 @@ flatpak_dir_pull (FlatpakDir *self, { 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, @@ -8111,6 +8127,7 @@ flatpak_dir_deploy (FlatpakDir *self, g_auto(GLnxLockFile) lock = { 0, }; g_autoptr(GFile) metadata_file = NULL; g_autofree char *metadata_contents = NULL; + gsize metadata_size = 0; gboolean is_oci; if (!flatpak_dir_ensure_repo (self, cancellable, error)) @@ -8320,11 +8337,12 @@ 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; @@ -8340,7 +8358,7 @@ flatpak_dir_deploy (FlatpakDir *self, */ 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"); diff --git a/common/flatpak-transaction.c b/common/flatpak-transaction.c index 1927498..721da14 100644 --- a/common/flatpak-transaction.c +++ b/common/flatpak-transaction.c @@ -2520,7 +2520,7 @@ flatpak_transaction_add_ref (FlatpakTransaction *self, 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; } @@ -2937,7 +2937,7 @@ load_deployed_metadata (FlatpakTransaction *self, FlatpakDecomposed *ref, char * 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 @@ -3034,7 +3034,7 @@ resolve_op_from_commit (FlatpakTransaction *self, 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); @@ -3074,7 +3074,7 @@ try_resolve_op_from_metadata (FlatpakTransaction *self, &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)) diff --git a/common/flatpak-utils.c b/common/flatpak-utils.c index 6901a62..9eedbfa 100644 --- a/common/flatpak-utils.c +++ b/common/flatpak-utils.c @@ -6604,6 +6604,7 @@ flatpak_pull_from_bundle (OstreeRepo *repo, 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; @@ -6620,6 +6621,8 @@ flatpak_pull_from_bundle (OstreeRepo *repo, 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; @@ -6689,12 +6692,10 @@ flatpak_pull_from_bundle (OstreeRepo *repo, 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 { -- 2.27.0