[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

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

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

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

 * 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.
 - Shouldn't this conditional also have an 'else' branch?

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

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

 * In or_handshake_state_record_cell() and
 or_handshake_state_record_var_cell() in the case of:
 {{{
   if (! *dptr)
      *dptr = crypto_new_digest256_env(DIGEST_SHA256);
 }}}
 shouldn't the new `dptr` be attached somewhere?

 I'll read the code more closely on Tuesday/Wednesday when my next exams
 are over!

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/4125#comment:4>
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