[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:
-------------------------------------+-------------------------------------

Comment (by cypherpunks):

 Replying to [comment:11 asn]:
 > 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.

 Thanks for finding the filesystem file-loading codepath. I'd naively
 concluded that `hs_client_register_auth_credentials()` was the only place
 that really called `digest256map_set(client_auths,` with new auths, so it
 was the only place that needed a change, and hadn't realized that
 `hs_config_client_authorization()` overwrites the entire map with a newly
 constructed map of x25519 keys loaded from the filesystem.

 I'd considered patching `parse_private_key_from_control_port()` at first,
 but went with patching `hs_client_register_auth_credentials()` because it
 seemed the lower level entrypoint, closer to where the invariant needed to
 be maintained, and it could theoretically in the future have a different
 caller, one that isn't the control port parsing code. If it isn't doing
 the error checking now, that function could use a BUG() just like the
 decryption code has, but then it still needs that new return value in the
 enum added anyway.

 (I'd put the unrelated
 [https://gitgud.io/onionk/tor/-/commit/0c1d5fcbb7f6d513e41241b075f49001161eef44
 comment documentation fix] in a separate commit instead of squashed with
 the rest, but that's just aesthetics.)

 gitweb.tpo and git.tpo seem to be down right now, incidentally. `fatal:
 unable to access 'https://git.torproject.org/tor.git/': Failed to connect
 to git.torproject.org port 443: No route to host`

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