[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #5185 [Tor Client]: Implement âsafe cookie authenticationâ in Tor
#5185: Implement âsafe cookie authenticationâ in Tor
--------------------------+-------------------------------------------------
Reporter: rransom | Owner:
Type: enhancement | Status: needs_revision
Priority: critical | Milestone: Tor: 0.2.2.x-final
Component: Tor Client | Version:
Keywords: security-fix | Parent:
Points: | Actualpoints:
--------------------------+-------------------------------------------------
Comment(by rransom):
Replying to [comment:6 nickm]:
> Re the spec:
{{{
13:27 < nickm> rransom_: Okay, so I think I am okay with the change, with
the
proviso that I won't absolutely commit to removing COOKIE
in
0.2.4.1-alpha. Deprecating it in 0.2.4.1-alpha, yes.
Having a
loud annoying warning, yes. Maybe making it
disabled-by-default, yes. Removing it before 0.2.4.x-rc,
yes.
I hope we can kill it entirely, but I want to keep the
freedom
for 0.2.4.1-alpha to come out before every controller gets
its
ducks in a line.
}}}
>
> On reflection, I think the idiom might be about getting one's ducks in a
row, not a line.
>
> Also wrt the spec, I'd rather not have QuotedString as an option for the
client AUTHCHALLENGE: It's just begging for somebody to do something
stupid.
I'd rather not have QuotedString as an option for the client AUTHENTICATE:
It's just begging for somebody to do something stupid. See also #4600.
I'd rather not have AUTHCHALLENGE behave differently from AUTHENTICATE:
It's just begging for more confusion like #4600.
> WRT the patch itself, looking at the version in safecookie-022-v3:
>
> * I didn't review the hmac-sha256 implementation; I'm the cherry-pick
was correct.
Git is supposed to complain loudly and messily during the merge if a
cherry-picked commit's diff is not identical to the commit it was cherry-
picked from.
> * We don't support NDEBUG, but as a matter of style, I think we try to
avoid sticking expressions with side-effects into tor_assert().
{{{
src/common/compat.c:2646: tor_assert(WaitForSingleObject(event, 0) ==
WAIT_TIMEOUT);
src/or/dirserv.c:2347: tor_assert(tor_version_parse("0.2.1.31",
src/or/dirserv.c:2349: tor_assert(tor_version_parse("0.2.2.34",
src/or/dirserv.c:2351: tor_assert(tor_version_parse("0.2.3.6-alpha",
src/or/rendservice.c:2327:
tor_assert(!crypto_pk_generate_key(intro->intro_key));
}}}
Looks like you missed a few.
Which of those `tor_assert` calls would become easier to read if they were
split across three additional lines each (one to begin a block, one to
define and initialize a variable, and one to end the block)?
Which of their resulting assertion-failure log messages would become
easier to read if the expression whose result was false were omitted?
> * The buffers (on the stack and on the heap) seem like the kind of
thing people might feel more comfortable if we memset(0) after using.
This is pure cargo-cult though.
They would be wrong.
> As a side-note, is there any place in control.c where we document that
the 'body' values passed into handle_control_* are NUL-terminated? If
not, we probably should!
{{{
src/or/control.c:806: (void) body_len; /* body is NUL-terminated; so we
can ignore len. */
src/or/control.c:1284: (void) len; /* body is NUL-terminated, so it's
safe to ignore the length. */
src/or/control.c:2222: (void) len; /* body is NUL-terminated, so it's
safe to ignore the length. */
src/or/control.c:2487: (void) len; /* body is NUL-terminated, so it's
safe to ignore the length. */
src/or/control.c:2831: (void) len; /* body is nul-terminated; it's safe
to ignore the length */
src/or/control.c:2946: (void) len; /* body is nul-terminated; it's safe
to ignore the length */
}}}
There are a few places.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/5185#comment:7>
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