Opened 3 months ago

Closed 6 weeks ago

#33545 closed defect (implemented)

assertion failure when "all zero" client auth key provided

Reported by: mcs Owned by: asn
Priority: High Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version: Tor: 0.4.4.0-alpha-dev
Severity: Normal Keywords: 043-must security extra-review
Cc: dgoulet, asn Actual Points: 0.2
Parent ID: Points:
Reviewer: dgoulet, nickm Sponsor:

Description

While doing some Tor Browser testing for Sponsor 27, I experienced the following after I intentionally used an incorrect client auth key for a v3 onion service:

... [err] tor_assertion_failed_: Bug: src/feature/hs/hs_descriptor.c:1423: decrypt_descriptor_cookie: Assertion !fast_mem_is_zero((char *) client_auth_sk, sizeof(*client_auth_sk)) failed; aborting. (on Tor 0.4.4.0-alpha-dev 1da0b05a5cace6ed)

As it turns out, I happened to enter a key that is consists entirely of zero bits. This is an unusual thing to do, but I do not think tor should exit.

Steps to reproduce in Tor Browser:

  1. Try to load an http or https page for a v3 onion service that requires client authentication, e.g., dgoulet's test server.
  2. Enter 56 'A's when prompted for a client auth key.

Result: tor exits due to the assertion failure. Behind the scenes, the browser installs the key via a control port command like the following:

onion_client_auth_add <onion-addr> x25519:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=

and then tries to access the onion service again (page reload).

Child Tickets

Change History (21)

comment:1 Changed 3 months ago by nickm

Keywords: 043-should added
Milestone: Tor: 0.4.3.x-final
Priority: MediumHigh

comment:2 Changed 3 months ago by nickm

Cc: dgoulet asn added

comment:4 Changed 2 months ago by nickm

Status: newneeds_review

comment:5 Changed 2 months ago by asn

Reviewer: asn

comment:6 Changed 2 months ago by asn

Resolution: duplicate
Status: needs_reviewclosed

This was fixed as part of #33137.

Many thanks for the fix branch. The branch we merged as part of #33137 is equivalent. Feel free to review it if you'd like.

Thanks! :)

comment:7 in reply to:  6 ; Changed 2 months ago by cypherpunks

Replying to asn:

This was fixed as part of #33137.

This duplicate ticket was filed by mcs on March 6th, at which point the bug labeled as TROVE-2020-003 isn't exactly a secret anymore. So why was it and its in-the-works branch fix still kept as a secret until March 17th?

comment:8 in reply to:  7 Changed 2 months ago by asn

Replying to cypherpunks:

Replying to asn:

This was fixed as part of #33137.

This duplicate ticket was filed by mcs on March 6th, at which point the bug labeled as TROVE-2020-003 isn't exactly a secret anymore. So why was it and its in-the-works branch fix still kept as a secret until March 17th?

Hello,

TROVE-2020-003 was being investigated when mcs opened this ticket independently. We decided to leave this ticket be while we keep on investigating. Given that TROVE-2020-003 was decided to be of 'low' severity we decided to not take any extreme measures on either hiding this independent ticket, or release TROVE-2020-003 faster.

comment:9 Changed 2 months ago by nickm

Right. There's another issue too: a big part of our investigation was to find out whether any other parts of our code might have another instance of this bug that could have been triggered _without_ authenticated control port access. We looked hard and didn't find any such instances, but if we had, then TROVE-2020-003 would have been a higher severity, and would not have been a duplicate of this issue.

comment:10 in reply to:  6 ; Changed 2 months ago by cypherpunks

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

comment:11 in reply to:  10 ; Changed 8 weeks ago by asn

Actual Points: 0.2
Keywords: 043-must security extra-review added; 043-should removed
Resolution: duplicate
Reviewer: asndgoulet, nickm
Status: closedreopened

Replying to cypherpunks:

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

comment:12 in reply to:  11 Changed 8 weeks ago by cypherpunks

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

comment:13 Changed 8 weeks ago by dgoulet

Status: reopenedneeds_review

comment:14 Changed 8 weeks ago by dgoulet

Owner: set to asn
Status: needs_reviewassigned

comment:15 Changed 8 weeks ago by dgoulet

Status: assignedneeds_review

comment:16 Changed 7 weeks ago by nickm

This lgtm; I'll wait for dgoulet to review as well.

comment:17 Changed 7 weeks ago by dgoulet

Status: needs_reviewneeds_revision

See PR for a possible issue. If it is good and I'm confused about that bit, lgtm.

comment:18 Changed 6 weeks ago by asn

Status: needs_revisionneeds_review

Great find!

Pushed fixup in both branches. Also opened #33889 to track the general class of issues here.

comment:19 Changed 6 weeks ago by nickm

I don't think there is an actual difference before and after the fixup. Try this demo:

#include <stdio.h>

int
main(int c, char**v)
{
  struct foo { char array[10]; };

  struct foo x;

  printf("%p\n", x.array);
  printf("%p\n", &x.array);
  return 0;
}

Note that both pointers are the same.

I'm happy to merge this now, or wait for dgoulet.

comment:20 Changed 6 weeks ago by asn

I think we are good to merge this today given that dgoulet is away and a release is pending.

comment:21 Changed 6 weeks ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Squashed and merged!

Note: See TracTickets for help on using tickets.