cryptonote_basic: serialization checks

(cherry picked from commit d869dc0981ac55f90d5b52a1a794b124ae274716)

src/cryptonote_basic/cryptonote_basic.h

@@ -53,8 +53,12 @@
 
 namespace cryptonote
 {
-  typedef std::vector<crypto::signature> ring_signature;
-
+  // These are guaranteed ceilings for the entire chain. Some fork rules may limit some further.
+  constexpr size_t COINBASE_VIN_COUNT          = 1;
+  constexpr size_t MAX_VIN_COUNT               = CRYPTONOTE_MAX_TX_SIZE / sizeof(crypto::key_image);
+  constexpr size_t MAX_NON_COINBASE_VOUT_COUNT = CRYPTONOTE_MAX_TX_SIZE / sizeof(crypto::public_key);
+  constexpr size_t MAX_COINBASE_VOUT_COUNT     = /*LEVIN_DEFAULT_MAX_PACKET_SIZE=*/100000000 / sizeof(crypto::public_key);
+  constexpr size_t MAX_TOTAL_KEY_OFFSETS       = CRYPTONOTE_MAX_TX_SIZE / sizeof(crypto::public_key);
 
   /* outputs */
 
@@ -143,7 +147,7 @@ namespace cryptonote
 
     BEGIN_SERIALIZE_OBJECT()
       VARINT_FIELD(amount)
-      FIELD(key_offsets)
+      CONTAINER_FIELD_CAPPED(key_offsets, MAX_TOTAL_KEY_OFFSETS)
       FIELD(k_image)
     END_SERIALIZE()
   };
@@ -184,11 +188,109 @@ namespace cryptonote
       VARINT_FIELD(version)
       if(version == 0 || CURRENT_TRANSACTION_VERSION < version) return false;
       VARINT_FIELD(unlock_time)
-      FIELD(vin)
-      FIELD(vout)
+      if (typename Archive<W>::is_saving())
+      {
+        // Serializing...
+        FIELD(vin)
+        FIELD(vout)
+      }
+      else
+      {
+        // De-serializing...
+        // vin
+        if (!this->deserialize_vin(ar))
+        {
+          ar.set_fail();
+          return false;
+        }
+
+        // vout
+        const size_t MAX_VOUT_COUNT = (this->is_coinbase() || vin.empty()) ? MAX_COINBASE_VOUT_COUNT : MAX_NON_COINBASE_VOUT_COUNT;
+        CONTAINER_FIELD_CAPPED(vout, MAX_VOUT_COUNT)
+      }
       FIELD(extra)
     END_SERIALIZE()
 
+    //-----------------------------------------------------------------------
+    bool is_coinbase() const
+    {
+      if (vin.size() != COINBASE_VIN_COUNT)
+        return false;
+
+      if (vin.at(0).type() != typeid(txin_gen))
+        return false;
+
+      return true;
+    }
+    //-----------------------------------------------------------------------
+  private:
+    // Do some validation on vin before doing allocations and reading the archive:
+    // 1. Can't exceed MAX_VIN_COUNT.
+    // 2. If coinbase, only 1 txin_gen.
+    // 3. If not coinbase: type txin_to_key (see check_inputs_types_supported).
+    // 4. If not coinbase: key offsets don't exceed max allowed for whole tx.
+    // Note: can't enforce non-empty because wallets may store empty vin.
+    template<template <bool> class Archive>
+    bool deserialize_vin(Archive<false> &ar)
+    {
+      auto &v = vin;
+      size_t cnt = 0;
+      ar.begin_array(cnt);
+      if (!ar.good())
+        return false;
+      v.clear();
+
+      if (ar.remaining_bytes() < cnt)
+        return false;
+
+      // 1. Can't exceed MAX_VIN_COUNT.
+      if (cnt > MAX_VIN_COUNT)
+        return false;
+
+      ::serialization::detail::do_reserve(v, cnt, ar.remaining_bytes());
+
+      size_t total_key_offsets = 0;
+      for (size_t i = 0; i < cnt; ++i) {
+        if (i > 0)
+          ar.delimit_array();
+        v.emplace_back();
+        if (!::serialization::detail::serialize_container_element(ar, v.back()))
+          return false;
+        if (!ar.good())
+          return false;
+
+        // 2. If coinbase, only 1 txin_gen.
+        const auto &in = v.back();
+        const bool coinbase = i == 0 && in.type() == typeid(txin_gen);
+        if (coinbase && cnt > COINBASE_VIN_COUNT)
+          return false;
+        if (coinbase)
+          break;
+
+        // 3. If not coinbase: type txin_to_key (see check_inputs_types_supported).
+        if (in.type() != typeid(txin_to_key))
+          return false;
+
+        // 4. If not coinbase: key offsets don't exceed max allowed for whole tx.
+        const std::size_t next_toal_key_offsets = total_key_offsets + boost::get<cryptonote::txin_to_key>(in).key_offsets.size();
+        if (next_toal_key_offsets < total_key_offsets)
+          return false; // overflow
+        if (next_toal_key_offsets >= MAX_TOTAL_KEY_OFFSETS)
+          return false;
+        total_key_offsets = next_toal_key_offsets;
+      }
+      ar.end_array();
+      v.shrink_to_fit();
+
+      return ar.good();
+    }
+
+    template<template <bool> class Archive>
+    bool deserialize_vin(Archive<true> &ar)
+    {
+      throw std::runtime_error("unexpected writer called deserialize_vin");
+    }
+
   public:
     transaction_prefix(){ set_null(); }
     void set_null()

src/cryptonote_basic/cryptonote_basic_impl.cpp

@@ -172,17 +172,6 @@ namespace cryptonote {
     return tools::base58::encode_addr(integrated_address_prefix, t_serializable_object_to_blob(iadr));
   }
   //-----------------------------------------------------------------------
-  bool is_coinbase(const transaction& tx)
-  {
-    if(tx.vin.size() != 1)
-      return false;
-
-    if(tx.vin[0].type() != typeid(txin_gen))
-      return false;
-
-    return true;
-  }
-  //-----------------------------------------------------------------------
   bool get_account_address_from_str(
       address_parse_info& info
     , network_type nettype

src/cryptonote_basic/cryptonote_basic_impl.h

@@ -108,8 +108,6 @@ namespace cryptonote {
     , std::function<std::string(const std::string&, const std::vector<std::string>&, bool)> dns_confirm = return_first_address
     );
 
-  bool is_coinbase(const transaction& tx);
-
   bool operator ==(const cryptonote::transaction& a, const cryptonote::transaction& b);
   bool operator ==(const cryptonote::block& a, const cryptonote::block& b);
 }

src/cryptonote_basic/cryptonote_format_utils.cpp

@@ -122,6 +122,7 @@ namespace cryptonote
     const uint64_t bp_clawback = (bp_base * n_padded_outputs - bp_size) * 4 / 5;
     return bp_clawback;
   }
+
   //---------------------------------------------------------------
 }
 
@@ -143,7 +144,7 @@ namespace cryptonote
   
   bool expand_transaction_1(transaction &tx, bool base_only)
   {
-    if (tx.version >= 2 && !is_coinbase(tx))
+    if (tx.version >= 2 && !tx.is_coinbase())
     {
       rct::rctSig &rv = tx.rct_signatures;
       if (rv.type == rct::RCTTypeNull)

src/cryptonote_core/blockchain.cpp

@@ -636,7 +636,7 @@ block Blockchain::pop_block_from_blockchain()
       ++pruned;
       continue;
     }
-    if (!is_coinbase(tx))
+    if (!tx.is_coinbase())
     {
       cryptonote::tx_verification_context tvc = AUTO_VAL_INIT(tvc);
 

src/cryptonote_core/tx_sanity_check.cpp

@@ -49,7 +49,7 @@ bool tx_sanity_check(const cryptonote::blobdata &tx_blob, uint64_t rct_outs_avai
     return false;
   }
 
-  if (cryptonote::is_coinbase(tx))
+  if (tx.is_coinbase())
   {
     MERROR("Transaction is coinbase");
     return false;

src/serialization/container.h

@@ -91,7 +91,7 @@ namespace serialization
 }
 
 template <template <bool> class Archive, typename C>
-bool do_serialize_container(Archive<false> &ar, C &v)
+bool do_serialize_container(Archive<false> &ar, C &v, size_t max_cnt = std::numeric_limits<size_t>::max())
 {
   size_t cnt;
   ar.begin_array(cnt);
@@ -99,8 +99,8 @@ bool do_serialize_container(Archive<false> &ar, C &v)
     return false;
   v.clear();
 
-  // very basic sanity check
-  if (ar.remaining_bytes() < cnt) {
+  // very basic sanity checks
+  if (max_cnt < cnt || ar.remaining_bytes() < cnt) {
     ar.set_fail();
     return false;
   }
@@ -122,9 +122,13 @@ bool do_serialize_container(Archive<false> &ar, C &v)
 }
 
 template <template <bool> class Archive, typename C>
-bool do_serialize_container(Archive<true> &ar, C &v)
+bool do_serialize_container(Archive<true> &ar, C &v, size_t max_cnt = std::numeric_limits<size_t>::max())
 {
   size_t cnt = v.size();
+  if (cnt > max_cnt) {
+    ar.set_fail();
+    return false;
+  }
   ar.begin_array(cnt);
   for (auto i = v.begin(); i != v.end(); ++i)
   {

src/serialization/containers.h

@@ -38,8 +38,8 @@
 #include <set>
 #include "serialization.h"
 
-template <template <bool> class Archive, class T> bool do_serialize(Archive<false> &ar, std::vector<T> &v);
-template <template <bool> class Archive, class T> bool do_serialize(Archive<true> &ar, std::vector<T> &v);
+template <template <bool> class Archive, class T> bool do_serialize(Archive<false> &ar, std::vector<T> &v, size_t max_cnt = std::numeric_limits<size_t>::max());
+template <template <bool> class Archive, class T> bool do_serialize(Archive<true> &ar, std::vector<T> &v, size_t max_cnt = std::numeric_limits<size_t>::max());
 
 template <template <bool> class Archive, class T> bool do_serialize(Archive<false> &ar, std::deque<T> &v);
 template <template <bool> class Archive, class T> bool do_serialize(Archive<true> &ar, std::deque<T> &v);
@@ -105,8 +105,8 @@ namespace serialization
 
 #include "container.h"
 
-template <template <bool> class Archive, class T> bool do_serialize(Archive<false> &ar, std::vector<T> &v) { return do_serialize_container(ar, v); }
-template <template <bool> class Archive, class T> bool do_serialize(Archive<true> &ar, std::vector<T> &v) { return do_serialize_container(ar, v); }
+template <template <bool> class Archive, class T> bool do_serialize(Archive<false> &ar, std::vector<T> &v, size_t max_cnt) { return do_serialize_container(ar, v, max_cnt); }
+template <template <bool> class Archive, class T> bool do_serialize(Archive<true> &ar, std::vector<T> &v, size_t max_cnt) { return do_serialize_container(ar, v, max_cnt); }
 
 template <template <bool> class Archive, class T> bool do_serialize(Archive<false> &ar, std::deque<T> &v) { return do_serialize_container(ar, v); }
 template <template <bool> class Archive, class T> bool do_serialize(Archive<true> &ar, std::deque<T> &v) { return do_serialize_container(ar, v); }

src/serialization/json_object.cpp

@@ -303,7 +303,7 @@ void fromJsonValue(const rapidjson::Value& val, cryptonote::transaction& tx)
   }
 
   const auto& rsig = tx.rct_signatures;
-  if (!cryptonote::is_coinbase(tx) && rsig.p.bulletproofs.empty() && rsig.p.bulletproofs_plus.empty() && rsig.p.rangeSigs.empty() && rsig.p.MGs.empty() && rsig.get_pseudo_outs().empty() && sigs == val.MemberEnd())
+  if (!tx.is_coinbase() && rsig.p.bulletproofs.empty() && rsig.p.bulletproofs_plus.empty() && rsig.p.rangeSigs.empty() && rsig.p.MGs.empty() && rsig.get_pseudo_outs().empty() && sigs == val.MemberEnd())
     tx.pruned = true;
 }
 

src/serialization/serialization.h

@@ -225,6 +225,16 @@ inline bool do_serialize(Archive &ar, bool &v)
     if (!ar.good()) return false;		\
   } while(0);
 
+/*! \macro CONTAINER_FIELD_CAPPED(f, c)
+ *
+ * \brief tags the field with the variable name and then serializes it
+ */
+#define CONTAINER_FIELD_CAPPED(f, c)					\
+  do {							\
+    ar.tag(#f);						\
+    bool r = do_serialize(ar, f, c);			\
+    if (!r || !ar.good()) return false;			\
+  } while(0);
 
 namespace serialization {
   /*! \namespace detail

src/simplewallet/simplewallet.cpp

@@ -5884,7 +5884,7 @@ void simple_wallet::on_money_received(uint64_t height, const crypto::hash &txid,
       message_writer(console_color_red, false) <<
         tr("WARNING: this transaction uses an unencrypted payment ID: these are obsolete and ignored. Use subaddresses instead.");
   }
-  if (unlock_time && !cryptonote::is_coinbase(tx))
+  if (unlock_time && !tx.is_coinbase())
     message_writer() << tr("NOTE: This transaction is locked, see details with: show_transfer ") + epee::string_tools::pod_to_hex(txid);
   if (m_auto_refresh_refreshing)
     m_cmd_binder.print_prompt();

src/wallet/wallet2.cpp

@@ -1764,7 +1764,7 @@ void wallet2::sort_scan_tx_entries(std::vector<process_tx_entry_t> &unsorted_tx_
   COMMAND_RPC_GET_BLOCKS_BY_HEIGHT::response res;
   for (const auto & tx_info : unsorted_tx_entries)
   {
-    if (!tx_info.tx_entry.in_pool && !cryptonote::is_coinbase(tx_info.tx))
+    if (!tx_info.tx_entry.in_pool && !tx_info.tx.is_coinbase())
     {
       const uint64_t height = tx_info.tx_entry.block_height;
       if (entry_heights.find(height) == entry_heights.end())
@@ -1811,9 +1811,9 @@ void wallet2::sort_scan_tx_entries(std::vector<process_tx_entry_t> &unsorted_tx_
     else // l.tx_entry.block_height == r.tx_entry.block_height
     {
       // coinbase tx is the first tx in a block
-      if (cryptonote::is_coinbase(r.tx))
+      if (r.tx.is_coinbase())
         return false;
-      if (cryptonote::is_coinbase(l.tx))
+      if (l.tx.is_coinbase())
         return true;
 
       // in case std::sort is comparing elem to itself
@@ -1864,7 +1864,7 @@ void wallet2::process_scan_txs(const tx_entry_data &txs_to_scan, const tx_entry_
       tx_entry.block_height,
       0,
       tx_entry.block_timestamp,
-      cryptonote::is_coinbase(tx_info.tx),
+      tx_info.tx.is_coinbase(),
       tx_entry.in_pool,
       tx_entry.double_spend_seen,
       {}, {}, // unused caches
@@ -14408,7 +14408,7 @@ uint64_t wallet2::import_key_images(const std::vector<std::pair<crypto::key_imag
         THROW_WALLET_EXCEPTION_IF(!r, error::wallet_internal_error, "Failed to generate key derivation");
       }
       size_t output_index = 0;
-      bool miner_tx = cryptonote::is_coinbase(spent_tx);
+      bool miner_tx = spent_tx.is_coinbase();
       for (const cryptonote::tx_out& out : spent_tx.vout)
       {
         tx_scan_info_t tx_scan_info;
@@ -14649,7 +14649,7 @@ void wallet2::process_background_cache(const background_sync_data_t &background_
     MDEBUG("Processing background synced tx " << bgs_tx.first);
 
     process_new_transaction(bgs_tx.first, bgs_tx.second.tx, bgs_tx.second.output_indices, bgs_tx.second.height, 0, bgs_tx.second.block_timestamp,
-        cryptonote::is_coinbase(bgs_tx.second.tx), false/*pool*/, bgs_tx.second.double_spend_seen, {}, {}, true/*ignore_callbacks*/);
+        bgs_tx.second.tx.is_coinbase(), false/*pool*/, bgs_tx.second.double_spend_seen, {}, {}, true/*ignore_callbacks*/);
 
     // Re-set destination addresses if they were previously set
     if (m_confirmed_txs.find(bgs_tx.first) != m_confirmed_txs.end() &&

tests/unit_tests/serialization.cpp

@@ -391,9 +391,7 @@ TEST(Serialization, serializes_transacion_signatures_correctly)
   tx.invalidate_hashes();
   ASSERT_TRUE(serialization::dump_binary(tx, blob));
   ASSERT_EQ(9, blob.size()); // 5 bytes + 2 * 2 bytes vins + 0 bytes extra + 0 bytes signatures
-  ASSERT_TRUE(serialization::parse_binary(blob, tx1));
-  ASSERT_EQ(tx, tx1);
-  ASSERT_EQ(linearize_vector2(tx.signatures), linearize_vector2(tx1.signatures));
+  ASSERT_FALSE(serialization::parse_binary(blob, tx1));
 
   // Two txin_gen, signatures vector contains only one empty element
   tx.signatures.resize(1);
@@ -405,9 +403,7 @@ TEST(Serialization, serializes_transacion_signatures_correctly)
   tx.invalidate_hashes();
   ASSERT_TRUE(serialization::dump_binary(tx, blob));
   ASSERT_EQ(9, blob.size()); // 5 bytes + 2 * 2 bytes vins + 0 bytes extra + 0 bytes signatures
-  ASSERT_TRUE(serialization::parse_binary(blob, tx1));
-  ASSERT_EQ(tx, tx1);
-  ASSERT_EQ(linearize_vector2(tx.signatures), linearize_vector2(tx1.signatures));
+  ASSERT_FALSE(serialization::parse_binary(blob, tx1));
 
   // Two txin_gen, signatures vector contains three empty elements
   tx.signatures.resize(3);