From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-CO1-obe.outbound.protection.outlook.com (NAM11-CO1-obe.outbound.protection.outlook.com [40.92.18.97]) by mx.groups.io with SMTP id smtpd.web10.60.1674854894473305443 for ; Fri, 27 Jan 2023 13:28:14 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@outlook.com header.s=selector1 header.b=mNECsMUk; spf=pass (domain: outlook.com, ip: 40.92.18.97, mailfrom: spbrogan@outlook.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=I2p1nzOfSWIjNf8mI9PEAGLPtlhEeeqpMMgDsANZBhi2Vx9ablUoa0993aWHiTDB7k5pSudJ0jN9ocWKqtvKdXCcTemAafutvebhAo3NXUXCoCDWG8cIyTDNegY4kU4K9KVnpoCAPnPP9JpJhGK7b4NP6MrpXRnuZQe+8wKLoEk/LSgiu6kuGpbZoSPNc5QHd4ilku1//knkyae8V6ogBW8IUB5NbHfNfWXL0isM6OiKTHlAfgB2uFIpzQ92M/skmMeddYhCifi4jwmtDAHqMLAxDcOpw7AjWI6bXjN47O7Hm5aHomVr11n3ThAJhhjX0+iWalWxEchOSfOpL0kKmQ== 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=Hg0X2WvQXnOl4lbfoF4KDtB7deqqAF1jnFjayR5Buw4=; b=ntEDtgOYcmLFxTR1cg2EYwG9NBrWwaA6rqkc4VHDba71ZtWqES3YO1790MTzArHEEm89sQ79WfpPhXAg1cEYW0xWHIJBL8PUZVftPVewHWFtyYDlKD6QkbKfazSoFXvO5HQ3nkaxLHnpQNruwW+iAO2pTexkUIxSmNg/+HE4xv9uuUsOv2A7tbrlplDmMkmnWq4aUd0RNfl2bsUWYOBR69zb78tifuDeppFZe2LAaUlftoAVkPDR/Fza6uY9LfFQpX2ay1yLTKV1bl6zlGyddjfiziQfQMU5wn0QrUz3ZYNLvK8TLDX3NUGw9Ql6/oJjKjlyOW16flQ8kczBfffYkQ== 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=Hg0X2WvQXnOl4lbfoF4KDtB7deqqAF1jnFjayR5Buw4=; b=mNECsMUkyYTnPk0HwH6BYsLFtFgyUzsRaB4QOA0Lysyua+Lw4DZZSxRadcFoZqsvn68ErN4S/I7U+F8RKJ2xUzUMNjk2JxrrXJgDm7kyQWkvgZXHEA2MxxeYWob0Dprp0QfkDCVOuuooNm6RUVxFHDxAHImDliRXMYBITn51oS2EgzqCvXXxLoH77TbUrFAC/mXEUO0yzIZdqooQ9+lSLl+dyNX7N8dXtZpoySCtmi/NUIFA+Ye6L6H4YEiVup4LcU68S4cHBeTDetAampNKhAzLjNyZIjatiGARPVa4OJ/hatyYFzS0pzyBq1Y7HWBOZOVbECmFHb0nBU21tJWKzw== Received: from BY3PR19MB4900.namprd19.prod.outlook.com (2603:10b6:a03:354::11) by MN2PR19MB3854.namprd19.prod.outlook.com (2603:10b6:208:1ea::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6043.22; Fri, 27 Jan 2023 21:28:12 +0000 Received: from BY3PR19MB4900.namprd19.prod.outlook.com ([fe80::dbe5:112f:db31:2409]) by BY3PR19MB4900.namprd19.prod.outlook.com ([fe80::dbe5:112f:db31:2409%4]) with mapi id 15.20.6043.022; Fri, 27 Jan 2023 21:28:11 +0000 Message-ID: Date: Fri, 27 Jan 2023 13:28:09 -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> From: "Sean" In-Reply-To: <87bkmmxpkr.fsf@nvidia.com> X-TMN: [dJwnYsY10ZgXTG/ytW/GIEtaMKJlPuWZ] X-ClientProxiedBy: MW4PR04CA0389.namprd04.prod.outlook.com (2603:10b6:303:81::34) To BY3PR19MB4900.namprd19.prod.outlook.com (2603:10b6:a03:354::11) 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: BY3PR19MB4900:EE_|MN2PR19MB3854:EE_ X-MS-Office365-Filtering-Correlation-Id: ab1b638e-9b90-40c9-3258-08db00ad641d X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: cVYiCbqQCLut0KJhlk7oednpQk3q07QB+XtjE1NOqzEPbly/tjCXbjxmWmu589Ro44Qe2o16VknpTPOwyGRLyPDZMvik52SlpOp26FKaHmdiqxiUr6SAeCttsmsTX5lw1f7QoskrSoZ4nFxxSwjF58GQ392jiuxfPk+Y8Su/yvC5KKffooJ0VH7l0BJralpmixpXnSTmvBDu6SseGE/Pb/uUynY9qcWpDgeEFeUVBA7aKijU7FLammcTb5f7maZZhswB4nniiwWuStsscxFRgI7O0xbWHejxxXewAyQwBdVHV/qABpR2dXkq5qU26CC/v9hgdVTdI4AXyl9vvjC3gpdkVlihVprLe6gFnuQwCxM6NNm9bQtn+w0gb10butfXFCjkG9gEAMoEvOJT689E6MK7rsRWFlE/JL1jAkMstdyRZQHrOpx7aGwLQie0NULHVs62Ms+T9QVjGx/JZoQErRpqqq89Rd1ZxBT04EZsmSbABmPw2/wBTwvDEUQJwfVC1rfHpFHsViPyEOMZCibV+Y0T1dY1izvX/NvBp6nHw1iPV974Z6us5ouMgUpREG0d/2mXGi0eWsQ8WiY6IguxmOBI8/mKWtbq/KhihAxNkcHwAYiaWedDhj2ePuV4PpIKQDJLOKdeZ10n7fRbkJ1RxQ== X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?LAAOe3yZzKwkk/A+Dm8NMUTxDbMqRKp8tmTRP6Mcy6q1tBoQFC2Ll6cYTn7O?= =?us-ascii?Q?+zLcz+5sT1ZF7WWGibaBiEIWX7KGehXFau3KgU85cxkzlBccySJaH/gKpWrU?= =?us-ascii?Q?ynhGEDQ2T05GNeNHi4b/idqQV2t/Yj0rpsBBHFe5jUtOOrrk5v4bDX4ZESru?= =?us-ascii?Q?lK3faGgbl9DD3l/h30yVB9uixSveuquZI8YZg6ludXs7meHz6j8Ah/qMoh+C?= =?us-ascii?Q?lWQsPR/scHahyIsD+5/AavrCxsHv54HfCD58hJUwL7Too9LFqPo9buvf4C40?= =?us-ascii?Q?T0ChYuBX2GILglVK1QAR9bcn9ZCiu5gBxtaWM3tTEhfAEdB13JBxIZiFye3C?= =?us-ascii?Q?9/ZfPP5In7kf3vHkgDFCQ1KlVPHWeQ82L8RjmxiV5yemWpmVc3v8clfB5Q4R?= =?us-ascii?Q?Ucvg59/ldPudCgpnHolnJx29yiMHb3YAML9QiiDPD+GWvKlh0w0JxTkPMZW+?= =?us-ascii?Q?p1G2pmaBUrSOuUyHMKFz1FHkXLA/tV+va/5a7rp0NGxjFEyDpbjLOXlblLhT?= =?us-ascii?Q?Y8CJDvMiYVvQqU1HSatUG/UG+Sc+5ky+qF8lH69XK7JvcEs7fWRZnDMKb/sN?= =?us-ascii?Q?lTMXrj/jMQxG2BVjDEVuXla5/hCBYhgNFITvY/QXkOQpw9+WFmrP/CitysvH?= =?us-ascii?Q?vf5HmT7v3ePORNGTC7uYUdVLCM91DJKuVIcfJF8C58kb6HPxUBoNC6xzbBKm?= =?us-ascii?Q?CDPV58fzNjZdiOM1no6FY336eWCv4Fj1XUSqBqfFONKRnMZQz0pgSaToJpBq?= =?us-ascii?Q?MpzojpZhNCB7R5eLfq0KLLAJfQy5mDqV/gr7ydp9bkmF8dBfWAEouvxRfkDX?= =?us-ascii?Q?8s/3szMvWtgYqiEUPbSytf1yaqWUpHRV3IAwREBXWC+ToRIIPyMkurufNNMs?= =?us-ascii?Q?svC66cE6KGor0yQTT6tyj/dKr7qlAeQSkbbAy4JzLc53DX3buCRYwWoICDMN?= =?us-ascii?Q?GxVyJ2YLyBktVnC9iH7WDeago1aMQhNeaSJXKdF571CyxauyBbws5YLk/r3l?= =?us-ascii?Q?yiMkHdpF7a6BhN2SneMXybvvtsyDYrQCoBSSn4QPXc4OcofARDjqG4txc8Hb?= =?us-ascii?Q?rPiXY7jbbS9SXhTv+Yk5X8o21Rs1wrrND8Sa+/+cf7hLKkGtvvEjZ689yOdX?= =?us-ascii?Q?8OkebNybRWcdTrkytTIpeM2bXduL5fYzDkvetAZWHugZ4l57Nz6F6h/Wo1Um?= =?us-ascii?Q?xVbFgw+eyqmwj44/JwgzEsqMQ5kaBXMDOBEWkZ/O0dQ1pJSbpD1S0cnPhygx?= =?us-ascii?Q?3O7050aciGGMYNhCw8wlfgEdt9qzjG8GleY5htmi7Q=3D=3D?= X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: ab1b638e-9b90-40c9-3258-08db00ad641d X-MS-Exchange-CrossTenant-AuthSource: BY3PR19MB4900.namprd19.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Jan 2023 21:28:11.7345 (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: MN2PR19MB3854 Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable 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 install= ed it would go in the original IF condition. In the new code it would because device is not in custom mode and the payl= oad is not targeted PK. Thanks Sean =20 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 exactly > what we want: we wish to enter the first block of the if-else branch > (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 mode > 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 to > 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 from = 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 Erse= k ; 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://bugzilla.tianocore.org/show_bug.cgi?id=3D2506 >> >> 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(-)