Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#4125 closed enhancement (implemented)

Implement proposal 176 (renegotiation-free handshake)

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Deliverable-Nov2011
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

To avoid distinguishability, and to make our protocol easier to implement longterm, we've got proposal 176 pending, which moves our link protocol to v3 and lets us handshake without renegotiation (and indeed without any weird-looking certs at all).

I have an implementation in a branch "prop176-hacking" in my public repository. I still need to clean it up and test it more before it's ready for review, though. When I'm going to stop rebasing it, I will change its name to prop176 or something.

Then again, why not have a look now?

Child Tickets

Change History (18)

comment:1 Changed 8 years ago by nickm

Owner: set to nickm
Status: newassigned

comment:2 Changed 8 years ago by nickm

Okay, I just got it bootstrapping with bufferevents too.

Stuff I still want to test: interoperability with 0.2.2; interoperability with Tor clients and servers using only the v1 handshake.

Stuff I want to double-check: that we implement all the protections that are required in the proposal; that our implementation matches the proposal.

Stuff I want to clean up: I should squash bugfix commits on top of the commits that introduce the bugs (where possible). I should fix all XXXs; I should fix all DOCDOCs; I should add lots more comments too.

comment:3 Changed 8 years ago by nickm

Status: assignedneeds_review

It is now squashed and documented enough that I have a version which it isn't embarrassing for others to read.

Now see the branch "prop176". This one won't get rebased; if I want to rebase, I'll make a new branch called something else.

There are some remaining places I want to fix up, and a couple more things I want to test, but there is no reason it shouldn't get wider attention now.

comment:4 Changed 8 years ago by asn

Hi, I read some of the code today.

Some things I noticed:

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!

comment:5 in reply to:  4 Changed 8 years ago by asn

Replying to asn:

  • 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?

Oops, just read that code carefully again, dptr is already attached to state. nvm.

comment:6 Changed 8 years ago by asn

  • If command_process_cert_cell receives a CERT cell, with two OR_CERT_TYPE_TLS_LINK certificates it will decode, ignore and *not* free the second one, because of:
            if (cert_type == OR_CERT_TYPE_TLS_LINK && !link_cert)
              link_cert = cert;
            ...
            else
              tor_cert_free(cert);
    

The same goes for the other types of allowed certificates.

  • In command_process_auth_challenge_cell() don't forget to use ERR() or return in:
        if (connection_or_send_authenticate_cell(conn, use_type) < 0) {
          /* XXX log */
          connection_mark_for_close(TO_CONN(conn));
        }
    

comment:7 in reply to:  4 Changed 8 years ago by nickm

Replying to asn:

Hi, I read some of the code today.

Some things I noticed:

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];

comment:8 Changed 8 years ago by rransom

In connection_or_check_valid_tls_handshake in src/or/connection_or.c:

+      control_event_bootstrap_problem("foo", END_OR_CONN_REASON_OR_IDENTITY);

I think it's time to change "foo" here.

comment:9 Changed 8 years ago by nickm

Replying to asn:

  • If command_process_cert_cell receives a CERT cell, with two OR_CERT_TYPE_TLS_LINK certificates it will decode, ignore and *not* free the second one, because of:
            if (cert_type == OR_CERT_TYPE_TLS_LINK && !link_cert)
              link_cert = cert;
            ...
            else
              tor_cert_free(cert);
    

The same goes for the other types of allowed certificates.

Are you sure there? If we get through the loop a second time, link_cert is already set, so the first branch isn't taken (because !link_cert is now false), so we should call tor_cert_free(cert);

  • In command_process_auth_challenge_cell() don't forget to use ERR() or return in:
        if (connection_or_send_authenticate_cell(conn, use_type) < 0) {
          /* XXX log */
          connection_mark_for_close(TO_CONN(conn));
        }
    

fixed

comment:10 in reply to:  8 Changed 8 years ago by nickm

Replying to rransom:

In connection_or_check_valid_tls_handshake in src/or/connection_or.c:

+      control_event_bootstrap_problem("foo", END_OR_CONN_REASON_OR_IDENTITY);

I think it's time to change "foo" here.

This wasn't added in prop176; I'm not going to fix it there. I'll add a ticket to do it once prop176 is merged. (See #4169 )

comment:11 Changed 8 years ago by asn

  • In tor_cert_decode():
      newcert = tor_cert_new(x509);
      if (!newcert) {
        X509_free(x509);
        return NULL;
      }
    

If tor_cert_new() fails, x509 gets freed twice (once inside tor_cert_new(), and once inside the if body). You probably want to kill the X509_free() inside the if, since tor_cert_new() steals the reference of x509_cert.

  • In tor_tls_context_new():
      result->my_link_cert = tor_cert_new(X509_dup(cert));
      result->my_id_cert = tor_cert_new(X509_dup(idcert));
      result->my_auth_cert = tor_cert_new(X509_dup(authcert));
    

We don't check the return values of tor_cert_new().
Probably because {,id,auth}cert are considered trusted data.
In any case maybe it's a good idea to check their return values [especially when the else body gets added to the if in tor_cert_new()]

comment:12 Changed 8 years ago by nickm

Fixed those behaviors, and improved the non-RSA case of setting pkey_digests. Thanks!

comment:13 Changed 8 years ago by asn

  • c026d58c3e8179f31acb0280d93b94b21ecdc1c9
           } else if (cert_type == OR_CERT_TYPE_AUTH_1024 && !auth_cert) {
              if (auth_cert) {
    

You forgot to remove the "&& !auth_cert" part.

comment:14 Changed 8 years ago by nickm

Fixed that, and added more documentation and log statements.

comment:15 Changed 8 years ago by nickm

Now with more fixes, a changes file, and a way to disable it if needed in prop176-v2. prop176-v2 as of now is exactly the current state of prop176, rebased and squashed.

comment:16 Changed 8 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged and closing. Please open new tickets assigned to me as needed for any bugs you find in the merged code.

comment:17 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:18 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.