diff --git a/rs/http_endpoints/fuzz/BUILD.bazel b/rs/http_endpoints/fuzz/BUILD.bazel index 3a0ed252d17..c7cc1d4791c 100644 --- a/rs/http_endpoints/fuzz/BUILD.bazel +++ b/rs/http_endpoints/fuzz/BUILD.bazel @@ -1,4 +1,4 @@ -load("//bazel:fuzz_testing.bzl", "rust_fuzz_test_binary") +load("//bazel:fuzz_testing.bzl", "rust_fuzz_test_binary_afl") package(default_visibility = ["//visibility:private"]) @@ -16,12 +16,15 @@ CALLSERVICE_FUZZER_DEPENDENCIES = [ "//rs/registry/keys", "//rs/registry/provisional_whitelist", "//rs/test_utilities", + "//rs/types/base_types", "//rs/types/error_types", "//rs/types/types", + "//rs/validator/http_request_arbitrary", "@crate_index//:arbitrary", "@crate_index//:crossbeam", "@crate_index//:bytes", "@crate_index//:hyper", + "@crate_index//:ic-agent", "@crate_index//:libfuzzer-sys", "@crate_index//:mockall", "@crate_index//:prost", @@ -32,21 +35,20 @@ CALLSERVICE_FUZZER_DEPENDENCIES = [ # required to compile tests/common DEV_DEPENDENCIES = [ - "//rs/crypto/tree_hash", - "//rs/interfaces/state_manager", - "//rs/registry/subnet_type", - "//rs/replicated_state", - "//rs/monitoring/pprof", "//rs/certification/test-utils", + "//rs/crypto/tree_hash", "//rs/crypto/tls_interfaces/mocks", "//rs/interfaces/mocks", + "//rs/interfaces/state_manager", "//rs/interfaces/state_manager/mocks", + "//rs/monitoring/pprof", "//rs/registry/routing_table", - "@crate_index//:ic-agent", + "//rs/registry/subnet_type", + "//rs/replicated_state", ] -rust_fuzz_test_binary( - name = "execute_call_service_libfuzzer", +rust_fuzz_test_binary_afl( + name = "execute_call_service_afl", testonly = True, srcs = [ "fuzz_targets/execute_call_service.rs", diff --git a/rs/http_endpoints/fuzz/fuzz_targets/execute_call_service.rs b/rs/http_endpoints/fuzz/fuzz_targets/execute_call_service.rs index dcdfefcf15f..9ffc8efeb53 100644 --- a/rs/http_endpoints/fuzz/fuzz_targets/execute_call_service.rs +++ b/rs/http_endpoints/fuzz/fuzz_targets/execute_call_service.rs @@ -1,14 +1,19 @@ #![no_main] +use arbitrary::Arbitrary; use bytes::Bytes; use hyper::{Body, Method, Request, Response}; +use ic_agent::{ + agent::{http_transport::reqwest_transport::ReqwestHttpReplicaV2Transport, UpdateBuilder}, + export::Principal, + identity::AnonymousIdentity, + Agent, +}; use ic_config::http_handler::Config; -use ic_error_types::UserError; +use ic_error_types::{ErrorCode, UserError}; use ic_http_endpoints_public::call::CallService; use ic_http_endpoints_public::metrics::HttpHandlerMetrics; use ic_http_endpoints_public::validator_executor::ValidatorExecutor; -use ic_interfaces::{ - execution_environment::QueryExecutionResponse, ingress_pool::IngressPoolThrottler, -}; +use ic_interfaces::ingress_pool::IngressPoolThrottler; use ic_interfaces_registry::RegistryClient; use ic_logger::replica_logger::no_op_logger; use ic_metrics::MetricsRegistry; @@ -17,20 +22,18 @@ use ic_test_utilities::{ crypto::temp_crypto_component_with_fake_registry, types::ids::{node_test_id, subnet_test_id}, }; -use ic_types::{ - malicious_flags::MaliciousFlags, - messages::{CertificateDelegation, SignedIngressContent, UserQuery}, - PrincipalId, -}; +use ic_types::{malicious_flags::MaliciousFlags, messages::SignedIngressContent, PrincipalId}; +use ic_validator_http_request_arbitrary::AnonymousContent; use libfuzzer_sys::fuzz_target; -use mockall::mock; use std::{ convert::Infallible, net::SocketAddr, - str::FromStr, sync::{Arc, RwLock}, }; -use tokio::runtime::Runtime; +use tokio::{ + runtime::Runtime, + sync::mpsc::{channel, Receiver}, +}; use tower::{util::BoxCloneService, Service, ServiceExt}; use tower_test::mock::Handle; @@ -38,16 +41,32 @@ use tower_test::mock::Handle; pub mod common; use common::{basic_registry_client, get_free_localhost_socket_addr, setup_ingress_filter_mock}; -pub type IngressFilterHandle = +type IngressFilterHandle = Handle<(ProvisionalWhitelist, SignedIngressContent), Result<(), UserError>>; -pub type QueryExecutionHandle = - Handle<(UserQuery, Option), QueryExecutionResponse>; +type CallServiceEndpoint = BoxCloneService, Response, Infallible>; -mock! { - pub IngressPoolThrottler {} +#[derive(Arbitrary, Clone, Debug)] +struct CallServiceImpl { + content: AnonymousContent, + allow_ingress_filter: bool, + allow_ingress_throttler: bool, +} - impl IngressPoolThrottler for IngressPoolThrottler { - fn exceeds_threshold(&self) -> bool; +struct MockIngressPoolThrottler { + rx: RwLock>, +} + +impl MockIngressPoolThrottler { + fn new(rx: Receiver) -> Self { + MockIngressPoolThrottler { + rx: RwLock::new(rx), + } + } +} + +impl IngressPoolThrottler for MockIngressPoolThrottler { + fn exceeds_threshold(&self) -> bool { + self.rx.write().unwrap().try_recv().unwrap_or(false) } } @@ -58,51 +77,111 @@ mock! { // The fuzz test is only compiled but not executed by CI. // // To execute the fuzzer run -// bazel run --config=fuzzing //rs/http_endpoints/fuzz:execute_call_service_libfuzzer -- corpus/ +// bazel run --config=afl //rs/http_endpoints/fuzz:execute_call_service_afl -- corpus/ // -// TODO (PSEC-1654): Implement Arbitrary for the request body. Details: -// This initial version of the fuzzer is currently likely ineffective. This is because as soon as the data -// can't be CBOR decoded, is incorrectly signed, or contains a mismatching effective canister id, `call` -// will fail, and such a failure will happen for most mutations of `data`. -// To address this, the next MR (PSEC-1654) will implement Arbitrary so that mutations of the data more -// effectively explore interesting inputs. -fuzz_target!(|data: &[u8]| { - let rt = Runtime::new().unwrap(); - let addr = get_free_localhost_socket_addr(); - let effective_canister_id = "223xb-saaaa-aaaaf-arlqa-cai"; - - let mut call_service = new_call_service(addr); - let mut req = Request::builder() - .method(Method::POST) - .uri(format!( - "http://{}/api/v2/canister/{}/call", - addr, effective_canister_id, - )) - .header("Content-Type", "application/cbor") - .body(Bytes::from(data.to_vec())) - .expect("Failed to build the request"); - - // The effective_canister_id is added to the request during routing - // and then removed from the request parts (see `remove_effective_principal_id` - // in http_endponts/public/src/common.rs). - // We simulate that behaviour in this line. - req.extensions_mut() - .insert(PrincipalId::from_str(effective_canister_id).unwrap()); - - rt.block_on(async move { - call_service - .ready() - .await - .expect("could not create call service") - .call(req) - .await - .unwrap() - }); +fuzz_target!(|call_impls: Vec| { + if !call_impls.is_empty() { + let rt = Runtime::new().unwrap(); + let (throttler_tx, throttler_rx) = channel(call_impls.len()); + let addr = get_free_localhost_socket_addr(); + let (mut ingress_filter_handle, call_service) = new_call_service(addr, throttler_rx); + let (filter_flags, throttler_flags) = extract_flags(&call_impls); + + // Mock ingress filter + rt.spawn(async move { + for flag in filter_flags { + let (_, resp) = ingress_filter_handle.next_request().await.unwrap(); + if flag { + resp.send_response(Ok(())) + } else { + resp.send_response(Err(UserError::new( + ErrorCode::CanisterNotFound, + "Fuzzing ingress filter error", + ))) + } + } + }); + + // Mock ingress throttler + rt.spawn(async move { + for flag in throttler_flags { + if let Err(err) = throttler_tx.send(flag).await { + eprintln!("Error sending message: {}", err); + } + } + }); + + for call_impl in call_impls { + let canister_id = + match Principal::try_from_slice(call_impl.content.canister_id.0.as_slice()) { + Ok(v) => v, + // The arbitrary impl for canister ids in AnonymousContent makes it posible to have more than 29 bytes + // which makes Principal::try_from_slice return an error, in such cases ignore this call_impl. + _ => continue, + }; + let signed_update_call = new_update_call(addr, call_impl.content, canister_id); + let mut call_service_clone = call_service.clone(); + let mut req = Request::builder() + .method(Method::POST) + .uri(format!( + "http://{}/api/v2/canister/{}/call", + addr, + canister_id.to_text(), + )) + .header("Content-Type", "application/cbor") + .body(Bytes::from(signed_update_call)) + .expect("Failed to build the request"); + + // The effective_canister_id is added to the request during routing + // and then removed from the request parts (see `remove_effective_principal_id` + // in http_endponts/public/src/common.rs). We simulate that behaviour in this line. + req.extensions_mut().insert(PrincipalId::from(canister_id)); + + let _res = rt.block_on(async move { + call_service_clone + .ready() + .await + .expect("could not create call service") + .call(req) + .await + .unwrap() + }); + //println!("{:#?}", _res) + } + } }); +fn extract_flags(calls: &[CallServiceImpl]) -> (Vec, Vec) { + let (filter_flags, throttler_flags): (Vec, Vec) = calls + .iter() + .map(|call| (call.allow_ingress_filter, call.allow_ingress_throttler)) + .unzip(); + + (filter_flags, throttler_flags) +} + +fn new_update_call( + addr: SocketAddr, + content: AnonymousContent, + effective_canister_id: Principal, +) -> Vec { + let agent = Agent::builder() + .with_identity(AnonymousIdentity) + .with_transport(ReqwestHttpReplicaV2Transport::create(format!("http://{}", addr)).unwrap()) + .build() + .unwrap(); + let update = UpdateBuilder::new(&agent, effective_canister_id, content.method_name) + .with_effective_canister_id(effective_canister_id) + .with_arg(content.arg.0) + .sign() + .unwrap(); + update.signed_update +} + fn new_call_service( addr: SocketAddr, -) -> BoxCloneService, Response, Infallible> { + throttler_rx: Receiver, +) -> (IngressFilterHandle, CallServiceEndpoint) { let config = Config { listen_addr: addr, ..Default::default() @@ -111,11 +190,11 @@ fn new_call_service( let metrics_registry = MetricsRegistry::new(); let mock_registry_client: Arc = Arc::new(basic_registry_client()); - let (ingress_filter, _ingress_filter_handle) = setup_ingress_filter_mock(); - let mut ingress_pool_throttler = MockIngressPoolThrottler::new(); - ingress_pool_throttler - .expect_exceeds_threshold() - .returning(|| false); + let (ingress_filter, ingress_filter_handle) = setup_ingress_filter_mock(); + let ingress_pool_throttler = MockIngressPoolThrottler::new(throttler_rx); + //ingress_pool_throttler + // .expect_exceeds_threshold() + // .returning(|| false); let ingress_throttler = Arc::new(RwLock::new(ingress_pool_throttler)); @@ -123,7 +202,7 @@ fn new_call_service( let sig_verifier = Arc::new(temp_crypto_component_with_fake_registry(node_test_id(1))); - CallService::new_service( + let call_service = CallService::new_service( config, log.clone(), HttpHandlerMetrics::new(&metrics_registry), @@ -139,5 +218,6 @@ fn new_call_service( ingress_filter, ingress_throttler, ingress_tx, - ) + ); + (ingress_filter_handle, call_service) } diff --git a/rs/validator/fuzz/BUILD.bazel b/rs/validator/fuzz/BUILD.bazel index b3ce40f4c01..00d02040a50 100644 --- a/rs/validator/fuzz/BUILD.bazel +++ b/rs/validator/fuzz/BUILD.bazel @@ -3,13 +3,9 @@ load("//bazel:fuzz_testing.bzl", "rust_fuzz_test_binary") package(default_visibility = ["//visibility:private"]) DEPENDENCIES = [ - "//rs/crypto/tree_hash", - "//rs/types/base_types", "//rs/types/types", + "//rs/validator/http_request_arbitrary", "//rs/validator/ingress_message", - "@crate_index//:arbitrary", - "@crate_index//:assert_matches", - "@crate_index//:lazy_static", "@crate_index//:libfuzzer-sys", ] diff --git a/rs/validator/fuzz/Cargo.toml b/rs/validator/fuzz/Cargo.toml index e67f84109bd..cd22dea4a77 100644 --- a/rs/validator/fuzz/Cargo.toml +++ b/rs/validator/fuzz/Cargo.toml @@ -11,13 +11,9 @@ documentation.workspace = true cargo-fuzz = true [dependencies] -arbitrary = "1.3.0" -assert_matches = "1.5.0" -ic-crypto-tree-hash = { path = "../../crypto/tree_hash" } -ic-base-types = { path = "../../types/base_types" } ic-types = { path = "../../types/types" } +ic-validator-http-request-arbitrary = { path = "../http_request_arbitrary" } ic-validator-ingress-message = { path = "../ingress_message" } -lazy_static = "1.4.0" libfuzzer-sys = "0.4" [dependencies.ic-validator] diff --git a/rs/validator/fuzz/fuzz_targets/validate_request.rs b/rs/validator/fuzz/fuzz_targets/validate_request.rs index 966b8cbd2c8..02e4882aa99 100644 --- a/rs/validator/fuzz/fuzz_targets/validate_request.rs +++ b/rs/validator/fuzz/fuzz_targets/validate_request.rs @@ -1,19 +1,15 @@ #![no_main] -use arbitrary::{Arbitrary, Result, Unstructured}; -use assert_matches::assert_matches; -use ic_base_types::CanisterId; -use ic_crypto_tree_hash::{Label, Path}; use ic_types::messages::{ - Blob, HttpCallContent, HttpCanisterUpdate, HttpQueryContent, HttpReadState, - HttpReadStateContent, HttpRequest, HttpRequestEnvelope, HttpRequestError, HttpUserQuery, + HttpCallContent, HttpQueryContent, HttpReadStateContent, HttpRequest, HttpRequestEnvelope, + HttpRequestError, }; use ic_types::time::GENESIS; +use ic_validator_http_request_arbitrary::AnonymousContent; use ic_validator_ingress_message::IngressMessageVerifier; use ic_validator_ingress_message::RequestValidationError; use ic_validator_ingress_message::{HttpRequestVerifier, TimeProvider}; use libfuzzer_sys::{fuzz_target, Corpus}; -use std::ops::RangeInclusive; fuzz_target!(|content: AnonymousContent| -> Corpus { let (call_content, query_content, read_content) = ( @@ -66,149 +62,6 @@ fuzz_target!(|content: AnonymousContent| -> Corpus { } }); -#[derive(Clone, Debug, Eq, PartialEq)] -struct AnonymousContent { - canister_id: Blob, - paths: Vec, - method_name: String, - arg: Blob, - ingress_expiry: u64, - nonce: Option, -} - -impl AnonymousContent { - fn sender(&self) -> Blob { - const ANONYMOUS_SENDER: u8 = 0x04; - Blob(vec![ANONYMOUS_SENDER]) - } -} - -impl<'a> Arbitrary<'a> for AnonymousContent { - fn arbitrary(u: &mut Unstructured<'a>) -> Result { - Ok(AnonymousContent { - canister_id: somewhat_arbitrary_canister_id(u)?, - paths: arbitrary_tree_paths(u, 0..=50_000, 0..=50_000)?, - method_name: String::arbitrary(u)?, - arg: arbitrary_blob(u)?, - ingress_expiry: u64::arbitrary(u)?, - nonce: arbitrary_option_blob(u)?, - }) - } -} - -impl From for HttpCallContent { - fn from(content: AnonymousContent) -> Self { - let sender = content.sender(); - HttpCallContent::Call { - update: HttpCanisterUpdate { - canister_id: content.canister_id, - method_name: content.method_name, - arg: content.arg, - sender, - ingress_expiry: content.ingress_expiry, - nonce: content.nonce, - }, - } - } -} - -impl From for HttpQueryContent { - fn from(content: AnonymousContent) -> Self { - let sender = content.sender(); - HttpQueryContent::Query { - query: HttpUserQuery { - canister_id: content.canister_id, - method_name: content.method_name, - arg: content.arg, - sender, - ingress_expiry: content.ingress_expiry, - nonce: content.nonce, - }, - } - } -} - -impl From for HttpReadStateContent { - fn from(content: AnonymousContent) -> Self { - let sender = content.sender(); - HttpReadStateContent::ReadState { - read_state: HttpReadState { - sender, - paths: content.paths, - ingress_expiry: content.ingress_expiry, - nonce: content.nonce, - }, - } - } -} - -fn arbitrary_blob(u: &mut Unstructured) -> Result { - Ok(Blob(>::arbitrary(u)?)) -} - -/// A CanisterId wraps a PrincipalId, whose length is at most 29 bytes. -/// We want to produce syntactically correct CanisterIds to avoid the fuzzer stopping too early, -/// while avoiding biasing the fuzzer too much. -fn somewhat_arbitrary_canister_id(u: &mut Unstructured) -> Result { - match u8::arbitrary(u)? % 4 { - 0 => arbitrary_blob(u), - _ => { - let bytes = arbitrary_variable_length_vector(u, 0..=29)?; - assert_matches!(CanisterId::try_from(&bytes), Ok(_)); - Ok(Blob(bytes)) - } - } -} - -fn arbitrary_variable_length_vector<'a, T: Arbitrary<'a>>( - u: &mut Unstructured<'a>, - length_range: RangeInclusive, -) -> Result> { - let length: usize = u.int_in_range(length_range)?; - let mut result = Vec::with_capacity(length); - for _ in 0..length { - result.push(T::arbitrary(u)?) - } - Ok(result) -} - -fn arbitrary_option_blob<'a>(u: &mut Unstructured<'a>) -> Result> { - Ok(if >::arbitrary(u)? { - Some(arbitrary_blob(u)?) - } else { - None - }) -} - -fn arbitrary_tree_paths( - u: &mut Unstructured, - num_paths_range: RangeInclusive, - num_labels_range: RangeInclusive, -) -> Result> { - let num_paths: usize = u.int_in_range(num_paths_range)?; - let mut result = Vec::with_capacity(num_paths); - for _ in 0..num_paths { - result.push(arbitrary_tree_path(u, num_labels_range.clone())?) - } - Ok(result) -} - -fn arbitrary_tree_path( - u: &mut Unstructured, - num_labels_range: RangeInclusive, -) -> Result { - let num_labels: usize = u.int_in_range(num_labels_range)?; - let mut result = Vec::with_capacity(num_labels); - for _ in 0..num_labels { - result.push(arbitrary_tree_label(u)?) - } - Ok(Path::from(result)) -} - -fn arbitrary_tree_label(u: &mut Unstructured) -> Result