rhboot / shim

UEFI shim loader
Other
819 stars 289 forks source link

Impoved revocation management #526

Closed jsetje closed 2 months ago

jsetje commented 1 year ago

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

SherifNagy commented 1 year ago

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:

  1. 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?
  2. will distros sign the new sbat_level.efi or MSFT?
  3. 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 commented 1 year ago
  • 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 commented 1 year ago

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.

jsetje commented 1 year ago

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 commented 1 year ago
  • 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 commented 1 year ago

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.

jsetje commented 2 months ago

Closing this PR since this was merged.