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

[Tor Bug Tracker & Wiki]

#15588: Allow client authorization on control port ADD_ONION services
-------------------------------------------------+-------------------------
 Reporter:  special                              |          Owner:  special
     Type:  enhancement                          |         Status:
 Priority:  High                                 |  needs_review
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 special):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:19 dgoulet]:
 > First, this branch has warnings (--enable-gcc-warnings SO useful :):

 Oof. That must've gotten lost from this build. Sorry about the sloppiness.

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

 It is; cookie_out is the return value after the jump.

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

 Done.

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

 Done.

 >
 >  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) {
 > }}}

 `descriptor_cookie_base64ext` has +1 for a null terminator. The source
 data is always exactly `REND_DESC_COOKIE_LEN_BASE64+2` characters here, so
 we are using the right srclen.

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

 Done.

 >
 >  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 :).

 Yeah :/

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

 I was using the same logic as rend_parse_client_keys, but
 rend_config_services did it differently. I've combined all of these into a
 rend_valid_client_name function that uses REND_CLIENTNAME_MAX_LEN.

 New commits are 567f76f31c31690f28739fde9cf00da3890da053 and
 855e5b72777a165bde86a14422b16adb10815dd0. I'll squash these once we're
 done with review. Thanks!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/15588#comment:21>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online