Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#17556 closed defect (not a bug)

Doc or implementation error in NTor handshake

Reported by: awick Owned by:
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Either the docs or the implementation seem to be off for the implementation of the NTor handshake. Specifically, the docs (in Section 5.1.4) state:

verify = H(secret_input, t_verify)

and

auth = H(auth_input, t_mac)

where H(x,t) is defined as HMAC_SHA256 with message x and key t.

Looking into the source code, the implementation of these two is via the function h_tweak. In all cases, h_tweak is called with h_tweak(input, input_length, t_value). However, it then calls the underlying hmac function with the arguments reversed. This has the effect of redefining verify as H(t_verify, secret_input) and auth as H(t_mac, auth_input).

I'm not sure what the security implications of this are, but it is confusing. If there is no difference in the security of the result, it'd obviously be easier to update the docs. Otherwise, the patch to h_tweak should be obvious, but it will make handshakes with previous implementations fail.

Child Tickets

Change History (5)

comment:1 Changed 4 years ago by yawning

Resolution: not a bug
Status: newclosed

crypto_hmac_sha256((char*)out, tweak, tweak_len, (const char*)inp, inp_len);

calls:

void
crypto_hmac_sha256(char *hmac_out,
                   const char *key, size_t key_len,
                   const char *msg, size_t msg_len)
{
  /* If we've got OpenSSL >=0.9.8 we can use its hmac implementation. */
  tor_assert(key_len < INT_MAX);
  tor_assert(msg_len < INT_MAX);
  HMAC(EVP_sha256(), key, (int)key_len, (unsigned char*)msg, (int)msg_len,
       (unsigned char*)hmac_out, NULL);
}

Resulting in behavior as specified (The spec defines H(x,t), the code expects to be called as H(t,x)). The spec would be clearer and more in line with how HMAC constructs are usually defined if everything was written down as H(t,x), but there's nothing wrong with either the code or spec at present beyond being confusing.

comment:2 Changed 4 years ago by awick

Resolution: not a bug
Status: closedreopened

Sorry, going to reopen again, because I think there is still an issue.

(As an aside, I'm finding this because I'm implementing this section of the protocol, and I'm finding a discrepancy between how KEY_SEED, verify, and auth are computed. According to the docs, they should all be computed the same way. It is not a bug in my crypto library or in the input; I've verified the inputs are identical between the implementations, my HMAC_SHA256 is correct, and I verified that swapping inputs for two of the three values makes the handshake work.)

Looking at them in more detail:

In the case of verify / auth / h_tweak, just as you say, the eventual call to crypto_hmac_sha256 turns into crypto_hmac_sha256(out, T->t_mac, s.auth_input).

However, In the case of KEY_SEED / crypto_expand_key_maerial_rfc5869_sha256(), the eventual call to crypto_hmac_sha256 turns into `crypto_hmac_sha256(prk, s.secret_input, T->t_key);

By the docs, these should be the same, as they are all defined as H(something, tweak).

comment:3 Changed 4 years ago by yawning

Resolution: not a bug
Status: reopenedclosed

Both of the calls to HKDF-SHA256 look like this:

  crypto_expand_key_material_rfc5869_sha256(
                           s.secret_input, sizeof(s.secret_input),
                           (const uint8_t*)T->t_key, strlen(T->t_key),
                           (const uint8_t*)T->m_expand, strlen(T->m_expand),
                           key_out, key_out_len);

The relevant portions of the function being called looks like this:

int
crypto_expand_key_material_rfc5869_sha256(
                                    const uint8_t *key_in, size_t key_in_len,
                                    const uint8_t *salt_in, size_t salt_in_len,
                                    const uint8_t *info_in, size_t info_in_len,
                                    uint8_t *key_out, size_t key_out_len)
{
  // Blah blah blah.

  crypto_hmac_sha256((char*)prk,
                     (const char*)salt_in, salt_in_len,
                     (const char*)key_in, key_in_len);

salt_in is the tweak (T->t_key), key_in is the secret(s.secret_input), so despite your insistence to the contrary, this is crypto_hmac_sha256(prk, T->t_key, s.secret_input).

Still not a bug.

comment:4 Changed 4 years ago by awick

Right, right. Sorry, sorry. Misread something in my own code, which I just found. Many apologies, thanks for your patience, etc.

comment:5 in reply to:  4 Changed 4 years ago by yawning

Replying to awick:

Right, right. Sorry, sorry. Misread something in my own code, which I just found. Many apologies, thanks for your patience, etc.

It's fine. To be honest I wouldn't be opposed to a spec change to specify ntor with H(t,x) so that this confusion is avoided in the future, but my plate's rather full at the moment.

Note: See TracTickets for help on using tickets.