Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#8121 closed defect (fixed)

IA-32 Tor users with NaCl may be distinguishable from others

Reported by: rransom Owned by:
Priority: Very High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: isis Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

curve25519-donna and curve25519-donna-c64 make no special effort to retain the high bit of a public-key coordinate-field element.

The ref implementation in NaCl makes no special effort to clear it. (Fortunately, Tor refuses to use this one.)

The non-free athlon implementation in NaCl is an unreadable blob with no source code in sight, and I don't have a 32-bit environment to test it in handy, but a web page documenting an earlier version of that implementation (http://cr.yp.to/ecdh.html#validate) seems to imply that the high bit is considered part of the coordinate-field element. If this is true, it's an anonymity issue for Tor users who use the ntor handshake.

The donna_c64 implementation in NaCl has the same behaviour as the curve25519-donna-c64 implementation shipped with Tor.

Tor must either clear the high bit of every Curve25519 public key it uses, or reduce every Curve25519 public key modulo the field order (the former is easier and consistent with the behaviour of the free Curve25519 implementations shipped in the Tor source package).

(It appears that a relay can only exploit this by causing a user's handshake to fail, but it's still an anonymity bug.)

Child Tickets

Change History (7)

comment:1 Changed 7 years ago by nickm

Keywords: tor-client added
Status: newneeds_review

I've just run a test on the athlon implementation in nacl, building it with -m32. Like the donna implementations, it ignores the high bit of the input public key.

Nonetheless, clearing the high bit of the public key is a fine idea. Have a look at branch "bug8121".

comment:2 Changed 7 years ago by rransom

Status: needs_reviewneeds_revision

In src/common/crypto_curve25519.c, you forgot to use bp as the Curve25519 implementation's input instead of basepoint.

comment:3 Changed 7 years ago by nickm

Status: needs_revisionneeds_review

So I did. Added a fixup commit for that.

comment:4 Changed 7 years ago by rransom

Looks good now. (The test change appears to be correct, too.)

comment:5 Changed 7 years ago by nickm

Great; merged into master.

comment:6 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

comment:7 Changed 5 years ago by isis

Cc: isis added
Note: See TracTickets for help on using tickets.