File glib2-CVE-2021-28153.patch of Package glib2.23780
From 84d7b850d76a46ba4a0cfeea5b4ecaa3a42e9b9e Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 24 Feb 2021 17:33:38 +0000
Subject: [PATCH 08/13] glocalfileoutputstream: Fix a typo in a comment
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
(cherry picked from commit 78420a75aeb70569a8cd79fa0fea7b786b6f785f)
---
gio/glocalfileoutputstream.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gio/glocalfileoutputstream.c b/gio/glocalfileoutputstream.c
index 57d2d5dfe..8a68be3de 100644
--- a/gio/glocalfileoutputstream.c
+++ b/gio/glocalfileoutputstream.c
@@ -755,7 +755,7 @@ handle_overwrite_open (const char *filename,
mode = mode_from_flags_or_info (flags, reference_info);
/* We only need read access to the original file if we are creating a backup.
- * We also add O_CREATE to avoid a race if the file was just removed */
+ * We also add O_CREAT to avoid a race if the file was just removed */
if (create_backup || readable)
open_flags = O_RDWR | O_CREAT | O_BINARY;
else
--
2.35.1
From 42020509ac9f0a13ced21871c7f5762f65bc142d Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 24 Feb 2021 17:34:32 +0000
Subject: [PATCH 09/13] tests: Stop using g_test_bug_base() in file tests
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Since a following commit is going to add a new test which references
Gitlab, so it’s best to move the URI bases inside the test cases.
Backported to GLib 2.58 by Simon McVittie.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
(cherry-picked from commit 32d3d02a50e7dcec5f4cf7908e7ac88d575d8fc5)
[GLib 2.58.x did not allow g_test_bug() without g_test_bug_base(),
so use an empty string as the base]
Signed-off-by: Simon McVittie <smcv@collabora.com>
---
gio/tests/file.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gio/tests/file.c b/gio/tests/file.c
index d2f147419..9c2af641f 100644
--- a/gio/tests/file.c
+++ b/gio/tests/file.c
@@ -679,7 +679,7 @@ test_replace_cancel (void)
guint count;
GError *error = NULL;
- g_test_bug ("629301");
+ g_test_bug ("https://bugzilla.gnome.org/629301");
path = g_dir_make_tmp ("g_file_replace_cancel_XXXXXX", &error);
g_assert_no_error (error);
@@ -1167,7 +1167,7 @@ main (int argc, char *argv[])
{
g_test_init (&argc, &argv, NULL);
- g_test_bug_base ("http://bugzilla.gnome.org/");
+ g_test_bug_base ("");
g_test_add_func ("/file/basic", test_basic);
g_test_add_func ("/file/build-filename", test_build_filename);
--
2.35.1
From b38fca2149a086b64198dd5b8ffe12209c513ad0 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 10 Mar 2021 16:05:55 +0000
Subject: [PATCH 10/13] glocalfileoutputstream: Factor out a flag check
This clarifies the code a little. It introduces no functional changes.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
(cherry picked from commit ce0eb088a68171eed3ac217cb92a72e36eb57d1b)
---
gio/glocalfileoutputstream.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/gio/glocalfileoutputstream.c b/gio/glocalfileoutputstream.c
index 8a68be3de..8e0e793ff 100644
--- a/gio/glocalfileoutputstream.c
+++ b/gio/glocalfileoutputstream.c
@@ -751,6 +751,7 @@ handle_overwrite_open (const char *filename,
int res;
int mode;
int errsv;
+ gboolean replace_destination_set = (flags & G_FILE_CREATE_REPLACE_DESTINATION);
mode = mode_from_flags_or_info (flags, reference_info);
@@ -858,7 +859,7 @@ handle_overwrite_open (const char *filename,
* to a backup file and rewrite the contents of the file.
*/
- if ((flags & G_FILE_CREATE_REPLACE_DESTINATION) ||
+ if (replace_destination_set ||
(!(original_stat.st_nlink > 1) && !is_symlink))
{
char *dirname, *tmp_filename;
@@ -877,7 +878,7 @@ handle_overwrite_open (const char *filename,
/* try to keep permissions (unless replacing) */
- if ( ! (flags & G_FILE_CREATE_REPLACE_DESTINATION) &&
+ if (!replace_destination_set &&
(
#ifdef HAVE_FCHOWN
fchown (tmpfd, original_stat.st_uid, original_stat.st_gid) == -1 ||
@@ -1016,7 +1017,7 @@ handle_overwrite_open (const char *filename,
}
}
- if (flags & G_FILE_CREATE_REPLACE_DESTINATION)
+ if (replace_destination_set)
{
g_close (fd, NULL);
--
2.35.1
From 4e64a27f4f170860ddcf835ca6858bda09911a23 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 24 Feb 2021 17:36:07 +0000
Subject: [PATCH 11/13] glocalfileoutputstream: Fix CREATE_REPLACE_DESTINATION
with symlinks
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The `G_FILE_CREATE_REPLACE_DESTINATION` flag is equivalent to unlinking
the destination file and re-creating it from scratch. That did
previously work, but in the process the code would call `open(O_CREAT)`
on the file. If the file was a dangling symlink, this would create the
destination file (empty). That’s not an intended side-effect, and has
security implications if the symlink is controlled by a lower-privileged
process.
Fix that by not opening the destination file if it’s a symlink, and
adjusting the rest of the code to cope with
- the fact that `fd == -1` is not an error iff `is_symlink` is true,
- and that `original_stat` will contain the `lstat()` results for the
symlink now, rather than the `stat()` results for its target (again,
iff `is_symlink` is true).
This means that the target of the dangling symlink is no longer created,
which was the bug. The symlink itself continues to be replaced (as
before) with the new file — this is the intended behaviour of
`g_file_replace()`.
The behaviour for non-symlink cases, or cases where the symlink was not
dangling, should be unchanged.
Includes a unit test.
Resolves CVE-2021-28153 (glib#2325). Backported to GLib 2.58 by
Simon McVittie.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
(cherry-picked from commit 317b3b587058a05dca95d56dac26568c5b098d33)
[Backport to 2.58.x: replace g_local_file_fstat with fstat]
[Backport to 2.58.x: replace g_local_file_lstat with lstat]
[Backport to 2.58.x: replace _g_stat_mode with direct access to st_mode]
[Backport to 2.58.x: don't call g_test_summary()]
Signed-off-by: Simon McVittie <smcv@collabora.com>
---
gio/glocalfileoutputstream.c | 49 +++++++++++-----
gio/tests/file.c | 107 +++++++++++++++++++++++++++++++++++
2 files changed, 142 insertions(+), 14 deletions(-)
diff --git a/gio/glocalfileoutputstream.c b/gio/glocalfileoutputstream.c
index 8e0e793ff..2be51ae12 100644
--- a/gio/glocalfileoutputstream.c
+++ b/gio/glocalfileoutputstream.c
@@ -779,16 +779,22 @@ handle_overwrite_open (const char *filename,
/* Could be a symlink, or it could be a regular ELOOP error,
* but then the next open will fail too. */
is_symlink = TRUE;
- fd = g_open (filename, open_flags, mode);
+ if (!replace_destination_set)
+ fd = g_open (filename, open_flags, mode);
}
-#else
- fd = g_open (filename, open_flags, mode);
- errsv = errno;
+#else /* if !O_NOFOLLOW */
/* This is racy, but we do it as soon as possible to minimize the race */
is_symlink = g_file_test (filename, G_FILE_TEST_IS_SYMLINK);
+
+ if (!is_symlink || !replace_destination_set)
+ {
+ fd = g_open (filename, open_flags, mode);
+ errsv = errno;
+ }
#endif
- if (fd == -1)
+ if (fd == -1 &&
+ (!is_symlink || !replace_destination_set))
{
char *display_name = g_filename_display_name (filename);
g_set_error (error, G_IO_ERROR,
@@ -802,7 +808,10 @@ handle_overwrite_open (const char *filename,
#ifdef G_OS_WIN32
res = GLIB_PRIVATE_CALL (g_win32_fstat) (fd, &original_stat);
#else
- res = fstat (fd, &original_stat);
+ if (!is_symlink)
+ res = fstat (fd, &original_stat);
+ else
+ res = lstat (filename, &original_stat);
#endif
errsv = errno;
@@ -821,16 +830,27 @@ handle_overwrite_open (const char *filename,
if (!S_ISREG (original_stat.st_mode))
{
if (S_ISDIR (original_stat.st_mode))
- g_set_error_literal (error,
- G_IO_ERROR,
- G_IO_ERROR_IS_DIRECTORY,
- _("Target file is a directory"));
- else
- g_set_error_literal (error,
+ {
+ g_set_error_literal (error,
+ G_IO_ERROR,
+ G_IO_ERROR_IS_DIRECTORY,
+ _("Target file is a directory"));
+ goto err_out;
+ }
+ else if (!is_symlink ||
+#ifdef S_ISLNK
+ !S_ISLNK (original_stat.st_mode)
+#else
+ FALSE
+#endif
+ )
+ {
+ g_set_error_literal (error,
G_IO_ERROR,
G_IO_ERROR_NOT_REGULAR_FILE,
_("Target file is not a regular file"));
- goto err_out;
+ goto err_out;
+ }
}
if (etag != NULL)
@@ -911,7 +931,8 @@ handle_overwrite_open (const char *filename,
}
}
- g_close (fd, NULL);
+ if (fd >= 0)
+ g_close (fd, NULL);
*temp_filename = tmp_filename;
return tmpfd;
}
diff --git a/gio/tests/file.c b/gio/tests/file.c
index 9c2af641f..1bdbe19b4 100644
--- a/gio/tests/file.c
+++ b/gio/tests/file.c
@@ -787,6 +787,112 @@ test_replace_cancel (void)
g_object_unref (tmpdir);
}
+static void
+test_replace_symlink (void)
+{
+#ifdef G_OS_UNIX
+ gchar *tmpdir_path = NULL;
+ GFile *tmpdir = NULL, *source_file = NULL, *target_file = NULL;
+ GFileOutputStream *stream = NULL;
+ const gchar *new_contents = "this is a test message which should be written to source and not target";
+ gsize n_written;
+ GFileEnumerator *enumerator = NULL;
+ GFileInfo *info = NULL;
+ gchar *contents = NULL;
+ gsize length = 0;
+ GError *local_error = NULL;
+
+ g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2325");
+
+ /* Create a fresh, empty working directory. */
+ tmpdir_path = g_dir_make_tmp ("g_file_replace_symlink_XXXXXX", &local_error);
+ g_assert_no_error (local_error);
+ tmpdir = g_file_new_for_path (tmpdir_path);
+
+ g_test_message ("Using temporary directory %s", tmpdir_path);
+ g_free (tmpdir_path);
+
+ /* Create symlink `source` which points to `target`. */
+ source_file = g_file_get_child (tmpdir, "source");
+ target_file = g_file_get_child (tmpdir, "target");
+ g_file_make_symbolic_link (source_file, "target", NULL, &local_error);
+ g_assert_no_error (local_error);
+
+ /* Ensure that `target` doesn’t exist */
+ g_assert_false (g_file_query_exists (target_file, NULL));
+
+ /* Replace the `source` symlink with a regular file using
+ * %G_FILE_CREATE_REPLACE_DESTINATION, which should replace it *without*
+ * following the symlink */
+ stream = g_file_replace (source_file, NULL, FALSE /* no backup */,
+ G_FILE_CREATE_REPLACE_DESTINATION, NULL, &local_error);
+ g_assert_no_error (local_error);
+
+ g_output_stream_write_all (G_OUTPUT_STREAM (stream), new_contents, strlen (new_contents),
+ &n_written, NULL, &local_error);
+ g_assert_no_error (local_error);
+ g_assert_cmpint (n_written, ==, strlen (new_contents));
+
+ g_output_stream_close (G_OUTPUT_STREAM (stream), NULL, &local_error);
+ g_assert_no_error (local_error);
+
+ g_clear_object (&stream);
+
+ /* At this point, there should still only be one file: `source`. It should
+ * now be a regular file. `target` should not exist. */
+ enumerator = g_file_enumerate_children (tmpdir,
+ G_FILE_ATTRIBUTE_STANDARD_NAME ","
+ G_FILE_ATTRIBUTE_STANDARD_TYPE,
+ G_FILE_QUERY_INFO_NOFOLLOW_SYMLINKS, NULL, &local_error);
+ g_assert_no_error (local_error);
+
+ info = g_file_enumerator_next_file (enumerator, NULL, &local_error);
+ g_assert_no_error (local_error);
+ g_assert_nonnull (info);
+
+ g_assert_cmpstr (g_file_info_get_name (info), ==, "source");
+ g_assert_cmpint (g_file_info_get_file_type (info), ==, G_FILE_TYPE_REGULAR);
+
+ g_clear_object (&info);
+
+ info = g_file_enumerator_next_file (enumerator, NULL, &local_error);
+ g_assert_no_error (local_error);
+ g_assert_null (info);
+
+ g_file_enumerator_close (enumerator, NULL, &local_error);
+ g_assert_no_error (local_error);
+ g_clear_object (&enumerator);
+
+ /* Double-check that `target` doesn’t exist */
+ g_assert_false (g_file_query_exists (target_file, NULL));
+
+ /* Check the content of `source`. */
+ g_file_load_contents (source_file,
+ NULL,
+ &contents,
+ &length,
+ NULL,
+ &local_error);
+ g_assert_no_error (local_error);
+ g_assert_cmpstr (contents, ==, new_contents);
+ g_assert_cmpuint (length, ==, strlen (new_contents));
+ g_free (contents);
+
+ /* Tidy up. */
+ g_file_delete (source_file, NULL, &local_error);
+ g_assert_no_error (local_error);
+
+ g_file_delete (tmpdir, NULL, &local_error);
+ g_assert_no_error (local_error);
+
+ g_clear_object (&target_file);
+ g_clear_object (&source_file);
+ g_clear_object (&tmpdir);
+#else /* if !G_OS_UNIX */
+ g_test_skip ("Symlink replacement tests can only be run on Unix")
+#endif
+}
+
static void
on_file_deleted (GObject *object,
GAsyncResult *result,
@@ -1182,6 +1288,7 @@ main (int argc, char *argv[])
g_test_add_data_func ("/file/async-create-delete/4096", GINT_TO_POINTER (4096), test_create_delete);
g_test_add_func ("/file/replace-load", test_replace_load);
g_test_add_func ("/file/replace-cancel", test_replace_cancel);
+ g_test_add_func ("/file/replace-symlink", test_replace_symlink);
g_test_add_func ("/file/async-delete", test_async_delete);
#ifdef G_OS_UNIX
g_test_add_func ("/file/copy-preserve-mode", test_copy_preserve_mode);
--
2.35.1
From 7bfe2dcebe59517e5d31c63b881c4227029a2fc4 Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 24 Feb 2021 17:42:24 +0000
Subject: [PATCH 12/13] glocalfileoutputstream: Add a missing O_CLOEXEC flag to
replace()
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
(cherry picked from commit 6c6439261bc7a8a0627519848a7222b3e1bd4ffe)
---
gio/glocalfileoutputstream.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/gio/glocalfileoutputstream.c b/gio/glocalfileoutputstream.c
index 2be51ae12..fff2439b9 100644
--- a/gio/glocalfileoutputstream.c
+++ b/gio/glocalfileoutputstream.c
@@ -56,6 +56,12 @@
#define O_BINARY 0
#endif
+#ifndef O_CLOEXEC
+#define O_CLOEXEC 0
+#else
+#define HAVE_O_CLOEXEC 1
+#endif
+
struct _GLocalFileOutputStreamPrivate {
char *tmp_filename;
char *original_filename;
@@ -1123,7 +1129,7 @@ _g_local_file_output_stream_replace (const char *filename,
sync_on_close = FALSE;
/* If the file doesn't exist, create it */
- open_flags = O_CREAT | O_EXCL | O_BINARY;
+ open_flags = O_CREAT | O_EXCL | O_BINARY | O_CLOEXEC;
if (readable)
open_flags |= O_RDWR;
else
@@ -1153,8 +1159,11 @@ _g_local_file_output_stream_replace (const char *filename,
set_error_from_open_errno (filename, error);
return NULL;
}
-
-
+#if !defined(HAVE_O_CLOEXEC) && defined(F_SETFD)
+ else
+ fcntl (fd, F_SETFD, FD_CLOEXEC);
+#endif
+
stream = g_object_new (G_TYPE_LOCAL_FILE_OUTPUT_STREAM, NULL);
stream->priv->fd = fd;
stream->priv->sync_on_close = sync_on_close;
--
2.35.1
From af39d83fc71408bca50f2cb3739099f36139281b Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Tue, 16 Mar 2021 11:36:27 +0000
Subject: [PATCH 13/13] glocalfileoutputstream: Tidy up error handling
After the recent reworking of this code it was possible for `g_close()`
to be called on `fd == -1`, which is invalid. It would have reported an
error, were errors not ignored. So it was harmless, but still best to
fix.
Simplify the error handling by combining both error labels and checking
the state of `fd` dynamically.
Coverity CID: #1450834
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
(cherry picked from commit c4b4fecaef5fa6eac63569513511ba6f8674548a)
---
gio/glocalfileoutputstream.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/gio/glocalfileoutputstream.c b/gio/glocalfileoutputstream.c
index fff2439b9..8d7eadd95 100644
--- a/gio/glocalfileoutputstream.c
+++ b/gio/glocalfileoutputstream.c
@@ -829,7 +829,7 @@ handle_overwrite_open (const char *filename,
_("Error when getting information for file “%s”: %s"),
display_name, g_strerror (errsv));
g_free (display_name);
- goto err_out;
+ goto error;
}
/* not a regular file */
@@ -841,7 +841,7 @@ handle_overwrite_open (const char *filename,
G_IO_ERROR,
G_IO_ERROR_IS_DIRECTORY,
_("Target file is a directory"));
- goto err_out;
+ goto error;
}
else if (!is_symlink ||
#ifdef S_ISLNK
@@ -855,7 +855,7 @@ handle_overwrite_open (const char *filename,
G_IO_ERROR,
G_IO_ERROR_NOT_REGULAR_FILE,
_("Target file is not a regular file"));
- goto err_out;
+ goto error;
}
}
@@ -869,7 +869,7 @@ handle_overwrite_open (const char *filename,
G_IO_ERROR_WRONG_ETAG,
_("The file was externally modified"));
g_free (current_etag);
- goto err_out;
+ goto error;
}
g_free (current_etag);
}
@@ -962,7 +962,7 @@ handle_overwrite_open (const char *filename,
G_IO_ERROR_CANT_CREATE_BACKUP,
_("Backup file creation failed"));
g_free (backup_filename);
- goto err_out;
+ goto error;
}
bfd = g_open (backup_filename,
@@ -976,7 +976,7 @@ handle_overwrite_open (const char *filename,
G_IO_ERROR_CANT_CREATE_BACKUP,
_("Backup file creation failed"));
g_free (backup_filename);
- goto err_out;
+ goto error;
}
/* If needed, Try to set the group of the backup same as the
@@ -993,7 +993,7 @@ handle_overwrite_open (const char *filename,
g_unlink (backup_filename);
g_close (bfd, NULL);
g_free (backup_filename);
- goto err_out;
+ goto error;
}
if ((original_stat.st_gid != tmp_statbuf.st_gid) &&
@@ -1010,7 +1010,7 @@ handle_overwrite_open (const char *filename,
g_unlink (backup_filename);
g_close (bfd, NULL);
g_free (backup_filename);
- goto err_out;
+ goto error;
}
}
#endif
@@ -1025,7 +1025,7 @@ handle_overwrite_open (const char *filename,
g_close (bfd, NULL);
g_free (backup_filename);
- goto err_out;
+ goto error;
}
g_close (bfd, NULL);
@@ -1040,7 +1040,7 @@ handle_overwrite_open (const char *filename,
g_io_error_from_errno (errsv),
_("Error seeking in file: %s"),
g_strerror (errsv));
- goto err_out;
+ goto error;
}
}
@@ -1056,7 +1056,7 @@ handle_overwrite_open (const char *filename,
g_io_error_from_errno (errsv),
_("Error removing old file: %s"),
g_strerror (errsv));
- goto err_out2;
+ goto error;
}
if (readable)
@@ -1073,7 +1073,7 @@ handle_overwrite_open (const char *filename,
_("Error opening file “%s”: %s"),
display_name, g_strerror (errsv));
g_free (display_name);
- goto err_out2;
+ goto error;
}
}
else
@@ -1091,15 +1091,16 @@ handle_overwrite_open (const char *filename,
g_io_error_from_errno (errsv),
_("Error truncating file: %s"),
g_strerror (errsv));
- goto err_out;
+ goto error;
}
}
return fd;
- err_out:
- g_close (fd, NULL);
- err_out2:
+error:
+ if (fd >= 0)
+ g_close (fd, NULL);
+
return -1;
}
--
2.35.1
