[tor-bugs] #6411 [Tor]: Adding hidden services through control socket

[Tor Bug Tracker & Wiki]

#6411: Adding hidden services through control socket
-------------------------+-------------------------------------------------
     Reporter:           |      Owner:  yawning
  kevinevans             |     Status:  needs_revision
         Type:           |  Milestone:  Tor: 0.2.7.x-final
  enhancement            |    Version:  Tor: 0.2.3.19-rc
     Priority:  normal   |   Keywords:  hidden-service control maybe-
    Component:  Tor      |  proposal tor-hs globalleaks-wants
   Resolution:           |  Parent ID:  #8993
Actual Points:           |
       Points:           |
-------------------------+-------------------------------------------------
Changes (by dgoulet):

 * status:  needs_review => needs_revision


Comment:

 Here is what I have after a first review. Very awesome feature I might add
 :).

 1) `crypto_pk_base64_decode()`

     * `size_t der_len` should be checked against <= 0 because
 base64_decode()
     can return negative value.

 2) `handle_control_add_onion()` --> HUGE :)

     * `tor_parse_long(port_str, 10, 1, 65536, NULL, ...`, shouldn be 65535
 here
     else 65536 is accepted and currently abort if used.

     * I think TARGET needs to be validated because this makes tor die
 (crazy
     IP):

     {{{ADD_ONION NEW:BEST Port=65535,181.121.123.1116:655}}}

     Quite related to previous port bug. Didn't investiguate more but seems
 `service->ports` is invalid in some way.

     * `if (!strcasecmp("RSA1024", key_type)) {`, I would continue the good
     practice of using "static const char *" like you did with `Flags=` and
     `Port=`. Same for `BEST` and `NEW`.

 3) `handle_control_del_onion()`

     * `if (onion_services == NULL || idx == -1) {`, the "idx == -1"
 doesn't seem useful.

     * In this function, the given service ID is not validated so it's
 blindly
       passed to `rend_service_del_ephemeral` which validates it and can
 return
       an error. The comment `This should *NEVER* fail,` is wrong in that
 sense.
       I think would be good if you could inform the user that the service
 ID is
       malformed in the first place instead of a generic "Failed to remove
 Onion
       Service" error?

 4) `rend_config_services`

     * This comment is a bit confusing. Can you clarify why you are doing
 that?
     {{{
     /* Pull all the ephemeral services out of old_service_list and add
 them
      * into rend_service_list so they remain active and
 surviving_service_list
      * so the intro points don't get closed.
      */
     }}}

 5) `rend_service_add_ephemeral`

     * Documenting the returned value would be desirable. (same for
 del_ephemeral)

     * `s->directory = NULL; /* This indicates the service is ephemeral. *`
 I
       would also document that in the structure definition in
 rendservice.c at
       the top.

     * This function has some code duplication from
 `rend_config_services()`. I
       would advocate for a kind of "rend_service_create" function that
 would do
       the allocation and initialization. We can even split that into
 "create()"
       and "init()" if you prefer. You even add this comment in
 del_ephemeral() ;)
       for which I agree and should be done.
       {{{
        * XXX: As with the comment in rend_config_services(), a nice
 abstraction
        * would be ideal here, but for now just duplicate the code.
       }}}

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