feat(nns): More secure random numbers. (dfinity#1633)

In NNS Governance. This is done by reseeding every hour. The seed is sourced from the raw_rand method of the management canister, and because of this, these seeds are "as secure as it gets". This is a compromise between speed and security that DFINITY's security team is ok with. Currently, there is no need for secure RNs. However, that could come up in the future, and when it does, it would be too easy for us to overlook (without this change). --------- Co-authored-by: IDX GitHub Automation <[email protected]>
levifeldman · Oct 1, 2024 · 29c253a · 29c253a
1 parent 4b1c623
commit 29c253a
Show file tree

Hide file tree

Showing 18 changed files with 399 additions and 124 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/rs/nns/governance/BUILD.bazel b/rs/nns/governance/BUILD.bazel
@@ -56,6 +56,7 @@ DEPENDENCIES = [
     "@crate_index//:dyn-clone",
     "@crate_index//:futures",
     "@crate_index//:ic-cdk",
+    "@crate_index//:ic-cdk-timers",
     "@crate_index//:ic-metrics-encoder",
     "@crate_index//:ic-stable-structures",
     "@crate_index//:itertools",

diff --git a/rs/nns/governance/Cargo.toml b/rs/nns/governance/Cargo.toml
@@ -39,6 +39,7 @@ ic-base-types = { path = "../../types/base_types" }
 ic-canisters-http-types = { path = "../../rust_canisters/http_types" }
 ic-cdk = { workspace = true }
 ic-cdk-macros = { workspace = true }
+ic-cdk-timers = { workspace = true }
 ic-crypto-getrandom-for-wasm = { path = "../../crypto/getrandom_for_wasm" }
 ic-crypto-sha2 = { path = "../../crypto/sha2/" }
 ic-ledger-core = { path = "../../rosetta-api/ledger_core" }

diff --git a/rs/nns/governance/benches/scale.rs b/rs/nns/governance/benches/scale.rs
@@ -20,7 +20,7 @@ use ic_base_types::{CanisterId, PrincipalId};
 use ic_nervous_system_common::{cmc::FakeCmc, ledger::IcpLedger, NervousSystemError};
 use ic_nns_common::pb::v1::NeuronId;
 use ic_nns_governance::{
-    governance::{Environment, Governance, HeapGrowthPotential},
+    governance::{Environment, Governance, HeapGrowthPotential, RngError},
     pb::v1::{
         neuron, proposal, ExecuteNnsFunction, Governance as GovernanceProto, GovernanceError,
         Motion, NetworkEconomics, Neuron, Proposal, Topic,
@@ -48,11 +48,19 @@ impl Environment for MockEnvironment {
         self.secs
     }
 
-    fn random_u64(&mut self) -> u64 {
+    fn random_u64(&mut self) -> Result<u64, RngError> {
         todo!()
     }
 
-    fn random_byte_array(&mut self) -> [u8; 32] {
+    fn random_byte_array(&mut self) -> Result<[u8; 32], RngError> {
+        todo!()
+    }
+
+    fn seed_rng(&mut self, _seed: [u8; 32]) {
+        todo!()
+    }
+
+    fn get_rng_seed(&self) -> Option<[u8; 32]> {
         todo!()
     }
 

diff --git a/rs/nns/governance/canister/canister.rs b/rs/nns/governance/canister/canister.rs
@@ -6,6 +6,7 @@ use ic_cdk::{
     api::call::arg_data_raw, caller as ic_cdk_caller, heartbeat, post_upgrade, pre_upgrade,
     println, query, spawn, update,
 };
+use ic_management_canister_types::IC_00;
 use ic_nervous_system_canisters::{cmc::CMCCanister, ledger::IcpLedgerCanister};
 use ic_nervous_system_common::{
     memory_manager_upgrade_storage::{load_protobuf, store_protobuf},
@@ -20,7 +21,7 @@ use ic_nns_common::{
 use ic_nns_constants::LEDGER_CANISTER_ID;
 use ic_nns_governance::{
     decoder_config, encode_metrics,
-    governance::{Environment, Governance, HeapGrowthPotential, TimeWarp as GovTimeWarp},
+    governance::{Environment, Governance, HeapGrowthPotential, RngError, TimeWarp as GovTimeWarp},
     neuron_data_validation::NeuronDataValidationSummary,
     pb::v1::{self as gov_pb, Governance as InternalGovernanceProto},
     storage::{grow_upgrades_memory_to, validate_stable_storage, with_upgrades_memory},
@@ -149,8 +150,38 @@ fn set_governance(gov: Governance) {
         .expect("Error initializing the governance canister.");
 }
 
+// Seeding interval seeks to find a balance between the need for rng secrecy, and
+// avoiding the overhead of frequent reseeding.
+const SEEDING_INTERVAL: Duration = Duration::from_secs(3600);
+const RETRY_SEEDING_INTERVAL: Duration = Duration::from_secs(30);
+
+fn schedule_seeding(duration: Duration) {
+    ic_cdk_timers::set_timer(duration, || {
+        spawn(async {
+            let result: Result<([u8; 32],), (i32, String)> =
+                CdkRuntime::call_with_cleanup(IC_00, "raw_rand", ()).await;
+
+            let seed = match result {
+                Ok((seed,)) => seed,
+                Err((code, msg)) => {
+                    println!(
+                        "{}Error seeding RNG. Error Code: {}. Error Message: {}",
+                        LOG_PREFIX, code, msg
+                    );
+                    schedule_seeding(RETRY_SEEDING_INTERVAL);
+                    return;
+                }
+            };
+
+            () = governance_mut().env.seed_rng(seed);
+            // Schedule reseeding on a timer with duration SEEDING_INTERVAL
+            schedule_seeding(SEEDING_INTERVAL);
+        })
+    });
+}
+
 struct CanisterEnv {
-    rng: ChaCha20Rng,
+    rng: Option<ChaCha20Rng>,
     time_warp: GovTimeWarp,
 }
 
@@ -174,23 +205,7 @@ fn now_seconds() -> u64 {
 impl CanisterEnv {
     fn new() -> Self {
         CanisterEnv {
-            // Seed the PRNG with the current time.
-            //
-            // This is safe since all replicas are guaranteed to see the same result of time()
-            // and it isn't easily predictable from the outside.
-            //
-            // Using raw_rand from the ic00 api is an asynchronous call so can't really be
-            // used to generate random numbers for most cases. It could be used to seed
-            // the PRNG, but that wouldn't help much since after inception the pseudo-random
-            // numbers could be predicted.
-            rng: {
-                let now_nanos = Duration::from_nanos(now_nanoseconds()).as_nanos();
-                let mut seed = [0u8; 32];
-                seed[..16].copy_from_slice(&now_nanos.to_be_bytes());
-                seed[16..32].copy_from_slice(&now_nanos.to_be_bytes());
-                ChaCha20Rng::from_seed(seed)
-            },
-
+            rng: None,
             time_warp: GovTimeWarp { delta_s: 0 },
         }
     }
@@ -206,14 +221,30 @@ impl Environment for CanisterEnv {
         self.time_warp = new_time_warp;
     }
 
-    fn random_u64(&mut self) -> u64 {
-        self.rng.next_u64()
+    fn random_u64(&mut self) -> Result<u64, RngError> {
+        match self.rng.as_mut() {
+            Some(rand) => Ok(rand.next_u64()),
+            None => Err(RngError::RngNotInitialized),
+        }
+    }
+
+    fn random_byte_array(&mut self) -> Result<[u8; 32], RngError> {
+        match self.rng.as_mut() {
+            Some(rand) => {
+                let mut bytes = [0u8; 32];
+                rand.fill_bytes(&mut bytes);
+                Ok(bytes)
+            }
+            None => Err(RngError::RngNotInitialized),
+        }
     }
 
-    fn random_byte_array(&mut self) -> [u8; 32] {
-        let mut bytes = [0u8; 32];
-        self.rng.fill_bytes(&mut bytes);
-        bytes
+    fn seed_rng(&mut self, seed: [u8; 32]) {
+        self.rng.replace(ChaCha20Rng::from_seed(seed));
+    }
+
+    fn get_rng_seed(&self) -> Option<[u8; 32]> {
+        self.rng.as_ref().map(|rng| rng.get_seed())
     }
 
     fn execute_nns_function(
@@ -369,6 +400,7 @@ fn canister_init_(init_payload: ApiGovernanceProto) {
         init_payload.neurons.len()
     );
 
+    schedule_seeding(Duration::from_nanos(0));
     set_governance(Governance::new(
         InternalGovernanceProto::from(init_payload),
         Box::new(CanisterEnv::new()),
@@ -411,6 +443,8 @@ fn canister_post_upgrade() {
         restored_state.neurons.len(),
         restored_state.xdr_conversion_rate,
     );
+
+    schedule_seeding(Duration::from_nanos(0));
     set_governance(Governance::new_restored(
         restored_state,
         Box::new(CanisterEnv::new()),

diff --git a/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto b/rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto
@@ -2415,6 +2415,10 @@ message Governance {
 
   // The summary of restore aging event.
   optional RestoreAgingSummary restore_aging_summary = 27;
+
+  // Used to initialize an internal pseudorandom number generator. This gets replaced periodically using a secure
+  // source of randomness (from the platform)
+  optional bytes rng_seed = 28;
 }
 
 message XdrConversionRate {

diff --git a/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs b/rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs
@@ -3261,6 +3261,10 @@ pub struct Governance {
     /// The summary of restore aging event.
     #[prost(message, optional, tag = "27")]
     pub restore_aging_summary: ::core::option::Option<RestoreAgingSummary>,
+    /// Used to initialize an internal pseudorandom number generator. This gets replaced periodically using a secure
+    /// source of randomness (from the platform)
+    #[prost(bytes = "vec", optional, tag = "28")]
+    pub rng_seed: ::core::option::Option<::prost::alloc::vec::Vec<u8>>,
 }
 /// Nested message and enum types in `Governance`.
 pub mod governance {