Opened 2 years ago

Closed 2 years ago

#24002 closed defect (fixed)

Check for ed25519 key is inverted in pick_intro_point()

Reported by: teor Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version: Tor: 0.3.2.1-alpha
Severity: Normal Keywords: prop224, tor-hs
Cc: Actual Points:
Parent ID: #23493 Points: 0.5
Reviewer: Sponsor:

Description

In pick_intro_point(), we should check that nodes that claim to support ed25519 have a key, not the other way around:

  /* Let's do a basic sanity check here so that we don't end up advertising the
   * ed25519 identity key of relays that don't actually support the link
   * protocol */
  if (!node_supports_ed25519_link_authentication(node)) {
    tor_assert_nonfatal(ed25519_public_key_is_zero(&info->ed_identity));
  }

Also, this check is already done in node_get_ed25519_id() via extend_info_for_node() for ri, but not md. So I think we could also fix this issue by fixing #24001 instead, and removing the check from pick_intro_point().

Child Tickets

Change History (6)

comment:1 Changed 2 years ago by dgoulet

Status: newneeds_information

So extend_info_from_node() does a series of checks (through other functions) before setting the ed25519 key in the extend info object. In particular:

  /* Don't send the ed25519 pubkey unless the target node actually supports
   * authenticating with it. */
  if (node_supports_ed25519_link_authentication(node)) {
    log_info(LD_CIRC, "Including Ed25519 ID for %s", node_describe(node));
    ed_pubkey = node_get_ed25519_id(node);

That node_get_ed25519_id() function checks both ri and md and make sure the ed key matches if they both exists. A NULL value can be returned but a log_warn() will happen or a BUG() which could ultimately lead to have a NULL ed key for a node that supports the ed25519 link auth. I think if that can happen, other places in Tor will go mad but lets be safe.

Then going back to the check that pick_intro_point() does, once reached, if the link auth is confirmed, we assume the ed key is set. If we don't have link auth support, we make sure to not advertise an ed key that will make the extend fail if we did.

All in all, I think we want that current check. We could be extra extra careful and check that we actually have an ed key if we support the link auth?

comment:2 Changed 2 years ago by teor

Parent ID: #23493
Status: needs_informationnew

Ok, then what we need in 0.3.2 is:

  • a comment that explains why we check for missing keys when nodes don't support ed25519
  • a comment that explains why we don't also check for keys being present when nodes do support ed25519 (if you're convinced this works, a BUG() warning would be a useful defence in depth)

Since we plan on changing this code in 0.3.3 to avoid using extend infos (#23576), I don't think we can rely on the checks in extend_info_from_node() in 0.3.3. So we can move the code changes in this ticket to 0.3.3 if you want.

comment:3 in reply to:  2 Changed 2 years ago by dgoulet

Keywords: tor-hs added
Owner: set to dgoulet
Status: newaccepted

Replying to teor:

Ok, then what we need in 0.3.2 is:

  • a comment that explains why we check for missing keys when nodes don't support ed25519

That one in the code? :)

  /* Let's do a basic sanity check here so that we don't end up advertising the
   * ed25519 identity key of relays that don't actually support the link
   * protocol */
  • a comment that explains why we don't also check for keys being present when nodes do support ed25519 (if you're convinced this works, a BUG() warning would be a useful defence in depth)

I would go for a check here. If we support the link auth, check that the ed key is non-zero. As you pointed out in some other ticket, nodes_get_ed25519_id() doesn't make the check for md so lets be safe.

Since we plan on changing this code in 0.3.3 to avoid using extend infos (#23576), I don't think we can rely on the checks in extend_info_from_node() in 0.3.3. So we can move the code changes in this ticket to 0.3.3 if you want.

I think the above is very reasonable for 032 and then we move on with #23756 in 033 to clean this thing up.

comment:4 Changed 2 years ago by dgoulet

Status: acceptedneeds_review

Added an extra check as discussed: bug24002_032_01

comment:5 Changed 2 years ago by teor

Status: needs_reviewmerge_ready

Looks good to me. Let's get it merged.

comment:6 Changed 2 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

merged to 0.3.2 and forward.

Note: See TracTickets for help on using tickets.