Boot-time Key Intake Metadata

[eric_metricspace.net]

Summary

This patch adds a general mechanism for providing encryption keys to the kernel from the boot loader. This is intended to enable GELI support at boot time, providing a better mechanism for passing keys to the kernel than environment variables. It is designed to be extensible to other applications, and can easily handle multiple encrypted volumes with different keys.

This mechanism is currently used by the pending GELI EFI work. Additionally, this mechanism can potentially be used to interface with GRUB, opening up options for coreboot+GRUB configurations with completely encrypted disks.

Another slight benefit over the existing system is that it does not require re-hashing the password.

Test Plan

Testing for this is currently blocked by an apparent issue with GELI-enabled gptboot in master.

The kernel-side code has been successfully tested in the EFI GELI support effort. Only the gptboot-side code needs to be tested here.

Once this issue is resolved, the patch can be tested simply by successfully booting a GELI-on-root partition. This should also be tested with multiple GELI partitions with different passwords.

[eric_metricspace.net]

Added hook to clear GELI keys out of the intake buffer at mountroot.

I'm not clearing the entire buffer, because someone might want to use it for something else in the future.

[eric_metricspace.net]

sys/geom/eli/g_eli.c

130 ↗

(On Diff #25169)

Comment corrected in my local repo.

[eric_metricspace.net]

Moved MODINFOMD_KEYBUF definition to sys/linker.h

[oshogbo]

sys/boot/geli/geliboot.c

32 ↗

(On Diff #25170)

" " includes should be lower then <>.

47 ↗

(On Diff #25170)

for (i =0;

sys/boot/geli/geliboot.h

33 ↗

(On Diff #25170)

Missing blank space.

sys/crypto/intake.h

38 ↗

(On Diff #25170)

Tabs.

41 ↗

(On Diff #25170)

Wrong tab.

sys/geom/eli/g_eli.c

117 ↗

(On Diff #25170)

Style:
void*

118 ↗

(On Diff #25170)

Style.
{

memset()

}

1020 ↗

(On Diff #25170)

We don't assaign default values with declaration.

sys/opencrypto/crypto.c

193 ↗

(On Diff #25170)

Is this assign help in something?

203 ↗

(On Diff #25170)

Style:

caddr_t kmdp;

218 ↗

(On Diff #25170)

Style.

keybuf_t *
get_keybuf(void)
{

        return (keybuf);
}

sys/sys/linker.h

220 ↗

(On Diff #25170)

Whole this file is incorrect in case of style(9), so i would lave the style exacly how all file is:

#define<space>MODINFOMD_KEYBUF[tab]0x000d

[cem]

This diff lacks context. Please use arc or diff with -U999999.

The parse_cmd change appears entirely unrelated? We prefer to keep individual changes to their own smaller individual commits.

sys/boot/geli/geliboot.c

45 ↗

(On Diff #25228)

nsaved_keys is unsigned; this should be too.

53 ↗

(On Diff #25228)

What ensures the passed keybuf has enough room for nsaved_keys entries?

54 ↗

(On Diff #25228)

explicit_bzero

sys/boot/geli/geliboot.h

30 ↗

(On Diff #25228)

shouldn't this sort after sys/ headers?

sys/boot/i386/libi386/bootinfo32.c

239 ↗

(On Diff #25228)

explicit_bzero

sys/boot/i386/libi386/bootinfo64.c

255 ↗

(On Diff #25228)

explicit_bzero

sys/crypto/intake.h

38 ↗

(On Diff #25228)

NBBY instead of 8?

45 ↗

(On Diff #25228)

Why use typedef for new structs? Generally, a bare struct is preferred in new code.

Additionally, _t suffix is strongly discouraged for any non-standard and especially non-scalar type.

47 ↗

(On Diff #25228)

This uses a distinct constant from G_ELI_DATAIVKEYLEN and no validation is performed that they are compatible in geli_fill_keybuf.

sys/geom/eli/g_eli.c

134–135 ↗

(On Diff #25228)

The sizeof() seems mismatched.

1065 ↗

(On Diff #25228)

I prefer:

if (...ke_type != KEYBUF_TYPE_GELI)
    continue;

To avoid the extra level of indentation in this already heavily indented area of code. But this is fine.

1066 ↗

(On Diff #25228)

ke_data and key have different constant sizes. Either they need to share a constant or this needs better validation.

1078 ↗

(On Diff #25228)

Maybe:

if (found_intake)
    goto have_key;

To avoid all of this churn.

1172 ↗

(On Diff #25228)

have_key: would go here.

sys/opencrypto/crypto.c

192 ↗

(On Diff #25228)

why?

sys/sys/linker.h

146 ↗

(On Diff #25228)

These unrelated whitespace changes are gratuitous.

[eric_metricspace.net]

In D9575#199104, @cem wrote:

The parse_cmd change appears entirely unrelated? We prefer to keep individual changes to their own smaller individual commits.

One of the added includes defines a function named "parse". Thus, it was necessary to rename this function.

sys/boot/geli/geliboot.c

54 ↗

(On Diff #25228)

explicit_bzero isn't available in the boot-time libraries presently. If it's to be added, I would suggest that be done in a separate patch.

sys/boot/i386/libi386/bootinfo32.c

239 ↗

(On Diff #25228)

See above.

[cem]

In D9575#199157, @eric_metricspace.net wrote:

One of the added includes defines a function named "parse". Thus, it was necessary to rename this function.

I see, thanks.

sys/boot/geli/geliboot.c

54 ↗

(On Diff #25228)

Sure, that's fine.

[eric_metricspace.net]

sys/boot/geli/geliboot.c

53 ↗

(On Diff #25228)

save_key won't increment it past GELI_MAX_KEYS

sys/crypto/intake.h

47 ↗

(On Diff #25228)

Note that the check that MAX_KEY_BYTES is greater than G_ELI_DATAIVKEYLEN is performed in g_eli.h. This is due to complications with the BIOS GELI implementation that prevent including geom/eli/g_eli.h in this file.

sys/geom/eli/g_eli.c

1065 ↗

(On Diff #25228)

I'd rather leave it as is. The continue thing could trip someone up in the future.

1066 ↗

(On Diff #25228)

The key intake mechanism is designed to support different kinds of keys without a great deal of modification, so ke_data is bigger than a GELI key. It's designed with 4096-bit RSA keys in mind (ie. the largest commonly-used crypto keys). GELI keys are going to be probably 256-bits at most.

sys/opencrypto/crypto.c

192 ↗

(On Diff #25228)

If exposed as a symbol, data exfiltration attacks become utterly trivial. Of course, these attacks should be defeated by zeroing out the keys, but there's no sense in leaving potential attack vectors lying around.

sys/sys/linker.h

146 ↗

(On Diff #25228)

My emacs config auto-deletes trailing whitespace. I can delete them from the diff manually.

[eric_metricspace.net]

In D9575#199104, @cem wrote:

This diff lacks context. Please use arc or diff with -U999999.

Is there a command line that works with git diff? I've been unable to get any context in phabricator, despite the generated diffs having context information.

[cem]

In D9575#199799, @eric_metricspace.net wrote:

In D9575#199104, @cem wrote:

This diff lacks context. Please use arc or diff with -U999999.

Is there a command line that works with git diff? I've been unable to get any context in phabricator, despite the generated diffs having context information.

git diff -U999999? You may need --full-index, I'm not sure. I just use arc with git repositories, which works fine.

If that doesn't work in phabricator, maybe just strip the git header(s) off. Phabricator handles plain diff -U999999 fine.

sys/crypto/intake.h

47 ↗

(On Diff #25228)

The check in g_eli.h is slightly different. It is comparing against a G_ELI_DATAKEYLEN (no "IV").

sys/geom/eli/g_eli.c

1025 ↗

(On Diff #25355)

What is the distinction in constant sizes here?

1065 ↗

(On Diff #25228)

It shouldn't be a problem. It's a common pattern.

sys/opencrypto/crypto.c

192 ↗

(On Diff #25228)

If we aren't zeroing the key memory we have big problems. Leaving it as a public symbol makes that easier to validate and makes the code more straightforward. This is not a potential attack vector.

[emaste]

Is there a command line that works with git diff?

FWIW I use exactly git diff -U9999 > foo.diff or git show -U9999 <hash> > foo.diff

[eric_metricspace.net]

sys/geom/eli/g_eli.c

1025 ↗

(On Diff #25355)

Note: this isn't my code change.

USERKEYLEN is the maximum length of a password, which is hashed to produce the mkey. DATAIVKEYLEN is 64 bytes, I believe.

sys/opencrypto/crypto.c

192 ↗

(On Diff #25228)

I don't see any benefit to making it a public symbol that isn't realized by the existing access method. Anyway, it's moot; the API is already designed to provide access through get_keybuf and the code already uses it.

[allanjude]

sys/boot/geli/geliboot.c

230 ↗

(On Diff #25355)

This is wrong.

in sys/geom/eli/g_eli.c you expect the keybuf to contain the decryption key for the master key, not the already-decrypted master key.

You want to save_key(key) a few lines above, before the call to g_eli_mkey_decrypt()

I strongly think we should pass around the decryption key for the master key, rather than the master key itself. If it is somehow leaked, you can easily change the decryption key using geli rekey, but if the master key itself it leaked, then the entire volume must be destroyed and rewritten with a different master key.

[eric_metricspace.net]

sys/boot/geli/geliboot.c

230 ↗

(On Diff #25355)

This was a straight-up mistake. It is supposed to be the hashed password (the decryption key), not the decrypted master key.

[allanjude]

In D9575#199157, @eric_metricspace.net wrote:

In D9575#199104, @cem wrote:

The parse_cmd change appears entirely unrelated? We prefer to keep individual changes to their own smaller individual commits.

One of the added includes defines a function named "parse". Thus, it was necessary to rename this function.

Where is the other parse() coming from?

sys/boot/i386/libi386/bootinfo64.c

255 ↗

(On Diff #25228)

this was marked as done, but does not appear to be done

[allanjude]

sys/boot/geli/geliboot.c

230 ↗

(On Diff #25355)

now that you have context in the diff, it is easier to show you why this is still wrong.

You are saving the key on line 230, but it was bzero'd on like 218.

You should move save_key up to after like 217.

[eric_metricspace.net]

sys/boot/geli/geliboot.c

230 ↗

(On Diff #25355)

I've fixed in my local repo, but I won't post the patch here until I've been able to test it (or unless there's enough other fixes to warrant doing so).

sys/boot/i386/libi386/bootinfo64.c

255 ↗

(On Diff #25228)

explicit_bzero isn't available in this context. As discussed elsewhere, it ought to be handled separately.

[eric_metricspace.net]

Add changes by Allan Jude, adding support to boot2

[eric_metricspace.net]

Rebased after alanjude's patch adding explicit_bzero to boot loaders

[allanjude]

sys/boot/geli/geliboot.c

269 ↗

(On Diff #26854)

this is likely a merge conflict, but you are zeroing the key before we can call save_key.

I'll post an update with some other fixes shortly.

[eric_metricspace.net]

Integrate allanjude's pull request