Synchronize prop224 spec with implementation

In our ed25519 key blinding code we have a few pieces that are not in the spec. At the very least we have the following constant strings that get hashed, which are not mentioned in the spec:

  const char str[] = "Derive temporary signing key";
...
  const char str[] = "Derive temporary signing key hash input";

We should eye the implementation for any other unspecified parts, and bake them in the spec.

changed milestone to %Tor: 0.3.1.x-final

added component::core tor/tor ed25519 milestone::Tor: 0.3.1.x-final owner::asn parent::21888 points::1 priority::medium prop224 resolution::implemented severity::normal status::closed tor-hs tor-spec type::defect labels

Trac:
Owner: N/A to asn
Status: new to assigned

Here are a few places where the Tor code diverges from the spec. I'm CCing Nick here since he is the author of the ed25519 blinding code.

---

In ed25519_donna_gettweak() tor clamps the blinding factor h before using it to calculate a' = h*a and A' = h*A:

static void
ed25519_donna_gettweak(unsigned char *out, const unsigned char *param)
{
...
  out[0] &= 248;  /* Is this necessary ? */
  out[31] &= 63;
  out[31] |= 64;
}

As discussed in [tor-dev] and [curves], clamping is not necessary for us because we are only dealing with signatures: https://moderncrypto.org/mail-archive/curves/2017/000874.html

Also, starting in #22006 (moved) we validate received ed25519 pubkeys so we are not afraid that a received onion address pubkey will have a torsion component (hence clearing the lowest three bits is not necessary).

I actually don't think that clamping there is a terrible thing to do, but it reduces the strength of the blinding factor by 3 bits or something. I'm not sure if we should go ahead and remove the clamping or not, but we should probably mention it in the spec if we do keep it.

---

The Tor ed25519 implementation also adds a constant string to the hash input of the extended secret key form like this:

  static const char str[] = "Derive temporary signing key hash input";
...
  ed25519_hash_init(&ctx);
  ed25519_hash_update(&ctx, (const unsigned char*)str, strlen(str));
  ed25519_hash_update(&ctx, inp + 32, 32);
  ed25519_hash_final(&ctx, tweak);

  memcpy(out + 32, tweak, 32);

We should probably specify this as it's not in the spec.

---

The Tor ed25519 implementation also adds a constant string to the computation of the blinding factor. Making it:

   static const char str[] = "Derive temporary signing key hash input";
...
   h = H(str | H(A | s | B | N))

We should specify this as it's not in the spec.

Trac:
Cc: N/A to nickm

Another thing we should fix:

In prop224 we actually don't need KH as part of the key expansion procedure. In the legacy design we used KH as a key confirmation of the key expansion, however in prop224 we have a whole mac just for this AUTH_INPUT_MAC. So we actually don't need KH in the following paragraph:

   The hidden service and its client need to derive crypto keys from the
   NTOR_KEY_SEED part of the handshake output. To do so, they use the KDF
   construction as follows:

       K = KDF(NTOR_KEY_SEED | m_hsexpand,    HASH_LEN * 3 + S_KEY_LEN * 2)

   The first HASH_LEN bytes of K form KH; the next HASH_LEN form the forward
   digest Df; the next HASH_LEN bytes form the backward digest Db; the next
   S_KEY_LEN bytes form Kf, and the final S_KEY_LEN bytes form Kb.  Excess
   bytes from K are discarded.

Another thing we need to do is to add the time period period_length whenever we hash period_num, as suggested here: https://gitlab.com/dgoulet/tor/merge_requests/27#note_27696937

Trac:
Summary: Synchronize prop224 key blinding spec with implementation to Synchronize prop224 spec with implementation

Please check my branch bug22052 for some prop224 improvements based on this ticket.

Trac:
Status: assigned to needs_review
Keywords: tor-hs prop224 ed25519 deleted, tor-hs prop224 ed25519 tor-spec added

merged asn/bug22052 into torspec! thanks!

Trac:
Status: needs_review to closed
Resolution: N/A to implemented

closed

changed time estimate to 8h

moved to tpo/core/tor#22052 (closed)