[tor-bugs] #5185 [Tor Client]: Implement ‘safe cookie authentication’ in Tor

[Tor Bug Tracker & Wiki]

#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