From 4e2b2b942daa4206ec44c66e59863670dfe3fde4 Mon Sep 17 00:00:00 2001 From: Riccardo Spagni Date: Wed, 24 Sep 2014 22:28:25 +0200 Subject: [PATCH 1/3] low risk, potentially varint overflow bug patched thanks to BBR --- src/common/varint.h | 25 ++++++++++++++- src/cryptonote_core/cryptonote_basic.h | 1 - src/cryptonote_core/cryptonote_core.cpp | 8 ++--- src/cryptonote_core/cryptonote_core.h | 2 +- .../cryptonote_format_utils.cpp | 17 ++++++++++ src/cryptonote_core/cryptonote_format_utils.h | 2 ++ src/cryptonote_core/tx_pool.cpp | 9 +++--- src/cryptonote_core/tx_pool.h | 4 +-- tests/unit_tests/serialization.cpp | 31 +++++++++++++++++++ 9 files changed, 85 insertions(+), 14 deletions(-) diff --git a/src/common/varint.h b/src/common/varint.h index 5a73c2746..57386597a 100644 --- a/src/common/varint.h +++ b/src/common/varint.h @@ -36,7 +36,7 @@ #include #include /*! \file varint.h - * \breif provides the implementation of varint's + * \brief provides the implementation of varint's * * The representation of varints is rather odd. The first bit of each * octet is significant, it represents wheter there is another part @@ -52,6 +52,29 @@ namespace tools { + template + size_t get_varint_packed_size(T v) + { + if(v <= 127) + return 1; + else if(v <= 16383) + return 2; + else if(v <= 2097151) + return 3; + else if(v <= 268435455) + return 4; + else if(v <= 34359738367) + return 5; + else if(v <= 4398046511103) + return 6; + else if(v <= 4398046511103) + return 6; + else if(v <= 562949953421311) + return 7; + else + return 8; + } + /*! \brief Error codes for varint */ enum { diff --git a/src/cryptonote_core/cryptonote_basic.h b/src/cryptonote_core/cryptonote_basic.h index dfdfa03e5..fca4e227e 100644 --- a/src/cryptonote_core/cryptonote_basic.h +++ b/src/cryptonote_core/cryptonote_basic.h @@ -226,7 +226,6 @@ namespace cryptonote ar.end_array(); END_SERIALIZE() - private: static size_t get_signature_size(const txin_v& tx_in); }; diff --git a/src/cryptonote_core/cryptonote_core.cpp b/src/cryptonote_core/cryptonote_core.cpp index 964d61e2f..e6aa679d7 100644 --- a/src/cryptonote_core/cryptonote_core.cpp +++ b/src/cryptonote_core/cryptonote_core.cpp @@ -191,7 +191,7 @@ namespace cryptonote return false; } - bool r = add_new_tx(tx, tx_hash, tx_prefixt_hash, tx_blob.size(), tvc, keeped_by_block); + bool r = add_new_tx(tx, tx_hash, tx_prefixt_hash, tvc, keeped_by_block); if(tvc.m_verifivation_failed) {LOG_PRINT_RED_L1("Transaction verification failed: " << tx_hash);} else if(tvc.m_verifivation_impossible) @@ -284,7 +284,7 @@ namespace cryptonote crypto::hash tx_prefix_hash = get_transaction_prefix_hash(tx); blobdata bl; t_serializable_object_to_blob(tx, bl); - return add_new_tx(tx, tx_hash, tx_prefix_hash, bl.size(), tvc, keeped_by_block); + return add_new_tx(tx, tx_hash, tx_prefix_hash, tvc, keeped_by_block); } //----------------------------------------------------------------------------------------------- size_t core::get_blockchain_total_transactions() @@ -297,7 +297,7 @@ namespace cryptonote // return m_blockchain_storage.get_outs(amount, pkeys); //} //----------------------------------------------------------------------------------------------- - bool core::add_new_tx(const transaction& tx, const crypto::hash& tx_hash, const crypto::hash& tx_prefix_hash, size_t blob_size, tx_verification_context& tvc, bool keeped_by_block) + bool core::add_new_tx(const transaction& tx, const crypto::hash& tx_hash, const crypto::hash& tx_prefix_hash, tx_verification_context& tvc, bool keeped_by_block) { if(m_mempool.have_tx(tx_hash)) { @@ -311,7 +311,7 @@ namespace cryptonote return true; } - return m_mempool.add_tx(tx, tx_hash, blob_size, tvc, keeped_by_block); + return m_mempool.add_tx(tx, tx_hash, tvc, keeped_by_block); } //----------------------------------------------------------------------------------------------- bool core::get_block_template(block& b, const account_public_address& adr, difficulty_type& diffic, uint64_t& height, const blobdata& ex_nonce) diff --git a/src/cryptonote_core/cryptonote_core.h b/src/cryptonote_core/cryptonote_core.h index ba2aed015..e9ab20114 100644 --- a/src/cryptonote_core/cryptonote_core.h +++ b/src/cryptonote_core/cryptonote_core.h @@ -120,7 +120,7 @@ namespace cryptonote uint64_t get_target_blockchain_height() const; private: - bool add_new_tx(const transaction& tx, const crypto::hash& tx_hash, const crypto::hash& tx_prefix_hash, size_t blob_size, tx_verification_context& tvc, bool keeped_by_block); + bool add_new_tx(const transaction& tx, const crypto::hash& tx_hash, const crypto::hash& tx_prefix_hash, tx_verification_context& tvc, bool keeped_by_block); bool add_new_tx(const transaction& tx, tx_verification_context& tvc, bool keeped_by_block); bool add_new_block(const block& b, block_verification_context& bvc); bool load_state_data(); diff --git a/src/cryptonote_core/cryptonote_format_utils.cpp b/src/cryptonote_core/cryptonote_format_utils.cpp index 33cad30c4..e46bd390d 100644 --- a/src/cryptonote_core/cryptonote_format_utils.cpp +++ b/src/cryptonote_core/cryptonote_format_utils.cpp @@ -742,6 +742,23 @@ namespace cryptonote return true; } //--------------------------------------------------------------- + size_t get_object_blobsize(const transaction& t) + { + size_t prefix_blob = get_object_blobsize(static_cast(t)); + + if(is_coinbase(t)) + return prefix_blob; + + for(const auto& in: t.vin) + { + size_t sig_count = transaction::get_signature_size(in); + prefix_blob += 64*sig_count; + prefix_blob += tools::get_varint_packed_size(sig_count); + } + prefix_blob += tools::get_varint_packed_size(t.vin.size()); + return prefix_blob; + } + //--------------------------------------------------------------- blobdata block_to_blob(const block& b) { return t_serializable_object_to_blob(b); diff --git a/src/cryptonote_core/cryptonote_format_utils.h b/src/cryptonote_core/cryptonote_format_utils.h index 9fa7f0545..58186b8cf 100644 --- a/src/cryptonote_core/cryptonote_format_utils.h +++ b/src/cryptonote_core/cryptonote_format_utils.h @@ -157,6 +157,8 @@ namespace cryptonote return b.size(); } //--------------------------------------------------------------- + size_t get_object_blobsize(const transaction& t); + //--------------------------------------------------------------- template bool get_object_hash(const t_object& o, crypto::hash& res, size_t& blob_size) { diff --git a/src/cryptonote_core/tx_pool.cpp b/src/cryptonote_core/tx_pool.cpp index 81f932014..3764cfe77 100644 --- a/src/cryptonote_core/tx_pool.cpp +++ b/src/cryptonote_core/tx_pool.cpp @@ -59,9 +59,9 @@ namespace cryptonote } //--------------------------------------------------------------------------------- - bool tx_memory_pool::add_tx(const transaction &tx, /*const crypto::hash& tx_prefix_hash,*/ const crypto::hash &id, size_t blob_size, tx_verification_context& tvc, bool kept_by_block) + bool tx_memory_pool::add_tx(const transaction &tx, const crypto::hash &id, tx_verification_context& tvc, bool kept_by_block) { - + size_t blob_size = get_object_blobsize(tx); if(!check_inputs_types_supported(tx)) { @@ -179,9 +179,8 @@ namespace cryptonote bool tx_memory_pool::add_tx(const transaction &tx, tx_verification_context& tvc, bool keeped_by_block) { crypto::hash h = null_hash; - size_t blob_size = 0; - get_transaction_hash(tx, h, blob_size); - return add_tx(tx, h, blob_size, tvc, keeped_by_block); + get_transaction_hash(tx, h); + return add_tx(tx, h, tvc, keeped_by_block); } //--------------------------------------------------------------------------------- bool tx_memory_pool::remove_transaction_keyimages(const transaction& tx) diff --git a/src/cryptonote_core/tx_pool.h b/src/cryptonote_core/tx_pool.h index 7617765cc..fb623a3d2 100644 --- a/src/cryptonote_core/tx_pool.h +++ b/src/cryptonote_core/tx_pool.h @@ -56,7 +56,7 @@ namespace cryptonote { public: tx_memory_pool(blockchain_storage& bchs); - bool add_tx(const transaction &tx, const crypto::hash &id, size_t blob_size, tx_verification_context& tvc, bool keeped_by_block); + bool add_tx(const transaction &tx, const crypto::hash &id, tx_verification_context& tvc, bool keeped_by_block); bool add_tx(const transaction &tx, tx_verification_context& tvc, bool keeped_by_block); //gets tx and remove it from pool bool take_tx(const crypto::hash &id, transaction &tx, size_t& blob_size, uint64_t& fee); @@ -81,7 +81,7 @@ namespace cryptonote /*bool flush_pool(const std::strig& folder); bool inflate_pool(const std::strig& folder);*/ -#define CURRENT_MEMPOOL_ARCHIVE_VER 8 +#define CURRENT_MEMPOOL_ARCHIVE_VER 9 template void serialize(archive_t & a, const unsigned int version) diff --git a/tests/unit_tests/serialization.cpp b/tests/unit_tests/serialization.cpp index 4a254b5a4..3ae5bcdf6 100644 --- a/tests/unit_tests/serialization.cpp +++ b/tests/unit_tests/serialization.cpp @@ -299,6 +299,37 @@ namespace } } +bool test_get_varint_packed_size_for_num(uint64_t n) +{ +std::stringstream ss; +typedef std::ostreambuf_iterator it; +tools::write_varint(it(ss), n); +uint64_t sz = ss.str().size(); +if(sz != tools::get_varint_packed_size(n)) +return false; +else +return true; +} +TEST(Serialization, validate_get_varint_packed_size) +{ +ASSERT_TRUE(test_get_varint_packed_size_for_num(127)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(128)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(16383)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(16383+1)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(2097151)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(2097151+1)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(268435455)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(268435455+1)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(34359738367)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(34359738367+1)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(4398046511103)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(4398046511103+1)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(4398046511103)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(4398046511103+1)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(562949953421311)); +ASSERT_TRUE(test_get_varint_packed_size_for_num(562949953421311+1)); +} + TEST(Serialization, serializes_transacion_signatures_correctly) { using namespace cryptonote; From 22481244535e05bb308eedf9c2d2a5abe2d79a9d Mon Sep 17 00:00:00 2001 From: Thomas Winget Date: Wed, 24 Sep 2014 17:15:49 -0400 Subject: [PATCH 2/3] Removed ldns dependency ldns dependency was only still around for constants defined in ldns/rr.h, but those constants are RFC specified DNS constants, and to reduce deps have been replicated in dns_utils.h instead of including ldns/rr.h. --- src/common/dns_utils.cpp | 7 +++---- src/common/dns_utils.h | 6 ++++++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/common/dns_utils.cpp b/src/common/dns_utils.cpp index e1151e190..4b43c7492 100644 --- a/src/common/dns_utils.cpp +++ b/src/common/dns_utils.cpp @@ -29,7 +29,6 @@ #include "common/dns_utils.h" #include #include -#include // for RR type and class defs #include namespace tools @@ -135,7 +134,7 @@ std::vector DNSResolver::get_ipv4(const std::string& url) ub_result_ptr result; // call DNS resolver, blocking. if return value not zero, something went wrong - if (!ub_resolve(m_data->m_ub_context, urlC, LDNS_RR_TYPE_A, LDNS_RR_CLASS_IN, &(result.ptr))) + if (!ub_resolve(m_data->m_ub_context, urlC, DNS_TYPE_A, DNS_CLASS_IN, &(result.ptr))) { if (result.ptr->havedata) { @@ -165,7 +164,7 @@ std::vector DNSResolver::get_ipv6(const std::string& url) ub_result_ptr result; // call DNS resolver, blocking. if return value not zero, something went wrong - if (!ub_resolve(m_data->m_ub_context, urlC, LDNS_RR_TYPE_AAAA, LDNS_RR_CLASS_IN, &(result.ptr))) + if (!ub_resolve(m_data->m_ub_context, urlC, DNS_TYPE_AAAA, DNS_CLASS_IN, &(result.ptr))) { if (result.ptr->havedata) { @@ -195,7 +194,7 @@ std::vector DNSResolver::get_txt_record(const std::string& url) ub_result_ptr result; // call DNS resolver, blocking. if return value not zero, something went wrong - if (!ub_resolve(m_data->m_ub_context, urlC, LDNS_RR_TYPE_TXT, LDNS_RR_CLASS_IN, &(result.ptr))) + if (!ub_resolve(m_data->m_ub_context, urlC, DNS_TYPE_TXT, DNS_CLASS_IN, &(result.ptr))) { if (result.ptr->havedata) { diff --git a/src/common/dns_utils.h b/src/common/dns_utils.h index 375f6ba30..d22e3fb1f 100644 --- a/src/common/dns_utils.h +++ b/src/common/dns_utils.h @@ -32,6 +32,12 @@ namespace tools { +// RFC defines for record types and classes for DNS, gleaned from ldns source +const static int DNS_CLASS_IN = 1; +const static int DNS_TYPE_A = 1; +const static int DNS_TYPE_TXT = 6; +const static int DNS_TYPE_AAAA = 8; + struct DNSResolverData; /** From fab95aef64e2e5ba209ca2f27763a8db489d4294 Mon Sep 17 00:00:00 2001 From: Thomas Winget Date: Wed, 24 Sep 2014 23:19:14 -0400 Subject: [PATCH 3/3] Remove LDNS dep and fix a bug in libunbound const correctness fix --- src/common/dns_utils.cpp | 6 +++--- src/common/dns_utils.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/common/dns_utils.cpp b/src/common/dns_utils.cpp index 4b43c7492..346761e74 100644 --- a/src/common/dns_utils.cpp +++ b/src/common/dns_utils.cpp @@ -125,7 +125,7 @@ std::vector DNSResolver::get_ipv4(const std::string& url) strncpy(urlC, url.c_str(), 999); urlC[999] = '\0'; - if (!check_address_syntax(url)) + if (!check_address_syntax(urlC)) { return addresses; } @@ -156,7 +156,7 @@ std::vector DNSResolver::get_ipv6(const std::string& url) strncpy(urlC, url.c_str(), 999); urlC[999] = '\0'; - if (!check_address_syntax(url)) + if (!check_address_syntax(urlC)) { return addresses; } @@ -186,7 +186,7 @@ std::vector DNSResolver::get_txt_record(const std::string& url) strncpy(urlC, url.c_str(), 999); urlC[999] = '\0'; - if (!check_address_syntax(url)) + if (!check_address_syntax(urlC)) { return records; } diff --git a/src/common/dns_utils.h b/src/common/dns_utils.h index d22e3fb1f..dd6946dc4 100644 --- a/src/common/dns_utils.h +++ b/src/common/dns_utils.h @@ -35,7 +35,7 @@ namespace tools // RFC defines for record types and classes for DNS, gleaned from ldns source const static int DNS_CLASS_IN = 1; const static int DNS_TYPE_A = 1; -const static int DNS_TYPE_TXT = 6; +const static int DNS_TYPE_TXT = 16; const static int DNS_TYPE_AAAA = 8; struct DNSResolverData;