-
Notifications
You must be signed in to change notification settings - Fork 291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Impoved revocation management #526
Conversation
@@ -1482,24 +1541,28 @@ load_certs(EFI_HANDLE image_handle) | |||
goto done; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
int i; | ||
char *sbat_var_previous = NULL; | ||
char *sbat_var_latest = NULL; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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:
|
No, these are the minimum levels that we require binaries to have in order to load them.
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.
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. :) ) |
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. |
03d93b5
to
f67e11b
Compare
a794b6c
to
2fe015a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_var_defs.h
Outdated
#define SSP_VAR_DEFS_H_ | ||
|
||
#define SSPVER_SIZE 8 | ||
#define SSPSIG_SIZE 131 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
shim.c
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right that's messy.
Since this can be done at compile time, that seems preferable.
return ; | ||
} | ||
dprint(L"found %.*s\n", len, section_name); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, those come from the spec. This may be a question for the folks that wrote that spec. Could that size ever evolve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should now match based on how we pulled it from the mule sections.
Thank you for looking at this!!
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. |
f03f032
to
8b0ff3d
Compare
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.
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. |
6bb81c5
to
e39b222
Compare
598df95
to
938a93d
Compare
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>
Closing this PR since this was merged. |
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