[ Please Cc me on replies, I am not subscribed to bug-tar@ ]
Hello Nate,
thank you for the observation and patch. To me the new behavior (introduced by
http://git.savannah.gnu.org/cgit/tar.git/commit/?id=14d8fc718f0
in response to
https://www.mail-archive.com/bug-tar@gnu.org/msg05838.html
) looks incorrect as well. But see below:
On Tue, Nov 26, 2024 at 02:50:26PM -0600, Nate Simon wrote:
> I went ahead and tested a patch for this issue. This is my first
> contribution, so I hope it is formatted acceptably.
>
> When looking at the extractor source, I wasn't sure about this section which
> switches a target directory to a "safe" permission mode then changes it back
> afterward. I'm not sure this logic follows the documented behavior of
> `--no-overwrite-dir` since it still updates the "Change"/"Modify" metadata
> fields. It seems to me if a target directory is not traversable, extraction
> with no-overwrite-dir should fail where it currently does not. Nonetheless,
> I'm sure there's a reason I'm simply not aware of, so this patch does
> preserve the existing behavior.
I would like to know the reason, because when extracting to an existing
directory that is not writable, tar does not make it writable, instead
it fails:
$ chmod a-w .
$ tar -x -f ../test.tar
tar: test: Cannot mkdir: Permission denied
tar: Exiting with failure status due to previous errors
so the user is responsible for having the permissions or appropriate
privileges to write to the directory, and it does not seem to have been
a problem, so why should be existing subdirectories given the
--no-overwrite-dir option behave differently? I feel the code is a bit
too smart.
Therefore I would like to propose an alternative patch that simply
removes the code to change the directory permissions - attached. WDYT?
(Of course the associated test would need changing as well.)
Best regards, Pavel
> Thus my addition checks the euid to see if the current user is able to
> perform this permissions swap; and if the change would fail, do nothing
> instead. This is preferred for cases where users can have absolute paths to
> user-specific files/folders. For example, extracting a tar file containing
> './home/nate/myfile', using the user 'nate' fails in the current tar
> version, but passes with this patch.
>
> Nate Simon
>
>
> On 11/11/24 08:43, Nate Simon wrote:
> > Tar appears to be not preserving all metadata when --no-overwrite-dir is
> > used for file extraction. I started seeing this regression when we
> > updated from Ubuntu 20 to Ubuntu 22: tar 1.30 -> 1.34.
> >
> > This is the example that made me aware of the issue. These commands pass
> > with tar 1.30.
> >
> > $ mkdir usr
> > $ tar -cf test.tar usr
> > $ tar --no-overwrite-dir -xf test.tar -C /
> > tar: usr: Cannot change mode to rwxr-xr-x: Operation not permitted
> > tar: Exiting with failure status due to previous errors
> >
> >
> > And here is a similar example with folder metadata. I observed that even
> > with '--no-overwrite-dir' passed to tar, the folder's "modify" metadata
> > is rolled back to match the folder contained in the tarball. But based
> > on the documentation, I expected neither Modify nor Change fields to
> > update.
> >
> > $ mkdir test
> > $ tar -cf test2.tar test
> > $ rmdir test
> > $ mkdir test
> > $ stat test
> > File: test
> > Size: 4096 Blocks: 8 IO Block: 4096 directory
> > Device: 259,2 Inode: 21561426 Links: 2
> > Access: (0755/drwxr-xr-x) Uid: ( 1000/ nate) Gid: ( 1000/ nate)
> > Access: 2024-11-11 08:26:02.696280292 -0600
> > Modify: 2024-11-11 08:26:02.696280292 -0600
> > Change: 2024-11-11 08:26:02.696280292 -0600
> > Birth: 2024-11-11 08:26:02.696280292 -0600
> >
> > $ tar --no-overwrite-dir -xf test2.tar
> > $ stat test
> > File: test
> > Size: 4096 Blocks: 8 IO Block: 4096 directory
> > Device: 259,2 Inode: 21561426 Links: 2
> > Access: (0755/drwxr-xr-x) Uid: ( 1000/ nate) Gid: ( 1000/ nate)
> > Access: 2024-11-11 08:26:02.696280292 -0600
> > Modify: 2024-11-11 08:23:40.000000000 -0600
> > Change: 2024-11-11 08:26:17.222659660 -0600
> > Birth: 2024-11-11 08:26:02.696280292 -0600
> diff --git a/src/extract.c b/src/extract.c
> index f741943f..d3205766 100644
> --- a/src/extract.c
> +++ b/src/extract.c
> @@ -1134,25 +1134,31 @@ extract_dir (char *file_name, char typeflag)
> /* Temporarily change the directory mode to a safe
> value, to be able to create files in it, should
> the need be.
> + If extracting as non-root user without permission to
> + the target folder, the user assumes responsibility for
> + ensuring workable permissions.
> */
> - mode = safe_dir_mode (&st);
> - status = fd_chmod (-1, file_name, mode,
> - AT_SYMLINK_NOFOLLOW, DIRTYPE);
> - if (status == 0)
> - {
> - /* Store the actual directory mode, to be restored
> - later.
> - */
> - current_stat_info.stat = st;
> - current_mode = mode & ~ current_umask;
> - current_mode_mask = MODE_RWX;
> - atflag = AT_SYMLINK_NOFOLLOW;
> - break;
> - }
> - else
> - {
> - chmod_error_details (file_name, mode);
> - }
> + if (we_are_root || st.st_uid == geteuid ())
> + {
> + mode = safe_dir_mode (&st);
> + status = fd_chmod (-1, file_name, mode,
> + AT_SYMLINK_NOFOLLOW, DIRTYPE);
> + if (status == 0)
> + {
> + /* Store the actual directory mode, to be restored
> + later.
> + */
> + current_stat_info.stat = st;
> + current_mode = mode & ~ current_umask;
> + current_mode_mask = MODE_RWX;
> + atflag = AT_SYMLINK_NOFOLLOW;
> + break;
> + }
> + else
> + {
> + chmod_error_details (file_name, mode);
> + }
> + }
> }
> break;
> }
diff --git a/src/extract.c b/src/extract.c
index 5fc2d9cf..448635ba 100644
--- a/src/extract.c
+++ b/src/extract.c
@@ -1129,31 +1129,6 @@ extract_dir (char *file_name, char typeflag)
repair_delayed_set_stat (file_name, &st);
return true;
}
- else if (old_files_option == NO_OVERWRITE_DIR_OLD_FILES)
- {
- /* Temporarily change the directory mode to a safe
- value, to be able to create files in it, should
- the need be.
- */
- mode = safe_dir_mode (&st);
- status = fd_chmod (-1, file_name, mode,
- AT_SYMLINK_NOFOLLOW, DIRTYPE);
- if (status == 0)
- {
- /* Store the actual directory mode, to be restored
- later.
- */
- current_stat_info.stat = st;
- current_mode = mode & ~ current_umask;
- current_mode_mask = MODE_RWX;
- atflag = AT_SYMLINK_NOFOLLOW;
- break;
- }
- else
- {
- chmod_error_details (file_name, mode);
- }
- }
break;
}
}
