Commit | Line | Data |
---|---|---|
5a06b83f MW |
1 | Backport of: |
2 | ||
3 | From 317b3b587058a05dca95d56dac26568c5b098d33 Mon Sep 17 00:00:00 2001 | |
4 | From: Philip Withnall <pwithnall@endlessos.org> | |
5 | Date: Wed, 24 Feb 2021 17:35:40 +0000 | |
6 | Subject: [PATCH] glocalfileoutputstream: Fix CREATE_REPLACE_DESTINATION | |
7 | with symlinks | |
8 | MIME-Version: 1.0 | |
9 | Content-Type: text/plain; charset=UTF-8 | |
10 | Content-Transfer-Encoding: 8bit | |
11 | ||
12 | The `G_FILE_CREATE_REPLACE_DESTINATION` flag is equivalent to unlinking | |
13 | the destination file and re-creating it from scratch. That did | |
14 | previously work, but in the process the code would call `open(O_CREAT)` | |
15 | on the file. If the file was a dangling symlink, this would create the | |
16 | destination file (empty). That’s not an intended side-effect, and has | |
17 | security implications if the symlink is controlled by a lower-privileged | |
18 | process. | |
19 | ||
20 | Fix that by not opening the destination file if it’s a symlink, and | |
21 | adjusting the rest of the code to cope with | |
22 | - the fact that `fd == -1` is not an error iff `is_symlink` is true, | |
23 | - and that `original_stat` will contain the `lstat()` results for the | |
24 | symlink now, rather than the `stat()` results for its target (again, | |
25 | iff `is_symlink` is true). | |
26 | ||
27 | This means that the target of the dangling symlink is no longer created, | |
28 | which was the bug. The symlink itself continues to be replaced (as | |
29 | before) with the new file — this is the intended behaviour of | |
30 | `g_file_replace()`. | |
31 | ||
32 | The behaviour for non-symlink cases, or cases where the symlink was not | |
33 | dangling, should be unchanged. | |
34 | ||
35 | Includes a unit test. | |
36 | ||
37 | Signed-off-by: Philip Withnall <pwithnall@endlessos.org> | |
38 | ||
39 | Fixes: #2325 | |
40 | --- | |
41 | gio/glocalfileoutputstream.c | 70 ++++++++++++++++------- | |
42 | gio/tests/file.c | 108 +++++++++++++++++++++++++++++++++++ | |
43 | 2 files changed, 158 insertions(+), 20 deletions(-) | |
44 | ||
45 | diff --git a/gio/glocalfileoutputstream.c b/gio/glocalfileoutputstream.c | |
46 | index a3dd62172..553fcbbae 100644 | |
47 | --- a/gio/glocalfileoutputstream.c | |
48 | +++ b/gio/glocalfileoutputstream.c | |
49 | @@ -874,16 +874,22 @@ handle_overwrite_open (const char *filename, | |
50 | /* Could be a symlink, or it could be a regular ELOOP error, | |
51 | * but then the next open will fail too. */ | |
52 | is_symlink = TRUE; | |
53 | - fd = g_open (filename, open_flags, mode); | |
54 | + if (!(flags & G_FILE_CREATE_REPLACE_DESTINATION)) | |
55 | + fd = g_open (filename, open_flags, mode); | |
56 | } | |
57 | -#else | |
58 | - fd = g_open (filename, open_flags, mode); | |
59 | - errsv = errno; | |
60 | +#else /* if !O_NOFOLLOW */ | |
61 | /* This is racy, but we do it as soon as possible to minimize the race */ | |
62 | is_symlink = g_file_test (filename, G_FILE_TEST_IS_SYMLINK); | |
63 | + | |
64 | + if (!is_symlink || !(flags & G_FILE_CREATE_REPLACE_DESTINATION)) | |
65 | + { | |
66 | + fd = g_open (filename, open_flags, mode); | |
67 | + errsv = errno; | |
68 | + } | |
69 | #endif | |
70 | ||
71 | - if (fd == -1) | |
72 | + if (fd == -1 && | |
73 | + (!is_symlink || !(flags & G_FILE_CREATE_REPLACE_DESTINATION))) | |
74 | { | |
75 | char *display_name = g_filename_display_name (filename); | |
76 | g_set_error (error, G_IO_ERROR, | |
77 | @@ -893,13 +899,25 @@ handle_overwrite_open (const char *filename, | |
78 | g_free (display_name); | |
79 | return -1; | |
80 | } | |
81 | - | |
82 | + | |
83 | + if (!is_symlink) | |
84 | + { | |
85 | #ifdef G_OS_WIN32 | |
86 | - res = GLIB_PRIVATE_CALL (g_win32_fstat) (fd, &original_stat); | |
87 | + res = GLIB_PRIVATE_CALL (g_win32_fstat) (fd, &original_stat); | |
88 | #else | |
89 | - res = fstat (fd, &original_stat); | |
90 | + res = fstat (fd, &original_stat); | |
91 | #endif | |
92 | - errsv = errno; | |
93 | + errsv = errno; | |
94 | + } | |
95 | + else | |
96 | + { | |
97 | +#ifdef G_OS_WIN32 | |
98 | + res = GLIB_PRIVATE_CALL (g_win32_lstat_utf8) (filename, &original_stat); | |
99 | +#else | |
100 | + res = g_lstat (filename, &original_stat); | |
101 | +#endif | |
102 | + errsv = errno; | |
103 | + } | |
104 | ||
105 | if (res != 0) | |
106 | { | |
107 | @@ -916,16 +934,27 @@ handle_overwrite_open (const char *filename, | |
108 | if (!S_ISREG (original_stat.st_mode)) | |
109 | { | |
110 | if (S_ISDIR (original_stat.st_mode)) | |
111 | - g_set_error_literal (error, | |
112 | - G_IO_ERROR, | |
113 | - G_IO_ERROR_IS_DIRECTORY, | |
114 | - _("Target file is a directory")); | |
115 | - else | |
116 | - g_set_error_literal (error, | |
117 | - G_IO_ERROR, | |
118 | - G_IO_ERROR_NOT_REGULAR_FILE, | |
119 | - _("Target file is not a regular file")); | |
120 | - goto err_out; | |
121 | + { | |
122 | + g_set_error_literal (error, | |
123 | + G_IO_ERROR, | |
124 | + G_IO_ERROR_IS_DIRECTORY, | |
125 | + _("Target file is a directory")); | |
126 | + goto err_out; | |
127 | + } | |
128 | + else if (!is_symlink || | |
129 | +#ifdef S_ISLNK | |
130 | + !S_ISLNK (original_stat.st_mode) | |
131 | +#else | |
132 | + FALSE | |
133 | +#endif | |
134 | + ) | |
135 | + { | |
136 | + g_set_error_literal (error, | |
137 | + G_IO_ERROR, | |
138 | + G_IO_ERROR_NOT_REGULAR_FILE, | |
139 | + _("Target file is not a regular file")); | |
140 | + goto err_out; | |
141 | + } | |
142 | } | |
143 | ||
144 | if (etag != NULL) | |
145 | @@ -1006,7 +1035,8 @@ handle_overwrite_open (const char *filename, | |
146 | } | |
147 | } | |
148 | ||
149 | - g_close (fd, NULL); | |
150 | + if (fd >= 0) | |
151 | + g_close (fd, NULL); | |
152 | *temp_filename = tmp_filename; | |
153 | return tmpfd; | |
154 | } | |
155 | diff --git a/gio/tests/file.c b/gio/tests/file.c | |
156 | index efb2eaadd..bc55f3af4 100644 | |
157 | --- a/gio/tests/file.c | |
158 | +++ b/gio/tests/file.c | |
159 | @@ -804,6 +804,113 @@ test_replace_cancel (void) | |
160 | g_object_unref (tmpdir); | |
161 | } | |
162 | ||
163 | +static void | |
164 | +test_replace_symlink (void) | |
165 | +{ | |
166 | +#ifdef G_OS_UNIX | |
167 | + gchar *tmpdir_path = NULL; | |
168 | + GFile *tmpdir = NULL, *source_file = NULL, *target_file = NULL; | |
169 | + GFileOutputStream *stream = NULL; | |
170 | + const gchar *new_contents = "this is a test message which should be written to source and not target"; | |
171 | + gsize n_written; | |
172 | + GFileEnumerator *enumerator = NULL; | |
173 | + GFileInfo *info = NULL; | |
174 | + gchar *contents = NULL; | |
175 | + gsize length = 0; | |
176 | + GError *local_error = NULL; | |
177 | + | |
178 | + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2325"); | |
179 | + g_test_summary ("Test that G_FILE_CREATE_REPLACE_DESTINATION doesn’t follow symlinks"); | |
180 | + | |
181 | + /* Create a fresh, empty working directory. */ | |
182 | + tmpdir_path = g_dir_make_tmp ("g_file_replace_symlink_XXXXXX", &local_error); | |
183 | + g_assert_no_error (local_error); | |
184 | + tmpdir = g_file_new_for_path (tmpdir_path); | |
185 | + | |
186 | + g_test_message ("Using temporary directory %s", tmpdir_path); | |
187 | + g_free (tmpdir_path); | |
188 | + | |
189 | + /* Create symlink `source` which points to `target`. */ | |
190 | + source_file = g_file_get_child (tmpdir, "source"); | |
191 | + target_file = g_file_get_child (tmpdir, "target"); | |
192 | + g_file_make_symbolic_link (source_file, "target", NULL, &local_error); | |
193 | + g_assert_no_error (local_error); | |
194 | + | |
195 | + /* Ensure that `target` doesn’t exist */ | |
196 | + g_assert_false (g_file_query_exists (target_file, NULL)); | |
197 | + | |
198 | + /* Replace the `source` symlink with a regular file using | |
199 | + * %G_FILE_CREATE_REPLACE_DESTINATION, which should replace it *without* | |
200 | + * following the symlink */ | |
201 | + stream = g_file_replace (source_file, NULL, FALSE /* no backup */, | |
202 | + G_FILE_CREATE_REPLACE_DESTINATION, NULL, &local_error); | |
203 | + g_assert_no_error (local_error); | |
204 | + | |
205 | + g_output_stream_write_all (G_OUTPUT_STREAM (stream), new_contents, strlen (new_contents), | |
206 | + &n_written, NULL, &local_error); | |
207 | + g_assert_no_error (local_error); | |
208 | + g_assert_cmpint (n_written, ==, strlen (new_contents)); | |
209 | + | |
210 | + g_output_stream_close (G_OUTPUT_STREAM (stream), NULL, &local_error); | |
211 | + g_assert_no_error (local_error); | |
212 | + | |
213 | + g_clear_object (&stream); | |
214 | + | |
215 | + /* At this point, there should still only be one file: `source`. It should | |
216 | + * now be a regular file. `target` should not exist. */ | |
217 | + enumerator = g_file_enumerate_children (tmpdir, | |
218 | + G_FILE_ATTRIBUTE_STANDARD_NAME "," | |
219 | + G_FILE_ATTRIBUTE_STANDARD_TYPE, | |
220 | + G_FILE_QUERY_INFO_NOFOLLOW_SYMLINKS, NULL, &local_error); | |
221 | + g_assert_no_error (local_error); | |
222 | + | |
223 | + info = g_file_enumerator_next_file (enumerator, NULL, &local_error); | |
224 | + g_assert_no_error (local_error); | |
225 | + g_assert_nonnull (info); | |
226 | + | |
227 | + g_assert_cmpstr (g_file_info_get_name (info), ==, "source"); | |
228 | + g_assert_cmpint (g_file_info_get_file_type (info), ==, G_FILE_TYPE_REGULAR); | |
229 | + | |
230 | + g_clear_object (&info); | |
231 | + | |
232 | + info = g_file_enumerator_next_file (enumerator, NULL, &local_error); | |
233 | + g_assert_no_error (local_error); | |
234 | + g_assert_null (info); | |
235 | + | |
236 | + g_file_enumerator_close (enumerator, NULL, &local_error); | |
237 | + g_assert_no_error (local_error); | |
238 | + g_clear_object (&enumerator); | |
239 | + | |
240 | + /* Double-check that `target` doesn’t exist */ | |
241 | + g_assert_false (g_file_query_exists (target_file, NULL)); | |
242 | + | |
243 | + /* Check the content of `source`. */ | |
244 | + g_file_load_contents (source_file, | |
245 | + NULL, | |
246 | + &contents, | |
247 | + &length, | |
248 | + NULL, | |
249 | + &local_error); | |
250 | + g_assert_no_error (local_error); | |
251 | + g_assert_cmpstr (contents, ==, new_contents); | |
252 | + g_assert_cmpuint (length, ==, strlen (new_contents)); | |
253 | + g_free (contents); | |
254 | + | |
255 | + /* Tidy up. */ | |
256 | + g_file_delete (source_file, NULL, &local_error); | |
257 | + g_assert_no_error (local_error); | |
258 | + | |
259 | + g_file_delete (tmpdir, NULL, &local_error); | |
260 | + g_assert_no_error (local_error); | |
261 | + | |
262 | + g_clear_object (&target_file); | |
263 | + g_clear_object (&source_file); | |
264 | + g_clear_object (&tmpdir); | |
265 | +#else /* if !G_OS_UNIX */ | |
266 | + g_test_skip ("Symlink replacement tests can only be run on Unix") | |
267 | +#endif | |
268 | +} | |
269 | + | |
270 | static void | |
271 | on_file_deleted (GObject *object, | |
272 | GAsyncResult *result, | |
273 | @@ -1754,6 +1861,7 @@ main (int argc, char *argv[]) | |
274 | g_test_add_data_func ("/file/async-create-delete/4096", GINT_TO_POINTER (4096), test_create_delete); | |
275 | g_test_add_func ("/file/replace-load", test_replace_load); | |
276 | g_test_add_func ("/file/replace-cancel", test_replace_cancel); | |
277 | + g_test_add_func ("/file/replace-symlink", test_replace_symlink); | |
278 | g_test_add_func ("/file/async-delete", test_async_delete); | |
279 | #ifdef G_OS_UNIX | |
280 | g_test_add_func ("/file/copy-preserve-mode", test_copy_preserve_mode); | |
281 | -- | |
282 | 2.30.1 | |
283 |