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 4DE2A89D3; Wed, 29 Oct 2025 12:02:09 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 993) id 00B398A25; Wed, 29 Oct 2025 12:02:07 +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-b2-smtp.messagingengine.com (fhigh-b2-smtp.messagingengine.com [202.12.124.153]) by atuin.qyliss.net (Postfix) with ESMTPS id F0E5489CC for ; Wed, 29 Oct 2025 12:02:05 +0000 (UTC) Received: from phl-compute-09.internal (phl-compute-09.internal [10.202.2.49]) by mailfhigh.stl.internal (Postfix) with ESMTP id B39E27A019B; Wed, 29 Oct 2025 08:02:04 -0400 (EDT) Received: from phl-mailfrontend-02 ([10.202.2.163]) by phl-compute-09.internal (MEProxy); Wed, 29 Oct 2025 08:02:04 -0400 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=1761739324; x=1761825724; bh=pXEF4OMo8b GnqFdBlC3jpZBJAlJLfzJvvw6icWnj/Bc=; b=en4JJBPTHA+tDm40Rt2/SxLRSn bHGARcSAsD/1+2S5XeIPFu5RoQ90N+QnySOx3OpAV6AqL3P+AxXrJvawr3FVEy/M OUwpxe/oDACRFTi5/rWXDH6qrjTtwB+T/Vf42s3yCukh+2qmr0u1JgjcYK1PzWBV HV1x65wOIfXlfdQ1fVRLghaucv6+Dv1x6EV9y0QBPAwzJGJLSi5/x+jLQDQ8CLZa ROvjZKXTyqCzlZXXiVBLWsJcXhxSaLneHoi2S3l4GyZa9tSv0mHR0Q/qnQ6jcswa A8KtTdbgUedw0WXnFzTMzg6DEVKm60/frXHqvejrNKQRhJI4ScUYJsUULg5A== 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= 1761739324; x=1761825724; bh=pXEF4OMo8bGnqFdBlC3jpZBJAlJLfzJvvw6 icWnj/Bc=; b=XRFvx6dexCmfIfTmoD8l/aQQ0eBEZGj8iXfazcEVmma9Xe4cqEg O1kyejT5+X/JQheBJ8QrQaqvytIrHsI8QHzGZxSyerpUSU2A7Hckf0JVEe8WRken +O+JeKKZlcXCifqeABpK6S8OAT622IQ//wbFNXorMkqwaiX4+LzAYcbbSyOK5C2+ wiHvnXV5WQOAwRW0+vAleJnYQNjNypeLLcIuVmsM4vZ/J/xHWOCPDZjYdIABSiWw q/iFCaSyJNGS0aEXi+YhhXZtpmR62csLMPfxu2fXW0o0l3XuPAnjVJlj4eYpLjw+ gr7NyvJuyHLpQwGJko72oa33Hqdw5DWhTHg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeffedrtdeggdduieefieejucetufdoteggodetrf 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, 29 Oct 2025 08:02:03 -0400 (EDT) Received: by fw12.qyliss.net (Postfix, from userid 1000) id B0D4B52147E; Wed, 29 Oct 2025 13:01:52 +0100 (CET) From: Alyssa Ross To: Demi Marie Obenour Subject: Re: [PATCH 3/7] tools: Add directory checker for updates In-Reply-To: <20251029-updates-v1-3-401c1be2a11b@gmail.com> References: <20251029-updates-v1-0-401c1be2a11b@gmail.com> <20251029-updates-v1-3-401c1be2a11b@gmail.com> Date: Wed, 29 Oct 2025 13:01:51 +0100 Message-ID: <87sef1kjbk.fsf@alyssa.is> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Message-ID-Hash: RKHE2FSHL4SHL3GH5NCO5XG5GA4Q6VRP X-Message-ID-Hash: RKHE2FSHL4SHL3GH5NCO5XG5GA4Q6VRP 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. > > Signed-off-by: Demi Marie Obenour > --- > host/rootfs/Makefile | 6 +- > lib/kcmdline-utils.mk | 6 ++ > tools/default.nix | 1 + > tools/meson.build | 1 + > tools/updates-dir-check/meson.build | 4 ++ > tools/updates-dir-check/updates-dir-check.c | 94 +++++++++++++++++++++++= ++++++ > 6 files changed, 110 insertions(+), 2 deletions(-) I still don't really understand why this needs to be a C program instead of find -H /path/to/dir -not -type f. None of the other checks seem very necessary? > diff --git a/host/rootfs/Makefile b/host/rootfs/Makefile > index 00d125774bb7b98736d0928c69cb307740cee034..15752286f5924291768f0655a= 12b90c702730520 100644 > --- a/host/rootfs/Makefile > +++ b/host/rootfs/Makefile > @@ -62,6 +62,9 @@ build/fifo: > build/empty: > mkdir -p $@ >=20=20 > +build/etc: > + mkdir -p $@ > + > # s6-rc-compile's input is a directory, but that doesn't play nice > # with Make, because it won't know to update if some file in the > # directory is changed, or a file is created or removed in a > @@ -69,8 +72,7 @@ build/empty: > # including files that aren't intended to be part of the input, like > # temporary editor files or .license files. So for all these reasons, > # only explicitly listed files are made available to s6-rc-compile. > -build/etc/s6-rc: $(S6_RC_FILES) file-list.mk > - mkdir -p $$(dirname $@) > +build/etc/s6-rc: $(S6_RC_FILES) file-list.mk build/etc > rm -rf $@ > set -uo pipefail && dir=3D$$(mktemp -d) && \ > { tar -c $(S6_RC_FILES) | tar -C $$dir -x --strip-components 3; } &= & \ > diff --git a/lib/kcmdline-utils.mk b/lib/kcmdline-utils.mk > new file mode 100644 > index 0000000000000000000000000000000000000000..fa228552e583f15fc77a74698= 5060ad5d04cdf2c > --- /dev/null > +++ b/lib/kcmdline-utils.mk > @@ -0,0 +1,6 @@ > +# SPDX-License-Identifier: EUPL-1.2+ > +# SPDX-FileCopyrightText: 2021-2024 Alyssa Ross > +READ_ROOTHASH =3D { \ > + set -eufo pipefail; \ > + read -r version < ../../version; \ > + LC_ALL=3DC expr "x$$version" : '^\(x0\|x[1-9][0-9]*\)\(\.\(0\|[1-9][0-9= ]*\)\)\{2\}$$' >/dev/null; } None of these changes seem to have anything to do with this patch. Did they end up in here by mistake? > diff --git a/tools/default.nix b/tools/default.nix > index ca165b5ec8ae1a63b75af4a34f33e320b262ba7b..e644f4e710e56f32de27ea100= 47cba3cffd0ecdf 100644 > --- a/tools/default.nix > +++ b/tools/default.nix > @@ -78,6 +78,7 @@ stdenv.mkDerivation (finalAttrs: { > ./start-vmm > ./subprojects > ./sd-notify-adapter > + ./updates-dir-check > ] ++ lib.optionals driverSupport [ > ./xdp-forwarder > ])); > diff --git a/tools/meson.build b/tools/meson.build > index 5d0ae81042fd3d77646594500f32cb1d48a6af0c..7da3bb451a5f1a797bc7e50a6= 7c44dbd37ba60bf 100644 > --- a/tools/meson.build > +++ b/tools/meson.build > @@ -28,6 +28,7 @@ if get_option('host') > subdir('lsvm') > subdir('start-vmm') > subdir('sd-notify-adapter') > + subdir('updates-dir-check') > endif >=20=20 > if get_option('app') > diff --git a/tools/updates-dir-check/meson.build b/tools/updates-dir-chec= k/meson.build > new file mode 100644 > index 0000000000000000000000000000000000000000..e19691d0e35f8a051e897990f= 0376384b3625c1a > --- /dev/null > +++ b/tools/updates-dir-check/meson.build > @@ -0,0 +1,4 @@ > +# SPDX-License-Identifier: EUPL-1.2+ > +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour > + > +executable('updates-dir-check', 'updates-dir-check.c', install: true, c_= args: ['-D_GNU_SOURCE=3D1', '-UNDEBUG', '-Wno-error=3Dpedantic']) How are -Werror=3Dpedantic and -DNDEBUG getting enabled in the first place? > diff --git a/tools/updates-dir-check/updates-dir-check.c b/tools/updates-= dir-check/updates-dir-check.c > new file mode 100644 > index 0000000000000000000000000000000000000000..94c7d54bec38d6efbd5b8aca2= 57f3ec4ad3fae35 > --- /dev/null > +++ b/tools/updates-dir-check/updates-dir-check.c > @@ -0,0 +1,94 @@ > +// SPDX-License-Identifier: EUPL-1.2+ > +// SPDX-FileCopyrightText: 2025 Demi Marie Obenour > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +#include > + > +static void checkdir(int fd) > +{ > + DIR *d =3D fdopendir(fd); > + if (d =3D=3D NULL) > + err(1, "fdopendir"); Usually we use EXIT_FAILURE. > + bool found_sha256sums =3D false; > + bool found_sha256sums_gpg =3D false; > + for (;;) { > + errno =3D 0; > + struct dirent *entry =3D readdir(d); > + if (entry =3D=3D NULL) { > + if (errno) > + err(1, "readdir"); > + break; > + } > + assert(entry->d_reclen > offsetof(struct dirent, d_name)); > + size_t len =3D strnlen(entry->d_name, entry->d_reclen - offsetof(struc= t dirent, d_name)); > + assert(len < entry->d_reclen - offsetof(struct dirent, d_name)); > + assert(len > 0); We do not need to second guess the kernel/libc. > + if (entry->d_name[0] =3D=3D '.') > + if (len =3D=3D 1 || (len =3D=3D 2 && entry->d_name[1] =3D=3D '.')) > + continue; > + if (strcmp(entry->d_name, "SHA256SUMS") =3D=3D 0) { > + found_sha256sums =3D true; > + continue; > + } > + if (strcmp(entry->d_name, "SHA256SUMS.gpg") =3D=3D 0) { > + found_sha256sums_gpg =3D true; > + 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(1, "Filename must begin with an ASCII letter"); > + 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(1, "Forbidden subsequent character in filename: '%c'", (int)c); > + else > + errx(1, "Forbidden subsequent character in filename: byte %d", (int= )c); > + } > + } Why do we care? Surely we don't expect systemd-sysupdate to put filenames unescaped into a shell or something. > + if (entry->d_name[len - 1] =3D=3D '.') > + errx(1, "Filename must not end with a '.'"); > + if (entry->d_type !=3D DT_REG) > + errx(1, "Entry contains non-regular file %s", entry->d_name); > + } > + if (!found_sha256sums) > + errx(1, "SHA256SUMS not found"); > + if (!found_sha256sums_gpg) > + errx(1, "SHA256SUMS.gpg not found"); > + closedir(d); > +} > + > +int main(int argc, char **argv) > +{ > + for (int i =3D 1; i < argc; ++i) { > + // Avoid symlink attacks. > + struct open_how how =3D { > + .flags =3D O_DIRECTORY|O_RDONLY|O_CLOEXEC|O_NOFOLLOW, > + .resolve =3D RESOLVE_NO_SYMLINKS|RESOLVE_NO_MAGICLINKS, > + }; For opening files given on the command line, wouldn't we want to use the mount's symlink behaviour? The VM presumably can't replace the root directory shared with it with a symlink. > + int fd =3D (int)syscall((long)SYS_openat2, (long)AT_FDCWD, (long)argv[= i], > + (long)&how, (long)sizeof(how)); > + if (fd < 0) > + err(1, "open(%s)", argv[i]); > + checkdir(fd); > + } > + return 0; > +} --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEARYKAB0WIQQGoGac7QfI+H5ZtFCZddwkt31pFQUCaQICLwAKCRCZddwkt31p FTF3APwJOO47HZuFZapRJeVFq2a2d/1VMGKUJZfsnV8dIr+eNwD9GYY2JRf+pIc3 XDZ/kZSCiIq3d75mMMBHHTZ99Zl5LAQ= =Zlp5 -----END PGP SIGNATURE----- --=-=-=--