Skip to content
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

AuthVariableLib: Set SB to enabled during transition to USER_MODE #151

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

miczyg1
Copy link
Contributor

@miczyg1 miczyg1 commented Jul 11, 2024

When the system is booted, the AuthVariableLibInitialize runs before SecureBootDefaultKeysDxe. When the system is booted for the first time, AuthVariableLibInitialize will not set SB to enabled state, because there are no keys yet. So the SecureBootEnable does not need to be set depending on PCD, even shouldn't. It is the SecureBootDefaultKeysDxe task to set the proper SB state when system boots with default settings. The PCD is still used there and in the UI when restoring default settings.

Quick tests:

When SB Default State PCD is FALSE:

  1. Secure Boot state on first boot (SMMSTORE region empty) - Disabled - PASS
  2. Secure Boot state when defaults are restored with F9 in UI - Disabled - PASS
    • enter SB menu and enable SB, reboot
    • enter SB menu and press F9, save and reboot
    • enter SB menu and confirm the SB state to be equal to the PCD value
  3. Secure Boot state when keys are enrolled from OS - Enabled - PASS
  4. Booting Ubuntu with SB:
    • Booting standard shim+grub - Access Denied - PASS
    • Booting custom signed grub - PASS (but did not boot OS due to shim_lock protocol not found, kind of expected when shim was omitted)

When SB Default State PCD is TRUE:

  1. Secure Boot state on first boot (SMMSTORE region empty) - Enabled - PASS
  2. Secure Boot state when defaults are restored with F9 in UI - Enabled - PASS
    • enter SB menu and disable SB, reboot
    • enter SB menu and press F9, save and reboot
    • enter SB menu and confirm the SB state to be equal to the PCD value
  3. Secure Boot state when keys are enrolled from OS - Enabled - PASS
  4. Booting Ubuntu with SB:
    • Booting standard shim+grub - Access Denied - PASS
    • Booting custom signed grub - PASS (but did not boot OS due to shim_lock protocol not found, kind of expected when shim was omitted)

Questions:

  1. Resetting Secure Boot keys to defaults causes the SB state to become enabled, but it can also set the state to disabled if we want to. Is it the behavior we want? Done, now when resetting the keys, Enable Secure Boot checkbox remains unchanged. Only the Current Secure Boot State changes from Disabled to Enabled, but there is now way around it. SecureBoot variable becomes locked after enrolling PK.
  2. Pressing F9 in SB menu restores the Secure Boot enable state only to the PCD value, it does not restore the keys to defaults, but it could if we want to. Is it the behavior we want?

@miczyg1 miczyg1 requested a review from macpijan July 11, 2024 13:25
@miczyg1
Copy link
Contributor Author

miczyg1 commented Jul 15, 2024

@macpijan ?

@macpijan
Copy link
Contributor

macpijan commented Jul 15, 2024

Resetting Secure Boot keys to defaults causes the SB state to become enabled, but it can also set the state to disabled if we want to. Is it the behavior we want?

Is option "do not change whatever has been set prior to resetting" on the table?

Otherwise, set the default for platform, if not possible, keep disabled?

Pressing F9 in SB menu restores the Secure Boot enable state only to the PCD value, it does not restore the keys to defaults, but it could if we want to. Is it the behavior we want?

I'd vote to keep it as is. If F9 would reset keys as well, the option of restoring default keys would be questionable to exist in the first place, if functionally equivalent to F9.

@miczyg1
Copy link
Contributor Author

miczyg1 commented Jul 16, 2024

Resetting Secure Boot keys to defaults causes the SB state to become enabled, but it can also set the state to disabled if we want to. Is it the behavior we want?

Is option "do not change whatever has been set prior to resetting" on the table?

Exactly this. Do we want the "reset keys to default" change the Secure Boot state or not?

@miczyg1 miczyg1 requested a review from mkopec July 16, 2024 14:05
@macpijan
Copy link
Contributor

Exactly this. Do we want the "reset keys to default" change the Secure Boot state or not?

Then, I'd say it should not change, and keep whatever was set before. If user wants to change, they should do it.

@macpijan
Copy link
Contributor

@miczyg1 We can proceed with the decisions discussed here.

@krystian-hebel
Copy link
Contributor

Exactly this. Do we want the "reset keys to default" change the Secure Boot state or not?

I'm assuming we aren't talking about SecureBoot, but SecureBootEnable variable. The former depends on keys that were enrolled during boot, the latter is just a flag that enables SecureBoot if and only if other requirements are met, which are well defined in the spec. SecureBoot isn't NV, it only lives in RAM, so there is nothing to change there. However, SecureBoot should change its value on "Platform Specific PKpub Clear" to disabled, which makes sense in case it can be changed without the reset - otherwise it would be possible to boot with SB reported as enabled even though keys were changed along the way.

Summing up, SecureBootEnable may retain its value, but SecureBoot must be cleared (in a sticky way until the next reboot) when keys are reset. This should be reflected in efivars after keys are enrolled from within OS: boot with SB enabled and custom keys enrolled (user mode), change the keys by writing new ones signed with old keys, check value of SecureBoot.

Please add some description to the commit message, like the firs paragraph of this PR description. Also, how does it relate to this?

@miczyg1
Copy link
Contributor Author

miczyg1 commented Jul 19, 2024

However, SecureBoot should change its value on "Platform Specific PKpub Clear" to disabled, which makes sense in case it can be changed without the reset - otherwise it would be possible to boot with SB reported as enabled even though keys were changed along the way.

Yes, when keys are erased, SB is being disabled. That's perfectly understandable.

Summing up, SecureBootEnable may retain its value, but SecureBoot must be cleared (in a sticky way until the next reboot) when keys are reset.

Yes, I think we want to retain SecureBootEnable value, because if someone had all keys enrolled and SB disbalde, resetting the keys would enable SB automatically. We enforce reboot/reset after every change in the menu, so SecureBoot variable will always have a proper value after reboot.

@miczyg1 miczyg1 force-pushed the auto_sb_enable branch 4 times, most recently from 3b03021 to 2146220 Compare July 19, 2024 13:48
@miczyg1
Copy link
Contributor Author

miczyg1 commented Jul 19, 2024

Now when resetting the keys, Enable Secure Boot checkbox remains unchanged. Only the Current Secure Boot State changes from Disabled to Enabled, but there is now way around it. SecureBoot variable becomes locked after enrolling PK.

@miczyg1 miczyg1 requested a review from krystian-hebel July 19, 2024 14:01
Copy link
Contributor

@krystian-hebel krystian-hebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From technical point of view it works more or less as discussed, but UX is poor due to Enable Secure Boot [ ] when SecureBootEnable is true, but no PK.

However, I don't think this by itself meets the expectations described by the vendor. Enrolling PK from OS does not enable SB without setting SecureBootEnable in setup menu, at least with current CONFIG_EDK2_SECURE_BOOT_DEFAULT_ENABLE state. It is acceptable, but:

  • Should be documented (https://docs.dasharo.com -> guides?)
    • preferably with instructions for extracting current/default KEK, db and dbx, and signing them with new PK to not have to sign existing bootloaders,
    • similar instructions for users that want to build edk2 with custom keys, maybe?
  • It would be easier for us to have the same default across all platforms.
  • Currently, SMMSTORE region is cleared with each update, which makes whole custom SB configuration tedious, but it may be fixed in the future with the help of capsules (in the wider meaning, not necessarily capsule updates).

Also, please add commit messages.

@pietrushnic
Copy link
Contributor

@macpijan @BeataZdunczyk I'm not sure what is the context of these changes, but they seem to be closely related to planned Arch4221: UEFI Secure Boot so if you could link that effort internally, it would be great.

miczyg1 added 2 commits July 22, 2024 14:57
Do not use PcdSecureBootDefaultEnable to initialize SecureBootEnable
variable value when platform is in USER_MODE, because it will prevent
enabling Secure Boot from OS when enrolling the keys and transitioning
from SETUP_MODE to USER_MODE. PcdSecureBootDefaultEnable is used when
settings are reset in UI and in the SecureBootDefaultKeysDxe where the
default keys are being restored.

Signed-off-by: Michał Żygowski <[email protected]>
Reset Secure Boot Keys option should only touch the keys and nothing
else. Keep the SecureBootEnable variable value from before selecting
to reset the keys as re-enrolling PK would unconditionally enable
Secure Boot.

Signed-off-by: Michał Żygowski <[email protected]>
@krystian-hebel krystian-hebel merged commit f066733 into dasharo Jul 22, 2024
3 checks passed
@krystian-hebel krystian-hebel deleted the auto_sb_enable branch July 22, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants