Re: [tor-bugs] #18620 [Core Tor/Tor]: HSFORGET command to clear cached client state for a HS

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#18620: HSFORGET command to clear cached client state for a HS
-------------------------------------------------+-------------------------
 Reporter:  str4d                                |          Owner:  str4d
     Type:  enhancement                          |         Status:
 Priority:  Medium                               |  needs_revision
Component:  Core Tor/Tor                         |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.9.x-final
 Keywords:  tor-hs, 029-accepted, review-        |        Version:  Tor:
  group-1                                        |  0.2.7.6
Parent ID:                                       |     Resolution:
 Reviewer:  asn, special                         |  Actual Points:
                                                 |         Points:  small
                                                 |        Sponsor:
                                                 |  SponsorR-can
-------------------------------------------------+-------------------------
Changes (by special):

 * status:  needs_review => needs_revision


Comment:

 I agree with arma. Any client application that needs this function is
 actually working around a bug in tor. We should make tor smarter to solve
 whatever issues this avoids.

 That said, I'm not opposed to having this function if people have other
 uses for it. I could imagine this being useful for testing and scripts.

 I have some control-spec comments:

 We encourage applications to use this by mentioning unstable internet
 connections as a use case. I would rather not.

 I don't think the `DescCookie` parameter is useful. There is no case as a
 client where we can use a descriptor that requires authorization without
 the cookie being configured (such as via `HidServAuth`). Instead of having
 a cookie parameter, you could look up the cookie using
 `rend_client_lookup_service_authorization`.

 There are also a few code issues, some of which are obsolete after my
 suggestion above, but I'll mention them anyway:

 {{{
  +  char descriptor_cookie_base64ext[REND_DESC_COOKIE_LEN_BASE64 + 2 + 1];
 ...
  +    descriptor_cookie_decoded = tor_malloc(REND_DESC_COOKIE_LEN + 2);
 ...
  +    /* Add trailing zero bytes (AA) to make base64-decoding happy. */
 }}}

 You could replace all of this logic with the recently-created
 `rend_auth_decode_cookie` function.

 {{{
  +    if (base64_decode(descriptor_cookie_decoded,
  +                      sizeof(descriptor_cookie_decoded),
 }}}

 `descriptor_cookie_decoded` is `char *`, not a char array, so sizeof is
 incorrect. This can never succeed.

 Also, you don't check the decoded length returned by `base64_decode` and
 `descriptor_cookie_decoded` was allocated with `tor_malloc` instead of
 `tor_malloc_zero`, so you could have ended up reading uninitialized memory
 later.

 {{{
  +  tor_free(onion_address);
  +  if (descriptor_cookie_base64) {
  +    tor_free(descriptor_cookie_base64);
  +    tor_free(descriptor_cookie_decoded);
  +  }
 }}}

 This block of code is repeated for the success and err cases. You could
 avoid that by having an `int result` to return and falling through the
 `err:` case after calling `send_control_done`.

 {{{
  +/** Remove any cached descriptors for <b>service_id</b>. */
 +void
 +rend_cache_remove_entry(const char *service_id)
 }}}

 This name (and documentation) is misleading. This function only removes
 from the client cache, not the service or directory caches. I would rename
 it to mention clients.

 `make check-spaces` has a few other comments for you.

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