security: Fix bug and gate it properly via asserts and warnings. Reported by @mkj, re-reviewed by @ramrunner, thanks and kudos to both

Author: brainstorm

ssh-stamp-esp32/src/bin/ssh-stamp-esp32.rs

@@ -87,14 +87,35 @@ async fn main(spawner: Spawner) -> ! {
     cfg_if::cfg_if! {
         if #[cfg(any(feature = "esp32c5", feature = "esp32c61"))] {
             // ESP32-C5/C61 have no TRNG peripheral — use the basic Rng directly.
+            // Until the TODO above is resolved, key material on these chips is
+            // only as good as the bare RNG register, so say so out loud.
+            warn!("No TRNG on this chip: RNG is not cryptographically secure until the radio is up");
             let rng = esp_hal::rng::Rng::new();
             register_custom_rng(rng);
         } else {
+            // The RNG register only yields true randomness while an entropy
+            // source is active. There are two, and this firmware uses both in
+            // sequence:
+            //
+            //   1. the SAR ADC source enabled here, covering early boot, and
+            //   2. the RF subsystem, once WiFi is up.
+            //
+            // `Trng::downgrade` returns `Rng`, a zero-sized handle that just
+            // reads the register, so it carries no guarantee of its own —
+            // whichever source is live at the time is what decides quality.
+            //
+            // The ADC source must therefore stay enabled until the radio takes
+            // over, because everything minted in between depends on it: the
+            // SSH host key, WiFi SSID/PSK and MAC all come from
+            // `store::load_or_create` and `prepare_ap_config` below. It is
+            // handed over (and explicitly dropped) just before the radio is
+            // initialised — see the drop site further down for why it cannot
+            // simply be left running.
             let trng_source = TrngSource::new(peripherals.RNG, peripherals.ADC1);
-            let trng = Trng::try_new().unwrap();
+            let trng = Trng::try_new()
+                .expect("TrngSource was just created, so the TRNG must be available");
             let rng = trng.downgrade();
             register_custom_rng(rng);
-            drop(trng_source);
         }
     }
 
@@ -125,6 +146,18 @@ async fn main(spawner: Spawner) -> ! {
         tx: tx_num,
     };
 
+    // On first boot this mints the SSH host key and the WiFi PSK, so the
+    // entropy source enabled above has to still be running. Guard the
+    // invariant rather than trusting a comment: `debug-assertions` are on
+    // even in release for this workspace, so reintroducing an early drop of
+    // the `TrngSource` fails loudly on the bench instead of silently
+    // producing predictable keys.
+    #[cfg(not(any(feature = "esp32c5", feature = "esp32c61")))]
+    debug_assert!(
+        TrngSource::is_enabled(),
+        "entropy source was disabled before host key generation"
+    );
+
     debug!("Loading config");
     let flash_config = {
         let Some(flash_storage_guard) = flash::get_flash_n_buffer() else {
@@ -197,10 +230,31 @@ async fn main(spawner: Spawner) -> ! {
 
     debug!("Initialising radio");
 
+    // Last consumer of randomness before the radio: mints the WiFi PSK if the
+    // config did not already carry one.
     let ap_config = app::prepare_ap_config(config, &platform)
         .await
         .expect("Failed to prepare AP config");
 
+    // Hand the entropy source over to the radio.
+    //
+    // The SAR ADC source cannot simply be left running: Espressif requires it
+    // to be switched off "before RF subsystem features, ADC, or I2S (ESP32
+    // only) are initialized", warning that it "is not safe to use if any other
+    // subsystem is accessing the RF subsystem or the ADC at the same time"
+    // (ESP-IDF, Random Number Generation). It also commandeers the SAR ADC —
+    // and I2S0 on the classic ESP32 — which the radio needs back.
+    //
+    // Nothing is lost by dropping it here: `WifiController::new` inside
+    // `bring_up()` enables the RF subsystem, which is itself an entropy
+    // source, and esp-radio registers that fact with esp-hal. So the SSH
+    // session and key-exchange material sunset draws per connection is still
+    // covered, just by the radio rather than the ADC.
+    //
+    // https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/system/random.html
+    #[cfg(not(any(feature = "esp32c5", feature = "esp32c61")))]
+    drop(trng_source);
+
     let mut wifi = EspWifi::new(spawner, peripherals.WIFI, rng, DEFAULT_IP);
     wifi.configure_ap(ap_config)
         .expect("Failed to configure AP");
@@ -220,6 +274,17 @@ async fn main(spawner: Spawner) -> ! {
         }
     }
 
+    // The radio should have picked up the entropy duty dropped above:
+    // esp-radio bumps esp-hal's entropy-source count once the RF subsystem is
+    // running. sunset draws fresh key-exchange material from `getrandom` for
+    // every SSH connection served below, so if this does not hold the handover
+    // has a hole in it.
+    #[cfg(not(any(feature = "esp32c5", feature = "esp32c61")))]
+    debug_assert!(
+        TrngSource::is_enabled(),
+        "no entropy source active after WiFi came up"
+    );
+
     if let Err(e) = app::run_app(stack.unwrap(), uart_buf, config, &platform).await {
         error!("run_app exited with error: {e}");
     }