From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from atuin.qyliss.net (localhost [IPv6:::1]) by atuin.qyliss.net (Postfix) with ESMTP id 00E021F1A9; Thu, 13 Nov 2025 13:22:20 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 993) id C70291F15C; Thu, 13 Nov 2025 13:22:17 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-26) on atuin.qyliss.net X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DMARC_MISSING,RCVD_IN_DNSWL_LOW,SPF_HELO_PASS autolearn=unavailable autolearn_force=no version=4.0.1 Received: from fhigh-a2-smtp.messagingengine.com (fhigh-a2-smtp.messagingengine.com [103.168.172.153]) by atuin.qyliss.net (Postfix) with ESMTPS id 7EB431F15A for ; Thu, 13 Nov 2025 13:22:16 +0000 (UTC) Received: from phl-compute-07.internal (phl-compute-07.internal [10.202.2.47]) by mailfhigh.phl.internal (Postfix) with ESMTP id EE46D14001D9; Thu, 13 Nov 2025 08:22:14 -0500 (EST) Received: from phl-mailfrontend-01 ([10.202.2.162]) by phl-compute-07.internal (MEProxy); Thu, 13 Nov 2025 08:22:14 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alyssa.is; h=cc :cc:content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm2; t=1763040134; x=1763126534; bh=aqbg/bQlKW UXKWeN4Q8cTYF+QK88WXmiS/Cvfg10m68=; b=ox3dgQSkR32mGgKOXmPeRf/TA1 XRt3wXdncC2hv/u7HkIQsxqaW/dwu33HQXD3uZaZiQrPCTeSeAQVugcFVjQ3NlJt qe4hCl18rCxtr239Lx1VHDczmK4uu3Y1XDedhUz0qItfwF8JeCTPwq+x0nzNcTVO /7EZRQ1AMfxxfW+vZwYHfZg7VHyI++GsG1IMi+cAciwEU4Qa2OSY6cCG6yne7gBi PNxayeJBVrd5hehYia21trFXEM/zdZv8odprRxnKtEMvjZB8+7fMtsggP0J966wJ OKMqxmJ5OMunsxzzvlt+eIbP/akQgkT4xfYlytBWBo8AUCyc2ok3ySwEXYng== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; t= 1763040134; x=1763126534; bh=aqbg/bQlKWUXKWeN4Q8cTYF+QK88WXmiS/C vfg10m68=; b=qvZqy6JGoX/JBpZDc02/Kuq+gAILOUA0oArmL2EhlgKzATBqSDY FuE+ebKuw3UE7X2tVERUSfm+SvjAeq8KyHbtLinUOfHzIIzHD2QRfAG2zJNlC789 3MUsy2Nw7i9VQYmWY/u5lfeV9mbgUd83pq9AwxLjXd0s8p79WJfbFia0h3bp+CGo Vjh7FrZVFsPIOwkAF6Zojr8RVJLv6p7jm1kvEE7pCLpSt/MnOvE4tEQXF3p6DP+f /h2b6GkVsobG/e1b4gsENozU8WGlZdKc+Z8PuyGRTWAX44iR8Jgey5lMR9rI3198 jAAMvvFTtYa/VchdFNJPG75JIzPKZFa5TdQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeffedrtdeggddvtdejtdehucetufdoteggodetrf dotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfurfetoffkrfgpnffqhgenuceu rghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujf gurhephffvvefujghffffkgggtsehgtderredttddtnecuhfhrohhmpeetlhihshhsrgcu tfhoshhsuceohhhisegrlhihshhsrgdrihhsqeenucggtffrrghtthgvrhhnpeeiudffue eilefgtefgtddttdekkeehkefgheekudefveetgeefiefftedvteeuveenucevlhhushht vghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehhihesrghlhihsshgrrd hishdpnhgspghrtghpthhtohepvddpmhhouggvpehsmhhtphhouhhtpdhrtghpthhtohep uggvmhhiohgsvghnohhurhesghhmrghilhdrtghomhdprhgtphhtthhopeguvghvvghlse hsphgvtghtrhhumhdqohhsrdhorhhg X-ME-Proxy: Feedback-ID: i12284293:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 13 Nov 2025 08:22:05 -0500 (EST) Received: by mbp.qyliss.net (Postfix, from userid 1000) id AABE8697A46B; Thu, 13 Nov 2025 14:21:12 +0100 (CET) From: Alyssa Ross To: Demi Marie Obenour Subject: Re: [PATCH v2 3/8] tools: Add directory checker for updates In-Reply-To: <20251112-updates-v2-3-88d96bf81b79@gmail.com> References: <20251112-updates-v2-0-88d96bf81b79@gmail.com> <20251112-updates-v2-3-88d96bf81b79@gmail.com> Date: Thu, 13 Nov 2025 14:21:10 +0100 Message-ID: <87h5uygjax.fsf@alyssa.is> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Message-ID-Hash: XD37LCAF43HH7FUG2PG23WJRIMR2NDKM X-Message-ID-Hash: XD37LCAF43HH7FUG2PG23WJRIMR2NDKM X-MailFrom: hi@alyssa.is X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-devel.spectrum-os.org-0; header-match-devel.spectrum-os.org-1; header-match-devel.spectrum-os.org-2; header-match-devel.spectrum-os.org-3; header-match-devel.spectrum-os.org-4; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Spectrum OS Development X-Mailman-Version: 3.3.9 Precedence: list List-Id: Patches and low-level development discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Demi Marie Obenour writes: > Spectrum OS's host has no network access. Updates must be downloaded by > VMs. The downloads are placed into a bind-mounted directory. The VM > can write whatever it wants into that directory. This includes symlinks > that subsequent code might open, which would create a path traversal > vulnerability. It also includes paths with names containing containing > terminal escape sequences, newlines, or other nastiness. Furthermore, > the directory should not have any subdirectories either. > > Add a simple C program that checks for such ugliness and indicates > (via its exit code) if the VM misbehaved. It also ensures that both > SHA256SUMS and SHA256SUMS.gpg are present. This needs updated, because it doesn't any more. > Signed-off-by: Demi Marie Obenour > --- > tools/default.nix | 1 + > tools/meson.build | 4 +++ > tools/updates-dir-check.c | 78 +++++++++++++++++++++++++++++++++++++++++= ++++++ > 3 files changed, 83 insertions(+) > > diff --git a/tools/default.nix b/tools/default.nix > index 18d4dd6353edf5c128213fa5c1716717f90edf07..a1b352e660f02a53e97ed1e34= 20a4de90bb24ce3 100644 > --- a/tools/default.nix > +++ b/tools/default.nix > @@ -77,6 +77,7 @@ stdenv.mkDerivation (finalAttrs: { > ./sd-notify-adapter.c > ./start-vmm > ./subprojects > + ./updates-dir-check.c > ] ++ lib.optionals driverSupport [ > ./xdp-forwarder > ])); > diff --git a/tools/meson.build b/tools/meson.build > index d465e99c2ef597fdf7e818748d08db3d96f4ec6b..a7c21684dd64ad9e87c85bcdf= 31792e81b55faa4 100644 > --- a/tools/meson.build > +++ b/tools/meson.build > @@ -29,6 +29,10 @@ if get_option('host') > c_args : '-D_GNU_SOURCE', > install: true) >=20=20 > + executable('updates-dir-check', 'updates-dir-check.c', > + c_args : '-D_GNU_SOURCE', > + install: true) > + > subdir('lsvm') > subdir('start-vmm') > endif > diff --git a/tools/updates-dir-check.c b/tools/updates-dir-check.c > new file mode 100644 > index 0000000000000000000000000000000000000000..15b58204476299d8e7fe7ffdb= ac5245e04332e7d > --- /dev/null > +++ b/tools/updates-dir-check.c > @@ -0,0 +1,78 @@ > +// SPDX-License-Identifier: EUPL-1.2+ > +// SPDX-FileCopyrightText: 2025 Demi Marie Obenour > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +#include Kernel headers should always follow libc headers. Sometimes they rely on defines from libc (especially with musl). Although openat2 doesn't actually seem to be used any more, so these headers just need a prune. > + > +static void checkdir(int fd) > +{ > + DIR *d =3D fdopendir(fd); > + if (d =3D=3D NULL) > + err(EXIT_FAILURE, "fdopendir"); > + // If there is an I/O error while there are dirty pages outstanding, > + // the dirty pages are silently discarded. This means that the contents > + // of the filesystem can change behind userspace's back. Flush all > + // dirty pages in the filesystem with the directory to prevent this. > + if (syncfs(fd) !=3D 0) > + err(EXIT_FAILURE, "syncfs"); > + for (;;) { > + errno =3D 0; > + struct dirent *entry =3D readdir(d); > + if (entry =3D=3D NULL) { > + if (errno) > + err(EXIT_FAILURE, "readdir"); > + break; > + } > + assert(entry->d_reclen > offsetof(struct dirent, d_name)); Would be a POSIX violation for d_name not to be valid I think. > + size_t len =3D strnlen(entry->d_name, entry->d_reclen - offsetof(struc= t dirent, d_name)); POSIX also guarantees a terminating null byte, so strlen() would be fine he= re. > + if (entry->d_name[0] =3D=3D '.') > + if (len =3D=3D 1 || (len =3D=3D 2 && entry->d_name[1] =3D=3D '.')) > + continue; > + unsigned char c =3D (unsigned char)entry->d_name[0]; > + if (!((c >=3D 'A' && c <=3D 'Z') || > + (c >=3D 'a' && c <=3D 'z'))) > + errx(EXIT_FAILURE, "Filename must begin with an ASCII letter"); Would the comparison not be valid without the cast? > + for (size_t i =3D 1; i < len; ++i) { > + c =3D (unsigned char)entry->d_name[i]; > + if (!((c >=3D 'A' && c <=3D 'Z') || > + (c >=3D 'a' && c <=3D 'z') || > + (c >=3D '0' && c <=3D '9') || > + (c =3D=3D '_') || > + (c =3D=3D '-') || > + (c =3D=3D '.'))) { > + if (c >=3D 0x20 && c <=3D 0x7E) > + errx(EXIT_FAILURE, "Forbidden subsequent character in filename: '%c= '", (int)c); > + else > + errx(EXIT_FAILURE, "Forbidden subsequent character in filename: byt= e %d", (int)c); > + } > + } > + if (entry->d_name[len - 1] =3D=3D '.') > + errx(EXIT_FAILURE, "Filename must not end with a '.'"); I'm still not sold on this validation, but as long as it doesn't cause problems I guess it's fine. > + if (entry->d_type !=3D DT_REG) > + errx(EXIT_FAILURE, "Entry contains non-regular file %s", entry->d_nam= e); > + } > + closedir(d); > +} > + > +int main(int argc, char **argv) > +{ > + for (int i =3D 1; i < argc; ++i) { > + int fd =3D open(argv[i], O_DIRECTORY|O_RDONLY|O_CLOEXEC|O_NOFOLLOW); Wasn't O_NOFOLLOW going to be removed here? > + if (fd < 0) > + err(EXIT_FAILURE, "open(%s)", argv[i]); > + checkdir(fd); > + } > + return 0; > +} > > --=20 > 2.51.2 --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEARYKAB0WIQRV/neXydHjZma5XLJbRZGEIw/wogUCaRXbRgAKCRBbRZGEIw/w oqkyAP9c9n/B/mTnKscYaDZDo7o893NIKuhoFZGPZlwE4Y8DbQEA8tInnvL6cSFD EQ5MLtGiCuEj7OlbXGjMZSE2oH001A8= =X8dz -----END PGP SIGNATURE----- --=-=-=--