linux-kernel - Re: bug in may_dedupe_file allows to deduplicate files we aren't allowed to write to

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF+WW=rRz0L-P9X2tV9svGdTbhAhpBea=huf-_DDfkz29fXUyQ@mail.gmail.com>
Date: Sat, 4 May 2024 22:50:35 +0200
From: Hugo Valtier <hugo@...tier.fr>
To: Amir Goldstein <amir73il@...il.com>
Cc: mfasheh@...e.de, viro@...iv.linux.org.uk, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Christian Brauner <brauner@...nel.org>
Subject: Re: bug in may_dedupe_file allows to deduplicate files we aren't
 allowed to write to

> My guess is that not many users try to dedupe other users' files,
> so this feature was never used and nobody complained.

+1

Thx for the answer, I'm new to this to be sure I understood what you meant:
> You should add an xfstest for this and include a
> _fixed_by_kernel_commit and that will signal all the distros that
> care to backport the fix.

So right now I wait for 6.9 to be released soon enough then
I then submit my patch which invert the condition.
Once that is merged in some tree (fsdevel I guess ?) I submit a patch for
xfstest which adds a regression test and has _fixed_by_kernel_commit
mentioning the commit just merged in the fsdevel linux tree.

Le sam. 4 mai 2024 à 11:43, Amir Goldstein <amir73il@...il.com> a écrit :
>
> On Sat, May 4, 2024 at 7:49 AM Hugo Valtier <hugo@...tier.fr> wrote:
> >
> > For context I am making a file based deduplication tool.
> >
> > I found that in this commit
> > 5de4480ae7f8 ("vfs: allow dedupe of user owned read-only files")
> > it states:
> > > - the process could get write access
> >
> > However the behavior added in allow_file_dedupe now may_dedupe_file is opposite:
> > > +       if (!inode_permission(file_inode(file), MAY_WRITE))
> > > +               return true
> >
> > I've tested that I can create an other readonly file as root and have
> > my unprivileged user deduplicate it however if I then make the file
> > other writeable I cannot anymore*.
> > It doesn't make sense to me why giving write permissions on a file
> > should remove the permission to deduplicate*.
>
> True. Here is the discussion about adding "could have been opened w"
> to allow dedupe:
> https://lore.kernel.org/linux-fsdevel/20180517230150.GA28045@wotan.suse.de/
>
> >
> > I'm not sure on how to fix this, flipping the condition would work but
> > that is a breaking change and idk if this is ok here.
> > Adding a check to also users who have write access to the file would
> > remove all the logic here since you would always be allowed to dedup
> > FDs you managed to get your hands on.
> >
> > Any input on this welcome, thx
>
> My guess is that not many users try to dedupe other users' files,
> so this feature was never used and nobody complained.
> What use case do you think flipping the condition could break?
> breaking uapi is not about theoretical use cases and in any
> case this needs to be marked with Fixes: and can be backported
> as far as anyone who cares wants to backport.
>
> You should add an xfstest for this and include a
> _fixed_by_kernel_commit and that will signal all the distros that
> care to backport the fix.
>
> Thanks,
> Amir.