Re: [edk2-devel] [staging/crypto-new-api PATCH] CryptoPkg: Fixed possible security implications in ECDH and BN.
Reviewed-by: Heng Luo > -Original Message- > From: Tan, Ming > Sent: Friday, July 15, 2022 1:35 PM > To: devel@edk2.groups.io; Li, Yi1 > Cc: Luo, Heng > Subject: RE: [edk2-devel] [staging/crypto-new-api PATCH] CryptoPkg: Fixed > possible security implications in ECDH and BN. > > Reviewed-by: Ming Tan > > -Original Message- > From: devel@edk2.groups.io On Behalf Of yi1 li > Sent: Friday, July 15, 2022 1:30 PM > To: devel@edk2.groups.io > Cc: Li, Yi1 ; Tan, Ming ; Luo, Heng > > Subject: [edk2-devel] [staging/crypto-new-api PATCH] CryptoPkg: Fixed possible > security implications in ECDH and BN. > > 1. Origenal code mixes up the input/output parameters for the BN_rshift() > function - the output is actually the first parameter and not the second one. > Now we correct BnRShift() param order. > > 2. NID_X9_62_prime192v1() and NID_secp224r1 prohibited by Intel Crypto/TLS > Guidelines (due to being insufficiently secure). Now we remove those curve. > > 3. ECDH pubilc key check is insufficient and therefore opens the > implementation > up to invalid curve attacks (see e.g.Dragonblood attack report). Need to > perform the checks described by Appendix D of the NIST SP800-186, or Section > 5.6.2.3 of NIST SP800-56Ar3. Now we add full public key validating procedures > to EcDhDeriveSecret(). > > 4. Some APIs need more detail comment. Fix some typos and add more detail > discription for return value. > > Cc: Ming Tan > Cc: Heng Luo > Signed-off-by: Yi Li > > --- > CryptoPkg/Driver/Crypto.c | 31 > - > -- > CryptoPkg/Include/Library/BaseCryptLib.h | 31 > --- > CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c| 7 --- > CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c| 4 +++- > CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c| 61 > ++--- > CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c| 27 > +-- > CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c| 4 +++- > CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c| 27 > +-- > CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c | 31 > --- > CryptoPkg/Private/Protocol/Crypto.h| 31 > -- > - > 10 files changed, 158 insertions(+), 96 deletions(-) > > diff --git a/CryptoPkg/Driver/Crypto.c b/CryptoPkg/Driver/Crypto.c index > de422b7f53..10a0ce8800 100644 > --- a/CryptoPkg/Driver/Crypto.c > +++ b/CryptoPkg/Driver/Crypto.c > @@ -4962,7 +4962,6 @@ CryptoServiceBigNumValueOne ( >@param[out] BnRes The result. > >@retval EFI_SUCCESS On success. > - @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures. >@retval EFI_PROTOCOL_ERROR Otherwise. > **/ > EFI_STATUS > @@ -5051,6 +5050,9 @@ CryptoServiceBigNumContextFree ( > >@param[in] Bn Big number to set. >@param[in] ValValue to set. > + > + @retval EFI_SUCCESS On success. > + @retval EFI_PROTOCOL_ERROR Otherwise. > **/ > EFI_STATUS > EFIAPI > @@ -5092,7 +5094,7 @@ CryptoServiceBigNumAddMod ( >using EcGroupFree() function. > >@param[in] Group Identifying number for the ECC group (IANA "Group > - Description" attribute registrty for RFC 2409). > + Description" attribute registry for RFC 2409). > >@retval EcGroup object On success. >@retval NULLOn failure. > @@ -5114,8 +5116,8 @@ CryptoServiceEcGroupInit ( > >@param[in] EcGroupEC group object. >@param[out] BnPrimeGroup prime number. > - @param[out] BnAA coofecient. > - @param[out] BnBB coofecient. > + @param[out] BnAA coefficient. > + @param[out] BnBB coefficient. >@param[in] BnCtx BN context. > >@retval EFI_SUCCESSOn success. > @@ -5426,13 +5428,14 @@ CryptoServiceEcPointSetCompressedCoordinates ( > /** >Generate a key using ECDH algorithm. Please note, this function uses >pseudo random number generator. The caller must make sure RandomSeed() > - funtion was properly called before. > + function was properly called before. > >@param[in] GroupIdentifying number for the ECC group (IANA "Group > - Description" attribute registrty for RFC 2409). > + Description" attribute registry for RFC 2409). >@param[out] PKey Pointer to an object that will hold the ECDH
Re: [edk2-devel] [staging/crypto-new-api PATCH] CryptoPkg: Fixed possible security implications in ECDH and BN.
Reviewed-by: Ming Tan -Original Message- From: devel@edk2.groups.io On Behalf Of yi1 li Sent: Friday, July 15, 2022 1:30 PM To: devel@edk2.groups.io Cc: Li, Yi1 ; Tan, Ming ; Luo, Heng Subject: [edk2-devel] [staging/crypto-new-api PATCH] CryptoPkg: Fixed possible security implications in ECDH and BN. 1. Origenal code mixes up the input/output parameters for the BN_rshift() function - the output is actually the first parameter and not the second one. Now we correct BnRShift() param order. 2. NID_X9_62_prime192v1() and NID_secp224r1 prohibited by Intel Crypto/TLS Guidelines (due to being insufficiently secure). Now we remove those curve. 3. ECDH pubilc key check is insufficient and therefore opens the implementation up to invalid curve attacks (see e.g.Dragonblood attack report). Need to perform the checks described by Appendix D of the NIST SP800-186, or Section 5.6.2.3 of NIST SP800-56Ar3. Now we add full public key validating procedures to EcDhDeriveSecret(). 4. Some APIs need more detail comment. Fix some typos and add more detail discription for return value. Cc: Ming Tan Cc: Heng Luo Signed-off-by: Yi Li --- CryptoPkg/Driver/Crypto.c | 31 --- CryptoPkg/Include/Library/BaseCryptLib.h | 31 --- CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c| 7 --- CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c| 4 +++- CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c| 61 ++--- CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c| 27 +-- CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c| 4 +++- CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c| 27 +-- CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c | 31 --- CryptoPkg/Private/Protocol/Crypto.h| 31 --- 10 files changed, 158 insertions(+), 96 deletions(-) diff --git a/CryptoPkg/Driver/Crypto.c b/CryptoPkg/Driver/Crypto.c index de422b7f53..10a0ce8800 100644 --- a/CryptoPkg/Driver/Crypto.c +++ b/CryptoPkg/Driver/Crypto.c @@ -4962,7 +4962,6 @@ CryptoServiceBigNumValueOne ( @param[out] BnRes The result. @retval EFI_SUCCESS On success. - @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures. @retval EFI_PROTOCOL_ERROR Otherwise. **/ EFI_STATUS @@ -5051,6 +5050,9 @@ CryptoServiceBigNumContextFree ( @param[in] Bn Big number to set. @param[in] ValValue to set. + + @retval EFI_SUCCESS On success. + @retval EFI_PROTOCOL_ERROR Otherwise. **/ EFI_STATUS EFIAPI @@ -5092,7 +5094,7 @@ CryptoServiceBigNumAddMod ( using EcGroupFree() function. @param[in] Group Identifying number for the ECC group (IANA "Group - Description" attribute registrty for RFC 2409). + Description" attribute registry for RFC 2409). @retval EcGroup object On success. @retval NULLOn failure. @@ -5114,8 +5116,8 @@ CryptoServiceEcGroupInit ( @param[in] EcGroupEC group object. @param[out] BnPrimeGroup prime number. - @param[out] BnAA coofecient. - @param[out] BnBB coofecient. + @param[out] BnAA coefficient. + @param[out] BnBB coefficient. @param[in] BnCtx BN context. @retval EFI_SUCCESSOn success. @@ -5426,13 +5428,14 @@ CryptoServiceEcPointSetCompressedCoordinates ( /** Generate a key using ECDH algorithm. Please note, this function uses pseudo random number generator. The caller must make sure RandomSeed() - funtion was properly called before. + function was properly called before. @param[in] GroupIdentifying number for the ECC group (IANA "Group - Description" attribute registrty for RFC 2409). + Description" attribute registry for RFC 2409). @param[out] PKey Pointer to an object that will hold the ECDH key. @retval EFI_SUCCESSOn success. + @retval EFI_UNSUPPORTEDECC group not supported. @retval EFI_PROTOCOL_ERROR On failure. **/ EFI_STATUS @@ -5466,8 +5469,9 @@ CryptoServiceEcDhKeyFree ( @param[in] PKey ECDH Key object. @param[out] EcPoint Properly initialized EC Point to hold the public key. - @retval EFI_SUCCESSOn success. - @retval EFI_PROTOCOL_ERROR On failure. + @retval EFI_SUCCESS On success. + @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly. + @retval EFI_PROTOCOL_ERROROn failure. **/ EFI_STATUS EFIAPI @@ -5484,15 +5488,20 @@ CryptoServiceEcDhGetPubKey ( @param[in] PKey ECDH Key object. @param[in] Group Identifying number for the ECC group (IANA "Group - Descriptio
[edk2-devel] [staging/crypto-new-api PATCH] CryptoPkg: Fixed possible security implications in ECDH and BN.
1. Origenal code mixes up the input/output parameters for the BN_rshift() function - the output is actually the first parameter and not the second one. Now we correct BnRShift() param order. 2. NID_X9_62_prime192v1() and NID_secp224r1 prohibited by Intel Crypto/TLS Guidelines (due to being insufficiently secure). Now we remove those curve. 3. ECDH pubilc key check is insufficient and therefore opens the implementation up to invalid curve attacks (see e.g.Dragonblood attack report). Need to perform the checks described by Appendix D of the NIST SP800-186, or Section 5.6.2.3 of NIST SP800-56Ar3. Now we add full public key validating procedures to EcDhDeriveSecret(). 4. Some APIs need more detail comment. Fix some typos and add more detail discription for return value. Cc: Ming Tan Cc: Heng Luo Signed-off-by: Yi Li --- CryptoPkg/Driver/Crypto.c | 31 --- CryptoPkg/Include/Library/BaseCryptLib.h | 31 --- CryptoPkg/Library/BaseCryptLib/Bn/CryptBn.c| 7 --- CryptoPkg/Library/BaseCryptLib/Bn/CryptBnNull.c| 4 +++- CryptoPkg/Library/BaseCryptLib/Ec/CryptEc.c| 61 ++--- CryptoPkg/Library/BaseCryptLib/Ec/CryptEcNull.c| 27 +-- CryptoPkg/Library/BaseCryptLibNull/Bn/CryptBnNull.c| 4 +++- CryptoPkg/Library/BaseCryptLibNull/Ec/CryptEcNull.c| 27 +-- CryptoPkg/Library/BaseCryptLibOnProtocolPpi/CryptLib.c | 31 --- CryptoPkg/Private/Protocol/Crypto.h| 31 --- 10 files changed, 158 insertions(+), 96 deletions(-) diff --git a/CryptoPkg/Driver/Crypto.c b/CryptoPkg/Driver/Crypto.c index de422b7f53..10a0ce8800 100644 --- a/CryptoPkg/Driver/Crypto.c +++ b/CryptoPkg/Driver/Crypto.c @@ -4962,7 +4962,6 @@ CryptoServiceBigNumValueOne ( @param[out] BnRes The result. @retval EFI_SUCCESS On success. - @retval EFI_OUT_OF_RESOURCES In case of internal allocation failures. @retval EFI_PROTOCOL_ERROR Otherwise. **/ EFI_STATUS @@ -5051,6 +5050,9 @@ CryptoServiceBigNumContextFree ( @param[in] Bn Big number to set. @param[in] ValValue to set. + + @retval EFI_SUCCESS On success. + @retval EFI_PROTOCOL_ERROR Otherwise. **/ EFI_STATUS EFIAPI @@ -5092,7 +5094,7 @@ CryptoServiceBigNumAddMod ( using EcGroupFree() function. @param[in] Group Identifying number for the ECC group (IANA "Group - Description" attribute registrty for RFC 2409). + Description" attribute registry for RFC 2409). @retval EcGroup object On success. @retval NULLOn failure. @@ -5114,8 +5116,8 @@ CryptoServiceEcGroupInit ( @param[in] EcGroupEC group object. @param[out] BnPrimeGroup prime number. - @param[out] BnAA coofecient. - @param[out] BnBB coofecient. + @param[out] BnAA coefficient. + @param[out] BnBB coefficient. @param[in] BnCtx BN context. @retval EFI_SUCCESSOn success. @@ -5426,13 +5428,14 @@ CryptoServiceEcPointSetCompressedCoordinates ( /** Generate a key using ECDH algorithm. Please note, this function uses pseudo random number generator. The caller must make sure RandomSeed() - funtion was properly called before. + function was properly called before. @param[in] GroupIdentifying number for the ECC group (IANA "Group - Description" attribute registrty for RFC 2409). + Description" attribute registry for RFC 2409). @param[out] PKey Pointer to an object that will hold the ECDH key. @retval EFI_SUCCESSOn success. + @retval EFI_UNSUPPORTEDECC group not supported. @retval EFI_PROTOCOL_ERROR On failure. **/ EFI_STATUS @@ -5466,8 +5469,9 @@ CryptoServiceEcDhKeyFree ( @param[in] PKey ECDH Key object. @param[out] EcPoint Properly initialized EC Point to hold the public key. - @retval EFI_SUCCESSOn success. - @retval EFI_PROTOCOL_ERROR On failure. + @retval EFI_SUCCESS On success. + @retval EFI_INVALID_PARAMETER EcPoint should be initialized properly. + @retval EFI_PROTOCOL_ERROROn failure. **/ EFI_STATUS EFIAPI @@ -5484,15 +5488,20 @@ CryptoServiceEcDhGetPubKey ( @param[in] PKey ECDH Key object. @param[in] Group Identifying number for the ECC group (IANA "Group - Description" attribute registrty for RFC 2409). + Description" attribute registry for RFC 2409). @param[in] EcPointPublic Peer public key. @param[out] SecretSize On success, holds secret size. @param[out] Secret On success, holds the derived secret. Should be freed by caller using FreePool()
