Coverity 1521557: Error handling issues #20409
Conversation
Check the return from DSA_set0_key and generate an error on failure. Technically a false positive since the function always returns success.
paulidale
added
branch: master
crypto/dsa/dsa_backend.c
Outdated
| @@ -173,7 +173,10 @@ DSA *ossl_dsa_key_from_pkcs8(const PKCS8_PRIV_KEY_INFO *p8inf, | |||
| ERR_raise(ERR_LIB_DSA, DSA_R_BN_ERROR); | |||
| goto dsaerr; | |||
| } | |||
| DSA_set0_key(dsa, dsa_pubkey, dsa_privkey); | |||
| if (!DSA_set0_key(dsa, dsa_pubkey, dsa_privkey)) { | |||
| ERR_raise(ERR_LIB_DSA, ERR_R_BN_LIB); | |||
There was a problem hiding this comment.
I have no idea how we came up with these different error reasons... ERR_R_BN_LIB and also DSA_R_BN_ERROR. I would think this should not be a ERR_R_BN_LIB?
There was a problem hiding this comment.
What should it be?
"internal error" might make sense.
There was a problem hiding this comment.
That is the only place that does a raise on the setkey :) Not sure INVALID_KEY is great either..
There was a problem hiding this comment.
At some point error raising probably needs to be added all over the place.
There was a problem hiding this comment.
Agreed about more error raising.
Technically, the error here cannot be raised. DSA_set0_key never fails. At least currently, but I doubt it will change.
t8m
removed
the
approval: otc review pending
slontis
added
approval: done
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
|
Merged, thanks for the reviews. |
Check the return from DSA_set0_key and generate an error on failure. Technically a false positive since the function always returns success. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Shane Lontis <shane.lontis@oracle.com> (Merged from #20409)
Check the return from DSA_set0_key and generate an error on failure.
Technically a false positive since the function always returns success.