[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #33137 [Core Tor/Tor]: Resolve TROVE-2020-003: crash adding bad ed25519 HSv3 private key from controller (was: Resolve TROVE-2020-003)



#33137: Resolve TROVE-2020-003: crash adding bad ed25519 HSv3 private key from
controller
-------------------------------------------------+-------------------------
 Reporter:  nickm                                |          Owner:  asn
     Type:  defect                               |         Status:  closed
 Priority:  High                                 |      Milestone:  Tor:
                                                 |  0.4.3.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:  fixed
 Keywords:  035-backport 041-backport            |  Actual Points:  2
  042-backport 043-backport. 043-must security   |
Parent ID:                                       |         Points:  1-5?
 Reviewer:  ahf, catalyst                        |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by nickm):

 * status:  needs_review => closed
 * actualpoints:   => 2
 * keywords:  043-must security => 035-backport 041-backport 042-backport
     043-backport. 043-must security
 * resolution:   => fixed


Old description:



New description:

 This bug is an assertion failure that can only be triggered by an attacker
 with access to the user's controlport: if they use ADD_ONION to pass in an
 invalid ed25519 key, then Tor will exit.

 Here is asn's analysis of the issue:
 {{{

 ================================================================================
 Analysis of TROVE-2020-003
 ================================================================================

 Summary
 ================================================================================

 The issue at hand is that hs_build_address() can crash with an assert
 failure
 if called with an "invalid" ed25519 public key as its 'key' argument.
 Usually
 that function is only called with valid public keys, but after the
 introduction
 of the ADD_ONION control port feature and the hs_service_add_ephemeral()
 function, it can now be called with an invalid public key and cause an
 assert
 crash.

 Tor considers an ed25519 public key to be "invalid" when it has a torsion
 component (see [TORSION-REFS] in rend-spec-v3.txt) so that phishing
 attackers
 cannot generate equivalent onion addresses for a normal onion address.
 This is
 a validation step that is usually not required for normal ed25519-based
 protocols, but it's actually necessary for the security of onion addresses
 or
 in any other place where keys or signatures are used as identifiers and
 security relies on their uniqueness.

 The validating function is ed25519_validate_pubkey() and it's currently
 used in
 two cases:
 1) for onion address validation, so that attackers cannot create
 equivalent
    sets of onion addresses
 2) when dirauths validate relay ed25519 keys, for reasons unclear to me
    (perhaps this check is not needed)

 Impact
 ================================================================================

 The impact of this bug is a local denial-of-service attack to Tor through
 an
 assert-failure.

 The particular ADD_ONION attack vector can only be triggered by an
 attacker who
 has access to the control port which assumes a local attacker. Also an
 attacker
 who has access to the control port can do various other modifications to
 Tor
 that will result in loss of security. This is the reason this bug is
 marked as
 'low' severity.

 Fix
 ================================================================================

 Given that ed25519 public key validity checks are usually not needed and
 (so
 far) they are only necessary for onion addesses in the Tor protocol, we
 decided
 to fix this specific bug instance without modifying the rest of the
 codebase
 (see below for other fix approaches).

 In our minimal fix we check that the pubkey in hs_service_add_ephemeral()
 is
 valid and error out otherwise.

 This will fix the issue in the current codebase but it doesn't solve it in
 the
 future if a new feature comes in which tried to do something like
 ADD_ONION, or
 if a new feature comes out which tries to use ed25519 in a non-standard
 and
 dangerous way.

 Considerations for the future
 ================================================================================

 ed25519 signature and public key malleability is a complex topic that
 protocol
 designers must be aware of when using ed25519 in non-standard ways in the
 protocol. In our case, we got bitten by passing ed25519 *private* keys
 around,
 but there are other theoretical cases where this can bite us. Hence,
 protocol
 designers and reviewers who work with ed25519 should be aware of such
 threats
 when creating new protocols.

 In the future, we should consider moving to signature schemes based on
 Ristretto (or others) which do not need additional optional key
 validation.

 Other fix approaches
 ================================================================================

 Here are some other fix approaches I ended up not taking:

 1) Check validity in ed25519_public_key_generate()

   One possible fix would be to patch ed25519_public_key_generate() so that
 it
   doesn't return "invalid" public keys. However, ed25519_validate_pubkey()
 is
   not cheap (it does a scalar mult and a point compression) and it's
 almost
   never needed, so we would make this validation optional and only enable
 it
   when it's needed.

   The problem here is that this is gonna complicate the interface of
   ed25519_public_key_generate() for everyone, and also it almost never
 makes
   sense to use the validation feature so developers will always be
 confused on
   whether they should use it or not.

   For reference, see how ed25519-dalek has the optional verify_strict()
   function which is not clear when should be used by developers [0].

 2) Enforce validity in ed25519_public_key_generate()

   Another approach would be to enforce that public keys created with
   ed25519_public_key_generate() are valid by clamping the secret key
 before
   producing the public key. Clamping is not expensive so this would not be
 too
   bad for performance.

   I actually tried to implement this but the blinded public key tests
   immediately broke: it seems like there are issues with clamping and
   hierarchical key derivation that would not allow us to do this [1].
   Furthermore, doing this would change our ed25519 behavior in a
   non-standard way.

 3) Fix this particular HSv3 bug (in hs_build_address())

   Another fix would be to patch just hs_build_address() and make sure that
 it
   fails if the resulting address is not valid.

   The problem here is that this will complicate the interface of
   hs_build_address() which is normally called with well behaving keys and
 is
   only called with a bad key in the case of ADD_ONION. The callers would
 have
   to check the retval for no gain, and the end result would be equal to
 the fix
   approach we took.

 Further investigation
 ================================================================================

 As part of our investigation for this bug, we did the following additional
 digging:

 - Audited all the places where hs_build_address() is called

   We audited all the places where hs_build_address() is called to see if
 any of
   them might be called with invalid pubkeys but it seems like the only
   vulnerable place is the bug above related to ADD_ONION.

   All other calls of hs_build_address() are either using the self-
 generated
   service public keys, or in the client case getting them directly from
 the HS
   identifier where the public key has been explicitly checked for validity
   during the SOCKS phase in connection_ap_handshake_rewrite_and_attach()
 with
   parse_extended_hostname().

   XXX Do you want me to make a branch with the various call sites
 annotated
       with why I think they are innocent?

 - Audited all the places where we get ed25519 private keys from foreign
 sources

   Tor mostly loads self-generated ed25519 private keys from files, or
 generates
   them itself (e.g. in the case of blinded keys). The only place where an
   ed25519 private key is loaded from a non-local source is the bug in
 question
   above using ADD_ONION.

 - Audited all the places where we get x25519 private keys from foreign
 sources

   In general, x25519 keys should not need validation like we do for
 ed25519
   keys [2]. Still, we decided to take a look at whether we load any x25519
 private
   keys from non-local sources as part of our investigation.

   Tor usually loads pre-generated x25519 keys from file, or generates them
   itself.

   However, in the HSv3 client authorization feature we can get an x25519
   privkey from the control port through the ONION_CLIENT_AUTH_ADD command
 (in
   handle_control_onion_client_auth_add()).  However, we never convert that
 key
   to a pubkey, as it always lives in hs_client_service_authorization_t as
 a
   secret key. Also, when we actually do use that secret key in
   build_descriptor_cookie_keys() the x25519 module is responsible for
 doing the
   necessary tweaks to make it well formed (see how curve25519_donna() does
 the
   necessary bit transformations on the 'secret' key).

 [0]: https://github.com/dalek-cryptography/ed25519-dalek#the-
 verify_strict-function
 [1]: https://moderncrypto.org/mail-archive/curves/2017/000858.html
 [2]: https://lists.torproject.org/pipermail/tor-dev/2017-April/012213.html

 }}}

--

Comment:

 Posted analysis from asn, along with brief description.  The fixes are now
 merged into all supported branches.  The unit test did not apply cleanly
 before 0.4.3, so I only applied that to 0.4.3 and forward.

 The patches are:
   * c940b7cf13d8ceb5705d52c215256e1de5a4e757
   * 089e57d22f7c5e755a2d88d0b102207f7207ee27

 The tests are:
   * 5ff8757aa89cd9caa17207beb080607941336a5e

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/33137#comment:6>
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