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.106]) by mx.groups.io with SMTP id smtpd.web11.39001.1674625901928840458 for ; Tue, 24 Jan 2023 21:51:42 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@outlook.com header.s=selector1 header.b=o6DyBYrC; spf=pass (domain: outlook.com, ip: 40.92.42.106, mailfrom: spbrogan@outlook.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Vk8Jntq4r3NgWiJdvTx1M2rrzjt0O23ghHC8z8sbMC7s8NnfDwuuSlotgIsIzVH7uZVxnPsRcyyNS6Ei3Yv9Qf4q6qvRypg47SdhfdA9j8JAKFGuWDoUhXY4isT2fNDc7NkKA9YFBrDfyH8XdkMobkYFheDNJTE+TCMZ8Su/JWMzjcgn6lr0o4XVBduxeXWYvYS1KNzWOhOaOQlhhtW4iB5PQrCbC0ciho6z5nQbadsRTvztphs2IRWjtJoJvQDPe8E1cdfliS9h7b2dn1VGDdavO5C0hxnR6dnZ4Mnhw3Lxa11YNoAoX3qeieYHiLuAkbh7drxfs2Ecmo5rGGAc0Q== 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=tSGZHEsMYb6Z1eeGsxFZ2iGpeex4oZEShOL8OkCQmyA=; b=IILE2c9PkVltDQhiZvOaO8hw8i3HOY5tVkJ8R5Dkhr5pvoeSnU7lfszCMsTpNGpRTRtTccsHzPLjaCcFAK8HSwIiKQnWpsaqkRpdEvHcKfMcQoiI4wvbekTqyaCrZs7WbVs7+FXM0X+pqQ+BWYzhDltr6pNgzMGwPEPalBV/+EeCFuAzY7C9Kl2UavLQm3DEVxnW61hW3tuvrpTBf8hJgaN62yCXWKKxywBZUepyrcrD6FO10M8VO9w8ZaQCZ/Zuf3k0r8b9b7lHHhAMrXRDuVvaboFO73n8XGvb36SfmRUCjPxjHjrEn3aQt64n7Cmwv9SWjA1oL7BA4N9uq0qGSQ== 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=tSGZHEsMYb6Z1eeGsxFZ2iGpeex4oZEShOL8OkCQmyA=; b=o6DyBYrCWsboovEIquwBDWg/nWxFwuaB7SjdoKME2rXNyuEMju1XVNpJKcX0dMh1UNRcBeYSQkPoFGC3Tgyk10M7uad3G2RTSc4l/8IWxFDHaS1CQma2PtqWgevDl74fX/SayYdNQ1GGw+1rgTgP9JUg9oBtW6fJCsXZ30Q7qYwmkzIqm6jeKQAFDjHZatY+zKcibPTQYbGogvlWjzwptelNwPhh5/i2TDqKExRQjjl2Zu2vlyA3INjvDEWxi2x2s5i1G48u/VWfyYNPmqMOBRTS9GpHZENNi3Bya/ikKCgoSlRBg/SZq4qtupUUFuvBK+m+jAFAWd46binHJu1Agw== Received: from BY3PR19MB4900.namprd19.prod.outlook.com (2603:10b6:a03:354::11) by BLAPR19MB4529.namprd19.prod.outlook.com (2603:10b6:208:29a::10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6043.21; Wed, 25 Jan 2023 05:51:40 +0000 Received: from BY3PR19MB4900.namprd19.prod.outlook.com ([fe80::dbe5:112f:db31:2409]) by BY3PR19MB4900.namprd19.prod.outlook.com ([fe80::dbe5:112f:db31:2409%5]) with mapi id 15.20.6002.033; Wed, 25 Jan 2023 05:51:40 +0000 Message-ID: Date: Tue, 24 Jan 2023 21:51:37 -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: "devel@edk2.groups.io" , "jiewen.yao@intel.com" , Jan Bobek , Sean Brogan Cc: Laszlo Ersek References: <20230120225835.42733-1-jbobek@nvidia.com> From: "Sean" In-Reply-To: X-TMN: [7kQ/ko+qP9qTCurzx/Tc5pklraz+9YiM] X-ClientProxiedBy: MW2PR16CA0055.namprd16.prod.outlook.com (2603:10b6:907:1::32) To BY3PR19MB4900.namprd19.prod.outlook.com (2603:10b6:a03:354::11) Return-Path: spbrogan@outlook.com X-Microsoft-Original-Message-ID: <12c833ee-c42c-fae3-63e9-f80b6948cf6a@outlook.com> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: BY3PR19MB4900:EE_|BLAPR19MB4529:EE_ X-MS-Office365-Filtering-Correlation-Id: fac41caf-c1ba-498f-98ff-08dafe983a5d X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: NWej30/fYP5hcdrDcfSFzu3O6sCbGZt/IPcSNRmn73/jo7Z0TXHkgCqcBzj6uPkRnMRa2b2B7m9xqdPB24Cjqk8WgIfzVL+rJE6a9W5K4ik0p7B1c3rAdeNW8+DC89ubCFxzWlntnvJC3UFecvYvJpHCwinkLN/PSCIeoO49H8aqgbLRFLAUGIUap2oHO8u9dOIrdggkmAFRIhX0wnf5U0hhZUXPDHF4419keT9PN/yw2Vf4BRAuVsAGPLbBr+vrSo91LwktlqWojiexc4n3zfj7C8F9wJEnum7RTMPkUbssr3LdKiAbVpkrjgrVhyjD9tffa80AaWcj6vRnTOfR45/KgVm7CGskUEATEurv9XAznXuWRy8EsVVloYat1Gcq3dmRRTtJFcIAob19rZlZsEd9Z/YNGer6SsUT/C2/gIQUJ3cWym5y/Rle8TYJisTkdARRTfJjOmHg8PcYq3bn9vj8A0Wx8+jfHnpAU0NNJovsJtuJmKhu4FxhKRiInh3Zq17qZkeve7ND9c7QMWdNiCB+p9SWslMyDRw+F2V8dgxmP/57X/DStzXG8b9PWesD5K8knE/ovs6Rwt0vHFqsoQpFQ2UlkAhb6Pp/TQTtCHun5HLqBrAuoFNwHiAinOarp+cS02H/mQpjuIsPHMfxrn+lY+cu8WMqzoOBzTqSEbEbD9qYASj5zoarnkJ9aHPwIeRLf7okQAaavFoi/54s0Q== X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TDM1ZkJSWm8vcnJqcnJBVkpwUlJjT3piTmhsZXcvQzA2dUlSU3J3YWlsK1cv?= =?utf-8?B?cXU3bVEvMnhYQ01VSkxTajIxWGFYVTN2OFg0WWtBRm01UVRMTUZYd0RTRkJQ?= =?utf-8?B?WTJNN214dHZIdXRRNEsrVzRvQ3JmVEdCUXNML3hvSFA3V050TU56bUNuQVhH?= =?utf-8?B?S3RaNzAySFU1L1I1NVVsVXQ0YWdqc29Kc3NwYlJpbU1KalZxUnRGWUhsc1Ar?= =?utf-8?B?QWhkTmNaWld3amQzNnl6OWNvU3dBQklLV3VPWExZZ2t3M1pIWnk0ZUxuQmZY?= =?utf-8?B?Rzl1RlE3U25UZlhmbVh0R0c2Q0V2QkFXY3pQeUtsQ3pObndIaFZZOHdCVzJR?= =?utf-8?B?U3ZIbjY5U2tqUXUyeEw5Qm96ZmsycllGT01HRkM5SnljOCtwV21MSG9LVlZX?= =?utf-8?B?NExuTGNUaUNCVkNzRnk1YWVTdEgvVkRsRlJsdmVlK1doZlJ1cnJnbVJuSHhV?= =?utf-8?B?QnN3enFuVkNreStaS1lHckFGODlWdU1rcEg3d1BOTElEanBWQlV1U08wTHJ1?= =?utf-8?B?MzdNeTYxaWkvdDNHUXRHOWxObml1c25yYi85TTY2RDMzQ2dNU2dyM0RwaDlq?= =?utf-8?B?TW16TXdHQTRVbWNCSVVzZHo2TXd0dm9veklGSUswZE5XZGdURE1BTUkxdTJJ?= =?utf-8?B?Q211ZWx0Vm9oVkUzbk9Cd2Zyc1VnNWEzaGE5VjRtUkpiUW9ta1hkd3BWbFE0?= =?utf-8?B?NDNxdDk1TlVNYnRVUEFJcVlVTEo3VUdhL2oxWmtQN1QvMHg2dldlZU56anQ4?= =?utf-8?B?TzlqUS8zVEowYmVGSUgvV0NSbHpjRE5QWXBDdDNFN3dWOGZmYy9EaC9EYlpV?= =?utf-8?B?blJieEthdzBrK3dFVmJRc1RqUFphUGo1U0dOYUI3R3ZxZkFVa3hSekdYMk1l?= =?utf-8?B?elJRL05wcGcvMlFVdUszb1RMSmx6TTNSYmxrQTFPc29iQnJNZTQ1UE5QMGk4?= =?utf-8?B?UkJ5MHh0enBwUkl6cUR1UDRWRkhEZ251eS93eXNia0RWSEt2b3JheU1xZnhu?= =?utf-8?B?d3lLbFF5US9neWx5cnNCa0hMZEd3K01LK21LTGtFNkRXa3VvVnE5QVVhTmdY?= =?utf-8?B?UVZTeWZhYUpaZGNuZFBzNks2SkhxUHBqUWpFVGRKa0RVbjBrRHlFdTQxb25L?= =?utf-8?B?K1hzOGV6Ym80UTRTMTJMajMzZFVOOFZubFhJeVZ5MnZRQmwrNlVUTFRlOVY5?= =?utf-8?B?dTcvYTBsbVNFT0E0Znk2d0FiMEl0Rk15RVQwaHFtSjhtb0xIOUtGRHJyVzk4?= =?utf-8?B?RXZyRmNCRVVQWVJ6TlhqcURVcFBDeXU1TC9iQ3dyL29CRHpCT0sxYW5UMnUy?= =?utf-8?B?ejhUenRhaWFYQU5PVmVteUovbit5bFlGWGllb1paWU0veVVaMnFzcUhwTFhO?= =?utf-8?B?dm9OWjZCMWNhUVQyaVgyaHBzRkhDRGZrYjUrUy9MQzZzZCtVT1FneE0vclpC?= =?utf-8?B?SndpTkg4YVBjUCswRTVaTHp3ODA5NjU0YjNMSzU1K3pwdzJBbFBjc2N0RXVt?= =?utf-8?B?Ly8vd0cxLzRNZzhmWFFSRWNoQ0VLSHJDd2JzR29qT2EvRmRKYzZMeWZabDNu?= =?utf-8?B?VlVVbjBodU9VYm95Q2Vnc2N0K3FRWlYyVGdHcG5EQjlPam1sS3l3SUVrK01H?= =?utf-8?B?bmNDYW5zVmJmaWdlVGZsbGc2UW40M0xWdHlKWE1aV3RwR3FWcFdjSXphSFlk?= =?utf-8?B?bTUzTXZIVHRqUW5yNFJjdWFYdnN6S3FoazJKcUhPWGJQNTBNcXFqd1ZRPT0=?= X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: fac41caf-c1ba-498f-98ff-08dafe983a5d X-MS-Exchange-CrossTenant-AuthSource: BY3PR19MB4900.namprd19.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Jan 2023 05:51:40.0760 (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: BLAPR19MB4529 Content-Type: multipart/alternative; boundary="------------5adWPvZfAvfdSCS1RoaUYHs3" Content-Language: en-US --------------5adWPvZfAvfdSCS1RoaUYHs3 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Jan, 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? 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 · tianocore/edk2 (github.com) Thanks Sean 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 Ersek; 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=2506 >> >> 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(-) >> >> -- >> 2.30.2 > > > > > --------------5adWPvZfAvfdSCS1RoaUYHs3 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Jan,

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?

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 · tianocore/edk2 (github.com)


Thanks

Sean




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 <jbobek@nvidia.com>
Sent: Saturday, January 21, 2023 6:59 AM
To: devel@edk2.groups.io
Cc: Jan Bobek <jbobek@nvidia.com>; Laszlo Ersek <lersek@redhat.com>; Yao,
Jiewen <jiewen.yao@intel.com>
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=2506

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(-)

--
2.30.2
--------------5adWPvZfAvfdSCS1RoaUYHs3--