diff --git a/.github/actions/prepare-build/action.yml b/.github/actions/prepare-build/action.yml index e461e219..449c9117 100644 --- a/.github/actions/prepare-build/action.yml +++ b/.github/actions/prepare-build/action.yml @@ -31,7 +31,7 @@ runs: if: ${{ runner.os == 'macOS' }} shell: bash run: | - brew install llvm pkg-config nasm + brew install llvm pkg-config nasm go ln -s "/usr/local/opt/llvm/bin/clang-format" "/usr/local/bin/clang-format" ln -s "/usr/local/opt/llvm/bin/clang-tidy" "/usr/local/bin/clang-tidy" diff --git a/.github/workflows/main_ci.yml b/.github/workflows/main_ci.yml index 0751b5c0..9a7841f1 100644 --- a/.github/workflows/main_ci.yml +++ b/.github/workflows/main_ci.yml @@ -38,7 +38,6 @@ jobs: crypto: [openssl_1.1, openssl_3, boringssl] env: - BUILD_DIR: "${RUNNER_TEMP}/build_${{ matrix.crypto }}" CRYPTO_DIR: "./alternatives/${{ matrix.crypto }}" steps: @@ -55,18 +54,12 @@ jobs: - name: Build run: | - cmake -B "${{ env.BUILD_DIR }}" -DVCPKG_MANIFEST_DIR="${{ env.CRYPTO_DIR }}" -DTESTING=ON - cmake --build "${{ env.BUILD_DIR }}" + cmake -B "${{ runner.temp }}/build_${{ matrix.crypto }}" -DVCPKG_MANIFEST_DIR="${{ env.CRYPTO_DIR }}" -DTESTING=ON + cmake --build "${{ runner.temp }}/build_${{ matrix.crypto }}" - - name: Unit Test (non-Windows) - if: matrix.os != 'windows-latest' + - name: Unit Test run: | - cmake --build "${{ env.BUILD_DIR }}" --target test - - - name: Unit Test (Windows) - if: matrix.os == 'windows-latest' - run: | - cmake --build "${{ env.BUILD_DIR }}" --target RUN_TESTS + ctest --test-dir "${{ runner.temp }}/build_${{ matrix.crypto }}" interop-test: if: github.event.pull_request.draft == false @@ -75,7 +68,6 @@ jobs: runs-on: ubuntu-latest env: - BUILD_DIR: "${RUNNER_TEMP}/build_openssl_1.1" CRYPTO_DIR: "./alternatives/openssl_1.1" steps: @@ -92,8 +84,8 @@ jobs: - name: Build run: | - cmake -B "${{ env.BUILD_DIR }}" -DVCPKG_MANIFEST_DIR="${{ env.CRYPTO_DIR }}" - cmake --build "${{ env.BUILD_DIR }}" + cmake -B "${{ runner.temp }}/build_openssl_1.1" -DVCPKG_MANIFEST_DIR="${{ env.CRYPTO_DIR }}" + cmake --build "${{ runner.temp }}/build_openssl_1.1" - name: Build (Interop Harness) run: | @@ -124,7 +116,6 @@ jobs: crypto: [openssl_1.1, openssl_3, boringssl] env: - BUILD_DIR: "${RUNNER_TEMP}/build_${{ matrix.crypto }}" CRYPTO_DIR: "./alternatives/${{ matrix.crypto }}" steps: @@ -141,6 +132,6 @@ jobs: - name: Build with clang-tidy run: | - cmake -B "${{ env.BUILD_DIR }}" -DVCPKG_MANIFEST_DIR="${{ env.CRYPTO_DIR }}" \ - -DTESTING=ON -DCLANG_TIDY=ON -DSANITIZERS=ON - cmake --build "${{ env.BUILD_DIR }}" + cmake -B "${{ runner.temp }}/build_${{ matrix.crypto }}" -DVCPKG_MANIFEST_DIR="${{ env.CRYPTO_DIR }}" \ + -DTESTING=ON -DCLANG_TIDY=ON -DSANITIZERS=ON + cmake --build "${{ runner.temp }}/build_${{ matrix.crypto }}" diff --git a/README.md b/README.md index 66296ea8..7bc8bded 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ MLSPP requires a few prerequisite libraries in order to fully build. * [nlohmann::json](https://github.com/nlohmann/json) - Tested with latest versions. * Cryptography Library - OpenSSL 1.1.1, OpenSSL 3.0, BoringSSL compatible (see details below) -* [doctest](https://github.com/doctest/doctest) - Tested with latest versions. Only required when building the test suite. +* [Catch2](https://github.com/catchorg/Catch2) - Only required when building the test suite. ### Installing Prerequisites diff --git a/include/mls/state.h b/include/mls/state.h index 1741d10d..5f736bea 100644 --- a/include/mls/state.h +++ b/include/mls/state.h @@ -397,6 +397,8 @@ class State static bool valid_restart(const std::vector& proposals, ResumptionPSKUsage allowed_usage); + static bool valid_external_proposal_type(const Proposal::Type proposal_type); + CommitParams infer_commit_type( const std::optional& sender, const std::vector& proposals, diff --git a/lib/bytes/include/bytes/bytes.h b/lib/bytes/include/bytes/bytes.h index 582701a2..7d18407e 100644 --- a/lib/bytes/include/bytes/bytes.h +++ b/lib/bytes/include/bytes/bytes.h @@ -17,7 +17,11 @@ struct bytes bytes& operator=(bytes&&) = default; // Zeroize on drop - ~bytes() { std::fill(_data.begin(), _data.end(), uint8_t(0)); } + ~bytes() + { + auto ptr = static_cast(_data.data()); + std::fill(ptr, ptr + _data.size(), uint8_t(0)); + } // Mimic std::vector ctors bytes(size_t count, const uint8_t& value = 0) diff --git a/src/state.cpp b/src/state.cpp index 826503ba..44972eee 100644 --- a/src/state.cpp +++ b/src/state.cpp @@ -1250,6 +1250,12 @@ State::cache_proposal(AuthenticatedContent content_auth) } const auto& proposal = var::get(content_auth.content.content); + + if (content_auth.content.sender.sender_type() == SenderType::external && + !valid_external_proposal_type(proposal.proposal_type())) { + throw ProtocolError("Invalid external proposal"); + } + if (!valid(sender_location, proposal)) { throw ProtocolError("Invalid proposal"); } @@ -1632,7 +1638,6 @@ State::valid(std::optional sender, const Proposal& proposal) const { const auto specifically_valid = overloaded{ [&](const Update& update) { return valid(opt::get(sender), update); }, - [&](const auto& proposal) { return valid(proposal); }, }; return var::visit(specifically_valid, proposal.content); @@ -1870,6 +1875,22 @@ State::valid_restart(const std::vector& proposals, return acceptable_psks && found_allowed; } +bool +State::valid_external_proposal_type(const Proposal::Type proposal_type) +{ + switch (proposal_type) { + case ProposalType::add: + case ProposalType::remove: + case ProposalType::psk: + case ProposalType::reinit: + case ProposalType::group_context_extensions: + return true; + + default: + return false; + } +} + bool // NOLINTNEXTLINE(readability-convert-member-functions-to-static) State::valid_external(const std::vector& proposals) const diff --git a/src/treekem.cpp b/src/treekem.cpp index 53aeea1c..db12582a 100644 --- a/src/treekem.cpp +++ b/src/treekem.cpp @@ -903,9 +903,10 @@ TreeKEMPublicKey::parent_hashes( } // The list of nodes for whom parent hashes are computed, namely: Direct path - // excluding root, including leaf + // excluding the last entry, including leaf auto from_node = NodeIndex(from); auto dp = fdp; + auto [last, _res_last] = dp.back(); dp.pop_back(); dp.insert(dp.begin(), { from_node, {} }); @@ -913,8 +914,8 @@ TreeKEMPublicKey::parent_hashes( throw ProtocolError("Malformed UpdatePath"); } - // Parent hash for all the parents, starting from the root - auto last = NodeIndex::root(size); + // Parent hash for all the parents, starting from the last entry of the + // filtered direct path auto last_hash = bytes{}; auto ph = std::vector(dp.size()); for (int i = static_cast(dp.size()) - 1; i >= 0; i--) { diff --git a/test/state.cpp b/test/state.cpp index 0f922ff2..c75b60e9 100644 --- a/test/state.cpp +++ b/test/state.cpp @@ -1069,3 +1069,197 @@ TEST_CASE_METHOD(RelatedGroupTest, "Reinitialize the group") REQUIRE(state == new_states[0]); } } + +TEST_CASE_METHOD(StateTest, "Parent Hash with Empty Left Subtree") +{ + // Create a group with 4 members + auto state_0 = State(group_id, + suite, + leaf_privs[0], + identity_privs[0], + key_packages[0].leaf_node, + ExtensionList{}); + + const auto adds = std::vector{ + state_0.add_proposal(key_packages[1]), + state_0.add_proposal(key_packages[2]), + state_0.add_proposal(key_packages[3]), + }; + + auto [_commit0, welcome0, new_state_0] = + state_0.commit(fresh_secret(), CommitOpts{ adds, true, false, {} }, {}); + silence_unused(_commit0); + state_0 = new_state_0; + + auto state_2 = State(init_privs[2], + leaf_privs[2], + identity_privs[2], + key_packages[2], + welcome0, + std::nullopt, + {}); + // Member @2 removes the members on the left side of the tree + const auto removes = std::vector{ + state_2.remove_proposal(LeafIndex{ 0 }), + state_2.remove_proposal(LeafIndex{ 1 }), + }; + + auto [commit2, welcome2, new_state_2] = + state_2.commit(fresh_secret(), CommitOpts{ removes, true, false, {} }, {}); + silence_unused(commit2); + silence_unused(welcome2); + state_2 = new_state_2; + + // Member @2 should have a valid tree, even though its filtered direct path no + // longer goes to the root. + REQUIRE(state_2.tree().parent_hash_valid()); +} + +class ExternalSenderTest : public StateTest +{ +protected: + const SignaturePrivateKey external_sig_priv = + SignaturePrivateKey::generate(suite); + const Credential external_sender_cred = Credential::basic({ 0 }); + const bytes psk_id = from_ascii("psk ID"); + ExtensionList group_extensions; + + ExternalSenderTest() + { + group_extensions.add(ExternalSendersExtension{ { + { external_sig_priv.public_key, external_sender_cred }, + } }); + + // Initialize the creator's state + states.emplace_back(group_id, + suite, + leaf_privs[0], + identity_privs[0], + key_packages[0].leaf_node, + group_extensions); + + // Add a second member so that we can test removal proposal + auto add = states[0].add_proposal(key_packages[1]); + auto [commit, welcome, new_state] = states[0].commit( + fresh_secret(), CommitOpts{ { add }, true, false, {} }, {}); + states[0] = new_state; + + silence_unused(commit); + + states.push_back({ init_privs[1], + leaf_privs[1], + identity_privs[1], + key_packages[1], + welcome, + std::nullopt, + {} }); + } + + PublicMessage GenerateExternalSenderProposal(const Proposal& proposal) + { + auto group_context = states[0].group_context(); + + auto proposal_content = GroupContent{ group_context.group_id, + group_context.epoch, + { ExternalSenderIndex{ 0 } }, + {}, + proposal }; + + auto content_auth_original = + AuthenticatedContent::sign(WireFormat::mls_public_message, + proposal_content, + suite, + external_sig_priv, + group_context); + + return PublicMessage::protect( + content_auth_original, suite, std::nullopt, group_context); + } +}; + +TEST_CASE_METHOD(ExternalSenderTest, + "Allows Expected Proposals from External Sender") +{ + // For expected proposals, we ensure that calling State::handle with the + // proposal does not throw an exception. + + // Add + auto add_proposal = Proposal{ Add{ key_packages[2] } }; + auto ext_add_message = GenerateExternalSenderProposal(add_proposal); + + REQUIRE(!states[0].handle(ext_add_message).has_value()); + + // Remove + auto remove_proposal = Proposal{ Remove{ LeafIndex{ 1 } } }; + auto ext_remove_message = GenerateExternalSenderProposal(remove_proposal); + + REQUIRE(!states[0].handle(ext_remove_message).has_value()); + + // PSK + auto group_context = states[0].group_context(); + auto psk_proposal = + Proposal{ PreSharedKey{ ResumptionPSK{ ResumptionPSKUsage::application, + group_context.group_id, + group_context.epoch }, + random_bytes(suite.secret_size()) } }; + auto ext_psk_message = GenerateExternalSenderProposal(psk_proposal); + + REQUIRE(!states[0].handle(ext_psk_message).has_value()); + + // ReInit + auto updated_extensions = group_extensions; + updated_extensions.add(CustomExtension{ 0xa0 }); + + auto reinit_proposal = Proposal{ ReInit{ group_context.group_id, + ProtocolVersion::mls10, + group_context.cipher_suite, + updated_extensions } }; + auto ext_reinit_message = GenerateExternalSenderProposal(reinit_proposal); + + REQUIRE(!states[0].handle(ext_reinit_message).has_value()); + + // GroupContextExtensions + + auto group_context_proposal = + Proposal{ GroupContextExtensions{ updated_extensions } }; + auto ext_group_context_message = + GenerateExternalSenderProposal(group_context_proposal); + + REQUIRE(!states[0].handle(ext_group_context_message).has_value()); +} + +TEST_CASE_METHOD(ExternalSenderTest, + "Refuses Unexpected Proposals from External Sender") +{ + // For unexpected proposals, we ensure that calling State::handle with the + // throws the expected exception. + + // The proposals throw bad_optional_access since the validation calls + // opt::get(sender) on a nullopt sender + + // Update + auto update_proposal = Proposal{ Update{ key_packages[1].leaf_node } }; + auto ext_update_message = GenerateExternalSenderProposal(update_proposal); + + REQUIRE_THROWS_WITH(states[0].handle(ext_update_message), + "Invalid external proposal"); + + // ExternalInit + auto group_info = states[0].group_info(false); + auto maybe_external_pub = group_info.extensions.find(); + + REQUIRE(maybe_external_pub.has_value()); + + const auto& external_pub = opt::get(maybe_external_pub).external_pub; + + auto [kem_output, force_init_secret] = + KeyScheduleEpoch::external_init(suite, external_pub); + silence_unused(force_init_secret); + + auto external_init_proposal = Proposal{ ExternalInit{ kem_output } }; + auto external_init_message = + GenerateExternalSenderProposal(external_init_proposal); + + REQUIRE_THROWS_WITH(states[0].handle(external_init_message), + "Invalid external proposal"); +}