diff options
author | Alexandre Erwin Ittner <alexandre@ittner.com.br> | 2014-09-21 11:37:49 -0300 |
---|---|---|
committer | Alexandre Erwin Ittner <alexandre@ittner.com.br> | 2014-09-21 11:37:49 -0300 |
commit | b52eb48c15a06fe198c7c5e0d765cfef8ff4fbdf (patch) | |
tree | b203648e6df5bd825461e8c09b45a10f874d55ae /toxcore | |
parent | 70b4018069c0bbfa7ac2cf907a76a693e78e2947 (diff) |
Remove chattiness from avatar data transfers
The chatty approach for the avatar data transfer was intended as a
security feature to add explicit delays to the transfer and prevent
amplification attacks among authenticated friends. This was deemed
unnecessary in the code review and, therefore, replaced by a simpler
approach that sends all data in a single burst.
Diffstat (limited to 'toxcore')
-rw-r--r-- | toxcore/Messenger.c | 133 | ||||
-rw-r--r-- | toxcore/Messenger.h | 5 |
2 files changed, 66 insertions, 72 deletions
diff --git a/toxcore/Messenger.c b/toxcore/Messenger.c index 4f8eee6b..fb6aafa5 100644 --- a/toxcore/Messenger.c +++ b/toxcore/Messenger.c | |||
@@ -42,7 +42,7 @@ | |||
42 | static void set_friend_status(Messenger *m, int32_t friendnumber, uint8_t status); | 42 | static void set_friend_status(Messenger *m, int32_t friendnumber, uint8_t status); |
43 | static int write_cryptpacket_id(const Messenger *m, int32_t friendnumber, uint8_t packet_id, const uint8_t *data, | 43 | static int write_cryptpacket_id(const Messenger *m, int32_t friendnumber, uint8_t packet_id, const uint8_t *data, |
44 | uint32_t length, uint8_t congestion_control); | 44 | uint32_t length, uint8_t congestion_control); |
45 | static int send_avatar_data_control(const Messenger *m, const uint32_t friendnumber, uint8_t op, uint32_t bytes_received); | 45 | static int send_avatar_data_control(const Messenger *m, const uint32_t friendnumber, uint8_t op); |
46 | 46 | ||
47 | // friend_not_valid determines if the friendnumber passed is valid in the Messenger object | 47 | // friend_not_valid determines if the friendnumber passed is valid in the Messenger object |
48 | static uint8_t friend_not_valid(const Messenger *m, int32_t friendnumber) | 48 | static uint8_t friend_not_valid(const Messenger *m, int32_t friendnumber) |
@@ -672,6 +672,8 @@ int m_request_avatar_data(const Messenger *m, const int32_t friendnumber) | |||
672 | avrd = malloc(sizeof(AVATARRECEIVEDATA)); | 672 | avrd = malloc(sizeof(AVATARRECEIVEDATA)); |
673 | if (avrd == NULL) | 673 | if (avrd == NULL) |
674 | return -1; | 674 | return -1; |
675 | memset(avrd, 0, sizeof(AVATARRECEIVEDATA)); | ||
676 | avrd->started = 0; | ||
675 | m->friendlist[friendnumber].avatar_recv_data = avrd; | 677 | m->friendlist[friendnumber].avatar_recv_data = avrd; |
676 | } | 678 | } |
677 | 679 | ||
@@ -680,13 +682,12 @@ int m_request_avatar_data(const Messenger *m, const int32_t friendnumber) | |||
680 | "friendnumber == %u", friendnumber); | 682 | "friendnumber == %u", friendnumber); |
681 | } | 683 | } |
682 | 684 | ||
683 | memset(avrd, 0, sizeof(AVATARRECEIVEDATA)); | ||
684 | avrd->started = 0; | 685 | avrd->started = 0; |
685 | avrd->bytes_received = 0; | 686 | avrd->bytes_received = 0; |
686 | avrd->total_length = 0; | 687 | avrd->total_length = 0; |
687 | avrd->format = AVATARFORMAT_NONE; | 688 | avrd->format = AVATARFORMAT_NONE; |
688 | 689 | ||
689 | return send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_REQ, 0); | 690 | return send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_REQ); |
690 | } | 691 | } |
691 | 692 | ||
692 | 693 | ||
@@ -2118,18 +2119,12 @@ static int handle_status(void *object, int i, uint8_t status) | |||
2118 | * values or request data. | 2119 | * values or request data. |
2119 | */ | 2120 | */ |
2120 | static int send_avatar_data_control(const Messenger *m, const uint32_t friendnumber, | 2121 | static int send_avatar_data_control(const Messenger *m, const uint32_t friendnumber, |
2121 | uint8_t op, uint32_t bytes_received) | 2122 | uint8_t op) |
2122 | { | 2123 | { |
2123 | uint8_t data[1 + sizeof(uint32_t)]; | ||
2124 | data[0] = op; | ||
2125 | uint32_t tmp_recv = htonl(bytes_received); | ||
2126 | memcpy(data+1, &tmp_recv, sizeof(uint32_t)); | ||
2127 | |||
2128 | int ret = write_cryptpacket_id(m, friendnumber, PACKET_ID_AVATAR_DATA_CONTROL, | 2124 | int ret = write_cryptpacket_id(m, friendnumber, PACKET_ID_AVATAR_DATA_CONTROL, |
2129 | data, sizeof(data), 0); | 2125 | &op, sizeof(op), 0); |
2130 | 2126 | LOGGER_DEBUG("friendnumber = %u, op = %u, ret = %d", | |
2131 | LOGGER_DEBUG("friendnumber = %u, op = %u, bytes_received = %u, ret = %d", | 2127 | friendnumber, op, ret); |
2132 | friendnumber, op, bytes_received, ret); | ||
2133 | return (ret >= 0) ? 0 : -1; | 2128 | return (ret >= 0) ? 0 : -1; |
2134 | } | 2129 | } |
2135 | 2130 | ||
@@ -2137,11 +2132,11 @@ static int send_avatar_data_control(const Messenger *m, const uint32_t friendnum | |||
2137 | static int handle_avatar_data_control(Messenger *m, uint32_t friendnumber, | 2132 | static int handle_avatar_data_control(Messenger *m, uint32_t friendnumber, |
2138 | uint8_t *data, uint32_t data_length) | 2133 | uint8_t *data, uint32_t data_length) |
2139 | { | 2134 | { |
2140 | if (data_length != 1 + sizeof(uint32_t)) { | 2135 | if (data_length != 1) { |
2141 | LOGGER_DEBUG("Error: PACKET_ID_AVATAR_DATA_CONTROL with bad " | 2136 | LOGGER_DEBUG("Error: PACKET_ID_AVATAR_DATA_CONTROL with bad " |
2142 | "data_length = %u, friendnumber = %u", | 2137 | "data_length = %u, friendnumber = %u", |
2143 | data_length, friendnumber); | 2138 | data_length, friendnumber); |
2144 | send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR, 0); | 2139 | send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR); |
2145 | return -1; /* Error */ | 2140 | return -1; /* Error */ |
2146 | } | 2141 | } |
2147 | 2142 | ||
@@ -2149,6 +2144,25 @@ static int handle_avatar_data_control(Messenger *m, uint32_t friendnumber, | |||
2149 | 2144 | ||
2150 | switch (data[0]) { | 2145 | switch (data[0]) { |
2151 | case AVATARDATACONTROL_REQ: { | 2146 | case AVATARDATACONTROL_REQ: { |
2147 | |||
2148 | /* Check data transfer limits for this friend */ | ||
2149 | AVATARSENDDATA * const avsd = &(m->friendlist[friendnumber].avatar_send_data); | ||
2150 | if (avsd->bytes_sent >= AVATAR_DATA_TRANSFER_LIMIT) { | ||
2151 | /* User reached data limit. Check timeout */ | ||
2152 | uint64_t now = unix_time(); | ||
2153 | if (avsd->last_reset > 0 | ||
2154 | && (avsd->last_reset + AVATAR_DATA_TRANSFER_TIMEOUT < now)) { | ||
2155 | avsd->bytes_sent = 0; | ||
2156 | avsd->last_reset = now; | ||
2157 | } else { | ||
2158 | /* Friend still rate-limitted. Send an error and stops. */ | ||
2159 | LOGGER_DEBUG("Avatar data transfer limit reached. " | ||
2160 | "friendnumber = %u", friendnumber); | ||
2161 | send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR); | ||
2162 | return 0; | ||
2163 | } | ||
2164 | } | ||
2165 | |||
2152 | /* Start the transmission with a DATA_START message. Format: | 2166 | /* Start the transmission with a DATA_START message. Format: |
2153 | * uint8_t format | 2167 | * uint8_t format |
2154 | * uint8_t hash[AVATAR_HASH_LENGTH] | 2168 | * uint8_t hash[AVATAR_HASH_LENGTH] |
@@ -2164,64 +2178,46 @@ static int handle_avatar_data_control(Messenger *m, uint32_t friendnumber, | |||
2164 | memcpy(start_data + 1, m->avatar_hash, AVATAR_HASH_LENGTH); | 2178 | memcpy(start_data + 1, m->avatar_hash, AVATAR_HASH_LENGTH); |
2165 | memcpy(start_data + 1 + AVATAR_HASH_LENGTH, &avatar_len, sizeof(uint32_t)); | 2179 | memcpy(start_data + 1 + AVATAR_HASH_LENGTH, &avatar_len, sizeof(uint32_t)); |
2166 | 2180 | ||
2181 | avsd->bytes_sent += sizeof(start_data); /* For rate limit */ | ||
2182 | |||
2167 | int ret = write_cryptpacket_id(m, friendnumber, PACKET_ID_AVATAR_DATA_START, | 2183 | int ret = write_cryptpacket_id(m, friendnumber, PACKET_ID_AVATAR_DATA_START, |
2168 | start_data, sizeof(start_data), 0); | 2184 | start_data, sizeof(start_data), 0); |
2169 | if (ret >= 0) | ||
2170 | return 0; /* Ok */ | ||
2171 | 2185 | ||
2172 | return -1; | 2186 | if (ret < 0) { |
2173 | } | 2187 | /* Something went wrong, try to signal the error so the friend |
2174 | 2188 | * can clear up the state. */ | |
2175 | case AVATARDATACONTROL_MORE: { | 2189 | send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR); |
2176 | /* Format: [uint8_t op][uint32_t position] | ||
2177 | * Friend received a chunk of data and asked for more. | ||
2178 | */ | ||
2179 | uint32_t bytes_received; | ||
2180 | memcpy(&bytes_received, data+1, sizeof(uint32_t)); | ||
2181 | bytes_received = ntohl(bytes_received); | ||
2182 | |||
2183 | if (m->avatar_format == AVATARFORMAT_NONE | ||
2184 | || m->avatar_data == NULL | ||
2185 | || m->avatar_data_length == 0 | ||
2186 | || m->avatar_data_length < bytes_received) { | ||
2187 | /* Avatar changed, or position became invalid, or we | ||
2188 | * received a incorrect or malicious request. Abort. */ | ||
2189 | LOGGER_DEBUG("Aborting. AVATARDATACONTROL_MORE with bad " | ||
2190 | "offset. bytes_received = %u, friendnumber = %u", | ||
2191 | bytes_received, friendnumber); | ||
2192 | send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR, 0); | ||
2193 | return 0; | 2190 | return 0; |
2194 | } | 2191 | } |
2195 | 2192 | ||
2196 | /* Check data transfer limits for this friend */ | 2193 | /* User have no avatar data, nothing more to do. */ |
2197 | AVATARSENDDATA * const avsd = &(m->friendlist[friendnumber].avatar_send_data); | 2194 | if (m->avatar_format == AVATARFORMAT_NONE) |
2198 | if (avsd->bytes_sent >= AVATAR_DATA_TRANSFER_LIMIT) { | 2195 | return 0; |
2199 | /* User reached data limit. Check timeout */ | 2196 | |
2200 | uint64_t now = unix_time(); | 2197 | /* Send the actual avatar data. */ |
2201 | if (avsd->last_reset > 0 | 2198 | uint32_t offset = 0; |
2202 | && (avsd->last_reset + AVATAR_DATA_TRANSFER_TIMEOUT < now)) { | 2199 | while (offset < m->avatar_data_length) { |
2203 | avsd->bytes_sent = 0; | 2200 | uint32_t chunk_len = m->avatar_data_length - offset; |
2204 | avsd->last_reset = now; | 2201 | if (chunk_len > AVATAR_DATA_MAX_CHUNK_SIZE) |
2205 | } else { | 2202 | chunk_len = AVATAR_DATA_MAX_CHUNK_SIZE; |
2206 | /* Friend still rate-limitted. Send an error ad stops. */ | 2203 | |
2207 | LOGGER_DEBUG("Avatar data transfer limit reached. " | 2204 | uint8_t chunk[AVATAR_DATA_MAX_CHUNK_SIZE]; |
2208 | "friendnumber = %u", friendnumber); | 2205 | memcpy(chunk, m->avatar_data + offset, chunk_len); |
2209 | send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR, 0); | 2206 | offset += chunk_len; |
2210 | return 0; | 2207 | avsd->bytes_sent += chunk_len; /* For rate limit */ |
2208 | |||
2209 | int ret = write_cryptpacket_id(m, friendnumber, | ||
2210 | PACKET_ID_AVATAR_DATA_PUSH, | ||
2211 | chunk, chunk_len, 0); | ||
2212 | if (ret < 0) { | ||
2213 | LOGGER_DEBUG("write_cryptpacket_id failed. ret = %d, " | ||
2214 | "friendnumber = %u, offset = %u", | ||
2215 | ret, friendnumber, offset); | ||
2216 | send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR); | ||
2217 | return -1; | ||
2211 | } | 2218 | } |
2212 | } | 2219 | } |
2213 | 2220 | ||
2214 | /* Send another chunk of data */ | ||
2215 | uint32_t chunk_len = m->avatar_data_length - bytes_received; | ||
2216 | if (chunk_len > AVATAR_DATA_MAX_CHUNK_SIZE) | ||
2217 | chunk_len = AVATAR_DATA_MAX_CHUNK_SIZE; | ||
2218 | |||
2219 | uint8_t chunk[AVATAR_DATA_MAX_CHUNK_SIZE]; | ||
2220 | memcpy(chunk, m->avatar_data + bytes_received, chunk_len); | ||
2221 | avsd->bytes_sent += chunk_len; /* For rate limit */ | ||
2222 | |||
2223 | write_cryptpacket_id(m, friendnumber, PACKET_ID_AVATAR_DATA_PUSH, | ||
2224 | chunk, chunk_len, 0); | ||
2225 | return 0; | 2221 | return 0; |
2226 | } | 2222 | } |
2227 | 2223 | ||
@@ -2305,8 +2301,8 @@ static int handle_avatar_data_start(Messenger *m, uint32_t friendnumber, | |||
2305 | return 0; | 2301 | return 0; |
2306 | } | 2302 | } |
2307 | 2303 | ||
2308 | LOGGER_DEBUG("requesting more data, friendnumber = %u", friendnumber); | 2304 | /* Waits for more data to be received */ |
2309 | return send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_MORE, 0); | 2305 | return 0; |
2310 | } | 2306 | } |
2311 | 2307 | ||
2312 | 2308 | ||
@@ -2371,9 +2367,8 @@ static int handle_avatar_data_push(Messenger *m, uint32_t friendnumber, | |||
2371 | return 0; | 2367 | return 0; |
2372 | } | 2368 | } |
2373 | 2369 | ||
2374 | /* Request more data */ | 2370 | /* Waits for more data to be received */ |
2375 | return send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_MORE, | 2371 | return 0; |
2376 | avrd->bytes_received); | ||
2377 | } | 2372 | } |
2378 | 2373 | ||
2379 | 2374 | ||
diff --git a/toxcore/Messenger.h b/toxcore/Messenger.h index 499964a7..ee8aaa5f 100644 --- a/toxcore/Messenger.h +++ b/toxcore/Messenger.h | |||
@@ -117,8 +117,8 @@ enum { | |||
117 | /* If no packets are received from friend in this time interval, kill the connection. */ | 117 | /* If no packets are received from friend in this time interval, kill the connection. */ |
118 | #define FRIEND_CONNECTION_TIMEOUT (FRIEND_PING_INTERVAL * 3) | 118 | #define FRIEND_CONNECTION_TIMEOUT (FRIEND_PING_INTERVAL * 3) |
119 | 119 | ||
120 | /* Must be <= MAX_CRYPTO_DATA_SIZE */ | 120 | /* Must be < MAX_CRYPTO_DATA_SIZE */ |
121 | #define AVATAR_DATA_MAX_CHUNK_SIZE 1024 | 121 | #define AVATAR_DATA_MAX_CHUNK_SIZE (MAX_CRYPTO_DATA_SIZE-1) |
122 | 122 | ||
123 | /* Per-friend data limit for avatar data requests */ | 123 | /* Per-friend data limit for avatar data requests */ |
124 | #define AVATAR_DATA_TRANSFER_LIMIT (10*MAX_AVATAR_DATA_LENGTH) | 124 | #define AVATAR_DATA_TRANSFER_LIMIT (10*MAX_AVATAR_DATA_LENGTH) |
@@ -151,7 +151,6 @@ AVATARFORMAT; | |||
151 | */ | 151 | */ |
152 | typedef enum { | 152 | typedef enum { |
153 | AVATARDATACONTROL_REQ, | 153 | AVATARDATACONTROL_REQ, |
154 | AVATARDATACONTROL_MORE, | ||
155 | AVATARDATACONTROL_ERROR | 154 | AVATARDATACONTROL_ERROR |
156 | } | 155 | } |
157 | AVATARDATACONTROL; | 156 | AVATARDATACONTROL; |