From 280344b017d27b7dd2b0eabe905f6aa649f2d067 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 23 Oct 2023 11:48:38 +0200 Subject: [PATCH 1/2] Check block requests response in higher level code instead --- lib/src/sync/optimistic.rs | 2 + .../json_rpc_service/background/chain_head.rs | 37 +++++++++++----- .../background/state_chain.rs | 43 ++++++++++++++----- light-base/src/transactions_service.rs | 24 ++++++++--- wasm-node/CHANGELOG.md | 4 ++ 5 files changed, 81 insertions(+), 29 deletions(-) diff --git a/lib/src/sync/optimistic.rs b/lib/src/sync/optimistic.rs index 4de5c2b21e..5ff54cd8c3 100644 --- a/lib/src/sync/optimistic.rs +++ b/lib/src/sync/optimistic.rs @@ -602,6 +602,8 @@ impl OptimisticSync { return (user_data, FinishRequestOutcome::Obsolete); } + // TODO: important /!\ should check whether the block bodies match the extrinsics root in the headers, in order to differentiate invalid blocks from malicious peers + let ((_, user_data), source_id) = self .inner .verification_queue diff --git a/light-base/src/json_rpc_service/background/chain_head.rs b/light-base/src/json_rpc_service/background/chain_head.rs index 67ab80d1d5..16ed77ff08 100644 --- a/light-base/src/json_rpc_service/background/chain_head.rs +++ b/light-base/src/json_rpc_service/background/chain_head.rs @@ -877,12 +877,14 @@ impl ChainHeadFollowTask { unreachable!() }; - // Determine whether the requested block hash is valid, and if yes its number. - let block_number = { + // Determine whether the requested block hash is valid, and if yes its number and + // extrinsics trie root. The extrinsics trie root is used to verify whether the body we + // download is correct. + let (block_number, extrinsics_root) = { if let Some(header) = self.pinned_blocks_headers.get(&hash.0) { let decoded = header::decode(header, self.sync_service.block_number_bytes()).unwrap(); // TODO: unwrap? - decoded.number + (decoded.number, *decoded.extrinsics_root) } else { // Block isn't pinned. Request is invalid. request.fail(json_rpc::parse::ErrorResponse::InvalidParams); @@ -949,20 +951,33 @@ impl ChainHeadFollowTask { None => return, // JSON-RPC client has unsubscribed in the meanwhile. }; - match outcome { - Ok(block_data) => { + // We must check whether the body is present in the response and valid. + // TODO: should try the request again with a different peer instead of failing immediately + let body = match outcome { + Ok(outcome) => { + if let Some(body) = outcome.body { + if header::extrinsics_root(&body) == extrinsics_root { + Ok(body) + } else { + Err(()) + } + } else { + Err(()) + } + } + Err(err) => Err(err), + }; + + // Send back the response. + match body { + Ok(body) => { let _ = to_main_task .send(OperationEvent { operation_id: operation_id.clone(), is_done: true, notification: methods::FollowEvent::OperationBodyDone { operation_id: operation_id.clone().into(), - value: block_data - .body - .unwrap() - .into_iter() - .map(methods::HexString) - .collect(), + value: body.into_iter().map(methods::HexString).collect(), }, }) .await; diff --git a/light-base/src/json_rpc_service/background/state_chain.rs b/light-base/src/json_rpc_service/background/state_chain.rs index 0950eded95..8b17cd427c 100644 --- a/light-base/src/json_rpc_service/background/state_chain.rs +++ b/light-base/src/json_rpc_service/background/state_chain.rs @@ -123,8 +123,8 @@ impl Background { rx.await.unwrap() }; - // Block bodies and justifications aren't stored locally. Ask the network. - let result = if let Some(block_number) = block_number { + // Block bodies and headers aren't stored locally. Ask the network. + let mut result = if let Some(block_number) = block_number { self.sync_service .clone() .block_query( @@ -133,7 +133,7 @@ impl Background { protocol::BlocksRequestFields { header: true, body: true, - justifications: true, + justifications: false, }, 3, Duration::from_secs(8), @@ -148,7 +148,7 @@ impl Background { protocol::BlocksRequestFields { header: true, body: true, - justifications: true, + justifications: false, }, 3, Duration::from_secs(8), @@ -157,9 +157,32 @@ impl Background { .await }; - // The `block_query` function guarantees that the header and body are present and - // are correct. + // Check whether the header and body are present and valid. + // TODO: try the request again with a different peerin case the response is invalid, instead of returning null + if let Ok(block) = &result { + if let (Some(header), Some(body)) = (&block.header, &block.body) { + if header::hash_from_scale_encoded_header(header) == hash { + if let Ok(decoded) = + header::decode(header, self.sync_service.block_number_bytes()) + { + if header::extrinsics_root(&body) != *decoded.extrinsics_root { + result = Err(()); + } + } else { + // Note that if the header is undecodable it doesn't necessarily mean + // that the header and/or body is bad, but given that we have no way to + // check this we return an error. + result = Err(()); + } + } else { + result = Err(()); + } + } else { + result = Err(()); + } + } + // Return the response. if let Ok(block) = result { request.respond(methods::Response::chain_getBlock(methods::Block { extrinsics: block @@ -173,11 +196,9 @@ impl Background { self.sync_service.block_number_bytes(), ) .unwrap(), - justifications: block.justifications.map(|list| { - list.into_iter() - .map(|j| (j.engine_id, j.justification)) - .collect() - }), + // There's no way to verify the correctness of the justifications, consequently + // we always return an empty list. + justifications: None, })) } else { request.respond_null() diff --git a/light-base/src/transactions_service.rs b/light-base/src/transactions_service.rs index efd30c4769..544ea411e3 100644 --- a/light-base/src/transactions_service.rs +++ b/light-base/src/transactions_service.rs @@ -601,7 +601,7 @@ async fn background_task(mut config: BackgroundTaskConfig(mut config: BackgroundTaskConfig(mut config: BackgroundTaskConfig { // A block body download has finished, successfully or not. - let (block_hash, block_body) = download; + let (block_hash, mut block_body) = download; let block = match worker.pending_transactions.block_user_data_mut(&block_hash) { Some(b) => b, @@ -714,8 +717,14 @@ async fn background_task(mut config: BackgroundTaskConfig(mut config: BackgroundTaskConfig Failed(block={})", diff --git a/wasm-node/CHANGELOG.md b/wasm-node/CHANGELOG.md index f8387b9433..6f9b962119 100644 --- a/wasm-node/CHANGELOG.md +++ b/wasm-node/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Changed + +- The `chain_getBlock` JSON-RPC function now always returns an empty list of justifications, because there is no (reasonable) way for smoldot to verify whether the justifications sent by full nodes are valid. + ## 2.0.6 - 2023-10-13 ### Fixed From 34993226a10fd3d76e10de498847632547a17556 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 23 Oct 2023 11:56:41 +0200 Subject: [PATCH 2/2] PR link --- wasm-node/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wasm-node/CHANGELOG.md b/wasm-node/CHANGELOG.md index 6f9b962119..d8ed0153bf 100644 --- a/wasm-node/CHANGELOG.md +++ b/wasm-node/CHANGELOG.md @@ -4,7 +4,7 @@ ### Changed -- The `chain_getBlock` JSON-RPC function now always returns an empty list of justifications, because there is no (reasonable) way for smoldot to verify whether the justifications sent by full nodes are valid. +- The `chain_getBlock` JSON-RPC function now always returns an empty list of justifications, because there is no (reasonable) way for smoldot to verify whether the justifications sent by full nodes are valid. ([#1238](https://github.com/smol-dot/smoldot/pull/1238)) ## 2.0.6 - 2023-10-13