[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/hipv2-dh-ecdh into lp:hipl
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: hipl-dev@xxxxxxxxxxxxx
- Date: Mon, 29 Oct 2012 20:48:41 +0100
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
Other related posts:
