From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM10-MW2-obe.outbound.protection.outlook.com (NAM10-MW2-obe.outbound.protection.outlook.com [40.92.42.77]) by mx.groups.io with SMTP id smtpd.web11.5450.1674873439036087287 for ; Fri, 27 Jan 2023 18:37:19 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@outlook.com header.s=selector1 header.b=nRGB3OUK; spf=pass (domain: outlook.com, ip: 40.92.42.77, mailfrom: spbrogan@outlook.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=cIS/N0xLAJffjUAOZCPyfvkFKe2UGxdd4N9F99VL6KgJfQb5MoYce2fGVY3nGf8Uc5RKn/YHrE8KXJdT1I1Tz5722h5OhwN3OBquGNNmJKB0hCtoc/4VWElMODeYOCzKouRaW++HLRh0ComELJ+1N8epboIm9hST69A+5w3mBPA5ZSv4ChuTRoCZTI/wksYJSn5Gh16hEplh2S9wZFS3ceNa5h2puSUFfNj2HHj2zCFxId5mf6rTYGClsCPAf6C6+ufBdn1ZJVlQLjnBjhmXhalq87OaxjXrTxic0NndYfsEkXnnnRGcL8siexdSYRLBxYAi1woOK3ihpK9cMIXRew== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=WxPNlOSnAn634BOLF2jFqzA5Azosz4Ys4CWH57/qLYo=; b=jxqi8Evx4pVSj8X1Zq2XskL0hdCzbY6PHFyXgMKKdzZm0KOBfcylDhIJieakjYjjdMxRFKh4p4edEz+tqFWW4WXqJzHujGROu2VPzZIM7/hz0q4hp3LJ/mLJ8GH9kBnoBgqhTMIhk5KazLiRR+A7Opxy8L2ubVg1YaptLufCkMTcPixWBQTwfGqYKBlejSVfYEd2mzoSnIgK0sEEXVyb3brfdla3Igm5NBv14yxVbYN5jMerdjbcdsjfxnoxYnGMCHhhaxV8AbDBZ/lCzMbUHK5ciNiggz1pArtLY+olq3oAhwGl0sih2iZE0RIYEYpGMjs3K4XJ+LLvMo5KM6UL0A== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=WxPNlOSnAn634BOLF2jFqzA5Azosz4Ys4CWH57/qLYo=; b=nRGB3OUKtbl5SPK4lX+ZnkPPTzX+S3/z9UkwIvno68GttGh2fqjRl1EpIf0kNhZEGYdxrDsAPfwJXliweowu63Pvc7HU/GKlMCHriXKUp5GV7S14r+mVLWi0rq+yR902RrqFmUTxgVei0PxQIhJt1WeyfAOhkbVS8nZ4PqWkbpeEE5gytjp/Ijdx9gCIWYc438SygU4mMoHrde5DFbTdlBj0wEUq2iMlx58W1sRMOL797Hu2tXF/IwNE7JCP/P8GqBgjyh/YG4G9zfGy67aTdiYahtor+Z0anzmm/Y0l5ll6EiMZg/dsjj2NYI0mD7plM8MF3YOJ//3HtC/KRy8T5A== Received: from SA1PR19MB4911.namprd19.prod.outlook.com (2603:10b6:806:185::10) by CH3PR19MB7212.namprd19.prod.outlook.com (2603:10b6:610:144::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6043.23; Sat, 28 Jan 2023 02:37:17 +0000 Received: from SA1PR19MB4911.namprd19.prod.outlook.com ([fe80::22eb:1af:29ef:511a]) by SA1PR19MB4911.namprd19.prod.outlook.com ([fe80::22eb:1af:29ef:511a%3]) with mapi id 15.20.6043.028; Sat, 28 Jan 2023 02:37:17 +0000 Message-ID: Date: Fri, 27 Jan 2023 18:37:13 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup mode To: Jan Bobek CC: "devel@edk2.groups.io" , "jiewen.yao@intel.com" , Sean Brogan , Laszlo Ersek References: <20230120225835.42733-1-jbobek@nvidia.com> <87bkmmxpkr.fsf@nvidia.com> <878rhny6pf.fsf@nvidia.com> From: "Sean" In-Reply-To: <878rhny6pf.fsf@nvidia.com> X-TMN: [lOeEO3t7pJDyC4NllY6MetjBDC9nu/Hz] X-ClientProxiedBy: MW4PR03CA0207.namprd03.prod.outlook.com (2603:10b6:303:b8::32) To SA1PR19MB4911.namprd19.prod.outlook.com (2603:10b6:806:185::10) Return-Path: spbrogan@outlook.com X-Microsoft-Original-Message-ID: MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: SA1PR19MB4911:EE_|CH3PR19MB7212:EE_ X-MS-Office365-Filtering-Correlation-Id: 767535f6-584c-474a-64da-08db00d89221 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: FlFD75cqW7LU63ErImrhWAWGEHZel0itd1kWAIPuY1urVtoiTFC8+XpqomB4sJKZ0+0JLFuBWFrOijJEpmMO7Z65je7Td2npK5emtXU6822naKRR3IxmEpbF8Ay3NWdBh9d8MwPkX9cul9l7xlooc531m3dkSWwVqMLH4NTq34rtEMu9vWMUAJZS1QG43r8M/SBkE5S7ctfj054N4zOr2LQ3NhjgbBF7ynp0NNI5uW9621vcjqDN1CHgck03Okf9msR0Gy3MWD9cNANG8rOWk9U77uZVCA30/jsolOLCYg1PW+sSVlq4Dj25AbhcI5bRP6UfgGrbE2/IZ7W6M9unO3bWomO6BIs5lYgS96BM7AF/g1GkhqVVcsjA+QqYeeJ8oqfMToHzycKAN5D81IUzDO85BoCqI2tpdv4Jq5tRqW9Y8ob40GG0kAYlUTaR5RCAhssG7X/g2HYFK8q5Lp09CT3YXpFg9yoKZZ2KVqKpy0B4ETEUIZoQHGUxWtcU2TBn0qBx5KxwEIeIw8j8jncASW60CAvxkSYJGhfJhJfbqatls3y4s2i3rxoHv83k+BE0Z9eUOEa0IW2cuEomPoYflWgRHc6vBm0GrnLbh6uNp2/azGkyIx0APlBDplEYq9QFcJ0eUu+Y1rl0UOkFOsa0Gg== X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?600Ty6lpn1DeCUI7WRlzcJWQJ1+VKRHoiBEkqVtv/UGa3Hj2qm43epHXzSgN?= =?us-ascii?Q?yOJ5HcgQiBhKNZOsSLY7fFfT8yseC20bo5Z4927I7Gtu6jMYav8tZeH1YRWp?= =?us-ascii?Q?z47SvauIxsjSBZeSDDkJBrig479eKPf+291acp6mQAsJBrpYihJb4R7g3QgQ?= =?us-ascii?Q?50tk7kDXlUU2WNsh3BZve6F5+kmEP9tDaVr+qvhJSFiIFuJVuNM65SzY4RrA?= =?us-ascii?Q?wq+D4yy7+pAgZ5MU67V2Sa6bERSb+PL6vWTDYhw2AazJoXM1g00UEdvbfPkW?= =?us-ascii?Q?TUI1z9v3lYSwanBoC2HCPx9UP3pdA42DTIVw6f+DfALqKTug/a4bUQnpsUX1?= =?us-ascii?Q?ISnD+pRp4qyy4NoQo1zm+PnElH+Ug8/YmljGsciURHKYMmt1VEkkHMqY3aIS?= =?us-ascii?Q?5G9OosC1xE+5Q2FLYpb0ndSCONxKHCXRZO+J9pM+7eo8X4AAKv29wbFi4Xzj?= =?us-ascii?Q?vLpOs2U9nK2HVdZVRP8WNAF8kEyIRmtr9nhagMIfmiLDtT/NqBukv6Q0+uor?= =?us-ascii?Q?9njoiUVqKEmOrQUd0o6QRRi0WJhZBhvFyEnnxswa5rGwVItU5LJ26CB1Bvup?= =?us-ascii?Q?G9H9Z3tVr1s2pgYecFYCRm5cpoS3caCdBj8q9JcGbWUwzgjBQnE+rdKNeMbg?= =?us-ascii?Q?zgrj+WH1Fol3Emba1RfhtSgczpwyTgHR2080Gb8TxzRO6tQYJV4jDFBCd0cQ?= =?us-ascii?Q?gfOOLxQtF9z18+fcVrJjPDKCDWtFcShFshM/rorzQhLSMHbW5TR0qNdJr+s+?= =?us-ascii?Q?6zmjH/KgYG+NWEHW7aO/GXiXhh/WttqlNecCtJmCTjpZYS+0+ZFxoff7jZxb?= =?us-ascii?Q?htvkzvOdciFMaHccrU+baO2Vr+58iJlQAYf0I0Un/vtEX6ZZQt9fsGUNgquB?= =?us-ascii?Q?6v5E0J7gLWpws9ZMcbTWQLU/wZrAi8JZWI1M1wS9Xsq6ZOpFwU1gV4bHJ2R2?= =?us-ascii?Q?jzUPgUGTnJuzIIbbfHfWss91LrCoJv3YSLmVQc17WSEBgy6ML46S3W98T+es?= =?us-ascii?Q?2hvVB4ewRybX8QNihHwwCp4g8yTOPOiDn7YjxWLalWFz7FsnF8Fgn1TCOoD0?= =?us-ascii?Q?40gagqUIhPN9BeHorh2R9oLof9F+W7h6J3EAG+mKGCxcz6dxv8eotb2LUA3C?= =?us-ascii?Q?oJKE5u5at5MwT4MjnbmBG9Uxgu/y0fMEXOGfJz/HWvw0n36g4xVier7KzPEf?= =?us-ascii?Q?3h6jsYltodpZHz7vnu+vPmfgM++Wv550ayGSY5f2ryKlpoKWWSKN56arPDRV?= =?us-ascii?Q?OpXrrDcy3a8aQSRWJRLka6j4pObIgC7FlkvCOlrU3w=3D=3D?= X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 767535f6-584c-474a-64da-08db00d89221 X-MS-Exchange-CrossTenant-AuthSource: SA1PR19MB4911.namprd19.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jan 2023 02:37:17.1716 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR19MB7212 Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Did i mention how much i hate reviewing code over email?=C2=A0 :) Even after looking at it a few times I missed that change. I think this change looks fine.=C2=A0 Personally, i think this code has=20 terrible readability and high complexity and with high impact.=C2=A0 It wou= ld=20 be great to at least get unit tests if not a full refactor of the=20 library.=C2=A0 I also think the edk2 idea of "custom mode" should be=20 removed.=C2=A0 But that feedback isn't for this patch and I don't think thi= s=20 patch should be held up just because the surrounding code is less than=20 ideal. For patch 1 and 4. Reviewed-by: Sean Brogan Thanks Sean On 1/27/2023 2:05 PM, Jan Bobek wrote: >> I read your replacement a little different. >> >> - if ((InCustomMode () && UserPhysicalPresent ()) || ((mPlatformMode = =3D=3D SETUP_MODE) && !IsPk)) { >> >> with >> >> + if ( (InCustomMode () && UserPhysicalPresent ()) >> + || ( (mPlatformMode =3D=3D SETUP_MODE) >> + && !(FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk))) >> + { >> >> >> In the upper part you replaced it says !IsPk. What am i missing? >> >> >> If a payload was in this function targeting a KEK change with no PK >> installed it would go in the original IF condition. In the new code >> it would because device is not in custom mode and the payload is not >> targeted PK. > Is it possible that you're missing the negation (i.e. exclamation mark) > in front of the parethesis? The old code was > > !IsPk > > The new code is > > !(FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk) > > If IsPk is FALSE, both of these evaluate to TRUE no matter what the PCD > is. > > -Jan > >> On 1/25/2023 1:38 PM, Jan Bobek wrote: >>> Hi Sean, >>> >>>> From looking over the patch 1/4 email i have a concern. >>>> >>>> In AuthService.c on the conditional change you made. Aren't we losing >>>> a case where we are evaluating a nonPK payload signed by the PK? >>>> Given the system is in SetupMode that means there is no PK but is this >>>> the conditional path that is used when installing Secure boot keys in >>>> reverse (DBX,DX,KEK,PK) order? >>> Thanks for sharing your concern! They way I think about the change is >>> that I've replaced the expression >>> >>> IsPk >>> >>> with >>> >>> FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk >>> >>> and nothing else. When you look at it this way, it's fairly easy to >>> break down how it affects the logic: >>> >>> 1. Assume PcdRequireSelfSignedPk is TRUE. In this case, the two >>> expressions are exactly the same and no change in behavior occurs, >>> just as we want. >>> >>> 2. Assume IsPk is FALSE. In this case, both expressions evaluate to >>> FALSE, no matter what the PCD is configured to. That's also good, >>> because we don't want to change how non-PK payloads are handled. >>> >>> 3. In fact, the only change in behavior that occurs is when >>> PcdRequireSelfSignedPk is FALSE and IsPk is TRUE; here the former >>> expression would be TRUE, whereas the latter is FALSE. That's exac= tly >>> what we want: we wish to enter the first block of the if-else bran= ch >>> (which changes the variable similarly to when we're in custom mode= ) >>> rather than falling through to the third block (which checks the >>> self-signature). >>> >>> To directly answer your question, I don't think the behavior changes at >>> all when processing non-PK payloads, by virtue of IsPk being FALSE and >>> what I said in point (2.) above. >>> >>> Additionally, yes, the first block of the if-else branch is exactly the >>> path that's taken when enrolling KEK/DB/DBX in Setup mode, and one that >>> has always been available even without Custom mode. It used to be that >>> you couldn't use this path to enroll PK without Custom mode (precisely >>> because of !IsPk in the condition), but I'm hoping to enable this path >>> with my patch. >>> >>> Let me know if I haven't answered or misunderstood your question. >>> >>>> Is there testing you have done? This code should be pretty easy to do >>>> host based unit testing on. Any chance you have authored that to >>>> confirm use cases are not unexpectedly impacted? Example of host >>>> based unit test of library is here: >>>> edk2/SecurityPkg/Library/SecureBootVariableLib/UnitTest at master =C2= =B7 >>>> tianocore/edk2 >>>> (github.com) >>> I've done an ad-hoc test by commenting out the switch to/from Custom >>> mode in EnrollFromDefaultKeysApp from SecurityPkg, booting in Setup mod= e >>> and using the modified app to enroll the keys. You could do a similar >>> test from the OS, but in my case this was more straightforward. >>> >>> I am aware of the host-based unit testing library in edk2, and I agree >>> that it would be great to have the code in AuthVariableLib tested for >>> all the different cases. However, I don't have any such tests right now= , >>> and while I'm willing to potentially look into writing some, I'd have t= o >>> do it more or less on the side, meaning it could take a while. >>> >>> Best, >>> -Jan >>> >>>> On 1/22/2023 10:13 PM, Yao, Jiewen wrote: >>>> >>>> Hi Sean >>>> I would like to hear your feedback, since it is a little different fro= m the original MSFT patch. >>>> >>>> Would you please take a look? >>>> >>>> Thank you >>>> Yao, Jiewen >>>> >>>> >>>> >>>> -----Original Message----- >>>> From: Jan Bobek >>>> Sent: Saturday, January 21, 2023 6:59 AM >>>> To: devel@edk2.groups.io >>>> Cc: Jan Bobek ; Laszlo Er= sek ; Yao, >>>> Jiewen >>>> Subject: [PATCH v1 0/4] Don't require self-signed PK in setup mode >>>> >>>> Hi all, >>>> >>>> I'm sending out v1 of my patch series that addresses a UEFI spec >>>> non-compliance when enrolling PK in setup mode. Additional info can be >>>> found in bugzilla [1]; the changes are split into 4 patches as >>>> suggested by Laszlo Ersek in comment #4. >>>> >>>> I've based my work on the patch by Matthew Carlson; I've credited him >>>> with co-authorship of the first patch even though in the end I decided >>>> to do the implementation a bit differently. >>>> >>>> Comments & reviews welcome! >>>> >>>> Cheers, >>>> -Jan >>>> >>>> References: >>>> 1. https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2= Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2506&data=3D05%7C01%7Cjbobek%= 40nvidia.com%7Cf2eff15ce44945cc0b4e08db00ad6625%7C43083d15727340c1b7db39efd= 9ccc17a%7C0%7C0%7C638104516992386171%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj= AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata= =3DPXam5BVxMmUwgRdGsq1RcnwC8yb8nmOzcLz3oKica7s%3D&reserved=3D0 >>>> >>>> Jan Bobek (4): >>>> SecurityPkg: limit verification of enrolled PK in setup mode >>>> OvmfPkg: require self-signed PK when secure boot is enabled >>>> ArmVirtPkg: require self-signed PK when secure boot is enabled >>>> SecurityPkg: don't require PK to be self-signed by default >>>> >>>> SecurityPkg/SecurityPkg.dec | 7 +++++++ >>>> ArmVirtPkg/ArmVirtCloudHv.dsc | 4 ++++ >>>> ArmVirtPkg/ArmVirtQemu.dsc | 4 ++++ >>>> ArmVirtPkg/ArmVirtQemuKernel.dsc | 4 ++++ >>>> OvmfPkg/Bhyve/BhyveX64.dsc | 3 +++ >>>> OvmfPkg/CloudHv/CloudHvX64.dsc | 3 +++ >>>> OvmfPkg/IntelTdx/IntelTdxX64.dsc | 3 +++ >>>> OvmfPkg/Microvm/MicrovmX64.dsc | 3 +++ >>>> OvmfPkg/OvmfPkgIa32.dsc | 3 +++ >>>> OvmfPkg/OvmfPkgIa32X64.dsc | 3 +++ >>>> OvmfPkg/OvmfPkgX64.dsc | 3 +++ >>>> SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf | 3 +++ >>>> SecurityPkg/Library/AuthVariableLib/AuthService.c | 9 +++++++-- >>>> 13 files changed, 50 insertions(+), 2 deletions(-)