rendservice.c: cleanup stack stored key material

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

The same for rendclient.c.

changed milestone to %Tor: 0.2.4.x-final

added audit component::core tor/tor milestone::Tor: 0.2.4.x-final priority::medium resolution::fixed status::closed tor-hs type::defect labels

Trac:
Milestone: N/A to Tor: 0.2.2.x-final

Trac:
Milestone: Tor: 0.2.2.x-final to Tor: 0.2.1.x-final

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

Trac:
Component: Tor Client to Tor hidden services
Actualpoints: N/A to N/A
Points: N/A to N/A

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.

Trac:
Milestone: Tor: 0.2.1.x-final to Tor: 0.2.2.x-final

Trac:
Milestone: Tor: 0.2.2.x-final to Tor: 0.2.3.x-final

Trac:
Keywords: N/A deleted, audit added

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

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.

See ticket 6176 for service IDs.

This is done; pull from branch bug2385 of my repo:

https://gitweb.torproject.org/user/andrea/tor.git/shortlog/refs/heads/bug2385

Trac:
Status: new to needs_review

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.

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.

Looks like an improvement to me. Thanks.

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

fwiw , please

Trac:
Status: closed to reopened
Resolution: fixed to N/A

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

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

Trac:
Status: reopened to needs_review

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?

Trac:
Milestone: Tor: 0.2.3.x-final to Tor: 0.2.4.x-final