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:   Wed, 2 Feb 2022 11:03:49 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Rich Felker <dalias@...c.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Ariadne Conill <ariadne@...eferenced.org>,
        Michael Kerrisk <mtk.manpages@...il.com>,
        Matthew Wilcox <willy@...radead.org>,
        Christian Brauner <brauner@...nel.org>,
        Eric Biederman <ebiederm@...ssion.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, stable@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] exec: Force single empty string when argv is empty

On Wed, Feb 02, 2022 at 12:12:19PM -0500, Rich Felker wrote:
> On Wed, Feb 02, 2022 at 07:50:42AM -0800, Kees Cook wrote:
> > On February 1, 2022 6:53:25 AM PST, Rich Felker <dalias@...c.org> wrote:
> > >From #musl:
> > >
> > ><mixi> kees: shouldn't the min(bprm->argc, 1) be max(...) in your patch?
> > 
> > Fix has already been sent, yup.
> > 
> > >I'm pretty sure without fixing that, you're introducing a giant vuln
> > >here.
> > 
> > I wouldn't say "giant", but yes, it weakened a defense in depth for
> > avoiding high stack utilization.
> 
> I thought it was deciding the amount of memory to allocate/reserve for
> the arg slots, but based on the comment it looks like it's just a way
> to fail early rather than making the new process image fault later if
> they don't fit.

Right.

> > > I believe this is the second time a patch attempting to fix this
> > >non-vuln has proposed adding a new vuln...
> > 
> > Mistakes happen, and that's why there is review and testing. Thank
> > you for being part of the review process! :)
> 
> I know, and I'm sorry for being a bit hostile over it, and for jumping
> the gun about the severity. I just get frustrated when I see a rush to
> make changes over an incidental part of a popularized vuln, with
> disproportionate weight on "doing something" and not enough on being
> careful.

Sure, I can see it looks that way. My sense of urgency on this in
particular is that we're early in the development cycle, and it's an
ABI-breaking change, so I want to maximize how much time it has to get
tested out in real workloads. (i.e. we've now seen that just rejecting
NULL argv is likely too painful, etc.)

All that said, I regularly bemoan the lack of sufficient regression
tests for execve() and the binfmt_*.c loaders. I've written a couple,
and so have others, but what I really want is a library of "binary that
got broken by exec change" for doing regression testing against. That
gets hampered by both size, redistribution issues, etc, so I've wanted
to have minimal reproducers for each, but creating those take time, etc,
etc.

(And you'll note I wrote[1] a test for this particular behavior, because
I'm trying to avoid falling further behind in test coverage.)

I would _love_ it if someone had the time and attention to go through
all the past binaries and make a regression test series. :)

-Kees

[1] https://lore.kernel.org/linux-hardening/20220201011637.2457646-1-keescook@chromium.org/

-- 
Kees Cook

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ