Fix incorrect return check of BN_bn2binpad #16942
Conversation
| @@ -71,7 +71,7 @@ static int pkey_get_bn_bytes(EVP_PKEY *pkey, const char *name, | |||
| buf = OPENSSL_zalloc(sz); | |||
| if (buf == NULL) | |||
| goto err; | |||
| if (!BN_bn2binpad(bn, buf, sz)) | |||
| if (BN_bn2binpad(bn, buf, sz) <= 0) | |||
There was a problem hiding this comment.
I changed to <= according to reutrn values check in /test/acvp_test.c:229, 231, 570, 572. There are a lot. Do we need to change them all?
There was a problem hiding this comment.
And /crypto/ec/ec_curve.c: 3404
There was a problem hiding this comment.
Same problem here
Hmm, @kroeckx but what the returned 0 indicates? A 0 byte sized number does not look like a number. Perhaps 0 cannot be returned at all? A zero BIGNUM should be encoded as a single zero byte, shouldn't it? So the return value should be 1.
There was a problem hiding this comment.
It's hard to see how a zero could be returned.
We should nonetheless try to be consistent in the return code handling.
There was a problem hiding this comment.
In #16943, the code invoking BN_bn2nativepad has a consistent handling way. As for BN_bn2binpad, == -1, <0, and <=0 can be found in codebase. Maybe it's better to unify them.
There was a problem hiding this comment.
Agreed, they should be unified. Not necessary for this PR IMO.
| @@ -46,7 +46,7 @@ static int fbytes(unsigned char *buf, size_t num, ossl_unused const char *name, | |||
| || !TEST_true(BN_hex2bn(&tmp, numbers[fbytes_counter])) | |||
| /* tmp might need leading zeros so pad it out */ | |||
| || !TEST_int_le(BN_num_bytes(tmp), num) | |||
| || !TEST_true(BN_bn2binpad(tmp, buf, num))) | |||
| || !TEST_int_gt(BN_bn2binpad(tmp, buf, num), 0)) | |||
|
Hmm, @kroeckx but what the returned 0 indicates? A 0 byte sized number does not look like a number. Perhaps 0 cannot be returned at all? A zero BIGNUM should be encoded as a single zero byte, shouldn't it? So the return value should be 1.
I guess you only get 0 back when the number is 0 and the size that is passed is 0, which seems unlikely. So <= 0 is fine.
|
| @@ -71,7 +71,7 @@ static int pkey_get_bn_bytes(EVP_PKEY *pkey, const char *name, | |||
| buf = OPENSSL_zalloc(sz); | |||
| if (buf == NULL) | |||
| goto err; | |||
| if (!BN_bn2binpad(bn, buf, sz)) | |||
| if (BN_bn2binpad(bn, buf, sz) <= 0) | |||
There was a problem hiding this comment.
It's hard to see how a zero could be returned.
We should nonetheless try to be consistent in the return code handling.
paulidale
added
approval: review pending
kroeckx
added
approval: done
openssl-machine
removed
the
approval: done
|
This pull request is ready to merge |
openssl-machine
added
the
approval: ready to merge
Reviewed-by: Kurt Roeckx <kurt@roeckx.be> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #16942)
|
Merged to master and 3.0. Thanks for the fix. |
Checklist