linux-ext4 - Re: [PATCH v2] fs/{posix_acl,ext2,jfs,ceph}: apply umask if ACL support is disabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <28DSITL9912E1.2LSZUVTGTO52Q@mforney.org>
Date: Wed, 13 Mar 2024 13:40:46 -0700
From: Michael Forney <mforney@...rney.org>
To: Jan Kara <jack@...e.cz>
Cc: Theodore Ts'o <tytso@....edu>, Christian Brauner <brauner@...nel.org>,
 Max Kellermann <max.kellermann@...os.com>, Xiubo Li <xiubli@...hat.com>,
 Ilya Dryomov <idryomov@...il.com>, Jeff Layton <jlayton@...nel.org>, Jan
 Kara <jack@...e.com>, Dave Kleikamp <shaggy@...nel.org>,
 ceph-devel@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-ext4@...r.kernel.org, jfs-discussion@...ts.sourceforge.net, Yang Xu
 <xuyang2018.jy@...itsu.com>, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2] fs/{posix_acl,ext2,jfs,ceph}: apply umask if ACL
 support is disabled

Jan Kara <jack@...e.cz> wrote:
> On Thu 12-10-23 10:29:18, Theodore Ts'o wrote:
> > On Wed, Oct 11, 2023 at 07:26:06PM +0200, Jan Kara wrote:
> > > I don't think this is accurate. posix_acl_create() needs unmasked 'mode'
> > > because instead of using current_umask() for masking it wants to use
> > > whatever is stored in the ACLs as an umask.
> > > 
> > > So I still think we need to keep umask handling in both posix_acl_create()
> > > and vfs_prepare_mode(). But filesystem's only obligation would be to call
> > > posix_acl_create() if the inode is IS_POSIXACL. No more caring about when
> > > to apply umask and when not based on config or mount options.
> > 
> > Ah, right, thanks for the clarification.  I *think* the following
> > patch in the ext4 dev branch (not yet in Linus's tree, but it should
> > be in linux-next) should be harmless, though, right?  And once we get
> > the changes in vfs_prepare_mode() we can revert in ext4 --- or do
> > folks I think I should just drop it from the ext4 dev branch now?
> 
> It definitely does no harm. As you say, you can revert it once the VFS
> changes land if you want.

I've been debugging why flatpak was always considering its database
corrupted, and found this commit to be the source of the issue.

$ ostree --repo=repo --mode=bare-user-only init
$ mkdir tree && umask 0 && ln -s target tree/symlink && umask 022
$ ostree --repo=repo commit --branch=foo tree/
c508e0564267b376661889b9016f8438bd6d39412078838f78856383fdd8ac2f
$ ostree --repo=repo fsck
Validating refs...
Validating refs in collections...
Enumerating commits...
Verifying content integrity of 1 commit objects...
fsck objects (1/4) [===          ]  25%
error: In commits c508e0564267b376661889b9016f8438bd6d39412078838f78856383fdd8ac2f: fsck content object a6b40a5400ed082fbe067d2c8397aab54046a089768651c392a36db46d24c1cd: Corrupted file object; checksum expected='a6b40a5400ed082fbe067d2c8397aab54046a089768651c392a36db46d24c1cd'
actual='6bdc88f9722f96dbd51735e381f8a1b0e01363e1d7ee2edbb474c091f83c3987'
$

Turns out that symlinks are inheriting umask on my system (which
has CONFIG_EXT4_FS_POSIX_ACL=n):

$ umask 022
$ ln -s target symlink
$ ls -l symlink
lrwxr-xr-x    1 michael  michael           6 Mar 13 13:28 symlink -> target
$

Looking at the referenced functions, posix_acl_create() returns
early before applying umask for symlinks, but ext4_init_acl() now
applies the umask unconditionally.

After reverting this commit, it works correctly. I am also unable
to reproduce the mentioned issue with O_TMPFILE after reverting the
commit. It seems that the bug was fixed properly in ac6800e279a2
('fs: Add missing umask strip in vfs_tmpfile'), and all branches
that have this ext4_init_acl patch already had ac6800e279a2 backported.

So I think this patch should be reverted, since the bug was already
fixed and it breaks symlink modes. If not, it should at least be
changed to not to apply the umask to symlinks.

> > commit 484fd6c1de13b336806a967908a927cc0356e312
> > Author: Max Kellermann <max.kellermann@...os.com>
> > Date:   Tue Sep 19 10:18:23 2023 +0200
> > 
> >     ext4: apply umask if ACL support is disabled
> >     
> >     The function ext4_init_acl() calls posix_acl_create() which is
> >     responsible for applying the umask.  But without
> >     CONFIG_EXT4_FS_POSIX_ACL, ext4_init_acl() is an empty inline function,
> >     and nobody applies the umask.
> >     
> >     This fixes a bug which causes the umask to be ignored with O_TMPFILE
> >     on ext4:
> >     
> >      https://github.com/MusicPlayerDaemon/MPD/issues/558
> >      https://bugs.gentoo.org/show_bug.cgi?id=686142#c3
> >      https://bugzilla.kernel.org/show_bug.cgi?id=203625
> >     
> >     Reviewed-by: "J. Bruce Fields" <bfields@...hat.com>
> >     Cc: stable@...r.kernel.org
> >     Signed-off-by: Max Kellermann <max.kellermann@...os.com>
> >     Link: https://lore.kernel.org/r/20230919081824.1096619-1-max.kellermann@ionos.com
> >     Signed-off-by: Theodore Ts'o <tytso@....edu>
> > 
> > diff --git a/fs/ext4/acl.h b/fs/ext4/acl.h
> > index 0c5a79c3b5d4..ef4c19e5f570 100644
> > --- a/fs/ext4/acl.h
> > +++ b/fs/ext4/acl.h
> > @@ -68,6 +68,11 @@ extern int ext4_init_acl(handle_t *, struct inode *, struct inode *);
> >  static inline int
> >  ext4_init_acl(handle_t *handle, struct inode *inode, struct inode *dir)
> >  {
> > +	/* usually, the umask is applied by posix_acl_create(), but if
> > +	   ext4 ACL support is disabled at compile time, we need to do
> > +	   it here, because posix_acl_create() will never be called */
> > +	inode->i_mode &= ~current_umask();
> > +
> >  	return 0;
> >  }
> >  #endif  /* CONFIG_EXT4_FS_POSIX_ACL */