[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: Second patch for proposal 121
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;
> +}
> +
>