Sam James <[email protected]> writes:
> Collin Funk <[email protected]> writes:
>
>> Paul Eggert <[email protected]> writes:
>>
>>> On 2025-08-07 08:47, Lior Kaplan wrote:
>>>> I was wondering if you had a chance to look at
>>>> https://nvd.nist.gov/vuln/detail/CVE-2025-45582
>>>
>>> First I've heard of it. Thanks for mentioning it.
>>>
>>> Sounds like tar by default should refuse to create symlinks to outside
>>> the working directory. Those symlinks are trouble anyway, regardless
>>> of whether the following program is tar or some other program.
>>
>> I agree with the behavior change.
>>
>> Extracting untrusted tar archives is already dangerous, so I don't think
>> it is worth any panic though.
>
> +1. Let's do this please.
I attached a patch which could probably be much improved. But works for
the original case.
Without '-P' we check for '..', so it seems reasonable to do it for
symlinks as well. Here is an example with this patch:
$ rm -rf /home/ubuntu/.ssh
$ ./src/tar -xf my_archive2.tar.gz
./src/tar: Member component has link my_directory ->
../../../../../../../home/ubuntu/ containing '..'
./src/tar: Member component has link my_directory ->
../../../../../../../home/ubuntu/ containing '..'
./src/tar: Exiting with failure status due to previous errors
$ stat /home/ubuntu/.ssh
stat: cannot statx '/home/ubuntu/.ssh': No such file or directory
And using '-P' with this patch:
$ ./src/tar -P -xf my_archive2.tar.gz
$ stat --format=%n /home/ubuntu/.ssh/authorized_keys
/home/ubuntu/.ssh/authorized_keys
I don't mind putting it behind a new option instead of '-P', but am not
creative enough to think of a long option name at the moment. :)
Collin
>From 9100a8768ee0b0925f9ca58356b3fbc3735a6c6e Mon Sep 17 00:00:00 2001
Message-ID: <9100a8768ee0b0925f9ca58356b3fbc3735a6c6e.1754637488.git.collin.fu...@gmail.com>
From: Collin Funk <[email protected]>
Date: Thu, 7 Aug 2025 23:59:07 -0700
Subject: [PATCH] tar: disallow symlinks containing '..' unless -P is used
* src/extract.c: Include areadlink.h
(contains_dot_dot_safe): New function.
(extract_archive): Use it.
---
src/extract.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/src/extract.c b/src/extract.c
index 3b913c54..22d61680 100644
--- a/src/extract.c
+++ b/src/extract.c
@@ -27,6 +27,7 @@
#include <priv-set.h>
#include <root-uid.h>
#include <utimens.h>
+#include <areadlink.h>
#include "common.h"
@@ -1806,6 +1807,65 @@ prepare_to_extract (char const *file_name, char typeflag)
return extractor;
}
+/* Checks for '..' in FILE_NAME or any path components that are symlink
+ containing '..'. */
+
+static bool
+contains_dot_dot_safe (char *file_name)
+{
+ /* Check for '..' without any symbolic links first. */
+ if (contains_dot_dot (file_name))
+ {
+ paxerror (0, _("%s: Member name contains '..'"),
+ quotearg_colon (file_name));
+ return true;
+ }
+
+ char *p = file_name + FILE_SYSTEM_PREFIX_LEN (file_name);
+ char *end = p + strlen (p);
+
+ for (char *current = p; current < end; ++current)
+ {
+ char tmp;
+ struct stat st;
+ for (; ISSLASH (current[0]); ++current)
+ ;
+ for (; current < end && ! ISSLASH (current[0]); ++current)
+ ;
+ tmp = current[0];
+ current[0] = '\0';
+ if (fstatat (chdir_fd, file_name, &st, AT_SYMLINK_NOFOLLOW) < 0)
+ {
+ if (errno == ENOENT)
+ return false;
+ else
+ stat_error (file_name);
+ }
+ if (S_ISLNK (st.st_mode))
+ {
+ char *link_name = areadlinkat_with_size (chdir_fd, file_name,
+ st.st_size);
+ if (link_name == nullptr)
+ readlink_error (file_name);
+ else
+ {
+ bool result = contains_dot_dot (link_name);
+ if (result)
+ {
+ paxerror (0, _("Member component has link %s -> %s "
+ "containing '..'"),
+ quotearg (file_name), quotearg_n (1, link_name));
+ free (link_name);
+ return true;
+ }
+ free (link_name);
+ }
+ }
+ current[0] = tmp;
+ }
+ return false;
+}
+
/* Extract a file from the archive. */
void
extract_archive (void)
@@ -1817,11 +1877,9 @@ extract_archive (void)
set_next_block_after (current_header);
+
skip_dotdot_name = (!absolute_names_option
- && contains_dot_dot (current_stat_info.orig_file_name));
- if (skip_dotdot_name)
- paxerror (0, _("%s: Member name contains '..'"),
- quotearg_colon (current_stat_info.orig_file_name));
+ && contains_dot_dot_safe (current_stat_info.orig_file_name));
if (!current_stat_info.file_name[0]
|| skip_dotdot_name
--
2.50.1
