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

uefi-ssp-policy: Add a new plugin to detect missing SkuSiPolicy attributes #7331

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hughsie
Copy link
Member

@hughsie hughsie commented Jun 8, 2024

Fixes #7329

Type of pull request:

@hughsie
Copy link
Member Author

hughsie commented Jun 8, 2024

@jsetje some review most welcome please

@hughsie hughsie marked this pull request as ready for review June 12, 2024 18:18
@hughsie
Copy link
Member Author

hughsie commented Jun 12, 2024

@jsetje does this look sane?

plugins/uefi-ssp-policy/README.md Outdated Show resolved Hide resolved
plugins/uefi-ssp-policy/fu-uefi-ssp-policy-plugin.c Outdated Show resolved Hide resolved
plugins/uefi-ssp-policy/fu-uefi-ssp-policy-plugin.c Outdated Show resolved Hide resolved
Comment on lines 66 to 77
/* we can fix this! */
fwupd_security_attr_add_flag(attr, FWUPD_SECURITY_ATTR_FLAG_CAN_FIX);
fwupd_security_attr_set_result(attr, FWUPD_SECURITY_ATTR_RESULT_NOT_FOUND);
Copy link
Member

Choose a reason for hiding this comment

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

premature, no? I don't see any code in here calling out the fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohh fu_uefi_ssp_policy_plugin_fix_host_security_attr does exist -- apparently by setting the SSPPolicy key to 1 on next reboot shim will create the SkuSiPolicyVersion and SkuSiPolicyUpdateSigners -- although it doesn't seem to work for me at all.

@hughsie
Copy link
Member Author

hughsie commented Jun 13, 2024

@vathpela can you see anything fishy here? I can't get shim to set the SkuSiPolicyVersion or SkuSiPolicyUpdateSigners keys and I'm not super sure how to debug shim. Thanks!

@hughsie hughsie force-pushed the hughsie/SkuSiPolicy branch 2 times, most recently from d89270a to ae627d5 Compare June 13, 2024 11:27
}

/* we can fix this! */
fwupd_security_attr_add_flag(attr, FWUPD_SECURITY_ATTR_FLAG_CAN_FIX);
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to check whether SSPPolicy already exists though don't you? It can only be fixed if the file doesn't exist.

Copy link
Member

@superm1 superm1 left a comment

Choose a reason for hiding this comment

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

Can you use the dummy efivars interface for a self test? Then you can cover existing variables, junk variables, all the permutations we might see in the wild.

@hughsie
Copy link
Member Author

hughsie commented Jun 14, 2024

@superm1 talking of testing -- do we have a plan for getting coverage back? At least physiologically seeing a number "go down" means we're more likely to write tests.

@superm1
Copy link
Member

superm1 commented Jun 14, 2024

@superm1 talking of testing -- do we have a plan for getting coverage back? At least physiologically seeing a number "go down" means we're more likely to write tests.

I'm trying to remember why it broke. It was some random bug in Debian we couldn't figure out right? Maybe we should just try to turn it back on in a PR and see what happens.

(void)g_setenv("G_MESSAGES_DEBUG", "all", TRUE);

/* tests go here */
g_test_add_func("/uefi-ssp-policy/hsi", fu_uefi_ssp_policy_plugin_func);
Copy link
Member

Choose a reason for hiding this comment

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

This is great! Can you add a second test of no Windows in the boot order?

@sei-vsarvepalli
Copy link

sei-vsarvepalli commented Jun 20, 2024

Look at SkuSiPolicyVersion perhaps - https://support.microsoft.com/en-us/topic/kb5027455-guidance-for-blocking-vulnerable-windows-boot-managers-522bb851-0a61-44ad-aa94-ad11119c5e91

SKU SiPolicy Variables

This policy uses two UEFI variables stored under the EFI Namespace/Vendor
GUID(SECUREBOOT_EFI_NAMESPACE_GUID):

#define SECUREBOOT_EFI_NAMESPACE_GUID \

{0x77fa9abd, 0x0359, 0x4d32, \

{0xbd, 0x60, 0x28, 0xf4, 0xe7, 0x8f, 0x78, 0x4b}};

SkuSiPolicyVersion

is of type ULONGLONG/UInt64 at runtime

is defined by <VersionEx>2.0.0.2</VersionEx> within policy XML in the form of (MAJOR.MINOR.REVISION.BUILDNUMBER)

It is translated into ULONGLONG as

((major##ULL << 48) + (minor##ULL << 32) + (revision##ULL << 16) + buildnumber)

Each version number has 16 bits, so it has a total of 64 bits.
..
Namespace Guid:

77fa9abd-0359-4d32-bd60-28f4e78f784b

@jsetje
Copy link

jsetje commented Jun 21, 2024

I assume you're running a sufficiently new shim? The SkuSi support went into 15.8. Here's the code in mokutil that sets the variable: https://github.com/lcp/mokutil/blob/master/src/mokutil.c#L1792 the attributes probably matter.

Also, the interface I've been trying to preserve is mokutil not inherently setting the variables, although I'm willing to promote that by adding an appropriate comment to where shim consumes the variables.

Oh, the SkuSi variables are all BS variables only. I don't think they get mirrored to RT counterparts, since they aren't really part of MOK, although perhaps they should -- which would open the question of whether or not they should be measured (only?) when the Windows CA is trusted, which adds quite a bit of complexity. Hmm.

guint8 val = SHIM_SSP_POLICY_LATEST;

/* shim will do the right thing on next boot */
if (!fu_efivars_set_data(efivars,
Copy link

Choose a reason for hiding this comment

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

I would think that would work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants