Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#2385 closed defect (fixed)

rendservice.c: cleanup stack stored key material

Reported by: cypherpunks Owned by:
Priority: normal Milestone: Tor: 0.2.4.x-final
Component: Tor Version:
Keywords: audit tor-hs Cc:
Actual Points: Parent ID:
Points:

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 5 years ago by nickm

  • Milestone set to Tor: 0.2.2.x-final

comment:2 Changed 5 years ago by nickm

  • Milestone changed from Tor: 0.2.2.x-final to Tor: 0.2.1.x-final

comment:3 Changed 5 years ago by nickm

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

comment:4 Changed 5 years ago by arma

  • Component changed from Tor Client to Tor hidden services

comment:5 Changed 4 years ago by nickm

  • Milestone changed from Tor: 0.2.1.x-final to Tor: 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 4 years ago by Sebastian

  • Milestone changed from Tor: 0.2.2.x-final to Tor: 0.2.3.x-final

comment:7 Changed 3 years ago by nickm

  • Keywords audit added

comment:8 Changed 3 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 3 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 3 years ago by andrea

See ticket 6176 for service IDs.

comment:11 Changed 3 years ago by andrea

  • Status changed from new to needs_review

comment:12 Changed 3 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 3 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 3 years ago by andrea

  • Resolution set to fixed
  • Status changed from needs_review to closed

Looks like an improvement to me. Thanks.

comment:15 Changed 3 years ago by nickm

  • Resolution fixed deleted
  • Status changed from closed to reopened

fwiw , please

comment:16 Changed 3 years ago by nickm

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

comment:17 Changed 3 years ago by nickm

  • Status changed from reopened to needs_review

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

comment:18 Changed 3 years ago by nickm

  • Milestone changed from Tor: 0.2.3.x-final to Tor: 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 3 years ago by andrea

No objections. Probably a good idea.

comment:20 Changed 3 years ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed

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

comment:21 Changed 3 years ago by nickm

  • Keywords tor-hs added

comment:22 Changed 3 years ago by nickm

  • Component changed from Tor Hidden Services to Tor
Note: See TracTickets for help on using tickets.