From d9a0dbf14dbc05ac735173fea69266b82d5a2594 Mon Sep 17 00:00:00 2001 From: Eric Swanson Date: Wed, 6 Mar 2024 17:04:07 -0800 Subject: [PATCH 1/3] fix: fetch_root_key refuses to play along if called on the mainnet --- CHANGELOG.md | 1 + ic-agent/src/agent/agent_error.rs | 5 +++++ ic-agent/src/agent/agent_test.rs | 15 +++++++++++++++ ic-agent/src/agent/mod.rs | 7 ++++++- 4 files changed, 27 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad8e8dc0..89fdcf93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Added node metrics, ECDSA, and Bitcoin functions to `MgmtMethod`. Most do not have wrappers in `ManagementCanister` because only canisters can call these functions. * Added `FetchCanisterLogs` function to `MgmtMethod` and a corresponding wrapper to `ManagementCanister`. * Updated the `ring` crate to 0.17.7. `ring` 0.16 has a bug where it requires incorrect Ed25519 PEM encoding. 0.17.7 fixes that and is backwards compatible. +* Agent::fetch_root_key() now returns an error, and sets its root key to an empty vector, if called on the mainnet. ## [0.33.0] - 2024-02-08 diff --git a/ic-agent/src/agent/agent_error.rs b/ic-agent/src/agent/agent_error.rs index 70477f05..892ca0a2 100644 --- a/ic-agent/src/agent/agent_error.rs +++ b/ic-agent/src/agent/agent_error.rs @@ -201,6 +201,11 @@ pub enum AgentError { /// Route provider failed to generate a url for some reason. #[error("Route provider failed to generate url: {0}")] RouteProviderError(String), + + /// It's an error to fetch the root key on the mainnet. + /// Doing so would enable a man-in-the-middle attack. + #[error("Never fetch the root key on the mainnet")] + NeverFetchRootKeyOnMainNet(), } impl PartialEq for AgentError { diff --git a/ic-agent/src/agent/agent_test.rs b/ic-agent/src/agent/agent_test.rs index d6939d37..34404df0 100644 --- a/ic-agent/src/agent/agent_test.rs +++ b/ic-agent/src/agent/agent_test.rs @@ -42,6 +42,21 @@ fn make_certifying_agent(url: &str) -> Agent { .unwrap() } +#[cfg_attr(not(target_family = "wasm"), tokio::test)] +#[cfg_attr(target_family = "wasm", wasm_bindgen_test)] +async fn refuse_to_install_mainnet_root_key() -> Result<(), AgentError> { + let url = "https://icp0.io"; + + let agent = make_agent(&url); + let result = agent.fetch_root_key().await; + assert!(matches!( + result, + Err(AgentError::NeverFetchRootKeyOnMainNet()) + )); + assert_eq!(agent.read_root_key(), Vec::::new()); + Ok(()) +} + #[cfg_attr(not(target_family = "wasm"), tokio::test)] #[cfg_attr(target_family = "wasm", wasm_bindgen_test)] async fn query() -> Result<(), AgentError> { diff --git a/ic-agent/src/agent/mod.rs b/ic-agent/src/agent/mod.rs index 3c859f04..3e61fb71 100644 --- a/ic-agent/src/agent/mod.rs +++ b/ic-agent/src/agent/mod.rs @@ -330,6 +330,11 @@ impl Agent { } let status = self.status().await?; let root_key = match status.root_key { + Some(key) if key[..] == IC_ROOT_KEY[..] => { + // even if the caller ignores this error, we're done here. + self.set_root_key(vec![]); + return Err(AgentError::NeverFetchRootKeyOnMainNet()); + } Some(key) => key, None => return Err(AgentError::NoRootKeyInStatus(status)), }; @@ -340,7 +345,7 @@ impl Agent { /// By default, the agent is configured to talk to the main Internet Computer, and verifies /// responses using a hard-coded public key. /// - /// Using this function you can set the root key to a known one if you know if beforehand. + /// Using this function you can set the root key to a known one if you know it beforehand. pub fn set_root_key(&self, root_key: Vec) { *self.root_key.write().unwrap() = root_key; } From d7fee85f1dfc3fba17d8c420e3e89dd95d5505b8 Mon Sep 17 00:00:00 2001 From: Eric Swanson Date: Wed, 6 Mar 2024 18:54:09 -0800 Subject: [PATCH 2/3] lint, do not run test on wasm target --- ic-agent/src/agent/agent_test.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ic-agent/src/agent/agent_test.rs b/ic-agent/src/agent/agent_test.rs index 34404df0..a901e18c 100644 --- a/ic-agent/src/agent/agent_test.rs +++ b/ic-agent/src/agent/agent_test.rs @@ -43,11 +43,10 @@ fn make_certifying_agent(url: &str) -> Agent { } #[cfg_attr(not(target_family = "wasm"), tokio::test)] -#[cfg_attr(target_family = "wasm", wasm_bindgen_test)] async fn refuse_to_install_mainnet_root_key() -> Result<(), AgentError> { let url = "https://icp0.io"; - let agent = make_agent(&url); + let agent = make_agent(url); let result = agent.fetch_root_key().await; assert!(matches!( result, From 16f5836b81dff7b69779aacb7131ac341cde4700 Mon Sep 17 00:00:00 2001 From: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com> Date: Mon, 6 May 2024 14:22:52 -0700 Subject: [PATCH 3/3] Update CHANGELOG.md Co-authored-by: Linwei Shang --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89fdcf93..0a092c67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Added node metrics, ECDSA, and Bitcoin functions to `MgmtMethod`. Most do not have wrappers in `ManagementCanister` because only canisters can call these functions. * Added `FetchCanisterLogs` function to `MgmtMethod` and a corresponding wrapper to `ManagementCanister`. * Updated the `ring` crate to 0.17.7. `ring` 0.16 has a bug where it requires incorrect Ed25519 PEM encoding. 0.17.7 fixes that and is backwards compatible. -* Agent::fetch_root_key() now returns an error, and sets its root key to an empty vector, if called on the mainnet. +* Agent::fetch_root_key() now returns an error, and sets its root key to an empty vector, if got the same root key as the mainnet. This reduce the potentiality of MITM attack. ## [0.33.0] - 2024-02-08