[edk2] [PATCH] SecurityPkg/Pkcs7Verify: Add the comments to address security problem

Long Qin posted 1 patch 6 years, 6 months ago
Failed in applying to current master (apply log)
MdePkg/Include/Protocol/Pkcs7Verify.h                   | 10 +++++++++-
SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c |  8 ++++++++
2 files changed, 17 insertions(+), 1 deletion(-)
[edk2] [PATCH] SecurityPkg/Pkcs7Verify: Add the comments to address security problem
Posted by Long Qin 6 years, 6 months ago
Add the comments to address security problems in the Pkcs7Verify Protocol
per UEFI 2.7 updates.

The Pkcs7Verifier function VerifySignature() has problematic use cases
where it might be used to unwittingly bypass security checks.  The specific
problem is that if the supplied hash is a different algorithm from the
blacklist hash, the hash will be approved even if it should have been
denied. The added comments place a strong warning about the problem.
It is possible to use the protocol reliably, either by agreeing a hash to
use for all time (like sha256) or by looping over all supported hashes when
using the protocol.

Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qin Long <qin.long@intel.com>
---
 MdePkg/Include/Protocol/Pkcs7Verify.h                   | 10 +++++++++-
 SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c |  8 ++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/Protocol/Pkcs7Verify.h b/MdePkg/Include/Protocol/Pkcs7Verify.h
index ca5ec75910..eaeda48300 100644
--- a/MdePkg/Include/Protocol/Pkcs7Verify.h
+++ b/MdePkg/Include/Protocol/Pkcs7Verify.h
@@ -6,7 +6,7 @@
   PKCS#7 is a general-purpose cryptographic standard (defined by RFC2315,
   available at http://tools.ietf.org/html/rfc2315).
 
-Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials are licensed and made available under
 the terms and conditions of the BSD License that accompanies this distribution.
 The full text of the license may be found at
@@ -140,6 +140,14 @@ EFI_STATUS
   verifies the signature of the content is valid and signing certificate was not revoked
   and is contained within a list of trusted signers.
 
+  Note: because this function uses hashes and the specification contains a variety of
+        hash choices, you should be aware that the check against the RevokedDb list
+        will improperly succeed if the signature is revoked using a different hash
+        algorithm.  For this reason, you should either cycle through all UEFI supported
+        hashes to see if one is forbidden, or rely on a single hash choice only if the
+        UEFI signature authority only signs and revokes with a single hash (at time
+        of writing, this hash choice is SHA256).
+
   @param[in]     This                 Pointer to EFI_PKCS7_VERIFY_PROTOCOL instance.
   @param[in]     Signature            Points to buffer containing ASN.1 DER-encoded PKCS
                                       detached signature.
diff --git a/SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c b/SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c
index 0da549a6bd..ac83e6d5c2 100644
--- a/SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c
+++ b/SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c
@@ -1321,6 +1321,14 @@ _Exit:
   verifies the signature of the content is valid and signing certificate was not revoked
   and is contained within a list of trusted signers.
 
+  Note: because this function uses hashes and the specification contains a variety of
+        hash choices, you should be aware that the check against the RevokedDb list
+        will improperly succeed if the signature is revoked using a different hash
+        algorithm.  For this reason, you should either cycle through all UEFI supported
+        hashes to see if one is forbidden, or rely on a single hash choice only if the
+        UEFI signature authority only signs and revokes with a single hash (at time
+        of writing, this hash choice is SHA256).
+
   @param[in]     This                 Pointer to EFI_PKCS7_VERIFY_PROTOCOL instance.
   @param[in]     Signature            Points to buffer containing ASN.1 DER-encoded PKCS
                                       detached signature.
-- 
2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] SecurityPkg/Pkcs7Verify: Add the comments to address security problem
Posted by Zhang, Chao B 6 years, 6 months ago
Qin:
    The bellowing checking log is a little confusing to me. 
       The specific problem is that if the supplied hash is a different algorithm from the blacklist hash, the hash will be approved even if it should have been denied.
     How about changing it to 
        The backlist hash check may result in false negative given hashes from other different algorithms.
  
     Others are good to me. 
     Reviewed-by : Chao Zhang <chao.b.zhang@intel.com>

-----Original Message-----
From: Long, Qin 
Sent: Thursday, October 12, 2017 9:18 AM
To: Zhang, Chao B <chao.b.zhang@intel.com>; James.Bottomley@HansenPartnership.com
Cc: edk2-devel@lists.01.org; Long, Qin <qin.long@intel.com>
Subject: [PATCH] SecurityPkg/Pkcs7Verify: Add the comments to address security problem

Add the comments to address security problems in the Pkcs7Verify Protocol per UEFI 2.7 updates.

The Pkcs7Verifier function VerifySignature() has problematic use cases where it might be used to unwittingly bypass security checks.  The specific problem is that if the supplied hash is a different algorithm from the blacklist hash, the hash will be approved even if it should have been denied. The added comments place a strong warning about the problem.
It is possible to use the protocol reliably, either by agreeing a hash to use for all time (like sha256) or by looping over all supported hashes when using the protocol.

Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qin Long <qin.long@intel.com>
---
 MdePkg/Include/Protocol/Pkcs7Verify.h                   | 10 +++++++++-
 SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c |  8 ++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/Protocol/Pkcs7Verify.h b/MdePkg/Include/Protocol/Pkcs7Verify.h
index ca5ec75910..eaeda48300 100644
--- a/MdePkg/Include/Protocol/Pkcs7Verify.h
+++ b/MdePkg/Include/Protocol/Pkcs7Verify.h
@@ -6,7 +6,7 @@
   PKCS#7 is a general-purpose cryptographic standard (defined by RFC2315,
   available at http://tools.ietf.org/html/rfc2315).
 
-Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials are licensed and made available under  the terms and conditions of the BSD License that accompanies this distribution.
 The full text of the license may be found at @@ -140,6 +140,14 @@ EFI_STATUS
   verifies the signature of the content is valid and signing certificate was not revoked
   and is contained within a list of trusted signers.
 
+  Note: because this function uses hashes and the specification contains a variety of
+        hash choices, you should be aware that the check against the RevokedDb list
+        will improperly succeed if the signature is revoked using a different hash
+        algorithm.  For this reason, you should either cycle through all UEFI supported
+        hashes to see if one is forbidden, or rely on a single hash choice only if the
+        UEFI signature authority only signs and revokes with a single hash (at time
+        of writing, this hash choice is SHA256).
+
   @param[in]     This                 Pointer to EFI_PKCS7_VERIFY_PROTOCOL instance.
   @param[in]     Signature            Points to buffer containing ASN.1 DER-encoded PKCS
                                       detached signature.
diff --git a/SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c b/SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c
index 0da549a6bd..ac83e6d5c2 100644
--- a/SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c
+++ b/SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c
@@ -1321,6 +1321,14 @@ _Exit:
   verifies the signature of the content is valid and signing certificate was not revoked
   and is contained within a list of trusted signers.
 
+  Note: because this function uses hashes and the specification contains a variety of
+        hash choices, you should be aware that the check against the RevokedDb list
+        will improperly succeed if the signature is revoked using a different hash
+        algorithm.  For this reason, you should either cycle through all UEFI supported
+        hashes to see if one is forbidden, or rely on a single hash choice only if the
+        UEFI signature authority only signs and revokes with a single hash (at time
+        of writing, this hash choice is SHA256).
+
   @param[in]     This                 Pointer to EFI_PKCS7_VERIFY_PROTOCOL instance.
   @param[in]     Signature            Points to buffer containing ASN.1 DER-encoded PKCS
                                       detached signature.
--
2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] SecurityPkg/Pkcs7Verify: Add the comments to address security problem
Posted by Long, Qin 6 years, 6 months ago
Thanks, Chao.
The suggested change looks too neutral against this problem. I still prefer to keep the original language, which was also cited from the description of this spec ECR document. 


Best Regards & Thanks,
LONG, Qin

-----Original Message-----
From: Zhang, Chao B 
Sent: Monday, October 16, 2017 10:24 PM
To: Long, Qin <qin.long@intel.com>; James.Bottomley@HansenPartnership.com
Cc: edk2-devel@lists.01.org
Subject: RE: [PATCH] SecurityPkg/Pkcs7Verify: Add the comments to address security problem

Qin:
    The bellowing checking log is a little confusing to me. 
       The specific problem is that if the supplied hash is a different algorithm from the blacklist hash, the hash will be approved even if it should have been denied.
     How about changing it to 
        The backlist hash check may result in false negative given hashes from other different algorithms.
  
     Others are good to me. 
     Reviewed-by : Chao Zhang <chao.b.zhang@intel.com>

-----Original Message-----
From: Long, Qin 
Sent: Thursday, October 12, 2017 9:18 AM
To: Zhang, Chao B <chao.b.zhang@intel.com>; James.Bottomley@HansenPartnership.com
Cc: edk2-devel@lists.01.org; Long, Qin <qin.long@intel.com>
Subject: [PATCH] SecurityPkg/Pkcs7Verify: Add the comments to address security problem

Add the comments to address security problems in the Pkcs7Verify Protocol per UEFI 2.7 updates.

The Pkcs7Verifier function VerifySignature() has problematic use cases where it might be used to unwittingly bypass security checks.  The specific problem is that if the supplied hash is a different algorithm from the blacklist hash, the hash will be approved even if it should have been denied. The added comments place a strong warning about the problem.
It is possible to use the protocol reliably, either by agreeing a hash to use for all time (like sha256) or by looping over all supported hashes when using the protocol.

Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qin Long <qin.long@intel.com>
---
 MdePkg/Include/Protocol/Pkcs7Verify.h                   | 10 +++++++++-
 SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c |  8 ++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/Protocol/Pkcs7Verify.h b/MdePkg/Include/Protocol/Pkcs7Verify.h
index ca5ec75910..eaeda48300 100644
--- a/MdePkg/Include/Protocol/Pkcs7Verify.h
+++ b/MdePkg/Include/Protocol/Pkcs7Verify.h
@@ -6,7 +6,7 @@
   PKCS#7 is a general-purpose cryptographic standard (defined by RFC2315,
   available at http://tools.ietf.org/html/rfc2315).
 
-Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials are licensed and made available under  the terms and conditions of the BSD License that accompanies this distribution.
 The full text of the license may be found at @@ -140,6 +140,14 @@ EFI_STATUS
   verifies the signature of the content is valid and signing certificate was not revoked
   and is contained within a list of trusted signers.
 
+  Note: because this function uses hashes and the specification contains a variety of
+        hash choices, you should be aware that the check against the RevokedDb list
+        will improperly succeed if the signature is revoked using a different hash
+        algorithm.  For this reason, you should either cycle through all UEFI supported
+        hashes to see if one is forbidden, or rely on a single hash choice only if the
+        UEFI signature authority only signs and revokes with a single hash (at time
+        of writing, this hash choice is SHA256).
+
   @param[in]     This                 Pointer to EFI_PKCS7_VERIFY_PROTOCOL instance.
   @param[in]     Signature            Points to buffer containing ASN.1 DER-encoded PKCS
                                       detached signature.
diff --git a/SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c b/SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c
index 0da549a6bd..ac83e6d5c2 100644
--- a/SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c
+++ b/SecurityPkg/Pkcs7Verify/Pkcs7VerifyDxe/Pkcs7VerifyDxe.c
@@ -1321,6 +1321,14 @@ _Exit:
   verifies the signature of the content is valid and signing certificate was not revoked
   and is contained within a list of trusted signers.
 
+  Note: because this function uses hashes and the specification contains a variety of
+        hash choices, you should be aware that the check against the RevokedDb list
+        will improperly succeed if the signature is revoked using a different hash
+        algorithm.  For this reason, you should either cycle through all UEFI supported
+        hashes to see if one is forbidden, or rely on a single hash choice only if the
+        UEFI signature authority only signs and revokes with a single hash (at time
+        of writing, this hash choice is SHA256).
+
   @param[in]     This                 Pointer to EFI_PKCS7_VERIFY_PROTOCOL instance.
   @param[in]     Signature            Points to buffer containing ASN.1 DER-encoded PKCS
                                       detached signature.
--
2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel