Hello, 2014-11-10 2:12 GMT+03:00 Miod Vallat <m...@online.fr>: > 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