sd-boot: added TPM measurements for initrd by mxre · Pull Request #6124 · systemd/systemd

mxre · 2017-06-14T13:50:56Z

Currently only the efi executable (the linux kernel) is hashed in the TPM PCR[4] using the LoadImage() function of the EFI system. The initrd, which contains code to either unlock or verify the root partition is not hashed in a PCR unless a unified EFI executable is created (systemd efi stub + kernel + initrd in one efi exe)

This patch adds support for hashing all the initrds that are supplied in the sd-boot configfile and append the hash values to the sd-boot TPM PCR hash of the boot options (usually PCR[8])

I thoroughly tested this patch and i have noticed that it has a noticeable performance impact. on my system sd-boot now takes about 250ms to boot, thats about 70ms longer than without this patch.

I'm also not sure if this is the direction the sd-boot developers want to take.

poettering · 2017-06-16T20:08:51Z

I have only a very limited understanding of TPMs, hence I am not going to comment whether this is conceptually OK, but I'll do a code review. For the conceptual part I figure @haraldh should say somehting, he knows TPMs, sd-boot and initrds best. @haraldh?

poettering · 2017-06-16T20:10:30Z

src/boot/efi/boot.c

 #ifdef SD_BOOT_LOG_TPM
+                UINTN i, j;
+                UINTN length;
+                CHAR16* begin, *end;


so far we tried to follow the logic that variables should be decalred at the beginning of code blocks only. Also see CODING_STYLE. Hence, please move these variable declarations up, or even better split this addition out into a new function maybe?

poettering · 2017-06-16T20:10:54Z

src/boot/efi/boot.c

+                UINTN length;
+                CHAR16* begin, *end;
+
+                EFI_FILE* root_dir = LibOpenRoot(entry->device);


also, please don't mix function invocations and variable declarations in the same line, also see CODING_STYLE.

poettering · 2017-06-16T20:11:10Z

src/boot/efi/boot.c

+
+                EFI_FILE* root_dir = LibOpenRoot(entry->device);
+                if (!root_dir) {
+                        Print(L"Unable to open root directory: %r ", err);


trailing whitespace at the end of the string? is that intended?

poettering · 2017-06-16T20:11:34Z

src/boot/efi/boot.c

+                EFI_FILE* root_dir = LibOpenRoot(entry->device);
+                if (!root_dir) {
+                        Print(L"Unable to open root directory: %r ", err);
+                        uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000);


310001000 appears quite arbitary? also, what#s the logic here?

This is the same statement that is used throughout boot.c, it salls, i.e. waits for 3 seconds to allow the user to read the error message

poettering · 2017-06-16T20:12:10Z

src/boot/efi/boot.c

+                                begin = &options[i + 7];
+                                for (j = i + 7; j < length && options[j] != L' '; j++);
+                                end = &options[j];
+                                UINTN initrd_path_len = (end - begin) * sizeof(CHAR16);


no variable declarations in the middle of code blocks. (here and evreywhere else, see above/CODING_STYLE)

poettering · 2017-06-16T20:13:02Z

src/boot/efi/boot.c

+                for (i = 0; i < length; i++) {
+                        if (StrnCmp(L"initrd=", &options[i], 7) == 0) {
+                                begin = &options[i + 7];
+                                for (j = i + 7; j < length && options[j] != L' '; j++);


if you write for() loops without body, please place the trailing semicolon in an empty line, to make this clear. i.e.:

for (…) ;

Measure all initrds that are passed in the commandline and measure them into the default PCR, before measuring the commandline itself and starting the kernel image.

mxre · 2017-06-25T13:54:52Z

I moved my changes in a separate methods. I originally extended the code that hashed the kernel command line for the TPM. I moved that part too, because it makes sense that code that is related should be in the same method.

As before the compilation is conditional on the SD_BOOT_LOG_TPM define.

poettering · 2017-06-27T21:25:03Z

@haraldh any chance you can have a look?

haraldh · 2017-06-28T08:49:02Z

I don't know. This patch measures a file, which is read from disk and then the buffer is thrown away and not passed to the kernel.
IMHO, what really has to happen, is that the kernel measures the buffer it reads from disk afterwards.
The bootloader only passes the path to the kernel. The kernel loads the file.
In theory one could craft a disk presenting a false file on the first request and then present the malicious file on kernel read.
IMHO, it's the wrong approach.

mxre · 2017-06-28T09:13:43Z

The buffer is thrown away because sd-boot uses the kernel's efi stub to load the initrd and does not pass an allocated memory like in the multiboot protocol.

The reason I created this patch primarily for my self is to assure that the initrd was not modified in between boots there is (currently) no possibility for the kernel to actually do this.

I do not see how a potential attacker could pass an alternate disk to load a modified file, because an efi executable that was loaded before sd-boot would modify PCR[4] through the LoadImage() call. And modifying the file on the disk would alter the generated hash, (and this is actually what we want, a mechanism to detect modification).

A later userspace mechanism obviously has to check the integrity of the boot using PCR[4] for the boot loader and the kernel and PCR[8] for the initrds and the commandline options.

Edit:

IMHO, it's the wrong approach.

Should it better be part of sd-efistub?

haraldh · 2017-06-28T10:50:18Z

    IMHO, it's the wrong approach.

Should it better be part of sd-efistub?

Not needed for the efistub, because the whole thing (combined kernel, initrd, cmdline) is measured by the EFI BIOS directly already.

mxre · 2017-06-28T11:03:57Z

Only if you use a combined efi, it needs support from the OS (using kernel-install). How would intel microcode updates work? these need a seperate initrd which need to be loaded first. (at least this is the case on arch-linux)

This patch only works for initrds in the commandline, for combined efi images it would not change a thing, thus only enhancing the experience for users who don't use the combined efi executables.

haraldh · 2017-06-29T11:23:19Z

For fedora the microcode initrd and the real initrd are concatenated. The kernel splits that automatically. So it's only one initrd file.

haraldh · 2017-06-29T11:25:13Z

As I said. It does not make sense to measure something, which you are not using immediately. The kernel efi stub should do the measurement, as it is providing the loaded bytes to the kernel directly.

poettering · 2017-08-31T10:32:58Z

OK, closing as per @haraldh's comments. Sorry!

poettering added the sd-boot/sd-stub/bootctl label Jun 16, 2017

poettering requested changes Jun 16, 2017

View reviewed changes

poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jun 16, 2017

mxre force-pushed the tpm-initrd branch from b37f85b to c558656 Compare June 24, 2017 13:00

sd-boot: added TPM measurements for initrd

a0a5a3e

Measure all initrds that are passed in the commandline and measure them into the default PCR, before measuring the commandline itself and starting the kernel image.

mxre force-pushed the tpm-initrd branch from c558656 to a0a5a3e Compare June 25, 2017 13:51

poettering closed this Aug 31, 2017

josephlr mentioned this pull request Jun 1, 2019

sd-boot: Measure initrd into TPM (attempt #2) #12716

Closed

mxre deleted the tpm-initrd branch October 15, 2021 11:34

Uh oh!

Conversation

mxre commented Jun 14, 2017

Uh oh!

poettering commented Jun 16, 2017

Uh oh!

poettering Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

poettering Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

poettering Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

poettering Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

mxre Jun 25, 2017

Choose a reason for hiding this comment

Uh oh!

poettering Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

poettering Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

mxre commented Jun 25, 2017

Uh oh!

poettering commented Jun 27, 2017

Uh oh!

haraldh commented Jun 28, 2017

Uh oh!

mxre commented Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haraldh commented Jun 28, 2017

Uh oh!

mxre commented Jun 28, 2017

Uh oh!

haraldh commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haraldh commented Jun 29, 2017

Uh oh!

poettering commented Aug 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

mxre commented Jun 28, 2017 •

edited

Loading

haraldh commented Jun 29, 2017 •

edited

Loading