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 EC93A1D419; Wed, 19 Nov 2025 14:55:28 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 993) id 0679A1D37A; Wed, 19 Nov 2025 14:55:26 +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 fout-a1-smtp.messagingengine.com (fout-a1-smtp.messagingengine.com [103.168.172.144]) by atuin.qyliss.net (Postfix) with ESMTPS id 654A21D403 for ; Wed, 19 Nov 2025 14:55:24 +0000 (UTC) Received: from phl-compute-11.internal (phl-compute-11.internal [10.202.2.51]) by mailfout.phl.internal (Postfix) with ESMTP id 94B12EC02D0; Wed, 19 Nov 2025 09:55:22 -0500 (EST) Received: from phl-mailfrontend-01 ([10.202.2.162]) by phl-compute-11.internal (MEProxy); Wed, 19 Nov 2025 09:55:22 -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=1763564122; x=1763650522; bh=+TaA8w4EqK atUv64u4QBiegPe0teMDBgSpPRs+N5EpA=; b=X5S7SDGyWKa4Sa1G0lEFHFUT8J Rz2evCiV6ItxAAdSxI0Yj+BogCTsX2hVWcH6Gdt9uuzCwAmZGsy01upaNGipMEKM 0xurMX333gkJaVZxeEbwZo0AxxCGgfnFzAeYENskfDxen1CxVxiYlZVT8ReFJJN2 cQ22ivI1KLty0K1lbhM1zCZbPpAWuNTEnTHHuh0EXlB7xiCfK4VwJN0qOChKB6Ie Awr033BTL4GeMJ0LII7jPS1MZ9yNvjD/fxn7vj0nc8agejxcKebgv5haO6M0Va3e /WBIksFyfErHYL8U4KTgj+axGakwMFzHR0qQKKo8Q36I5J9rgp4KjLSoljHA== 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= 1763564122; x=1763650522; bh=+TaA8w4EqKatUv64u4QBiegPe0teMDBgSpP Rs+N5EpA=; b=iA8hw5z+tQzalwy5bZgOGTqKGOjzhqqq3RJWQBHWXWYPN1y/cl+ 90Tp0tS8DMLvWpLQSvlToFfkUtpWrhjkTBZWxq1qkYzrQUmcfsY/NzjVQIfdeunj qSAzRrHbUDZ9w7nipyaTxUSNJ8ev/8ffFhwLk2BMl1/3qd1YF/CGx7Ghec1ylHbQ s7xcOPqwED4Vbmd2o0lSAybyvQAYzDNuDfO6k8EjkjpJI6KkIKc2ohgCBwmNjTFF CFz79g11+2nnUEOPyabYcw0bosbgUrCiLDApiEcEmAHGkyFOnHZ8epqAz7w91EcW kKEMtLMfK36N5TJOtVlISKMSXz8nbqS0RiA== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeffedrtdeggddvvdeghedtucetufdoteggodetrf dotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfurfetoffkrfgpnffqhgenuceu rghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujf gurhephffvvefujghffffkgggtsehgtderredttddtnecuhfhrohhmpeetlhihshhsrgcu tfhoshhsuceohhhisegrlhihshhsrgdrihhsqeenucggtffrrghtthgvrhhnpeeiudffue eilefgtefgtddttdekkeehkefgheekudefveetgeefiefftedvteeuveenucevlhhushht vghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehhihesrghlhihsshgrrd hishdpnhgspghrtghpthhtohepvddpmhhouggvpehsmhhtphhouhhtpdhrtghpthhtohep uggvmhhiohgsvghnohhurhesghhmrghilhdrtghomhdprhgtphhtthhopeguvghvvghlse hsphgvtghtrhhumhdqohhsrdhorhhg X-ME-Proxy: Feedback-ID: i12284293:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 19 Nov 2025 09:55:19 -0500 (EST) Received: by fw12.qyliss.net (Postfix, from userid 1000) id 8065A22884FB; Wed, 19 Nov 2025 15:45:17 +0100 (CET) From: Alyssa Ross To: Demi Marie Obenour Subject: Re: [PATCH v3 03/14] tools: Add directory checker for updates In-Reply-To: <20251119-updates-v3-3-b88a99915509@gmail.com> References: <20251119-updates-v3-0-b88a99915509@gmail.com> <20251119-updates-v3-3-b88a99915509@gmail.com> Date: Wed, 19 Nov 2025 15:45:16 +0100 Message-ID: <874iqqqdxf.fsf@alyssa.is> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Message-ID-Hash: T4IF7THCNTBO24V6PAEFWVTZP335UQOX X-Message-ID-Hash: T4IF7THCNTBO24V6PAEFWVTZP335UQOX 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 Just one "containing" is fine. :P > 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. systemd-sysupdate can leave > behind temporary files with names starting with '.', so delete them > instead of failing. Linux can lose cache coherency if there is an I/O > error, so call syncfs() on the directory before checking anything. For > the same reason, fsync() the directory if any hidden files were deleted. > > The directory checker also serves another critical function: it checks > if the VM actually downloaded anything. Otherwise, network problems > could cause updates to silently do nothing. Specifically, it checks > that the VM provided a file starting with the prefix "SHA256SUMS.". > These will be the last ones the in-VM updater downloads. An additional > mode is provided to clean out all such files. This will be used to > ensure that before the in-VM updater runs, no such files are present. > Hence, if the VM didn't actually download anything, the user will get a > clear error instead of a false success message or a confusing error. > > Signed-off-by: Demi Marie Obenour > --- > Changes since v2: > > - Purge leftover temporary files rather than returning an error. > > - Split into two modes: one that deletes signature files, and one that > checks that at least one signature file exists. This allows checking > that the VM actually sent something. > --- > tools/default.nix | 1 + > tools/meson.build | 4 ++ > tools/updates-dir-check.c | 133 ++++++++++++++++++++++++++++++++++++++++= ++++++ > 3 files changed, 138 insertions(+) Looking good. Left some style comments, but Reviewed-by: Alyssa Ross > diff --git a/tools/updates-dir-check.c b/tools/updates-dir-check.c > new file mode 100644 > index 0000000000000000000000000000000000000000..07eb059f2718e1ad8ab087fe6= 509c1437ea3e96c > --- /dev/null > +++ b/tools/updates-dir-check.c > @@ -0,0 +1,133 @@ > +// SPDX-License-Identifier: EUPL-1.2+ > +// SPDX-FileCopyrightText: 2025 Demi Marie Obenour > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > + > +#include > + > +[[noreturn]] static void bad_char(char c, char *msg_component) > +{ > + if (c >=3D 0x20 && c <=3D 0x7E) > + errx(EXIT_FAILURE, "Forbidden %s character in filename: '%c'", > + msg_component, (int)c); > + errx(EXIT_FAILURE, > + "Forbidden %s character in filename: byte %d", > + msg_component, (int)(unsigned char)c); Why not %hhu, so you don't need two layers of casts? > +} > + > +[[noreturn]] static void usage(void) > +{ > + errx(EXIT_FAILURE, "Usage: updates-dir-check [cleanup|check] DIRECTORIE= S..."); > +} > + > +static void checkdir(int fd, bool check_sig) [[gnu::fd_arg_read (1)]] (I'm bad at remembering this too so you'll see other code missing it, but it's good to add.) > +{ > + bool found_sig =3D false; > + 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"); > + bool changed =3D false; > + for (;;) { > + errno =3D 0; > + struct dirent *entry =3D readdir(d); > + if (entry =3D=3D NULL) { > + if (errno) > + err(EXIT_FAILURE, "readdir"); > + break; > + } > + const char *ptr =3D entry->d_name; > + if (ptr[0] =3D=3D '.') { > + if (ptr[1] =3D=3D '\0') > + continue; > + if (ptr[1] =3D=3D '.' && ptr[2] =3D=3D '\0') > + continue; > + // systemd-sysupdate uses these for temporary files. > + // It normally cleans them up itself, but if there is an error > + // it does not always clean them up. I'm not sure if it is > + // guaranteed to clean up temporary files from a past run, so > + // delete them instead of returning an error. > + if (unlinkat(fd, ptr, 0)) > + err(EXIT_FAILURE, "Failed to unlink temporary file"); > + changed =3D true; > + continue; > + } > + char c =3D ptr[0]; > + if (!((c >=3D 'A' && c <=3D 'Z') || > + (c >=3D 'a' && c <=3D 'z'))) > + bad_char(c, "initial"); > + while ((c =3D *++ptr)) { > + 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 '.'))) > + bad_char(c, "subsequent"); > + } > + // Empty filenames are rejected as having a bad initial character, > + // and POSIX forbids them from being returned anyway. Therefore, > + // this cannot be out of bounds. > + if (ptr[-1] =3D=3D '.') > + errx(EXIT_FAILURE, "Filename %s ends with a '.'", entry->d_name); > + if (entry->d_type =3D=3D DT_UNKNOWN) > + errx(EXIT_FAILURE, "Filesystem didn't report type of file %s", entry-= >d_name); > + if (entry->d_type !=3D DT_REG) > + errx(EXIT_FAILURE, "Entry contains non-regular file %s", entry->d_nam= e); > + if (strncmp(entry->d_name, "SHA256SUMS.", sizeof("SHA256SUMS.") - 1) = =3D=3D 0) { > + // Found a signature file! This comment seems a bit redundant. > + if (check_sig) > + found_sig =3D true; > + else { > + if (unlinkat(fd, entry->d_name, 0)) > + err(EXIT_FAILURE, "Unlinking old signature file"); > + changed =3D true; > + } > + } > + } > + // fsync() the directory if it was changed, to avoid the above > + // cache-incoherency problem. Above where? > + if (changed && fsync(fd)) > + errx(EXIT_FAILURE, "fsync"); > + if (check_sig && !found_sig) { > + warnx("sys.appvm-systemd-sysupdate didn't send a signature file."); > + warnx("There was probably a problem downloading the update."); > + errx(EXIT_FAILURE, "Check its logs for more information."); > + } > + closedir(d); > +} > + > +int main(int argc, char **argv) > +{ > + if (argc !=3D 3) > + usage(); > + > + bool check_sig; > + if (strcmp(argv[1], "cleanup") =3D=3D 0) > + check_sig =3D false; > + else if (strcmp(argv[1], "check") =3D=3D 0) > + check_sig =3D true; > + else > + usage(); > + > + for (int i =3D 2; i < argc; ++i) { > + int fd =3D open(argv[i], O_DIRECTORY|O_RDONLY|O_CLOEXEC); > + if (fd < 0) > + err(EXIT_FAILURE, "open(%s)", argv[i]); Maybe we could just fdopen(argv[1]) inside checkdir()? We don't need any special flags AFAICT. > + checkdir(fd, check_sig); > + } > + return 0; > +} > > --=20 > 2.52.0 --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEARYKAB0WIQQGoGac7QfI+H5ZtFCZddwkt31pFQUCaR3X/AAKCRCZddwkt31p FeHPAP47svBmJZYits4x8N/WloeIn6AZHG2hsXb1Qirde8kUKgEA6lSYXEV4wb+L W4jDnsTs43ka/wML2AfzyhzQ4xfmsAk= =uthB -----END PGP SIGNATURE----- --=-=-=--