Re: Re: bin/58212: cgdconfig(8): Add zfs verification method

[Malte Dehling <mdehling%gmail.com@localhost>]

The following reply was made to PR bin/58212; it has been noted by GNATS.

From: Malte Dehling <mdehling%gmail.com@localhost>
To: Taylor R Campbell <riastradh%netbsd.org@localhost>
Cc: gnats-bugs%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost
Subject: Re: Re: bin/58212: cgdconfig(8): Add zfs verification method
Date: Sun, 28 Apr 2024 16:07:27 -0700

 --00000000000069a3400617303387
 Content-Type: text/plain; charset="UTF-8"
 
 Hi Taylor,
 
 On Sun, Apr 28, 2024 at 08:38:55PM +0000, Taylor R Campbell wrote:
 > Cool, thanks!  We should definitely have a zfs verification method for
 > cgdconfig(8).  But I think we can do it a little more robustly -- and
 > with fewer dependencies outside libc in cgdconfig(8).
 >
 > Here's what I wrote the last time someone suggested this -- identical
 > to the nvlist approach you adapted from fstyp(8) -- but my round tuit
 > supply ran short of taking the approach I suggested of verifying a
 > fixed magic number and a SHA-256 hash over the header:
 >
 > I wonder if we can do this without pulling so much into cgdconfig and
 > doing parsing there, though?
 >
 > A comment in fstyp(8) suggests there's a checksum (which it doesn't
 > verify); maybe we can use that?
 >
 > Would be nice if there were a reference to the data format that we can
 > put in a comment -- which should appear in fstyp(8) too.
 >
 > Would also be nice if we could quantify the false acceptance
 > probability under uniform random data (i.e., wrong password, modelling
 > password hash as uniform random function).  For the current
 > verification methods it looks like:
 >
 > verify_ffs      = 1 - (1 - 4/2^32)^4 < one in 250 * 10^6
 > verify_gpt      < 1 - (1 - 1/2^96)^4 < one in 10^28
 > verify_mbr      = 1/2^16 (much too high for my comfort!)
 >
 > For zfs I would hope it can be much lower than for ffs.
 >
 > I went hunting through the zfs header files and it looks like the vdev
 > label has a magic number and some kind of 256-bit checksum -- not
 > specific to the vdev label (other zpool data structures have the same
 > magic number and checksum), but maybe we can use it:
 >
 > /* vdev_impl.h */
 > typedef struct vdev_phys {
 >         char            vp_nvlist[VDEV_PHYS_SIZE - sizeof (zio_eck_t)];
 >         zio_eck_t       vp_zbt;
 > } vdev_phys_t;
 >
 > typedef struct vdev_label {
 >         char            vl_pad1[VDEV_PAD_SIZE];                 /*  8K */
 >         char            vl_pad2[VDEV_PAD_SIZE];                 /*  8K */
 >         vdev_phys_t     vl_vdev_phys;                           /* 112K */
 >         char            vl_uberblock[VDEV_UBERBLOCK_RING];      /* 128K */
 > } vdev_label_t;                                                 /* 256K total */
 >
 > /* zio.h */
 > #define ZEC_MAGIC       0x210da7ab10c7a11ULL
 >
 > typedef struct zio_eck {
 >         uint64_t        zec_magic;      /* for validation, endianness   */
 >         zio_cksum_t     zec_cksum;      /* 256-bit checksum             */
 > } zio_eck_t;
 >
 > Judging by zio_checksum.c, it looks like it's SHA-256.
 >
 > I grepped for some of the nvlist keys that zfs uses, and found
 > libzfs_import.c, which does a couple things:
 >
 > - Tries not just offset 0, but up to four different offsets with the
 >   label_offset function.
 >
 > - Checks for "state" and "txg" in the nvlist.
 >
 > vdev_label_read_config also tries several different offsets, but only
 > checks for "txg" -- and doesn't even require that.  So the nvlist keys
 > may be a red herring here.
 >
 > But with the checksum, it looks like we could get by with:
 >
 > 1. (optional) for each possible label offset:
 >
 > 2. verify vl->vl_vdev_phys.vp_zbt.zec_magic == ZEC_MAGIC or
 >    byte-swapped, and
 >
 > 3. verify vl->vl_vdev_phys.vp_zbt.zec_cksum is the SHA-256 hash of
 >    whatever part of vl it's computed over.
 >
 > The SHA-256 hash appears to be calculated over vl->vl_vdev_phys, but
 > with vp_zbt.zec_cksum set to
 >
 >    {labeloffset + offsetof(struct vdev_label, vl_vdev_phys), 0, 0, 0},
 >
 > with the 64-bit words possibly encoded in the same byte order as
 > zec_magic.  Except then the bytes of each 64-bit word in the SHA-256
 > output hash are byte-swapped.
 >
 > This seems to check out on the one sample I tried, and it should have
 > a false detection probability under uniform random data well below
 > 1/2^256 which is extremely comfortable.  (Should also have low false
 > detection probability under other formats too because of the checksum
 > even if you store a file with the magic number in it.)
 
 Thanks for the detailed response!
 
 I did the same reading to try and find out if there is a simple checksum
 verification function but didn't find anything.  It seems like zpool
 import never actually verifies the checksums or magic bytes in
 vl->vl_vdev_phys->vp_zbt .  There are VDEV_LABELS (4) labels, two at the
 beginning of the disk (offset 0 and 256k) and two at the end (at -512k,
 -256k) and even with all of their vl->vl_vdev_phys->vp_zbt structures
 zeroed out, zpool import shows no complaints.  The checksums are updated
 on export.
 
 I did a quick test and computed sha256 for vl->vdev_phys and it works as
 you described: the magic bytes need to be set first (byte-swapped on my
 system) and the offset encoded in the first of the 64bit checksum words
 (also byte-swapped).  The computed sha256 checksum is then stored as 4
 64bit words in byte-swapped order.
 
 Implementing the checksum verification is easy and allows us to get rid
 of the libnvpair dependency, so I don't mind implementing it that way.
 Since I haven't looked into exactly how much checking libnvpair does, I
 don't know what the risk of false positives for the current proposal is.
 
 Cheers,
 -- 
 Malte Dehling
 
 --00000000000069a3400617303387
 Content-Type: application/pgp-signature; name="signature.asc"
 Content-Disposition: attachment; filename="signature.asc"
 Content-Transfer-Encoding: base64
 X-Attachment-Id: 7279cfb2c4807360_0.1
 
 LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRSXpCQUFCQ0FBZEZpRUVva3ZsOUFOMEFo
 T05jQXdlSzhMazZxS3dkMFFGQW1ZdTFxY0FDZ2tRSzhMazZxS3cKZDBUTi9nLy9Va3lYdGZqc2pF
 S0p2OGFRQ3VETHIrWHFYRVNnaEhkZHo2Rk9OdFQxaE9Wbnl0RVkvUWFOWWNMNgpoc2JEdDAvMmND
 S1JpTFFTL1lmVExtdmRkMm43S1E5V3VHbGtGcXBjZXFyZXR3RHJOTXFTN3RNcGI5eXE2VTR3Ck1D
 T1ZEZDlMTmdONllwdjF1cWZ5S3RyTzRXTERRLzJiMUZTN0F4N0VDb2ZLUGphajNVemtUUlJmU3FZ
 TkZxVEIKUzBVbXVsN0pnU25lRnhGZnRJVG9sZUQxUWZYSFRFSGpCTlN4YkVEak00cmpHbXVuMU5n
 QlJqNjRmaS9WZVZxcQpWYURjOCt5Ly9pOFUxcE9EUjZTVjkwMDQyblEreHN3TzB5Q1pwZzlXT3Y5
 QXNST1hKbTJoNTFHNmxIS0tNM25DCnR0K3p0L294UHFQY2hHZTl5dXhra05oZkdWZlJUWFl2aUJZ
 Q1ptVkxvUWl4OExDU2l6UzNMTHpjMm9rcHdLK2cKSFlmd0dqRENRK20wNXhOUEZGNWcvSTdwd0R4
 ZU8xVGdWL0Z3b1IzMHhCZUQzMEkxaVlLcDRPZjFWOWorZGNDSApNQjdwbm45ZmFzbzM4QllmVG1L
 OUNXOEZWWDNxQWJCZFhGNFoxcmlyU2gvME5yV0ViSDBVaGRISldPRFFCSFcxClJJajJvS3ZLRFV5
 N2pqT09oUFF4N3RhSUU0WWxSMVRmS1g4dHhKTXVydnY1TVZvNmpveEZPdmRjaGpYaytnVG4KRlFm
 QUhnZ3poQXZEdS95VmhubTgydzd1amNlRmVHbUxjdm5xcWVzMkxrWVFBbDEzYVhiY0JQWVRZUE1t
 V1B3QwpoQ28zeHJVQlNyU1FUVzdwWVZhcVBnRysybXAycXlxdzk5d1I3WHJ2N1Q3SVBPV1NDUzg9
 Cj1YZ092Ci0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo=
 --00000000000069a3400617303387--