In ProcessVarWithPk in AuthService.c, we check fall into the last else branch if we are in SETUP mode and it is the PK that we're updating. The PK packet shouldn't be self-signed.
Matthew, The following mailing list thread (a patch posting from 2019) is related: * [edk2-devel] [PATCH] SecurityPkg: Don't Verify the enrolled PK in setup mode http://mid.mail-archive.com/7564.1562045108414671150@groups.io https://edk2.groups.io/g/devel/message/43150 The patch there removes the last "else" branch that you mention. The patch was not been merged; I objected to it in its posted form. A v2 was requested (with a FeaturePCD), but then Derek Lin (the original submitter) decided he liked the in-tree behavior more than a new FeaturePCD. To summarize my request from back then: I don't object to the proposed PK enrollment relaxation as an option (even as the default option), I'd just like a FeaturePCD for platforms that prefer keeping the current self-signedness requirement. Thanks!
Laszlo, Thanks for commented with those links, it's very helpful to see more of the history around this issue. I agree- a FeaturePCD is needed here. Should the disabled behavior to be the same as what it currently does now? Since we the spec says that it doesn't have to be checked for self-signing, should the PCD be opt-in or opt-out?
Hi Matthew, I think a platform should not be required to tweak a PCD (i.e., to diverge from the PCD default inherited from the package DEC file), in order to get spec-conformant behavior. In other words, the default behavior for the logic in question should be changed (relaxed) so that it match the spec. And then the PCD could be used by platforms to diverge from the spec (i.e., in this particular case, to enforce self-signedness on the PK, which is stricter than what the spec mandates). So whoever proposed a patch for SecurityPkg, I'd ask them to attach two more patches (one for OvmfPkg and ArmVirtPkg each), for opting into the strict self-signedness check. Whether the name of the new PCD should be in the negative sense ("disable self-signedness check") or in the positive sense ("enable self-signedness check"), is an orthogonal question. I'll defer to the SecurityPkg maintainers on that. Thanks! Laszlo
In order to keep bisectability, I'd suggest something like: - patch#1: introduce "PcdRequireSelfSignedPk" to SecurityPkg (code and PCD), with the PCD default being TRUE - patch#2: explicitly set PcdRequireSelfSignedPk to TRUE in the OvmfPkg DSC files, when SECURE_BOOT_ENABLE - patch#3: similarly in ArmVirtPkg - patch#4: flip the DEC default in SecurityPkg to FALSE.
I think we can go ahead with that plan. Perhaps #2 and #3 could be combined, but I can understand the case for them to be seperate. The fix is public here: https://github.com/microsoft/mu_basecore/commit/0c424a7c0a505bbf81825b617a335cbbf455f091
Created attachment 460 [details] Potential patch for PK Self-signing spec A potential patch that lines up with #1 on Lazlo's suggested patch list
Matthew Carlson: can you share what test has been verified for this change?
Hey Liming, I verified this works by clearing the PK and applying a new PK that was not self signed via the EFI shell and an OS call. I then verified that once set, the PK was not able to be set again, self-signed or not.
Any update on this?
(1) updating Liming's adddress on the CC list (2) I think once Liming confirms that Matthew's testing described in comment 8 is okay, Matthew should post the series to edk2-devel.
Would you please send out the patch set, we can continue the review process?
I've sent out a patch series to edk2-devel according to Laszlo's suggestion in comment #4, cover letter here: https://edk2.groups.io/g/devel/message/98945
Merged https://github.com/tianocore/edk2/pull/3998
