From 8f66b7053a8e4521fdd68c1d74718e73345fb158 Mon Sep 17 00:00:00 2001 From: anonimal Date: Sat, 9 Mar 2019 09:11:44 +0000 Subject: cryptonote_protocol_handler: prevent potential DoS Essentially, one can send such a large amount of IDs that core exhausts all free memory. This issue can theoretically be exploited using very large CN blockchains, such as Monero. This is a partial fix. Thanks and credit given to CryptoNote author 'cryptozoidberg' for collaboration and the fix. Also thanks to 'moneromooo'. Referencing HackerOne report #506595. --- src/cryptonote_protocol/cryptonote_protocol_handler.h | 1 + src/cryptonote_protocol/cryptonote_protocol_handler.inl | 11 +++++++++++ 2 files changed, 12 insertions(+) (limited to 'src') diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.h b/src/cryptonote_protocol/cryptonote_protocol_handler.h index 0927b5d7f..f1fd69960 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.h +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.h @@ -52,6 +52,7 @@ PUSH_WARNINGS DISABLE_VS_WARNINGS(4355) #define LOCALHOST_INT 2130706433 +#define CURRENCY_PROTOCOL_MAX_BLOCKS_REQUEST_COUNT 500 namespace cryptonote { diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl index 8958af7c7..0eacc7cd4 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl @@ -914,6 +914,17 @@ namespace cryptonote int t_cryptonote_protocol_handler::handle_request_get_objects(int command, NOTIFY_REQUEST_GET_OBJECTS::request& arg, cryptonote_connection_context& context) { MLOG_P2P_MESSAGE("Received NOTIFY_REQUEST_GET_OBJECTS (" << arg.blocks.size() << " blocks, " << arg.txs.size() << " txes)"); + + if (arg.blocks.size() > CURRENCY_PROTOCOL_MAX_BLOCKS_REQUEST_COUNT) + { + LOG_ERROR_CCONTEXT( + "Requested objects count is too big (" + << arg.blocks.size() << ") expected not more then " + << CURRENCY_PROTOCOL_MAX_BLOCKS_REQUEST_COUNT); + drop_connection(context, false, false); + return 1; + } + NOTIFY_RESPONSE_GET_OBJECTS::request rsp; if(!m_core.handle_get_objects(arg, rsp, context)) { -- cgit v1.2.3 From 1cc61018e52893382ae110430b38d1e0bf6e5870 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Fri, 8 Mar 2019 19:17:20 +0000 Subject: cryptonote_protocol: expand basic DoS protection Count transactions as well --- src/cryptonote_protocol/cryptonote_protocol_handler.h | 2 +- src/cryptonote_protocol/cryptonote_protocol_handler.inl | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.h b/src/cryptonote_protocol/cryptonote_protocol_handler.h index f1fd69960..dcc5ec6ed 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.h +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.h @@ -52,7 +52,7 @@ PUSH_WARNINGS DISABLE_VS_WARNINGS(4355) #define LOCALHOST_INT 2130706433 -#define CURRENCY_PROTOCOL_MAX_BLOCKS_REQUEST_COUNT 500 +#define CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT 500 namespace cryptonote { diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl index 0eacc7cd4..78acb3179 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl @@ -915,12 +915,12 @@ namespace cryptonote { MLOG_P2P_MESSAGE("Received NOTIFY_REQUEST_GET_OBJECTS (" << arg.blocks.size() << " blocks, " << arg.txs.size() << " txes)"); - if (arg.blocks.size() > CURRENCY_PROTOCOL_MAX_BLOCKS_REQUEST_COUNT) + if (arg.blocks.size() + arg.txs.size() > CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT) { LOG_ERROR_CCONTEXT( "Requested objects count is too big (" - << arg.blocks.size() << ") expected not more then " - << CURRENCY_PROTOCOL_MAX_BLOCKS_REQUEST_COUNT); + << arg.blocks.size() + arg.txs.size() << ") expected not more then " + << CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT); drop_connection(context, false, false); return 1; } -- cgit v1.2.3 From 68ad548193134c111c75382939f7d77dbd15ad3f Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Sat, 9 Mar 2019 09:08:53 +0000 Subject: cryptonote_protocol: fix another potential P2P DoS When asking for txes in a fluffy transaction, one might ask for the same (large) tx many times --- src/cryptonote_protocol/cryptonote_protocol_handler.inl | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 'src') diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl index 78acb3179..b38407840 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl @@ -809,12 +809,27 @@ namespace cryptonote NOTIFY_NEW_FLUFFY_BLOCK::request fluffy_response; fluffy_response.b.block = t_serializable_object_to_blob(b); fluffy_response.current_blockchain_height = arg.current_blockchain_height; + std::vector seen(b.tx_hashes.size(), false); for(auto& tx_idx: arg.missing_tx_indices) { if(tx_idx < b.tx_hashes.size()) { MDEBUG(" tx " << b.tx_hashes[tx_idx]); + if (seen[tx_idx]) + { + LOG_ERROR_CCONTEXT + ( + "Failed to handle request NOTIFY_REQUEST_FLUFFY_MISSING_TX" + << ", request is asking for duplicate tx " + << ", tx index = " << tx_idx << ", block tx count " << b.tx_hashes.size() + << ", block_height = " << arg.current_blockchain_height + << ", dropping connection" + ); + drop_connection(context, true, false); + return 1; + } txids.push_back(b.tx_hashes[tx_idx]); + seen[tx_idx] = true; } else { -- cgit v1.2.3 From db2b9fba651670a9c8f86c316f36bc9d9dbb82cc Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Mon, 11 Mar 2019 23:36:17 +0000 Subject: serialization: fail on read_varint error --- src/serialization/binary_archive.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/serialization/binary_archive.h b/src/serialization/binary_archive.h index a0e4eff9d..9f60cf311 100644 --- a/src/serialization/binary_archive.h +++ b/src/serialization/binary_archive.h @@ -146,7 +146,8 @@ struct binary_archive : public binary_archive_base void serialize_uvarint(T &v) { typedef std::istreambuf_iterator it; - tools::read_varint(it(stream_), it(), v); // XXX handle failure + if (tools::read_varint(it(stream_), it(), v) < 0) + stream_.setstate(std::ios_base::failbit); } void begin_array(size_t &s) -- cgit v1.2.3 From f2152192529a6e307ac2befeaa8c4887544697da Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Tue, 19 Mar 2019 01:07:10 +0000 Subject: cryptonote: throw on tx hash calculation error --- src/cryptonote_basic/cryptonote_format_utils.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/cryptonote_basic/cryptonote_format_utils.cpp b/src/cryptonote_basic/cryptonote_format_utils.cpp index 566622c1a..7d7de416d 100644 --- a/src/cryptonote_basic/cryptonote_format_utils.cpp +++ b/src/cryptonote_basic/cryptonote_format_utils.cpp @@ -221,8 +221,7 @@ namespace cryptonote tx.invalidate_hashes(); //TODO: validate tx - get_transaction_hash(tx, tx_hash); - return true; + return get_transaction_hash(tx, tx_hash); } //--------------------------------------------------------------- bool parse_and_validate_tx_from_blob(const blobdata& tx_blob, transaction& tx, crypto::hash& tx_hash, crypto::hash& tx_prefix_hash) @@ -975,6 +974,7 @@ namespace cryptonote { crypto::hash h = null_hash; get_transaction_hash(t, h, NULL); + CHECK_AND_ASSERT_THROW_MES(get_transaction_hash(t, h, NULL), "Failed to calculate transaction hash"); return h; } //--------------------------------------------------------------- @@ -1327,7 +1327,7 @@ namespace cryptonote txs_ids.reserve(1 + b.tx_hashes.size()); crypto::hash h = null_hash; size_t bl_sz = 0; - get_transaction_hash(b.miner_tx, h, bl_sz); + CHECK_AND_ASSERT_THROW_MES(get_transaction_hash(b.miner_tx, h, bl_sz), "Failed to calculate transaction hash"); txs_ids.push_back(h); for(auto& th: b.tx_hashes) txs_ids.push_back(th); -- cgit v1.2.3 From a00cabd4f343cc10a1ccf6713c7ee2c9fa1496ea Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Thu, 4 Apr 2019 00:15:07 +0000 Subject: tree-hash: allocate variable memory on heap, not stack Large amounts might run out of stack Reported by guidov --- src/crypto/tree-hash.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/crypto/tree-hash.c b/src/crypto/tree-hash.c index 7802fb67f..0a5860f3b 100644 --- a/src/crypto/tree-hash.c +++ b/src/crypto/tree-hash.c @@ -30,6 +30,7 @@ #include #include +#include #include #include "hash-ops.h" @@ -82,23 +83,24 @@ void tree_hash(const char (*hashes)[HASH_SIZE], size_t count, char *root_hash) { size_t cnt = tree_hash_cnt( count ); - char ints[cnt][HASH_SIZE]; - memset(ints, 0 , sizeof(ints)); // zero out as extra protection for using uninitialized mem + char *ints = calloc(cnt, HASH_SIZE); // zero out as extra protection for using uninitialized mem + assert(ints); memcpy(ints, hashes, (2 * cnt - count) * HASH_SIZE); for (i = 2 * cnt - count, j = 2 * cnt - count; j < cnt; i += 2, ++j) { - cn_fast_hash(hashes[i], 64, ints[j]); + cn_fast_hash(hashes[i], 64, ints + j * HASH_SIZE); } assert(i == count); while (cnt > 2) { cnt >>= 1; for (i = 0, j = 0; j < cnt; i += 2, ++j) { - cn_fast_hash(ints[i], 64, ints[j]); + cn_fast_hash(ints + i * HASH_SIZE, 64, ints + j * HASH_SIZE); } } - cn_fast_hash(ints[0], 64, root_hash); + cn_fast_hash(ints, 64, root_hash); + free(ints); } } -- cgit v1.2.3 From 1387549e905fc206426d3099b069bd28df0aad71 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Tue, 16 Apr 2019 18:48:40 +0000 Subject: serialization: check stream good flag at the end just in case --- src/cryptonote_basic/cryptonote_basic.h | 2 +- src/ringct/rctTypes.h | 8 ++++---- src/serialization/serialization.h | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/cryptonote_basic/cryptonote_basic.h b/src/cryptonote_basic/cryptonote_basic.h index 20d92bdf1..055c4a22b 100644 --- a/src/cryptonote_basic/cryptonote_basic.h +++ b/src/cryptonote_basic/cryptonote_basic.h @@ -320,7 +320,7 @@ namespace cryptonote } if (!typename Archive::is_saving()) pruned = true; - return true; + return ar.stream().good(); } private: diff --git a/src/ringct/rctTypes.h b/src/ringct/rctTypes.h index e5413f1dc..f8729b872 100644 --- a/src/ringct/rctTypes.h +++ b/src/ringct/rctTypes.h @@ -252,7 +252,7 @@ namespace rct { { FIELD(type) if (type == RCTTypeNull) - return true; + return ar.stream().good(); if (type != RCTTypeFull && type != RCTTypeSimple && type != RCTTypeBulletproof && type != RCTTypeBulletproof2) return false; VARINT_FIELD(txnFee) @@ -312,7 +312,7 @@ namespace rct { ar.delimit_array(); } ar.end_array(); - return true; + return ar.stream().good(); } }; struct rctSigPrunable { @@ -325,7 +325,7 @@ namespace rct { bool serialize_rctsig_prunable(Archive &ar, uint8_t type, size_t inputs, size_t outputs, size_t mixin) { if (type == RCTTypeNull) - return true; + return ar.stream().good(); if (type != RCTTypeFull && type != RCTTypeSimple && type != RCTTypeBulletproof && type != RCTTypeBulletproof2) return false; if (type == RCTTypeBulletproof || type == RCTTypeBulletproof2) @@ -429,7 +429,7 @@ namespace rct { } ar.end_array(); } - return true; + return ar.stream().good(); } }; diff --git a/src/serialization/serialization.h b/src/serialization/serialization.h index 007bf265f..553e9951f 100644 --- a/src/serialization/serialization.h +++ b/src/serialization/serialization.h @@ -212,7 +212,7 @@ inline bool do_serialize(Archive &ar, bool &v) * \brief self-explanatory */ #define END_SERIALIZE() \ - return true; \ + return ar.stream().good(); \ } /*! \macro VALUE(f) -- cgit v1.2.3 From 0564da5fdc165948ba7c862fb81478f9287a072d Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Mon, 29 Apr 2019 08:17:32 +0000 Subject: ensure no NULL is passed to memcpy NULL is valid when size is 0, but memcpy uses nonnull attributes, so let's not poke the bear --- src/blockchain_db/lmdb/db_lmdb.cpp | 4 ++-- src/crypto/keccak.c | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/blockchain_db/lmdb/db_lmdb.cpp b/src/blockchain_db/lmdb/db_lmdb.cpp index a03a0989b..f05eb0f30 100644 --- a/src/blockchain_db/lmdb/db_lmdb.cpp +++ b/src/blockchain_db/lmdb/db_lmdb.cpp @@ -1077,11 +1077,11 @@ void BlockchainLMDB::add_tx_amount_output_indices(const uint64_t tx_id, int result = 0; - int num_outputs = amount_output_indices.size(); + size_t num_outputs = amount_output_indices.size(); MDB_val_set(k_tx_id, tx_id); MDB_val v; - v.mv_data = (void *)amount_output_indices.data(); + v.mv_data = num_outputs ? (void *)amount_output_indices.data() : (void*)""; v.mv_size = sizeof(uint64_t) * num_outputs; // LOG_PRINT_L1("tx_outputs[tx_hash] size: " << v.mv_size); diff --git a/src/crypto/keccak.c b/src/crypto/keccak.c index 170911262..18ed3152f 100644 --- a/src/crypto/keccak.c +++ b/src/crypto/keccak.c @@ -116,7 +116,8 @@ void keccak(const uint8_t *in, size_t inlen, uint8_t *md, int mdlen) local_abort("Bad keccak use"); } - memcpy(temp, in, inlen); + if (inlen > 0) + memcpy(temp, in, inlen); temp[inlen++] = 1; memset(temp + inlen, 0, rsiz - inlen); temp[rsiz - 1] |= 0x80; -- cgit v1.2.3 From 2eef90d6ef40a9ba8e08ad67b270da1edc7c3ddd Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Sun, 12 May 2019 13:27:34 +0000 Subject: rpc: restrict the recent cutoff size in restricted RPC mode --- src/rpc/core_rpc_server.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'src') diff --git a/src/rpc/core_rpc_server.cpp b/src/rpc/core_rpc_server.cpp index 28c53d6e3..3db138719 100644 --- a/src/rpc/core_rpc_server.cpp +++ b/src/rpc/core_rpc_server.cpp @@ -59,6 +59,8 @@ using namespace epee; #define MAX_RESTRICTED_FAKE_OUTS_COUNT 40 #define MAX_RESTRICTED_GLOBAL_FAKE_OUTS_COUNT 5000 +#define OUTPUT_HISTOGRAM_RECENT_CUTOFF_RESTRICTION (3 * 86400) // 3 days max, the wallet requests 1.8 days + namespace { void add_reason(std::string &reasons, const char *reason) @@ -1882,6 +1884,13 @@ namespace cryptonote if (use_bootstrap_daemon_if_necessary(invoke_http_mode::JON_RPC, "get_output_histogram", req, res, r)) return r; + const bool restricted = m_restricted && ctx; + if (restricted && req.recent_cutoff > 0 && req.recent_cutoff < (uint64_t)time(NULL) - OUTPUT_HISTOGRAM_RECENT_CUTOFF_RESTRICTION) + { + res.status = "Recent cutoff is too old"; + return true; + } + std::map> histogram; try { -- cgit v1.2.3