messaging: stop BLE once authenticated or OTAing over serial or QR

If a user successfully authenticates or starts an OTA over over serial
or QR, stop the BLE stack.

Enable BLE started (if so configured) when unit unlocked.

NOTE: also adjust handling in Bluetooth settings screen.

jadepy/jade_ble.py

@@ -104,7 +104,7 @@ async def _input_stream():
 
         # Connect - seems pretty flaky so allow retries
         connected = False
-        attempts_remaining = 3
+        attempts_remaining = 5
         while not connected:
             try:
                 attempts_remaining -= 1
@@ -118,7 +118,9 @@ async def _input_stream():
                 logger.warning("BLE connection exception: '{}'".format(e))
                 if not attempts_remaining:
                     logger.warning("Exhausted retries - BLE connection failed")
-                    raise
+                    raise JadeError(2, "Unable to connect to BLE device",
+                                    "Device name: {}, Serial number: {}".format(
+                                        self.device_name, self.serial_number or '<any>'))
 
         # Peruse services and characteristics
         # Get the 'handle' of the receiving charactersitic

jadepy/jade_serial.py

@@ -2,6 +2,7 @@
 import logging
 
 from serial.tools import list_ports
+from .jade_error import JadeError
 
 logger = logging.getLogger(__name__)
 
@@ -53,7 +54,10 @@ def connect(self):
         assert self.ser is not None
 
         if not self.ser.is_open:
-            self.ser.open()
+            try:
+                self.ser.open()
+            except serial.serialutil.SerialException:
+                raise JadeError(1, "Unable to open port", self.device)
 
         # Ensure RTS and DTR are not set (as this can cause the hw to reboot)
         self.ser.setRTS(False)

main/process/dashboard.c

@@ -29,6 +29,7 @@
 static inline bool ble_enabled(void) { return false; }
 static inline bool ble_connected(void) { return false; }
 static inline void ble_start(void) { JADE_ASSERT(false); }
+static inline void ble_stop(void) { return; }
 #endif
 #include "process/ota_defines.h"
 #include "process_utils.h"
@@ -227,6 +228,53 @@ void build_version_info_reply(const void* ctx, CborEncoder* container);
 // Set flag to change PIN on next successful unlock
 void set_request_change_pin(bool change_pin);
 
+static void process_get_version_info_request(jade_process_t* process)
+{
+    ASSERT_CURRENT_MESSAGE(process, "get_version_info");
+    jade_process_reply_to_message_result(process->ctx, &process->ctx.source, build_version_info_reply);
+}
+
+// If the user has successfully authenticated over a given connection interface,
+// we stop/close the 'other' interface for security and performance/stability reasons.
+// If the user is not authenticated or tied to a given interface, enable all.
+static void enable_connection_interfaces(const jade_msg_source_t source)
+{
+    JADE_LOGD("enable_connection_interfaces(%u)", source);
+
+    if (source == SOURCE_SERIAL || (source == SOURCE_NONE && internal_relogin_source == SOURCE_SERIAL)) {
+        // serial_start();
+        ble_stop();
+    } else if (source == SOURCE_BLE || (source == SOURCE_NONE && internal_relogin_source == SOURCE_BLE)) {
+        ble_start();
+#ifdef CONFIG_LOG_DEFAULT_LEVEL_NONE // Leave serial up if used for logging
+        // serial_stop();
+#endif
+    } else if (source == SOURCE_INTERNAL || (source == SOURCE_NONE && internal_relogin_source == SOURCE_INTERNAL)) {
+        ble_stop();
+#ifdef CONFIG_LOG_DEFAULT_LEVEL_NONE // Leave serial up if used for logging
+        // serial_stop();
+#endif
+    } else { // ie. SOURCE_NONE
+        JADE_ASSERT(internal_relogin_source == SOURCE_NONE);
+
+        // Ensure serial running, and start BLE if configured but not running
+        // serial_start();
+
+#ifdef CONFIG_BT_ENABLED
+        if (!ble_enabled()) {
+#ifndef CONFIG_DEBUG_UNATTENDED_CI
+            const uint8_t ble_flags = storage_get_ble_flags();
+#else
+            const uint8_t ble_flags = BLE_ENABLED;
+#endif
+            if (ble_flags & BLE_ENABLED) {
+                ble_start();
+            }
+        }
+#endif
+    }
+}
+
 // Home screen/menu update
 static void update_home_screen(gui_view_node_t* status_light, gui_view_node_t* status_text, gui_view_node_t* label)
 {
@@ -299,12 +347,6 @@ static void format_pin(char* buf, const uint8_t buf_len, const uint8_t* pin, con
     }
 }
 
-static void process_get_version_info_request(jade_process_t* process)
-{
-    ASSERT_CURRENT_MESSAGE(process, "get_version_info");
-    jade_process_reply_to_message_result(process->ctx, &process->ctx.source, build_version_info_reply);
-}
-
 // Unpack entropy bytes from message and add to random generator
 static void process_add_entropy_request(jade_process_t* process)
 {
@@ -404,6 +446,9 @@ static void dispatch_message(jade_process_t* process)
         // or
         // b) There is no PIN set (ie. no encrypted keys set, eg. new device)
         if ((KEYCHAIN_UNLOCKED_BY_MESSAGE_SOURCE(process) && !keychain_has_temporary()) || !keychain_has_pin()) {
+            // If we are about to start an OTA we stop the other/unused connection
+            // interface for performance, stabililty and security reasons.
+            enable_connection_interfaces(process->ctx.source);
             task_function = ota_process;
         } else {
             // Reject the message as hw locked
@@ -822,19 +867,30 @@ static void handle_ble_reset(void)
     }
 }
 
+static inline void update_ble_status_item(gui_view_node_t* ble_status_item, const bool enabled)
+{
+    gui_update_text(ble_status_item, enabled ? "Status: Enabled" : "Status: Disabled");
+}
+
+static inline void update_ble_carousel_label(gui_view_node_t* status_textbox, const bool enabled)
+{
+    gui_update_text(status_textbox, enabled ? "Enabled" : "Disabled");
+}
+
 // BLE properties screen
 static void handle_ble(void)
 {
     uint8_t ble_flags = storage_get_ble_flags();
+    bool enabled = (ble_flags & BLE_ENABLED);
 
     gui_view_node_t* ble_status_item = NULL;
     gui_activity_t* const act = make_ble_activity(&ble_status_item);
-    gui_update_text(ble_status_item, ble_enabled() ? "Status: Enabled" : "Status: Disabled");
+    update_ble_status_item(ble_status_item, enabled);
     gui_set_current_activity(act);
 
     gui_view_node_t* status_textbox = NULL;
     gui_activity_t* const act_status = make_carousel_activity("Bluetooth Status", NULL, &status_textbox);
-    gui_update_text(status_textbox, ble_enabled() ? "Enabled" : "Disabled");
+    update_ble_carousel_label(status_textbox, enabled);
 
     int32_t ev_id;
     while (true) {
@@ -852,12 +908,11 @@ static void handle_ble(void)
         if (ret) {
             if (ev_id == BTN_BLE_STATUS) {
                 gui_set_current_activity(act_status);
-                bool enable_ble = ble_enabled();
                 while (true) {
-                    gui_update_text(status_textbox, enable_ble ? "Enabled" : "Disabled");
+                    update_ble_carousel_label(status_textbox, enabled);
                     if (gui_activity_wait_event(act_status, GUI_EVENT, ESP_EVENT_ANY_ID, NULL, &ev_id, NULL, 0)) {
                         if (ev_id == GUI_WHEEL_LEFT_EVENT || ev_id == GUI_WHEEL_RIGHT_EVENT) {
-                            enable_ble = !enable_ble; // Just toggle label at this point
+                            enabled = !enabled; // Just toggle label at this point
                         } else if (ev_id == gui_get_click_event()) {
                             // Done - apply ble change
                             break;
@@ -866,9 +921,15 @@ static void handle_ble(void)
                 }
 
                 // Start/stop BLE and persist pref/flags
-                if (enable_ble) {
+                if (enabled) {
                     if (!ble_enabled()) {
-                        ble_start();
+                        // Only start BLE immediately if not using some other interface
+                        if (keychain_get_userdata() == SOURCE_NONE) {
+                            ble_start();
+                        } else {
+                            const char* message[] = { "Bluetooth will be", "started on logout", "or disconnection" };
+                            await_message_activity(message, 3);
+                        }
                     }
                     ble_flags |= BLE_ENABLED;
                     storage_set_ble_flags(ble_flags);
@@ -879,7 +940,7 @@ static void handle_ble(void)
                     ble_flags &= ~BLE_ENABLED;
                     storage_set_ble_flags(ble_flags);
                 }
-                gui_update_text(ble_status_item, ble_enabled() ? "Status: Enabled" : "Status: Disabled");
+                update_ble_status_item(ble_status_item, enabled);
             } else if (ev_id == BTN_BLE_RESET_PAIRING) {
                 handle_ble_reset();
             } else if (ev_id == BTN_BLE_HELP) {
@@ -2365,6 +2426,10 @@ static void display_screen(jade_process_t* process, gui_activity_t* act)
     // Refeed sensor entropy every time we return to dashboard screen
     const TickType_t tick_count = xTaskGetTickCount();
     refeed_entropy(&tick_count, sizeof(tick_count));
+
+    // Ensure the correct/expected connection interface(s) are enabled
+    // depending on whether the user is authenticated and on which interface.
+    enable_connection_interfaces(keychain_get_userdata());
 }
 
 // Display the dashboard ready or welcome screen.  Await messages or user GUI input.

test_jade.py

@@ -3394,6 +3394,9 @@ def run_api_tests(jadeapi, isble, qemu, authuser=False):
     check_frag = has_psram and not has_ble
     check_mem_stats(startinfo, endinfo, check_frag=check_frag)
 
+    rslt = jadeapi.clean_reset()
+    assert rslt is True
+
 
 # Run tests using passed interface
 def run_interface_tests(jadeapi,
@@ -3434,9 +3437,11 @@ def run_interface_tests(jadeapi,
 
         # Sanity check selfcheck time on Jade hw (skip for qemu)
         # May need updating if more tests added to selfcheck.c
-        time_ms = jadeapi.run_remote_selfcheck()
-        logger.info('selfcheck time: ' + str(time_ms) + 'ms')
-        assert qemu or time_ms < 82500
+        # Don't need to run internal tests more than once (so skip for ble)
+        if not isble:
+            time_ms = jadeapi.run_remote_selfcheck()
+            logger.info('selfcheck time: ' + str(time_ms) + 'ms')
+            assert qemu or time_ms < 82500
 
         # Test good pinserver handshake, and also 'bad sig' pinserver
         test_handshake(jadeapi.jade)
@@ -3484,6 +3489,9 @@ def run_interface_tests(jadeapi,
     check_frag = has_psram and not has_ble
     check_mem_stats(startinfo, endinfo, check_frag=check_frag)
 
+    rslt = jadeapi.clean_reset()
+    assert rslt is True
+
 
 # Run all selected tests over a passed JadeAPI instance.
 def run_jade_tests(jadeapi, args, isble):
@@ -3498,44 +3506,83 @@ def run_jade_tests(jadeapi, args, isble):
         run_api_tests(jadeapi, isble, args.qemu, authuser=args.authuser)
 
 
-# This test should be passed 2 different connections to the same jade hw
-# - both serial and ble connected at the same time.
-# Test that is we auth over one, we can't do sensitive calls over the other.
-def mixed_sources_test(jade1, jade2):
+# This test should be passed 2 different connection details to the same jade hw
+# Test that is we auth over one, we can't connect with the other.
+def mixed_sources_test(serialport, bleid):
+    logger.info("Running 'mixed sources' Tests")
+    SRTIMEOUT = 3  # Use a short timeout
+    SCAN_TIMEOUT = 6
 
     # Example of a 'sensitve' call
     path, network, expected = GET_XPUB_DATA[0]
 
-    # jade1 can unlock jade, then get xpub fine
-    jade1.set_mnemonic(TEST_MNEMONIC)
-    rslt = jade1.get_xpub(network, path)
-    assert rslt == expected
+    # 1. Authorise over serial, check BLE connection fails
+    with JadeAPI.create_serial(serialport, timeout=SRTIMEOUT) as jade_serial:
+        jade_serial.set_mnemonic(TEST_MNEMONIC)
+        time.sleep(1)
 
-    # jade2 gets an error about jade being locked (for them)
-    try:
-        rslt = jade2.get_xpub(network, path)
-        assert False, "Excepted exception from mixed sources test"
-    except JadeError as err:
-        assert err.code == JadeError.HW_LOCKED
+        rslt = jade_serial.get_xpub(network, path)
+        assert rslt == expected
 
-    # jade1 is still fine
-    rslt = jade1.get_xpub(network, path)
-    assert rslt == expected
+        try:
+            # With BLE we shouldn't even be able to scan/connect the device
+            with JadeAPI.create_ble(serial_number=bleid, scan_timeout=SCAN_TIMEOUT) as jade_ble:
+                assert False, "Expected BLE connection to fail as authorised over serial!"
 
-    # Now jade2 unlocks jade - they can get xpub but jade1 now cannot
-    jade2.set_mnemonic(TEST_MNEMONIC)
-    rslt = jade2.get_xpub(network, path)
-    assert rslt == expected
+        except JadeError as err:
+            assert err.code == 1 and "Unable to locate BLE device" in err.message
 
-    try:
-        rslt = jade1.get_xpub(network, path)
-        assert False, "Expected exception from mixed sources test"
-    except JadeError as err:
-        assert err.code == JadeError.HW_LOCKED
+        rslt = jade_serial.logout()
+        assert rslt is True
+
+    # 2. Authorise over BLE, check serial connection can be made, but is limited
+    with JadeAPI.create_ble(serial_number=bleid) as jade_ble:
+        rslt = jade_ble.set_mnemonic(TEST_MNEMONIC)
+        assert rslt
 
-    # jade2 is still fine
-    rslt = jade2.get_xpub(network, path)
-    assert rslt == expected
+        rslt = jade_ble.get_xpub(network, path)
+        assert rslt == expected
+
+        # With serial it can connect but can't make any 'wallet-sensitive' calls
+        # expect 'HW_LOCKED' error over un-authenticated connection
+        with JadeAPI.create_serial(serialport, timeout=SRTIMEOUT) as jade_serial:
+            # Can, for example, get info
+            info = jade_serial.get_version_info()
+            assert len(info) == NUM_VALUES_VERINFO
+
+            try:
+                rslt = jade_serial.get_xpub(network, path)
+                assert False, "Expected exception from mixed sources test"
+            except JadeError as err:
+                assert err.code == JadeError.HW_LOCKED
+
+            # Can unlock with serial, but that will break BLE connection
+            rslt = jade_serial.set_mnemonic(TEST_MNEMONIC)
+            assert rslt
+
+            rslt = jade_serial.get_xpub(network, path)
+            assert rslt == expected
+
+            # BLE connection should now be broken
+            try:
+                rslt = jade_ble.get_xpub(network, path)
+                assert False, "Expected exception from mixed sources test"
+            except AssertionError as err:
+                assert jade_ble.jade.impl.client is None
+
+            rslt = jade_serial.logout()
+            assert rslt is True
+
+    # 3. Check BLE re-enabled, and both interfces can be used when user not authenticated
+    time.sleep(1)
+    with JadeAPI.create_serial(serialport, timeout=SRTIMEOUT) as jade_serial:
+        info1 = jade_serial.get_version_info()
+        with JadeAPI.create_ble(serial_number=bleid) as jade_ble:
+            info2 = jade_ble.get_version_info()
+            info1 = jade_serial.get_version_info()
+
+        rslt = jade_serial.clean_reset()
+        assert rslt is True
 
 
 # Run all selected tests over all selected backends (serial/ble)
@@ -3557,12 +3604,9 @@ def run_all_jade_tests(info, args):
             with JadeAPI.create_ble(serial_number=bleid) as jade:
                 run_jade_tests(jade, args, isble=True)
 
-                # 3. If also testing over serial, run the 'mixed sources' tests
-                if not args.skipserial:
-                    logger.info("Running 'mixed sources' Tests")
-                    with JadeAPI.create_serial(args.serialport,
-                                               timeout=args.serialtimeout) as jadeserial:
-                        mixed_sources_test(jadeserial, jade)
+            # 3. If testing both interfaces, test cannot connect 'other' when one in use
+            if not args.skipserial:
+                mixed_sources_test(args.serialport, bleid)
         else:
             msg = "Skipping BLE tests - not enabled on the hardware"
             logger.warning(msg)