Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#2385 closed defect (fixed)

rendservice.c: cleanup stack stored key material

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: audit tor-hs
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Non complete list:
rend_service_introduce(): char keys[DIGEST_LEN+CPATH_KEY_MATERIAL_LEN]

The same for rendclient.c.

Child Tickets

Change History (22)

comment:1 Changed 7 years ago by nickm

Milestone: Tor: 0.2.2.x-final

comment:2 Changed 7 years ago by nickm

Milestone: Tor: 0.2.2.x-finalTor: 0.2.1.x-final

comment:3 Changed 7 years ago by nickm

Fixed two of these in branch bug2384 (yes, bug2384. Not bug2385). More hunting will be needed.

comment:4 Changed 7 years ago by arma

Component: Tor ClientTor hidden services

comment:5 Changed 6 years ago by nickm

Milestone: Tor: 0.2.1.x-finalTor: 0.2.2.x-final

Not backporting this to 0.2.1: the risk/benefit there is too low.

Looks like my bug2384 branch already got merged into 0.2.2 a while ago: the remaining task is for somebody to carefully go through the rend*.c files to find what needs to be zeroed and add appropriate code to do so.

comment:6 Changed 6 years ago by Sebastian

Milestone: Tor: 0.2.2.x-finalTor: 0.2.3.x-final

comment:7 Changed 5 years ago by nickm

Keywords: audit added

comment:8 Changed 5 years ago by andrea

The following instances of keys and key-derived material on the stack or heap occur. Whenever on the stack, we must be sure they are zeroed before the function returns. Whenever on the heap, zero before they are freed.

  • rendclient.c:
    • rend_client_send_introduction() (line 124)
      • Payload contains hashed key on stack
    • rend_client_refetch_v2_renddesc() (line 624)
      • Descriptor ID on stack
    • rend_client_receive_rendezvous() (line 844)
      • Descriptor cookie and keys on stack
    • rend_parse_service_authorization() (line 1167)
      • Descriptor cookie on heap
  • rendservice.c:
    • rend_service_load_keys() (line 615)
      • Keys allocated on the heap
      • Descriptor cookies on the stack
    • rend_service_introduce() (line 1038)
      • Keys, digest, descriptor cookies on stack
    • rend_service_intro_has_opened() (line 1562)
      • Keys, digest on stack
    • rend_service_rendezvous_has_opened()
      • Descriptor cookie on stack

comment:9 Changed 5 years ago by andrea

Note: there are also service IDs derived from public keys by hashing which should be cleaned up. I am creating a separate ticket for this.

comment:10 Changed 5 years ago by andrea

See ticket 6176 for service IDs.

comment:11 Changed 5 years ago by andrea

Status: newneeds_review

comment:12 Changed 5 years ago by nickm

Looks good! Notes to myself:

  • In rend_service_load_keys in 9f55dfd91561643, I think the duplicated free code is somewhat worrisome. I should check whether there's a reason not to use the goto err/goto done pattern there.
  • Same function, ab2e007ffbb6a6c, there are some internal spaces between the parens in the "if ( ... )" and the tested thing.
  • In the changes file, a changelog entry that says it's a bugfix is supposed to say what the bug number was and version the bug appeared in.

comment:13 Changed 5 years ago by nickm

Okay, I just refactored the living heck out of rend_service_load_keys, because having two separate cleanup codas (one inside the loop and one outside) was making me super twitchy. Now branch "bug2385" in my public repository (based on yours) could use some review, when you have time.

comment:14 Changed 5 years ago by andrea

Resolution: fixed
Status: needs_reviewclosed

Looks like an improvement to me. Thanks.

comment:15 Changed 5 years ago by nickm

Resolution: fixed
Status: closedreopened

fwiw , please

comment:16 Changed 5 years ago by nickm

oops. This isn't actually merged yet. Let's not close it right now.

comment:17 Changed 5 years ago by nickm

Status: reopenedneeds_review

In addition to thinking it was an improvement, are you also reasonably satisfied I didn't break anything?

comment:18 Changed 5 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

I think maybe I should merge this into 0.2.4.x instead; it's gotten kind of big for a change between -beta and -rc. Any objections? Any more reviews?

comment:19 Changed 5 years ago by andrea

No objections. Probably a good idea.

comment:20 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Okay; merged into master for inclusion in 0.2.4.x. Thanks!

comment:21 Changed 5 years ago by nickm

Keywords: tor-hs added

comment:22 Changed 5 years ago by nickm

Component: Tor Hidden ServicesTor
Note: See TracTickets for help on using tickets.