Impoved revocation management #526

jsetje · 2022-11-10T21:20:02Z

This covers delivering updates to SBAT_LEVEL without the need to create and sign a new shim

Signed-off-by: Jan Setje-Eilers Jan.SetjeEilers@oracle.com

jsetje · 2022-11-10T23:47:21Z

shim.c

@@ -1482,24 +1541,28 @@ load_certs(EFI_HANDLE image_handle)
 		goto done;
 	}



The set of diffs below are not as strange as they look: load_sbat_level_file(image_handle, PathName); is added and then everything just moved into if (secure_mode()) { }
The diff just hung onto the empty line because it's not smart enough to ignore indenting.

jsetje · 2022-11-10T23:49:45Z

shim.c

+	int i;
+	char *sbat_var_previous = NULL;
+	char *sbat_var_latest = NULL;
+


The read_image(), verify_image() could be factored out and re-used by load_cert_file(), let me know if you prefer that, it's ultimately a short function with a ton of arguments.

SherifNagy · 2022-11-11T10:14:17Z

I have a couple of question and I hope they make sense, I am still trying to get my head around lots of concepts with shim and secureboot:

Is this the SBAT level for SHIM? if so, would that means we will get rid of having global generation in data/sbat.cvs file?
will distros sign the new sbat_level.efi or MSFT?
I assume shim will need to verify the signed sbat_level.efi somehow, is that why you want to refactor and reuse the load_cert_file() function?

jsetje · 2022-11-11T20:51:54Z

Is this the SBAT level for SHIM? if so, would that means we will get rid of having global generation in data/sbat.cvs file?

No, these are the minimum levels that we require binaries to have in order to load them.

will distros sign the new sbat_level.efi or MSFT?

It could be any key in the db, either built into shim, or the MS UEFI CA. I'd like to get MS to sign a single universal binary, but if they can't distros can sign their own.

I assume shim will need to verify the signed sbat_level.efi somehow, is that why you want to refactor and reuse the load_cert_file() function?

Correct, that file needs to be signed and not also not revoked via sbat level in case there's an exploit for the certmule binary (which seems awfully unlikely, but best to have everything in place. :) )

jsetje · 2022-11-12T18:55:09Z

I added another call to try to load / apply sbat_level after bringing in the certmules in case the first on failed. This will pick up an sbat_level file signed with a key in a certmule.

chrisccoulson

Hi,

I've not done a thorough review yet and I haven't finished looking at everything, but I've left a few comments inline. I've got some other general comments as well:

As the code is currently implemented, shim performs its self check with the current SbatLevel, then potentially updates the contents of SbatLevel from an external binary before measuring the updated contents once to the TPM. As we make a policy decision with the old value (when doing the shim self check), I wonder whether the TPM measurement should be made before we load an update (with the value that was used for the self check), and then re-measuring the updated value of SbatLevel if it is updated afterwards?
Is it necessary to supply "previous" and "latest" values from an external binary? Whilst this makes sense for payloads that are delivered via the shim binary, can we just deliver a single revocation payload via the external binary? In this case, the revocation level that is applied depends on whatever binary is placed in the ESP.
SBAT updates currently use a timestamp for preventing rollback of the revocations - I wonder whether we'd have been better off assigning a component name for SbatLevel updates and using the same SBAT mechanism that is used to revoke other binaries to prevent downgrades. This makes more sense with the model where SbatLevel updates are consumed from an external binary, because that binary will end up having to have its own SBAT component name and generation number anyway.

include/ssp.h

chrisccoulson · 2023-07-02T20:41:39Z

include/ssp_var_defs.h

+#define SSP_VAR_DEFS_H_
+
+#define SSPVER_SIZE 8
+#define SSPSIG_SIZE 131


Rather than hardcoding the sizes here, can we do:

uint8_t SkuSiPolicyVersion[] = {...}; uint8_t SkuSiPolicyUpdateSigners[] = {...}; #define SSPVER_SIZE sizeof(SkuSiPolicyVersion) #define SSPSIG_SIZE sizeof(SkuSiPolicyUpdateSigners)

Those sizes came from the spec, I don't want to accidentally set something that doesn't match here since I have to assume that the consumer may make some explicit assumptions about the length as well.

I think I came up with the right solution here. I kept the size definitions and assert the actual size of the structures at build time. Doing something similar for the mule is less obvious. Hmm.

chrisccoulson · 2023-07-02T21:07:42Z

shim.c

@@ -1377,6 +1377,84 @@ uninstall_shim_protocols(void)
 #endif
 }

+void
+check_section(char *section_name, int len, void **pointer,
+		EFI_IMAGE_SECTION_HEADER *Section, void *data, int datasize)


As the section_name argument to this function is a static string and always NULL terminated, is the len parameter necessary here? The function could just call strlen. Or if we wanted the size to be determined at compile time, an alternative would be to use sizeof at the caller and wrap it in a macro to avoid manually specifying the size:

#define CHECK_SECTION(section_name, pointer, section, data, datasize) check_section(section_name, sizeof(section_name) - 1, pointer, section, data, datasize)

I'm not sure what style others prefer here.

You're right that's messy.

Since this can be done at compile time, that seems preferable.

chrisccoulson · 2023-07-02T21:42:50Z

shim.c

+			return ;
+		}
+		dprint(L"found %.*s\n", len, section_name);
+	}


It doesn't look like we're checking the size of the section here, and subsequently whether the end of the section is within bounds.

I can add those checks, but does it really make sense to validate that a signed binary isn't broken?
I suppose someone could sign a negative test and ship that by mistake.

Since I couldn't immediately figure out a way to do this at build time in the mule binary, I added some checks here. I didn't limit the sbat payload size, although we may want to come up with a worst case number for that as well.

I'm still thinking about a way to assert this at build time in the mule as well.

chrisccoulson · 2023-07-02T21:47:51Z

sbat.c

+
+EFI_STATUS
+set_ssp_uefi_variable(uint8_t *ssp_ver_previous, uint8_t *ssp_sig_previous,
+		uint8_t *ssp_ver_latest, uint8_t *ssp_sig_latest)


This function is taking pointers to buffers that come from elsewhere, but making assumptions about their size internally. Eg, it assumes that that the size of ssp_sig_previous and ssp_sig_latest - both of which could come from an external binary - have the size of SSPVER_SIZE. I wonder whether we should be encoding and obtaining the actual sizes from the binary we're reading them from and then supplying them here?

Again, those come from the spec. This may be a question for the folks that wrote that spec. Could that size ever evolve?

This should now match based on how we pulled it from the mule sections.

jsetje · 2023-07-03T15:46:43Z

Thank you for looking at this!!

Is it necessary to supply "previous" and "latest" values from an external binary? Whilst this makes sense for payloads that are delivered via the shim binary, can we just deliver a single revocation payload via the external binary? In this case, the revocation level that is applied depends on whatever binary is placed in the ESP.

This is the only comment I have a quick answer to offhand. I did want the ability to push out either a forced update, or an optional update via the mule binary. Perhaps latest and previous should be renamed.

I'll think over the other two points today. I suspect you're right about re-measuring, although perhaps shim should also check that it doesn't revoke itself.

jsetje · 2023-07-06T01:33:40Z

As the code is currently implemented, shim performs its self check with the current SbatLevel, then potentially updates the contents of SbatLevel from an external binary before measuring the updated contents once to the TPM. As we make a policy decision with the old value (when doing the shim self check), I wonder whether the TPM measurement should be made before we load an update (with the value that was used for the self check), and then re-measuring the updated value of SbatLevel if it is updated afterwards?

Initially I agreed with you but then I got stuck on not wanting the measurement to be a different value between the first boot with a mule based update and a subsequent reboot. Since it looks like we make the measurement later, when we mirror the variable, and any newer variable should effectively be a super set, I think this is in the state I want it to be in.

Edit: What's probably missing here is another self check that prevents shim from applying a revocation that will revoke itself. Matthew had asked for that a while back as well. If that's in place, then I feel like we can say that the policy we measured at RT mirror time is in fact the one we booted with.

SBAT updates currently use a timestamp for preventing rollback of the revocations - I wonder whether we'd have been better off assigning a component name for SbatLevel updates and using the same SBAT mechanism that is used to revoke other binaries to prevent downgrades. This makes more sense with the model where SbatLevel updates are consumed from an external binary, because that binary will end up having to have its own SBAT component name and generation number anyway.

While this could be done, it then overloads that level versioning with revocations for the certmule binary. I also do want to retain the ability to deliver built into shim updates.

jsetje · 2023-07-18T22:54:54Z

Rather than running everything through clang-format this cleans up a couple of new pieces of code in sbat.c and doesn't change anything else. I did cleanup a reference to mule binaries since those are being renamed.

Ingest SBAT Levels from revocations binary thereby allowing level requirements to be updated independently from shipping a new shim. Do not automatically apply any revocations from a stock shim at this point. Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>

Unless an explict sbat policy is specified, always delete SbatLevel when secure boot is disabled. Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>

This adds support for applying SkuSiPolicy UEFI BS variables. These varaibles are needed for non-dbx based Windows revocations and are described here: https://support.microsoft.com/en-us/topic/kb5027455-guidance-for-blocking-vulnerable-windows-boot-managers-522bb851-0a61-44ad-aa94-ad11119c5e91 Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>

Before applying an updated SbatLevel shim should re-run introspection and never apply a revocation level that would prevent the currently running shim from booting. The proper way forward is to update shim first. Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>

If shim detects a self revocation in a new proposed SbatLevel and refuses to apply this new set of revocations a message should be printed even in non-verbose modes. Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>

shim.c

jsetje · 2024-05-16T15:37:22Z

Closing this PR since this was merged.

jsetje commented Nov 10, 2022

View reviewed changes

jsetje force-pushed the main branch from 85aa841 to 7f50db1 Compare November 12, 2022 18:53

jsetje force-pushed the main branch from 7f50db1 to cd85aa9 Compare March 31, 2023 21:40

jsetje force-pushed the main branch from cd85aa9 to 704b172 Compare April 11, 2023 03:04

jsetje force-pushed the main branch from 704b172 to 7d7a1a6 Compare April 27, 2023 23:32

jsetje force-pushed the main branch from 7cbf781 to 5f970fa Compare May 16, 2023 21:53

jsetje changed the title ~~Ingest SBAT Levels from sbat_level binary~~ Impoved revocation management May 16, 2023

jsetje marked this pull request as draft May 16, 2023 22:00

jsetje force-pushed the main branch 5 times, most recently from 03d93b5 to f67e11b Compare June 12, 2023 23:09

jsetje marked this pull request as ready for review June 12, 2023 23:29

jsetje force-pushed the main branch 3 times, most recently from a794b6c to 2fe015a Compare June 23, 2023 21:15

chrisccoulson reviewed Jul 2, 2023

View reviewed changes

jsetje force-pushed the main branch 3 times, most recently from f03f032 to 8b0ff3d Compare July 4, 2023 04:18

jsetje force-pushed the main branch 3 times, most recently from 6bb81c5 to e39b222 Compare July 7, 2023 20:35

jsetje force-pushed the main branch 6 times, most recently from 598df95 to 938a93d Compare July 14, 2023 20:42

jsetje force-pushed the main branch from 938a93d to 278a3ff Compare July 14, 2023 20:54

jsetje mentioned this pull request Jul 18, 2023

Revocation management #588

Closed

jsetje force-pushed the main branch from 278a3ff to 673c0e9 Compare July 18, 2023 22:52

vathpela force-pushed the main branch from 62e4b44 to 1e985a3 Compare July 19, 2023 20:09

jsetje added 5 commits July 19, 2023 14:14

Always clear SbatLevel when Secure Boot is disabled

f9f5873

Unless an explict sbat policy is specified, always delete SbatLevel when secure boot is disabled. Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>

Print message when refusing to apply SbatLevel

f6fc0a3

If shim detects a self revocation in a new proposed SbatLevel and refuses to apply this new set of revocations a message should be printed even in non-verbose modes. Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>

jsetje force-pushed the main branch from 673c0e9 to f6fc0a3 Compare July 19, 2023 21:14

vathpela reviewed Aug 2, 2023

View reviewed changes

shim.c Show resolved Hide resolved

jsetje closed this May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Impoved revocation management #526

Impoved revocation management #526

jsetje commented Nov 10, 2022

jsetje Nov 10, 2022

jsetje Nov 10, 2022

SherifNagy commented Nov 11, 2022

jsetje commented Nov 11, 2022

jsetje commented Nov 12, 2022

chrisccoulson left a comment

chrisccoulson Jul 2, 2023

jsetje Jul 3, 2023

jsetje Jul 4, 2023 •

edited

chrisccoulson Jul 2, 2023

jsetje Jul 3, 2023

chrisccoulson Jul 2, 2023

jsetje Jul 3, 2023

jsetje Jul 4, 2023

chrisccoulson Jul 2, 2023

jsetje Jul 3, 2023

jsetje Jul 4, 2023

jsetje commented Jul 3, 2023

jsetje commented Jul 6, 2023 •

edited

jsetje commented Jul 18, 2023

jsetje commented May 16, 2024

		@@ -1482,24 +1541,28 @@ load_certs(EFI_HANDLE image_handle)
		goto done;
		}

Impoved revocation management #526

Impoved revocation management #526

Conversation

jsetje commented Nov 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SherifNagy commented Nov 11, 2022

jsetje commented Nov 11, 2022

jsetje commented Nov 12, 2022

chrisccoulson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsetje Jul 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsetje commented Jul 3, 2023

jsetje commented Jul 6, 2023 • edited

jsetje commented Jul 18, 2023

jsetje commented May 16, 2024

jsetje Jul 4, 2023 •

edited

jsetje commented Jul 6, 2023 •

edited