Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup: use crypto_memzero to wipe secret keys when no longer in use #1753

Merged
merged 1 commit into from
Dec 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions toxcore/DHT.c
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,10 @@ static unsigned int bit_by_bit_cmp(const uint8_t *pk1, const uint8_t *pk2)
}

/* Shared key generations are costly, it is therefore smart to store commonly used
* ones so that they can re used later without being computed again.
* ones so that they can be re-used later without being computed again.
*
* If shared key is already in shared_keys, copy it to shared_key.
* else generate it into shared_key and copy it to shared_keys
* If a shared key is already in shared_keys, copy it to shared_key.
* Otherwise generate it into shared_key and copy it to shared_keys
*/
void get_shared_key(const Mono_Time *mono_time, Shared_Keys *shared_keys, uint8_t *shared_key,
const uint8_t *secret_key, const uint8_t *public_key)
Expand Down Expand Up @@ -1342,6 +1342,8 @@ static int getnodes(DHT *dht, IP_Port ip_port, const uint8_t *public_key, const
const int len = dht_create_packet(dht->self_public_key, shared_key, NET_PACKET_GET_NODES,
plain, sizeof(plain), data);

crypto_memzero(shared_key, sizeof(shared_key));

if (len != sizeof(data)) {
return -1;
}
Expand Down Expand Up @@ -1423,13 +1425,16 @@ static int handle_getnodes(void *object, IP_Port source, const uint8_t *packet,
plain);

if (len != CRYPTO_NODE_SIZE) {
crypto_memzero(shared_key, sizeof(shared_key));
return true;
}

sendnodes_ipv6(dht, source, packet + 1, plain, plain + CRYPTO_PUBLIC_KEY_SIZE, sizeof(uint64_t), shared_key);

ping_add(dht->ping, packet + 1, source);

crypto_memzero(shared_key, sizeof(shared_key));

return false;
}

Expand Down Expand Up @@ -1492,6 +1497,8 @@ static int handle_sendnodes_core(void *object, IP_Port source, const uint8_t *pa
1 + data_size + sizeof(uint64_t) + CRYPTO_MAC_SIZE,
plain);

crypto_memzero(shared_key, sizeof(shared_key));

if ((unsigned int)len != SIZEOF_VLA(plain)) {
return 1;
}
Expand Down Expand Up @@ -2787,6 +2794,9 @@ void kill_dht(DHT *dht)
ping_kill(dht->ping);
free(dht->friends_list);
free(dht->loaded_nodes_list);
crypto_memzero(&dht->shared_keys_recv, sizeof(dht->shared_keys_recv));
crypto_memzero(&dht->shared_keys_sent, sizeof(dht->shared_keys_sent));
crypto_memzero(dht->self_secret_key, sizeof(dht->self_secret_key));
free(dht);
}

Expand Down
6 changes: 3 additions & 3 deletions toxcore/DHT.h
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,10 @@ const uint8_t *dht_get_friend_public_key(const DHT *dht, uint32_t friend_num);
/*----------------------------------------------------------------------------------*/

/* Shared key generations are costly, it is therefore smart to store commonly used
* ones so that they can re used later without being computed again.
* ones so that they can be re-used later without being computed again.
*
* If shared key is already in shared_keys, copy it to shared_key.
* else generate it into shared_key and copy it to shared_keys
* If a shared key is already in shared_keys, copy it to shared_key.
* Otherwise generate it into shared_key and copy it to shared_keys
*/
void get_shared_key(const Mono_Time *mono_time, Shared_Keys *shared_keys, uint8_t *shared_key,
const uint8_t *secret_key, const uint8_t *public_key);
Expand Down
2 changes: 2 additions & 0 deletions toxcore/TCP_connection.c
Original file line number Diff line number Diff line change
Expand Up @@ -1495,6 +1495,8 @@ void kill_tcp_connections(TCP_Connections *tcp_c)
kill_TCP_connection(tcp_c->tcp_connections[i].connection);
}

crypto_memzero(tcp_c->self_secret_key, sizeof(tcp_c->self_secret_key));

free(tcp_c->tcp_connections);
free(tcp_c->connections);
free(tcp_c);
Expand Down
8 changes: 8 additions & 0 deletions toxcore/TCP_server.c
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,7 @@ static int handle_TCP_handshake(TCP_Secure_Connection *con, const uint8_t *data,
data + CRYPTO_PUBLIC_KEY_SIZE + CRYPTO_NONCE_SIZE, TCP_HANDSHAKE_PLAIN_SIZE + CRYPTO_MAC_SIZE, plain);

if (len != TCP_HANDSHAKE_PLAIN_SIZE) {
crypto_memzero(shared_key, sizeof(shared_key));
return -1;
}

Expand All @@ -601,15 +602,20 @@ static int handle_TCP_handshake(TCP_Secure_Connection *con, const uint8_t *data,
response + CRYPTO_NONCE_SIZE);

if (len != TCP_HANDSHAKE_PLAIN_SIZE + CRYPTO_MAC_SIZE) {
crypto_memzero(shared_key, sizeof(shared_key));
return -1;
}

JFreegman marked this conversation as resolved.
Show resolved Hide resolved
if (TCP_SERVER_HANDSHAKE_SIZE != net_send(con->sock, response, TCP_SERVER_HANDSHAKE_SIZE)) {
crypto_memzero(shared_key, sizeof(shared_key));
return -1;
}

encrypt_precompute(plain, temp_secret_key, con->shared_key);
con->status = TCP_STATUS_UNCONFIRMED;

crypto_memzero(shared_key, sizeof(shared_key));

return 1;
}

Expand Down Expand Up @@ -1475,6 +1481,8 @@ void kill_TCP_server(TCP_Server *tcp_server)

free_accepted_connection_array(tcp_server);

crypto_memzero(tcp_server->secret_key, sizeof(tcp_server->secret_key));

free(tcp_server->socks_listening);
free(tcp_server);
}
4 changes: 4 additions & 0 deletions toxcore/onion.c
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ int create_onion_path(const DHT *dht, Onion_Path *new_path, const Node_format *n
encrypt_precompute(nodes[2].public_key, random_secret_key, new_path->shared_key3);
memcpy(new_path->public_key3, random_public_key, CRYPTO_PUBLIC_KEY_SIZE);

crypto_memzero(random_secret_key, sizeof(random_secret_key));

new_path->ip_port1 = nodes[0].ip_port;
new_path->ip_port2 = nodes[1].ip_port;
new_path->ip_port3 = nodes[2].ip_port;
Expand Down Expand Up @@ -694,5 +696,7 @@ void kill_onion(Onion *onion)
networking_registerhandler(onion->net, NET_PACKET_ONION_RECV_2, nullptr, nullptr);
networking_registerhandler(onion->net, NET_PACKET_ONION_RECV_1, nullptr, nullptr);

crypto_memzero(onion->secret_symmetric_key, sizeof(onion->secret_symmetric_key));

free(onion);
}
9 changes: 9 additions & 0 deletions toxcore/ping.c
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ int32_t ping_send_request(Ping *ping, IP_Port ipp, const uint8_t *public_key)
ping_id = ping_array_add(ping->ping_array, ping->mono_time, data, sizeof(data));

if (ping_id == 0) {
crypto_memzero(shared_key, sizeof(shared_key));
return 1;
}

Expand All @@ -83,6 +84,8 @@ int32_t ping_send_request(Ping *ping, IP_Port ipp, const uint8_t *public_key)
ping_plain, sizeof(ping_plain),
pk + 1 + CRYPTO_PUBLIC_KEY_SIZE + CRYPTO_NONCE_SIZE);

crypto_memzero(shared_key, sizeof(shared_key));

if (rc != PING_PLAIN_SIZE + CRYPTO_MAC_SIZE) {
return 1;
}
Expand Down Expand Up @@ -148,10 +151,12 @@ static int handle_ping_request(void *object, IP_Port source, const uint8_t *pack
ping_plain);

if (rc != sizeof(ping_plain)) {
crypto_memzero(shared_key, sizeof(shared_key));
return 1;
}

JFreegman marked this conversation as resolved.
Show resolved Hide resolved
if (ping_plain[0] != NET_PACKET_PING_REQUEST) {
crypto_memzero(shared_key, sizeof(shared_key));
return 1;
}

Expand All @@ -161,6 +166,8 @@ static int handle_ping_request(void *object, IP_Port source, const uint8_t *pack
ping_send_response(ping, source, packet + 1, ping_id, shared_key);
ping_add(ping, packet + 1, source);

crypto_memzero(shared_key, sizeof(shared_key));

return 0;
}

Expand Down Expand Up @@ -192,6 +199,8 @@ static int handle_ping_response(void *object, IP_Port source, const uint8_t *pac
PING_PLAIN_SIZE + CRYPTO_MAC_SIZE,
ping_plain);

crypto_memzero(shared_key, sizeof(shared_key));

if (rc != sizeof(ping_plain)) {
return 1;
}
Expand Down