[tor-bugs] #4125 [Tor Relay]: Implement proposal 176 (renegotiation-free handshake)

[Tor Bug Tracker & Wiki]

#4125: Implement proposal 176 (renegotiation-free handshake)
-------------------------+--------------------------------------------------
 Reporter:  nickm        |          Owner:  nickm              
     Type:  enhancement  |         Status:  needs_review       
 Priority:  normal       |      Milestone:  Deliverable-Nov2011
Component:  Tor Relay    |        Version:                     
 Keywords:               |         Parent:                     
   Points:               |   Actualpoints:                     
-------------------------+--------------------------------------------------

Comment(by nickm):

 Replying to [comment:4 asn]:
 > Hi, I read some of the code today.
 >
 > Some things I noticed:
 >
 > * The man page of d2i_x509(3)
 [http://www.openssl.org/docs/crypto/d2i_X509.html] says:
 >
 > {{{For OpenSSL 0.9.7 and later if *out is NULL memory will be allocated
 for a buffer and the encoded data written to it. In this case *out is not
 incremented and it points to the start of the data just written.}}}
 >
 > You could use this instead of doing the `len = i2d_RSAPublicKey(pk->key,
 NULL); ...` thing.
 > It seems a bit more intuitive/readable (maybe).

 Not doing this one; I don't think we rely on being able to use tor_free()
 to free memory allocated by openssl.

 > * There is something fishy with the memory allocation of tor_cert_new()
 and its callers.
 > Its callers duplicate x509 cert when calling tor_cert_new(), and
 tor_cert_new() re-duplicates it. I don't see all the duplications getting
 freed.

 Fixed

 > * In tor_cert_new():
 > {{{
 >   if ((pkey = X509_get_pubkey(x509_cert)) &&
 >       (rsa = EVP_PKEY_get1_RSA(pkey))) {
 >     crypto_pk_env_t *pk = _crypto_new_pk_env_rsa(rsa);
 >     crypto_pk_get_all_digests(pk, &cert->pkey_digests);
 >     crypto_free_pk_env(pk);
 >   }
 >
 >   return cert;
 > }}}
 > - `pkey` is not freed.

 fixed

 > - Shouldn't this conditional also have an 'else' branch?

 Will try to think about what such a branch should do.

 > * In tor_tls_received_v3_certificate() the key is not freed if the "Key
 is fancy".

 Fixed

 > * Shouldn't `seen` and `expected` in
 connection_or_client_learned_peer_id() be of length (DIGEST_LEN+1)?

 Looks like they're HEX_DIGEST_LEN+1 to me.  What am I missing?
 {{{
     char seen[HEX_DIGEST_LEN+1];
     char expected[HEX_DIGEST_LEN+1];
 }}}

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/4125#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online