otrv4 / libotr-ng

A new implementation of OTR with support for version 4. This is a mirror of https://bugs.otr.im/otrv4/libotr-ng
Other
43 stars 9 forks source link

The ecdh and dh keys are calculated twice for different purposes #174

Closed claucece closed 5 years ago

claucece commented 5 years ago

It is first calculated here:

tstatic otrng_result generate_tmp_key_r(uint8_t *dst, otrng_s *otr) {
  k_ecdh tmp_ecdh_k1;
  k_ecdh tmp_ecdh_k2;
  k_ecdh ecdh_key;
  dh_shared_secret dh_key;
  size_t k_dh_len = 0;
  k_brace brace_key;

  // TODO: @refactoring this will be calculated again later
  if (!otrng_ecdh_shared_secret(ecdh_key, ED448_POINT_BYTES,
                                otr->keys->our_ecdh->priv,
                                otr->keys->their_ecdh)) {
    return OTRNG_ERROR;
  }

  // TODO: @refactoring this will be calculated again later
  if (!otrng_dh_shared_secret(dh_key, &k_dh_len, otr->keys->our_dh->priv,
                              otr->keys->their_dh)) {
    return OTRNG_ERROR;
  }

and here:

INTERNAL otrng_result otrng_key_manager_generate_shared_secret(
    key_manager_s *manager, const otrng_bool interactive) {

  if (interactive) {
    k_ecdh ecdh_key;

    if (!otrng_ecdh_shared_secret(ecdh_key, ED448_POINT_BYTES,
                                  manager->our_ecdh->priv,
                                  manager->their_ecdh)) {
      return OTRNG_ERROR;
    }

This could be refactored.

olabini commented 5 years ago

Simple cleanup. I'll take care of it.

claucece commented 5 years ago

This refers to this TODO: // TODO: @refactoring this workaround is not the nicest there is on the otrng.c file.

olabini commented 5 years ago

Perfect. Easier to find now.

olabini commented 5 years ago

So, I started looking at this, but it got me a bit confused. Specifically, the things that look like they are duplicated are in generate_tmp_key_i and generate_tmp_key_r in otrng.c. However, the thing that seems to be duplicated is in otrng_key_manager_generate_shared_secret. But looking at this, it doesn't actually seem to do the calculation two times. Specifically, generate_tmp_key_i/r are only called in the non-interactive path, while the branch of otrng_key_manager_generate_shared_secret that calculates the ecdh and dh shared values is only in the interactive path.

Unless I'm misunderstanding something here, I don't think there's a problem, Can you please review and tell me if I'm wrong?

claucece commented 5 years ago

Unless I'm misunderstanding something here, I don't think there's a problem, Can you please review and tell me if I'm wrong?

You are right. All is good now.