Hello,
2014-11-10 2:12 GMT+03:00 Miod Vallat <[email protected]>:
> The following diff attempts to polish the GOST code in libcrypto and add
> many missing error checks (probably not exhaustive, but a good start).
I knew that I'm not perfect, but I didn't know the depth of my imperfectness...
I will review your changes later with greater care.
> I am also considering fixing the gost2001_do_sign() interface violation
> (it frees something which has been allocated by its caller), if only to
> make it match gost2001_do_verify(), which leaves the responsibility to
> the caller; but that will happen in a later diff.
That should be ok.
Few comments below.
> @@ -102,36 +105,44 @@ int gost2001_compute_public(GOST_KEY * e
[..]
> - }
> - ok = 256;
> + if (GOST_KEY_set_public_key(ec, pub_key) == 0)
> + goto err;
> + ok = 256; /* XXX */
This skipped through my review/refactoring. This function can be
changed to return 1
in non-error cases.
> +
> + if (ok == 0) {
> err:
> @@ -151,70 +168,94 @@ ECDSA_SIG *gost2001_do_sign(BIGNUM * md,
> r = newsig->r;
> group = GOST_KEY_get0_group(eckey);
> order = BN_CTX_get(ctx);
> - EC_GROUP_get_order(group, order, ctx);
> + if (order == NULL)
> + goto err;
> + if (EC_GROUP_get_order(group, order, ctx) == 0) {
> + /*
> + * XXX EC_GROUP_get_order() will return 0 if successful but
> + * XXX order == 0. But then BN_mod below would fail anyway.
> + */
> + goto err;
> + }
In theory it is fine to add
if (BN_is_zero(order)) goto err;
On the other hand no GOST curves/keys should have order == 0.
[...]
> + if (ctx != NULL) {
> + BN_CTX_end(ctx);
> + BN_CTX_free(ctx);
> + }
> + BN_free(md); /* XXX */
Yes, this probably should be fixed also.
> @@ -234,88 +277,145 @@ int gost2001_do_verify(BIGNUM * md, ECDS
> X = BN_CTX_get(ctx);
> R = BN_CTX_get(ctx);
> v = BN_CTX_get(ctx);
> + if (v == NULL)
> + goto err;
>
> - EC_GROUP_get_order(group, order, ctx);
> + if (EC_GROUP_get_order(group, order, ctx) == 0) {
> + /*
> + * XXX EC_GROUP_get_order() will return 0 if successful but
> + * XXX order == 0. But then BN_mod below would fail anyway.
> + */
> + goto err;
> + }
Same as in gost2001_do_sign.
> +int
> +VKO_compute_key(BIGNUM *X, BIGNUM *Y, const GOST_KEY *pkey, GOST_KEY
> *priv_key,
> + const BIGNUM *ukm)
> {
[...]
> + if (EC_GROUP_get_order(group, order, ctx) == 0) {
> + /*
> + * XXX EC_GROUP_get_order() will return 0 if successful but
> + * XXX order == 0. But then BN_mod_mul below would fail
> anyway.
> + */
> + goto err;
> + }
And again. You can add a check, but if it fails, the problem is elsewhere.
> -int gost2001_keygen(GOST_KEY * ec)
> +int
> +gost2001_keygen(GOST_KEY *ec)
> {
> BIGNUM *order = BN_new(), *d = BN_new();
> const EC_GROUP *group = GOST_KEY_get0_group(ec);
> - EC_GROUP_get_order(group, order, NULL);
> + int rc = 0;
> +
> + if (order == NULL || d == NULL)
> + goto err;
> + if (EC_GROUP_get_order(group, order, NULL) == 0) {
> + /*
> + * XXX EC_GROUP_get_order() will return 0 if successful but
> + * XXX order == 0. But then BN_rand_range below would fail
> + * XXX anyway.
> + */
> + goto err;
> + }
And again.
--
With best wishes
Dmitry
