127 lines
5.3 KiB
Diff
127 lines
5.3 KiB
Diff
From 3e7b4aca4b0afe9fb1b1160bd26f791d7a636980 Mon Sep 17 00:00:00 2001
|
|
From: Ondrej Holy <oholy@redhat.com>
|
|
Date: Mon, 1 Mar 2021 17:16:27 +0100
|
|
Subject: [PATCH] extractor: Do not allow symlink in parents
|
|
|
|
Currently, it is still possible that some files are extracted outside of
|
|
the destination dir in case of malicious archives. The checks from commit
|
|
adb067e6 can be still bypassed in certain cases. See GNOME/file-roller#108
|
|
for more details. After some investigation, I am convinced that it would be
|
|
best to simply disallow symlinks in parents. For example, `tar` fails to
|
|
extract such files with the `ENOTDIR` error. Let's do the same here.
|
|
|
|
Fixes: https://gitlab.gnome.org/GNOME/gnome-autoar/-/issues/12
|
|
---
|
|
gnome-autoar/autoar-extractor.c | 59 +++++++++++++++++++++++++--------
|
|
1 file changed, 46 insertions(+), 13 deletions(-)
|
|
|
|
diff --git a/gnome-autoar/autoar-extractor.c b/gnome-autoar/autoar-extractor.c
|
|
index ce6e6e9..79a7278 100644
|
|
--- a/gnome-autoar/autoar-extractor.c
|
|
+++ b/gnome-autoar/autoar-extractor.c
|
|
@@ -892,27 +892,42 @@ autoar_extractor_do_sanitize_pathname (AutoarExtractor *self,
|
|
return extracted_filename;
|
|
}
|
|
|
|
-static gboolean
|
|
-autoar_extractor_check_file_conflict (GFile *file,
|
|
+/* The function checks @file for conflicts with already existing files on the
|
|
+ * disk. It also recursively checks parents of @file to be sure it is directory.
|
|
+ * It doesn't follow symlinks, so symlinks in parents are also considered as
|
|
+ * conflicts even though they point to directory. It returns #GFile object for
|
|
+ * the file, which cause the conflict (so @file, or some of its parents). If
|
|
+ * there aren't any conflicts, NULL is returned.
|
|
+ */
|
|
+static GFile *
|
|
+autoar_extractor_check_file_conflict (AutoarExtractor *self,
|
|
+ GFile *file,
|
|
mode_t extracted_filetype)
|
|
{
|
|
GFileType file_type;
|
|
+ g_autoptr (GFile) parent = NULL;
|
|
|
|
file_type = g_file_query_file_type (file,
|
|
G_FILE_QUERY_INFO_NOFOLLOW_SYMLINKS,
|
|
NULL);
|
|
- /* If there is no file with the given name, there will be no conflict */
|
|
- if (file_type == G_FILE_TYPE_UNKNOWN) {
|
|
- return FALSE;
|
|
+
|
|
+ /* It is a conflict if the file already exists with an exception for already
|
|
+ * existing directories.
|
|
+ */
|
|
+ if (file_type != G_FILE_TYPE_UNKNOWN &&
|
|
+ (file_type != G_FILE_TYPE_DIRECTORY ||
|
|
+ extracted_filetype != AE_IFDIR)) {
|
|
+ return g_object_ref (file);
|
|
}
|
|
|
|
- /* It is not problem if the directory already exists */
|
|
- if (file_type == G_FILE_TYPE_DIRECTORY &&
|
|
- extracted_filetype == AE_IFDIR) {
|
|
- return FALSE;
|
|
+ if ((self->new_prefix && g_file_equal (self->new_prefix, file)) ||
|
|
+ (!self->new_prefix && g_file_equal (self->destination_dir, file))) {
|
|
+ return NULL;
|
|
}
|
|
|
|
- return TRUE;
|
|
+ /* Check also parents for conflict to be sure it is directory. */
|
|
+ parent = g_file_get_parent (file);
|
|
+ return autoar_extractor_check_file_conflict (self, parent, AE_IFDIR);
|
|
}
|
|
|
|
static void
|
|
@@ -1804,7 +1819,7 @@ autoar_extractor_step_extract (AutoarExtractor *self) {
|
|
g_autoptr (GFile) extracted_filename = NULL;
|
|
g_autoptr (GFile) hardlink_filename = NULL;
|
|
AutoarConflictAction action;
|
|
- gboolean file_conflict;
|
|
+ g_autoptr (GFile) file_conflict = NULL;
|
|
|
|
if (g_cancellable_is_cancelled (self->cancellable)) {
|
|
archive_read_free (a);
|
|
@@ -1823,11 +1838,27 @@ autoar_extractor_step_extract (AutoarExtractor *self) {
|
|
}
|
|
|
|
/* Attempt to solve any name conflict before doing any operations */
|
|
- file_conflict = autoar_extractor_check_file_conflict (extracted_filename,
|
|
+ file_conflict = autoar_extractor_check_file_conflict (self,
|
|
+ extracted_filename,
|
|
archive_entry_filetype (entry));
|
|
while (file_conflict) {
|
|
GFile *new_extracted_filename = NULL;
|
|
|
|
+ /* Do not try to solve any conflicts in parents for now. Especially
|
|
+ * symlinks in parents are dangerous as it can easily happen that files
|
|
+ * are written outside of the destination. The tar cmd fails to extract
|
|
+ * such archives with ENOTDIR. Let's do the same here. This is most
|
|
+ * probably malicious, or corrupted archive if the conflict was caused
|
|
+ * only by files from the archive...
|
|
+ */
|
|
+ if (!g_file_equal (file_conflict, extracted_filename)) {
|
|
+ self->error = g_error_new (G_IO_ERROR,
|
|
+ G_IO_ERROR_NOT_DIRECTORY,
|
|
+ "The file is not a directory");
|
|
+ archive_read_free (a);
|
|
+ return;
|
|
+ }
|
|
+
|
|
action = autoar_extractor_signal_conflict (self,
|
|
extracted_filename,
|
|
&new_extracted_filename);
|
|
@@ -1855,7 +1886,9 @@ autoar_extractor_step_extract (AutoarExtractor *self) {
|
|
break;
|
|
}
|
|
|
|
- file_conflict = autoar_extractor_check_file_conflict (extracted_filename,
|
|
+ g_clear_object (&file_conflict);
|
|
+ file_conflict = autoar_extractor_check_file_conflict (self,
|
|
+ extracted_filename,
|
|
archive_entry_filetype (entry));
|
|
}
|
|
|
|
--
|
|
2.31.1
|
|
|