Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?

From: Rich Felker <dalias@aerifal.cx>
To: Christian Brauner <brauner@kernel.org>
Cc: Xi Ruoyao <xry111@xry111.site>, Arnd Bergmann <arnd@arndb.de>,
	Icenowy Zheng <uwu@icenowy.me>,
	Huacai Chen <chenhuacai@kernel.org>,
	WANG Xuerui <kernel@xen0n.name>,
	Adhemerval Zanella <adhemerval.zanella@linaro.org>,
	linux-api@vger.kernel.org, Kees Cook <keescook@chromium.org>,
	Xuefeng Li <lixuefeng@loongson.cn>,
	Jianmin Lv <lvjianmin@loongson.cn>,
	Xiaotian Wu <wuxiaotian@loongson.cn>,
	WANG Rui <wangrui@loongson.cn>,
	Miao Wang <shankerwangmiao@gmail.com>,
	"loongarch@lists.linux.dev" <loongarch@lists.linux.dev>,
	Linux-Arch <linux-arch@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
Date: Mon, 26 Feb 2024 09:33:16 -0500	[thread overview]
Message-ID: <20240226143315.GA22081@brightrain.aerifal.cx> (raw)
In-Reply-To: <20240226-siegen-desolat-49d1e20ba2cd@brauner>

On Mon, Feb 26, 2024 at 01:57:55PM +0100, Christian Brauner wrote:
> On Mon, Feb 26, 2024 at 07:57:56PM +0800, Xi Ruoyao wrote:
> > On Mon, 2024-02-26 at 10:20 +0100, Arnd Bergmann wrote:
> > 
> > /* snip */
> > 
> > > 
> > > > Or maybe we can just introduce a new AT_something to make statx
> > > > completely ignore pathname but behave like AT_EMPTY_PATH + "".
> 
> I'm not at all convinced about doing custom semantics for this.

I did not follow the entirety of this thread. I've been aware for a
while that the need to use AT_EMPTY_PATH (on archs that don't have old
syscalls) is a performance problem in addition to being a sandboxing
problem, because the semantics for it were defined to use the string
argument if present, thereby requiring the kernel to perform an
additional string read from user memory.

Unfortunately, I don't see any good fix. Even if we could add
AT_STATX_NO_PATH/AT_STATX_NULL_PATH, libc would not be using it,
because using them would incur EINVAL-then-fallback on kernels that
don't support it.

In regards to the Chromium sandbox, I think Chromium is just wrong
here. Blocking statx is not safe (it also does not work on 32-bit
archs -- it breaks time64 support! and riscv32 doesn't even have
legacy stat either, just like loongarch64), and there is really no
serious security risk from being able to stat arbitrary pathnames.
Maybe it's annoying from a theoretical purity standpoint that you
can't block that, but from a practical standpoint it doesn't really
matter.

I'd like to see a solution to this, but all the possible ones look
bad. And it's all a consequence of poor consideration of how
AT_EMPTY_PATH should work when it was first invented/added. >_<

> > > I think this is better than going back to fstat64_time64(), but
> > > it's still not great because
> > > 
> > > - all the reserved flags on statx() are by definition incompatible
> > >   with existing kernels that return -EINVAL for any flag they do
> > >   not recognize.
> > 
> > Oops, we are deeming passing undefined flags in "mask" undefined
> > behavior but not "flags", thus "wild software" may be relying on EINVAL
> > for invalid flags...  We *might* make this new AT_xxx a bit in mask
> > instead of flags but it would be very dirty IMO.
> 
> Uhm, no. AT_* flags have nothing to do in statx()'s mask argument at all.

They definitely cannot go in mask; that's semantically wrong.

Rich