public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* Timebased Auth Variable driver should ensure AuthAlgorithm is SHA256 before further verification
@ 2017-12-11 10:40 Wim Vervoorn
  2017-12-11 15:55 ` Long, Qin
  0 siblings, 1 reply; 3+ messages in thread
From: Wim Vervoorn @ 2017-12-11 10:40 UTC (permalink / raw)
  To: edk2-devel@lists.01.org

Hello,

We ran into issues with the Timebased Authenticated variable handling.

In commit: c035e37335ae43229d7e68de74a65f2c01ebc0af

This was added. This assumed the very first tag will be the Sha256 Oid. We have noticed situations where this is the case.

The question is if the check below represents the specification and the tools generating the databuffer should be changed. Or if this check is not correct. It seems to me that the data should be parsed to check for the correct OID and not assume this is the first one

  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;
        }
    }
  }


----
Modified: SecurityPkg/Library/AuthVariableLib/AuthService.c
Modified: SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h


Best Regards,
Wim Vervoorn

Eltan B.V.
Ambachtstraat 23
5481 SM Schijndel
The Netherlands

T : +31-(0)73-594 46 64
E : wvervoorn@eltan.com
W : http://www.eltan.com


"THIS MESSAGE CONTAINS CONFIDENTIAL INFORMATION. UNLESS YOU ARE THE INTENDED RECIPIENT OF THIS MESSAGE, ANY USE OF THIS MESSAGE IS STRICTLY PROHIBITED. IF YOU HAVE RECEIVED THIS MESSAGE IN ERROR, PLEASE IMMEDIATELY NOTIFY THE SENDER BY TELEPHONE +31-(0)73-5944664 OR REPLY EMAIL, AND IMMEDIATELY DELETE THIS MESSAGE AND ALL COPIES." 





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

* Re: Timebased Auth Variable driver should ensure AuthAlgorithm is SHA256 before further verification
  2017-12-11 10:40 Timebased Auth Variable driver should ensure AuthAlgorithm is SHA256 before further verification Wim Vervoorn
@ 2017-12-11 15:55 ` Long, Qin
  2017-12-12 14:14   ` Wim Vervoorn
  0 siblings, 1 reply; 3+ messages in thread
From: Long, Qin @ 2017-12-11 15:55 UTC (permalink / raw)
  To: Wim Vervoorn, edk2-devel@lists.01.org

Hi, Wim Vervoorn,

Yes, the logic here is a little tricky. We wouldn't like to introduce the full ASN.1 parse interfaces to handle the encoding data check. So as the comments states, the digestAlgorithms field usually has the fixed offset (based on two bytes of length encoding) in one PKCS#7 signedData structure. So the new codes (added by that commit) used this assumption to check the Sha256 OID directly. 
  //
  // 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:
  //        SignedData ::= SEQUENCE {
  //            version Version,
  //            digestAlgorithms DigestAlgorithmIdentifiers,
  //            contentInfo ContentInfo,
  //            .... }
  //    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.
  //
  ......

One typical ASN.1 structure of PKCS7 Signature is
  ContentInfo {
     contentType = 1.2.840.113549.1.7.2   //(signedData)
     content {
       SignedData {
         version = 1
         ...
       }
     }
  }
But please note, the PKCS#7 signedData definition for Authenticated Variable in UEFI spec didn't include the contentType fields. So if you used some third-party tool (e.g. OpenSSL) to generate the signedData, you need to strip-off some bytes. 

See more discussion & clarifications from https://bugzilla.tianocore.org/show_bug.cgi?id=586
And share us the binary data for more analysis if you still have verification issues. 


Best Regards & Thanks,
LONG, Qin

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wim Vervoorn
Sent: Monday, December 11, 2017 6:40 PM
To: edk2-devel@lists.01.org
Subject: [edk2] Timebased Auth Variable driver should ensure AuthAlgorithm is SHA256 before further verification

Hello,

We ran into issues with the Timebased Authenticated variable handling.

In commit: c035e37335ae43229d7e68de74a65f2c01ebc0af

This was added. This assumed the very first tag will be the Sha256 Oid. We have noticed situations where this is the case.

The question is if the check below represents the specification and the tools generating the databuffer should be changed. Or if this check is not correct. It seems to me that the data should be parsed to check for the correct OID and not assume this is the first one

  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;
        }
    }
  }


----
Modified: SecurityPkg/Library/AuthVariableLib/AuthService.c
Modified: SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h


Best Regards,
Wim Vervoorn

Eltan B.V.
Ambachtstraat 23
5481 SM Schijndel
The Netherlands

T : +31-(0)73-594 46 64
E : wvervoorn@eltan.com
W : http://www.eltan.com


"THIS MESSAGE CONTAINS CONFIDENTIAL INFORMATION. UNLESS YOU ARE THE INTENDED RECIPIENT OF THIS MESSAGE, ANY USE OF THIS MESSAGE IS STRICTLY PROHIBITED. IF YOU HAVE RECEIVED THIS MESSAGE IN ERROR, PLEASE IMMEDIATELY NOTIFY THE SENDER BY TELEPHONE +31-(0)73-5944664 OR REPLY EMAIL, AND IMMEDIATELY DELETE THIS MESSAGE AND ALL COPIES." 



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


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

* Re: Timebased Auth Variable driver should ensure AuthAlgorithm is SHA256 before further verification
  2017-12-11 15:55 ` Long, Qin
@ 2017-12-12 14:14   ` Wim Vervoorn
  0 siblings, 0 replies; 3+ messages in thread
From: Wim Vervoorn @ 2017-12-12 14:14 UTC (permalink / raw)
  To: Long, Qin, edk2-devel@lists.01.org

Hello LONG, Quin,

Thank you very much for the quick response. From the discussion it is clear to me where the problem is and how the data can be signed using signtool to prevent this.

Do you know if there are any updates to the Linux tools (e.g. efitools) that allow supporting UEFI 2.6 in an easy way?


Best Regards,
Wim Vervoorn

Eltan B.V.
Ambachtstraat 23
5481 SM Schijndel
The Netherlands

T : +31-(0)73-594 46 64
E : wvervoorn@eltan.com
W : http://www.eltan.com


"THIS MESSAGE CONTAINS CONFIDENTIAL INFORMATION. UNLESS YOU ARE THE INTENDED RECIPIENT OF THIS MESSAGE, ANY USE OF THIS MESSAGE IS STRICTLY PROHIBITED. IF YOU HAVE RECEIVED THIS MESSAGE IN ERROR, PLEASE IMMEDIATELY NOTIFY THE SENDER BY TELEPHONE +31-(0)73-5944664 OR REPLY EMAIL, AND IMMEDIATELY DELETE THIS MESSAGE AND ALL COPIES." 






-----Original Message-----
From: Long, Qin [mailto:qin.long@intel.com] 
Sent: Monday, December 11, 2017 4:56 PM
To: Wim Vervoorn <wvervoorn@eltan.com>; edk2-devel@lists.01.org
Subject: RE: Timebased Auth Variable driver should ensure AuthAlgorithm is SHA256 before further verification

Hi, Wim Vervoorn,

Yes, the logic here is a little tricky. We wouldn't like to introduce the full ASN.1 parse interfaces to handle the encoding data check. So as the comments states, the digestAlgorithms field usually has the fixed offset (based on two bytes of length encoding) in one PKCS#7 signedData structure. So the new codes (added by that commit) used this assumption to check the Sha256 OID directly. 
  //
  // 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:
  //        SignedData ::= SEQUENCE {
  //            version Version,
  //            digestAlgorithms DigestAlgorithmIdentifiers,
  //            contentInfo ContentInfo,
  //            .... }
  //    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.
  //
  ......

One typical ASN.1 structure of PKCS7 Signature is
  ContentInfo {
     contentType = 1.2.840.113549.1.7.2   //(signedData)
     content {
       SignedData {
         version = 1
         ...
       }
     }
  }
But please note, the PKCS#7 signedData definition for Authenticated Variable in UEFI spec didn't include the contentType fields. So if you used some third-party tool (e.g. OpenSSL) to generate the signedData, you need to strip-off some bytes. 

See more discussion & clarifications from https://bugzilla.tianocore.org/show_bug.cgi?id=586
And share us the binary data for more analysis if you still have verification issues. 


Best Regards & Thanks,
LONG, Qin

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wim Vervoorn
Sent: Monday, December 11, 2017 6:40 PM
To: edk2-devel@lists.01.org
Subject: [edk2] Timebased Auth Variable driver should ensure AuthAlgorithm is SHA256 before further verification

Hello,

We ran into issues with the Timebased Authenticated variable handling.

In commit: c035e37335ae43229d7e68de74a65f2c01ebc0af

This was added. This assumed the very first tag will be the Sha256 Oid. We have noticed situations where this is the case.

The question is if the check below represents the specification and the tools generating the databuffer should be changed. Or if this check is not correct. It seems to me that the data should be parsed to check for the correct OID and not assume this is the first one

  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;
        }
    }
  }


----
Modified: SecurityPkg/Library/AuthVariableLib/AuthService.c
Modified: SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h


Best Regards,
Wim Vervoorn

Eltan B.V.
Ambachtstraat 23
5481 SM Schijndel
The Netherlands

T : +31-(0)73-594 46 64
E : wvervoorn@eltan.com
W : http://www.eltan.com


"THIS MESSAGE CONTAINS CONFIDENTIAL INFORMATION. UNLESS YOU ARE THE INTENDED RECIPIENT OF THIS MESSAGE, ANY USE OF THIS MESSAGE IS STRICTLY PROHIBITED. IF YOU HAVE RECEIVED THIS MESSAGE IN ERROR, PLEASE IMMEDIATELY NOTIFY THE SENDER BY TELEPHONE +31-(0)73-5944664 OR REPLY EMAIL, AND IMMEDIATELY DELETE THIS MESSAGE AND ALL COPIES." 



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





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

end of thread, other threads:[~2017-12-12 14:09 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-11 10:40 Timebased Auth Variable driver should ensure AuthAlgorithm is SHA256 before further verification Wim Vervoorn
2017-12-11 15:55 ` Long, Qin
2017-12-12 14:14   ` Wim Vervoorn

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