From 9f55dfd915616436d00a4495bbca749a3ae7d287 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Fri, 15 Jun 2012 19:54:54 -0700 Subject: [PATCH 01/17] Clean up keys on stack in rend_service_load_keys() --- src/or/rendservice.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 11e80b70f9..2713596903 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -617,6 +617,8 @@ rend_service_load_keys(void) int r = 0; char fname[512]; char buf[1500]; + char desc_cook_out[3*REND_DESC_COOKIE_LEN_BASE64+1]; + char service_id[16+1]; SMARTLIST_FOREACH_BEGIN(rend_service_list, rend_service_t *, s) { if (s->private_key) @@ -709,8 +711,6 @@ rend_service_load_keys(void) * ones if a client is new. */ SMARTLIST_FOREACH_BEGIN(s->clients, rend_authorized_client_t *, client) { - char desc_cook_out[3*REND_DESC_COOKIE_LEN_BASE64+1]; - char service_id[16+1]; rend_authorized_client_t *parsed = strmap_get(parsed_clients, client->client_name); int written; @@ -727,6 +727,9 @@ rend_service_load_keys(void) REND_DESC_COOKIE_LEN) < 0) { log_warn(LD_BUG, "Could not base64-encode descriptor cookie."); strmap_free(parsed_clients, rend_authorized_client_strmap_item_free); + /* Clear these here for the early error exit */ + memset(desc_cook_out, 0, sizeof(desc_cook_out)); + memset(service_id, 0, sizeof(service_id)); return -1; } /* Copy client key from parsed entry or create new one if required. */ @@ -823,6 +826,9 @@ rend_service_load_keys(void) tor_free(client_keys_str); strmap_free(parsed_clients, rend_authorized_client_strmap_item_free); if (r<0) { + /* Clear these here for the early error exit */ + memset(desc_cook_out, 0, sizeof(desc_cook_out)); + memset(service_id, 0, sizeof(service_id)); if (open_cfile) abort_writing_to_file(open_cfile); if (open_hfile) @@ -833,7 +839,18 @@ rend_service_load_keys(void) finish_writing_to_file(open_hfile); } } + + /* + * Clear stack buffers that held key-derived material; we do this here + * for each iteration so that we don't have to put this before all of + * the early error exits (check/create directory, check service file, + * and so forth) that otherwise might have leftover key from the + * previous iteration on the stack. + */ + memset(desc_cook_out, 0, sizeof(desc_cook_out)); + memset(service_id, 0, sizeof(service_id)); } SMARTLIST_FOREACH_END(s); + return r; } From d43ba536df1b7a2433b4e1f7a2faa5a089b570c5 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Fri, 15 Jun 2012 20:19:02 -0700 Subject: [PATCH 02/17] Clean up keys on stack in rend_service_introduce() --- src/or/rendservice.c | 40 +++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 2713596903..85e8500ff0 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -1055,6 +1055,7 @@ int rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, size_t request_len) { + int status = 0; char *ptr, *r_cookie; extend_info_t *extend_info = NULL; char buf[RELAY_PAYLOAD_SIZE]; @@ -1085,7 +1086,7 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, log_warn(LD_PROTOCOL, "Got an INTRODUCE2 over a non-introduction circuit %d.", circuit->_base.n_circ_id); - return -1; + goto err; } #ifndef NON_ANONYMOUS_MODE_ENABLED @@ -1103,7 +1104,7 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, DH_KEY_LEN+42) { log_warn(LD_PROTOCOL, "Got a truncated INTRODUCE2 cell on circ %d.", circuit->_base.n_circ_id); - return -1; + goto err; } /* look up service depending on circuit. */ @@ -1113,7 +1114,7 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, log_warn(LD_BUG, "Internal error: Got an INTRODUCE2 cell on an intro " "circ for an unrecognized service %s.", escaped(serviceid)); - return -1; + goto err; } /* use intro key instead of service key. */ @@ -1126,14 +1127,14 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, (char*)request, REND_SERVICE_ID_LEN); log_warn(LD_REND, "Got an INTRODUCE2 cell for the wrong service (%s).", escaped(serviceid)); - return -1; + goto err; } keylen = crypto_pk_keysize(intro_key); if (request_len < keylen+DIGEST_LEN) { log_warn(LD_PROTOCOL, "PK-encrypted portion of INTRODUCE2 cell was truncated."); - return -1; + goto err; } intro_point = find_intro_point(circuit); @@ -1141,7 +1142,7 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, log_warn(LD_BUG, "Internal error: Got an INTRODUCE2 cell on an intro circ " "(for service %s) with no corresponding rend_intro_point_t.", escaped(serviceid)); - return -1; + goto err; } if (!service->accepted_intro_dh_parts) @@ -1160,7 +1161,7 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, log_warn(LD_REND, "Possible replay detected! We received an " "INTRODUCE2 cell with same PK-encrypted part %d seconds ago. " "Dropping cell.", (int)(now-*access_time)); - return -1; + goto err; } access_time = tor_malloc(sizeof(time_t)); *access_time = now; @@ -1176,7 +1177,7 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, PK_PKCS1_OAEP_PADDING,1); if (r<0) { log_warn(LD_PROTOCOL, "Couldn't decrypt INTRODUCE2 cell."); - return -1; + goto err; } len = r; if (*buf == 3) { @@ -1191,7 +1192,7 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, if (auth_len != REND_DESC_COOKIE_LEN) { log_info(LD_REND, "Wrong auth data size %d, should be %d.", (int)auth_len, REND_DESC_COOKIE_LEN); - return -1; + goto err; } memcpy(auth_data, buf+4, sizeof(auth_data)); v3_shift += 2+REND_DESC_COOKIE_LEN; @@ -1252,12 +1253,12 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, if (!ptr || ptr == rp_nickname) { log_warn(LD_PROTOCOL, "Couldn't find a nul-padded nickname in INTRODUCE2 cell."); - return -1; + goto err; } if ((version == 0 && !is_legal_nickname(rp_nickname)) || (version == 1 && !is_legal_nickname_or_hexdigest(rp_nickname))) { log_warn(LD_PROTOCOL, "Bad nickname in INTRODUCE2 cell."); - return -1; + goto err; } /* Okay, now we know that a nickname is at the start of the buffer. */ ptr = rp_nickname+nickname_field_len; @@ -1421,15 +1422,24 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, memcpy(cpath->handshake_digest, keys, DIGEST_LEN); if (extend_info) extend_info_free(extend_info); - memset(keys, 0, sizeof(keys)); - return 0; + goto done; + err: - memset(keys, 0, sizeof(keys)); + status = -1; if (dh) crypto_dh_free(dh); if (launched) circuit_mark_for_close(TO_CIRCUIT(launched), reason); if (extend_info) extend_info_free(extend_info); - return -1; + done: + memset(keys, 0, sizeof(keys)); + memset(buf, 0, sizeof(buf)); + memset(serviceid, 0, sizeof(serviceid)); + memset(hexcookie, 0, sizeof(hexcookie)); + memset(intro_key_digest, 0, sizeof(intro_key_digest)); + memset(auth_data, 0, sizeof(auth_data)); + memset(diffie_hellman_hash, 0, sizeof(diffie_hellman_hash)); + + return status; } /** Called when we fail building a rendezvous circuit at some point other From 88c5d3ca55dddd39e336272c9ce56a59e0092265 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Fri, 15 Jun 2012 20:43:33 -0700 Subject: [PATCH 03/17] Clean keys on stack in rend_service_intro_has_opened() --- src/or/rendservice.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 85e8500ff0..50950d1ac2 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -1627,8 +1627,8 @@ rend_service_intro_has_opened(origin_circuit_t *circuit) this case, we might as well close the thing. */ log_info(LD_CIRC|LD_REND, "We have just finished an introduction " "circuit, but we already have enough. Closing it."); - circuit_mark_for_close(TO_CIRCUIT(circuit), END_CIRC_REASON_NONE); - return; + reason = END_CIRC_REASON_NONE; + goto err; } else { tor_assert(circuit->build_state->is_internal); log_info(LD_CIRC|LD_REND, "We have just finished an introduction " @@ -1649,7 +1649,7 @@ rend_service_intro_has_opened(origin_circuit_t *circuit) } circuit_has_opened(circuit); - return; + goto done; } } @@ -1695,9 +1695,16 @@ rend_service_intro_has_opened(origin_circuit_t *circuit) goto err; } - return; + goto done; + err: circuit_mark_for_close(TO_CIRCUIT(circuit), reason); + done: + memset(buf, 0, sizeof(buf)); + memset(auth, 0, sizeof(auth)); + memset(serviceid, 0, sizeof(serviceid)); + + return; } /** Called when we get an INTRO_ESTABLISHED cell; mark the circuit as a From 276f95182c864a4cb9457bf56d842ef297e2cd2b Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Fri, 15 Jun 2012 20:54:45 -0700 Subject: [PATCH 04/17] Clean keys on stack in rend_service_rendezvous_has_opened() --- src/or/rendservice.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 50950d1ac2..acc3cea223 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -1847,9 +1847,16 @@ rend_service_rendezvous_has_opened(origin_circuit_t *circuit) /* Change the circuit purpose. */ circuit_change_purpose(TO_CIRCUIT(circuit), CIRCUIT_PURPOSE_S_REND_JOINED); - return; + goto done; + err: circuit_mark_for_close(TO_CIRCUIT(circuit), reason); + done: + memset(buf, 0, sizeof(buf)); + memset(serviceid, 0, sizeof(serviceid)); + memset(hexcookie, 0, sizeof(hexcookie)); + + return; } /* From ab2e007ffbb6a6cdf8765e4beaa2bb69c1289036 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Fri, 15 Jun 2012 21:17:02 -0700 Subject: [PATCH 05/17] In rend_service_load_keys(), clear extended descriptor cookie and buffer, clear temporary heap space for client key, and check if serializing client key fails --- src/or/rendservice.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index acc3cea223..b257c7a190 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -619,6 +619,7 @@ rend_service_load_keys(void) char buf[1500]; char desc_cook_out[3*REND_DESC_COOKIE_LEN_BASE64+1]; char service_id[16+1]; + char extended_desc_cookie[REND_DESC_COOKIE_LEN+1]; SMARTLIST_FOREACH_BEGIN(rend_service_list, rend_service_t *, s) { if (s->private_key) @@ -765,15 +766,25 @@ rend_service_load_keys(void) } if (client->client_key) { char *client_key_out = NULL; - crypto_pk_write_private_key_to_string(client->client_key, - &client_key_out, &len); + if ( crypto_pk_write_private_key_to_string(client->client_key, + &client_key_out, &len) != 0 ) { + log_warn(LD_BUG, "Internal error: " + "crypto_pk_write_private_key_to_string() failed."); + goto err; + } if (rend_get_service_id(client->client_key, service_id)<0) { log_warn(LD_BUG, "Internal error: couldn't encode service ID."); + /* + * len is string length, not buffer length, but last byte is NUL + * anyway. + */ + memset(client_key_out, 0, len); tor_free(client_key_out); goto err; } written = tor_snprintf(buf + written, sizeof(buf) - written, "client-key\n%s", client_key_out); + memset(client_key_out, 0, len); tor_free(client_key_out); if (written < 0) { log_warn(LD_BUG, "Could not write client entry."); @@ -794,7 +805,6 @@ rend_service_load_keys(void) tor_snprintf(buf, sizeof(buf),"%s.onion %s # client: %s\n", s->service_id, desc_cook_out, client->client_name); } else { - char extended_desc_cookie[REND_DESC_COOKIE_LEN+1]; memcpy(extended_desc_cookie, client->descriptor_cookie, REND_DESC_COOKIE_LEN); extended_desc_cookie[REND_DESC_COOKIE_LEN] = @@ -827,8 +837,11 @@ rend_service_load_keys(void) strmap_free(parsed_clients, rend_authorized_client_strmap_item_free); if (r<0) { /* Clear these here for the early error exit */ + /* We have to clear buf because encoded keys can get written to it */ + memset(buf, 0, sizeof(buf)); memset(desc_cook_out, 0, sizeof(desc_cook_out)); memset(service_id, 0, sizeof(service_id)); + memset(extended_desc_cookie, 0, sizeof(extended_desc_cookie)); if (open_cfile) abort_writing_to_file(open_cfile); if (open_hfile) @@ -847,8 +860,10 @@ rend_service_load_keys(void) * and so forth) that otherwise might have leftover key from the * previous iteration on the stack. */ + memset(buf, 0, sizeof(buf)); memset(desc_cook_out, 0, sizeof(desc_cook_out)); memset(service_id, 0, sizeof(service_id)); + memset(extended_desc_cookie, 0, sizeof(extended_desc_cookie)); } SMARTLIST_FOREACH_END(s); return r; From a8bcbe7bf7d826c5aa97bb1298669e9b41cbadc5 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Fri, 15 Jun 2012 21:25:25 -0700 Subject: [PATCH 06/17] Clean up keys on stack in rend_client_send_introduction() --- src/or/rendclient.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/or/rendclient.c b/src/or/rendclient.c index 6c751be27d..39ba05a0a0 100644 --- a/src/or/rendclient.c +++ b/src/or/rendclient.c @@ -132,6 +132,7 @@ rend_client_send_introduction(origin_circuit_t *introcirc, crypt_path_t *cpath; off_t dh_offset; crypto_pk_t *intro_key = NULL; + int status = 0; tor_assert(introcirc->_base.purpose == CIRCUIT_PURPOSE_C_INTRODUCING); tor_assert(rendcirc->_base.purpose == CIRCUIT_PURPOSE_C_REND_READY); @@ -161,7 +162,8 @@ rend_client_send_introduction(origin_circuit_t *introcirc, } } - return -1; + status = -1; + goto cleanup; } /* first 20 bytes of payload are the hash of Bob's pk */ @@ -184,13 +186,16 @@ rend_client_send_introduction(origin_circuit_t *introcirc, smartlist_len(entry->parsed->intro_nodes)); if (rend_client_reextend_intro_circuit(introcirc)) { + status = -2; goto perm_err; } else { - return -1; + status = -1; + goto cleanup; } } if (crypto_pk_get_digest(intro_key, payload)<0) { log_warn(LD_BUG, "Internal error: couldn't hash public key."); + status = -2; goto perm_err; } @@ -202,10 +207,12 @@ rend_client_send_introduction(origin_circuit_t *introcirc, cpath->magic = CRYPT_PATH_MAGIC; if (!(cpath->dh_handshake_state = crypto_dh_new(DH_TYPE_REND))) { log_warn(LD_BUG, "Internal error: couldn't allocate DH."); + status = -2; goto perm_err; } if (crypto_dh_generate_public(cpath->dh_handshake_state)<0) { log_warn(LD_BUG, "Internal error: couldn't generate g^x."); + status = -2; goto perm_err; } } @@ -256,6 +263,7 @@ rend_client_send_introduction(origin_circuit_t *introcirc, if (crypto_dh_get_public(cpath->dh_handshake_state, tmp+dh_offset, DH_KEY_LEN)<0) { log_warn(LD_BUG, "Internal error: couldn't extract g^x."); + status = -2; goto perm_err; } @@ -269,6 +277,7 @@ rend_client_send_introduction(origin_circuit_t *introcirc, PK_PKCS1_OAEP_PADDING, 0); if (r<0) { log_warn(LD_BUG,"Internal error: hybrid pk encrypt failed."); + status = -2; goto perm_err; } @@ -288,7 +297,8 @@ rend_client_send_introduction(origin_circuit_t *introcirc, introcirc->cpath->prev)<0) { /* introcirc is already marked for close. leave rendcirc alone. */ log_warn(LD_BUG, "Couldn't send INTRODUCE1 cell"); - return -2; + status = -2; + goto cleanup; } /* Now, we wait for an ACK or NAK on this circuit. */ @@ -299,12 +309,17 @@ rend_client_send_introduction(origin_circuit_t *introcirc, * state. */ introcirc->_base.timestamp_dirty = time(NULL); - return 0; + goto cleanup; + perm_err: if (!introcirc->_base.marked_for_close) circuit_mark_for_close(TO_CIRCUIT(introcirc), END_CIRC_REASON_INTERNAL); circuit_mark_for_close(TO_CIRCUIT(rendcirc), END_CIRC_REASON_INTERNAL); - return -2; + cleanup: + memset(payload, 0, sizeof(payload)); + memset(tmp, 0, sizeof(tmp)); + + return status; } /** Called when a rendezvous circuit is open; sends a establish From 7f24b9b8c3d29143deb65dae34d2c35b940319e3 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Fri, 15 Jun 2012 21:39:28 -0700 Subject: [PATCH 07/17] Clean up keys on stack in rend_client_refetch_v2_renddesc() --- src/or/rendclient.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/or/rendclient.c b/src/or/rendclient.c index 39ba05a0a0..7208fb813b 100644 --- a/src/or/rendclient.c +++ b/src/or/rendclient.c @@ -674,10 +674,17 @@ rend_client_refetch_v2_renddesc(const rend_data_t *rend_query) time(NULL), chosen_replica) < 0) { log_warn(LD_REND, "Internal error: Computing v2 rendezvous " "descriptor ID did not succeed."); - return; + /* + * Hmm, can this write anything to descriptor_id and still fail? + * Let's clear it just to be safe. + * + * From here on, any returns should goto done which clears + * descriptor_id so we don't leave key-derived material on the stack. + */ + goto done; } if (directory_get_from_hs_dir(descriptor_id, rend_query) != 0) - return; /* either success or failure, but we're done */ + goto done; /* either success or failure, but we're done */ } /* If we come here, there are no hidden service directories left. */ log_info(LD_REND, "Could not pick one of the responsible hidden " @@ -685,6 +692,10 @@ rend_client_refetch_v2_renddesc(const rend_data_t *rend_query) "we already tried them all unsuccessfully."); /* Close pending connections. */ rend_client_desc_trynow(rend_query->onion_address); + +done: + memset(descriptor_id, 0, sizeof(descriptor_id)); + return; } From b5280efc179bd5ed5607246a3b4460fb7f7c0068 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Fri, 15 Jun 2012 21:47:06 -0700 Subject: [PATCH 08/17] Clean up keys on stack in rend_parse_service_authorization() --- src/or/rendclient.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/or/rendclient.c b/src/or/rendclient.c index 7208fb813b..c74be520c5 100644 --- a/src/or/rendclient.c +++ b/src/or/rendclient.c @@ -1198,11 +1198,11 @@ rend_parse_service_authorization(const or_options_t *options, strmap_t *parsed = strmap_new(); smartlist_t *sl = smartlist_new(); rend_service_authorization_t *auth = NULL; + char descriptor_cookie_tmp[REND_DESC_COOKIE_LEN+2]; + char descriptor_cookie_base64ext[REND_DESC_COOKIE_LEN_BASE64+2+1]; for (line = options->HidServAuth; line; line = line->next) { char *onion_address, *descriptor_cookie; - char descriptor_cookie_tmp[REND_DESC_COOKIE_LEN+2]; - char descriptor_cookie_base64ext[REND_DESC_COOKIE_LEN_BASE64+2+1]; int auth_type_val = 0; auth = NULL; SMARTLIST_FOREACH(sl, char *, c, tor_free(c);); @@ -1279,6 +1279,8 @@ rend_parse_service_authorization(const or_options_t *options, } else { strmap_free(parsed, rend_service_authorization_strmap_item_free); } + memset(descriptor_cookie_tmp, 0, sizeof(descriptor_cookie_tmp)); + memset(descriptor_cookie_base64ext, 0, sizeof(descriptor_cookie_base64ext)); return res; } From 10130e5979d01e8707033d0a23cc2f686ae6f598 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Fri, 15 Jun 2012 21:48:15 -0700 Subject: [PATCH 09/17] Appease make check-spaces --- src/or/rendclient.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/rendclient.c b/src/or/rendclient.c index c74be520c5..5b3b92e406 100644 --- a/src/or/rendclient.c +++ b/src/or/rendclient.c @@ -693,7 +693,7 @@ rend_client_refetch_v2_renddesc(const rend_data_t *rend_query) /* Close pending connections. */ rend_client_desc_trynow(rend_query->onion_address); -done: + done: memset(descriptor_id, 0, sizeof(descriptor_id)); return; From a782893ed0d565aa5698929ec85524cd67258e7c Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Fri, 15 Jun 2012 21:54:26 -0700 Subject: [PATCH 10/17] Add change file bug2385 --- changes/bug2385 | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changes/bug2385 diff --git a/changes/bug2385 b/changes/bug2385 new file mode 100644 index 0000000000..e7151f49eb --- /dev/null +++ b/changes/bug2385 @@ -0,0 +1,5 @@ + o Minor bugfixes: + - Clear keys and key-derived material left on the stack in rendservice.c + and rendclient.c + - Check return value of crypto_pk_write_private_key_to_string() in + end_service_load_keys() From b44693f32d1add5dbf1f42665889b9eac0be6aa3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 18 Jun 2012 12:43:20 -0400 Subject: [PATCH 11/17] Refactor rend_service_load_keys() into outer loop and loop contents --- src/or/config.c | 2 +- src/or/rendservice.c | 33 +++++++++++++++++++++++---------- src/or/rendservice.h | 2 +- 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index 598051ef4f..2ea3746139 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1548,7 +1548,7 @@ options_act(const or_options_t *old_options) monitor_owning_controller_process(options->OwningControllerProcess); /* reload keys as needed for rendezvous services. */ - if (rend_service_load_keys()<0) { + if (rend_service_load_all_keys()<0) { log_warn(LD_GENERAL,"Error loading rendezvous service keys"); return -1; } diff --git a/src/or/rendservice.c b/src/or/rendservice.c index b257c7a190..d1400938fd 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -31,6 +31,8 @@ static rend_intro_point_t *find_intro_point(origin_circuit_t *circ); static int intro_point_accepted_intro_count(rend_intro_point_t *intro); static int intro_point_should_expire_now(rend_intro_point_t *intro, time_t now); +struct rend_service_t; +static int rend_service_load_keys(struct rend_service_t *s); /** Represents the mapping from a virtual port of a rendezvous service to * a real port on some IP. @@ -609,10 +611,28 @@ rend_service_update_descriptor(rend_service_t *service) /** Load and/or generate private keys for all hidden services, possibly * including keys for client authorization. Return 0 on success, -1 on - * failure. - */ + * failure. */ int -rend_service_load_keys(void) +rend_service_load_all_keys(void) +{ + SMARTLIST_FOREACH_BEGIN(rend_service_list, rend_service_t *, s) { + if (s->private_key) + continue; + log_info(LD_REND, "Loading hidden-service keys from \"%s\"", + s->directory); + + if (rend_service_load_keys(s) < 0) + return -1; + } SMARTLIST_FOREACH_END(s); + + return 0; +} + +/** Load and/or generate private keys for the hidden service s, + * possibly including keys for client authorization. Return 0 on success, -1 + * on failure. */ +static int +rend_service_load_keys(rend_service_t *s) { int r = 0; char fname[512]; @@ -621,12 +641,6 @@ rend_service_load_keys(void) char service_id[16+1]; char extended_desc_cookie[REND_DESC_COOKIE_LEN+1]; - SMARTLIST_FOREACH_BEGIN(rend_service_list, rend_service_t *, s) { - if (s->private_key) - continue; - log_info(LD_REND, "Loading hidden-service keys from \"%s\"", - s->directory); - /* Check/create directory */ if (check_private_dir(s->directory, CPD_CREATE, get_options()->User) < 0) return -1; @@ -864,7 +878,6 @@ rend_service_load_keys(void) memset(desc_cook_out, 0, sizeof(desc_cook_out)); memset(service_id, 0, sizeof(service_id)); memset(extended_desc_cookie, 0, sizeof(extended_desc_cookie)); - } SMARTLIST_FOREACH_END(s); return r; } diff --git a/src/or/rendservice.h b/src/or/rendservice.h index e5848785a8..baf8d5fb43 100644 --- a/src/or/rendservice.h +++ b/src/or/rendservice.h @@ -14,7 +14,7 @@ int num_rend_services(void); int rend_config_services(const or_options_t *options, int validate_only); -int rend_service_load_keys(void); +int rend_service_load_all_keys(void); void rend_services_introduce(void); void rend_consider_services_upload(time_t now); void rend_hsdir_routers_changed(void); From 53f5a389426caed833d5c792f55501e1f9d1dc19 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 18 Jun 2012 12:45:55 -0400 Subject: [PATCH 12/17] Fix indentation and whitespace in rend_service_load_keys --- src/or/rendservice.c | 427 +++++++++++++++++++++---------------------- 1 file changed, 213 insertions(+), 214 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index d1400938fd..275ee9837e 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -641,243 +641,242 @@ rend_service_load_keys(rend_service_t *s) char service_id[16+1]; char extended_desc_cookie[REND_DESC_COOKIE_LEN+1]; - /* Check/create directory */ - if (check_private_dir(s->directory, CPD_CREATE, get_options()->User) < 0) - return -1; + /* Check/create directory */ + if (check_private_dir(s->directory, CPD_CREATE, get_options()->User) < 0) + return -1; - /* Load key */ - if (strlcpy(fname,s->directory,sizeof(fname)) >= sizeof(fname) || - strlcat(fname,PATH_SEPARATOR"private_key",sizeof(fname)) - >= sizeof(fname)) { - log_warn(LD_CONFIG, "Directory name too long to store key file: \"%s\".", - s->directory); - return -1; - } - s->private_key = init_key_from_file(fname, 1, LOG_ERR); - if (!s->private_key) - return -1; + /* Load key */ + if (strlcpy(fname,s->directory,sizeof(fname)) >= sizeof(fname) || + strlcat(fname,PATH_SEPARATOR"private_key",sizeof(fname)) + >= sizeof(fname)) { + log_warn(LD_CONFIG, "Directory name too long to store key file: \"%s\".", + s->directory); + return -1; + } + s->private_key = init_key_from_file(fname, 1, LOG_ERR); + if (!s->private_key) + return -1; - /* Create service file */ - if (rend_get_service_id(s->private_key, s->service_id)<0) { - log_warn(LD_BUG, "Internal error: couldn't encode service ID."); - return -1; - } - if (crypto_pk_get_digest(s->private_key, s->pk_digest)<0) { - log_warn(LD_BUG, "Couldn't compute hash of public key."); - return -1; - } - if (strlcpy(fname,s->directory,sizeof(fname)) >= sizeof(fname) || - strlcat(fname,PATH_SEPARATOR"hostname",sizeof(fname)) - >= sizeof(fname)) { - log_warn(LD_CONFIG, "Directory name too long to store hostname file:" - " \"%s\".", s->directory); - return -1; - } - tor_snprintf(buf, sizeof(buf),"%s.onion\n", s->service_id); - if (write_str_to_file(fname,buf,0)<0) { - log_warn(LD_CONFIG, "Could not write onion address to hostname file."); - return -1; - } + /* Create service file */ + if (rend_get_service_id(s->private_key, s->service_id)<0) { + log_warn(LD_BUG, "Internal error: couldn't encode service ID."); + return -1; + } + if (crypto_pk_get_digest(s->private_key, s->pk_digest)<0) { + log_warn(LD_BUG, "Couldn't compute hash of public key."); + return -1; + } + if (strlcpy(fname,s->directory,sizeof(fname)) >= sizeof(fname) || + strlcat(fname,PATH_SEPARATOR"hostname",sizeof(fname)) + >= sizeof(fname)) { + log_warn(LD_CONFIG, "Directory name too long to store hostname file:" + " \"%s\".", s->directory); + return -1; + } + tor_snprintf(buf, sizeof(buf),"%s.onion\n", s->service_id); + if (write_str_to_file(fname,buf,0)<0) { + log_warn(LD_CONFIG, "Could not write onion address to hostname file."); + return -1; + } - /* If client authorization is configured, load or generate keys. */ - if (s->auth_type != REND_NO_AUTH) { - char *client_keys_str = NULL; - strmap_t *parsed_clients = strmap_new(); - char cfname[512]; - FILE *cfile, *hfile; - open_file_t *open_cfile = NULL, *open_hfile = NULL; + /* If client authorization is configured, load or generate keys. */ + if (s->auth_type != REND_NO_AUTH) { + char *client_keys_str = NULL; + strmap_t *parsed_clients = strmap_new(); + char cfname[512]; + FILE *cfile, *hfile; + open_file_t *open_cfile = NULL, *open_hfile = NULL; - /* Load client keys and descriptor cookies, if available. */ - if (tor_snprintf(cfname, sizeof(cfname), "%s"PATH_SEPARATOR"client_keys", - s->directory)<0) { - log_warn(LD_CONFIG, "Directory name too long to store client keys " - "file: \"%s\".", s->directory); + /* Load client keys and descriptor cookies, if available. */ + if (tor_snprintf(cfname, sizeof(cfname), "%s"PATH_SEPARATOR"client_keys", + s->directory)<0) { + log_warn(LD_CONFIG, "Directory name too long to store client keys " + "file: \"%s\".", s->directory); + goto err; + } + client_keys_str = read_file_to_str(cfname, RFTS_IGNORE_MISSING, NULL); + if (client_keys_str) { + if (rend_parse_client_keys(parsed_clients, client_keys_str) < 0) { + log_warn(LD_CONFIG, "Previously stored client_keys file could not " + "be parsed."); goto err; + } else { + log_info(LD_CONFIG, "Parsed %d previously stored client entries.", + strmap_size(parsed_clients)); + tor_free(client_keys_str); } - client_keys_str = read_file_to_str(cfname, RFTS_IGNORE_MISSING, NULL); - if (client_keys_str) { - if (rend_parse_client_keys(parsed_clients, client_keys_str) < 0) { - log_warn(LD_CONFIG, "Previously stored client_keys file could not " - "be parsed."); + } + + /* Prepare client_keys and hostname files. */ + if (!(cfile = start_writing_to_stdio_file(cfname, + OPEN_FLAGS_REPLACE | O_TEXT, + 0600, &open_cfile))) { + log_warn(LD_CONFIG, "Could not open client_keys file %s", + escaped(cfname)); + goto err; + } + if (!(hfile = start_writing_to_stdio_file(fname, + OPEN_FLAGS_REPLACE | O_TEXT, + 0600, &open_hfile))) { + log_warn(LD_CONFIG, "Could not open hostname file %s", escaped(fname)); + goto err; + } + + /* Either use loaded keys for configured clients or generate new + * ones if a client is new. */ + SMARTLIST_FOREACH_BEGIN(s->clients, rend_authorized_client_t *, client) { + rend_authorized_client_t *parsed = + strmap_get(parsed_clients, client->client_name); + int written; + size_t len; + /* Copy descriptor cookie from parsed entry or create new one. */ + if (parsed) { + memcpy(client->descriptor_cookie, parsed->descriptor_cookie, + REND_DESC_COOKIE_LEN); + } else { + crypto_rand(client->descriptor_cookie, REND_DESC_COOKIE_LEN); + } + if (base64_encode(desc_cook_out, 3*REND_DESC_COOKIE_LEN_BASE64+1, + client->descriptor_cookie, + REND_DESC_COOKIE_LEN) < 0) { + log_warn(LD_BUG, "Could not base64-encode descriptor cookie."); + strmap_free(parsed_clients, rend_authorized_client_strmap_item_free); + /* Clear these here for the early error exit */ + memset(desc_cook_out, 0, sizeof(desc_cook_out)); + memset(service_id, 0, sizeof(service_id)); + return -1; + } + /* Copy client key from parsed entry or create new one if required. */ + if (parsed && parsed->client_key) { + client->client_key = crypto_pk_dup_key(parsed->client_key); + } else if (s->auth_type == REND_STEALTH_AUTH) { + /* Create private key for client. */ + crypto_pk_t *prkey = NULL; + if (!(prkey = crypto_pk_new())) { + log_warn(LD_BUG,"Error constructing client key"); goto err; - } else { - log_info(LD_CONFIG, "Parsed %d previously stored client entries.", - strmap_size(parsed_clients)); - tor_free(client_keys_str); } + if (crypto_pk_generate_key(prkey)) { + log_warn(LD_BUG,"Error generating client key"); + crypto_pk_free(prkey); + goto err; + } + if (crypto_pk_check_key(prkey) <= 0) { + log_warn(LD_BUG,"Generated client key seems invalid"); + crypto_pk_free(prkey); + goto err; + } + client->client_key = prkey; } - - /* Prepare client_keys and hostname files. */ - if (!(cfile = start_writing_to_stdio_file(cfname, - OPEN_FLAGS_REPLACE | O_TEXT, - 0600, &open_cfile))) { - log_warn(LD_CONFIG, "Could not open client_keys file %s", - escaped(cfname)); + /* Add entry to client_keys file. */ + desc_cook_out[strlen(desc_cook_out)-1] = '\0'; /* Remove newline. */ + written = tor_snprintf(buf, sizeof(buf), + "client-name %s\ndescriptor-cookie %s\n", + client->client_name, desc_cook_out); + if (written < 0) { + log_warn(LD_BUG, "Could not write client entry."); goto err; } - if (!(hfile = start_writing_to_stdio_file(fname, - OPEN_FLAGS_REPLACE | O_TEXT, - 0600, &open_hfile))) { - log_warn(LD_CONFIG, "Could not open hostname file %s", escaped(fname)); - goto err; - } - - /* Either use loaded keys for configured clients or generate new - * ones if a client is new. */ - SMARTLIST_FOREACH_BEGIN(s->clients, rend_authorized_client_t *, client) - { - rend_authorized_client_t *parsed = - strmap_get(parsed_clients, client->client_name); - int written; - size_t len; - /* Copy descriptor cookie from parsed entry or create new one. */ - if (parsed) { - memcpy(client->descriptor_cookie, parsed->descriptor_cookie, - REND_DESC_COOKIE_LEN); - } else { - crypto_rand(client->descriptor_cookie, REND_DESC_COOKIE_LEN); + if (client->client_key) { + char *client_key_out = NULL; + if (crypto_pk_write_private_key_to_string(client->client_key, + &client_key_out, &len) != 0) { + log_warn(LD_BUG, "Internal error: " + "crypto_pk_write_private_key_to_string() failed."); + goto err; } - if (base64_encode(desc_cook_out, 3*REND_DESC_COOKIE_LEN_BASE64+1, - client->descriptor_cookie, - REND_DESC_COOKIE_LEN) < 0) { - log_warn(LD_BUG, "Could not base64-encode descriptor cookie."); - strmap_free(parsed_clients, rend_authorized_client_strmap_item_free); - /* Clear these here for the early error exit */ - memset(desc_cook_out, 0, sizeof(desc_cook_out)); - memset(service_id, 0, sizeof(service_id)); - return -1; + if (rend_get_service_id(client->client_key, service_id)<0) { + log_warn(LD_BUG, "Internal error: couldn't encode service ID."); + /* + * len is string length, not buffer length, but last byte is NUL + * anyway. + */ + memset(client_key_out, 0, len); + tor_free(client_key_out); + goto err; } - /* Copy client key from parsed entry or create new one if required. */ - if (parsed && parsed->client_key) { - client->client_key = crypto_pk_dup_key(parsed->client_key); - } else if (s->auth_type == REND_STEALTH_AUTH) { - /* Create private key for client. */ - crypto_pk_t *prkey = NULL; - if (!(prkey = crypto_pk_new())) { - log_warn(LD_BUG,"Error constructing client key"); - goto err; - } - if (crypto_pk_generate_key(prkey)) { - log_warn(LD_BUG,"Error generating client key"); - crypto_pk_free(prkey); - goto err; - } - if (crypto_pk_check_key(prkey) <= 0) { - log_warn(LD_BUG,"Generated client key seems invalid"); - crypto_pk_free(prkey); - goto err; - } - client->client_key = prkey; - } - /* Add entry to client_keys file. */ - desc_cook_out[strlen(desc_cook_out)-1] = '\0'; /* Remove newline. */ - written = tor_snprintf(buf, sizeof(buf), - "client-name %s\ndescriptor-cookie %s\n", - client->client_name, desc_cook_out); + written = tor_snprintf(buf + written, sizeof(buf) - written, + "client-key\n%s", client_key_out); + memset(client_key_out, 0, len); + tor_free(client_key_out); if (written < 0) { log_warn(LD_BUG, "Could not write client entry."); goto err; } - if (client->client_key) { - char *client_key_out = NULL; - if ( crypto_pk_write_private_key_to_string(client->client_key, - &client_key_out, &len) != 0 ) { - log_warn(LD_BUG, "Internal error: " - "crypto_pk_write_private_key_to_string() failed."); - goto err; - } - if (rend_get_service_id(client->client_key, service_id)<0) { - log_warn(LD_BUG, "Internal error: couldn't encode service ID."); - /* - * len is string length, not buffer length, but last byte is NUL - * anyway. - */ - memset(client_key_out, 0, len); - tor_free(client_key_out); - goto err; - } - written = tor_snprintf(buf + written, sizeof(buf) - written, - "client-key\n%s", client_key_out); - memset(client_key_out, 0, len); - tor_free(client_key_out); - if (written < 0) { - log_warn(LD_BUG, "Could not write client entry."); - goto err; - } - } - - if (fputs(buf, cfile) < 0) { - log_warn(LD_FS, "Could not append client entry to file: %s", - strerror(errno)); - goto err; - } - - /* Add line to hostname file. */ - if (s->auth_type == REND_BASIC_AUTH) { - /* Remove == signs (newline has been removed above). */ - desc_cook_out[strlen(desc_cook_out)-2] = '\0'; - tor_snprintf(buf, sizeof(buf),"%s.onion %s # client: %s\n", - s->service_id, desc_cook_out, client->client_name); - } else { - memcpy(extended_desc_cookie, client->descriptor_cookie, - REND_DESC_COOKIE_LEN); - extended_desc_cookie[REND_DESC_COOKIE_LEN] = - ((int)s->auth_type - 1) << 4; - if (base64_encode(desc_cook_out, 3*REND_DESC_COOKIE_LEN_BASE64+1, - extended_desc_cookie, - REND_DESC_COOKIE_LEN+1) < 0) { - log_warn(LD_BUG, "Could not base64-encode descriptor cookie."); - goto err; - } - desc_cook_out[strlen(desc_cook_out)-3] = '\0'; /* Remove A= and - newline. */ - tor_snprintf(buf, sizeof(buf),"%s.onion %s # client: %s\n", - service_id, desc_cook_out, client->client_name); - } - - if (fputs(buf, hfile)<0) { - log_warn(LD_FS, "Could not append host entry to file: %s", - strerror(errno)); - goto err; - } } - SMARTLIST_FOREACH_END(client); - goto done; - err: - r = -1; - done: - tor_free(client_keys_str); - strmap_free(parsed_clients, rend_authorized_client_strmap_item_free); - if (r<0) { - /* Clear these here for the early error exit */ - /* We have to clear buf because encoded keys can get written to it */ - memset(buf, 0, sizeof(buf)); - memset(desc_cook_out, 0, sizeof(desc_cook_out)); - memset(service_id, 0, sizeof(service_id)); - memset(extended_desc_cookie, 0, sizeof(extended_desc_cookie)); - if (open_cfile) - abort_writing_to_file(open_cfile); - if (open_hfile) - abort_writing_to_file(open_hfile); - return r; + if (fputs(buf, cfile) < 0) { + log_warn(LD_FS, "Could not append client entry to file: %s", + strerror(errno)); + goto err; + } + + /* Add line to hostname file. */ + if (s->auth_type == REND_BASIC_AUTH) { + /* Remove == signs (newline has been removed above). */ + desc_cook_out[strlen(desc_cook_out)-2] = '\0'; + tor_snprintf(buf, sizeof(buf),"%s.onion %s # client: %s\n", + s->service_id, desc_cook_out, client->client_name); } else { - finish_writing_to_file(open_cfile); - finish_writing_to_file(open_hfile); + memcpy(extended_desc_cookie, client->descriptor_cookie, + REND_DESC_COOKIE_LEN); + extended_desc_cookie[REND_DESC_COOKIE_LEN] = + ((int)s->auth_type - 1) << 4; + if (base64_encode(desc_cook_out, 3*REND_DESC_COOKIE_LEN_BASE64+1, + extended_desc_cookie, + REND_DESC_COOKIE_LEN+1) < 0) { + log_warn(LD_BUG, "Could not base64-encode descriptor cookie."); + goto err; + } + desc_cook_out[strlen(desc_cook_out)-3] = '\0'; /* Remove A= and + newline. */ + tor_snprintf(buf, sizeof(buf),"%s.onion %s # client: %s\n", + service_id, desc_cook_out, client->client_name); + } + + if (fputs(buf, hfile)<0) { + log_warn(LD_FS, "Could not append host entry to file: %s", + strerror(errno)); + goto err; } } + SMARTLIST_FOREACH_END(client); - /* - * Clear stack buffers that held key-derived material; we do this here - * for each iteration so that we don't have to put this before all of - * the early error exits (check/create directory, check service file, - * and so forth) that otherwise might have leftover key from the - * previous iteration on the stack. - */ - memset(buf, 0, sizeof(buf)); - memset(desc_cook_out, 0, sizeof(desc_cook_out)); - memset(service_id, 0, sizeof(service_id)); - memset(extended_desc_cookie, 0, sizeof(extended_desc_cookie)); + goto done; + err: + r = -1; + done: + tor_free(client_keys_str); + strmap_free(parsed_clients, rend_authorized_client_strmap_item_free); + if (r<0) { + /* Clear these here for the early error exit */ + /* We have to clear buf because encoded keys can get written to it */ + memset(buf, 0, sizeof(buf)); + memset(desc_cook_out, 0, sizeof(desc_cook_out)); + memset(service_id, 0, sizeof(service_id)); + memset(extended_desc_cookie, 0, sizeof(extended_desc_cookie)); + if (open_cfile) + abort_writing_to_file(open_cfile); + if (open_hfile) + abort_writing_to_file(open_hfile); + return r; + } else { + finish_writing_to_file(open_cfile); + finish_writing_to_file(open_hfile); + } + } + + /* + * Clear stack buffers that held key-derived material; we do this here + * for each iteration so that we don't have to put this before all of + * the early error exits (check/create directory, check service file, + * and so forth) that otherwise might have leftover key from the + * previous iteration on the stack. + */ + memset(buf, 0, sizeof(buf)); + memset(desc_cook_out, 0, sizeof(desc_cook_out)); + memset(service_id, 0, sizeof(service_id)); + memset(extended_desc_cookie, 0, sizeof(extended_desc_cookie)); return r; } From be28d10622bbeda9d31538696547474b21c401ad Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 18 Jun 2012 12:59:29 -0400 Subject: [PATCH 13/17] Refactor rend_service_load_keys() into main portion and auth portion. --- src/or/rendservice.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 275ee9837e..dedddd84b2 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -33,6 +33,7 @@ static int intro_point_should_expire_now(rend_intro_point_t *intro, time_t now); struct rend_service_t; static int rend_service_load_keys(struct rend_service_t *s); +static int rend_service_load_auth_keys(struct rend_service_t *s); /** Represents the mapping from a virtual port of a rendezvous service to * a real port on some IP. @@ -634,12 +635,8 @@ rend_service_load_all_keys(void) static int rend_service_load_keys(rend_service_t *s) { - int r = 0; char fname[512]; - char buf[1500]; - char desc_cook_out[3*REND_DESC_COOKIE_LEN_BASE64+1]; - char service_id[16+1]; - char extended_desc_cookie[REND_DESC_COOKIE_LEN+1]; + char buf[128]; /* Check/create directory */ if (check_private_dir(s->directory, CPD_CREATE, get_options()->User) < 0) @@ -676,16 +673,35 @@ rend_service_load_keys(rend_service_t *s) tor_snprintf(buf, sizeof(buf),"%s.onion\n", s->service_id); if (write_str_to_file(fname,buf,0)<0) { log_warn(LD_CONFIG, "Could not write onion address to hostname file."); + memset(buf, 0, sizeof(buf)); return -1; } + memset(buf, 0, sizeof(buf)); /* If client authorization is configured, load or generate keys. */ if (s->auth_type != REND_NO_AUTH) { - char *client_keys_str = NULL; - strmap_t *parsed_clients = strmap_new(); - char cfname[512]; - FILE *cfile, *hfile; - open_file_t *open_cfile = NULL, *open_hfile = NULL; + if (rend_service_load_auth_keys(s) < 0) + return -1; + } + + return 0; +} + +/** Load and/or generate client authorization keys for the hidden service + * s. Return 0 on success, -1 on failure. */ +static int +rend_service_load_auth_keys(rend_service_t *s) +{ + int r = 0; + char fname[512], cfname[512]; + char *client_keys_str = NULL; + strmap_t *parsed_clients = strmap_new(); + FILE *cfile, *hfile; + open_file_t *open_cfile = NULL, *open_hfile = NULL; + char extended_desc_cookie[REND_DESC_COOKIE_LEN+1]; + char desc_cook_out[3*REND_DESC_COOKIE_LEN_BASE64+1]; + char service_id[16+1]; + char buf[1500]; /* Load client keys and descriptor cookies, if available. */ if (tor_snprintf(cfname, sizeof(cfname), "%s"PATH_SEPARATOR"client_keys", @@ -864,7 +880,6 @@ rend_service_load_keys(rend_service_t *s) finish_writing_to_file(open_cfile); finish_writing_to_file(open_hfile); } - } /* * Clear stack buffers that held key-derived material; we do this here From e5a61c5176352d02852dbe996be10fc51649debc Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 18 Jun 2012 13:01:15 -0400 Subject: [PATCH 14/17] Fix indentation in rend_service_load_auth_keys --- src/or/rendservice.c | 320 +++++++++++++++++++++---------------------- 1 file changed, 160 insertions(+), 160 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index dedddd84b2..69a86afb4c 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -703,184 +703,184 @@ rend_service_load_auth_keys(rend_service_t *s) char service_id[16+1]; char buf[1500]; - /* Load client keys and descriptor cookies, if available. */ - if (tor_snprintf(cfname, sizeof(cfname), "%s"PATH_SEPARATOR"client_keys", - s->directory)<0) { - log_warn(LD_CONFIG, "Directory name too long to store client keys " - "file: \"%s\".", s->directory); + /* Load client keys and descriptor cookies, if available. */ + if (tor_snprintf(cfname, sizeof(cfname), "%s"PATH_SEPARATOR"client_keys", + s->directory)<0) { + log_warn(LD_CONFIG, "Directory name too long to store client keys " + "file: \"%s\".", s->directory); + goto err; + } + client_keys_str = read_file_to_str(cfname, RFTS_IGNORE_MISSING, NULL); + if (client_keys_str) { + if (rend_parse_client_keys(parsed_clients, client_keys_str) < 0) { + log_warn(LD_CONFIG, "Previously stored client_keys file could not " + "be parsed."); goto err; + } else { + log_info(LD_CONFIG, "Parsed %d previously stored client entries.", + strmap_size(parsed_clients)); + tor_free(client_keys_str); } - client_keys_str = read_file_to_str(cfname, RFTS_IGNORE_MISSING, NULL); - if (client_keys_str) { - if (rend_parse_client_keys(parsed_clients, client_keys_str) < 0) { - log_warn(LD_CONFIG, "Previously stored client_keys file could not " - "be parsed."); + } + + /* Prepare client_keys and hostname files. */ + if (!(cfile = start_writing_to_stdio_file(cfname, + OPEN_FLAGS_REPLACE | O_TEXT, + 0600, &open_cfile))) { + log_warn(LD_CONFIG, "Could not open client_keys file %s", + escaped(cfname)); + goto err; + } + if (!(hfile = start_writing_to_stdio_file(fname, + OPEN_FLAGS_REPLACE | O_TEXT, + 0600, &open_hfile))) { + log_warn(LD_CONFIG, "Could not open hostname file %s", escaped(fname)); + goto err; + } + + /* Either use loaded keys for configured clients or generate new + * ones if a client is new. */ + SMARTLIST_FOREACH_BEGIN(s->clients, rend_authorized_client_t *, client) { + rend_authorized_client_t *parsed = + strmap_get(parsed_clients, client->client_name); + int written; + size_t len; + /* Copy descriptor cookie from parsed entry or create new one. */ + if (parsed) { + memcpy(client->descriptor_cookie, parsed->descriptor_cookie, + REND_DESC_COOKIE_LEN); + } else { + crypto_rand(client->descriptor_cookie, REND_DESC_COOKIE_LEN); + } + if (base64_encode(desc_cook_out, 3*REND_DESC_COOKIE_LEN_BASE64+1, + client->descriptor_cookie, + REND_DESC_COOKIE_LEN) < 0) { + log_warn(LD_BUG, "Could not base64-encode descriptor cookie."); + strmap_free(parsed_clients, rend_authorized_client_strmap_item_free); + /* Clear these here for the early error exit */ + memset(desc_cook_out, 0, sizeof(desc_cook_out)); + memset(service_id, 0, sizeof(service_id)); + return -1; + } + /* Copy client key from parsed entry or create new one if required. */ + if (parsed && parsed->client_key) { + client->client_key = crypto_pk_dup_key(parsed->client_key); + } else if (s->auth_type == REND_STEALTH_AUTH) { + /* Create private key for client. */ + crypto_pk_t *prkey = NULL; + if (!(prkey = crypto_pk_new())) { + log_warn(LD_BUG,"Error constructing client key"); goto err; - } else { - log_info(LD_CONFIG, "Parsed %d previously stored client entries.", - strmap_size(parsed_clients)); - tor_free(client_keys_str); } + if (crypto_pk_generate_key(prkey)) { + log_warn(LD_BUG,"Error generating client key"); + crypto_pk_free(prkey); + goto err; + } + if (crypto_pk_check_key(prkey) <= 0) { + log_warn(LD_BUG,"Generated client key seems invalid"); + crypto_pk_free(prkey); + goto err; + } + client->client_key = prkey; } - - /* Prepare client_keys and hostname files. */ - if (!(cfile = start_writing_to_stdio_file(cfname, - OPEN_FLAGS_REPLACE | O_TEXT, - 0600, &open_cfile))) { - log_warn(LD_CONFIG, "Could not open client_keys file %s", - escaped(cfname)); + /* Add entry to client_keys file. */ + desc_cook_out[strlen(desc_cook_out)-1] = '\0'; /* Remove newline. */ + written = tor_snprintf(buf, sizeof(buf), + "client-name %s\ndescriptor-cookie %s\n", + client->client_name, desc_cook_out); + if (written < 0) { + log_warn(LD_BUG, "Could not write client entry."); goto err; } - if (!(hfile = start_writing_to_stdio_file(fname, - OPEN_FLAGS_REPLACE | O_TEXT, - 0600, &open_hfile))) { - log_warn(LD_CONFIG, "Could not open hostname file %s", escaped(fname)); - goto err; - } - - /* Either use loaded keys for configured clients or generate new - * ones if a client is new. */ - SMARTLIST_FOREACH_BEGIN(s->clients, rend_authorized_client_t *, client) { - rend_authorized_client_t *parsed = - strmap_get(parsed_clients, client->client_name); - int written; - size_t len; - /* Copy descriptor cookie from parsed entry or create new one. */ - if (parsed) { - memcpy(client->descriptor_cookie, parsed->descriptor_cookie, - REND_DESC_COOKIE_LEN); - } else { - crypto_rand(client->descriptor_cookie, REND_DESC_COOKIE_LEN); + if (client->client_key) { + char *client_key_out = NULL; + if (crypto_pk_write_private_key_to_string(client->client_key, + &client_key_out, &len) != 0) { + log_warn(LD_BUG, "Internal error: " + "crypto_pk_write_private_key_to_string() failed."); + goto err; } - if (base64_encode(desc_cook_out, 3*REND_DESC_COOKIE_LEN_BASE64+1, - client->descriptor_cookie, - REND_DESC_COOKIE_LEN) < 0) { - log_warn(LD_BUG, "Could not base64-encode descriptor cookie."); - strmap_free(parsed_clients, rend_authorized_client_strmap_item_free); - /* Clear these here for the early error exit */ - memset(desc_cook_out, 0, sizeof(desc_cook_out)); - memset(service_id, 0, sizeof(service_id)); - return -1; + if (rend_get_service_id(client->client_key, service_id)<0) { + log_warn(LD_BUG, "Internal error: couldn't encode service ID."); + /* + * len is string length, not buffer length, but last byte is NUL + * anyway. + */ + memset(client_key_out, 0, len); + tor_free(client_key_out); + goto err; } - /* Copy client key from parsed entry or create new one if required. */ - if (parsed && parsed->client_key) { - client->client_key = crypto_pk_dup_key(parsed->client_key); - } else if (s->auth_type == REND_STEALTH_AUTH) { - /* Create private key for client. */ - crypto_pk_t *prkey = NULL; - if (!(prkey = crypto_pk_new())) { - log_warn(LD_BUG,"Error constructing client key"); - goto err; - } - if (crypto_pk_generate_key(prkey)) { - log_warn(LD_BUG,"Error generating client key"); - crypto_pk_free(prkey); - goto err; - } - if (crypto_pk_check_key(prkey) <= 0) { - log_warn(LD_BUG,"Generated client key seems invalid"); - crypto_pk_free(prkey); - goto err; - } - client->client_key = prkey; - } - /* Add entry to client_keys file. */ - desc_cook_out[strlen(desc_cook_out)-1] = '\0'; /* Remove newline. */ - written = tor_snprintf(buf, sizeof(buf), - "client-name %s\ndescriptor-cookie %s\n", - client->client_name, desc_cook_out); + written = tor_snprintf(buf + written, sizeof(buf) - written, + "client-key\n%s", client_key_out); + memset(client_key_out, 0, len); + tor_free(client_key_out); if (written < 0) { log_warn(LD_BUG, "Could not write client entry."); goto err; } - if (client->client_key) { - char *client_key_out = NULL; - if (crypto_pk_write_private_key_to_string(client->client_key, - &client_key_out, &len) != 0) { - log_warn(LD_BUG, "Internal error: " - "crypto_pk_write_private_key_to_string() failed."); - goto err; - } - if (rend_get_service_id(client->client_key, service_id)<0) { - log_warn(LD_BUG, "Internal error: couldn't encode service ID."); - /* - * len is string length, not buffer length, but last byte is NUL - * anyway. - */ - memset(client_key_out, 0, len); - tor_free(client_key_out); - goto err; - } - written = tor_snprintf(buf + written, sizeof(buf) - written, - "client-key\n%s", client_key_out); - memset(client_key_out, 0, len); - tor_free(client_key_out); - if (written < 0) { - log_warn(LD_BUG, "Could not write client entry."); - goto err; - } - } - - if (fputs(buf, cfile) < 0) { - log_warn(LD_FS, "Could not append client entry to file: %s", - strerror(errno)); - goto err; - } - - /* Add line to hostname file. */ - if (s->auth_type == REND_BASIC_AUTH) { - /* Remove == signs (newline has been removed above). */ - desc_cook_out[strlen(desc_cook_out)-2] = '\0'; - tor_snprintf(buf, sizeof(buf),"%s.onion %s # client: %s\n", - s->service_id, desc_cook_out, client->client_name); - } else { - memcpy(extended_desc_cookie, client->descriptor_cookie, - REND_DESC_COOKIE_LEN); - extended_desc_cookie[REND_DESC_COOKIE_LEN] = - ((int)s->auth_type - 1) << 4; - if (base64_encode(desc_cook_out, 3*REND_DESC_COOKIE_LEN_BASE64+1, - extended_desc_cookie, - REND_DESC_COOKIE_LEN+1) < 0) { - log_warn(LD_BUG, "Could not base64-encode descriptor cookie."); - goto err; - } - desc_cook_out[strlen(desc_cook_out)-3] = '\0'; /* Remove A= and - newline. */ - tor_snprintf(buf, sizeof(buf),"%s.onion %s # client: %s\n", - service_id, desc_cook_out, client->client_name); - } - - if (fputs(buf, hfile)<0) { - log_warn(LD_FS, "Could not append host entry to file: %s", - strerror(errno)); - goto err; - } } - SMARTLIST_FOREACH_END(client); - goto done; - err: - r = -1; - done: - tor_free(client_keys_str); - strmap_free(parsed_clients, rend_authorized_client_strmap_item_free); - if (r<0) { - /* Clear these here for the early error exit */ - /* We have to clear buf because encoded keys can get written to it */ - memset(buf, 0, sizeof(buf)); - memset(desc_cook_out, 0, sizeof(desc_cook_out)); - memset(service_id, 0, sizeof(service_id)); - memset(extended_desc_cookie, 0, sizeof(extended_desc_cookie)); - if (open_cfile) - abort_writing_to_file(open_cfile); - if (open_hfile) - abort_writing_to_file(open_hfile); - return r; + if (fputs(buf, cfile) < 0) { + log_warn(LD_FS, "Could not append client entry to file: %s", + strerror(errno)); + goto err; + } + + /* Add line to hostname file. */ + if (s->auth_type == REND_BASIC_AUTH) { + /* Remove == signs (newline has been removed above). */ + desc_cook_out[strlen(desc_cook_out)-2] = '\0'; + tor_snprintf(buf, sizeof(buf),"%s.onion %s # client: %s\n", + s->service_id, desc_cook_out, client->client_name); } else { - finish_writing_to_file(open_cfile); - finish_writing_to_file(open_hfile); + memcpy(extended_desc_cookie, client->descriptor_cookie, + REND_DESC_COOKIE_LEN); + extended_desc_cookie[REND_DESC_COOKIE_LEN] = + ((int)s->auth_type - 1) << 4; + if (base64_encode(desc_cook_out, 3*REND_DESC_COOKIE_LEN_BASE64+1, + extended_desc_cookie, + REND_DESC_COOKIE_LEN+1) < 0) { + log_warn(LD_BUG, "Could not base64-encode descriptor cookie."); + goto err; + } + desc_cook_out[strlen(desc_cook_out)-3] = '\0'; /* Remove A= and + newline. */ + tor_snprintf(buf, sizeof(buf),"%s.onion %s # client: %s\n", + service_id, desc_cook_out, client->client_name); } + if (fputs(buf, hfile)<0) { + log_warn(LD_FS, "Could not append host entry to file: %s", + strerror(errno)); + goto err; + } + } + SMARTLIST_FOREACH_END(client); + + goto done; + err: + r = -1; + done: + tor_free(client_keys_str); + strmap_free(parsed_clients, rend_authorized_client_strmap_item_free); + if (r<0) { + /* Clear these here for the early error exit */ + /* We have to clear buf because encoded keys can get written to it */ + memset(buf, 0, sizeof(buf)); + memset(desc_cook_out, 0, sizeof(desc_cook_out)); + memset(service_id, 0, sizeof(service_id)); + memset(extended_desc_cookie, 0, sizeof(extended_desc_cookie)); + if (open_cfile) + abort_writing_to_file(open_cfile); + if (open_hfile) + abort_writing_to_file(open_hfile); + return r; + } else { + finish_writing_to_file(open_cfile); + finish_writing_to_file(open_hfile); + } + /* * Clear stack buffers that held key-derived material; we do this here * for each iteration so that we don't have to put this before all of From b8d1e8e3755e6d12bd6a12cc8e91353b55a64186 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 18 Jun 2012 13:05:28 -0400 Subject: [PATCH 15/17] Refactor exit path in rend_service_load_auth_keys Now it's an orthodox "goto err/done" exit path, and it isn't some screwy thing where we stick err/done at the end of a loop and duplicate our cleanup code. --- src/or/rendservice.c | 61 +++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 37 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 69a86afb4c..22f3b272e8 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -33,7 +33,8 @@ static int intro_point_should_expire_now(rend_intro_point_t *intro, time_t now); struct rend_service_t; static int rend_service_load_keys(struct rend_service_t *s); -static int rend_service_load_auth_keys(struct rend_service_t *s); +static int rend_service_load_auth_keys(struct rend_service_t *s, + const char *hfname); /** Represents the mapping from a virtual port of a rendezvous service to * a real port on some IP. @@ -670,6 +671,7 @@ rend_service_load_keys(rend_service_t *s) " \"%s\".", s->directory); return -1; } + tor_snprintf(buf, sizeof(buf),"%s.onion\n", s->service_id); if (write_str_to_file(fname,buf,0)<0) { log_warn(LD_CONFIG, "Could not write onion address to hostname file."); @@ -680,7 +682,7 @@ rend_service_load_keys(rend_service_t *s) /* If client authorization is configured, load or generate keys. */ if (s->auth_type != REND_NO_AUTH) { - if (rend_service_load_auth_keys(s) < 0) + if (rend_service_load_auth_keys(s, fname) < 0) return -1; } @@ -688,12 +690,13 @@ rend_service_load_keys(rend_service_t *s) } /** Load and/or generate client authorization keys for the hidden service - * s. Return 0 on success, -1 on failure. */ + * s, which stores its hostname in hfname. Return 0 on success, + * -1 on failure. */ static int -rend_service_load_auth_keys(rend_service_t *s) +rend_service_load_auth_keys(rend_service_t *s, const char *hfname) { int r = 0; - char fname[512], cfname[512]; + char cfname[512]; char *client_keys_str = NULL; strmap_t *parsed_clients = strmap_new(); FILE *cfile, *hfile; @@ -731,10 +734,11 @@ rend_service_load_auth_keys(rend_service_t *s) escaped(cfname)); goto err; } - if (!(hfile = start_writing_to_stdio_file(fname, + + if (!(hfile = start_writing_to_stdio_file(hfname, OPEN_FLAGS_REPLACE | O_TEXT, 0600, &open_hfile))) { - log_warn(LD_CONFIG, "Could not open hostname file %s", escaped(fname)); + log_warn(LD_CONFIG, "Could not open hostname file %s", escaped(hfname)); goto err; } @@ -756,11 +760,7 @@ rend_service_load_auth_keys(rend_service_t *s) client->descriptor_cookie, REND_DESC_COOKIE_LEN) < 0) { log_warn(LD_BUG, "Could not base64-encode descriptor cookie."); - strmap_free(parsed_clients, rend_authorized_client_strmap_item_free); - /* Clear these here for the early error exit */ - memset(desc_cook_out, 0, sizeof(desc_cook_out)); - memset(service_id, 0, sizeof(service_id)); - return -1; + goto err; } /* Copy client key from parsed entry or create new one if required. */ if (parsed && parsed->client_key) { @@ -855,39 +855,26 @@ rend_service_load_auth_keys(rend_service_t *s) strerror(errno)); goto err; } - } - SMARTLIST_FOREACH_END(client); + } SMARTLIST_FOREACH_END(client); + + finish_writing_to_file(open_cfile); + finish_writing_to_file(open_hfile); goto done; err: r = -1; + if (open_cfile) + abort_writing_to_file(open_cfile); + if (open_hfile) + abort_writing_to_file(open_hfile); done: + /* XXXX zero these two as well */ tor_free(client_keys_str); strmap_free(parsed_clients, rend_authorized_client_strmap_item_free); - if (r<0) { - /* Clear these here for the early error exit */ - /* We have to clear buf because encoded keys can get written to it */ - memset(buf, 0, sizeof(buf)); - memset(desc_cook_out, 0, sizeof(desc_cook_out)); - memset(service_id, 0, sizeof(service_id)); - memset(extended_desc_cookie, 0, sizeof(extended_desc_cookie)); - if (open_cfile) - abort_writing_to_file(open_cfile); - if (open_hfile) - abort_writing_to_file(open_hfile); - return r; - } else { - finish_writing_to_file(open_cfile); - finish_writing_to_file(open_hfile); - } - /* - * Clear stack buffers that held key-derived material; we do this here - * for each iteration so that we don't have to put this before all of - * the early error exits (check/create directory, check service file, - * and so forth) that otherwise might have leftover key from the - * previous iteration on the stack. - */ + memset(cfname, 0, sizeof(cfname)); + + /* Clear stack buffers that held key-derived material. */ memset(buf, 0, sizeof(buf)); memset(desc_cook_out, 0, sizeof(desc_cook_out)); memset(service_id, 0, sizeof(service_id)); From 4a8eaad7efd4e24e89f969f79d9bde1874f366d1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 18 Jun 2012 13:13:53 -0400 Subject: [PATCH 16/17] Clear a couple more fields in rend_service_load_auth_keys --- src/common/util.c | 10 ++++++++++ src/common/util.h | 1 + src/or/rendservice.c | 4 +++- 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/common/util.c b/src/common/util.c index 28ecff3983..cb9a65d361 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -655,6 +655,16 @@ fast_memcmpstart(const void *mem, size_t memlen, return fast_memcmp(mem, prefix, plen); } +/** Given a nul-terminated string s, set every character before the nul + * to zero. */ +void +tor_strclear(char *s) +{ + while (*s) { + *s++ = '\0'; + } +} + /** Return a pointer to the first char of s that is not whitespace and * not a comment, or to the terminating NUL if no such character exists. */ diff --git a/src/common/util.h b/src/common/util.h index a2b196c88b..5e52292cb1 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -188,6 +188,7 @@ int strcasecmpstart(const char *s1, const char *s2) ATTR_NONNULL((1,2)); int strcmpend(const char *s1, const char *s2) ATTR_NONNULL((1,2)); int strcasecmpend(const char *s1, const char *s2) ATTR_NONNULL((1,2)); int fast_memcmpstart(const void *mem, size_t memlen, const char *prefix); +void tor_strclear(char *s); void tor_strstrip(char *s, const char *strip) ATTR_NONNULL((1,2)); long tor_parse_long(const char *s, int base, long min, diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 22f3b272e8..38268d4d2f 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -139,7 +139,9 @@ rend_authorized_client_free(rend_authorized_client_t *client) return; if (client->client_key) crypto_pk_free(client->client_key); + tor_strclear(client->client_name); tor_free(client->client_name); + memset(client->descriptor_cookie, 0, sizeof(client->descriptor_cookie)); tor_free(client); } @@ -868,7 +870,7 @@ rend_service_load_auth_keys(rend_service_t *s, const char *hfname) if (open_hfile) abort_writing_to_file(open_hfile); done: - /* XXXX zero these two as well */ + tor_strclear(client_keys_str); tor_free(client_keys_str); strmap_free(parsed_clients, rend_authorized_client_strmap_item_free); From dcb1d0a67baa35b9645b19f2cf44f8ba3d0b30d1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 18 Jun 2012 13:20:02 -0400 Subject: [PATCH 17/17] add bug number and explanation to changes/bug2385; call it a feature --- changes/bug2385 | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/changes/bug2385 b/changes/bug2385 index e7151f49eb..5d571d910f 100644 --- a/changes/bug2385 +++ b/changes/bug2385 @@ -1,5 +1,9 @@ - o Minor bugfixes: - - Clear keys and key-derived material left on the stack in rendservice.c - and rendclient.c + o Minor features (security): + - Clear keys and key-derived material left on the stack in + rendservice.c and rendclient.c. This should make us more + forward-secure against cold-boot attacks and the like. Fix for + bug 2385. + - Check return value of crypto_pk_write_private_key_to_string() in - end_service_load_keys() + end_service_load_keys(). This should make us more forward-secure + against cold-boot attacks and the like. Fix for bug 2385.