From: Tobias Brunner Date: Fri, 23 Jul 2021 15:29:41 +0000 (+0200) Subject: wolfssl: Move shared secret calculation to get_shared_secret() X-Git-Tag: 5.9.7dr2~1^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4e5a2037e7b48eed1fd012bfb60ab09cc0fb235c;p=thirdparty%2Fstrongswan.git wolfssl: Move shared secret calculation to get_shared_secret() The ECDH implementation gets a bit simpler since we removed the ecp_x_coordinate_only option a while ago. Also added calls to verify public keys. --- diff --git a/src/libstrongswan/plugins/wolfssl/wolfssl_diffie_hellman.c b/src/libstrongswan/plugins/wolfssl/wolfssl_diffie_hellman.c index 9856e8d40d..2f64c48000 100644 --- a/src/libstrongswan/plugins/wolfssl/wolfssl_diffie_hellman.c +++ b/src/libstrongswan/plugins/wolfssl/wolfssl_diffie_hellman.c @@ -68,6 +68,11 @@ struct private_wolfssl_diffie_hellman_t { */ chunk_t pub; + /** + * Public key provided by peer + */ + chunk_t other; + /** * Shared secret */ @@ -84,9 +89,19 @@ METHOD(key_exchange_t, get_public_key, bool, METHOD(key_exchange_t, get_shared_secret, bool, private_wolfssl_diffie_hellman_t *this, chunk_t *secret) { + word32 len; + if (!this->shared_secret.len) { - return FALSE; + this->shared_secret = chunk_alloc(this->len); + if (wc_DhAgree(&this->dh, this->shared_secret.ptr, &len, this->priv.ptr, + this->priv.len, this->other.ptr, this->other.len) != 0) + { + DBG1(DBG_LIB, "DH shared secret computation failed"); + chunk_free(&this->shared_secret); + return FALSE; + } + this->shared_secret.len = len; } *secret = chunk_copy_pad(chunk_alloc(this->len), this->shared_secret, 0x00); return TRUE; @@ -95,23 +110,17 @@ METHOD(key_exchange_t, get_shared_secret, bool, METHOD(key_exchange_t, set_public_key, bool, private_wolfssl_diffie_hellman_t *this, chunk_t value) { - word32 len; - if (!key_exchange_verify_pubkey(this->group, value)) { return FALSE; } - - chunk_clear(&this->shared_secret); - this->shared_secret = chunk_alloc(this->len); - if (wc_DhAgree(&this->dh, this->shared_secret.ptr, &len, this->priv.ptr, - this->priv.len, value.ptr, value.len) != 0) + if (wc_DhCheckPubKey(&this->dh, value.ptr, value.len) != 0) { - DBG1(DBG_LIB, "DH shared secret computation failed"); - chunk_free(&this->shared_secret); + DBG1(DBG_LIB, "DH public key value invalid"); return FALSE; } - this->shared_secret.len = len; + chunk_clear(&this->other); + this->other = chunk_clone(value); return TRUE; } @@ -153,6 +162,7 @@ METHOD(key_exchange_t, destroy, void, wc_FreeDhKey(&this->dh); chunk_clear(&this->pub); chunk_clear(&this->priv); + chunk_clear(&this->other); chunk_clear(&this->shared_secret); free(this); } diff --git a/src/libstrongswan/plugins/wolfssl/wolfssl_ec_diffie_hellman.c b/src/libstrongswan/plugins/wolfssl/wolfssl_ec_diffie_hellman.c index d622e14dbc..4ee286f712 100644 --- a/src/libstrongswan/plugins/wolfssl/wolfssl_ec_diffie_hellman.c +++ b/src/libstrongswan/plugins/wolfssl/wolfssl_ec_diffie_hellman.c @@ -62,29 +62,17 @@ struct private_wolfssl_ec_diffie_hellman_t { */ ecc_key key; + /** + * Public key provided by peer + */ + ecc_key pubkey; + /** * Shared secret */ chunk_t shared_secret; }; -/** - * Convert a chunk to an ecc_point (which must already exist). The x and y - * coordinates of the point have to be concatenated in the chunk. - */ -static bool chunk2ecp(chunk_t chunk, ecc_point *point) -{ - if (!wolfssl_mp_split(chunk, point->x, point->y)) - { - return FALSE; - } - if (mp_set(point->z, 1) != 0) - { - return FALSE; - } - return TRUE; -} - /** * Convert an ec_point to a chunk by concatenating the x and y coordinates of * the point. This function allocates memory for the chunk. @@ -138,69 +126,30 @@ static bool wolfssl_ecc_multiply(const ecc_set_type *ecc_set, mp_int *scalar, return ret == 0; } -/** - * Compute the shared secret. - * - * We cannot use the function wc_ecc_shared_secret() because that returns only - * the x coordinate of the shared secret point (which is defined, for instance, - * in 'NIST SP 800-56A'). - * However, we need both coordinates as RFC 4753 says: "The Diffie-Hellman - * public value is obtained by concatenating the x and y values. The format - * of the Diffie-Hellman shared secret value is the same as that of the - * Diffie-Hellman public value." - */ -static bool compute_shared_key(private_wolfssl_ec_diffie_hellman_t *this, - ecc_point *pub_key, chunk_t *shared_secret) -{ - ecc_point* secret; - bool success = FALSE; - - if ((secret = wc_ecc_new_point()) == NULL) - { - return FALSE; - } - - if (wolfssl_ecc_multiply(this->key.dp, &this->key.k, pub_key, secret)) - { - success = ecp2chunk(this->keysize, secret, shared_secret, TRUE); - } - - wc_ecc_del_point(secret); - return success; -} - METHOD(key_exchange_t, set_public_key, bool, private_wolfssl_ec_diffie_hellman_t *this, chunk_t value) { - ecc_point *pub_key; + chunk_t uncomp; if (!key_exchange_verify_pubkey(this->group, value)) { return FALSE; } - if ((pub_key = wc_ecc_new_point()) == NULL) - { - return FALSE; - } - - if (!chunk2ecp(value, pub_key)) + /* prepend 0x04 to indicate uncompressed point format */ + uncomp = chunk_cata("cc", chunk_from_chars(0x04), value); + if (wc_ecc_import_x963_ex(uncomp.ptr, uncomp.len, &this->pubkey, + this->curve_id) != 0) { DBG1(DBG_LIB, "ECDH public value is malformed"); - wc_ecc_del_point(pub_key); return FALSE; } - chunk_clear(&this->shared_secret); - - if (!compute_shared_key(this, pub_key, &this->shared_secret)) + if (wc_ecc_check_key(&this->pubkey) != 0) { - DBG1(DBG_LIB, "ECDH shared secret computation failed"); - chunk_clear(&this->shared_secret); - wc_ecc_del_point(pub_key); + DBG1(DBG_LIB, "ECDH public value is invalid"); return FALSE; } - wc_ecc_del_point(pub_key); return TRUE; } @@ -248,10 +197,51 @@ METHOD(key_exchange_t, set_private_key, bool, return success; } +/** + * Derive the shared secret + */ +static bool compute_shared_key(private_wolfssl_ec_diffie_hellman_t *this) +{ + word32 len; +#ifdef ECC_TIMING_RESISTANT + WC_RNG rng; + + if (wc_InitRng(&rng) != 0) + { + return FALSE; + } + if (wc_ecc_set_rng(&this->key, &rng) != 0) + { + wc_FreeRng(&rng); + return FALSE; + } +#endif + + this->shared_secret = chunk_alloc(this->keysize); + len = this->shared_secret.len; + + if (wc_ecc_shared_secret(&this->key, &this->pubkey, this->shared_secret.ptr, + &len) != 0) + { + DBG1(DBG_LIB, "ECDH shared secret computation failed"); + chunk_clear(&this->shared_secret); +#ifdef ECC_TIMING_RESISTANT + wc_FreeRng(&rng); +#endif + return FALSE; + } + this->shared_secret.len = len; +#ifdef ECC_TIMING_RESISTANT + wc_FreeRng(&rng); +#endif + return TRUE; +} + METHOD(key_exchange_t, get_shared_secret, bool, private_wolfssl_ec_diffie_hellman_t *this, chunk_t *secret) { - if (!this->shared_secret.len) + if (!this->shared_secret.len && + !compute_shared_key(this)) { return FALSE; } @@ -269,6 +259,7 @@ METHOD(key_exchange_t, destroy, void, private_wolfssl_ec_diffie_hellman_t *this) { wc_ecc_free(&this->key); + wc_ecc_free(&this->pubkey); chunk_clear(&this->shared_secret); free(this); } @@ -295,10 +286,10 @@ wolfssl_ec_diffie_hellman_t *wolfssl_ec_diffie_hellman_create(key_exchange_metho .group = group, ); - if (wc_ecc_init(&this->key) != 0) + if (wc_ecc_init(&this->key) != 0 || wc_ecc_init(&this->pubkey) != 0) { DBG1(DBG_LIB, "key init failed, ecdh create failed"); - free(this); + destroy(this); return NULL; } diff --git a/src/libstrongswan/plugins/wolfssl/wolfssl_x_diffie_hellman.c b/src/libstrongswan/plugins/wolfssl/wolfssl_x_diffie_hellman.c index f2aca03e46..f2361f2efb 100644 --- a/src/libstrongswan/plugins/wolfssl/wolfssl_x_diffie_hellman.c +++ b/src/libstrongswan/plugins/wolfssl/wolfssl_x_diffie_hellman.c @@ -68,6 +68,18 @@ struct private_diffie_hellman_t { #endif } key; + /** + * Public key provided by peer + */ + union { +#ifdef HAVE_CURVE25519 + curve25519_key key25519; +#endif +#ifdef HAVE_CURVE448 + curve448_key key448; +#endif + } pub; + /** * Shared secret */ @@ -76,47 +88,42 @@ struct private_diffie_hellman_t { #ifdef HAVE_CURVE25519 -METHOD(key_exchange_t, set_public_key_25519, bool, - private_diffie_hellman_t *this, chunk_t value) +METHOD(key_exchange_t, get_shared_secret_25519, bool, + private_diffie_hellman_t *this, chunk_t *secret) { word32 len = CURVE25519_KEYSIZE; - curve25519_key pub; - int ret; - if (!key_exchange_verify_pubkey(this->group, value)) + if (!this->shared_secret.len) { - return FALSE; + this->shared_secret = chunk_alloc(len); + if (wc_curve25519_shared_secret_ex(&this->key.key25519, &this->pub.key25519, + this->shared_secret.ptr, &len, EC25519_LITTLE_ENDIAN) != 0) + { + DBG1(DBG_LIB, "%N shared secret computation failed", + key_exchange_method_names, this->group); + chunk_clear(&this->shared_secret); + return FALSE; + } } + *secret = chunk_clone(this->shared_secret); + return TRUE; +} - ret = wc_curve25519_init(&pub); - if (ret != 0) +METHOD(key_exchange_t, set_public_key_25519, bool, + private_diffie_hellman_t *this, chunk_t value) +{ + if (!key_exchange_verify_pubkey(this->group, value)) { - DBG1(DBG_LIB, "%N public key initialization failed", - key_exchange_method_names, this->group); return FALSE; } - ret = wc_curve25519_import_public_ex(value.ptr, value.len, &pub, - EC25519_LITTLE_ENDIAN); - if (ret != 0) + if (wc_curve25519_import_public_ex(value.ptr, value.len, &this->pub.key25519, + EC25519_LITTLE_ENDIAN) != 0) { DBG1(DBG_LIB, "%N public value is malformed", key_exchange_method_names, this->group); return FALSE; } - - chunk_clear(&this->shared_secret); - this->shared_secret = chunk_alloc(len); - if (wc_curve25519_shared_secret_ex(&this->key.key25519, &pub, - this->shared_secret.ptr, &len, EC25519_LITTLE_ENDIAN) != 0) - { - DBG1(DBG_LIB, "%N shared secret computation failed", - key_exchange_method_names, this->group); - chunk_clear(&this->shared_secret); - wc_curve25519_free(&pub); - return FALSE; - } - wc_curve25519_free(&pub); return TRUE; } @@ -168,47 +175,42 @@ METHOD(key_exchange_t, set_private_key_25519, bool, #ifdef HAVE_CURVE448 -METHOD(key_exchange_t, set_public_key_448, bool, - private_diffie_hellman_t *this, chunk_t value) +METHOD(key_exchange_t, get_shared_secret_448, bool, + private_diffie_hellman_t *this, chunk_t *secret) { word32 len = CURVE448_KEY_SIZE; - curve448_key pub; - int ret; - if (!key_exchange_verify_pubkey(this->group, value)) + if (!this->shared_secret.len) { - return FALSE; + this->shared_secret = chunk_alloc(len); + if (wc_curve448_shared_secret_ex(&this->key.key448, &this->pub.key448, + this->shared_secret.ptr, &len, EC448_LITTLE_ENDIAN) != 0) + { + DBG1(DBG_LIB, "%N shared secret computation failed", + key_exchange_method_names, this->group); + chunk_clear(&this->shared_secret); + return FALSE; + } } + *secret = chunk_clone(this->shared_secret); + return TRUE; +} - ret = wc_curve448_init(&pub); - if (ret != 0) +METHOD(key_exchange_t, set_public_key_448, bool, + private_diffie_hellman_t *this, chunk_t value) +{ + if (!key_exchange_verify_pubkey(this->group, value)) { - DBG1(DBG_LIB, "%N public key initialization failed", - key_exchange_method_names, this->group); return FALSE; } - ret = wc_curve448_import_public_ex(value.ptr, value.len, &pub, - EC448_LITTLE_ENDIAN); - if (ret != 0) + if (wc_curve448_import_public_ex(value.ptr, value.len, &this->pub.key448, + EC448_LITTLE_ENDIAN) != 0) { DBG1(DBG_LIB, "%N public value is malformed", key_exchange_method_names, this->group); return FALSE; } - - chunk_clear(&this->shared_secret); - this->shared_secret = chunk_alloc(len); - if (wc_curve448_shared_secret_ex(&this->key.key448, &pub, - this->shared_secret.ptr, &len, EC448_LITTLE_ENDIAN) != 0) - { - DBG1(DBG_LIB, "%N shared secret computation failed", - key_exchange_method_names, this->group); - chunk_clear(&this->shared_secret); - wc_curve448_free(&pub); - return FALSE; - } - wc_curve448_free(&pub); return TRUE; } @@ -258,17 +260,6 @@ METHOD(key_exchange_t, set_private_key_448, bool, #endif /* HAVE_CURVE448 */ -METHOD(key_exchange_t, get_shared_secret, bool, - private_diffie_hellman_t *this, chunk_t *secret) -{ - if (!this->shared_secret.len) - { - return FALSE; - } - *secret = chunk_clone(this->shared_secret); - return TRUE; -} - METHOD(key_exchange_t, get_method, key_exchange_method_t, private_diffie_hellman_t *this) { @@ -282,12 +273,14 @@ METHOD(key_exchange_t, destroy, void, { #ifdef HAVE_CURVE25519 wc_curve25519_free(&this->key.key25519); + wc_curve25519_free(&this->pub.key25519); #endif } else if (this->group == CURVE_448) { #ifdef HAVE_CURVE448 wc_curve448_free(&this->key.key448); + wc_curve448_free(&this->pub.key448); #endif } chunk_clear(&this->shared_secret); @@ -305,7 +298,6 @@ key_exchange_t *wolfssl_x_diffie_hellman_create(key_exchange_method_t group) INIT(this, .public = { - .get_shared_secret = _get_shared_secret, .get_method = _get_method, .destroy = _destroy, }, @@ -322,14 +314,16 @@ key_exchange_t *wolfssl_x_diffie_hellman_create(key_exchange_method_t group) if (group == CURVE_25519) { #ifdef HAVE_CURVE25519 + this->public.get_shared_secret = _get_shared_secret_25519; this->public.set_public_key = _set_public_key_25519; this->public.get_public_key = _get_public_key_25519; this->public.set_private_key = _set_private_key_25519; - if (wc_curve25519_init(&this->key.key25519) != 0) + if (wc_curve25519_init(&this->key.key25519) != 0 || + wc_curve25519_init(&this->pub.key25519) != 0) { DBG1(DBG_LIB, "initializing key failed"); - free(this); + destroy(this); return NULL; } ret = wc_curve25519_make_key(&rng, CURVE25519_KEYSIZE, @@ -339,14 +333,16 @@ key_exchange_t *wolfssl_x_diffie_hellman_create(key_exchange_method_t group) else if (group == CURVE_448) { #ifdef HAVE_CURVE448 + this->public.get_shared_secret = _get_shared_secret_448; this->public.set_public_key = _set_public_key_448; this->public.get_public_key = _get_public_key_448; this->public.set_private_key = _set_private_key_448; - if (wc_curve448_init(&this->key.key448) != 0) + if (wc_curve448_init(&this->key.key448) != 0 || + wc_curve448_init(&this->pub.key448) != 0) { DBG1(DBG_LIB, "initializing key failed"); - free(this); + destroy(this); return NULL; } ret = wc_curve448_make_key(&rng, CURVE448_KEY_SIZE, &this->key.key448);