Handshake security #103
Handshake security #103
Conversation
yaelhoma
force-pushed
the
handshake_security
branch
3 times, most recently
from
62160ef to
af8eccb
Compare
src/cobo/handshake.c
Outdated
| uint64_t rand_num, uuid_num; | ||
|
|
||
| //random number file | ||
| FILE *random_file = fopen("/dev/random", "r"); |
There was a problem hiding this comment.
Suggest using linux's getrandom() call (which wraps reading of /dev/urandom), which looks to be the preferred way to do this.
src/cobo/handshake.c
Outdated
| fclose(uuid_file); | ||
| return -1; | ||
| } | ||
| if (fread(&uuid_num, sizeof(uuid_num), 1, uuid_file) != 1) { |
There was a problem hiding this comment.
A) The /proc/sys/kernel/random/uuid file returns and ASCII encoded number, but this is reading it like a binary number.
B) If we're doing a read/fread of random data we need to handle the EINTR return codes
src/cobo/handshake.c
Outdated
| time_t now = time(NULL); | ||
|
|
||
| // Combine timestamp,random number, uuid | ||
| *unique_value = now ^ rand_num ^ uuid_num; |
There was a problem hiding this comment.
We don't want to xor the data together. We're essentially xor'ing a random number with a partially-random number with a non-random number. I don't think that produces anything interesting. The goal is to make a unique number. I think we could do that by concat'ing into 128 bits (or fill in multiple 64-bit fields) with a random number, session id, and a counter.
yaelhoma
force-pushed
the
handshake_security
branch
from
24d684c to
e8201ab
Compare
src/cobo/handshake.c
Outdated
| static unsigned int saved_key_len; | ||
| static connection_info_t *saved_conninfo; | ||
| static int timeout_seconds = 0; | ||
| uint32_t local_global_countr = 0; |
There was a problem hiding this comment.
Make this a static variable: 'static uint32_t local_global_countr'
That keeps it scope limited to this file.
Also suggest a more descriptive name for what it does, i.e., unique_number_counter
src/cobo/handshake.c
Outdated
| /** | ||
| * Decrypt their packet with their random number | ||
| **/ | ||
| debug_printf("Decrypting their random number packet\n"); |
There was a problem hiding this comment.
Misleading comment and printf. This is decrypting the main handshake packet (which does contain the initial random number). But it is not the random number packet, which this comment implies.
src/cobo/handshake.c
Outdated
|
|
||
| static int compare_packets(handshake_packet_t *expected_packet, | ||
| handshake_packet_t *recvd_packet) | ||
| static int compare_packets(handshake_packet_t *expected_packet, handshake_packet_t *recvd_packet, |
There was a problem hiding this comment.
Let's break this into two functions: compare_handshake_packets() and compare_random_number_packets().
Then compare_handshake_packets() after the first exchange, but before doing the random packet exchange.
Then compare_random_number_packets() after the random packet exchange.
Two reasons for this. A) It's a common occurrence for two different spindle sessions to connect to each other and realize they are the wrong sessions (the HSHAKE_DROP_CONNECTION test below). Let's get those dropped quickly before sending the other packets. B) No reason to be signing and sending those other packets when we can do an earlier test for common cases..
| return_result = result; | ||
| goto done; | ||
| } | ||
|
|
There was a problem hiding this comment.
See comment below about splitting up compare_packet(). The compare_handshake_packet() would go here.
| //encrypt random number packet that they are expecting | ||
| random_number_packet.random_number = recvd_packet.random_number; | ||
| random_number_packet.signature = packet.signature; | ||
| result = encrypt_packet(hdata, &random_number_packet, sizeof(random_number_packet_t), &packet_buffer, &packet_buffer_size); |
There was a problem hiding this comment.
There's a memory leak on packet_buffer here. Before this commit packet_buffer was allocated and free'd once at the exit to this function. Now it's allocated twice and free'd once at the end of the function. It needs to be free'd before being passed to encryp_packet.
src/cobo/handshake.c
Outdated
| result = getrandom((void*) &rand_num->random_number,sizeof(rand_num->random_number),GRND_RANDOM|GRND_NONBLOCK); | ||
| if (result<0) { | ||
| local_errno = errno; | ||
| if (local_errno == EAGAIN){ |
There was a problem hiding this comment.
Need to handle EINTR here.
Also we don't want EAGAIN to be a fatal error (nor EINTR).
yaelhoma
force-pushed
the
handshake_security
branch
from
e8201ab to
b980f21
Compare
…acket for all sec-types.
yaelhoma
force-pushed
the
handshake_security
branch
from
b980f21 to
c460188
Compare
3 participants
Adds random number exchange to handshake protocol integrated with all sec-types.