Implement a stripped down version of GELI (AES-XTS and AES-CBC only) in gptboot and gptzfsboot

[allanjude]

Summary

Allows you to load /boot/loader and /boot/kernel from a GELI encrypted disk. Required to use ZFS boot environments with encrypted disks

[smh]

sys/boot/geli/geli.h

43 ↗

(On Diff #11352)

Yer it does doesn't it, common package?

sys/boot/geli/geli_hmac.c

29 ↗

(On Diff #11364)

cdefs.h is only needed for __FBSDID so as you've moved it to the header I don't think this is needed any more.

I'd be interested to know which method is accepted best practice for .h and .c files as the tree seems to be quite confused in that regard.

More of these below.

sys/boot/geli/geliimpl.c

180 ↗

(On Diff #11352)

Not sure I've only really started looking a loader stuff with the EFI ZFS loader in the last month, so still finding my way too I'm afraid.

[allanjude]

sys/boot/geli/geliimpl.c

250 ↗

(On Diff #11364)

yeah, I fixed almost every other instance in the file, but missed the one you pointed out...

sys/boot/i386/gptboot/gptboot.c

125 ↗

(On Diff #11364)

The issue is, as you can see from this malloc implementation, there is no free(), so an out of memory error can never be resolved. I think panic() is the only choice here.

[allanjude]

More feedback from smh@

[allanjude]

Cleanup cdefs

[allanjude]

Cleanup geliboot code duplication

• Remove copied version of rijndael, build libgeliboot.a with versions of sha256, sha512, and rijndael from sys/crypto

• Remove copied version of AES-XTS, use newly broken out version from opencrypto (see https://reviews.freebsd.org/D4674)

• Rename geliimpl.c to geliboot.c, and geli.h to geliboot.h

Depends on D4674

[allanjude]

Updated to reuse sys/geom/eli instead of bundling its own copy/pasted version

Depends on D4699

[allanjude]

Chase sha512 update

[allanjude]

Updated with some cleanups, less copy/pasted code, and general diff reduction

[delphij]

I'd like to request some changes, please see comment inline.

sys/boot/common/bootstrap.h

124 ↗

(On Diff #11748)

#ifdef LOADER_GELI_SUPPORT? (Ignore this if this is intended for a more generic context, but for now looks like it's GELI specific).

sys/boot/common/console.c

265 ↗

(On Diff #11748)

#ifdef LOADER_GELI_SUPPORT?

274 ↗

(On Diff #11748)

(not necessarily a problem: I don't quite follow here and would like to ask a clarification) -- when will we get 0 here? Do we need to filter out other control characters?

292 ↗

(On Diff #11748)

Actually, if a character is NOT accepted (s - pwstr == pwstrsize - 1), we shouldn't give an * as echo. This should along with the same *s++ = c in the same if {} block.

sys/boot/geli/geliboot.c

148 ↗

(On Diff #11748)

key material should always be zeroed as soon as they are no longer needed. (Note that the kernel geli does this)

(There are also some similar instances below).

186 ↗

(On Diff #11748)

Is this a legitimate case? (It won't hurt to have a catch-all error though).

192 ↗

(On Diff #11748)

(Not necessarily a real problem) -- we should probably re-visit this in the future to make it scale as this lookup is done in other loops, and that would result in O(n^2) complexity.

193 ↗

(On Diff #11748)

Can you add comments here to explain the matching logic, so it's easier to understand?

Also, instead of doing negative logics here, it's probably better to use straight tests instead:

if (geli_e->dsk->drive == dskp->drive) {

if (geli_e->dsk->slice == dskp->slice || 
    geli_e->dsk->part == dskp->part ||
    (dskp->part == 255 && geli_e->dsk->part == dskp->slice)) {
        return (0);
}

}

(Just an example; it's probably better to reorganize the code a little bit to make commenting it easier).

226 ↗

(On Diff #11748)

This is mostly the same code in is_geli. It makes sense to abstract this one out and make it its own function.

287 ↗

(On Diff #11748)

Not supporting keyfile as another factor is not secure because that means one key is used to encrypt multiple messages (master key) and is far from ideal! We should at least comment this as a TODO.

sys/boot/geli/geliboot.h

64 ↗

(On Diff #11748)

64-84: these are 'static' variables, shouldn't these be moved to the geliboot.c instead of appearing here?

sys/boot/geli/geliboot_crypto.c

2 ↗

(On Diff #11748)

This is derived from sys/geom/eli/g_eli_crypto.c and I think Pawel should be credited for the file too.

102 ↗

(On Diff #11748)

102-125: can we just reuse the code in sys/geom/eli/g_eli_crypto.c?

sys/boot/i386/common/cons.c

152 ↗

(On Diff #11748)

#ifdef LOADER_GELI_SUPPORT? Also see my comments in boot/common/.

sys/boot/i386/loader/Makefile

62 ↗

(On Diff #11748)

Can all GELI be replaced with LOADER_GELI_SUPPORT?

sys/boot/i386/loader/main.c

176 ↗

(On Diff #11748)

This MAY break those who do 'make installworld' without updating their boot sector and we need a way to tell version of zargs.

188 ↗

(On Diff #11748)

Similar.

sys/boot/i386/zfsboot/Makefile

39 ↗

(On Diff #11748)

Consistency: LOADER_GELI_SUPPORT?

sys/boot/zfs/libzfs.h

58 ↗

(On Diff #11748)

Why does gelipw belong to zfs_boot_args? :)

Also note that this changes the interface between zfsboot and zfsloader and since we don't previously have a version number embedded, we should have one. Otherwise new loader might not work with old boot block.

My recommendation is that we add a signature field and a version number field before this one.

[allanjude]

sys/boot/common/bootstrap.h

124 ↗

(On Diff #11748)

I instead decided to replace the two different getpwstr() functions with a copy of libstand's ngets() built into libgeliboot

sys/boot/common/console.c

274 ↗

(On Diff #11748)

this is a copy of getstr() from sys/boot/i386/common/cons.c with the echo changed to a *, so any problems are inherited.

292 ↗

(On Diff #11748)

This is actually a bug in the original code that I stole.

I have replaced this code with the ngets() from libstand which does indeed have the purchar inside that if block.

In a separate commit, the getstr() in sys/boot/i386/common/cons.c should be fixed or replaced.

sys/boot/geli/geliboot_crypto.c

2 ↗

(On Diff #11748)

You are correct

102 ↗

(On Diff #11748)

this file is specifically a substitute for g_eli_crypto.c because g_eli_crypto_cipher() uses openssl or the kernel crypto framework, neither of which are available at this point. I managed to reuse code from sys/geom/eli for everything else.

sys/boot/i386/common/cons.c

152 ↗

(On Diff #11748)

removed.

This is where getpwstr() originally came from, it is a copy of the getstr() above it in this same file, but with the final putchar() changed. The separate copy in sys/boot/common/console.c used by the loader (whereas this copy is used by {gpt,zfs,gptzfs}boot) uses a different method to access the console (a special getchar() implementation, vs xgetc())

sys/boot/i386/loader/Makefile

62 ↗

(On Diff #11748)

that makes sense

sys/boot/i386/loader/main.c

176 ↗

(On Diff #11748)

this should 'do the right thing'. If the system was booted using an older boot sector, zargs->size will be smaller than offsetof(... gelipw), and so this if statement will evaluate to false, and we never try to read the new struct member that does not exist.

this construct was not created by me, but rather reused from existing ZFS boot args code later in loader, see the references to zargs->primary_pool around line 275

sys/boot/i386/zfsboot/Makefile

39 ↗

(On Diff #11748)

yep

sys/boot/zfs/libzfs.h

58 ↗

(On Diff #11748)

I thought that was the point of the 'size' field at the top of this struct. When this struct is populated (see like 774 of zfsboot.c) the size is set to sizeof(struct zfs_boot_args). New fields are always added to the bottom of the struct, and only if zfs_boot_args.size > offsetof(struct zfs_boot_args, member), can we safely access the 'new' member.

I could be wrong, this might not be good enough.

[allanjude]

Address feedback from @delphi

[allanjude]

sys/boot/geli/geliboot.c

187 ↗

(On Diff #11750)

This happens if you call geli_attach() on a partition that has not been geli_taste()'d yet, or was tasted but was not found to contain any GELI

193 ↗

(On Diff #11750)

Yeah, I am quite new at this stuff, I just got it working ;)

194 ↗

(On Diff #11750)

yeah, this cleanup and de-duplication was badly needed.

227 ↗

(On Diff #11750)

done

288 ↗

(On Diff #11750)

yes, the big question to be able to support key files is, where to keep the key files. With this setup, you no longer have to have an unencrypted partition. I wonder how reliably you can access USB thumb drives at this point in the boot process...

[allanjude]

sys/boot/geli/geliboot.h

64 ↗

(On Diff #11748)

all that static silliness is from when I was trying to make this all fit in 64kb, before Colin rescued me from that hell. I've removed them.

[allanjude]

sys/boot/geli/geliboot.c

148 ↗

(On Diff #11748)

I think I got them all now, please check

sys/boot/zfs/libzfs.h

58 ↗

(On Diff #11748)

Maybe it makes sense to make a separate geli_boot_args to be used by both UFS and ZFS and try to send it as another parameter, although I am not sure how to 'find' it with a variable length zfs_boot_args infront of it (although maybe the size member solves that).

[allanjude]

Zero out key material as soon as done using it

address the last of the other feedback from @delphij

[allanjude]

Fix some bzero()s

Add a prototype for getpws()

Move geli_head from geliboot.h to geliboot.c

[allanjude]

Do not compile 'zfsboot' with GELI, as it is for MBR only, and needs to be only 64kb

Fix compile error re gelipw in boot args when LOADER_GELI_SUPPORT is not defined

[allanjude]

Rebase on latest head.

I think this is ready to go in now, I have addressed all of the outstanding feedback

[allanjude]

Update to sync with r296434

[delphij]

Please add an extra bzero (see my comment inline) and comment the fact that setenv() won't really reset the memory as a TODO.

Other than that I don't see any big problems and consider this as an "implicit" approval, ideally seek another committer to review as well.

sys/boot/i386/libi386/biosdisk.c

361 ↗

(On Diff #14124)

Why is this defined as static? Can we make it non-static and always bzero()? Being static may make it easier to find out its location...

416 ↗

(On Diff #14124)

Please add a /* TODO */ here: setenv() do not wipe memory and it needs to do it.

420 ↗

(On Diff #14124)

I would bzero() gelipw here unconditionally. This makes sure that we always conclude the bcopy() with a bzero() here.

[ed]

sys/boot/i386/common/bootargs.h

70 ↗

(On Diff #14124)

As we briefly discussed during your talk at AsiaBSDCon to make sure it won't get lost: Would it make sense to store the actual key instead of the passphrase here? I think we reached the following list of advantages:

• Less time spent on PKCS #5: This is currently done by all of the individual boot loader passes, but can be prevented if the key itself is passed on.
• As a result of the above, maybe it would even be possible to remove PKCS #5 code from some of the individual boot loaders?
• It would would make it possible to use crypto keys that are not derived from PKCS #5. For example, keys stored in TPMs, USB tokens, etc.
• Less risk of accidentally disclosing the passphrase length (timing attacks, addresses of neighbouring allocations, etc).

That said, I think you mentioned the passphrase is passed to the kernel as a kenv. Kenv isn't binary safe, right? That may be annoying.

[gnn]

It looks like most of the issues are addressed and this can go in. I've not run the code but I have read it over.