fix: Transaction list validation (#1275)

gsstoykov · web-flow · commit 6e2943a11557 · 2026-03-31T10:56:00.000-04:00
Signed-off-by: gsstoykov &lt;georgi.stoykov@limechain.tech&gt;
diff --git a/src/sdk/main/src/ChunkedTransaction.cc b/src/sdk/main/src/ChunkedTransaction.cc
@@ -415,6 +415,20 @@ ChunkedTransaction<SdkRequestType>::ChunkedTransaction(
     return;
   }
 
+  // Security: Determine the expected transaction type from the first entry.
+  // All chunks must have the same transaction type to prevent transaction smuggling attacks.
+  proto::TransactionBody::DataCase expectedDataCase = proto::TransactionBody::DATA_NOT_SET;
+  if (!transactions.empty() && !transactions.cbegin()->second.empty())
+  {
+    const proto::Transaction& firstTx = transactions.cbegin()->second.cbegin()->second;
+    proto::SignedTransaction signedTx;
+    signedTx.ParseFromArray(firstTx.signedtransactionbytes().data(),
+                            static_cast<int>(firstTx.signedtransactionbytes().size()));
+    proto::TransactionBody txBody;
+    txBody.ParseFromArray(signedTx.bodybytes().data(), static_cast<int>(signedTx.bodybytes().size()));
+    expectedDataCase = txBody.data_case();
+  }
+
   // Go through each additional TransactionId and store its information.
   auto iter = transactions.cbegin();
   std::advance(iter, 1);
@@ -432,6 +446,23 @@ ChunkedTransaction<SdkRequestType>::ChunkedTransaction(
       // ID, then no more account IDs will be in the map.
       if (!(accountId == Transaction<SdkRequestType>::DUMMY_ACCOUNT_ID))
       {
+        // Security: Validate that this chunk has the same transaction type as the first chunk.
+        // This is defense-in-depth against transaction smuggling attacks where an attacker
+        // might try to inject a different transaction type (e.g., CryptoTransfer) as a "chunk"
+        // of what appears to be a FileAppend or TopicMessageSubmit transaction.
+        proto::SignedTransaction signedTx;
+        signedTx.ParseFromArray(transaction.signedtransactionbytes().data(),
+                                static_cast<int>(transaction.signedtransactionbytes().size()));
+        proto::TransactionBody txBody;
+        txBody.ParseFromArray(signedTx.bodybytes().data(), static_cast<int>(signedTx.bodybytes().size()));
+
+        if (txBody.data_case() != expectedDataCase)
+        {
+          throw std::invalid_argument(
+            "ChunkedTransaction contains chunks with inconsistent transaction types. "
+            "All chunks must have the same transaction type to prevent transaction smuggling attacks.");
+        }
+
         Transaction<SdkRequestType>::addTransaction(transaction);
       }
     }
diff --git a/src/sdk/main/src/Transaction.cc b/src/sdk/main/src/Transaction.cc
@@ -199,6 +199,7 @@ WrappedTransaction Transaction<SdkRequestType>::fromBytes(const std::vector<std:
   proto::Transaction tx;
   proto::SignedTransaction signedTx;
   proto::TransactionBody txBody;
+  proto::TransactionBody::DataCase expectedDataCase = proto::TransactionBody::DATA_NOT_SET;
 
   bool batchified = false;
 
@@ -222,13 +223,28 @@ WrappedTransaction Transaction<SdkRequestType>::fromBytes(const std::vector<std:
     {
       std::string currentGroupRefBodyBytes;
       std::string currentGroupTxIdBytes;
+
       for (int i = 0; i < txList.transaction_list_size(); ++i)
       {
         tx = txList.transaction_list(i);
         signedTx.ParseFromArray(tx.signedtransactionbytes().data(),
                                 static_cast<int>(tx.signedtransactionbytes().size()));
         txBody.ParseFromArray(signedTx.bodybytes().data(), static_cast<int>(signedTx.bodybytes().size()));
 
+        // Security: Validate that all entries have the same transaction type.
+        // This is critical to prevent transaction smuggling where an attacker injects
+        // a hidden malicious transaction with a different type that gets signed but not displayed.
+        if (i == 0)
+        {
+          expectedDataCase = txBody.data_case();
+        }
+        else if (txBody.data_case() != expectedDataCase)
+        {
+          throw std::invalid_argument(
+            "Transaction list contains entries with inconsistent transaction types. "
+            "All entries must have the same transaction type to prevent transaction smuggling attacks.");
+        }
+
         std::string thisTxIdBytes = txBody.transactionid().SerializeAsString();
 
         // Create a sanitized body (removing NodeAccountID) for comparison
@@ -293,6 +309,23 @@ WrappedTransaction Transaction<SdkRequestType>::fromBytes(const std::vector<std:
     }
   }
 
+  // Security: For non-chunked transaction types, reject if there are multiple transactionIds.
+  // Only FileAppend and TopicMessageSubmit (chunked transactions) legitimately have multiple transactionIds.
+  // This prevents an attacker from smuggling hidden transactions with different transactionIds
+  // that would get signed but not displayed to the user.
+  const auto transactionType =
+    (expectedDataCase != proto::TransactionBody::DATA_NOT_SET) ? expectedDataCase : txBody.data_case();
+  const bool isChunkedTransactionType = (transactionType == proto::TransactionBody::kFileAppend ||
+                                         transactionType == proto::TransactionBody::kConsensusSubmitMessage);
+
+  if (!isChunkedTransactionType && transactions.size() > 1)
+  {
+    throw std::invalid_argument(
+      "Non-chunked transaction types cannot have multiple transaction IDs. "
+      "This may indicate an attempt to smuggle hidden transactions. "
+      "Only FileAppend and TopicMessageSubmit support multiple transaction IDs for chunking.");
+  }
+
   switch (txBody.data_case())
   {
     case proto::TransactionBody::kCryptoApproveAllowance:
diff --git a/src/sdk/tests/unit/TransactionUnitTests.cc b/src/sdk/tests/unit/TransactionUnitTests.cc
@@ -3174,3 +3174,244 @@ TEST_F(TransactionUnitTests, RemoveSignatureFromMultiNodeTransaction)
   // Verify that all internal signatory tracking and protobuf signature maps are completely clear
   EXPECT_EQ(finalRemoval.size(), 0);
 }
+
+//-----
+// Security Test: Prevent transaction smuggling attacks (CVE fix)
+// This test verifies that fromBytes() rejects TransactionLists containing entries
+// with different transaction types, which could be used to smuggle a malicious
+// transaction that gets signed but not displayed to the user.
+TEST_F(TransactionUnitTests, FromBytesRejectsTransactionListWithMixedTransactionTypes)
+{
+  // Given
+  // Entry 1: A benign TransferTransaction (CryptoTransfer)
+  proto::TransactionBody benignBody;
+  benignBody.set_memo("Benign transfer");
+  benignBody.set_allocated_cryptotransfer(new proto::CryptoTransferTransactionBody);
+
+  proto::SignedTransaction signedTx1;
+  signedTx1.set_bodybytes(benignBody.SerializeAsString());
+  proto::Transaction tx1;
+  tx1.set_signedtransactionbytes(signedTx1.SerializeAsString());
+
+  // Entry 2: A different transaction type (AccountCreate) - this simulates
+  // an attacker trying to smuggle a different transaction type
+  proto::TransactionBody maliciousBody;
+  maliciousBody.set_memo("Hidden transaction");
+  maliciousBody.set_allocated_cryptocreateaccount(new proto::CryptoCreateTransactionBody);
+
+  proto::SignedTransaction signedTx2;
+  signedTx2.set_bodybytes(maliciousBody.SerializeAsString());
+  proto::Transaction tx2;
+  tx2.set_signedtransactionbytes(signedTx2.SerializeAsString());
+
+  // Create a TransactionList with mixed types
+  proto::TransactionList txList;
+  *txList.add_transaction_list() = tx1;
+  *txList.add_transaction_list() = tx2;
+
+  // When / Then
+  // This should throw because entries have different transaction types
+  EXPECT_THROW(
+    Transaction<TransferTransaction>::fromBytes(internal::Utilities::stringToByteVector(txList.SerializeAsString())),
+    std::invalid_argument);
+}
+
+//-----
+// Security Test: Prevent transaction smuggling via multiple transactionIds
+// For non-chunked transaction types, multiple transactionIds are not allowed
+// as they could be used to hide malicious transactions.
+TEST_F(TransactionUnitTests, FromBytesRejectsNonChunkedTransactionWithMultipleTransactionIds)
+{
+  // Given
+  // Entry 1: TransferTransaction with TransactionId A
+  proto::TransactionBody txBody1;
+  txBody1.set_memo("Transaction 1");
+  txBody1.set_allocated_cryptotransfer(new proto::CryptoTransferTransactionBody);
+
+  proto::TransactionID txId1;
+  txId1.mutable_accountid()->set_accountnum(201);
+  txId1.mutable_transactionvalidstart()->set_seconds(1700000010);
+  txId1.mutable_transactionvalidstart()->set_nanos(1);
+  txBody1.set_allocated_transactionid(new proto::TransactionID(txId1));
+
+  proto::SignedTransaction signedTx1;
+  signedTx1.set_bodybytes(txBody1.SerializeAsString());
+  proto::Transaction tx1;
+  tx1.set_signedtransactionbytes(signedTx1.SerializeAsString());
+
+  // Entry 2: Same type but different TransactionId (different timestamp)
+  // This simulates an attacker hiding a second transaction
+  proto::TransactionBody txBody2;
+  txBody2.set_memo("Transaction 1"); // Same memo to look identical
+  txBody2.set_allocated_cryptotransfer(new proto::CryptoTransferTransactionBody);
+
+  proto::TransactionID txId2;
+  txId2.mutable_accountid()->set_accountnum(201);
+  txId2.mutable_transactionvalidstart()->set_seconds(1700000011); // Different timestamp!
+  txId2.mutable_transactionvalidstart()->set_nanos(1);
+  txBody2.set_allocated_transactionid(new proto::TransactionID(txId2));
+
+  proto::SignedTransaction signedTx2;
+  signedTx2.set_bodybytes(txBody2.SerializeAsString());
+  proto::Transaction tx2;
+  tx2.set_signedtransactionbytes(signedTx2.SerializeAsString());
+
+  // Create a TransactionList with two different transactionIds
+  proto::TransactionList txList;
+  *txList.add_transaction_list() = tx1;
+  *txList.add_transaction_list() = tx2;
+
+  // When / Then
+  // This should throw because TransferTransaction (non-chunked) cannot have multiple transactionIds
+  EXPECT_THROW(
+    Transaction<TransferTransaction>::fromBytes(internal::Utilities::stringToByteVector(txList.SerializeAsString())),
+    std::invalid_argument);
+}
+
+//-----
+// Security Test: FileAppend (chunked transaction) can have multiple transactionIds
+// but they must all have the same transaction type.
+TEST_F(TransactionUnitTests, FromBytesAllowsChunkedTransactionWithMultipleTransactionIdsOfSameType)
+{
+  // Given
+  // Entry 1: FileAppend chunk 1
+  proto::TransactionBody txBody1;
+  txBody1.set_memo("FileAppend chunk 1");
+  txBody1.set_allocated_fileappend(new proto::FileAppendTransactionBody);
+
+  proto::TransactionID txId1;
+  txId1.mutable_accountid()->set_accountnum(201);
+  txId1.mutable_transactionvalidstart()->set_seconds(1700000010);
+  txId1.mutable_transactionvalidstart()->set_nanos(1);
+  txBody1.set_allocated_transactionid(new proto::TransactionID(txId1));
+
+  proto::SignedTransaction signedTx1;
+  signedTx1.set_bodybytes(txBody1.SerializeAsString());
+  proto::Transaction tx1;
+  tx1.set_signedtransactionbytes(signedTx1.SerializeAsString());
+
+  // Entry 2: FileAppend chunk 2 (same type, different transactionId - this is valid for chunks)
+  proto::TransactionBody txBody2;
+  txBody2.set_memo("FileAppend chunk 2");
+  txBody2.set_allocated_fileappend(new proto::FileAppendTransactionBody);
+
+  proto::TransactionID txId2;
+  txId2.mutable_accountid()->set_accountnum(201);
+  txId2.mutable_transactionvalidstart()->set_seconds(1700000011); // Different timestamp for chunk 2
+  txId2.mutable_transactionvalidstart()->set_nanos(1);
+  txBody2.set_allocated_transactionid(new proto::TransactionID(txId2));
+
+  proto::SignedTransaction signedTx2;
+  signedTx2.set_bodybytes(txBody2.SerializeAsString());
+  proto::Transaction tx2;
+  tx2.set_signedtransactionbytes(signedTx2.SerializeAsString());
+
+  // Create a TransactionList
+  proto::TransactionList txList;
+  *txList.add_transaction_list() = tx1;
+  *txList.add_transaction_list() = tx2;
+
+  // When / Then
+  // FileAppend is a chunked transaction type, so multiple transactionIds are allowed
+  // as long as they all have the same transaction type
+  EXPECT_NO_THROW(
+    Transaction<FileAppendTransaction>::fromBytes(internal::Utilities::stringToByteVector(txList.SerializeAsString())));
+}
+
+//-----
+// Security Test: Even FileAppend (chunked) must reject mixed transaction types
+TEST_F(TransactionUnitTests, FromBytesRejectsChunkedTransactionWithMixedTypes)
+{
+  // Given
+  // Entry 1: FileAppend
+  proto::TransactionBody txBody1;
+  txBody1.set_memo("FileAppend");
+  txBody1.set_allocated_fileappend(new proto::FileAppendTransactionBody);
+
+  proto::TransactionID txId1;
+  txId1.mutable_accountid()->set_accountnum(201);
+  txId1.mutable_transactionvalidstart()->set_seconds(1700000010);
+  txId1.mutable_transactionvalidstart()->set_nanos(1);
+  txBody1.set_allocated_transactionid(new proto::TransactionID(txId1));
+
+  proto::SignedTransaction signedTx1;
+  signedTx1.set_bodybytes(txBody1.SerializeAsString());
+  proto::Transaction tx1;
+  tx1.set_signedtransactionbytes(signedTx1.SerializeAsString());
+
+  // Entry 2: Malicious CryptoTransfer hidden as "chunk 2"
+  proto::TransactionBody txBody2;
+  txBody2.set_memo("Hidden drain");
+  txBody2.set_allocated_cryptotransfer(new proto::CryptoTransferTransactionBody); // Different type!
+
+  proto::TransactionID txId2;
+  txId2.mutable_accountid()->set_accountnum(201);
+  txId2.mutable_transactionvalidstart()->set_seconds(1700000011);
+  txId2.mutable_transactionvalidstart()->set_nanos(1);
+  txBody2.set_allocated_transactionid(new proto::TransactionID(txId2));
+
+  proto::SignedTransaction signedTx2;
+  signedTx2.set_bodybytes(txBody2.SerializeAsString());
+  proto::Transaction tx2;
+  tx2.set_signedtransactionbytes(signedTx2.SerializeAsString());
+
+  // Create a TransactionList
+  proto::TransactionList txList;
+  *txList.add_transaction_list() = tx1;
+  *txList.add_transaction_list() = tx2;
+
+  // When / Then
+  // Even for FileAppend, mixed transaction types must be rejected
+  EXPECT_THROW(
+    Transaction<FileAppendTransaction>::fromBytes(internal::Utilities::stringToByteVector(txList.SerializeAsString())),
+    std::invalid_argument);
+}
+
+//-----
+// Security Test: Directly exercise ChunkedTransaction constructor validation.
+// Build a mixed-type transactions map and ensure chunked constructor rejects it.
+TEST_F(TransactionUnitTests, ChunkedTransactionConstructorRejectsMixedTypes)
+{
+  // Given
+  // Entry 1: FileAppend (expected chunked type)
+  proto::TransactionBody txBody1;
+  txBody1.set_memo("FileAppend chunk 1");
+  txBody1.set_allocated_fileappend(new proto::FileAppendTransactionBody);
+
+  proto::TransactionID txId1;
+  txId1.mutable_accountid()->set_accountnum(201);
+  txId1.mutable_transactionvalidstart()->set_seconds(1700000020);
+  txId1.mutable_transactionvalidstart()->set_nanos(1);
+  txBody1.set_allocated_transactionid(new proto::TransactionID(txId1));
+
+  proto::SignedTransaction signedTx1;
+  signedTx1.set_bodybytes(txBody1.SerializeAsString());
+  proto::Transaction tx1;
+  tx1.set_signedtransactionbytes(signedTx1.SerializeAsString());
+
+  // Entry 2: Different TransactionId and different type (CryptoTransfer)
+  // This simulates a smuggled chunk that should be rejected by ChunkedTransaction.
+  proto::TransactionBody txBody2;
+  txBody2.set_memo("Smuggled transfer");
+  txBody2.set_allocated_cryptotransfer(new proto::CryptoTransferTransactionBody);
+
+  proto::TransactionID txId2;
+  txId2.mutable_accountid()->set_accountnum(201);
+  txId2.mutable_transactionvalidstart()->set_seconds(1700000021);
+  txId2.mutable_transactionvalidstart()->set_nanos(1);
+  txBody2.set_allocated_transactionid(new proto::TransactionID(txId2));
+
+  proto::SignedTransaction signedTx2;
+  signedTx2.set_bodybytes(txBody2.SerializeAsString());
+  proto::Transaction tx2;
+  tx2.set_signedtransactionbytes(signedTx2.SerializeAsString());
+
+  std::map<TransactionId, std::map<AccountId, proto::Transaction>> transactions;
+  const auto txIdKey1 = TransactionId::withValidStart(AccountId(201), std::chrono::system_clock::from_time_t(1700000020));
+  const auto txIdKey2 = TransactionId::withValidStart(AccountId(201), std::chrono::system_clock::from_time_t(1700000021));
+  transactions[txIdKey1][AccountId(3)] = tx1;
+  transactions[txIdKey2][AccountId(3)] = tx2;
+
+  // When / Then
+  EXPECT_THROW(FileAppendTransaction{ transactions }, std::invalid_argument);
+}
