[tor-bugs] #33545 [Core Tor/Tor]: assertion failure when "all zero" client auth key provided

[Tor Bug Tracker & Wiki]

#33545: assertion failure when "all zero" client auth key provided
-------------------------------------+-------------------------------------
 Reporter:  mcs                      |          Owner:  (none)
     Type:  defect                   |         Status:  reopened
 Priority:  High                     |      Milestone:  Tor: 0.4.3.x-final
Component:  Core Tor/Tor             |        Version:  Tor: 0.4.4.0-alpha-
                                     |  dev
 Severity:  Normal                   |     Resolution:
 Keywords:  043-must security        |  Actual Points:  0.2
  extra-review                       |
Parent ID:                           |         Points:
 Reviewer:  dgoulet, nickm           |        Sponsor:
-------------------------------------+-------------------------------------
Changes (by asn):

 * status:  closed => reopened
 * keywords:  043-should => 043-must security extra-review
 * reviewer:  asn => dgoulet, nickm
 * resolution:  duplicate =>
 * actualpoints:   => 0.2


Comment:

 Replying to [comment:10 cypherpunks]:
 > Replying to [comment:6 asn]:
 > > Many thanks for the fix branch. The branch we merged as part of #33137
 is equivalent.
 >
 > What part of the commits to fix #33137 is equivalent to the branch in
 this ticket, exactly? They cover entirely different codepaths.
 >
 > The #33137 investigation into fixing faulty keys passed with `ADD_ONION`
 explicitly reached the conclusion that faulty keys passed to
 `ONION_CLIENT_AUTH_ADD` aren't even a problem that needs to be fixed. This
 ticket was filed to say those actually are an issue that needs to be
 fixed.
 >
 > {{{
 >   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).
 > }}}

 You are absolutely right. It was my mistake to brush this ticket off as
 identical to #33137. While #33137 was regarding the public keys in
 `ADD_ONION`, this ticket was about the private keys in
 `ONION_CLIENT_AUTH_ADD`. During my investigation of #33137 I mushed it all
 together and got confused here. Thanks for your dilligence here to make
 sure that this bug gets fixed.

 With regards to the fix, I decided to take a similar approach to you but
 do the all-zeroes check deeper into the
 `parse_private_key_from_control_port()` which seemed like the right place
 to do it. I also loosed the assert, and made the same check for the
 filesystem client auth approach. It would be great if you could check out
 my patch and please comment if you find any issues.

 Branch for 043: https://github.com/torproject/tor/pull/1842
 Branch for 044: https://github.com/torproject/tor/pull/1843

 I'm reopening the ticket and assigning two reviewers here.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/33545#comment:11>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online