Merge pull request #36 from jagerman/post-compress-size-limits

Apply size limit check *after* compression & encryption

.drone.jsonnet

@@ -152,7 +152,7 @@ local windows_cross_pipeline(name,
                    commands: [
                      apt_get_quiet + ' install -y --no-install-recommends wine64',
                      'cd build',
-                     'wine64-stable ./tests/testAll.exe --colour-mode ansi',
+                     'wine-stable ./tests/testAll.exe --colour-mode ansi',
                    ],
                  }] else [])
 );
@@ -292,7 +292,7 @@ local static_build(name,
   debian_build('Debian stable (armhf)', docker_base + 'debian-stable/arm32v7', arch='arm64', jobs=4),
 
   // Windows builds (x64)
-  windows_cross_pipeline('Windows (amd64)', docker_base + 'debian-win32-cross'),
+  windows_cross_pipeline('Windows (amd64)', docker_base + 'debian-win32-cross-wine'),
 
   // Macos builds:
   mac_builder('macOS (Release)'),

src/config.cpp

@@ -762,57 +762,48 @@ ustring ConfigMessage::serialize(bool enable_signing) {
 }
 
 ustring ConfigMessage::serialize_impl(const oxenc::bt_dict& curr_diff, bool enable_signing) {
-    ustring result;
-    // FIXME: we don't have a max (multi-messages will get encoded to larger and then split up).
-    result.resize(MAX_MESSAGE_SIZE);
-    oxenc::bt_dict_producer outer{from_unsigned(result.data()), result.size()};
+    oxenc::bt_dict_producer outer{};
 
-    try {
-        outer.append("#", seqno());
+    outer.append("#", seqno());
 
-        auto unknown_it = append_unknown(outer, unknown_.begin(), unknown_.end(), "&");
+    auto unknown_it = append_unknown(outer, unknown_.begin(), unknown_.end(), "&");
 
-        serialize_data(outer.append_dict("&"), data_);
+    serialize_data(outer.append_dict("&"), data_);
 
-        unknown_it = append_unknown(outer, unknown_it, unknown_.end(), "<");
+    unknown_it = append_unknown(outer, unknown_it, unknown_.end(), "<");
 
-        {
-            auto lags = outer.append_list("<");
-            for (auto& [seqno_hash, lag_data] : lagged_diffs_) {
-                const auto& [lag_seqno, lag_hash] = seqno_hash;
-                if (lag_seqno <= seqno() - lag || lag_seqno >= seqno())
-                    continue;
-                auto lag = lags.append_list();
-                lag.append(lag_seqno);
-                lag.append(view(lag_hash));
-                lag.append_bt(lag_data);
-            }
+    {
+        auto lags = outer.append_list("<");
+        for (auto& [seqno_hash, lag_data] : lagged_diffs_) {
+            const auto& [lag_seqno, lag_hash] = seqno_hash;
+            if (lag_seqno <= seqno() - lag || lag_seqno >= seqno())
+                continue;
+            auto lag = lags.append_list();
+            lag.append(lag_seqno);
+            lag.append(view(lag_hash));
+            lag.append_bt(lag_data);
         }
+    }
 
-        unknown_it = append_unknown(outer, unknown_it, unknown_.end(), "=");
+    unknown_it = append_unknown(outer, unknown_it, unknown_.end(), "=");
 
-        outer.append_bt("=", curr_diff);
+    outer.append_bt("=", curr_diff);
 
-        unknown_it = append_unknown(outer, unknown_it, unknown_.end(), "~");
-        assert(unknown_it == unknown_.end());
+    unknown_it = append_unknown(outer, unknown_it, unknown_.end(), "~");
+    assert(unknown_it == unknown_.end());
 
-        if (signer && enable_signing) {
-            auto to_sign = to_unsigned_sv(outer.view());
-            // The view contains the trailing "e", but we don't sign it (we are going to append the
-            // signature there instead):
-            to_sign.remove_suffix(1);
-            auto sig = signer(to_sign);
-            if (sig.size() != 64)
-                throw std::logic_error{
-                        "Invalid signature: signing function did not return 64 bytes"};
+    if (signer && enable_signing) {
+        auto to_sign = to_unsigned_sv(outer.view());
+        // The view contains the trailing "e", but we don't sign it (we are going to append the
+        // signature there instead):
+        to_sign.remove_suffix(1);
+        auto sig = signer(to_sign);
+        if (sig.size() != 64)
+            throw std::logic_error{"Invalid signature: signing function did not return 64 bytes"};
 
-            outer.append("~", from_unsigned_sv(sig));
-        }
-    } catch (const std::length_error&) {
-        throw std::length_error{"Config data is too large"};
+        outer.append("~", from_unsigned_sv(sig));
     }
-    result.resize(outer.view().size());
-    return result;
+    return ustring{to_unsigned_sv(outer.view())};
 }
 
 const hash_t& MutableConfigMessage::hash() {

src/config/base.cpp

@@ -256,9 +256,6 @@ std::tuple<seqno_t, ustring, std::vector<std::string>> ConfigBase::push() {
     if (_keys_size == 0)
         throw std::logic_error{"Cannot push data without an encryption key!"};
 
-    if (is_dirty())
-        set_state(ConfigState::Waiting);
-
     std::tuple<seqno_t, ustring, std::vector<std::string>> ret{
             _config->seqno(), _config->serialize(), {}};
 
@@ -268,6 +265,12 @@ std::tuple<seqno_t, ustring, std::vector<std::string>> ConfigBase::push() {
     pad_message(msg);  // Prefix pad with nulls
     encrypt_inplace(msg, key(), encryption_domain());
 
+    if (msg.size() > MAX_MESSAGE_SIZE)
+        throw std::length_error{"Config data is too large"};
+
+    if (is_dirty())
+        set_state(ConfigState::Waiting);
+
     for (auto& old : _old_hashes)
         obs.push_back(std::move(old));
     _old_hashes.clear();

tests/test_config_contacts.cpp

@@ -1,3 +1,4 @@
+#include <oxenc/endian.h>
 #include <oxenc/hex.h>
 #include <session/config/contacts.h>
 #include <sodium/crypto_sign_ed25519.h>
@@ -371,3 +372,47 @@ TEST_CASE("Contacts (C API)", "[config][contacts][c]") {
     CHECK(contacts_get(conf, &ci, definitely_real_id));
     CHECK_FALSE(contacts_get(conf, &ci, another_id));
 }
+
+TEST_CASE("huge contacts compression", "[config][compression][contacts]") {
+    // Test that we can produce a config message whose *uncompressed* length exceeds the maximum
+    // message length as long as its *compressed* length does not.
+
+    const auto seed = "0123456789abcdef0123456789abcdef00000000000000000000000000000000"_hexbytes;
+    std::array<unsigned char, 32> ed_pk, curve_pk;
+    std::array<unsigned char, 64> ed_sk;
+    crypto_sign_ed25519_seed_keypair(
+            ed_pk.data(), ed_sk.data(), reinterpret_cast<const unsigned char*>(seed.data()));
+    int rc = crypto_sign_ed25519_pk_to_curve25519(curve_pk.data(), ed_pk.data());
+    REQUIRE(rc == 0);
+
+    REQUIRE(oxenc::to_hex(ed_pk.begin(), ed_pk.end()) ==
+            "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7");
+    REQUIRE(oxenc::to_hex(curve_pk.begin(), curve_pk.end()) ==
+            "d2ad010eeb72d72e561d9de7bd7b6989af77dcabffa03a5111a6c859ae5c3a72");
+
+    session::config::Contacts contacts{ustring_view{seed}, std::nullopt};
+
+    for (uint16_t i = 0; i < 10000; i++) {
+        char buf[2];
+        oxenc::write_host_as_big(i, buf);
+        std::string session_id = "05000000000000000000000000000000000000000000000000000000000000";
+        session_id += oxenc::to_hex(buf, buf + 2);
+        REQUIRE(session_id.size() == 66);
+
+        auto c = contacts.get_or_construct(session_id);
+        c.nickname = "My friend " + std::to_string(i);
+        c.approved = true;
+        c.approved_me = true;
+        contacts.set(c);
+    }
+
+    CHECK(contacts.needs_push());
+    CHECK(contacts.needs_dump());
+
+    auto [seqno, to_push, obs] = contacts.push();
+    CHECK(seqno == 1);
+    CHECK(to_push.size() == 46'080);
+    auto dump = contacts.dump();
+    // With tons of duplicate info the push should have been nicely compressible:
+    CHECK(dump.size() > 1'320'000);
+}