From 07393363cf07365b1a84f95af2b6b04215f862a4 Mon Sep 17 00:00:00 2001 From: oneiric Date: Wed, 20 Jun 2018 22:05:12 +0000 Subject: [PATCH 1/8] RouterInfo: sign during router creation Sign, and append the signature, during RouterInfo creation. Referencing #627 + #917 --- src/core/router/info.cc | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/core/router/info.cc b/src/core/router/info.cc index 052edbc9..44fa28c9 100644 --- a/src/core/router/info.cc +++ b/src/core/router/info.cc @@ -699,15 +699,6 @@ void RouterInfo::CreateBuffer(const PrivateKeys& private_keys) m_Buffer( reinterpret_cast(router_info.Str().c_str()), router_info.Str().size()); - - // Signature - // TODO(anonimal): signing should be done when creating RI, not after. Requires other refactoring. - private_keys.Sign( - m_Buffer.data(), - m_Buffer.size(), - m_Buffer.data() + m_Buffer.size()); - - m_Buffer(m_Buffer.size() + private_keys.GetPublic().GetSignatureLen()); } catch (...) { @@ -879,7 +870,17 @@ void RouterInfo::CreateRouterInfo( // Write remaining options to RI router_info.Write(options.Str().c_str(), options.Str().size()); - // TODO(anonimal): we should implement RI signing *here* + // Ensure signature has proper capacity + std::vector sig_buf(private_keys.GetPublic().GetSignatureLen()); + + // Sign RI + private_keys.Sign( + reinterpret_cast(router_info.Str().c_str()), + router_info.Str().size(), + sig_buf.data()); + + // Write signature to RI + router_info.Write(sig_buf.data(), sig_buf.size()); LOG(debug) << "RouterInfo: " << __func__ << " total RI size: " << router_info.Str().size(); From fcee1416698c2c7639a4216e2c53c2d3575e310e Mon Sep 17 00:00:00 2001 From: oneiric Date: Wed, 20 Jun 2018 23:22:54 +0000 Subject: [PATCH 2/8] RouterInfo: verify signed router Verify that a router has a valid signature. Referencing #627 + #917 --- src/core/router/info.cc | 36 +++++++++++++++++++++++++++--------- src/core/router/info.h | 6 ++++++ 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/src/core/router/info.cc b/src/core/router/info.cc index 44fa28c9..23b8e263 100644 --- a/src/core/router/info.cc +++ b/src/core/router/info.cc @@ -164,15 +164,7 @@ void RouterInfo::ReadFromBuffer(bool verify_signature) // Verify signature if (verify_signature) { - // Note: signature length is guaranteed to be no less than buffer length - std::uint16_t const len = - m_Buffer.size() - m_RouterIdentity.GetSignatureLen(); - if (!m_RouterIdentity.Verify( - m_Buffer.data(), len, m_Buffer.data() + len)) - { - LOG(error) << "RouterInfo: signature verification failed"; - m_IsUnreachable = true; - } + Verify(); m_RouterIdentity.DropVerifier(); } } @@ -699,6 +691,32 @@ void RouterInfo::CreateBuffer(const PrivateKeys& private_keys) m_Buffer( reinterpret_cast(router_info.Str().c_str()), router_info.Str().size()); + + // Verify signature + Verify(); + } + catch (...) + { + m_Exception.Dispatch(__func__); + throw; + } +} + +void RouterInfo::Verify() +{ + try + { + if (!m_Buffer.data()) + throw std::runtime_error("RouterInfo: null buffer"); + std::size_t const len = m_Buffer.size() - m_RouterIdentity.GetSignatureLen(); + if (len < Size::MinUnsignedBuffer) + throw std::length_error("RouterInfo: invalid RouterInfo size"); + auto const buf = m_Buffer.data(); + if (!m_RouterIdentity.Verify(buf, len, &buf[len])) + { + m_IsUnreachable = true; + throw std::runtime_error("RouterInfo: signature verification failed"); + } } catch (...) { diff --git a/src/core/router/info.h b/src/core/router/info.h index a834498c..de16e823 100644 --- a/src/core/router/info.h +++ b/src/core/router/info.h @@ -69,6 +69,7 @@ struct RouterInfoTraits { MinBuffer = core::DSA_SIGNATURE_LENGTH, // TODO(unassigned): see #498 MaxBuffer = 2048, // TODO(anonimal): review if arbitrary + MinUnsignedBuffer = 399, // Minimum RouterInfo length w/o signature, see spec // TODO(unassigned): algorithm to dynamically determine cost NTCPCost = 10, // NTCP *should* have priority over SSU SSUCost = 5, @@ -523,6 +524,11 @@ class RouterInfo : public RouterInfoTraits, public RoutingDestination /// (and subsequently sign the RI with) void CreateBuffer(const PrivateKeys& private_keys); + /// @brief Verify RI signature + /// @throws std::length_error if unsigned buffer length is below minimum + /// @throws std::runtime_error if signature verification fails + void Verify(); + /// @brief Save RI to file /// @param path Full RI path of file to save to void SaveToFile(const std::string& path); From e850f5743c4b20346935b059cf078005a3c8d3e1 Mon Sep 17 00:00:00 2001 From: oneiric Date: Wed, 20 Jun 2018 03:43:14 +0000 Subject: [PATCH 3/8] Tests: RouterInfo signature unit-tests Referencing #627 + #917 --- tests/unit_tests/CMakeLists.txt | 1 + tests/unit_tests/core/router/info.cc | 64 ++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 tests/unit_tests/core/router/info.cc diff --git a/tests/unit_tests/CMakeLists.txt b/tests/unit_tests/CMakeLists.txt index e9be09e5..6653818b 100644 --- a/tests/unit_tests/CMakeLists.txt +++ b/tests/unit_tests/CMakeLists.txt @@ -19,6 +19,7 @@ add_executable(kovri-tests core/crypto/util/x509.cc core/router/identity.cc core/router/net_db/impl.cc + core/router/info.cc core/router/transports/ssu/packet.cc core/util/buffer.cc core/util/byte_stream.cc diff --git a/tests/unit_tests/core/router/info.cc b/tests/unit_tests/core/router/info.cc new file mode 100644 index 00000000..dcb61bbb --- /dev/null +++ b/tests/unit_tests/core/router/info.cc @@ -0,0 +1,64 @@ +/** + * Copyright (c) 2015-2018, The Kovri I2P Router Project + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without modification, are + * permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this list of + * conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, this list + * of conditions and the following disclaimer in the documentation and/or other + * materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors may be + * used to endorse or promote products derived from this software without specific + * prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL + * THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +#define BOOST_TEST_DYN_LINK + +#include + +#include "core/router/identity.h" + +namespace core = kovri::core; + +struct RouterInfoFixture +{ + core::PrivateKeys keys = core::PrivateKeys::CreateRandomKeys( + core::SIGNING_KEY_TYPE_EDDSA_SHA512_ED25519); +}; + +BOOST_FIXTURE_TEST_SUITE(RouterInfoTests, RouterInfoFixture) + +BOOST_AUTO_TEST_CASE(ValidSignature) +{ + // Ensure EdDSA router is created & signature verification succeeds + BOOST_CHECK_NO_THROW(core::RouterInfo r(keys, {{"127.0.0.1", 10701}}, {})); +} + +BOOST_AUTO_TEST_CASE(InvalidSignature) +{ + core::RouterInfo router; + + // Ensure default constructed router fails verification + BOOST_CHECK_THROW(router.Verify(), std::exception); + + // Create router buffer without setting default options + BOOST_CHECK_THROW(router.CreateBuffer(keys), std::exception); +} + +BOOST_AUTO_TEST_SUITE_END() From 580a5b82b4557622a1d6046e519528b5a29960a5 Mon Sep 17 00:00:00 2001 From: anonimal Date: Mon, 2 Jul 2018 21:50:23 +0000 Subject: [PATCH 4/8] Tests: cleanup RI test comments Referencing #917 --- tests/unit_tests/core/router/info.cc | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/tests/unit_tests/core/router/info.cc b/tests/unit_tests/core/router/info.cc index dcb61bbb..365df97f 100644 --- a/tests/unit_tests/core/router/info.cc +++ b/tests/unit_tests/core/router/info.cc @@ -34,8 +34,6 @@ #include "core/router/identity.h" -namespace core = kovri::core; - struct RouterInfoFixture { core::PrivateKeys keys = core::PrivateKeys::CreateRandomKeys( @@ -46,18 +44,14 @@ BOOST_FIXTURE_TEST_SUITE(RouterInfoTests, RouterInfoFixture) BOOST_AUTO_TEST_CASE(ValidSignature) { - // Ensure EdDSA router is created & signature verification succeeds BOOST_CHECK_NO_THROW(core::RouterInfo r(keys, {{"127.0.0.1", 10701}}, {})); } BOOST_AUTO_TEST_CASE(InvalidSignature) { + // If RI is not built completely, insufficient data will throw core::RouterInfo router; - - // Ensure default constructed router fails verification BOOST_CHECK_THROW(router.Verify(), std::exception); - - // Create router buffer without setting default options BOOST_CHECK_THROW(router.CreateBuffer(keys), std::exception); } From e89e992de93fdac171058f233505989cf2646c5c Mon Sep 17 00:00:00 2001 From: anonimal Date: Mon, 2 Jul 2018 21:54:55 +0000 Subject: [PATCH 5/8] RouterInfo: Verify: remove auto for data pointer Unclear contract (and auto* may as well be uint8_t*). Referencing #917 --- src/core/router/info.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/router/info.cc b/src/core/router/info.cc index 23b8e263..9f10af6d 100644 --- a/src/core/router/info.cc +++ b/src/core/router/info.cc @@ -711,7 +711,7 @@ void RouterInfo::Verify() std::size_t const len = m_Buffer.size() - m_RouterIdentity.GetSignatureLen(); if (len < Size::MinUnsignedBuffer) throw std::length_error("RouterInfo: invalid RouterInfo size"); - auto const buf = m_Buffer.data(); + const std::uint8_t* buf = m_Buffer.data(); if (!m_RouterIdentity.Verify(buf, len, &buf[len])) { m_IsUnreachable = true; From ed041ea676e742d36993e725aa5a53c6f7f21ef6 Mon Sep 17 00:00:00 2001 From: anonimal Date: Mon, 2 Jul 2018 21:56:35 +0000 Subject: [PATCH 6/8] RouterInfo: remove unnecessary null buffer check Buffer is always at least zero initialized. Referencing #917 --- src/core/router/info.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/core/router/info.cc b/src/core/router/info.cc index 9f10af6d..93d5829e 100644 --- a/src/core/router/info.cc +++ b/src/core/router/info.cc @@ -706,9 +706,8 @@ void RouterInfo::Verify() { try { - if (!m_Buffer.data()) - throw std::runtime_error("RouterInfo: null buffer"); - std::size_t const len = m_Buffer.size() - m_RouterIdentity.GetSignatureLen(); + std::size_t const len = + m_Buffer.size() - m_RouterIdentity.GetSignatureLen(); if (len < Size::MinUnsignedBuffer) throw std::length_error("RouterInfo: invalid RouterInfo size"); const std::uint8_t* buf = m_Buffer.data(); From 18b35268aa6f6eb6918c0f37fca8eb7b5d5c6774 Mon Sep 17 00:00:00 2001 From: anonimal Date: Mon, 2 Jul 2018 23:20:09 +0000 Subject: [PATCH 7/8] RouterInfo: don't throw when RI fails sig verify Sanely, we would want to throw here but doing so will stop the router instead of allowing the RI to become updated. Referencing #917 --- src/core/router/info.cc | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/core/router/info.cc b/src/core/router/info.cc index 93d5829e..22753658 100644 --- a/src/core/router/info.cc +++ b/src/core/router/info.cc @@ -712,10 +712,7 @@ void RouterInfo::Verify() throw std::length_error("RouterInfo: invalid RouterInfo size"); const std::uint8_t* buf = m_Buffer.data(); if (!m_RouterIdentity.Verify(buf, len, &buf[len])) - { - m_IsUnreachable = true; - throw std::runtime_error("RouterInfo: signature verification failed"); - } + m_IsUnreachable = true; } catch (...) { From 237e1f5b53e7ee48e577bb68990c74f758dd875f Mon Sep 17 00:00:00 2001 From: anonimal Date: Tue, 3 Jul 2018 03:27:38 +0000 Subject: [PATCH 8/8] RouterInfo: don't throw on invalid RI size Sanely, we would want to throw here but doing so will stop the router instead of allowing the RI to become updated. Referencing #917 --- src/core/router/info.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/core/router/info.cc b/src/core/router/info.cc index 22753658..7fd3145f 100644 --- a/src/core/router/info.cc +++ b/src/core/router/info.cc @@ -706,12 +706,14 @@ void RouterInfo::Verify() { try { + // Get RI length without signature std::size_t const len = m_Buffer.size() - m_RouterIdentity.GetSignatureLen(); - if (len < Size::MinUnsignedBuffer) - throw std::length_error("RouterInfo: invalid RouterInfo size"); + + // Confirm if valid and usable const std::uint8_t* buf = m_Buffer.data(); - if (!m_RouterIdentity.Verify(buf, len, &buf[len])) + if (len < Size::MinUnsignedBuffer + || !m_RouterIdentity.Verify(buf, len, &buf[len])) m_IsUnreachable = true; } catch (...)