[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #15588 [Tor]: Allow client authorization on control port ADD_ONION services
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs