linux-kernel - Re: [PATCH] fs/coredump: open coredump file in O_WRONLY instead of O

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]

Date:   Mon, 15 May 2023 19:55:04 +0200
From:   Christian Brauner <brauner@...nel.org>
To:     Vladimir Sementsov-Ogievskiy <vsementsov@...dex-team.ru>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        ptikhomirov@...tuozzo.com, Andrey Ryabinin <arbn@...dex-team.com>,
        Linus Torvalds <torvalds@...uxfoundation.org>
Subject: Re: [PATCH] fs/coredump: open coredump file in O_WRONLY instead of
 O_RDWR

On Wed, May 10, 2023 at 06:15:41PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Gently ping.
> 
> Is there any interest?

The question that I would've loved to have an answer to was why was it
made O_RDWR and not just O_WRONLY in the first place. Was there a time
when this was meaningful? Because honestly this looks innocent and
straightforward and then it always makes me go and think "Oh, there's
probably a good reason and something super obvious I'm missing.".

Funny enough, this code was originally:

    if (open_namei("core",O_CREAT | O_WRONLY | O_TRUNC,0600,&inode,NULL)

and then became:

    if (open_namei("core",O_CREAT | 2 | O_TRUNC,0600,&inode,NULL))

in

    commit 9cb9f18b5d26 ("[PATCH] Linux-0.99.10 (June 7, 1993)")

Author/applier of said patch Cced (more for the fun of referencing
Linux-0.99.10 than anything else).

So after this commit the flag combination just got copied over and over.
First when coredump handling was moved out of fs/exec.c into the
individual binfmt handlers and then again when it was moved back into
fs/exec.c and then again when it was moved to fs/coredump.c.

So that open-coded 2 added in commit 9cb9f18b5d26 ("[PATCH]
Linux-0.99.10 (June 7, 1993)") survived for 23 years until it was
replaced by Jan in 378c6520e7d2 ("fs/coredump: prevent fsuid=0 dumps
into user-controlled directories"). 

So no one could be bothered for 23 years to use O_RDWR instead of that
lonely 2 which is kinda funny. :)

In any case, I don't see anything that suggests this would cause issues.
So I'm going to pick this up unless I'm being told I'm being obviously
stupid and this absolutely needs to be O_RDWR...

> 
> On 20.04.23 15:04, Vladimir Sementsov-Ogievskiy wrote:
> > This makes it possible to make stricter apparmor profile and don't
> > allow the program to read any coredump in the system.
> > 
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@...dex-team.ru>
> > ---
> >   fs/coredump.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index 5df1e6e1eb2b..8f263a389175 100644
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> > @@ -646,7 +646,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
> >   	} else {
> >   		struct mnt_idmap *idmap;
> >   		struct inode *inode;
> > -		int open_flags = O_CREAT | O_RDWR | O_NOFOLLOW |
> > +		int open_flags = O_CREAT | O_WRONLY | O_NOFOLLOW |
> >   				 O_LARGEFILE | O_EXCL;
> >   		if (cprm.limit < binfmt->min_coredump)
> 
> -- 
> Best regards,
> Vladimir
>