cryptonote_basic: serialization checks

src/cryptonote_basic/cryptonote_basic.h

@@ -52,6 +52,13 @@
 
 namespace cryptonote
 {
+  // 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 */
 
   struct txout_to_script
@@ -139,7 +146,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()
   };
@@ -179,11 +186,102 @@ namespace cryptonote
       VARINT_FIELD(version)
       if((version == 0 || CURRENT_TRANSACTION_VERSION < version)) return false;
       VARINT_FIELD(unlock_time)
-      FIELD(vin)
-      FIELD(vout)
+      if constexpr (W)
+      {
+        // 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();
+        if (!::serialization::detail::serialize_container_element(ar, v.emplace_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();
+    }
+
   public:
     transaction_prefix(){ set_null(); }
     void set_null()

src/cryptonote_basic/cryptonote_basic_impl.cpp

@@ -150,17 +150,6 @@ namespace cryptonote {
     return tools::base58::encode_addr(integrated_address_prefix, t_serializable_object_to_blob(iadr));
   }
   //-----------------------------------------------------------------------
-  bool is_coinbase(const transaction_prefix& 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

@@ -98,8 +98,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_prefix& 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

@@ -96,27 +96,6 @@ namespace cryptonote
     return bp_clawback;
   }
 
-  std::size_t max_total_key_offsets()
-  {
-    // This is a 100% guaranteed ceiling for the entire chain
-    return get_max_tx_size() / sizeof(crypto::public_key);
-  }
-
-  bool n_key_offsets_exceeds_max(const transaction_prefix& tx)
-  {
-    const std::size_t max_allowed = max_total_key_offsets();
-    std::size_t total_key_offsets = 0;
-    for (const auto &vin : tx.vin)
-    {
-      if (vin.type() != typeid(cryptonote::txin_to_key))
-        continue;
-      const std::size_t n_key_offsets = boost::get<cryptonote::txin_to_key>(vin).key_offsets.size();
-      CHECK_AND_ASSERT_MES((n_key_offsets + total_key_offsets) >= total_key_offsets, true, "key offsets overflow");
-      total_key_offsets += n_key_offsets;
-    }
-    return total_key_offsets >= max_allowed;
-  }
-
   bool passes_max_size_check(const bool max_size_check, const blobdata_ref &tx_blob)
   {
     if (!max_size_check)
@@ -144,7 +123,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)
@@ -228,7 +207,6 @@ namespace cryptonote
     binary_archive<false> ba{epee::strspan<std::uint8_t>(tx_blob)};
     bool r = ::serialization::serialize(ba, tx);
     CHECK_AND_ASSERT_MES(r, false, "Failed to parse transaction from blob");
-    CHECK_AND_ASSERT_MES(!n_key_offsets_exceeds_max(tx), false, "Transaction contains too many ring members");
     CHECK_AND_ASSERT_MES(expand_transaction_1(tx, false), false, "Failed to expand transaction data");
     tx.invalidate_hashes();
     tx.set_blob_size(tx_blob.size());
@@ -241,7 +219,6 @@ namespace cryptonote
     binary_archive<false> ba{epee::strspan<std::uint8_t>(tx_blob)};
     bool r = tx.serialize_base(ba);
     CHECK_AND_ASSERT_MES(r, false, "Failed to parse transaction from blob");
-    CHECK_AND_ASSERT_MES(!n_key_offsets_exceeds_max(tx), false, "Transaction contains too many ring members");
     CHECK_AND_ASSERT_MES(expand_transaction_1(tx, true), false, "Failed to expand transaction data");
     tx.invalidate_hashes();
     return true;
@@ -253,7 +230,6 @@ namespace cryptonote
     binary_archive<false> ba{epee::strspan<std::uint8_t>(tx_blob)};
     bool r = ::serialization::serialize_noeof(ba, tx);
     CHECK_AND_ASSERT_MES(r, false, "Failed to parse transaction prefix from blob");
-    CHECK_AND_ASSERT_MES(!n_key_offsets_exceeds_max(tx), false, "Transaction contains too many ring members");
     return true;
   }
   //---------------------------------------------------------------
@@ -263,7 +239,6 @@ namespace cryptonote
     binary_archive<false> ba{epee::strspan<std::uint8_t>(tx_blob)};
     bool r = ::serialization::serialize(ba, tx);
     CHECK_AND_ASSERT_MES(r, false, "Failed to parse transaction from blob");
-    CHECK_AND_ASSERT_MES(!n_key_offsets_exceeds_max(tx), false, "Transaction contains too many ring members");
     CHECK_AND_ASSERT_MES(expand_transaction_1(tx, false), false, "Failed to expand transaction data");
     tx.invalidate_hashes();
     tx.set_blob_size(tx_blob.size());

src/cryptonote_core/blockchain.cpp

@@ -618,7 +618,7 @@ block Blockchain::pop_block_from_blockchain(bool keep_txs)
       ++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

@@ -113,7 +113,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)
 {
   size_t cnt;
   ar.begin_array(cnt);
@@ -121,8 +121,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;
   }
@@ -144,9 +144,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)
 {
   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

@@ -57,7 +57,7 @@ namespace serialization
 
 template <class Archive, class Container>
 std::enable_if_t<::serialization::is_container<Container>::value, bool>
-do_serialize(Archive &ar, Container &c)
+do_serialize(Archive &ar, Container &c, size_t max_cnt = std::numeric_limits<size_t>::max())
 {
-    return ::do_serialize_container(ar, c);
+    return ::do_serialize_container(ar, c, max_cnt);
 }

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

@@ -293,6 +293,16 @@ inline auto do_serialize(Archive &ar, T &v, Args&&... args)
     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

@@ -5479,7 +5479,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

@@ -1750,7 +1750,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())
@@ -1796,9 +1796,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
@@ -1849,7 +1849,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
@@ -13423,7 +13423,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;
@@ -13664,7 +13664,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/json_serialization.cpp

@@ -232,7 +232,7 @@ namespace
             CHECK_AND_ASSERT_MES(tx_document.HasMember("signatures"), false, "compare_tx_to_json_tx: missing signatures");
             CHECK_AND_ASSERT_MES(tx_document["signatures"].IsArray(), false, "compare_tx_to_json_tx: signatures is not array");
 
-            if (!cryptonote::is_coinbase(tx))
+            if (!tx.is_coinbase())
             {
                 CHECK_AND_ASSERT_MES(tx_document["signatures"].Size() == tx.signatures.size(), false, "compare_tx_to_json_tx: signatures wrong size");
                 for (size_t i = 0; i < tx.signatures.size(); ++i)
@@ -259,7 +259,7 @@ namespace
             CHECK_AND_ASSERT_MES(jrv["type"].IsUint(), false, "compare_tx_to_json_tx: rct_signatures type is not int");
             CHECK_AND_ASSERT_MES(jrv["type"].GetUint() == tx.rct_signatures.type, false, "compare_tx_to_json_tx: rct_signatures wrong type");
 
-            if (!cryptonote::is_coinbase(tx))
+            if (!tx.is_coinbase())
             {
                 // txnFee
                 CHECK_AND_ASSERT_MES(jrv.HasMember("txnFee"), false, "compare_tx_to_json_tx: rct_signatures has no fee");

tests/unit_tests/serialization.cpp

@@ -461,9 +461,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);
@@ -475,9 +473,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);