Opened 2 years ago

Closed 2 years ago

#22052 closed defect (implemented)

Synchronize prop224 spec with implementation

Reported by: asn Owned by: asn
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs prop224 ed25519 tor-spec
Cc: nickm Actual Points:
Parent ID: #21888 Points: 1
Reviewer: Sponsor:

Description

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.

Child Tickets

Change History (6)

comment:1 Changed 2 years ago by dgoulet

Owner: set to asn
Status: newassigned

comment:2 Changed 2 years ago by asn

Cc: nickm added

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 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.

Last edited 2 years ago by asn (previous) (diff)

comment:3 Changed 2 years ago by asn

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.

comment:4 Changed 2 years ago by asn

Summary: Synchronize prop224 key blinding spec with implementationSynchronize prop224 spec with implementation

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

comment:5 Changed 2 years ago by asn

Keywords: tor-spec added
Status: assignedneeds_review

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

comment:6 Changed 2 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

merged asn/bug22052 into torspec! thanks!

Note: See TracTickets for help on using tickets.