Skip to content

Commit b5e122f

Browse files
upbqdnoxarbitrage
andauthored
fix(rpc): getblock verbosity 2 side-chain panic (GHSA-x6v8-c2xp-928m) (#10889)
* test(rpc): add regression test for getblock side-chain panic (GHSA-x6v8-c2xp-928m) * fix(rpc): use i64 for transaction confirmations to avoid side-chain panic (GHSA-x6v8-c2xp-928m) * fix(rpc): resolve clippy and rustfmt warnings Remove unused `mut` on mempool mock and apply rustfmt formatting. --------- Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>
1 parent 15e30b3 commit b5e122f

3 files changed

Lines changed: 116 additions & 26 deletions

File tree

zebra-rpc/src/methods.rs

Lines changed: 20 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1328,27 +1328,24 @@ where
13281328
zebra_state::ReadResponse::BlockAndSize(block_and_size) => {
13291329
let (block, size) = block_and_size.ok_or_misc_error("Block not found")?;
13301330
let block_time = block.header.time;
1331-
let transactions =
1332-
block
1333-
.transactions
1334-
.iter()
1335-
.map(|tx| {
1336-
GetBlockTransaction::Object(Box::new(
1337-
TransactionObject::from_transaction(
1338-
tx.clone(),
1339-
Some(height),
1340-
Some(confirmations.try_into().expect(
1341-
"should be less than max block height, i32::MAX",
1342-
)),
1343-
&network,
1344-
Some(block_time),
1345-
Some(hash),
1346-
Some(true),
1347-
tx.hash(),
1348-
),
1349-
))
1350-
})
1351-
.collect();
1331+
let transactions = block
1332+
.transactions
1333+
.iter()
1334+
.map(|tx| {
1335+
GetBlockTransaction::Object(Box::new(
1336+
TransactionObject::from_transaction(
1337+
tx.clone(),
1338+
Some(height),
1339+
Some(confirmations),
1340+
&network,
1341+
Some(block_time),
1342+
Some(hash),
1343+
Some(true),
1344+
tx.hash(),
1345+
),
1346+
))
1347+
})
1348+
.collect();
13521349
(transactions, Some(size))
13531350
}
13541351
_ => unreachable!("unmatched response to a transaction_ids_for_block request"),
@@ -1791,7 +1788,7 @@ where
17911788
AnyTx::Mined(mined) if in_best_chain => (
17921789
mined.tx.clone(),
17931790
Some(mined.height),
1794-
Some(mined.confirmations),
1791+
Some(mined.confirmations.into()),
17951792
Some(mined.block_time),
17961793
),
17971794
_ => {
@@ -1834,7 +1831,7 @@ where
18341831
TransactionObject::from_transaction(
18351832
tx.tx.clone(),
18361833
Some(tx.height),
1837-
Some(tx.confirmations),
1834+
Some(tx.confirmations.into()),
18381835
&self.network,
18391836
// TODO: Performance gain:
18401837
// https://github.com/ZcashFoundation/zebra/pull/9458#discussion_r2059352752

zebra-rpc/src/methods/tests/vectors.rs

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -831,6 +831,99 @@ async fn rpc_getblock_missing_error() {
831831
assert!(rpc_tx_queue_task_result.is_none());
832832
}
833833

834+
/// Regression test for GHSA-x6v8-c2xp-928m — panics (aborts) before the fix.
835+
///
836+
/// When `Depth` returns `None` (side-chain block), `get_block_header` sets
837+
/// `confirmations = -1`:
838+
/// https://github.com/ZcashFoundation/zebra/blob/v5.2.0/zebra-rpc/src/methods.rs#L1508
839+
/// The old code narrowed that to `u32` via `.try_into().expect()`, which panicked.
840+
///
841+
/// The fix changes `TransactionObject.confirmations` from `u32` to `i64`, matching
842+
/// zcashd's signed `int`:
843+
/// https://github.com/zcash/zcash/blob/v6.3.0/src/rpc/rawtransaction.cpp#L311
844+
/// https://github.com/zcash/zcash/blob/v6.3.0/src/rpc/blockchain.cpp#L404
845+
#[tokio::test(flavor = "multi_thread")]
846+
async fn rpc_getblock_side_chain_verbosity2_does_not_panic() {
847+
let _init_guard = zebra_test::init();
848+
849+
let block: Arc<Block> = zebra_test::vectors::BLOCK_MAINNET_GENESIS_BYTES
850+
.zcash_deserialize_into()
851+
.unwrap();
852+
let block_hash = block.hash();
853+
let block_header = block.header.clone();
854+
let block_size = block.zcash_serialized_size();
855+
856+
let mempool: MockService<_, _, _, BoxError> = MockService::build().for_unit_tests();
857+
let state: MockService<_, _, _, BoxError> = MockService::build().for_unit_tests();
858+
let mut read_state: MockService<_, _, _, BoxError> = MockService::build()
859+
.with_max_request_delay(std::time::Duration::from_secs(5))
860+
.for_unit_tests();
861+
862+
let (_tx, rx) = tokio::sync::watch::channel(None);
863+
let (rpc, _rpc_tx_queue) = RpcImpl::new(
864+
Mainnet,
865+
Default::default(),
866+
Default::default(),
867+
"0.0.1",
868+
"RPC test",
869+
Buffer::new(mempool.clone(), 1),
870+
Buffer::new(state.clone(), 1),
871+
Buffer::new(read_state.clone(), 1),
872+
MockService::build().for_unit_tests(),
873+
MockSyncStatus::default(),
874+
NoChainTip,
875+
MockAddressBookPeers::default(),
876+
rx,
877+
None,
878+
);
879+
880+
let rpc_clone = rpc.clone();
881+
let hash_str = block_hash.to_string();
882+
let block_future = tokio::spawn(async move { rpc_clone.get_block(hash_str, Some(2u8)).await });
883+
884+
// get_block_header: BlockHeader, SaplingTree, Depth (None = side chain)
885+
read_state
886+
.expect_request(ReadRequest::BlockHeader(block_hash.into()))
887+
.await
888+
.respond(ReadResponse::BlockHeader {
889+
header: block_header,
890+
hash: block_hash,
891+
height: zebra_chain::block::Height(0),
892+
next_block_hash: None,
893+
});
894+
read_state
895+
.expect_request_that(|req| matches!(req, ReadRequest::SaplingTree(_)))
896+
.await
897+
.respond(ReadResponse::SaplingTree(Some(Default::default())));
898+
read_state
899+
.expect_request(ReadRequest::Depth(block_hash))
900+
.await
901+
.respond(ReadResponse::Depth(None));
902+
903+
// get_block: BlockAndSize, OrchardTree, BlockInfo x2
904+
read_state
905+
.expect_request_that(|req| matches!(req, ReadRequest::BlockAndSize(_)))
906+
.await
907+
.respond(ReadResponse::BlockAndSize(Some((block, block_size))));
908+
read_state
909+
.expect_request_that(|req| matches!(req, ReadRequest::OrchardTree(_)))
910+
.await
911+
.respond(ReadResponse::OrchardTree(Some(Default::default())));
912+
read_state
913+
.expect_request_that(|req| matches!(req, ReadRequest::BlockInfo(_)))
914+
.await
915+
.respond(ReadResponse::BlockInfo(None));
916+
read_state
917+
.expect_request_that(|req| matches!(req, ReadRequest::BlockInfo(_)))
918+
.await
919+
.respond(ReadResponse::BlockInfo(Some(BlockInfo::default())));
920+
921+
block_future
922+
.await
923+
.expect("task should not panic")
924+
.expect("getblock should succeed for side-chain blocks");
925+
}
926+
834927
#[tokio::test(flavor = "multi_thread")]
835928
async fn rpc_getblockheader() {
836929
let _init_guard = zebra_test::init();
@@ -1146,7 +1239,7 @@ async fn rpc_getrawtransaction() {
11461239
panic!("unexpected response to Depth request");
11471240
};
11481241

1149-
let expected_confirmations = 1 + depth.expect("depth should be Some");
1242+
let expected_confirmations: i64 = (1 + depth.expect("depth should be Some")).into();
11501243

11511244
(confirmations, expected_confirmations)
11521245
}

zebra-rpc/src/methods/types/transaction.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,7 @@ pub struct TransactionObject {
290290
/// mempool.
291291
#[serde(skip_serializing_if = "Option::is_none")]
292292
#[getter(copy)]
293-
pub(crate) confirmations: Option<u32>,
293+
pub(crate) confirmations: Option<i64>,
294294

295295
/// Transparent inputs of the transaction.
296296
#[serde(rename = "vin")]
@@ -794,7 +794,7 @@ impl TransactionObject {
794794
pub fn from_transaction(
795795
tx: Arc<Transaction>,
796796
height: Option<block::Height>,
797-
confirmations: Option<u32>,
797+
confirmations: Option<i64>,
798798
network: &Network,
799799
block_time: Option<DateTime<Utc>>,
800800
block_hash: Option<block::Hash>,

0 commit comments

Comments
 (0)