Opened 7 years ago

Closed 7 years ago

Last modified 6 months ago

#5185 closed enhancement (implemented)

Implement ‘safe cookie authentication’ in Tor

Reported by: rransom Owned by:
Priority: Medium Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: security-fix tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

See my safecookie-v2 torspec branch ( https://git.torproject.org/rransom/torspec.git safecookie-v2 ) for a spec-change branch specifying the new ‘safe cookie authentication’ protocol.

See my safecookie-022-v2 tor branch ( https://git.torproject.org/rransom/tor.git safecookie-022-v2 ) for a code-change branch implementing it on maint-0.2.2.

Child Tickets

Change History (13)

comment:1 Changed 7 years ago by rransom

Status: newneeds_review

comment:2 Changed 7 years ago by atagar

I would much rather if part of this patch included an addition to stem for handling this new safe cookie handshake. If you did that then I'd be happy to provide integ tests for your new feature. That said, I proposed this on your tor-talk@ thread and got stony silence. Pity - it's not good if we neglect maintainability...

comment:3 in reply to:  2 Changed 7 years ago by rransom

Replying to atagar:

I would much rather if part of this patch included an addition to stem for handling this new safe cookie handshake. If you did that then I'd be happy to provide integ tests for your new feature. That said, I proposed this on your tor-talk@ thread and got stony silence. Pity - it's not good if we neglect maintainability...

I was busy revising the protocol at the time, to remove some bugs from the protocol. I'll post Python 2 code to implement the controller side of the protocol soon.

comment:4 in reply to:  description Changed 7 years ago by rransom

Status: needs_reviewneeds_revision

Replying to rransom:

See my safecookie-022-v2 tor branch ( https://git.torproject.org/rransom/tor.git safecookie-022-v2 ) for a code-change branch implementing it on maint-0.2.2.

... which doesn't compile now that I've backported the patch, because crypto_hmac_sha256 isn't in 0.2.2.x.

comment:5 Changed 7 years ago by rransom

Status: needs_revisionneeds_review

See my safecookie-022-v3 tor branch ( https://git.torproject.org/rransom/tor.git safecookie-022-v3 ) for a code-change branch implementing ‘safe cookie authentication’ on maint-0.2.2. (I rebased to put the cherry-picked commit introducing crypto_hmac_sha256 in before the commit using it.)

This time I've tested it before posting the branch. See branch safecookie-python of my tor-utils repo ( https://git.torproject.org/rransom/tor-utils.git safecookie-python ) for the code I used to test the Tor changes (only tested with Python 2.7.2 as packaged in Debian Wheezy).

comment:6 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

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.

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.
  • 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().
  • 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.

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!

comment:7 in reply to:  6 Changed 7 years ago by rransom

Replying to 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.

comment:8 Changed 7 years ago by nickm

Status: needs_revisionneeds_review

Okay, looks fine. On the AUTHCHALLENGE quoting issue, I still disagree, but I'll solve my objections by adding a note to the spec that says that the challenges need to be generated with a secure PRNG. I also disagree about tor_assert() style, but not enough to change the code right now.

I think arma is doing a release RSN and would be sad if I merged another patch today. I'll merge this immediately after he does that release, or after he tells me that he is not in fact doing a release RSN.

comment:9 Changed 7 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Well, *that* took me too long. :(

Merged into maint-0.2.2 and master. Added a changes file. Merged tor-spec changes into into tor-spec with 0.2.4.1-alpha => "A future version of Tor" change.

comment:10 Changed 7 years ago by nickm

Keywords: tor-client added

comment:11 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:12 in reply to:  6 Changed 6 months ago by arma

Severity: Blocker

Replying to nickm:

13:27 < nickm> [...] Removing it before 0.2.4.x-rc, yes.

#28675 looks like a fine opportunity for removing it.

comment:13 Changed 6 months ago by arma

Priority: Very HighMedium
Severity: BlockerNormal
Note: See TracTickets for help on using tickets.