Page MenuHomeFreeBSD

Encrypted kernel crash dumps.
ClosedPublic

Authored by def on Dec 26 2015, 2:43 PM.

Details

Summary

This is a review of the encrypted kernel crash dumps feature. You can read more about this project on the freebsd-security mailing list: https://lists.freebsd.org/pipermail/freebsd-security/2015-December/008780.html .

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jhb added inline comments.Dec 28 2015, 4:59 PM
sys/dev/null/null.c
119 ↗(On Diff #11701)

This is here so that dumps can be disabled by "setting" them on /dev/null. (That is, there isn't a separate ioctl request to disable dumps, instead /dev/null is (ab)used as a cookie to disable them.) I think a FALLTHROUGH as you suggested is fine, but you can also do it even simpler as:

#ifdef COMPAT_FREEBSD10
    case DIOCSKERNELDUMPOLD:
#endif
    case DIOCSKERNELDUMP:
sys/geom/geom_dev.c
494 ↗(On Diff #11701)

I know that we use 'OLD' suffixes for other ioctls, but it might be better practice going forward to use a '_10' suffix instead to match COMPAT_FREEBSD10. This removes the need for "OLDOLD" if we change the structure in the future.

pjd added inline comments.Dec 29 2015, 12:39 AM
sbin/dumpon/dumpon.8
235 ↗(On Diff #11701)

I like that idea.

sbin/savecore/savecore.c
708 ↗(On Diff #11701)

You would need:
isencrypted = (dumpkeysize > 0);

sys/geom/geom_dev.c
494 ↗(On Diff #11701)

Are there existing examples of _<version> suffix for old ioctls? If not, I'd suggest _FREEBSD<version> suffix. It is more consistent with COMPAT_FREEBSD10 and more obvious than just _10.

sys/sys/conf.h
321 ↗(On Diff #11701)

This is just in-memory structure.

pjd added inline comments.Dec 29 2015, 12:52 AM
sbin/decryptcore/decryptcore.c
193 ↗(On Diff #11701)

Why do you want to increase TCB? Even if the code doesn't introduce any risk now, it may be modified in the future. I'd need to hear a convincing argument to agree on this one:)

317 ↗(On Diff #11701)

I'm fine with this suggestion, but I'd stick to regular getopt(3), which should be more than enough here.

sbin/dumpon/Makefile
5 ↗(On Diff #11701)

Yes, this needs to be fixed. Konrad, take a look if you can separate crypto code, so dumpon can still be compiled without EKCD support.

sys/dev/null/null.c
119 ↗(On Diff #11701)

I like this one the most.

jhb added inline comments.Dec 29 2015, 7:48 PM
sys/geom/geom_dev.c
494 ↗(On Diff #11701)

No. I found a mixture of FOO_OLD, OLD_FOO, and one OFOO in some simple greps. I think _FREEBSD10 is fine.

cem added a subscriber: cem.Feb 5 2016, 6:36 PM
def updated this revision to Diff 14731.Mar 30 2016, 2:19 AM
def edited edge metadata.
def marked 36 inline comments as done.Mar 30 2016, 2:32 AM
def added inline comments.
sbin/decryptcore/decryptcore.c
188 ↗(On Diff #11701)

It's assumed that disabling Capsicum doesn't disable a functionality which uses Capsicum.

193 ↗(On Diff #11701)

As Paweł said we don't want to perform any unnecessary operation before entering capability mode.

sbin/dumpon/Makefile
5 ↗(On Diff #11701)

dumpon(8) should work without OpenSSL now.

sbin/dumpon/dumpon.8
203 ↗(On Diff #11701)

Good point. Thanks!

235 ↗(On Diff #11701)

I changed decryptcore to provide the following options:
decryptcore [-Lv] -p privatekey -k key -e encryptedcore -c core
decryptcore [-Lv] [-d crashdir] -p privatekey -n dumpnr
As you can see it is similar to kgdb(1) now.

share/man/man5/rc.conf.5
3516 ↗(On Diff #11701)

I've added a reference to dumpon(8) in both etc/defaults/rc.conf and rc.conf(5).

sys/sys/kerneldump.h
78 ↗(On Diff #11701)

I've bumped both KERNELDUMPVERSION and KERNELDUMP_TEXT_VERSION for this review. I'll move KERNELDUMP_TEXT_VERSION in a separate commit.

def marked 9 inline comments as done.Mar 30 2016, 2:33 AM
def added a comment.Mar 30 2016, 2:40 AM
In D4712#99936, @kib wrote:

I looked over the patch, it looks fine to me.
Still, I have a question. You choice was to modify the dump code, which resulted in spreading the patch over the MI dump and all MD minidump code. Semi-obvious alternate approach is to introduce a geom which would write the generated encrypted symmetric key into the first block and write data block N into encrypted block N+1 of the underlying provider. Then you do not have to touch the kernel dump code at all.
Is it possible technically ?

The kerneldumpkey structure has an arbitrary size. Besides the key two kernel dump headers have to be saved in plaintext, one before and one after a dump. Unfortunately I'm not very familiar with GEOM but currently it sounds like a workaround for me. Wouldn't be it better to unify MD minidump code?

cem added a comment.Apr 2 2016, 3:34 PM
In D4712#123472, @def wrote:
In D4712#99936, @kib wrote:

Still, I have a question. You choice was to modify the dump code, which resulted in spreading the patch over the MI dump and all MD minidump code. Semi-obvious alternate approach is to introduce a geom which would write the generated encrypted symmetric key into the first block and write data block N into encrypted block N+1 of the underlying provider. Then you do not have to touch the kernel dump code at all.

I agree with kib@. It would be nice not to have separate userspace tools here.

In fact we already have encrypted GEOM providers (g_eli, ...). All we lack is a way to dump core through them—maybe? I'm not even sure we lack that.

The kerneldumpkey structure has an arbitrary size.

I am sure this is not a problem in practice. An AES key is far smaller than 512 bytes.

Besides the key two kernel dump headers have to be saved in plaintext, one before and one after a dump.

No, they would be written encrypted too. On boot the encrypted crashdump GEOM would be unlocked and then the normal dump contents would be readable—as if they were plaintext—from the .eli device by the regular crashdump utilities.

Wouldn't be it better to unify MD minidump code?

This would be great, but is not a requirement if crypto is implemented in GEOM instead of in the dump code. It's also hard, slow work (lots of recompiling the kernel for every single supported architecture).

def added a comment.Apr 2 2016, 5:58 PM
In D4712#124230, @cem wrote:
In D4712#123472, @def wrote:
In D4712#99936, @kib wrote:

Still, I have a question. You choice was to modify the dump code, which resulted in spreading the patch over the MI dump and all MD minidump code. Semi-obvious alternate approach is to introduce a geom which would write the generated encrypted symmetric key into the first block and write data block N into encrypted block N+1 of the underlying provider. Then you do not have to touch the kernel dump code at all.

I agree with kib@. It would be nice not to have separate userspace tools here.
In fact we already have encrypted GEOM providers (g_eli, ...). All we lack is a way to dump core through them—maybe? I'm not even sure we lack that.

The kerneldumpkey structure has an arbitrary size.

I am sure this is not a problem in practice. An AES key is far smaller than 512 bytes.

An one-time AES key is encrypted with an asymmetric key. The kerneldumpkey structure includes an encrypted key, its size, an IV and an encryption algorithm identifier.

Besides the key two kernel dump headers have to be saved in plaintext, one before and one after a dump.

No, they would be written encrypted too. On boot the encrypted crashdump GEOM would be unlocked and then the normal dump contents would be readable—as if they were plaintext—from the .eli device by the regular crashdump utilities.

Currently EKCD allows us to save an encrypted crash dump, its encrypted key and send them to another server where we keep a private key. Would it be possible if we used geli(8)?

Wouldn't be it better to unify MD minidump code?

This would be great, but is not a requirement if crypto is implemented in GEOM instead of in the dump code. It's also hard, slow work (lots of recompiling the kernel for every single supported architecture).

pjd edited edge metadata.Apr 2 2016, 9:10 PM
In D4712#99936, @kib wrote:

I looked over the patch, it looks fine to me.
Still, I have a question. You choice was to modify the dump code, which resulted in spreading the patch over the MI dump and all MD minidump code. Semi-obvious alternate approach is to introduce a geom which would write the generated encrypted symmetric key into the first block and write data block N into encrypted block N+1 of the underlying provider. Then you do not have to touch the kernel dump code at all.
Is it possible technically ?

Not really. Dumping kernel memory has nothing to do with GEOM. The only thing that GEOM is involved with is to tell which offset on the physical disk the given partition (that you want to use for dumping the core) has. Dumping memory bypasses GEOM entirely. It is also not disk encryption that should be used here, this is one-time encryption and it is more similar to encrypting a file than block device (hence CBC mode and not XTS). As much I as like GEOM, this is definitely not the right place to do that. We went long way from the initial proposal to the current design and I strongly believe the current code is the right way to do it.

pjd added a comment.Apr 2 2016, 9:16 PM
In D4712#124230, @cem wrote:

I agree with kib@. It would be nice not to have separate userspace tools here.
In fact we already have encrypted GEOM providers (g_eli, ...). All we lack is a way to dump core through them—maybe? I'm not even sure we lack that.

With all due respect, cem. Do you know how the kernel dumping code works? There is no GEOM involved at all. Not only that would be impossible with GELI, as it needs scheduler running, which is not the case when you're dumping kernel core, but GEOM itself cannot work in that situation. Dumping kernel bypasses GEOM completely. Please see my reply to kib@.

Besides the key two kernel dump headers have to be saved in plaintext, one before and one after a dump.

No, they would be written encrypted too. On boot the encrypted crashdump GEOM would be unlocked and then the normal dump contents would be readable—as if they were plaintext—from the .eli device by the regular crashdump utilities.

No, no, no. 99% of the time swap is your dump device and for swap you use one-time keys that aren't stored and are generated on every boot, as you don't need swap content after boot.

Wouldn't be it better to unify MD minidump code?

This would be great, but is not a requirement if crypto is implemented in GEOM instead of in the dump code. It's also hard, slow work (lots of recompiling the kernel for every single supported architecture).

Again, there is no GEOM in the picture.

cem added a comment.Apr 2 2016, 9:39 PM
In D4712#124262, @pjd wrote:

With all due respect, cem. Do you know how the kernel dumping code works?

Adding 'with all due respect' does not make a sentence respectful. This was rude. Please check svn/git history on sys/kern/kern_dump.c.

There is no GEOM involved at all.

GEOM sets the dumpdev device and provides the dump-time IO callbacks.

Not only that would be impossible with GELI, as it needs scheduler running,

Obviously encrypting a block and writing it does not require a scheduler running, so there is nothing preventing a geom device from providing dump callbacks that perform encryption. Any necessary headers can be written with the block 0 call or the final NULL,NULL flush call.

No, no, no. 99% of the time swap is your dump device and for swap you use one-time keys that aren't stored and are generated on every boot, as you don't need swap content after boot.

Your proposed solution doesn't use the same key for swap and encrypted crash dump, why would you assume that limitation in kib/my proposals? Don't use an ephemeral key for crash dumps, obviously.

cem added a comment.Apr 2 2016, 9:43 PM
In D4712#124247, @def wrote:

An one-time AES key is encrypted with an asymmetric key. The kerneldumpkey structure includes an encrypted key, its size, an IV and an encryption algorithm identifier.

This is still far smaller than 512 bytes. And there is no problem with a header spanning multiple blocks either.

Currently EKCD allows us to save an encrypted crash dump, its encrypted key and send them to another server where we keep a private key. Would it be possible if we used geli(8)?

I think you could do the exact same thing, just pushing the encryption code out of kernel dump.

cem added a comment.Apr 3 2016, 12:55 AM

Some minor nits.

sbin/decryptcore/decryptcore.c
37–49 ↗(On Diff #14731)

I believe openssl headers and pjdlog should be in block(s) following standard C headers.

sys/kern/kern_dump.c
124 ↗(On Diff #14731)

Maybe make this static.

131–134 ↗(On Diff #14731)

nbytes = MIN(sz, sizeof(buf));?

sys/kern/kern_shutdown.c
913–920 ↗(On Diff #14731)

Why should it use a different IV (maybe for network dumps, which we don't support today)? And if it should use a different IV, why make that subsequent IV predictable (sha256 of the previous one) instead of arc4random()?

936 ↗(On Diff #14731)

Given this is also the successful exit path, maybe rename failed label to just out.

1056 ↗(On Diff #14731)

Maybe make this static.

1088–1091 ↗(On Diff #14731)

nbytes = MIN(length, sizeof(buf));

1094 ↗(On Diff #14731)

Does rijndael_blockEncrypt really support src == dst?

Why have dump_encrypt take a src and dst parameter at all if we're only going to use it with both equal?

1166–1175 ↗(On Diff #14731)

Ew. Can you just use an ordinary struct for this like kerneldumpheader does?

pjd added inline comments.Apr 3 2016, 2:06 AM
sys/kern/kern_shutdown.c
913–920 ↗(On Diff #14731)

We don't want the same IV, because user may enter DDB, do a crash dump, change dump device and do it again. We would end up with two crash dumps encrypted with the same IV. Or even if user won't switch dump device, so other user may read its content after first dump and before second dump.

I also remember there was a reason to use SHA256 instead of arc4random(). Maybe arc4random() may need scheduler?

emaste added a comment.Apr 5 2016, 5:37 PM

In the future please upload with full context as described in https://wiki.freebsd.org/CodeReview.

sbin/dumpon/dumpon.8
40 ↗(On Diff #14731)

Should this be public_key_file, key_file or something similar? The current form is confusing to me in the context of the "one-time key" description below.

91 ↗(On Diff #14731)

Can we clarify one-time here? One kernel crash? One boot?

197 ↗(On Diff #14731)

This .Xr seems misplaced now?
What about just "...is to use the special debugging sysctl sysctl debug.kdb.panic=1."

followed by

Otherwise a core dump may be triggered from the
.Xr ddb 4
debugger:

but I think @jhb was suggesting just dropping the ddb instructions altogether.

def updated this revision to Diff 15199.Apr 14 2016, 8:02 PM
def edited edge metadata.
def marked 18 inline comments as done.Apr 14 2016, 8:27 PM

Thanks for the review. I sent some replies to the inline comments.

sbin/dumpon/dumpon.8
197 ↗(On Diff #14731)

A user wouldn't have to call doadump if it has ddb enabled in rc.conf and kdb.enter.panic contains 'call doadump; reset' in ddb.conf.

sbin/savecore/savecore.8
31 ↗(On Diff #11701)

Ok.

share/man/man5/rc.conf.5
27 ↗(On Diff #11701)

Ok.

sys/kern/kern_shutdown.c
1094 ↗(On Diff #14731)

Yes, rijndael_blockEncrypt supports it. It's already used in gbde(8) in g_bde_crypt_delete (sys/geom/bde/g_bde_crypt.c) via AES_encrypt.

1166–1175 ↗(On Diff #14731)

We'd like to write packed data and an ordinary structure can have gaps between fields. geli does similar thing in geom/eli/g_eli.h to encode metadata so it can be used by various architectures.

cem added inline comments.Apr 14 2016, 8:30 PM
sys/kern/kern_shutdown.c
1094 ↗(On Diff #14731)

Why have dump_encrypt take a src and dst parameter at all if we're only going to use it with both equal?

1166–1175 ↗(On Diff #14731)

We'd like to write packed data and an ordinary structure can have gaps between fields

So use __packed from sys/cdefs.h.

cem added inline comments.Apr 14 2016, 8:32 PM
sys/kern/kern_shutdown.c
1094 ↗(On Diff #14731)

Nevermind, I see dump_encrypt() was changed to only take the one pointer. Sorry about that.

def updated this revision to Diff 18921.Jul 31 2016, 7:28 PM

I've merged the changes from HEAD. The current patch is against r303588 [1]. The main changes include:

  • ambrisko submitted a patch to write crash dumps on dump devices with a block size other than 512B. EKCD should be compatible with these changes now;
  • DIOCSKERNELDUMP_FREEBSD10 became DIOCSKERNELDUMP_FREEBSD11 as we already have FreeBSD 12.0-CURRENT;
  • DIOCSKERNELDUMP ioctl has a new number as a new DIOCZONECMD ioctl was introduced in [3].

[1] https://svnweb.freebsd.org/base?view=revision&revision=303588
[2] https://svnweb.freebsd.org/base?view=revision&revision=298076
[3] https://svnweb.freebsd.org/base?view=revision&revision=300207

def marked 4 inline comments as done.Jul 31 2016, 7:38 PM
def added inline comments.Jul 31 2016, 7:40 PM
sys/kern/kern_shutdown.c
1166–1175 ↗(On Diff #14731)

Wouldn't it create bus errors on some architectures, e.g. SPARC [1]? It would be nice to have a way to save a crash dump generated by another architecture.

https://docs.oracle.com/cd/E60778_01/html/E60745/bjaby.html#OSSCGbjacr

cem added inline comments.Jul 31 2016, 7:53 PM
sys/kern/kern_shutdown.c
1166–1175 ↗(On Diff #14731)

Nope. The compiler is responsible for generating appropriately sized and aligned memory operations and truncating as needed.

(I don't think that particular piece of Solaris documentation is relevant, but maybe I'm missing something.)

def added inline comments.Jul 31 2016, 8:44 PM
sys/kern/kern_shutdown.c
1166–1175 ↗(On Diff #14731)

This is a quote from the link I sent:

Note - If you use #pragma pack to align struct or union members on boundaries other than their natural boundaries, accessing these fields usually leads to a bus error on SPARC.

As far as I understand SPARC has strict alignment and it won't be possible to save a crash dump generated by an amd64 machine on a SPARC machine because of alignment differences but correct me if I'm wrong.

cem added inline comments.Jul 31 2016, 8:51 PM
sys/kern/kern_shutdown.c
1166–1175 ↗(On Diff #14731)

Does Solaris' #pragma pack have *any* relationship to FreeBSD/Clang(/GCC on Sparc64)'s __packed aka __attribute__((__packed__))? I don't think it does.

As far as I understand it, it's completely invalid for a compiler to produce assembly that will generate bus errors when the struct itself is aligned, even if packed. The standards don't say anything about packed, of course, so I don't have anything authoritative to point to here.

Strict alignment just means the compiler generates assembly instructions to read the aligned DWORD(s) and uses binary shifts or ANDs to extract the relevant field from the aligned memory read. There is no reason packed should generate bus errors. That would be a compiler bug.

oshogbo added inline comments.Aug 15 2016, 12:04 PM
sys/kern/kern_shutdown.c
1221 ↗(On Diff #18921)

I just start wonder, is WAITOK here is a good thing?
Our process was interrupted by panic so we cannot swap any memory, and there isn't any other way to get more memory after a panic, right?
In that case I think we will deadlock here.

Maybe we should change it to the NOWAIT and dump some message like "No enough memory" or so.

The doc say as well that:

Note that M_NOWAIT is required when running in an interrupt context.

I'm not expert but this is interrupt context, right?

kib added inline comments.Aug 15 2016, 12:16 PM
sys/kern/kern_shutdown.c
1221 ↗(On Diff #18921)

No, this is not an interrupt context, really. If you add something like 'NMI interrupt context' definition, then it would be it, but the definition is non-sensical, because it is really any point in the code. Spinlocks cannot protect such context against interrupted execution, and this is the main and fatal difference with the usual interrupt context definition.

That said, malloc/UMA data structures are protected by normal (AKA sleepable) mutexes and thus malloc even with M_NOWAIT flag cannot be used from an interrupt context. It can be used from the context of interrupt thread, but this is much harder requirement. In other words, malloc(9) in any form is highly undesirable in the dumping path.

oshogbo added inline comments.Aug 15 2016, 12:50 PM
sys/kern/kern_shutdown.c
1221 ↗(On Diff #18921)

So do you think that we should have static buffer for it or malloc this memory earlier?

def updated this revision to Diff 19281.Aug 15 2016, 2:06 PM

The patch was updated to fix two problems:

  • Use a packed structure instead of writing a kernel dump key structure byte per byte;
  • Allocate require memory using malloc(9) when it's safe to do that which is during a dump device setup.

Currently all the problems reported in the review are fixed. Once the review is approved I would like to ask the community to test the patch using other architectures than amd64.

def added inline comments.Aug 15 2016, 2:08 PM
sys/kern/kern_shutdown.c
1221 ↗(On Diff #18921)

Thanks Mariusz and Konstantin for discussing this issue. I changed the code to allocate a buffer during a dump device setup.

1166–1175 ↗(On Diff #14731)

Thanks for the explanation, Conrad. It's fixed now.

def marked 12 inline comments as done.Aug 15 2016, 2:10 PM
kib added inline comments.Aug 15 2016, 4:31 PM
sys/kern/kern_shutdown.c
1221 ↗(On Diff #18921)

It would be better that way, as @def said to change. Note that drivers which handle dumpdev might have different idea of dump time, e.g. they could use busdma, which often allocates memory. Still, less reliance on the working kernel subsystems for dump, more reliable it is.

My opinion is that such non-trivial things from ddb/panic context should be performed by mechanism like kexec.

cem accepted this revision.Aug 15 2016, 4:32 PM
cem edited edge metadata.

Changes in this latest version look good to me.

sys/kern/kern_shutdown.c
870 ↗(On Diff #19281)

dumpkeysize = roundup2(sizeof(*kdk) + encryptedkeysize, blocksize);

872 ↗(On Diff #19281)

Do we care if dumpkey is block aligned (kdc isn't likely to be block sized)?

887 ↗(On Diff #19281)

This alias increases the alignment requirements and may produce bus errors outside of x86, I think.

Instead, maybe make the type of the kdc_dumpkey array struct kerneldumpkey[] so that the struct member is suitably aligned.

This revision is now accepted and ready to land.Aug 15 2016, 4:32 PM
def updated this revision to Diff 19334.Aug 16 2016, 12:10 PM
def edited edge metadata.

This revision fixes alignment of the kdc_dumpkey member of the kerneldumpcrypto structure and uses roundup2 macro to round a kernel dump key size.

This revision now requires review to proceed.Aug 16 2016, 12:10 PM
def marked 4 inline comments as done.Aug 16 2016, 12:12 PM
def added inline comments.
sys/kern/kern_shutdown.c
872 ↗(On Diff #19281)

kdc_dumpkey has struct kerneldumpkey[] type now.

cem accepted this revision.Aug 16 2016, 3:37 PM
cem edited edge metadata.

Looks good to me.

This revision is now accepted and ready to land.Aug 16 2016, 3:37 PM
oshogbo accepted this revision.Sep 22 2016, 10:05 AM
oshogbo edited edge metadata.
This revision was automatically updated to reflect the committed changes.
def marked an inline comment as done.