test: Add unit test for create/handle request packets.

Also added an assert to tell coverity that we don't actually have an
overflow (or underflow) of the length parameter.

other/bootstrap_daemon/docker/tox-bootstrapd.sha256

@@ -1 +1 @@
-ce191b497af89aafb2837d053043469c725ea7af0b1aa2bbedc39f8de2686d56  /usr/local/bin/tox-bootstrapd
+ac88db3ee0c9ca621846897bb508c469eba97c263637966d74483e266b5564b0  /usr/local/bin/tox-bootstrapd

toxcore/DHT.c

@@ -313,33 +313,44 @@ void dht_get_shared_key_sent(DHT *dht, uint8_t *shared_key, const uint8_t *publi
 
 #define CRYPTO_SIZE (1 + CRYPTO_PUBLIC_KEY_SIZE * 2 + CRYPTO_NONCE_SIZE)
 
-/** Create a request to peer.
- * send_public_key and send_secret_key are the pub/secret keys of the sender.
- * recv_public_key is public key of receiver.
- * packet must be an array of MAX_CRYPTO_REQUEST_SIZE big.
- * Data represents the data we send with the request with length being the length of the data.
- * request_id is the id of the request (32 = friend request, 254 = ping request).
+/**
+ * @brief Create a request to peer.
  *
- *  return -1 on failure.
- *  return the length of the created packet on success.
+ * Packs the data and sender public key and encrypts the packet.
+ *
+ * @param[in] send_public_key public key of the sender.
+ * @param[in] send_secret_key secret key of the sender.
+ * @param[out] packet an array of @ref MAX_CRYPTO_REQUEST_SIZE big.
+ * @param[in] recv_public_key public key of the receiver.
+ * @param[in] data represents the data we send with the request.
+ * @param[in] data_length the length of the data.
+ * @param[in] request_id the id of the request (32 = friend request, 254 = ping request).
+ *
+ * @attention Constraints:
+ * @code
+ * sizeof(packet) >= MAX_CRYPTO_REQUEST_SIZE
+ * @endcode
+ *
+ * @retval -1 on failure.
+ * @return the length of the created packet on success.
  */
 int create_request(const uint8_t *send_public_key, const uint8_t *send_secret_key, uint8_t *packet,
-                   const uint8_t *recv_public_key, const uint8_t *data, uint32_t length, uint8_t request_id)
+                   const uint8_t *recv_public_key, const uint8_t *data, uint32_t data_length, uint8_t request_id)
 {
     if (send_public_key == nullptr || packet == nullptr || recv_public_key == nullptr || data == nullptr) {
         return -1;
     }
 
-    if (MAX_CRYPTO_REQUEST_SIZE < length + CRYPTO_SIZE + 1 + CRYPTO_MAC_SIZE) {
+    if (MAX_CRYPTO_REQUEST_SIZE < data_length + CRYPTO_SIZE + 1 + CRYPTO_MAC_SIZE) {
         return -1;
     }
 
     uint8_t *const nonce = packet + 1 + CRYPTO_PUBLIC_KEY_SIZE * 2;
     random_nonce(nonce);
     uint8_t temp[MAX_CRYPTO_REQUEST_SIZE];
-    memcpy(temp + 1, data, length);
+    memcpy(temp + 1, data, data_length);
     temp[0] = request_id;
-    const int len = encrypt_data(recv_public_key, send_secret_key, nonce, temp, length + 1,
+    const int len = encrypt_data(recv_public_key, send_secret_key, nonce, temp, data_length + 1,
                                  packet + CRYPTO_SIZE);
 
     if (len == -1) {
@@ -355,21 +366,37 @@ int create_request(const uint8_t *send_public_key, const uint8_t *send_secret_ke
     return len + CRYPTO_SIZE;
 }
 
-/** Puts the senders public key in the request in public_key, the data from the request
- * in data if a friend or ping request was sent to us and returns the length of the data.
- * packet is the request packet and length is its length.
+/**
+ * @brief Decrypts and unpacks a DHT request packet.
  *
- *  return -1 if not valid request.
+ * Puts the senders public key in the request in @p public_key, the data from
+ * the request in @p data.
+ *
+ * @param[in] self_public_key public key of the receiver (us).
+ * @param[in] self_secret_key secret key of the receiver (us).
+ * @param[out] public_key public key of the sender, copied from the input packet.
+ * @param[out] data decrypted request data, copied from the input packet, must
+ *    have room for @ref MAX_CRYPTO_REQUEST_SIZE bytes.
+ * @param[in] packet is the request packet.
+ * @param[in] packet_length length of the packet.
+ *
+ * @attention Constraints:
+ * @code
+ * sizeof(data) >= MAX_CRYPTO_REQUEST_SIZE
+ * @endcode
+ *
+ * @retval -1 if not valid request.
+ * @return the length of the unpacked data.
  */
 int handle_request(const uint8_t *self_public_key, const uint8_t *self_secret_key, uint8_t *public_key, uint8_t *data,
-                   uint8_t *request_id, const uint8_t *packet, uint16_t length)
+                   uint8_t *request_id, const uint8_t *packet, uint16_t packet_length)
 {
     if (self_public_key == nullptr || public_key == nullptr || data == nullptr || request_id == nullptr
             || packet == nullptr) {
         return -1;
     }
 
-    if (length <= CRYPTO_SIZE + CRYPTO_MAC_SIZE || length > MAX_CRYPTO_REQUEST_SIZE) {
+    if (packet_length <= CRYPTO_SIZE + CRYPTO_MAC_SIZE || packet_length > MAX_CRYPTO_REQUEST_SIZE) {
         return -1;
     }
 
@@ -381,13 +408,14 @@ int handle_request(const uint8_t *self_public_key, const uint8_t *self_secret_ke
     const uint8_t *const nonce = packet + 1 + CRYPTO_PUBLIC_KEY_SIZE * 2;
     uint8_t temp[MAX_CRYPTO_REQUEST_SIZE];
     int len1 = decrypt_data(public_key, self_secret_key, nonce,
-                            packet + CRYPTO_SIZE, length - CRYPTO_SIZE, temp);
+                            packet + CRYPTO_SIZE, packet_length - CRYPTO_SIZE, temp);
 
     if (len1 == -1 || len1 == 0) {
         crypto_memzero(temp, MAX_CRYPTO_REQUEST_SIZE);
         return -1;
     }
 
+    assert(len1 > 0);
     request_id[0] = temp[0];
     --len1;
     memcpy(data, temp + 1, len1);

toxcore/DHT.h

@@ -3,8 +3,8 @@
  * Copyright © 2013 Tox project.
  */
 
-/**
- * An implementation of the DHT as seen in docs/updates/DHT.md
+/** @file
+ * @brief An implementation of the DHT as seen in docs/updates/DHT.md
  */
 #ifndef C_TOXCORE_TOXCORE_DHT_H
 #define C_TOXCORE_TOXCORE_DHT_H
@@ -53,37 +53,65 @@ extern "C" {
 /** The number of "fake" friends to add (for optimization purposes and so our paths for the onion part are more random) */
 #define DHT_FAKE_FRIEND_NUMBER 2
 
+/** Maximum packet size for a DHT request packet. */
 #define MAX_CRYPTO_REQUEST_SIZE 1024
 
 #define CRYPTO_PACKET_FRIEND_REQ    32  // Friend request crypto packet ID.
 #define CRYPTO_PACKET_DHTPK         156
 #define CRYPTO_PACKET_NAT_PING      254 // NAT ping crypto packet ID.
 
-/** Create a request to peer.
- * send_public_key and send_secret_key are the pub/secret keys of the sender.
- * recv_public_key is public key of receiver.
- * packet must be an array of MAX_CRYPTO_REQUEST_SIZE big.
- * Data represents the data we send with the request with length being the length of the data.
- * request_id is the id of the request (32 = friend request, 254 = ping request).
+/**
+ * @brief Create a request to peer.
  *
- * return -1 on failure.
- * return the length of the created packet on success.
+ * Packs the data and sender public key and encrypts the packet.
+ *
+ * @param[in] send_public_key public key of the sender.
+ * @param[in] send_secret_key secret key of the sender.
+ * @param[out] packet an array of @ref MAX_CRYPTO_REQUEST_SIZE big.
+ * @param[in] recv_public_key public key of the receiver.
+ * @param[in] data represents the data we send with the request.
+ * @param[in] data_length the length of the data.
+ * @param[in] request_id the id of the request (32 = friend request, 254 = ping request).
+ *
+ * @attention Constraints:
+ * @code
+ * sizeof(packet) >= MAX_CRYPTO_REQUEST_SIZE
+ * @endcode
+ *
+ * @retval -1 on failure.
+ * @return the length of the created packet on success.
  */
 non_null()
 int create_request(
     const uint8_t *send_public_key, const uint8_t *send_secret_key, uint8_t *packet,
-    const uint8_t *recv_public_key, const uint8_t *data, uint32_t length, uint8_t request_id);
+    const uint8_t *recv_public_key, const uint8_t *data, uint32_t data_length, uint8_t request_id);
 
-/** Puts the senders public key in the request in public_key, the data from the request
- * in data if a friend or ping request was sent to us and returns the length of the data.
- * packet is the request packet and length is its length.
+/**
+ * @brief Decrypts and unpacks a DHT request packet.
  *
- *  return -1 if not valid request.
+ * Puts the senders public key in the request in @p public_key, the data from
+ * the request in @p data.
+ *
+ * @param[in] self_public_key public key of the receiver (us).
+ * @param[in] self_secret_key secret key of the receiver (us).
+ * @param[out] public_key public key of the sender, copied from the input packet.
+ * @param[out] data decrypted request data, copied from the input packet, must
+ *    have room for @ref MAX_CRYPTO_REQUEST_SIZE bytes.
+ * @param[in] packet is the request packet.
+ * @param[in] packet_length length of the packet.
+ *
+ * @attention Constraints:
+ * @code
+ * sizeof(data) >= MAX_CRYPTO_REQUEST_SIZE
+ * @endcode
+ *
+ * @retval -1 if not valid request.
+ * @return the length of the unpacked data.
  */
 non_null()
 int handle_request(
     const uint8_t *self_public_key, const uint8_t *self_secret_key, uint8_t *public_key, uint8_t *data,
-    uint8_t *request_id, const uint8_t *packet, uint16_t length);
+    uint8_t *request_id, const uint8_t *packet, uint16_t packet_length);
 
 typedef struct IPPTs {
     IP_Port     ip_port;

toxcore/DHT_test.cc

@@ -10,23 +10,34 @@
 namespace {
 
 using PublicKey = std::array<uint8_t, CRYPTO_PUBLIC_KEY_SIZE>;
+using SecretKey = std::array<uint8_t, CRYPTO_SECRET_KEY_SIZE>;
+
+struct KeyPair {
+    PublicKey pk;
+    SecretKey sk;
+
+    KeyPair() { crypto_new_keypair(pk.data(), sk.data()); }
+};
 
 template <typename T, size_t N>
 std::array<T, N> to_array(T const (&arr)[N])
 {
     std::array<T, N> stdarr;
-    memcpy(stdarr.data(), arr, N);
+    std::copy(arr, arr + N, stdarr.begin());
     return stdarr;
 }
 
+PublicKey random_pk()
+{
+    PublicKey pk;
+    random_bytes(pk.data(), pk.size());
+    return pk;
+}
+
 TEST(IdClosest, IdenticalKeysAreSameDistance)
 {
-    PublicKey pk0;
-    random_bytes(pk0.data(), CRYPTO_PUBLIC_KEY_SIZE);
-
-    PublicKey pk1;
-    random_bytes(pk1.data(), CRYPTO_PUBLIC_KEY_SIZE);
-
+    PublicKey pk0 = random_pk();
+    PublicKey pk1 = random_pk();
     PublicKey pk2 = pk1;
 
     EXPECT_EQ(id_closest(pk0.data(), pk1.data(), pk2.data()), 0);
@@ -35,14 +46,9 @@ TEST(IdClosest, IdenticalKeysAreSameDistance)
 TEST(IdClosest, DistanceIsCommutative)
 {
     for (uint32_t i = 0; i < 100; ++i) {
-        PublicKey pk0;
-        random_bytes(pk0.data(), CRYPTO_PUBLIC_KEY_SIZE);
-
-        PublicKey pk1;
-        random_bytes(pk1.data(), CRYPTO_PUBLIC_KEY_SIZE);
-
-        PublicKey pk2;
-        random_bytes(pk2.data(), CRYPTO_PUBLIC_KEY_SIZE);
+        PublicKey pk0 = random_pk();
+        PublicKey pk1 = random_pk();
+        PublicKey pk2 = random_pk();
 
         ASSERT_NE(pk1, pk2);  // RNG can't produce the same random key twice
 
@@ -116,4 +122,58 @@ TEST(AddToList, OverridesKeysWithCloserKeys)
     EXPECT_EQ(to_array(nodes[3].public_key), keys[2]);
 }
 
+TEST(Request, CreateAndParse)
+{
+    // Peers.
+    const KeyPair sender;
+    const KeyPair receiver;
+    const uint8_t sent_pkt_id = CRYPTO_PACKET_FRIEND_REQ;
+
+    // Encoded packet.
+    std::array<uint8_t, MAX_CRYPTO_REQUEST_SIZE> packet;
+
+    // Received components.
+    PublicKey pk;
+    std::array<uint8_t, MAX_CRYPTO_REQUEST_SIZE> incoming;
+    uint8_t recvd_pkt_id;
+
+    // Request data: maximum payload is 918 bytes, so create a payload 1 byte larger than max.
+    std::vector<uint8_t> outgoing(919);
+    random_bytes(outgoing.data(), outgoing.size());
+
+    EXPECT_LT(create_request(sender.pk.data(), sender.sk.data(), packet.data(), receiver.pk.data(),
+                  outgoing.data(), outgoing.size(), sent_pkt_id),
+        0);
+
+    // Pop one element so the payload is 918 bytes. Packing should now succeed.
+    outgoing.pop_back();
+
+    const int max_sent_length = create_request(sender.pk.data(), sender.sk.data(), packet.data(),
+        receiver.pk.data(), outgoing.data(), outgoing.size(), sent_pkt_id);
+    ASSERT_GT(max_sent_length, 0);  // success.
+
+    // Check that handle_request rejects packets larger than the maximum created packet size.
+    EXPECT_LT(handle_request(receiver.pk.data(), receiver.sk.data(), pk.data(), incoming.data(),
+                  &recvd_pkt_id, packet.data(), max_sent_length + 1),
+        0);
+
+    // Now try all possible packet sizes from max (918) to 0.
+    while (!outgoing.empty()) {
+        // Pack:
+        const int sent_length = create_request(sender.pk.data(), sender.sk.data(), packet.data(),
+            receiver.pk.data(), outgoing.data(), outgoing.size(), sent_pkt_id);
+        ASSERT_GT(sent_length, 0);
+
+        // Unpack:
+        const int recvd_length = handle_request(receiver.pk.data(), receiver.sk.data(), pk.data(),
+            incoming.data(), &recvd_pkt_id, packet.data(), sent_length);
+        ASSERT_GE(recvd_length, 0);
+
+        EXPECT_EQ(
+            std::vector<uint8_t>(incoming.begin(), incoming.begin() + recvd_length), outgoing);
+
+        outgoing.pop_back();
+    }
+}
+
 }  // namespace