crypto: Improve error checking and reporting #3753
Conversation
mscdex
added
crypto
|
This looks pretty straight forward. Just one question on this part: bool DiffieHellman::Init(int primeLength, int g) {
dh = DH_new();
- DH_generate_parameters_ex(dh, primeLength, g, 0);
+ if (!DH_generate_parameters_ex(dh, primeLength, g, 0))
+ return false;
bool result = VerifyContext();
if (!result)
return false;
Can you explain how it failed without the change versus with. I'm assuming that VerifyContext() would have returned an error but that checking the result from DH_generate_parameters_ex() makes it return a better error |
|
Without that change you will get a segfault. Here is the VerifyContext code: If the previous call to DH_generate_parameters_ex fails, the dh output parameter will not be populated, and DH_check will segfault. For example: gdb --args out/Debug/node node/test/parallel/test-crypto-dh-odd-key.js |
|
Thanks for the details. lgtm |
|
LGTM cc @nodejs/crypto I would like to have one more LGTM on this, just to be sure |
|
LGTM |
|
lint failures Done processing src/node_win32_etw_provider.cc Stefan please update commit/test and make sure "make lint" passes then I'll respin the CI run |
Added checks where necessary to prevent hard crashes and gave precedence to returning the OpenSSL error strings instead of generic error strings.
stefanmb
force-pushed
the
fips-cs2-error-checking
branch
from
0c278d4
to
2eec1ce
Compare
|
Fixed! |
|
LGTM |
|
lgtm and follow up CI Run https://ci.nodejs.org/job/node-test-pull-request/714/ Will land once this completes |
Added checks where necessary to prevent hard crashes and gave precedence to returning the OpenSSL error strings instead of generic error strings. PR-URL: #3753 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
|
Nothing in the CI run looked related (failures mostly job not starting to run as opposed to a test failing) Landed as b7089f6 |
Added checks where necessary to prevent hard crashes and gave precedence to returning the OpenSSL error strings instead of generic error strings. PR-URL: #3753 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Added checks where necessary to prevent hard crashes and gave precedence to returning the OpenSSL error strings instead of generic error strings. PR-URL: #3753 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
|
landed in v4.x-staging in c37a560 |
Added checks where necessary to prevent hard crashes and gave precedence to returning the OpenSSL error strings instead of generic error strings. PR-URL: #3753 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
| @@ -3530,6 +3535,9 @@ bool Hash::HashInit(const char* hash_type) { | |||
| return false; | |||
| EVP_MD_CTX_init(&mdctx_); | |||
| EVP_DigestInit_ex(&mdctx_, md_, nullptr); | |||
| if (0 != ERR_peek_error()) { | |||
| return false; | |||
| } | |||
There was a problem hiding this comment.
@bnoordhuis Nope, chalk it up to my unfamiliarity with OpenSSL API. Checking EVP_DigestInit_ex's return code should accomplish the same thing, without breaking #4221.
There was a problem hiding this comment.
Added checks where necessary to prevent hard crashes and gave precedence to returning the OpenSSL error strings instead of generic error strings. PR-URL: #3753 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Added checks where necessary to prevent hard crashes and gave precedence to returning the OpenSSL error strings instead of generic error strings. PR-URL: #3753 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
In FIPS mode several test failures occur because of missing checks for OpenSSL API return codes. Where error checking already existed, the error messages often report some static string instead of passing through the actual error string from OpenSSL (which is significantly more helpful in identifying the root cause of the problem). I’ve added checks where necessary to prevent hard crashes and have given precedence to returning the OpenSSL error strings, instead of generic error strings, whenever possible.