[ seems that this message was not sent to the list - resending ]
Hello,
I found another problem with the current behavior: if tar changes the
permissions of the directory in question, it does not always restore them
back exactly - the permissions seem to be masked by the current umask.
So if the umask is 022 and the premissions before extracting have been
'dr-xrwxrwx', the permissions after will be 'dr-xr-xr-x'. This can be
easily shown by the attached patch to the testsuite. I suspect it is
caused by the
current_mode = mode & ~ current_umask;
line in the code.
Regarding changing directory permissions and restoring them in general:
On Fri, Jan 17, 2025 at 04:35:34PM -0600, Nate Simon wrote:
> I would agree. I considered sharing a similar patch, but decided against it
> only because it seemed improper to delete code that I didn't understand the
> original purpose of.
>
> So if consensus is that the existing code needlessly complicates the way
> this argument is handled, I'd gladly withdraw my patch in favor of Pavel's.
If the existing behavior is to be preserved, I understand the temptation
to solve it in such a simple way as you proposed. Please note though that
this approach could presumably fail in many corner cases (I have not
tested them though, these are only thought experiments):
- the code might determine that it should change the permissions when in
fact it can't change them. More sophisticated security features like
SELinux or missing capabilities could prevent us from changing the
permissions even if we are root or owners of the directory. One very
common case could be root squashing on NFS, when the server would not
allow us to change the permissions as it does not consider us to be root
even though locally we are root. Another simple case would be a
read-only mounted filesystem. I don't think we should remount the
filesystem containing the directory read-write and back again inside
tar, but since you mention an archive containing an empty ./usr
directory, note that extraction of this archive would still fail if done
as root and /usr is mounted read-only.
- the code might determine that it should not change the permissions
when in fact it can change them. For example, ACLs could allow this for
a non-root, non-owner user, or the process could have a special
capability (CAP_FOWNER).
I believe that in order to solve all these issues properly, we should
not preemptively change the mode of the directory at the beginning (when
we extract the directory itself), but delay the change until the point
when we try to extract something under the directory and fail (which may
not happen at all if the directory is empty in the archive). I.e. only
when we find out that we actually need it. I suspect that such a fix
will not be simple, though.
Regards, Pavel
> On 1/16/25 15:57, Pavel Cahyna wrote:
> > [ 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;
> > > }
> >
>
>From 7d3f1fce63da1c89c0f3726da6cc054757e59e29 Mon Sep 17 00:00:00 2001
From: Pavel Cahyna <pcah...@redhat.com>
Date: Fri, 17 Jan 2025 18:08:43 +0100
Subject: [PATCH] Add umask to --no-overwrite-dir test
Proves that with umask permissions of the directory are not restored
correctly.
---
tests/extrac23.at | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tests/extrac23.at b/tests/extrac23.at
index c8943a73..3908cd9d 100644
--- a/tests/extrac23.at
+++ b/tests/extrac23.at
@@ -43,7 +43,8 @@ genfile --stat=mode.777 dir
genfile --file dir/file
tar cf a.tar dir
rm dir/file
-chmod 400 dir
+chmod 444 dir
+umask 066
tar -xf a.tar --no-overwrite-dir
genfile --stat=mode.777 dir
chmod 700 dir
@@ -51,7 +52,7 @@ find dir
],
[0],
[777
-400
+444
dir
dir/file
])
--
2.45.0
