Aside from the coding style, which could benefit as a minimum from not
having instructions on the same line as labels, but would also benefit
from explicit checks against NULL or zero instead of (!foo) in many
places....
> Index: lib/libssl/src/crypto/dstu/dstu_ameth.c
> +static int
> +dstu_asn1_param_copy(EVP_PKEY * to, const EVP_PKEY * from)
> +{
> + if (!to_key) {
> + to_key = DSTU_KEY_new();
Unchecked allocation.
> Index: lib/libssl/src/crypto/dstu/dstu_compress.c
> +static int
> +bn_trace(const BIGNUM * bn, const BIGNUM * p, BN_CTX * ctx)
> +{
> + BIGNUM *r = NULL;
> + int res = -1, i;
> +
> + BN_CTX_start(ctx);
> +
> + r = BN_CTX_get(ctx);
> +
> + if (!BN_copy(r, bn))
> + goto err;
Unchecked allocation. BN_copy(NULL, whatever) will happily dereference
NULL.
> Index: lib/libssl/src/crypto/dstu/dstu_key.c
> +DSTU_AlgorithmParameters *
> +asn1_from_key(const DSTU_KEY * key,
> + int is_little_endian)
> +{
> + DSTU_AlgorithmParameters *params = DSTU_AlgorithmParameters_new(),
> + *ret =
> + NULL;
> + const EC_GROUP *group = EC_KEY_get0_group(key->ec);
> + if (!params || !group)
> + return NULL;
If group is NULL but params isn't, params is leaked.
> +DSTU_KEY_CTX *
> +DSTU_KEY_CTX_new(void)
> +{
> + DSTU_KEY_CTX *ctx = malloc(sizeof(DSTU_KEY_CTX));
> + if (ctx) {
> + memset(ctx, 0, sizeof(DSTU_KEY_CTX));
> + return ctx;
> + }
> + return NULL;
Why not simply use calloc() here?
> Index: lib/libssl/src/crypto/dstu/dstu_params.c
> + for (i = 0; i < 256; i++) {
> + t = (unsigned int) (unpacked_sbox.k8[i >> 4] << 4 |
> unpacked_sbox.k7[i & 15]) << 24;
unpacked_sbox.k7[] needs to be explicitely casted to unsigned int before
the shift, otherwise it gets implicitely converted from unsigned char to
signed int, and the shift becomes an UB.
> Index: lib/libssl/src/crypto/dstu/dstu_sign.c
> +static int
> +hash_to_field(const unsigned char *hash, int hash_len, BIGNUM * fe,
> + int fieldsize)
> +{
> + if (BN_is_zero(fe))
> + BN_one(fe);
BN_one() may fail.
> +int
> +dstu_do_sign(const EC_KEY * key, const unsigned char *tbs, size_t tbslen,
> + unsigned char *sig)
> +{
> + do {
> + if (!BN_rand_range(e, n))
> + goto err;
> +
> + if (!EC_POINT_mul(group, eG, e, NULL, NULL,
> ctx))
> + goto err;
You might want to add
if (BN_is_zero(e))
continue;
before attempting to multiply here...
> Index: lib/libssl/src/crypto/evp/e_dstu.c
> +static int
> +dstu_cipher_set_asn1_parameters(EVP_CIPHER_CTX * ctx, ASN1_TYPE * asn1_type)
> +{
> + /*
> + * We defined params asn1 structure, but for now we will use manual
> + * composition for speed here.
> + */
No, please don't. This code is unreadable and exactly the kind of code
which caused OpenSSL to become a huge mess of bugs in the first place.
Please use the proper ASN1 encoding functions to build your parameters
object.
> +static int
> +dstu_cipher_get_asn1_parameters(EVP_CIPHER_CTX * ctx, ASN1_TYPE * asn1_type)
> +{
Same here.
