Skip to content

Commit ba818f5

Browse files
refi64alexlarsson
authored andcommitted
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>
1 parent 2380309 commit ba818f5

File tree

3 files changed

+36
-17
lines changed

3 files changed

+36
-17
lines changed

Diff for: common/flatpak-dir.c

+27-9
Original file line numberDiff line numberDiff line change
@@ -1794,19 +1794,29 @@ static gboolean
17941794
validate_commit_metadata (GVariant *commit_data,
17951795
const char *ref,
17961796
const char *required_metadata,
1797+
gsize required_metadata_size,
17971798
gboolean require_xa_metadata,
17981799
GError **error)
17991800
{
18001801
g_autoptr(GVariant) commit_metadata = NULL;
1802+
g_autoptr(GVariant) xa_metadata_v = NULL;
18011803
const char *xa_metadata = NULL;
1804+
gsize xa_metadata_size = 0;
18021805

18031806
commit_metadata = g_variant_get_child_value (commit_data, 0);
18041807

18051808
if (commit_metadata != NULL)
1806-
g_variant_lookup (commit_metadata, "xa.metadata", "&s", &xa_metadata);
1809+
{
1810+
xa_metadata_v = g_variant_lookup_value (commit_metadata,
1811+
"xa.metadata",
1812+
G_VARIANT_TYPE_STRING);
1813+
if (xa_metadata_v)
1814+
xa_metadata = g_variant_get_string (xa_metadata_v, &xa_metadata_size);
1815+
}
18071816

18081817
if ((xa_metadata == NULL && require_xa_metadata) ||
1809-
(xa_metadata != NULL && g_strcmp0 (required_metadata, xa_metadata) != 0))
1818+
(xa_metadata != NULL && (xa_metadata_size != required_metadata_size ||
1819+
memcmp (xa_metadata, required_metadata, xa_metadata_size) != 0)))
18101820
{
18111821
g_set_error (error, G_IO_ERROR, G_IO_ERROR_PERMISSION_DENIED,
18121822
_("Commit metadata for %s not matching expected metadata"), ref);
@@ -3515,6 +3525,7 @@ upgrade_deploy_data (GBytes *deploy_data,
35153525
g_autoptr(GKeyFile) keyfile = NULL;
35163526
g_autoptr(GFile) metadata_file = NULL;
35173527
g_autofree char *metadata_contents = NULL;
3528+
gsize metadata_size = 0;
35183529
g_autofree char *id = flatpak_decomposed_dup_id (ref);
35193530

35203531
/* Add fields from commit metadata to deploy */
@@ -3528,9 +3539,9 @@ upgrade_deploy_data (GBytes *deploy_data,
35283539
keyfile = g_key_file_new ();
35293540
metadata_file = g_file_resolve_relative_path (deploy_dir, "metadata");
35303541
if (!g_file_load_contents (metadata_file, cancellable,
3531-
&metadata_contents, NULL, NULL, error))
3542+
&metadata_contents, &metadata_size, NULL, error))
35323543
return NULL;
3533-
if (!g_key_file_load_from_data (keyfile, metadata_contents, -1, 0, error))
3544+
if (!g_key_file_load_from_data (keyfile, metadata_contents, metadata_size, 0, error))
35343545
return NULL;
35353546
add_metadata_to_deploy_data (&metadata_dict, keyfile);
35363547

@@ -5833,8 +5844,13 @@ flatpak_dir_pull (FlatpakDir *self,
58335844
{
58345845
g_autoptr(GVariant) commit_data = NULL;
58355846
if (!ostree_repo_load_commit (repo, rev, &commit_data, NULL, error) ||
5836-
!validate_commit_metadata (commit_data, ref, (const char *)g_bytes_get_data (require_metadata, NULL), TRUE, error))
5837-
return FALSE;
5847+
!validate_commit_metadata (commit_data,
5848+
ref,
5849+
(const char *)g_bytes_get_data (require_metadata, NULL),
5850+
g_bytes_get_size (require_metadata),
5851+
TRUE,
5852+
error))
5853+
goto out;
58385854
}
58395855

58405856
if (!flatpak_dir_pull_extra_data (self, repo,
@@ -8156,6 +8172,7 @@ flatpak_dir_deploy (FlatpakDir *self,
81568172
g_auto(GLnxLockFile) lock = { 0, };
81578173
g_autoptr(GFile) metadata_file = NULL;
81588174
g_autofree char *metadata_contents = NULL;
8175+
gsize metadata_size = 0;
81598176
gboolean is_oci;
81608177
const char *flatpak;
81618178

@@ -8366,11 +8383,12 @@ flatpak_dir_deploy (FlatpakDir *self,
83668383
keyfile = g_key_file_new ();
83678384
metadata_file = g_file_resolve_relative_path (checkoutdir, "metadata");
83688385
if (g_file_load_contents (metadata_file, NULL,
8369-
&metadata_contents, NULL, NULL, NULL))
8386+
&metadata_contents,
8387+
&metadata_size, NULL, NULL))
83708388
{
83718389
if (!g_key_file_load_from_data (keyfile,
83728390
metadata_contents,
8373-
-1,
8391+
metadata_size,
83748392
0, error))
83758393
return FALSE;
83768394

@@ -8386,7 +8404,7 @@ flatpak_dir_deploy (FlatpakDir *self,
83868404
*/
83878405
is_oci = flatpak_dir_get_remote_oci (self, origin);
83888406
if (!validate_commit_metadata (commit_data, flatpak_decomposed_get_ref (ref),
8389-
metadata_contents, !is_oci, error))
8407+
metadata_contents, metadata_size, !is_oci, error))
83908408
return FALSE;
83918409

83928410
dotref = g_file_resolve_relative_path (checkoutdir, "files/.ref");

Diff for: common/flatpak-transaction.c

+4-4
Original file line numberDiff line numberDiff line change
@@ -2539,7 +2539,7 @@ flatpak_transaction_add_ref (FlatpakTransaction *self,
25392539
return FALSE;
25402540

25412541
if (external_metadata)
2542-
op->external_metadata = g_bytes_new (external_metadata, strlen (external_metadata) + 1);
2542+
op->external_metadata = g_bytes_new (external_metadata, strlen (external_metadata));
25432543

25442544
return TRUE;
25452545
}
@@ -2950,7 +2950,7 @@ load_deployed_metadata (FlatpakTransaction *self, FlatpakDecomposed *ref, char *
29502950
return NULL;
29512951
}
29522952

2953-
return g_bytes_new_take (g_steal_pointer (&metadata_contents), metadata_contents_length + 1);
2953+
return g_bytes_new_take (g_steal_pointer (&metadata_contents), metadata_contents_length);
29542954
}
29552955

29562956
static void
@@ -3051,7 +3051,7 @@ resolve_op_from_commit (FlatpakTransaction *self,
30513051
if (xa_metadata == NULL)
30523052
g_message ("Warning: No xa.metadata in local commit %s ref %s", checksum, flatpak_decomposed_get_ref (op->ref));
30533053
else
3054-
metadata_bytes = g_bytes_new (xa_metadata, strlen (xa_metadata) + 1);
3054+
metadata_bytes = g_bytes_new (xa_metadata, strlen (xa_metadata));
30553055

30563056
if (g_variant_lookup (commit_metadata, "xa.download-size", "t", &download_size))
30573057
op->download_size = GUINT64_FROM_BE (download_size);
@@ -3091,7 +3091,7 @@ try_resolve_op_from_metadata (FlatpakTransaction *self,
30913091
&download_size, &installed_size, &metadata, NULL))
30923092
return FALSE;
30933093

3094-
metadata_bytes = g_bytes_new (metadata, strlen (metadata) + 1);
3094+
metadata_bytes = g_bytes_new (metadata, strlen (metadata));
30953095

30963096
if (flatpak_remote_state_lookup_ref (state, flatpak_decomposed_get_ref (op->ref),
30973097
NULL, NULL, &info, NULL, NULL))

Diff for: common/flatpak-utils.c

+5-4
Original file line numberDiff line numberDiff line change
@@ -6667,6 +6667,7 @@ flatpak_pull_from_bundle (OstreeRepo *repo,
66676667
GCancellable *cancellable,
66686668
GError **error)
66696669
{
6670+
gsize metadata_size = 0;
66706671
g_autofree char *metadata_contents = NULL;
66716672
g_autofree char *to_checksum = NULL;
66726673
g_autoptr(GFile) root = NULL;
@@ -6683,6 +6684,8 @@ flatpak_pull_from_bundle (OstreeRepo *repo,
66836684
if (metadata == NULL)
66846685
return FALSE;
66856686

6687+
metadata_size = strlen (metadata_contents);
6688+
66866689
if (!ostree_repo_get_remote_option (repo, remote, "collection-id", NULL,
66876690
&remote_collection_id, NULL))
66886691
remote_collection_id = NULL;
@@ -6752,12 +6755,10 @@ flatpak_pull_from_bundle (OstreeRepo *repo,
67526755
cancellable, error) < 0)
67536756
return FALSE;
67546757

6755-
/* Null terminate */
6756-
g_output_stream_write (G_OUTPUT_STREAM (data_stream), "\0", 1, NULL, NULL);
6757-
67586758
metadata_valid =
67596759
metadata_contents != NULL &&
6760-
strcmp (metadata_contents, g_memory_output_stream_get_data (data_stream)) == 0;
6760+
metadata_size == g_memory_output_stream_get_data_size (data_stream) &&
6761+
memcmp (metadata_contents, g_memory_output_stream_get_data (data_stream), metadata_size) == 0;
67616762
}
67626763
else
67636764
{

0 commit comments

Comments
 (0)