https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=297553
Bug ID: 297553
Summary: stand: zfs_stat() leaves st_dev and st_ino
uninitialised, breaking loader veriexec
Product: Base System
Version: 15.1-RELEASE
Hardware: Any
OS: Any
Status: New
Severity: Affects Some People
Priority: ---
Component: bin
Assignee: [email protected]
Reporter: [email protected]
Created attachment 273793
--> https://bugs.freebsd.org/bugzilla/attachment.cgi?id=273793&action=edit
tvo output: same manifest/files verified in userland, where fstat(2) supplies
st_dev and st_ino
I am a FreeBSD user, not a kernel developer. The observations are from my
machine and accurate; the code analysis was done with AI assistance and I
cannot
verify it myself. I am not able to run follow-up experiments beyond simple
commands.
## Summary
`stand`'s ZFS reader never assigns `st_dev` or `st_ino`. Both fields therefore
contain whatever was on the stack. `libsecureboot` relies on both — a
requirement that is documented in `stand/libsa/tftp.c`, and that `ufs.c` and
`pkgfs.c` honour, but the ZFS path does not.
Consequences when booting `loader_lua` built `WITH_LOADER_VERIEXEC` from a ZFS
pool:
1. The signed manifest is re-verified for nearly every file instead of once,
because the `(st_dev, st_ino)` cache never matches.
2. `fingerprint_info_lookup()` skips manifest entries whose stored `fi_dev`
does not match, so a file that *is* listed can be reported as having no
entry and is then accepted according to `accept_no_fp`.
3. Because the values are uninitialised, `is_verified()` can also match a
*different* file's cached entry and return its status. If that status is
`VE_VERIFIED`, `verify_prep()` returns it and `verify_file()` returns early
—
an unverified file is accepted. This is inferred from the code, not
observed.
Point 3 is why I am filing this as security-relevant rather than as a
performance issue.
## Observed
FreeBSD 15.1-RELEASE-p2 amd64. `loader_lua` built with
`WITH_LOADER_VERIEXEC=yes`, `WITH_BEARSSL=yes`, `VE_SIGNATURE_LIST=OPENPGP`.
`/boot` on a plain (non-GELI) ZFS pool on removable media; signed
`/boot/manifest` plus `/boot/manifest.asc`; root pool is GELI-encrypted.
A single boot prints the manifest verification **seven** times, once per
directory containing a verified file:
```
Verified /boot/kernel/../manifest signed by <key>
Verified /boot/keys/../manifest signed by <key> (x2)
Verified /boot/kernel/../manifest signed by <key> (x4 more)
```
That is one OpenPGP verification per verified file, i.e. `is_verified()` never
reports a hit for the manifest.
More importantly, the behaviour is not reproducible within one boot. A file
`/boot/loader.conf.d/zz-test.conf` that is listed in the manifest with a
deliberately wrong hash is
* **accepted** while `config.load()` reads `loader.conf.d/` early in the boot
(its settings take effect, verified via `kenv` and via the loader honouring
`autoboot_delay`), but
* **rejected** later in the same boot when the identical path is opened from
the
loader prompt:
```
OK include /boot/loader.conf.d/zz-test.conf
Unverified /boot/loader.conf.d/zz-test.conf: <real hash> != 0000...0000
```
The difference correlates with how many manifest copies have been added to the
fingerprint list by then, which is consistent with an uninitialised value that
happens to be zero in some of them (`fi_dev != 0` is the "don't care" case in
the lookup).
For comparison, the same manifest and the same files evaluate correctly in
userland with the shipped test harness (`lib/libsecureboot/tests/tvo`, built
`-DUNIT_TEST`), where `fstat(2)` supplies real values: the manifest is verified
once, and the wrong-hash file yields `verify_file(...) = -3`. See attachment.
## Analysis
`stand/libsa/zfs/zfs.c`:
```c
static int
zfs_stat(struct open_file *f, struct stat *sb)
{
struct devdesc *dev = f->f_devdata;
const spa_t *spa = ((struct zfsmount *)dev->d_opendata)->spa;
struct file *fp = (struct file *)f->f_fsdata;
return (zfs_dnode_stat(spa, &fp->f_dnode, sb));
}
```
`zfs_dnode_stat()` in `stand/libsa/zfs/zfsimpl.c` sets `st_mode`, `st_uid`,
`st_gid` and `st_size` — and nothing else. `st_dev` and `st_ino` do not occur
anywhere under `stand/libsa/zfs/`.
Other readers set them deliberately:
```c
stand/libsa/tftp.c:684 /* libsecureboot needs st_dev and st_ino at minimum;
*/
stand/libsa/tftp.c:688 sb->st_dev = (dev_t)tftpfile->iodesc->destip.s_addr;
stand/libsa/ufs.c:869 sb->st_dev = (dev_t)(fp->f_fs->fs_id[0] ^
fp->f_fs->fs_id[1]);
stand/libsa/pkgfs.c:419 sb->st_dev = (off_t)((uintptr_t)tf->tf_pkg);
```
The two consumers in `libsecureboot`:
```c
lib/libsecureboot/veopen.c fingerprint_info_add(): nfip->fi_dev =
stp->st_dev;
lib/libsecureboot/veopen.c fingerprint_info_lookup():
if (fip->fi_dev != 0 && fip->fi_dev != dev)
continue; /* "skipping dev" */
lib/libsecureboot/verify_file.c is_verified():
if (stp->st_ino > 0) {
for (vsp = verified_files; ...) {
if (stp->st_dev == vsp->vs_dev &&
stp->st_ino == vsp->vs_ino) {
rc = vsp->vs_status; /* trusted verbatim */
```
and `verify_prep()` returns that status, upon which `verify_file()` returns
early without hashing the file.
Note that `fingerprint_info_add()` already zeroes `fi_dev` under `UNIT_TEST`,
which is presumably why the defect is invisible to the test harness.
## Reproduce (expected)
1. Build `loader_lua` with `WITH_LOADER_VERIEXEC=yes` and a trust anchor.
2. Place `/boot` and a signed `/boot/manifest` on a ZFS pool, boot from it.
3. Count the `Verified .../manifest signed by ...` lines — expected once,
observed once per verified file.
4. Add a file under `/boot/loader.conf.d/` that is either absent from the
manifest or listed with a wrong hash, and observe that its settings are
honoured during `config.load()`.
## Workaround
None that keeps the check intact. Forcing `fi_dev = 0` in
`fingerprint_info_add()` makes lookups deterministic again, but that is the
documented "don't care" value and disables the binding between a manifest and
the device it was found on.
## Suggested fix
Assign both fields in `zfs_stat()`, analogous to `ufs.c`. `struct zfsmount`
(`zfsimpl.c`) already carries `spa` and `rootobj`; the object number has to be
recorded at lookup time, as `dnode_phys_t` does not contain it.
`stand/libsa/zfs/zfs.c`:
```diff
struct file {
off_t f_seekp; /* seek pointer */
dnode_phys_t f_dnode;
+ uint64_t f_objnum; /* object number of f_dnode */
uint64_t f_zap_type; /* zap type for readdir */
```
```diff
- rc = zfs_lookup(mount, upath, &fp->f_dnode);
+ rc = zfs_lookup(mount, upath, &fp->f_dnode, &fp->f_objnum);
```
```diff
static int
zfs_stat(struct open_file *f, struct stat *sb)
{
struct devdesc *dev = f->f_devdata;
- const spa_t *spa = ((struct zfsmount *)dev->d_opendata)->spa;
+ struct zfsmount *mount = dev->d_opendata;
+ const spa_t *spa = mount->spa;
struct file *fp = (struct file *)f->f_fsdata;
+ int rc;
- return (zfs_dnode_stat(spa, &fp->f_dnode, sb));
+ rc = zfs_dnode_stat(spa, &fp->f_dnode, sb);
+ if (rc == 0) {
+ /*
+ * libsecureboot needs st_dev and st_ino at minimum,
+ * cf. libsa/tftp.c and libsa/ufs.c.
+ */
+ sb->st_dev = (dev_t)(spa->spa_guid ^ mount->rootobj);
+ sb->st_ino = (ino_t)fp->f_objnum;
+ }
+ return (rc);
}
```
`stand/libsa/zfs/zfsimpl.c` (`zfs.c` includes `zfsimpl.c`, so there is no
header to change; a forward declaration may need the same treatment):
```diff
-zfs_lookup(const struct zfsmount *mount, const char *upath, dnode_phys_t
*dnode)
+zfs_lookup(const struct zfsmount *mount, const char *upath,
+ dnode_phys_t *dnode, uint64_t *objnump)
```
```diff
+ if (objnump != NULL)
+ *objnump = objnum;
*dnode = dn;
done:
```
`spa_guid ^ rootobj` is stable per pool and dataset, which matches what the
device check is meant to express (manifest and file on the same dataset). The
ZFS object number is unique within the dataset and greater than zero for
regular files, which `is_verified()` requires.
## Side notes
Two further issues in the same area, found while getting this configuration to
build. I can file them separately if that is preferred.
1. `lib/libsecureboot/Makefile.inc`: the `ta.h` rule (line 153) uses
`${VE_HASH_KAT_STR_INPUT}` unconditionally, but that variable is only
defined
inside `.if ${VE_SELF_TESTS} != "no"` (lines 106–116). Building with
`VE_SELF_TESTS=no` therefore fails with `"|" unexpected`.
2. `lib/libsecureboot/local.trust.mk`: `XCFLAGS.opgp_key+= -DHAVE_TA_ASC_H`
(line 69) is only set in the `.if exists(${SIGNER})` branch. In the
serverless branch (lines 81–108), which is the one used when trust anchors
are provided as local `t*.asc` files, the macro is never defined. Since
`opgp_key.c` guards `#include <ta_asc.h>` with it, the resulting loader
contains the OpenPGP code but no trust anchor, and fails at boot with
`cannot find key-id ...` — with no indication at build time.
--
You are receiving this mail because:
You are the assignee for the bug.
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.