Re: [tor-bugs] #22460 [Core Tor/Tor]: Link handshake trouble: certificates and keys can get out of sync

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#22460: Link handshake trouble: certificates and keys can get out of sync
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  High                                 |      Milestone:  Tor:
                                                 |  0.3.1.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Major                                |     Resolution:
 Keywords:  tor-relay certs handshake ed25519    |  Actual Points:  1
  needs-analysis 030-backport 029-backport       |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by asn):

 Review:

 `bug22460_030_01`: Looks good, but I get a bit anxious where we
 conditionally set `s->own_link_cert`, but then we unconditionally use it
 in `connection_or_send_certs_cell()`:
 {{{
 +  if (! started_here && get_current_link_cert_cert()) {
 +    s->own_link_cert = tor_cert_dup(get_current_link_cert_cert());
 +  }
 ...
      add_ed25519_cert(certs_cell,
                       CERTTYPE_ED_SIGN_LINK,
 -                     get_current_link_cert_cert());
 +                     conn->handshake_state->own_link_cert);
 }}}

 Are we sure that there is no chance we will leave `own_link_cert`
 uninitialized? If there is such a chance, should we default to
 `get_current_link_cert_cert()` if `own_link_cert` is unset in
 `connection_or_send_certs_cell()`?

 ----

 `bug22460_case2_029_01`: Seems like `tor_tls_get_peer_cert()` and
 `tor_tls_get_own_cert()` are the same function but with a different log
 message? Am i right, that we did that so that we can mock one function but
 not the other?

 And is it for the same mocking reason we use both functions here, even tho
 the functionality is the same?
 {{{
     if (server) {
       cert = tor_tls_get_own_cert(conn->tls);
     } else {
       cert = tor_tls_get_peer_cert(conn->tls);
     }
 }}}

 I guess the code duplication is a bit naughty here, as well as the name
 similarity between `tor_tls_get_own_cert()` and `tor_tls_get_my_certs()`
 (even tho they do a different thing IIUC). But if this is just for 0.2.9
 then it's probably OK.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/22460#comment:34>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs