Re: Fourth patch for proposal 121

[Nick Mathewson <nickm@xxxxxxxxxxxxx>]

Thanks for all the revisions!  I'm checking this in , with some
changes noted below.

On Tue, Sep 23, 2008 at 10:15:29PM +0200, Karsten Loesing wrote:
 [...]
>    } else { /* not general */
> -    if (rend_cmp_service_ids(conn->rend_query,
> -                             TO_ORIGIN_CIRCUIT(circ)->rend_query)) {
> +    if ((conn->rend_data && !TO_ORIGIN_CIRCUIT(circ)->rend_data) ||
> +        (!conn->rend_data && TO_ORIGIN_CIRCUIT(circ)->rend_data) ||
> +        (conn->rend_data && TO_ORIGIN_CIRCUIT(circ)->rend_data &&
> +         rend_cmp_service_ids(conn->rend_data->onion_address,
> +             TO_ORIGIN_CIRCUIT(circ)->rend_data->onion_address))) {
>        /* this circ is not for this conn */

It's totally okay to introduce local variables to hold frequently
calculated things like TO_ORIGIN_CIRCUIT() here; Sometimes it makes
stuff easier to read.

  [...]
> Index: /home/karsten/tor/tor-trunk-121-patches/src/or/connection.c
> ===================================================================
> --- /home/karsten/tor/tor-trunk-121-patches/src/or/connection.c	(revision 16941)
> +++ /home/karsten/tor/tor-trunk-121-patches/src/or/connection.c	(working copy)
> @@ -385,6 +385,7 @@
>        memset(edge_conn->socks_request, 0xcc, sizeof(socks_request_t));
>        tor_free(edge_conn->socks_request);
>      }
> +    tor_free(edge_conn->rend_data);


I've introduced a rend_data_free() to call on rend_data_t instead of
tor_free().  Generally, it's better to have a type-specific free
function for each type---even types that don't have any subfields that
need to be freed---so that you can more easily add such subfields
later.

 [...]
> @@ -535,9 +537,10 @@
>         * is changed to DIR_PURPOSE_HAS_FETCHED_RENDDESC to mark that
>         * refetching is unnecessary.) */
>        if (conn->purpose == DIR_PURPOSE_FETCH_RENDDESC_V2 &&
> -          dir_conn->rend_query &&
> -          strlen(dir_conn->rend_query) == REND_SERVICE_ID_LEN_BASE32)
> -        rend_client_refetch_v2_renddesc(dir_conn->rend_query);
> +          dir_conn->rend_data && dir_conn->rend_data->onion_address &&
> +          strlen(dir_conn->rend_data->onion_address) ==
> +              REND_SERVICE_ID_LEN_BASE32)
> +        rend_client_refetch_v2_renddesc(dir_conn->rend_data);

You don't need to test dir_conn->rend_data->onion_address; it is an
array member of dir_conn->rend_data, and is always present when
rend_data is present.

  [...]
>        else if (CONN_IS_EDGE(conn) &&
> -              !rend_cmp_service_ids(rendquery, TO_EDGE_CONN(conn)->rend_query))
> +               TO_DIR_CONN(conn)->rend_data &&
> +              !rend_cmp_service_ids(rendquery,
> +                        TO_EDGE_CONN(conn)->rend_data->onion_address))

I think you mean this TO_DIR_CONN to be a TO_EDGE_CONN.

 [...]
> Index: /home/karsten/tor/tor-trunk-121-patches/src/or/rendservice.c
> ===================================================================
> --- /home/karsten/tor/tor-trunk-121-patches/src/or/rendservice.c	(revision 16941)
> +++ /home/karsten/tor/tor-trunk-121-patches/src/or/rendservice.c	(working copy)
> @@ -69,8 +69,16 @@
>                           * up-to-date. */
>    time_t next_upload_time; /**< Scheduled next hidden service descriptor
>                              * upload time. */
> +  smartlist_t *accepted_intros; /**< List of client_access_event_t's for
> +                                 * accepted and answered INTRODUCE2 cells. */

This isn't freed in rend_service_free().

>  } rend_service_t;
>  

 [...]
> +
> +  /* Allow the request. */
> +  log_info(LD_REND, "Client %s could be identified for service %s.",
> +           auth_client->client_name, service->service_id);

I'm going to change this log message.  "Client could be identified"
sounds like we broke somebody's anonymity.


 [...]
> +  /* Iterate over past requests, remove those which are older than one hour,
> +   * and check whether there is one with same Diffie-Hellman, part 1. */
> +  if (!service->accepted_intros)
> +    service->accepted_intros = smartlist_create();
> +  SMARTLIST_FOREACH(service->accepted_intros, client_access_event_t *,
> +                    access, {
> +    if (access->access_time + REND_REPLAY_TIME_INTERVAL < now) {
> +      tor_free(access);
> +      SMARTLIST_DEL_CURRENT(service->accepted_intros, access);
> +    } else if (!memcmp(access->diffie_hellman_hash, diffie_hellman_hash,
> +                       DIGEST_LEN)) {
> +      log_warn(LD_REND, "Possible replay detected! We received an "
> +                        "INTRODUCE2 cell with same first part of "
> +                        "Diffie-Hellman handshake %d seconds ago. Dropping "
> +                        "cell.",
> +               (uint32_t) (now - access->access_time));

The format here is wrong.  %d takes an int; uint32_t is not an int.

> +      return 0;

Doesn't this leak?  Shouldn't it "goto err"?

> +    }
> +  });

This is O(N) in the number of requests we've seen; that will really
suck in terms of performance.  I'm changing it to use a digestmap_t.

-- 
Nick