Merge pull request #5639

2eef90d6 rpc: restrict the recent cutoff size in restricted RPC mode (moneromooo-monero)
0564da5f ensure no NULL is passed to memcpy (moneromooo-monero)
bc09766b abstract_tcp_server2: improve DoS resistance (moneromooo-monero)
1387549e serialization: check stream good flag at the end (moneromooo-monero)
a00cabd4 tree-hash: allocate variable memory on heap, not stack (moneromooo-monero)
f2152192 cryptonote: throw on tx hash calculation error (moneromooo-monero)
db2b9fba serialization: fail on read_varint error (moneromooo-monero)
68ad5481 cryptonote_protocol: fix another potential P2P DoS (moneromooo-monero)
1cc61018 cryptonote_protocol: expand basic DoS protection (moneromooo-monero)
8f66b705 cryptonote_protocol_handler: prevent potential DoS (anonimal)
39169ace epee: basic sanity check on allocation size from untrusted source (moneromooo-monero)

15 files changed, 78 insertions, 28 deletions

diff --git a/contrib/epee/include/net/abstract_tcp_server2.inl b/contrib/epee/include/net/abstract_tcp_server2.inl
index 821594355..fa5858b9f 100644
--- a/contrib/epee/include/net/abstract_tcp_server2.inl
+++ b/contrib/epee/include/net/abstract_tcp_server2.inl
@@ -54,6 +54,9 @@
 #undef MONERO_DEFAULT_LOG_CATEGORY
 #define MONERO_DEFAULT_LOG_CATEGORY "net"
 
+#define AGGRESSIVE_TIMEOUT_THRESHOLD 120 // sockets
+#define NEW_CONNECTION_TIMEOUT_LOCAL 1200000 // 2 minutes
+#define NEW_CONNECTION_TIMEOUT_REMOTE 10000 // 10 seconds
 #define DEFAULT_TIMEOUT_MS_LOCAL 1800000 // 30 minutes
 #define DEFAULT_TIMEOUT_MS_REMOTE 300000 // 5 minutes
 #define TIMEOUT_EXTRA_MS_PER_BYTE 0.2
@@ -189,7 +192,7 @@ PRAGMA_WARNING_DISABLE_VS(4355)
 
     m_protocol_handler.after_init_connection();
 
-    reset_timer(get_default_timeout(), false);
+    reset_timer(boost::posix_time::milliseconds(m_local ? NEW_CONNECTION_TIMEOUT_LOCAL : NEW_CONNECTION_TIMEOUT_REMOTE), false);
 
     // first read on the raw socket to detect SSL for the server
     buffer_ssl_init_fill = 0;
@@ -691,7 +694,7 @@ PRAGMA_WARNING_DISABLE_VS(4355)
   {
     unsigned count;
     try { count = host_count(m_host); } catch (...) { count = 0; }
-    const unsigned shift = std::min(std::max(count, 1u) - 1, 8u);
+    const unsigned shift = get_state().sock_count > AGGRESSIVE_TIMEOUT_THRESHOLD ? std::min(std::max(count, 1u) - 1, 8u) : 0;
     boost::posix_time::milliseconds timeout(0);
     if (m_local)
       timeout = boost::posix_time::milliseconds(DEFAULT_TIMEOUT_MS_LOCAL >> shift);
@@ -730,8 +733,6 @@ PRAGMA_WARNING_DISABLE_VS(4355)
   template<class t_protocol_handler>
   void connection<t_protocol_handler>::reset_timer(boost::posix_time::milliseconds ms, bool add)
   {
-    if (m_connection_type != e_connection_type_RPC)
-      return;
     MTRACE("Setting " << ms << " expiry");
     auto self = safe_shared_from_this();
     if(!self)
diff --git a/contrib/epee/include/storages/portable_storage_from_bin.h b/contrib/epee/include/storages/portable_storage_from_bin.h
index 2884f8c5e..e0a32b3ca 100644
--- a/contrib/epee/include/storages/portable_storage_from_bin.h
+++ b/contrib/epee/include/storages/portable_storage_from_bin.h
@@ -136,6 +136,7 @@ namespace epee
       //for pod types
       array_entry_t<type_name> sa;
       size_t size = read_varint();
+      CHECK_AND_ASSERT_THROW_MES(size <= m_count, "Size sanity check failed");
       sa.reserve(size);
       //TODO: add some optimization here later
       while(size--)
diff --git a/contrib/epee/src/buffer.cpp b/contrib/epee/src/buffer.cpp
index d637b905e..10ea6de56 100644
--- a/contrib/epee/src/buffer.cpp
+++ b/contrib/epee/src/buffer.cpp
@@ -64,7 +64,8 @@ void buffer::append(const void *data, size_t sz)
       size_t reserve = (((size() + sz) * 3 / 2) + 4095) & ~4095;
       new_storage.reserve(reserve);
       new_storage.resize(size());
-      memcpy(new_storage.data(), storage.data() + offset, storage.size() - offset);
+      if (size() > 0)
+        memcpy(new_storage.data(), storage.data() + offset, storage.size() - offset);
       offset = 0;
       std::swap(storage, new_storage);
     }
diff --git a/contrib/epee/src/wipeable_string.cpp b/contrib/epee/src/wipeable_string.cpp
index 3a6ee5dac..4209b71bf 100644
--- a/contrib/epee/src/wipeable_string.cpp
+++ b/contrib/epee/src/wipeable_string.cpp
@@ -62,13 +62,15 @@ wipeable_string::wipeable_string(wipeable_string &&other)
 wipeable_string::wipeable_string(const std::string &other)
 {
   grow(other.size());
-  memcpy(buffer.data(), other.c_str(), size());
+  if (size() > 0)
+    memcpy(buffer.data(), other.c_str(), size());
 }
 
 wipeable_string::wipeable_string(std::string &&other)
 {
   grow(other.size());
-  memcpy(buffer.data(), other.c_str(), size());
+  if (size() > 0)
+    memcpy(buffer.data(), other.c_str(), size());
   if (!other.empty())
   {
     memwipe(&other[0], other.size()); // we're kinda left with this again aren't we
@@ -79,7 +81,8 @@ wipeable_string::wipeable_string(std::string &&other)
 wipeable_string::wipeable_string(const char *s)
 {
   grow(strlen(s));
-  memcpy(buffer.data(), s, size());
+  if (size() > 0)
+    memcpy(buffer.data(), s, size());
 }
 
 wipeable_string::wipeable_string(const char *s, size_t len)
@@ -112,14 +115,18 @@ void wipeable_string::grow(size_t sz, size_t reserved)
   }
   size_t old_sz = buffer.size();
   std::unique_ptr<char[]> tmp{new char[old_sz]};
-  memcpy(tmp.get(), buffer.data(), old_sz * sizeof(char));
   if (old_sz > 0)
+  {
+    memcpy(tmp.get(), buffer.data(), old_sz * sizeof(char));
     memwipe(buffer.data(), old_sz * sizeof(char));
+  }
   buffer.reserve(reserved);
   buffer.resize(sz);
-  memcpy(buffer.data(), tmp.get(), old_sz * sizeof(char));
   if (old_sz > 0)
+  {
+    memcpy(buffer.data(), tmp.get(), old_sz * sizeof(char));
     memwipe(tmp.get(), old_sz * sizeof(char));
+  }
 }
 
 void wipeable_string::push_back(char c)
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;
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 <assert.h>
 #include <stddef.h>
+#include <stdlib.h>
 #include <string.h>
 
 #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);
   }
 }
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<W>::is_saving())
         pruned = true;
-      return true;
+      return ar.stream().good();
     }
 
   private:
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);
diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.h b/src/cryptonote_protocol/cryptonote_protocol_handler.h
index 0927b5d7f..dcc5ec6ed 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_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 8958af7c7..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<bool> 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
       {
@@ -914,6 +929,17 @@ namespace cryptonote
   int t_cryptonote_protocol_handler<t_core>::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() + arg.txs.size() > CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT)
+      {
+        LOG_ERROR_CCONTEXT(
+            "Requested objects count is too big ("
+            << arg.blocks.size() + arg.txs.size() << ") expected not more then "
+            << CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT);
+        drop_connection(context, false, false);
+        return 1;
+      }
+
     NOTIFY_RESPONSE_GET_OBJECTS::request rsp;
     if(!m_core.handle_get_objects(arg, rsp, context))
     {
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<W> &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/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<COMMAND_RPC_GET_OUTPUT_HISTOGRAM>(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<uint64_t, std::tuple<uint64_t, uint64_t, uint64_t>> histogram;
     try
     {
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<false> : public binary_archive_base<std::istream, false>
   void serialize_uvarint(T &v)
   {
     typedef std::istreambuf_iterator<char> 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)
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)