[tor-bugs] #6411 [Tor]: Adding hidden services through control socket
Tor Bug Tracker & Wiki
blackhole at torproject.org
Fri Mar 20 02:27:13 UTC 2015
#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: |
-------------------------+-------------------------------------------------
Comment (by yawning):
Replying to [comment:45 dgoulet]:
> Here is what I have after a first review. Very awesome feature I might
add :).
Wow, that was fast thanks.
> 1) `crypto_pk_base64_decode()`
>
> * `size_t der_len` should be checked against <= 0 because
base64_decode()
> can return negative value.
Fixed.
> 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.
I'll think of a nice way to fix this. Probably involving exposing
`parse_port_config()`.
> * `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`.
Fixed.
> 3) `handle_control_del_onion()`
>
> * `if (onion_services == NULL || idx == -1) {`, the "idx == -1"
doesn't seem useful.
Fixed.
> * 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?
Kind of, did you read the rest of the comment? There's no way that a
malformed service ID will be on either list, so a "552 Unknown Onion
Service id" error is going to be returned (`onion_services` will be NULL,
`idx` will be -1, so `rend_service_del_ephemeral()` will never get
called).
`rend_service_del_ephemeral(service_id)` failing here is a bug, because it
indicates that there's an inconsistency between the per-control-
connection/global detached HS lists, and the HSes that are actually
running.
I will add a call to `rend_valid_service_id()` before going through the
lists for clarity and the sake of a more descriptive error, but I believe
the current code is correct.
> 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.
> */
> }}}
That comment is utter failboat. Replaced with:
{{{
/* Preserve the existing ephemeral services.
*
* This is the ephemeral service equivalent of the "Copy introduction
* points to new services" block, except there's no copy required
since
* the service structure isn't regenerated.
*
* After this is done, all ephemeral services will be:
* * Removed from old_service_list, so the equivalent non-ephemeral
code
* will not attempt to preserve them.
* * Added to the new rend_service_list (that previously only had the
* services listed in the configuration).
* * Added to surviving_services, which is the list of services that
* will NOT have their intro point closed.
*/
}}}
Fixed.
> 5) `rend_service_add_ephemeral`
>
> * Documenting the returned value would be desirable. (same for
del_ephemeral)
Ok, will do.
> * `s->directory = NULL; /* This indicates the service is ephemeral.
*` I
> would also document that in the structure definition in
rendservice.c at
> the top.
Ok, will do.
> * 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.
> }}}
Hmm. I could add rend_service_new() or something, but to properly
minimize duplication (to the point where it makes sense to me),
`rend_config_services()` would need a lot of changes. I'd like to avoid
doing that in this patch if possible to minimize the impact on the rest of
the HS code.
The common parts that are immediately obviously extracted are:
{{{
service = tor_malloc_zero(sizeof(rend_service_t));
service->ports = smartlist_new();
service->intro_period_started = time(NULL);
service->n_intro_points_wanted = NUM_INTRO_POINTS_DEFAULT;
}}}
Is that worth it?
TODO:
* Add validation to `handle_del_onion()` for user friendliness.
* Figure out what to do with `Port`.
* Add documentation comments for the rendservice.c stuff (return values,
directory == NULL).
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/6411#comment:46>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list