| From ed8f2235da7d2a408bfa18c1003f4a07f90b05e8 Mon Sep 17 00:00:00 2001 |
| From: Philip Withnall <pwithnall@endlessos.org> |
| Date: Wed, 24 Feb 2021 17:36:07 +0000 |
| Subject: [PATCH 4/5] 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. |
| |
| Signed-off-by: Philip Withnall <pwithnall@endlessos.org> |
| |
| Fixes: #2325 |
| |
| CVE: CVE-2021-28153 |
| |
| Upstream-Status: Backport [https://gitlab.gnome.org/GNOME/glib/-/issues/2325] |
| |
| Signed-off-by: Chen Qi <Qi.Chen@windriver.com> |
| --- |
| gio/glocalfileoutputstream.c | 77 ++++++++++++++++++------- |
| gio/tests/file.c | 108 +++++++++++++++++++++++++++++++++++ |
| 2 files changed, 163 insertions(+), 22 deletions(-) |
| |
| diff --git a/gio/glocalfileoutputstream.c b/gio/glocalfileoutputstream.c |
| index 392d0b0..a2c7e3c 100644 |
| --- a/gio/glocalfileoutputstream.c |
| +++ b/gio/glocalfileoutputstream.c |
| @@ -878,16 +878,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, |
| @@ -898,15 +904,30 @@ handle_overwrite_open (const char *filename, |
| return -1; |
| } |
| |
| - res = g_local_file_fstat (fd, |
| - G_LOCAL_FILE_STAT_FIELD_TYPE | |
| - G_LOCAL_FILE_STAT_FIELD_MODE | |
| - G_LOCAL_FILE_STAT_FIELD_UID | |
| - G_LOCAL_FILE_STAT_FIELD_GID | |
| - G_LOCAL_FILE_STAT_FIELD_MTIME | |
| - G_LOCAL_FILE_STAT_FIELD_NLINK, |
| - G_LOCAL_FILE_STAT_FIELD_ALL, &original_stat); |
| - errsv = errno; |
| + if (!is_symlink) |
| + { |
| + res = g_local_file_fstat (fd, |
| + G_LOCAL_FILE_STAT_FIELD_TYPE | |
| + G_LOCAL_FILE_STAT_FIELD_MODE | |
| + G_LOCAL_FILE_STAT_FIELD_UID | |
| + G_LOCAL_FILE_STAT_FIELD_GID | |
| + G_LOCAL_FILE_STAT_FIELD_MTIME | |
| + G_LOCAL_FILE_STAT_FIELD_NLINK, |
| + G_LOCAL_FILE_STAT_FIELD_ALL, &original_stat); |
| + errsv = errno; |
| + } |
| + else |
| + { |
| + res = g_local_file_lstat (filename, |
| + G_LOCAL_FILE_STAT_FIELD_TYPE | |
| + G_LOCAL_FILE_STAT_FIELD_MODE | |
| + G_LOCAL_FILE_STAT_FIELD_UID | |
| + G_LOCAL_FILE_STAT_FIELD_GID | |
| + G_LOCAL_FILE_STAT_FIELD_MTIME | |
| + G_LOCAL_FILE_STAT_FIELD_NLINK, |
| + G_LOCAL_FILE_STAT_FIELD_ALL, &original_stat); |
| + errsv = errno; |
| + } |
| |
| if (res != 0) |
| { |
| @@ -923,16 +944,27 @@ handle_overwrite_open (const char *filename, |
| if (!S_ISREG (_g_stat_mode (&original_stat))) |
| { |
| if (S_ISDIR (_g_stat_mode (&original_stat))) |
| - 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 (_g_stat_mode (&original_stat)) |
| +#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) |
| @@ -1015,7 +1047,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 39d51da..ddd1ffc 100644 |
| --- a/gio/tests/file.c |
| +++ b/gio/tests/file.c |
| @@ -805,6 +805,113 @@ 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"); |
| + g_test_summary ("Test that G_FILE_CREATE_REPLACE_DESTINATION doesn’t follow symlinks"); |
| + |
| + /* 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, |
| @@ -1798,6 +1905,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); |
| g_test_add_func ("/file/copy-preserve-mode", test_copy_preserve_mode); |
| g_test_add_func ("/file/measure", test_measure); |
| -- |
| 2.17.1 |
| |