review needs-fixing

On Sun, Oct 28, 2012 at 04:35:25PM +0000, Miika Komu wrote:
> 
> I haven't heard from Xin for a while. I fixed two things on this branch:
> * The cookie indexing bug that Christof discovered
> * The code passes now commit hooks
>
> We need this code for HIPv2. At the moment, I don't have much time to
> restructure/modularize/rewrite the entire branch, so please no generic
> or high level comments, only specific ones! Thanks.

This branch should not be merged before the code duplication is addressed
IMNSHO.  It's just too much and too much of a sin.

> --- libcore/crypto.c	2012-05-12 06:54:33 +0000
> +++ libcore/crypto.c	2012-10-28 16:34:23 +0000
> @@ -68,6 +69,11 @@
>  #include "crypto.h"
>  
>  
> +const uint8_t HIP_DH_GROUP_LIST[HIP_DH_GROUP_LIST_SIZE] = {
> +    HIP_DH_NIST_P_384,
> +    HIP_DH_OAKLEY_15,
> +    HIP_DH_OAKLEY_5
> +};
>  /*
>   * Diffie-Hellman primes
>   */

This could use an empty line before the comment block.

> @@ -701,6 +711,172 @@
> +/**
> + * Generate a new Elliptic Curve Diffie-Hellman key.
> + *
> + * @param group_id the group ID of the DH_GROUP defined in HIPv2
> + * @return         a new ECDH key (caller deallocates), or NULL on error.
> + */
> +EC_KEY *hip_generate_ecdh_key(const int group_id)
> +{
> +    char           rnd_seed[20];
> +    struct timeval tv;
> +    EC_KEY        *key;
> +    int            nid;
> +
> +    if (group_id == HIP_DH_NIST_P_256) {
> +        nid = NID_X9_62_prime256v1;
> +    } else if (group_id == HIP_DH_NIST_P_384) {
> +        nid = NID_secp384r1;
> +    } else if (group_id == HIP_DH_NIST_P_521) {
> +        nid = NID_secp521r1;
> +    } else {
> +        HIP_ERROR("Unsupported ECDH group: %d\n", group_id);
> +        return NULL;
> +    }

This calls for a switch statement.

> +int hip_gen_ecdh_shared_key(EC_KEY *const key,
> +                            const uint8_t *const peer_pub_x,
> +                            const uint8_t *const peer_pub_y,
> +                            const size_t peer_len,
> +                            uint8_t *const shared_key,
> +                            const size_t outlen)
> +{
> +    const EC_GROUP *group;
> +    BIGNUM         *peer_pubx = NULL;
> +    BIGNUM         *peer_puby = NULL;
> +    EC_POINT       *peer_pub  = NULL;
> +    int             err       = 1;
> +    unsigned int    out;
> +
> +    out = ECDH_compute_key(shared_key, outlen, peer_pub, key, NULL);
> +    HIP_IFEL(out == 0 || out != peer_len, 0,
> +             "Failed to compute the ECDH shared key\n");
> +    err = out;

The indirection through out seems quite pointless, why not assign to
err in the first place?  Since ECDH_compute_key() returns int, this
is doubly weird.

> +int hip_encode_ecdh_publickey(EC_KEY *key, uint8_t *out, int outlen)
> +{
> +    BIGNUM *pubx = NULL;
> +    BIGNUM *puby = NULL;
> +    int     len;
> +    int     err = 0;
> +
> +    pubx = BN_new();
> +    puby = BN_new();
> +    HIP_IFEL(pubx == NULL || puby == NULL, -1, "Failed to initialize Big Number\n");

This rather looks like memory allocation failure and should return -ENOMEM.

> +    if (EC_POINT_get_affine_coordinates_GFp(EC_KEY_get0_group(key),
> +                                            EC_KEY_get0_public_key(key),
> +                                            pubx, puby, NULL) == 0) {
> +        HIP_ERROR("Failed to get x,y coordinates from the ECDH key\n");
> +        return -1;
> +    }
> +
> +    len = BN_num_bytes(pubx);
> +    HIP_IFEL(outlen < len * 2, -1, "Output buffer too small\n");
> +
> +    bn2bin_safe(pubx, out, outlen / 2);
> +    bn2bin_safe(puby, out + outlen / 2, outlen / 2);
> +
> +    return outlen;
> +
> +out_err:
> +    BN_free(pubx);
> +    BN_free(puby);
> +    return err;
> +}

The use of the "err" variable is completely silly; it is never assigned
to after initialization.  Just get rid of it.

> --- libhipl/cookie.c	2012-06-03 10:26:55 +0000
> +++ libhipl/cookie.c	2012-10-28 16:34:23 +0000
> @@ -219,41 +218,93 @@
>  
> +struct hip_common *hip_get_r1_v2(struct in6_addr *ip_i, struct in6_addr *ip_r,
> +                                 struct in6_addr *our_hit, const int dh_group_id)
> +{
> +    struct hip_common    *r1  = NULL;
> +    struct local_host_id *hid = NULL;
> +    int                   idx, len;
> +
> +    /* Find the proper R1 table and copy the R1 message from the table */
> +    hid = hip_get_hostid_entry_by_lhi_and_algo(our_hit, HIP_ANY_ALGO, -1);
> +    if (hid == NULL) {
> +        HIP_ERROR("Unknown HIT\n");
> +        return NULL;
> +    }
> +
> +    idx = calc_cookie_idx(ip_i, ip_r);
> +    HIP_DEBUG("Calculated index: %d\n", idx);
> +
> +    /* Create a copy of the found entry */
> +    len = hip_get_msg_total_len(&hid->r1_v2[dh_group_id][idx].buf.msg);
> +    if (len <= 0) {
> +        HIP_ERROR("Invalid r1_v2 entry at %d, %d\n", dh_group_id, idx);
> +        return NULL;
> +    }
> +
> +    if ((r1 = hip_msg_alloc()) == NULL) {
> +        return NULL;
> +    }
> +
> +    memcpy(r1, &hid->r1_v2[dh_group_id][idx].buf.msg, len);
> +
> +    return r1;
> +}

This is too close to hip_get_r1 for comfort in terms of code duplication.

> --- libhipl/dh.c	2012-05-12 10:21:32 +0000
> +++ libhipl/dh.c	2012-10-28 16:34:23 +0000
> @@ -95,66 +97,288 @@
>  }
>  
>  /**
> - * create a shared secret based on the public key of the peer
> + * Store the bytes of the current ECDH public key in the given buffer.
> + *
> + * A new ECDH key will be created if it doesn't exist,
> + *
> + * @param buffer   buffer to store the public part of the ECDH key
> + * @param bufsize  size of the @c buffer
> + * @param group_id group ID of the ECDH key
> + * @return         the number of bytes written to the buffer, -1 on error.
> + */
> +int hip_insert_ecdh(uint8_t *buffer, int bufsize, int group_id)
> +{
> +    EC_KEY *key;
> +    int     ret;
> +
> +    if (!hip_is_ecdh_group(group_id)) {
> +        HIP_ERROR("Invalid group id for ECDH: %d\n", group_id);
> +        return -1;
> +    }
> +
> +    if (ecdh_table[group_id] == NULL) {
> +        key = hip_generate_ecdh_key(group_id);
> +        if (key == NULL) {
> +            HIP_ERROR("Failed to generate an ECDH key for group: %d\n",
> +                      group_id);
> +            return -1;
> +        }
> +        ecdh_table[group_id] = key;
> +    }
> +
> +    key = ecdh_table[group_id];
> +    if ((ret = hip_encode_ecdh_publickey(key, buffer, bufsize)) < 0) {
> +        HIP_ERROR("Failed to encode the ECDH public key\n");
> +        return -1;
> +    }
> +
> +    return ret;
> +}

This looks awfully similar to hip_insert_dh().

> +static int regen_dh_key(const int group_id)
>  {
>      DH *tmp, *okey;
> +
> +    tmp = hip_generate_dh_key(group_id);
> +    if (!tmp) {
> +        HIP_INFO("Failed to generate a DH key for group: %d\n", group_id);
> +        return -1;
> +    }
> +
> +    okey               = dh_table[group_id];
> +    dh_table[group_id] = tmp;
> +
> +    DH_free(okey);
> +    return 0;
> +}
> +
> +#ifdef HAVE_EC_CRYPTO
> +static int regen_ecdh_key(const int group_id)
> +{
> +    EC_KEY *tmp, *okey;
> +
> +    tmp = hip_generate_ecdh_key(group_id);
> +    if (!tmp) {
> +        HIP_INFO("Failed to generate an ECDH key for group: %d\n", group_id);
> +        return -1;
> +    }
> +
> +    okey                 = ecdh_table[group_id];
> +    ecdh_table[group_id] = tmp;
> +
> +    EC_KEY_free(okey);
> +    return 0;
> +}
> +#endif /* HAVE_EC_CRYPTO */

These two also look awfully similar.

> @@ -205,10 +431,13 @@
>  
>      supported_groups = (1 << HIP_DH_OAKLEY_1 |
>                          1 << HIP_DH_OAKLEY_5 |
> -                        1 << HIP_DH_384);
> +                        1 << HIP_DH_384 |
> +                        1 << HIP_DH_NIST_P_256 |
> +                        1 << HIP_DH_NIST_P_384 |
> +                        1 << HIP_DH_NIST_P_521);

nit: This would look nicer with the | vertically aligned.

> --- libhipl/input.c	2012-07-13 13:16:17 +0000
> +++ libhipl/input.c	2012-10-28 16:34:23 +0000
> @@ -1464,13 +1526,47 @@
>  
> +    /* Verify cookie */
> +    if (hip_version == HIP_V1) {
> +        HIP_IFEL(hip_verify_cookie(&ctx->src_addr, &ctx->dst_addr,
> +                                   ctx->input_msg, solution,
> +                                   hip_version, 0),
> +                 -EPROTO,
> +                 "Cookie solution rejected. Dropping the I2 packet.\n");
> +    } else {
> +        HIP_IFEL(hip_verify_cookie(&ctx->src_addr, &ctx->dst_addr,
> +                                   ctx->input_msg, solution,
> +                                   hip_version, dh_group_id),
> +                 -EPROTO,
> +                 "Cookie solution rejected. Dropping the I2 packet.\n");
> +    }

Set dh_group_id to 0 instead and avoid some code duplication.

> --- libhipl/output.c	2012-06-07 12:45:16 +0000
> +++ libhipl/output.c	2012-10-28 16:34:23 +0000
> @@ -776,6 +851,172 @@
>  
> +int hip_create_r1_v2(struct hip_common *const msg,
> +                     const struct in6_addr *const src_hit,
> +                     int (*sign)(void *const key, struct hip_common *const m),
> +                     void *const private_key,
> +                     const struct hip_host_id *const host_id_pub,
> +                     const int cookie_k,
> +                     const int dh_group_id)
> +{

The amount of code duplication is just staggering.

> --- test/libcore/crypto.c	2012-05-12 06:54:33 +0000
> +++ test/libcore/crypto.c	2012-10-28 16:34:23 +0000
> @@ -28,12 +28,235 @@
>  
>  #include "libcore/crypto.h"
>  #include "config.h"
>  #include "test_suites.h"
>  
> +
>  #ifdef HAVE_EC_CRYPTO
> +
> +static const int TEST_ECDH_PRIV_A = 0;
> +static const int TEST_ECDH_PUBX_A = 1;

nit: stray empty line before the #ifdef?

> +    switch (group_id) {
> +    case HIP_DH_NIST_P_256:
> +        data_set = TEST_ECDH_NIST_P_256;
> +        break;
> +
> +    case HIP_DH_NIST_P_384:
> +        data_set = TEST_ECDH_NIST_P_384;
> +        break;
> +
> +    case HIP_DH_NIST_P_521:
> +        data_set = TEST_ECDH_NIST_P_521;
> +        break;
> +
> +    default:
> +        return NULL;
> +    }

nit: silly empty lines, more in other places

Diego