Take in BoringSSL service indicator improvements #564
Conversation
| } | ||
|
|
||
| void EVP_PKEY_keygen_verify_service_indicator(const EVP_PKEY *pkey) { | ||
| if (pkey->type == EVP_PKEY_RSA || pkey->type== EVP_PKEY_RSA_PSS) { |
There was a problem hiding this comment.
Why was EVP_PKEY_RSA_PSS removed? I understand we support it: #140 and ACVP-tested it.
There was a problem hiding this comment.
The function was moved down to Line 280, so this part wasn't actually removed. But yeah I forgot to add EVP_PKEY_RSA_PSS to BoringSSL's code on Line 213, good catch.
There was a problem hiding this comment.
Thank you, Samuel. So I'm curious, how were the ACVP tests passing for PSS if we didn't have this check in the if condition?
There was a problem hiding this comment.
Oh we already had the check, I just forgot to add it back in when porting over BoringSSL's changes.
But on a side note, I think this only checks if the RSA key is a PSS type in the service indicator, the ACVP functionality shouldn't be effected by it either way.
There was a problem hiding this comment.
We discussed offline. The ACVP tests for PSS were added back in 5691781 along with the check, see https://github.com/awslabs/aws-lc/blob/56917817a32e5f0012a0f9e29fbdd7e403f940d5/crypto/fipsmodule/service_indicator/service_indicator_test.cc#L1196-L1225
|
|
||
| if (pkey_type == EVP_PKEY_RSA || pkey_type == EVP_PKEY_RSA_PSS) { | ||
| // EVP_PKEY_RSA_PSS SPKIs aren't supported. |
There was a problem hiding this comment.
Related to my question about removing EVP_PKEY_RSA_PSS above.
| class EVP_ServiceIndicatorTest : public awslc::TestWithNoErrors<CipherTestVector> {}; | ||
| class EVPServiceIndicatorTest : public TestWithNoErrors<CipherTestVector> {}; | ||
|
|
||
| static void TestOperation(const EVP_CIPHER *cipher, bool encrypt, |
There was a problem hiding this comment.
This function seems to have gotten smaller when moved here. Is that intended? The comments seem to have been kept, but I don't know if we're testing
|EVP_EncryptFinal_ex| and |EVP_DecryptFinal_ex| for approval at the end.
Also I had documented internally:
Commit from Boringssl (google/boringssl@16a9493) adds new functions EVP_CipherFinal, EVP_EncryptFinal, and EVP_DecryptFinal . These functions are wrapper functions to the respective EVP_*Final_ex functions, and provide no extra logic. All three of these functions should be Service Indicator approved like the functions they call.
There was a problem hiding this comment.
It was intentional, and their deduction was as follows:
You've both DoEncryptFinal and DoCipherFinal, but if you look at the definition of EVP_CipherFinal_ex you'll see that they can be collapsed.
It aligns with what you documented internally. Looking into the function definition, it is true the original DoEncryptFinal test was a bit redundant:
https://github.com/awslabs/aws-lc/blob/752ee755f10de080c055d87cea9b7d667342a528/crypto/fipsmodule/cipher/cipher.c#L518-L524
|
The AES key round check for the service indicator was removed in this PR. BoringSSL removed the key round check because the validity of the key size is already checked within the key initialization process. Any invalid size will throw an error. From normal use of AWS-LC/BoringSSL there is no way this value could be wrong. |
Issues:
Resolves
CryptoAlg-1209Description of changes:
This PR takes in the service indicator improvements BoringSSL made in google/boringssl@118a892#diff-ce7ffb6368fc741735e07a58ef82c4e0bb9b31d389299f8d0d1e961c76dd9f38
Specific change details can be found within the SIM and internal document: [BoringSSL-service-indicator-differences].
Any new behavior within the tests/functions will most likely be brought up within the internal documentation.
Most of the changes were taken except:
NOT_APPROVEDfor SHA-512/256. This is because we have not gone through FIPS validation for algorithms involving SHA-512/256, but we can expect to do so within the next round of FIPS.kEVPKeyGenShouldCallFIPSFunctions,kCurveSecp256k1Supported,kEVPDeriveSetsServiceIndicatorare set totrue. BoringSSL was nice enough to use theseboolto help us keep some of our existing AWS-LC specific tests. These are set tofalsefor Boring.Call-outs:
N/A
Testing:
Some test cases were rearranged and optimized according to their improvements.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.