From e4f66475d8b47ee3b289fdd75bbbce3230949eed Mon Sep 17 00:00:00 2001 From: Alexandre Erwin Ittner Date: Sat, 30 Aug 2014 16:43:07 -0300 Subject: Add support for user avatars in the core protocol Add a protocol and the APIs to straightforwardly support user avatars in client applications. The protocol is designed to transfer avatars in background, between friends only, and minimize network load by providing a lightweight avatar notification for local cache validation. Strict safeguards are imposed to avoid damage from non-cooperative or malicious users and to limit network usage. The complete documentation is available in docs/Avatars.md and sample code is available in testing/test_avatars.c. Code and documentation are released under the GNU GPLv3 or later, as described in the file COPYING. --- docs/Avatars.md | 658 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 658 insertions(+) create mode 100644 docs/Avatars.md (limited to 'docs') diff --git a/docs/Avatars.md b/docs/Avatars.md new file mode 100644 index 00000000..bd4046d4 --- /dev/null +++ b/docs/Avatars.md @@ -0,0 +1,658 @@ +# User avatars in Tox + + + +## Introduction and rationale + +User avatars are small icons or images used to identify users in the friend +list; they exists in virtually all VoIP and IM protocols and provide an easy +way to an user identify another in the friend list. + +This document describes the implementation of avatars in the Tox protocol, +according to the following design considerations: + + - Avatars are handled as private information, ie., only exchanged over + Tox encrypted channels among previously authenticated friends; + + - The library treats all images as blobs and does not interpret or + understands image formats, only ensures that the avatar data sent by + an user is correctly received by the other. The client application is + responsible for validating, decoding, resizing, and presenting the + image to the user. + + - There is a strict limit of 16 KiB to the avatar raw data size -- this + seems suitable for practical use as, for example, the raw data of an + uncompressed 64 x 64 pixels 24 bpp RGB bitmap is 12288 bytes long; the + data limit provides enough space for larger bitmaps if the usual + compressed formats are used. + + **Notice:** As designed, this limit can be changed in the future without + breaking the protocol compatibility, but clients using the original + limit will reject larger avatars; + + - The protocol MUST provide means to allow caching and avoid unnecessary + data transfers; + + - Avatars are transfered between clients in a background operation; + + - Avatars are served in a "best effort" basis, without breaking clients + who do not support them; + + - The protocol MUST resist to malicious users; + + - The protocol MUST work with both UDP and TCP networks. + + +The Single Tox Standard Draft v.0.1.0 recommends implementing avatars as +a purely client-side feature through a procedure that can be summarized as +sending a specially named file as a file transfer request and accepting +it silently. This procedure can be improved to provide the previously stated +design considerations, but this requires a higher integration with the core +protocol. Moving this feature to the core protocol also: + + - Provides a simpler and cleaner interfaces for client applications; + + - Hides protocol complexities from the client; + + - Avoids code duplication and ad-hoc protocols in the clients; + + - Avoids incompatibility between client implementations; + + - Allows important optimizations such as lightweight notification of + removed and updated avatars; + + - Plays well with cache schemes; + + - Makes avatar transfer an essentially background operation. + + + + + + +## High level description + +The avatar exchange is implemented with the following new elements in the +Tox protocol. This is a very high level description and the usage patterns +expected from client applications are described in Section "Using Avatars +in Client Applications" and a low level protocol description is available +in Section "Internal Protocol Description". + + - **Avatar Information Notifications** are events which may be sent by + an user to another anytime, but are usually sent after one of them + connects to the network, changes his avatar, or in reply to an **avatar + information request**. They are delivered by a very lightweight message + but with information enough to allow an user to validate or discard an + avatar from the local cache and decide if is interesting to request the + avatar data from the peer. + + This event contain two data fields: (1) the image format and (2) the + cryptographic hash of the actual image data. Image format may be NONE + (for users who have no avatar or removed their avatars), JPEG, PNG, or + GIF. The cryptographic hash is intended to be compared with the hash o + the currently cached avatar (if any) and check if it stills up to date. + + - **Avatar Information Requests** are very lightweight messages sent by an + user asking for an **avatar information notification**. They may be sent + as part of the login process or when the client thinks the currently + cached avatar is outdated. The receiver may or may not answer to this + request. This message contains no data fields; + + - An **Avatar Data Request** is sent by an user asking another for his + complete avatar data. It is sent only when the requesting user decides + the avatar do not exists in the local cache or is outdated. The receiver + may or may not answer to this request. This message contains no data + fields. + + - An **Avatar Data Notification** is an event signaling the client that + the complete avatar image data of another user is available. The actual + data transfer is implemented using several data and control messages, + but the details are hidden from the client applications. This event can + only arrive in reply to an **avatar data request**. + + This event contains three data fields: (1) the image format, (2) the + cryptographic hash of the image data, and (3) the raw image data. If the + image format is NONE (i.e. no avatar) the hash is zeroed and the image + data is empty. The raw image data is locally validated and ensured to + match the hash (the event is **not** triggered otherwise). + + + + + +## API + +To implement this feature, the following public symbols were added. The +complete API documentation is available in `tox.h`. + + +``` +#define TOX_MAX_AVATAR_DATA_LENGTH 16384 +#define TOX_AVATAR_HASH_LENGTH 32 + + +/* Data formats for user avatar images */ +typedef enum { + TOX_AVATARFORMAT_NONE, + TOX_AVATARFORMAT_JPEG, + TOX_AVATARFORMAT_PNG, + TOX_AVATARFORMAT_GIF +} +TOX_AVATARFORMAT; + + + +/* Set the user avatar image data. */ +int tox_set_avatar(Tox *tox, uint8_t format, const uint8_t *data, uint32_t length); + +/* Get avatar data from the current user. */ +int tox_get_self_avatar(const Tox *tox, uint8_t *format, uint8_t *buf, uint32_t *length, uint32_t maxlen, uint8_t *hash); + +/* Generates a cryptographic hash of the given avatar data. */ +int tox_avatar_hash(const Tox *tox, uint8_t *hash, const uint8_t *data, const uint32_t datalen); + +/* Request avatar information from a friend. */ +int tox_request_avatar_info(const Tox *tox, const int32_t friendnumber); + +/* Send an unrequested avatar information to a friend. */ +int tox_send_avatar_info(Tox *tox, const int32_t friendnumber); + +/* Request the avatar data from a friend. */ +int tox_request_avatar_data(const Tox *tox, const int32_t friendnumber); + +/* Set the callback function for avatar data. */ +void tox_callback_avatar_info(Tox *tox, void (*function)(Tox *tox, int32_t, uint8_t, uint8_t*, void *), void *userdata); + +/* Set the callback function for avatar data. */ +void tox_callback_avatar_data(Tox *tox, void (*function)(Tox *tox, int32_t, uint8_t, uint8_t*, uint8_t*, uint32_t, void *), void *userdata); +``` + + + + +## Using Avatars in Client Applications + + +### General recommendations + + - Clients MUST NOT imply the availability of avatars in other users. + Avatars are an optional feature and not all users and clients may + support them; + + - Clients MUST NOT block waiting for avatar information and avatar data + packets; + + - Clients MUST treat avatar data as insecure and potentially malicious; + For example, users may accidentally use corrupted images as avatars, + a malicious user may send a specially crafted image to exploit a know + vulnerability in an image decoding library, etc. It is recommended to + handle the avatar image data in the same way as an image downloaded + from an unknown Internet source; + + - The peers MUST NOT assume any coupling between the operations of + receiving an avatar information packet, sending unrequested avatar + information packets, requesting avatar data, or receiving avatar data. + + For example, the following situations are valid: + + * A text-mode client may send avatars to other users, but never + request them; + + * A client may not understand a particular image format and ignore + avatars using it, but request and handle other formats; + + - Clients SHOULD implement a local cache of avatars and do not request + avatar data from other peers unless necessary; + + - When an avatar information is received, the client should delete the + avatar if the new avatar format is NONE or compare the hash received + from the peer with the hash of the currently cached avatar. If they + differ, send an avatar data request; + + - If the cached avatar is older than a given threshold, the client may + also send an avatar info request to that friend once he is online and + mark the avatar as updated *before* any avatar information is received + (to not spam the peer with such requests); + + - When an avatar data notification is received, the client must update + the cached avatar with the new one; + + - Clients should resize or crop the image to the way it better adapts + to the client user interface; + + - If the user already have an avatar defined in the client configuration, + it must be set before connecting to the network to avoid spurious avatar + change notifications and unnecessary data transfers. + + - If no avatar data is available for a given friend, the client should + show a placeholder image. + + + +### Interoperability and sharing avatars among different clients + +**This section is a tentative recommendation of how clients should store +avatars to ensure local interoperability and should be revised if this +code is accepted into Tox core.** + +It is desirable that the user avatar and the cached friends avatars could be +shared among different Tox clients in the same system, in the spirit of the +proposed Single Tox Standard. This not only makes switching from one client +to another easier, but also minimizes the need of data transfers, as avatars +already downloaded by other clients can be reused. + +Given the Tox data directory described in STS Draft v0.1.0: + + - The user avatar is stored in a file named "avatar.ext", where "ext" is + "jpg", "png", or "gif", according to the image format. Clients should + keep just one of these files, with the data of the last avatar set by + the user. If the user have no avatar, no such files should be kept in + the data directory; + + - Friends avatars are stored in a directory called "avatars" and named + as "xxxxx.ext", where "xxxxx" is the complete client id encoded as an + uppercase hexadecimal string and "ext" is "jpg", "png", or "gif", + according to the image format. Clients should keep just one of these + files per friend, with the data received from the last avatar data + notification. No file should be kept for an user who have no avatar. + + **To be discussed:** User keys are usually presented in Tox clients as + upper case strings, but lower case file names are more usual. + + +Example for Linux and other Unix systems, assuming an user called "gildor": + + Tox data directory: /home/gildor/.config/tox/ + Tox data file: /home/gildor/.config/tox/data + Gildor's avatar: /home/gildor/.config/tox/avatar.jpg + Avatar data dir: /home/gildor/.config/tox/avatars/ + Elrond's avatar: /home/gildor/.config/tox/avatars/43656C65627269616E20646F6E277420546F782E426164206D656D6F72696573.jpg + Elladan's avatar: /home/gildor/.config/tox/avatars/49486174655768656E48756D616E735468696E6B49416D4D7942726F74686572.gif + Elrohir's avatar /home/gildor/.config/tox/avatars/726568746F7242794D6D41496B6E696854736E616D75486E6568576574614849.jpg + Arwen's avatar: /home/gildor/.config/tox/avatars/53686520746F6F6B20476C6F7266696E64656C277320706C6163652068657265.png + Lindir's avatar: /home/gildor/.config/tox/avatars/417070735772697474656E42794D6F7274616C734C6F6F6B54686553616D652E.gif + +This recommendation is partially implemented by "testing/test_avatars.c". + + + + + +### Common operations + +These are minimal examples of how perform common operations with avatar +functions. For a complete, working, example, see `testing/test_avatars.c`. + + +#### Setting an avatar for the current user + +In this example `load_data_file` is just an hypothetical function that loads +data from a file into the buffer and sets the length accordingly. + + uint8_t buf[TOX_MAX_AVATAR_DATA_LENGTH]; + uint32_t len; + + if (load_data_file("avatar.png", buf, &len) == 0) + if (tox_set_avatar(tox, TOX_AVATARFORMAT_PNG, buf, len) != 0) + fprintf(stderr, "Failed to set avatar.\n"); + +If the user is connected, this function will also notify all connected +friends about the avatar change. + +If the user already have an avatar defined in the client configuration, it +must be set before connecting to the network to avoid spurious avatar change +notifications and unnecessary data transfers. + + + + +#### Removing the avatar from the current user + +To remove an avatar, an application must set it to `TOX_AVATARFORMAT_NONE`. + + tox_set_avatar(tox, TOX_AVATARFORMAT_NONE, NULL, 0); + +If the user is connected, this function will also notify all connected +friends about the avatar change. + + + + + +#### Receiving avatar information from friends + +All avatar information is passed to a callback function with the prototype: + + void function(Tox *tox, int32_t friendnumber, uint8_t format, + uint8_t *hash, uint8_t *data, uint32_t datalen, void *userdata) + +As in this example: + + static void avatar_info_cb(Tox *tox, int32_t friendnumber, uint8_t format, + uint8_t *hash, void *userdata) + { + printf("Receiving avatar information from friend %d. Format = %d\n", + friendnumber, format); + printf("Data hash: "); + hex_printf(hash, TOX_AVATAR_HASH_LENGTH); /* Hypothetical function */ + printf("\n"); + } + +And, somewhere in the Tox initialization calls, set if as the callback to be +triggered when an avatar information event arrives: + + tox_callback_avatar_info(tox, avatar_info_cb, NULL); + + +A typical client will test the currently cached avatar against the hash given +in the avatar information event and, if needed, request the avatar data. + + + +#### Receiving avatar data from friends + +Avatar data events are only delivered in reply of avatar data requests which +**should** only be sent after getting the user avatar information (format +and hash) from an avatar information event and checking it against a local +cache. + +For this, an application must define an avatar information callback which +checks the local avatar cache and emits an avatar data request if necessary: + + static void avatar_info_cb(Tox *tox, int32_t friendnumber, uint8_t format, + uint8_t *hash, void *userdata) + { + printf("Receiving avatar information from friend %d. Format = %d\n", + friendnumber, format); + if (format = TOX_AVATARFORMAT_NONE) { + /* User have no avatar or removed the avatar */ + delete_avatar_from_cache(tox, friendnumber); + } else { + /* Use the received hash to check if the cached avatar is + still updated. */ + if (!is_user_cached_avatar_updated(tox, friendnumber, hash)) { + /* User avatar is outdated, send data request */ + tox_request_avatar_data(tox, friendnumber); + } + } + } + + +Then define an avatar data callback to store the received data in the local +cache: + + static void avatar_data_cb(Tox *tox, int32_t friendnumber, uint8_t format, + uint8_t *hash, uint8_t *data, uint32_t datalen, void *userdata) + { + if (format = TOX_AVATARFORMAT_NONE) { + /* User have no avatar or removed the avatar */ + delete_avatar_from_cache(tox, friendnumber); + } else { + save_avatar_data_to_cache(tox, friendnumber, format, hash, + data, datalen); + } + } + + +And, finally, register both callbacks somewhere in the Tox initialization +calls: + + tox_callback_avatar_info(tox, avatar_info_cb, NULL); + tox_callback_avatar_data(tox, avatar_data_cb, NULL); + + +In the previous examples, implementation of the functions to check, store +and retrieve data from the cache were omitted for brevity. These functions +will also need to get the friend client ID (public key) from they friend +number and, usually, convert it from a byte string to a hexadecimal +string. A complete, yet more complex, example is available in the file +`testing/test_avatars.c`. + + + + + + + + + + + +## Internal Protocol Description + +### New packet types + +The avatar transfer protocol adds the following new packet types and ids: + + PACKET_ID_AVATAR_INFO_REQ = 52 + PACKET_ID_AVATAR_INFO = 53 + PACKET_ID_AVATAR_DATA_CONTROL = 54 + PACKET_ID_AVATAR_DATA_START = 55 + PACKET_ID_AVATAR_DATA_PUSH = 56 + + + + +### Requesting avatar information + +To request avatar information, an user must send a packet of type +`PACKET_ID_AVATAR_INFO_REQ`. This packet have no data fields. Upon +receiving this packet, a client which supports avatars should answer with +a `PACKET_ID_AVATAR_INFO`. The sender must accept that the friend may +not answer at all. + + + + +### Receiving avatar information + +Avatar information arrives in a packet of type `PACKET_ID_AVATAR_INFO` with +the following structure: + + PACKET_ID_AVATAR_INFO (53) + Packet data size: 33 bytes + [1: uint8_t format][32: uint8_t hash] + +Where 'format' is the image data format, one of the following: + + 0 = AVATARFORMAT_NONE (no avatar set) + 1 = AVATARFORMAT_JPEG + 2 = AVATARFORMAT_PNG + 3 = AVATARFORMAT_GIF + +and 'hash' is the SHA-256 message digest of the avatar data. + +This packet may be sent at any time and no previous request is required. +Clients should send this packet upon connection or when a friend +connects, in the same way Tox sends name, status and action information. + + + + + +### Requesting avatar data + +Transmission of avatar data is a multi-step procedure using three new packet +types. + + - Packet `PACKET_ID_AVATAR_DATA_CONTROL` have the format: + + PACKET_ID_AVATAR_DATA_CONTROL (54) + Packet data size: 5 bytes + [1: uint8_t op][1: uint32_t bytes_received] + + 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 + 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: + + PACKET_ID_AVATAR_DATA_START (55) + Packet data size: 37 bytes + [1: uint8_t format][32: uint8_t hash][1: uint32_t data_length] + + + where 'format' is the image format, with the same values accepted for + the field 'format' in packet type `PACKET_ID_AVATAR_INFO`, 'hash' is + the SHA-256 cryptographic hash of the avatar raw data and 'data_length' + is the total number of bytes the raw avatar data. + + + - 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. + + + +The following procedure assumes that a client "A" is requesting avatar data +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 + 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 + 'data_length' set to the respective values from the current avatar. + If "B" have no avatar set, 'format' must be `AVATARFORMAT_NONE`, 'hash' + must be zeroed and 'data_length' must be zero. + + If "B" does not accept sending the avatar, it may send a packet + `PACKET_ID_AVATAR_DATA_CONTROL` with the field 'op' set to + `AVATARDATACONTROL_ERROR` or simply ignore this request. "A" must cope + with this. + + - 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 + started. If true, it is an error and it just ignores the request. + + Otherwise, "A" decodes the message data and checks if the avatar data + length stated in the field 'data_length' is acceptable (ie. less or + equal than `TOX_MAX_AVATAR_DATA_LENGTH`). If not, it replies with an + `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 + `PACKET_ID_AVATAR_DATA_CONTROL` with the field 'op' set to + `AVATARDATACONTROL_ERROR`. + + If valid, "A" updates the 'bytes_received' counter and concatenates the + newly arrived data to the buffer. + + The "A" checks if all the data was already received by comparing the + counter 'bytes_received' with the field 'total_length'. If they are + equal, "A" takes a SHA-256 hash of the data and compares it with the + hash stored in the field 'hash' received from the first + `PACKET_ID_AVATAR_DATA_START`. + + If the hashes match, the avatar data was correctly received and "A" + 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 + `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. + + + + + +## Security considerations + +The major security implication of background data transfers of large objects, +like avatars, is the possibility of exhausting the network resources from a +client. This problem is exacerbated when there is the possibility of an +amplification attack as happens, for example, when sending a very small +avatar request message will force the user to reply with a larger avatar +data message. + +The present proposal mitigates this situation by: + + - Only transferring data between previously authenticated friends; + + - Enforcing strict limits on the avatar data size; + + - 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 + the amount of data a single user can request for some time. For now, + the library will not allow an user to request more than + `10*TOX_MAX_AVATAR_DATA_LENGTH` in less than 20 minutes; + + - 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 +convert the images to neutral formats, the client applications must handle +this situation by themselves using stable and secure image libraries and +imposing limits on the maximum amount of system resources the decoding +process can take. Images coming from Tox friends must be treated in the same +way as images coming from random Internet sources. -- cgit v1.2.3 From 70b4018069c0bbfa7ac2cf907a76a693e78e2947 Mon Sep 17 00:00:00 2001 From: Alexandre Erwin Ittner Date: Sun, 21 Sep 2014 10:25:46 -0300 Subject: Remove support for avatar image formats other than PNG Support for other formats was deemed unnecessary in the code review and therefore removed. The value for the constant TOX_AVATARFORMAT_PNG is now set in stone; if the other formats become needed again in the future, this commit shall be reverted and the enum values reordered to keep compatibility. --- docs/Avatars.md | 44 ++++++++++++++++++++------------------------ testing/test_avatars.c | 6 ++---- toxcore/Messenger.h | 4 +--- toxcore/tox.h | 4 +--- 4 files changed, 24 insertions(+), 34 deletions(-) (limited to 'docs') diff --git a/docs/Avatars.md b/docs/Avatars.md index bd4046d4..f0d7a1eb 100644 --- a/docs/Avatars.md +++ b/docs/Avatars.md @@ -88,9 +88,9 @@ in Section "Internal Protocol Description". This event contain two data fields: (1) the image format and (2) the cryptographic hash of the actual image data. Image format may be NONE - (for users who have no avatar or removed their avatars), JPEG, PNG, or - GIF. The cryptographic hash is intended to be compared with the hash o - the currently cached avatar (if any) and check if it stills up to date. + (for users who have no avatar or removed their avatars) or PNG. The + cryptographic hash is intended to be compared with the hash o the + currently cached avatar (if any) and check if it stills up to date. - **Avatar Information Requests** are very lightweight messages sent by an user asking for an **avatar information notification**. They may be sent @@ -134,9 +134,7 @@ complete API documentation is available in `tox.h`. /* Data formats for user avatar images */ typedef enum { TOX_AVATARFORMAT_NONE, - TOX_AVATARFORMAT_JPEG, - TOX_AVATARFORMAT_PNG, - TOX_AVATARFORMAT_GIF + TOX_AVATARFORMAT_PNG } TOX_AVATARFORMAT; @@ -243,18 +241,18 @@ already downloaded by other clients can be reused. Given the Tox data directory described in STS Draft v0.1.0: - - The user avatar is stored in a file named "avatar.ext", where "ext" is - "jpg", "png", or "gif", according to the image format. Clients should - keep just one of these files, with the data of the last avatar set by - the user. If the user have no avatar, no such files should be kept in - the data directory; + - The user avatar is stored in a file named "avatar.png". As more formats + may be used in the future, another extensions are reserved and clients + should keep just one file named "avatar.*", with the data of the last + avatar set by the user. If the user have no avatar, no such files should + be kept in the data directory; - Friends avatars are stored in a directory called "avatars" and named - as "xxxxx.ext", where "xxxxx" is the complete client id encoded as an - uppercase hexadecimal string and "ext" is "jpg", "png", or "gif", - according to the image format. Clients should keep just one of these - files per friend, with the data received from the last avatar data - notification. No file should be kept for an user who have no avatar. + as "xxxxx.png", where "xxxxx" is the complete client id encoded as an + uppercase hexadecimal string and "png" is the extension for the PNG + avatar. As new image formats may be used in the future, clients should + ensure no other file "xxxxx.*" exists. No file should be kept for an user + who have no avatar. **To be discussed:** User keys are usually presented in Tox clients as upper case strings, but lower case file names are more usual. @@ -264,13 +262,13 @@ Example for Linux and other Unix systems, assuming an user called "gildor": Tox data directory: /home/gildor/.config/tox/ Tox data file: /home/gildor/.config/tox/data - Gildor's avatar: /home/gildor/.config/tox/avatar.jpg + Gildor's avatar: /home/gildor/.config/tox/avatar.png Avatar data dir: /home/gildor/.config/tox/avatars/ - Elrond's avatar: /home/gildor/.config/tox/avatars/43656C65627269616E20646F6E277420546F782E426164206D656D6F72696573.jpg - Elladan's avatar: /home/gildor/.config/tox/avatars/49486174655768656E48756D616E735468696E6B49416D4D7942726F74686572.gif - Elrohir's avatar /home/gildor/.config/tox/avatars/726568746F7242794D6D41496B6E696854736E616D75486E6568576574614849.jpg + Elrond's avatar: /home/gildor/.config/tox/avatars/43656C65627269616E20646F6E277420546F782E426164206D656D6F72696573.png + Elladan's avatar: /home/gildor/.config/tox/avatars/49486174655768656E48756D616E735468696E6B49416D4D7942726F74686572.png + Elrohir's avatar /home/gildor/.config/tox/avatars/726568746F7242794D6D41496B6E696854736E616D75486E6568576574614849.png Arwen's avatar: /home/gildor/.config/tox/avatars/53686520746F6F6B20476C6F7266696E64656C277320706C6163652068657265.png - Lindir's avatar: /home/gildor/.config/tox/avatars/417070735772697474656E42794D6F7274616C734C6F6F6B54686553616D652E.gif + Lindir's avatar: /home/gildor/.config/tox/avatars/417070735772697474656E42794D6F7274616C734C6F6F6B54686553616D652E.png This recommendation is partially implemented by "testing/test_avatars.c". @@ -456,9 +454,7 @@ the following structure: Where 'format' is the image data format, one of the following: 0 = AVATARFORMAT_NONE (no avatar set) - 1 = AVATARFORMAT_JPEG - 2 = AVATARFORMAT_PNG - 3 = AVATARFORMAT_GIF + 1 = AVATARFORMAT_PNG and 'hash' is the SHA-256 message digest of the avatar data. diff --git a/testing/test_avatars.c b/testing/test_avatars.c index 1940d369..124b67c7 100644 --- a/testing/test_avatars.c +++ b/testing/test_avatars.c @@ -17,8 +17,8 @@ * * Data dir MAY have: * - * - A file named avatar.png, avatar.jpg, or avatar.gif. If given, the - * bot will publish it. Otherwhise, no avatar will be set. + * - A file named avatar.png. If given, the bot will publish it. Otherwise, + * no avatar will be set. * * - A directory named "avatars" with the currently cached avatars. * @@ -87,8 +87,6 @@ typedef struct { static const def_avatar_name_t def_avatar_names[] = { /* In order of preference */ { TOX_AVATARFORMAT_PNG, "png", "avatar.png" }, - { TOX_AVATARFORMAT_JPEG, "jpg", "avatar.jpg" }, - { TOX_AVATARFORMAT_GIF, "gif", "avatar.gif" }, { TOX_AVATARFORMAT_NONE, NULL, NULL }, /* Must be the last one */ }; diff --git a/toxcore/Messenger.h b/toxcore/Messenger.h index 33d40f60..499964a7 100644 --- a/toxcore/Messenger.h +++ b/toxcore/Messenger.h @@ -142,9 +142,7 @@ USERSTATUS; */ typedef enum { AVATARFORMAT_NONE, - AVATARFORMAT_JPEG, - AVATARFORMAT_PNG, - AVATARFORMAT_GIF + AVATARFORMAT_PNG } AVATARFORMAT; diff --git a/toxcore/tox.h b/toxcore/tox.h index df348659..f4527785 100644 --- a/toxcore/tox.h +++ b/toxcore/tox.h @@ -75,9 +75,7 @@ TOX_USERSTATUS; */ typedef enum { TOX_AVATARFORMAT_NONE, - TOX_AVATARFORMAT_JPEG, - TOX_AVATARFORMAT_PNG, - TOX_AVATARFORMAT_GIF + TOX_AVATARFORMAT_PNG } TOX_AVATARFORMAT; -- cgit v1.2.3 From b52eb48c15a06fe198c7c5e0d765cfef8ff4fbdf Mon Sep 17 00:00:00 2001 From: Alexandre Erwin Ittner Date: Sun, 21 Sep 2014 11:37:49 -0300 Subject: 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 | 81 ++++++++------------------------ toxcore/Messenger.c | 133 +++++++++++++++++++++++++--------------------------- toxcore/Messenger.h | 5 +- 3 files changed, 85 insertions(+), 134 deletions(-) (limited to 'docs') diff --git a/docs/Avatars.md b/docs/Avatars.md index f0d7a1eb..97b65c10 100644 --- a/docs/Avatars.md +++ b/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 - [1: uint8_t op][1: uint32_t bytes_received] + Packet data size: 1 byte + [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 - 2 = AVATARDATACONTROL_ERROR - - and 'bytes_received' is the number of bytes already received by the - client when the operation is `MORE` or zero otherwise. + 1 = AVATARDATACONTROL_ERROR - 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 - should be set to zero and is ignored in this step. + `AVATARDATACONTROL_REQ`. - 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 - started. If true, it is an error and it just ignores the request. - - Otherwise, "A" decodes the message data and checks if the avatar data - length stated in the field 'data_length' is acceptable (ie. less or - equal than `TOX_MAX_AVATAR_DATA_LENGTH`). If not, it replies with an - `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 "A" really requested avatar data and the format is `AVATARFORMAT_NONE`, + 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`. - 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 + - Upon receiving a `PACKET_ID_AVATAR_DATA_PUSH`, "A" checks if it really + sent an avatar data request and if the `PACKET_ID_AVATAR_DATA_START` was + already received. If this conditions are valid, it 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 - `PACKET_ID_AVATAR_DATA_CONTROL` with the field 'op' set to - `AVATARDATACONTROL_MORE` and 'bytes_received' set to the new offset. + If not all data was received, "A" simply waits for more data. 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 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 @@ 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); - - LOGGER_DEBUG("friendnumber = %u, op = %u, bytes_received = %u, ret = %d", - friendnumber, op, bytes_received, ret); + &op, sizeof(op), 0); + LOGGER_DEBUG("friendnumber = %u, op = %u, ret = %d", + friendnumber, op, 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; - } - - case AVATARDATACONTROL_MORE: { - /* 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); + if (ret < 0) { + /* Something went wrong, try to signal the error so the friend + * can clear up the state. */ + send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR); return 0; } - /* 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 ad stops. */ - LOGGER_DEBUG("Avatar data transfer limit reached. " - "friendnumber = %u", friendnumber); - send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_ERROR, 0); - return 0; + /* User have no avatar data, nothing more to do. */ + if (m->avatar_format == AVATARFORMAT_NONE) + return 0; + + /* Send the actual avatar data. */ + uint32_t offset = 0; + while (offset < m->avatar_data_length) { + uint32_t chunk_len = m->avatar_data_length - offset; + 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 + offset, chunk_len); + offset += chunk_len; + 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); - return send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_MORE, 0); + /* Waits for more data to be received */ + return 0; } @@ -2371,9 +2367,8 @@ static int handle_avatar_data_push(Messenger *m, uint32_t friendnumber, return 0; } - /* Request more data */ - return send_avatar_data_control(m, friendnumber, AVATARDATACONTROL_MORE, - avrd->bytes_received); + /* Waits for more data to be received */ + return 0; } 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 { /* 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 */ -#define AVATAR_DATA_MAX_CHUNK_SIZE 1024 +/* Must be < MAX_CRYPTO_DATA_SIZE */ +#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; -- cgit v1.2.3