Re: [tor-bugs] #15588 [Tor]: Allow client authorization on control port ADD_ONION services

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

#15588: Allow client authorization on control port ADD_ONION services
-------------------------------------------------+-------------------------
 Reporter:  special                              |          Owner:  special
     Type:  enhancement                          |         Status:
 Priority:  High                                 |  needs_revision
Component:  Tor                                  |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.8.x-final
 Keywords:  hidden-service, control, tor-hs,     |        Version:
  028-triaged, pre028-patch                      |     Resolution:
Parent ID:  #8993                                |  Actual Points:
  Sponsor:                                       |         Points:  small
-------------------------------------------------+-------------------------
Changes (by dgoulet):

 * status:  needs_review => needs_revision


Comment:

 First, this branch has warnings (--enable-gcc-warnings SO useful :):

 {{{
 src/or/control.c: In function âadd_onion_helper_clientauthâ:
 src/or/control.c:4147:69: error: comparison between signed and unsigned
 integer expressions [-Werror=sign-compare]
        strspn(client->client_name, REND_LEGAL_CLIENTNAME_CHARACTERS) !=
 len) {
                                                                      ^
 src/or/control.c:4110:63: error: unused parameter âauth_typeâ [-Werror
 =unused-parameter]
  add_onion_helper_clientauth(const char *arg, rend_auth_type_t auth_type,

 }}}

 * 9e44364643eba61567249705374579d58836f832
 * 2ac5c5c59cee3ad471eee14995d316988cc909c8

  In rend_auth_encode_cookie: `cookie_out = NULL;`, I don't think that's
 needed.

  In rend_auth_decode_cookie: `int len = strlen(descriptor_cookie);`, it
 should be `size_t`.

  This `memset(descriptor_cookie_tmp, 0, sizeof(descriptor_cookie_tmp));`
 can be replaced by `char descriptor_cookie_tmp[REND_DESC_COOKIE_LEN+2] =
 {0};`

  Does that work if `descriptor_cookie` points to
 `descriptor_cookie_base64ext` which has +1 length? If yes, I would comment
 on why we don't use the right srclen.
 {{{
 +  if (base64_decode(descriptor_cookie_tmp, sizeof(descriptor_cookie_tmp),
 +                    descriptor_cookie, REND_DESC_COOKIE_LEN_BASE64+2) <
 0) {
 }}}

  While we are at it, I would rename `descriptor_cookie_tmp` to something
 more meaningful like `descriptor_cookie_decoded`.

  I know this is a "move to function" thing so let's not change too much
 but for the record, this is not pretty: `if (auth_type_val < 1 ||
 auth_type_val > 2)`. There, it is on the record :).

 * dce6310a49fb6c0b08a0d5c3220d46834df24d61
 * 11575f3be9705ff571eb24c2506f6e83ae284aa9

  Should be `size_t` here: `int len = strlen(client->client_name);`

  This `if (len < 1 || len > 19` should use the define
 `REND_CLIENTNAME_MAX_LEN`. I'm not sure where `19` comes from here...

  At some point, we should have functions that validate input for auth.
 because I see some code duplication with rendservice.c ... :(

  `crypto_rand()` can fail. Better check that so we avoid having empty
 cookies.

 ---

 Done for now.

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