public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v2 1/1] SecurityPkg/AuthVariableLib: Check SHA-256 OID with ContentInfo present
@ 2023-01-22 21:53 Jan Bobek
  2023-01-23  2:37 ` Yao, Jiewen
  0 siblings, 1 reply; 3+ messages in thread
From: Jan Bobek @ 2023-01-22 21:53 UTC (permalink / raw)
  To: devel; +Cc: Jan Bobek, Jiewen Yao, Jian J Wang, Min Xu

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4305

Based on whether the DER-encoded ContentInfo structure is present in
authenticated SetVariable payload or not, the SHA-256 OID can be
located at different places.

UEFI specification explicitly states the driver shall support both
cases, but the old code assumed ContentInfo was not present and
incorrectly rejected authenticated variable updates when it were
present.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Jan Bobek <jbobek@nvidia.com>
---
 .../Library/AuthVariableLib/AuthService.c     | 50 ++++++++++++++++---
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
index 054ee4d1d988..9beeca09aeba 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
+++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
@@ -1925,7 +1925,7 @@ VerifyTimeBasedPayload (
   // SignedData.digestAlgorithms shall contain the digest algorithm used when preparing the
   // signature. Only a digest algorithm of SHA-256 is accepted.
   //
-  //    According to PKCS#7 Definition:
+  //    According to PKCS#7 Definition (https://www.rfc-editor.org/rfc/rfc2315):
   //        SignedData ::= SEQUENCE {
   //            version Version,
   //            digestAlgorithms DigestAlgorithmIdentifiers,
@@ -1933,15 +1933,49 @@ VerifyTimeBasedPayload (
   //            .... }
   //    The DigestAlgorithmIdentifiers can be used to determine the hash algorithm
   //    in VARIABLE_AUTHENTICATION_2 descriptor.
-  //    This field has the fixed offset (+13) and be calculated based on two bytes of length encoding.
+  //    This field has the fixed offset (+13) or (+32) based on whether the DER-encoded
+  //    ContentInfo structure is present or not, and can be calculated based on two
+  //    bytes of length encoding.
+  //
+  //    Both condition can be handled in WrapPkcs7Data() in CryptPkcs7VerifyCommon.c.
+  //
+  //    See below examples:
+  //
+  // 1. Without ContentInfo
+  //    30 82 0c da // SEQUENCE (5 element) (3294 BYTES) -- SignedData
+  //       02 01 01 // INTEGER 1 -- Version
+  //       31 0f // SET (1 element) (15 BYTES) -- DigestAlgorithmIdentifiers
+  //          30 0d // SEQUENCE (2 element) (13 BYTES) -- AlgorithmIdentifier
+  //             06 09 // OBJECT-IDENTIFIER (9 BYTES) -- algorithm
+  //                60 86 48 01 65 03 04 02 01 // sha256 [2.16.840.1.101.3.4.2.1]
+  //             05 00 // NULL (0 BYTES) -- parameters
+  //
+  // Example from: https://uefi.org/revocationlistfile
+  //
+  // 2. With ContentInfo
+  //    30 82 05 90 // SEQUENCE (1424 BYTES) -- ContentInfo
+  //       06 09 // OBJECT-IDENTIFIER (9 BYTES) -- ContentType
+  //          2a 86 48 86 f7 0d 01 07 02 // signedData [1.2.840.113549.1.7.2]
+  //       a0 82 05 81 // CONTEXT-SPECIFIC CONSTRUCTED TAG 0 (1409 BYTES) -- content
+  //          30 82 05 7d // SEQUENCE (1405 BYTES) -- SignedData
+  //             02 01 01 // INTEGER 1 -- Version
+  //             31 0f // SET (1 element) (15 BYTES) -- DigestAlgorithmIdentifiers
+  //                30 0d // SEQUENCE (13 BYTES) -- AlgorithmIdentifier
+  //                   06 09 // OBJECT-IDENTIFIER (9 BYTES) -- algorithm
+  //                      60 86 48 01 65 03 04 02 01 // sha256 [2.16.840.1.101.3.4.2.1]
+  //                   05 00 // NULL (0 BYTES) -- parameters
+  //
+  // Example generated with: https://wiki.archlinux.org/title/Unified_Extensible_Firmware_Interface/Secure_Boot#Manual_process
   //
   if ((Attributes & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) != 0) {
-    if (SigDataSize >= (13 + sizeof (mSha256OidValue))) {
-      if (((*(SigData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE) ||
-          (CompareMem (SigData + 13, &mSha256OidValue, sizeof (mSha256OidValue)) != 0))
-      {
-        return EFI_SECURITY_VIOLATION;
-      }
+    if (  (  (SigDataSize >= (13 + sizeof (mSha256OidValue)))
+          && (  ((*(SigData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE)
+             || (CompareMem (SigData + 13, &mSha256OidValue, sizeof (mSha256OidValue)) != 0)))
+       && (  (SigDataSize >= (32 + sizeof (mSha256OidValue)))
+          && (  ((*(SigData + 20) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE)
+             || (CompareMem (SigData + 32, &mSha256OidValue, sizeof (mSha256OidValue)) != 0))))
+    {
+      return EFI_SECURITY_VIOLATION;
     }
   }
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v2 1/1] SecurityPkg/AuthVariableLib: Check SHA-256 OID with ContentInfo present
  2023-01-22 21:53 [PATCH v2 1/1] SecurityPkg/AuthVariableLib: Check SHA-256 OID with ContentInfo present Jan Bobek
@ 2023-01-23  2:37 ` Yao, Jiewen
  2023-01-23  6:06   ` Yao, Jiewen
  0 siblings, 1 reply; 3+ messages in thread
From: Yao, Jiewen @ 2023-01-23  2:37 UTC (permalink / raw)
  To: Jan Bobek, devel@edk2.groups.io; +Cc: Jan Bobek, Wang, Jian J, Xu, Min M

[-- Attachment #1: Type: text/plain, Size: 5077 bytes --]

reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
________________________________
发件人: Jan Bobek <jbobek@nvidia.com>
发送时间: Monday, January 23, 2023 5:53:48 AM
收件人: devel@edk2.groups.io <devel@edk2.groups.io>
抄送: Jan Bobek <jbobek@nvidia.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Xu, Min M <min.m.xu@intel.com>
主题: [PATCH v2 1/1] SecurityPkg/AuthVariableLib: Check SHA-256 OID with ContentInfo present

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4305

Based on whether the DER-encoded ContentInfo structure is present in
authenticated SetVariable payload or not, the SHA-256 OID can be
located at different places.

UEFI specification explicitly states the driver shall support both
cases, but the old code assumed ContentInfo was not present and
incorrectly rejected authenticated variable updates when it were
present.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Jan Bobek <jbobek@nvidia.com>
---
 .../Library/AuthVariableLib/AuthService.c     | 50 ++++++++++++++++---
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
index 054ee4d1d988..9beeca09aeba 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
+++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
@@ -1925,7 +1925,7 @@ VerifyTimeBasedPayload (
   // SignedData.digestAlgorithms shall contain the digest algorithm used when preparing the
   // signature. Only a digest algorithm of SHA-256 is accepted.
   //
-  //    According to PKCS#7 Definition:
+  //    According to PKCS#7 Definition (https://www.rfc-editor.org/rfc/rfc2315):
   //        SignedData ::= SEQUENCE {
   //            version Version,
   //            digestAlgorithms DigestAlgorithmIdentifiers,
@@ -1933,15 +1933,49 @@ VerifyTimeBasedPayload (
   //            .... }
   //    The DigestAlgorithmIdentifiers can be used to determine the hash algorithm
   //    in VARIABLE_AUTHENTICATION_2 descriptor.
-  //    This field has the fixed offset (+13) and be calculated based on two bytes of length encoding.
+  //    This field has the fixed offset (+13) or (+32) based on whether the DER-encoded
+  //    ContentInfo structure is present or not, and can be calculated based on two
+  //    bytes of length encoding.
+  //
+  //    Both condition can be handled in WrapPkcs7Data() in CryptPkcs7VerifyCommon.c.
+  //
+  //    See below examples:
+  //
+  // 1. Without ContentInfo
+  //    30 82 0c da // SEQUENCE (5 element) (3294 BYTES) -- SignedData
+  //       02 01 01 // INTEGER 1 -- Version
+  //       31 0f // SET (1 element) (15 BYTES) -- DigestAlgorithmIdentifiers
+  //          30 0d // SEQUENCE (2 element) (13 BYTES) -- AlgorithmIdentifier
+  //             06 09 // OBJECT-IDENTIFIER (9 BYTES) -- algorithm
+  //                60 86 48 01 65 03 04 02 01 // sha256 [2.16.840.1.101.3.4.2.1]
+  //             05 00 // NULL (0 BYTES) -- parameters
+  //
+  // Example from: https://uefi.org/revocationlistfile
+  //
+  // 2. With ContentInfo
+  //    30 82 05 90 // SEQUENCE (1424 BYTES) -- ContentInfo
+  //       06 09 // OBJECT-IDENTIFIER (9 BYTES) -- ContentType
+  //          2a 86 48 86 f7 0d 01 07 02 // signedData [1.2.840.113549.1.7.2]
+  //       a0 82 05 81 // CONTEXT-SPECIFIC CONSTRUCTED TAG 0 (1409 BYTES) -- content
+  //          30 82 05 7d // SEQUENCE (1405 BYTES) -- SignedData
+  //             02 01 01 // INTEGER 1 -- Version
+  //             31 0f // SET (1 element) (15 BYTES) -- DigestAlgorithmIdentifiers
+  //                30 0d // SEQUENCE (13 BYTES) -- AlgorithmIdentifier
+  //                   06 09 // OBJECT-IDENTIFIER (9 BYTES) -- algorithm
+  //                      60 86 48 01 65 03 04 02 01 // sha256 [2.16.840.1.101.3.4.2.1]
+  //                   05 00 // NULL (0 BYTES) -- parameters
+  //
+  // Example generated with: https://wiki.archlinux.org/title/Unified_Extensible_Firmware_Interface/Secure_Boot#Manual_process
   //
   if ((Attributes & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) != 0) {
-    if (SigDataSize >= (13 + sizeof (mSha256OidValue))) {
-      if (((*(SigData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE) ||
-          (CompareMem (SigData + 13, &mSha256OidValue, sizeof (mSha256OidValue)) != 0))
-      {
-        return EFI_SECURITY_VIOLATION;
-      }
+    if (  (  (SigDataSize >= (13 + sizeof (mSha256OidValue)))
+          && (  ((*(SigData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE)
+             || (CompareMem (SigData + 13, &mSha256OidValue, sizeof (mSha256OidValue)) != 0)))
+       && (  (SigDataSize >= (32 + sizeof (mSha256OidValue)))
+          && (  ((*(SigData + 20) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE)
+             || (CompareMem (SigData + 32, &mSha256OidValue, sizeof (mSha256OidValue)) != 0))))
+    {
+      return EFI_SECURITY_VIOLATION;
     }
   }

--
2.30.2


[-- Attachment #2: Type: text/html, Size: 8587 bytes --]

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v2 1/1] SecurityPkg/AuthVariableLib: Check SHA-256 OID with ContentInfo present
  2023-01-23  2:37 ` Yao, Jiewen
@ 2023-01-23  6:06   ` Yao, Jiewen
  0 siblings, 0 replies; 3+ messages in thread
From: Yao, Jiewen @ 2023-01-23  6:06 UTC (permalink / raw)
  To: Jan Bobek, devel@edk2.groups.io; +Cc: Jan Bobek, Wang, Jian J, Xu, Min M

[-- Attachment #1: Type: text/plain, Size: 5828 bytes --]

Merged https://github.com/tianocore/edk2/pull/3940

From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Monday, January 23, 2023 10:38 AM
To: Jan Bobek <jbobek@nvidia.com>; devel@edk2.groups.io
Cc: Jan Bobek <jbobek@nvidia.com>; Wang, Jian J <jian.j.wang@intel.com>; Xu, Min M <min.m.xu@intel.com>
Subject: Re: [PATCH v2 1/1] SecurityPkg/AuthVariableLib: Check SHA-256 OID with ContentInfo present

reviewed-by: Jiewen Yao <Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>>
________________________________
发件人: Jan Bobek <jbobek@nvidia.com<mailto:jbobek@nvidia.com>>
发送时间: Monday, January 23, 2023 5:53:48 AM
收件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
抄送: Jan Bobek <jbobek@nvidia.com<mailto:jbobek@nvidia.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Xu, Min M <min.m.xu@intel.com<mailto:min.m.xu@intel.com>>
主题: [PATCH v2 1/1] SecurityPkg/AuthVariableLib: Check SHA-256 OID with ContentInfo present

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4305

Based on whether the DER-encoded ContentInfo structure is present in
authenticated SetVariable payload or not, the SHA-256 OID can be
located at different places.

UEFI specification explicitly states the driver shall support both
cases, but the old code assumed ContentInfo was not present and
incorrectly rejected authenticated variable updates when it were
present.

Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
Cc: Min Xu <min.m.xu@intel.com<mailto:min.m.xu@intel.com>>
Signed-off-by: Jan Bobek <jbobek@nvidia.com<mailto:jbobek@nvidia.com>>
---
 .../Library/AuthVariableLib/AuthService.c     | 50 ++++++++++++++++---
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
index 054ee4d1d988..9beeca09aeba 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
+++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
@@ -1925,7 +1925,7 @@ VerifyTimeBasedPayload (
   // SignedData.digestAlgorithms shall contain the digest algorithm used when preparing the
   // signature. Only a digest algorithm of SHA-256 is accepted.
   //
-  //    According to PKCS#7 Definition:
+  //    According to PKCS#7 Definition (https://www.rfc-editor.org/rfc/rfc2315):
   //        SignedData ::= SEQUENCE {
   //            version Version,
   //            digestAlgorithms DigestAlgorithmIdentifiers,
@@ -1933,15 +1933,49 @@ VerifyTimeBasedPayload (
   //            .... }
   //    The DigestAlgorithmIdentifiers can be used to determine the hash algorithm
   //    in VARIABLE_AUTHENTICATION_2 descriptor.
-  //    This field has the fixed offset (+13) and be calculated based on two bytes of length encoding.
+  //    This field has the fixed offset (+13) or (+32) based on whether the DER-encoded
+  //    ContentInfo structure is present or not, and can be calculated based on two
+  //    bytes of length encoding.
+  //
+  //    Both condition can be handled in WrapPkcs7Data() in CryptPkcs7VerifyCommon.c.
+  //
+  //    See below examples:
+  //
+  // 1. Without ContentInfo
+  //    30 82 0c da // SEQUENCE (5 element) (3294 BYTES) -- SignedData
+  //       02 01 01 // INTEGER 1 -- Version
+  //       31 0f // SET (1 element) (15 BYTES) -- DigestAlgorithmIdentifiers
+  //          30 0d // SEQUENCE (2 element) (13 BYTES) -- AlgorithmIdentifier
+  //             06 09 // OBJECT-IDENTIFIER (9 BYTES) -- algorithm
+  //                60 86 48 01 65 03 04 02 01 // sha256 [2.16.840.1.101.3.4.2.1]
+  //             05 00 // NULL (0 BYTES) -- parameters
+  //
+  // Example from: https://uefi.org/revocationlistfile
+  //
+  // 2. With ContentInfo
+  //    30 82 05 90 // SEQUENCE (1424 BYTES) -- ContentInfo
+  //       06 09 // OBJECT-IDENTIFIER (9 BYTES) -- ContentType
+  //          2a 86 48 86 f7 0d 01 07 02 // signedData [1.2.840.113549.1.7.2]
+  //       a0 82 05 81 // CONTEXT-SPECIFIC CONSTRUCTED TAG 0 (1409 BYTES) -- content
+  //          30 82 05 7d // SEQUENCE (1405 BYTES) -- SignedData
+  //             02 01 01 // INTEGER 1 -- Version
+  //             31 0f // SET (1 element) (15 BYTES) -- DigestAlgorithmIdentifiers
+  //                30 0d // SEQUENCE (13 BYTES) -- AlgorithmIdentifier
+  //                   06 09 // OBJECT-IDENTIFIER (9 BYTES) -- algorithm
+  //                      60 86 48 01 65 03 04 02 01 // sha256 [2.16.840.1.101.3.4.2.1]
+  //                   05 00 // NULL (0 BYTES) -- parameters
+  //
+  // Example generated with: https://wiki.archlinux.org/title/Unified_Extensible_Firmware_Interface/Secure_Boot#Manual_process
   //
   if ((Attributes & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) != 0) {
-    if (SigDataSize >= (13 + sizeof (mSha256OidValue))) {
-      if (((*(SigData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE) ||
-          (CompareMem (SigData + 13, &mSha256OidValue, sizeof (mSha256OidValue)) != 0))
-      {
-        return EFI_SECURITY_VIOLATION;
-      }
+    if (  (  (SigDataSize >= (13 + sizeof (mSha256OidValue)))
+          && (  ((*(SigData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE)
+             || (CompareMem (SigData + 13, &mSha256OidValue, sizeof (mSha256OidValue)) != 0)))
+       && (  (SigDataSize >= (32 + sizeof (mSha256OidValue)))
+          && (  ((*(SigData + 20) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE)
+             || (CompareMem (SigData + 32, &mSha256OidValue, sizeof (mSha256OidValue)) != 0))))
+    {
+      return EFI_SECURITY_VIOLATION;
     }
   }

--
2.30.2

[-- Attachment #2: Type: text/html, Size: 13432 bytes --]

^ permalink raw reply related	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-01-23  6:06 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-22 21:53 [PATCH v2 1/1] SecurityPkg/AuthVariableLib: Check SHA-256 OID with ContentInfo present Jan Bobek
2023-01-23  2:37 ` Yao, Jiewen
2023-01-23  6:06   ` Yao, Jiewen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox