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.

docs/Avatars.md

@@ -474,19 +474,15 @@ types.
   - Packet `PACKET_ID_AVATAR_DATA_CONTROL` have the format:
 
         PACKET_ID_AVATAR_DATA_CONTROL (54)
-        Packet data size: 5 bytes
+        Packet data size: 1 byte
-        [1: uint8_t op][1: uint32_t bytes_received]
+        [1: uint8_t op]
 
     where 'op' is a code signaling both an operation request or a status
     return, which semantics are explained bellow. The following values are
     defined:
 
         0 = AVATARDATACONTROL_REQ
-        1 = AVATARDATACONTROL_MORE
+        1 = AVATARDATACONTROL_ERROR
-        2 = AVATARDATACONTROL_ERROR
-
-    and 'bytes_received' is the number of bytes already received by the
-    client when the operation is `MORE` or zero otherwise.
 
 
   - Packet `PACKET_ID_AVATAR_DATA_START` have the following format:
@@ -503,7 +499,9 @@ types.
 
 
   - Packet `PACKET_ID_AVATAR_DATA_PUSH` have no format structure, just up
-    to `AVATAR_DATA_MAX_CHUNK_SIZE` (56) bytes of raw avatar image data.
+    to `AVATAR_DATA_MAX_CHUNK_SIZE` bytes of raw avatar image data; this
+    value is defined according to the maximum amount of data a Tox crypted
+    packet can hold.
 
 
 
@@ -513,8 +511,7 @@ from a client "B":
   - "A" must initialize its control structures and mark its data transfer
     as not yet started. Then it requests avatar data from "B" by sending a
     packet `PACKET_ID_AVATAR_DATA_CONTROL` with 'op' set to
-    `AVATARDATACONTROL_REQ`. The field 'bytes_received' must be present, but
+    `AVATARDATACONTROL_REQ`.
-    should be set to zero and is ignored in this step.
 
   - If "B" accepts this transfer, it answers by sending an
     `PACKET_ID_AVATAR_DATA_START` with the fields 'format', 'hash' and
@@ -527,42 +524,21 @@ from a client "B":
     `AVATARDATACONTROL_ERROR` or simply ignore this request. "A" must cope
     with this.
 
+    If "B" have an avatar, it sends a variable number of
+    `PACKET_ID_AVATAR_DATA_PUSH` packets with the avatar data in a single
+    shot.
+
   - Upon receiving a `PACKET_ID_AVATAR_DATA_START`, "A" checks if it
     has sent a data request to "B". If not, just ignores the packet.
 
-    If "A" really sent a request to "B", checks if the request was already
+    If "A" really requested avatar data and the format is `AVATARFORMAT_NONE`,
-    started. If true, it is an error and it just ignores the request.
+    it triggers the avatar data callback, and clears all the temporary data,
+    finishing the process. For other formats, "A" just waits for packets
+    of type `PACKET_ID_AVATAR_DATA_PUSH`.
 
-    Otherwise, "A" decodes the message data and checks if the avatar data
+  - Upon receiving a `PACKET_ID_AVATAR_DATA_PUSH`, "A" checks if it really
-    length stated in the field 'data_length' is acceptable (ie. less or
+    sent an avatar data request and if the `PACKET_ID_AVATAR_DATA_START` was
-    equal than `TOX_MAX_AVATAR_DATA_LENGTH`). If not, it replies with an
+    already received. If this conditions are valid, it checks if the total
-    `PACKET_ID_AVATAR_DATA_CONTROL` with the field 'op' set to
-    `AVATARDATACONTROL_ERROR` (or just ignores this, "A" holds no state).
-
-    If the size is acceptable, "A" marks the request as stated, stores the
-    format, hash, and data length in the local state for user "B", sets a
-    counter for the number of bytes received from the peer and replies with
-    a `PACKET_ID_AVATAR_DATA_CONTROL` with the field 'op' set to
-    `AVATARDATACONTROL_MORE` and 'bytes_received' set to zero (as no data
-    was received yet).
-
-  - Upon receiving a `PACKET_ID_AVATAR_DATA_CONTROL` with op
-    `AVATARDATACONTROL_MORE`, "B" sends an `PACKET_ID_AVATAR_DATA_PUSH`
-    with up to `AVATAR_DATA_MAX_CHUNK_SIZE` bytes of data from the avatar,
-    starting from the offset stated in the field 'bytes_received'.
-
-    If the requested offset is invalid, "B" replies with a
-    `PACKET_ID_AVATAR_DATA_CONTROL` with the field 'op' set to
-    `AVATARDATACONTROL_ERROR`.
-
-    "B" must have full control of the amount of data it sends to "A" and
-    may, at any time, abort the transfer by sending a
-    `PACKET_ID_AVATAR_DATA_CONTROL` with the field 'op' set to
-    `AVATARDATACONTROL_ERROR`. This may happens, for example, if some limit
-    was hit or a network data usage throttle enabled. A rationale for this
-    procedures is available in section "Security considerations".
-
-  - Upon receiving a `PACKET_ID_AVATAR_DATA_PUSH`, "A" checks if the total
     length of the data already stored in the receiving buffer plus the data
     present in the push packet is still less or equal than
     `TOX_MAX_AVATAR_DATA_LENGTH`. If invalid, it replies with a
@@ -582,22 +558,13 @@ from a client "B":
     triggers the avatar data callback, and clears all the temporary data,
     finishing the process.
 
-    If not all data was received, "A" requests more data by sending a
+    If not all data was received, "A" simply waits for more data.
-    `PACKET_ID_AVATAR_DATA_CONTROL` with the field 'op' set to
-    `AVATARDATACONTROL_MORE` and 'bytes_received' set to the new offset.
 
     Client "A" is always responsible for controlling the transfer and
     validating the data received. "B" don't need to keep any state for the
     protocol, have full control over the data sent and should implement
     some transfer limit for the data it sends.
 
-    This "chatty" protocol mitigates a potential amplification attack,
-    i.e., a malicious friend sending a very small data packet that causes
-    another user to send a larger amount of data.  The hash validation
-    ensures that the avatar data is correct even if "B" changed its avatar
-    data in the middle of the transfer. A rationale for this procedures is
-    available in section "Security considerations".
-
   - Any peer receiving a `PACKET_ID_AVATAR_DATA_CONTROL` with the field 'op'
     set to `AVATARDATACONTROL_ERROR` clears any existing control state and
     finishes sending or receiving data.
@@ -624,12 +591,6 @@ The present proposal mitigates this situation by:
   - Providing an alternate, smaller, message to cooperative users refresh
     avatar information when nothing has changed (`PACKET_ID_AVATAR_INFO`);
 
-  - Making the avatar data transfer chatty: The user requesting avatar data
-    can not force a peer to send large amounts of data in a single shot and
-    must request new chunks as needed. The sender will never send more that
-    1 kB of data in a single push and have ultimate control over the amount
-    of data sent in a chunk;
-
   - Having per-friend data transfer limit. As the current protocol still
     allows an user to request an infinite data stream by asking the the
     same offset of the avatar again and again, the implementation limits
@@ -640,10 +601,6 @@ The present proposal mitigates this situation by:
   - Making the requester responsible for storing partial data and state
     information;
 
-  - (currently not implemented) Treating avatar data transfers as a low
-    priority operation, handled only if no other packets are being
-    processed.
-
 Another problem present in the avatars is the possibility of a friend send
 a maliciously crafted image intended to exploit vulnerabilities in image
 decoders. Without an intermediate server to recompress and validate and

toxcore/Messenger.c

@@ -42,7 +42,7 @@
 static void set_friend_status(Messenger *m, int32_t friendnumber, uint8_t status);
 static int write_cryptpacket_id(const Messenger *m, int32_t friendnumber, uint8_t packet_id, const uint8_t *data,
                                 uint32_t length, uint8_t congestion_control);
-static int send_avatar_data_control(const Messenger *m, const uint32_t friendnumber, uint8_t op, uint32_t bytes_received);
+static int send_avatar_data_control(const Messenger *m, const uint32_t friendnumber, uint8_t op);
 
 // friend_not_valid determines if the friendnumber passed is valid in the Messenger object
 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)
         avrd = malloc(sizeof(AVATARRECEIVEDATA));
         if (avrd == NULL)
             return -1;
+        memset(avrd, 0, sizeof(AVATARRECEIVEDATA));
+        avrd->started = 0;
         m->friendlist[friendnumber].avatar_recv_data = avrd;
     }
 
@@ -680,13 +682,12 @@ int m_request_avatar_data(const Messenger *m, const int32_t friendnumber)
                      "friendnumber == %u", friendnumber);
     }
 
-    memset(avrd, 0, sizeof(AVATARRECEIVEDATA));
     avrd->started = 0;
     avrd->bytes_received = 0;
     avrd->total_length = 0;
     avrd->format = AVATARFORMAT_NONE;
 
-    return send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_REQ, 0);
+    return send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_REQ);
 }
 
 
@@ -2118,18 +2119,12 @@ static int handle_status(void *object, int i, uint8_t status)
  * values or request data.
  */
 static int send_avatar_data_control(const Messenger *m, const uint32_t friendnumber,
-    uint8_t op, uint32_t bytes_received)
+    uint8_t op)
 {
-    uint8_t data[1 + sizeof(uint32_t)];
-    data[0] = op;
-    uint32_t tmp_recv = htonl(bytes_received);
-    memcpy(data+1, &tmp_recv, sizeof(uint32_t));
-
     int ret = write_cryptpacket_id(m, friendnumber, PACKET_ID_AVATAR_DATA_CONTROL,
-                                   data, sizeof(data), 0);
+                                   &op, sizeof(op), 0);
-
+    LOGGER_DEBUG("friendnumber = %u, op = %u, ret = %d",
-    LOGGER_DEBUG("friendnumber = %u, op = %u, bytes_received = %u, ret = %d",
+        friendnumber, op, ret);
-        friendnumber, op, bytes_received, ret);
     return (ret >= 0) ? 0 : -1;
 }
 
@@ -2137,11 +2132,11 @@ static int send_avatar_data_control(const Messenger *m, const uint32_t friendnum
 static int handle_avatar_data_control(Messenger *m, uint32_t friendnumber,
     uint8_t *data, uint32_t data_length)
 {
-    if (data_length != 1 + sizeof(uint32_t)) {
+    if (data_length != 1) {
         LOGGER_DEBUG("Error: PACKET_ID_AVATAR_DATA_CONTROL with bad "
                      "data_length = %u, friendnumber = %u",
                      data_length, friendnumber);
-        send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR, 0);
+        send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR);
         return -1;  /* Error */
     }
 
@@ -2149,6 +2144,25 @@ static int handle_avatar_data_control(Messenger *m, uint32_t friendnumber,
 
     switch (data[0]) {
         case AVATARDATACONTROL_REQ: {
+
+            /* Check data transfer limits for this friend */
+            AVATARSENDDATA * const avsd = &(m->friendlist[friendnumber].avatar_send_data);
+            if (avsd->bytes_sent >= AVATAR_DATA_TRANSFER_LIMIT) {
+                /* User reached data limit. Check timeout */
+                uint64_t now = unix_time();
+                if (avsd->last_reset > 0
+                && (avsd->last_reset + AVATAR_DATA_TRANSFER_TIMEOUT < now)) {
+                    avsd->bytes_sent = 0;
+                    avsd->last_reset = now;
+                } else {
+                    /* Friend still rate-limitted. Send an error and stops. */
+                    LOGGER_DEBUG("Avatar data transfer limit reached. "
+                                 "friendnumber = %u", friendnumber);
+                    send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR);
+                    return 0;
+                }
+            }
+
             /* Start the transmission with a DATA_START message. Format:
              *  uint8_t format
              *  uint8_t hash[AVATAR_HASH_LENGTH]
@@ -2164,64 +2178,46 @@ static int handle_avatar_data_control(Messenger *m, uint32_t friendnumber,
             memcpy(start_data + 1, m->avatar_hash, AVATAR_HASH_LENGTH);
             memcpy(start_data + 1 + AVATAR_HASH_LENGTH, &avatar_len, sizeof(uint32_t));
 
+            avsd->bytes_sent += sizeof(start_data);  /* For rate limit */
+
             int ret = write_cryptpacket_id(m, friendnumber, PACKET_ID_AVATAR_DATA_START,
                                            start_data, sizeof(start_data), 0);
-            if (ret >= 0)
-                return 0; /* Ok */
 
-            return -1;
+            if (ret < 0) {
-        }
+                /* Something went wrong, try to signal the error so the friend
-
+                 * can clear up the state. */
-        case AVATARDATACONTROL_MORE: {
+                send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR);
-            /* Format:  [uint8_t op][uint32_t position]
-             * Friend received a chunk of data and asked for more.
-             */
-            uint32_t bytes_received;
-            memcpy(&bytes_received, data+1, sizeof(uint32_t));
-            bytes_received = ntohl(bytes_received);
-
-            if (m->avatar_format == AVATARFORMAT_NONE
-            || m->avatar_data == NULL
-            || m->avatar_data_length == 0
-            || m->avatar_data_length < bytes_received) {
-                /* Avatar changed, or position became invalid, or we
-                 * received a incorrect or malicious request. Abort. */
-                LOGGER_DEBUG("Aborting. AVATARDATACONTROL_MORE with bad "
-                             "offset. bytes_received = %u, friendnumber = %u",
-                             bytes_received, friendnumber);
-                send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR, 0);
                 return 0;
             }
 
-            /* Check data transfer limits for this friend */
+            /* User have no avatar data, nothing more to do. */
-            AVATARSENDDATA * const avsd = &(m->friendlist[friendnumber].avatar_send_data);
+            if (m->avatar_format == AVATARFORMAT_NONE)
-            if (avsd->bytes_sent >= AVATAR_DATA_TRANSFER_LIMIT) {
+                return 0;
-                /* User reached data limit. Check timeout */
+
-                uint64_t now = unix_time();
+            /* Send the actual avatar data. */
-                if (avsd->last_reset > 0
+            uint32_t offset = 0;
-                && (avsd->last_reset + AVATAR_DATA_TRANSFER_TIMEOUT < now)) {
+            while (offset < m->avatar_data_length) {
-                    avsd->bytes_sent = 0;
+                uint32_t chunk_len = m->avatar_data_length - offset;
-                    avsd->last_reset = now;
+                if (chunk_len > AVATAR_DATA_MAX_CHUNK_SIZE)
-                } else {
+                    chunk_len = AVATAR_DATA_MAX_CHUNK_SIZE;
-                    /* Friend still rate-limitted. Send an error ad stops. */
+
-                    LOGGER_DEBUG("Avatar data transfer limit reached. "
+                uint8_t chunk[AVATAR_DATA_MAX_CHUNK_SIZE];
-                                 "friendnumber = %u", friendnumber);
+                memcpy(chunk, m->avatar_data + offset, chunk_len);
-                    send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR, 0);
+                offset += chunk_len;
-                    return 0;
+                avsd->bytes_sent += chunk_len;  /* For rate limit */
+
+                int ret = write_cryptpacket_id(m, friendnumber,
+                                               PACKET_ID_AVATAR_DATA_PUSH,
+                                               chunk, chunk_len, 0);
+                if (ret < 0) {
+                    LOGGER_DEBUG("write_cryptpacket_id failed. ret = %d, "
+                                 "friendnumber = %u, offset = %u",
+                                 ret, friendnumber, offset);
+                    send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR);
+                    return -1;
                 }
             }
 
-            /* Send another chunk of data */
-            uint32_t chunk_len = m->avatar_data_length - bytes_received;
-            if (chunk_len > AVATAR_DATA_MAX_CHUNK_SIZE)
-                chunk_len = AVATAR_DATA_MAX_CHUNK_SIZE;
-
-            uint8_t chunk[AVATAR_DATA_MAX_CHUNK_SIZE];
-            memcpy(chunk, m->avatar_data + bytes_received, chunk_len);
-            avsd->bytes_sent += chunk_len;  /* For rate limit */
-
-            write_cryptpacket_id(m, friendnumber, PACKET_ID_AVATAR_DATA_PUSH,
-                                chunk, chunk_len, 0);
             return 0;
         }
 
@@ -2305,8 +2301,8 @@ static int handle_avatar_data_start(Messenger *m, uint32_t friendnumber,
         return 0;
     }
 
-    LOGGER_DEBUG("requesting more data, friendnumber = %u", friendnumber);
+    /* Waits for more data to be received */
-    return send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_MORE, 0);
+    return 0;
 }
 
 
@@ -2371,9 +2367,8 @@ static int handle_avatar_data_push(Messenger *m, uint32_t friendnumber,
         return 0;
     }
 
-    /* Request more data */
+    /* Waits for more data to be received */
-    return send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_MORE,
+    return 0;
-                                    avrd->bytes_received);
 }
 
 

toxcore/Messenger.h

@@ -117,8 +117,8 @@ enum {
 /* If no packets are received from friend in this time interval, kill the connection. */
 #define FRIEND_CONNECTION_TIMEOUT (FRIEND_PING_INTERVAL * 3)
 
-/* Must be <= MAX_CRYPTO_DATA_SIZE */
+/* Must be < MAX_CRYPTO_DATA_SIZE */
-#define AVATAR_DATA_MAX_CHUNK_SIZE 1024
+#define AVATAR_DATA_MAX_CHUNK_SIZE (MAX_CRYPTO_DATA_SIZE-1)
 
 /* Per-friend data limit for avatar data requests */
 #define AVATAR_DATA_TRANSFER_LIMIT  (10*MAX_AVATAR_DATA_LENGTH)
@@ -151,7 +151,6 @@ AVATARFORMAT;
  */
 typedef enum {
     AVATARDATACONTROL_REQ,
-    AVATARDATACONTROL_MORE,
     AVATARDATACONTROL_ERROR
 }
 AVATARDATACONTROL;