Second patch for proposal 121

[Nick Mathewson]

On Sun, Aug 10, 2008 at 12:38:35AM +0200, Karsten Loesing wrote:
 [...]

Hi, Karsten.  This one needs revision before I can check it in.

The big problem here is that the parsing logic in this patch is purely
additive: whenever it sees a HidServAuth line, it adds or replaces an
entry in auth_hid_servs.

Why is this a problem?  Consider what happens when we start with a
torrc file of

   HidServAuth abc.onion cookie1
   HidServAuth xyz.onion cookie2

and then we remove the second line and send Tor a sighup.  The
authorization entry for xyz.onion will still be in auth_hid_servs,
which is not what the user intended.



 [...]
> +/** Client-side configuration of authorization for a hidden service. */
> +typedef struct rend_service_authorization_t {
> +  char descriptor_cookie[REND_DESC_COOKIE_LEN];
> +  char onion_address[REND_SERVICE_ID_LEN_BASE32+1+5+1];
> +  int auth_type;
> +} rend_service_authorization_t;

auth_type should still not be an int.


>  /** ASCII-encoded v2 hidden service descriptor. */
>  typedef struct rend_encoded_v2_service_descriptor_t {
>    char desc_id[DIGEST_LEN]; /**< Descriptor ID. */
> @@ -3890,6 +3900,7 @@
>  void rend_get_descriptor_id_bytes(char *descriptor_id_out,
>                                    const char *service_id,
>                                    const char *secret_id_part);
> +rend_service_authorization_t *lookup_client_auth(char *onion_address);

This should take a "const char *".  When a function takes a "char *",
that usually means that it is going to modify the pointed-to memory
that it gets.  C has had a const modifier since C90, I believe.  18
years in, we should darned well be using it.

The same goes for rend_parse_client_auth, I believe.

Also, the name of this function should to be prefixed to make its
content clear.  If I see it in code, I should know *what* it's looking
up the authorization for.  rend_client_lookup_auth() would be a better
name.



>  
>  /********************************* rendservice.c ***************************/
>  
> Index: /home/karsten/tor/tor-trunk-121-patches/src/or/rendclient.c
> ===================================================================
> --- /home/karsten/tor/tor-trunk-121-patches/src/or/rendclient.c	(revision 16480)
> +++ /home/karsten/tor/tor-trunk-121-patches/src/or/rendclient.c	(working copy)
> @@ -713,3 +713,104 @@
>    return extend_info_dup(intro->extend_info);
>  }
>  
> +/** Client-side authorizations for hidden services; map of onion address to
> + * rend_service_authorization_t*. */
> +static strmap_t *auth_hid_servs = NULL;

There needs to be an appropriate free_all() function to release the
storage held here, invoked from tor_free_all() in main.c, so that our
leak-checking methods won't freak out.


> +/** Parse <b>config_line</b> as a client-side authorization for a hidden
> + * service and add it to the local map of hidden service authorizations.
> + * Return 0 for success and -1 for failure. */
> +int
> +rend_parse_client_auth(char *config_line)
> +{
> +  char *onion_address, *descriptor_cookie;
> +  char descriptor_cookie_tmp[REND_DESC_COOKIE_LEN+2];
> +  char descriptor_cookie_base64ext[REND_DESC_COOKIE_LEN_BASE64+2+1];
> +  smartlist_t *sl = smartlist_create();
> +  rend_service_authorization_t *auth = NULL;
> +  int res = -1, auth_type = 0;
> +  tor_assert(config_line);
> +  smartlist_split_string(sl, config_line, " ",
> +                         SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 3);
> +  if (smartlist_len(sl) < 2) {
> +    log_warn(LD_CONFIG, "Configuration line does not consist of "
> +             "\"onion-address authorization-cookie [service-name]\": '%s'",
> +             config_line);
> +    goto free;
> +  }
> +  auth = tor_malloc_zero(sizeof(rend_service_authorization_t));
> +  /* Parse onion address. */
> +  onion_address = smartlist_get(sl, 0);
> +  if (strlen(onion_address) != 16+1+5 ||

16+1+5 is 3 magic numbers in a row.

> +      strstr(onion_address, ".onion") != onion_address + 16) {

It would be more idiomatic to use strcmpend() for this check.

> +    log_warn(LD_CONFIG, "Onion address has wrong format: '%s'",
> +             onion_address);
> +    goto free;
> +  }
> +  strlcpy(auth->onion_address, onion_address, 16+1);
> +  if (!rend_valid_service_id(auth->onion_address)) {
> +    log_warn(LD_CONFIG, "Onion address has wrong format: '%s'",
> +             onion_address);
> +    goto free;
> +  }
> +  /* Parse descriptor cookie. */
> +  descriptor_cookie = smartlist_get(sl, 1);
> +  if (strlen(descriptor_cookie) != 22) {
> +    log_warn(LD_CONFIG, "Authorization cookie has wrong length: '%s'",
> +             descriptor_cookie);
> +    goto free;
> +  }
> +  /* Add trailing zero bytes (AA) to make base64-decoding happy. */
> +  tor_snprintf(descriptor_cookie_base64ext,
> +               REND_DESC_COOKIE_LEN_BASE64+2+1,
> +               "%sAA", descriptor_cookie);
> +  if (base64_decode(descriptor_cookie_tmp, REND_DESC_COOKIE_LEN+2,
> +                    descriptor_cookie_base64ext,
> +                    strlen(descriptor_cookie_base64ext)) < 0) {
> +    log_warn(LD_CONFIG, "Decoding authorization cookie failed: '%s'",
> +             descriptor_cookie);
> +    goto free;
> +  }
> +  auth_type = (descriptor_cookie_tmp[16] >> 4) + 1;
> +  if (auth_type < 1 || auth_type > 2) {
> +    log_warn(LD_CONFIG, "Authorization cookie has unknown authorization type "
> +             "encoded.");
> +    goto free;


Didn't we have an enum for this?

> +  }
> +  auth->auth_type = auth_type;
> +  memcpy(auth->descriptor_cookie, descriptor_cookie_tmp,
> +         REND_DESC_COOKIE_LEN);
> +  /* Add parsed client authorization to local map. */
> +  if (!auth_hid_servs)
> +    auth_hid_servs = strmap_new();
> +  strmap_set(auth_hid_servs, auth->onion_address, auth);

If had an earlier setting for this service, we just leaked the memory
for it.

> +  auth = NULL;
> +  res = 0;
> + free:
> +  if (sl)
> +    SMARTLIST_FOREACH(sl, char *, c, tor_free(c););
> +  smartlist_free(sl);
> +  if (auth)
> +    rend_service_authorization_free(auth);
> +  return res;
> +}
> +
>