signing: access private key only once (#330)

Pass a Signing() object 'signer' to Package() and Package.sign_firmware(), instead of a 'key_file' string (path). Access the private key only once, when generating 'signer', which: 1. Fixes a security issue where caller must write private key to disk instead of passing it purely in memory eg: nrfutil --key-file $(secure-key-retrieval) 2. Gives a small speed improvement. Fixes #327 Signed-off-by: Sirio Balmelli <sirio@b-ad.ch>
NordicSemiconductor · Aug 5, 2020 · a540fe4 · a540fe4
1 parent a0034f2
commit a540fe4
Show file tree

Hide file tree

Showing 5 changed files with 79 additions and 48 deletions.
diff --git a/nordicsemi/__main__.py b/nordicsemi/__main__.py
@@ -373,9 +373,16 @@ def generate(hex_file,
     if bl_settings_version == 1 and (app_boot_validation or sd_boot_validation):
         raise click.BadParameter("Bootloader settings version 1 does not support boot validation.", param_hint='bl_settings_version')
 
-    if (app_boot_validation == 'VALIDATE_ECDSA_P256_SHA256' and key_file is None) or \
-        (sd_boot_validation == 'VALIDATE_ECDSA_P256_SHA256' and key_file is None):
-        raise click.UsageError("Key file must be given when 'VALIDATE_ECDSA_P256_SHA256' boot validation is used")
+    # load signing key (if needed) only once
+    if 'VALIDATE_ECDSA_P256_SHA256' in (app_boot_validation, sd_boot_validation):
+        if not os.path.isfile(key_file):
+            raise click.UsageError("Key file must be given when 'VALIDATE_ECDSA_P256_SHA256' boot validation is used")
+        signer = Signing()
+        default_key = signer.load_key(key_file)
+        if default_key:
+            display_sec_warning()
+    else:
+        signer = None
 
     if app_boot_validation and not application:
         raise click.UsageError("--application hex file must be set when using --app_boot_validation")
@@ -393,7 +400,7 @@ def generate(hex_file,
     sett.generate(arch=family, app_file=application, app_ver=application_version_internal, bl_ver=bootloader_version,
                   bl_sett_ver=bl_settings_version, custom_bl_sett_addr=start_address, no_backup=no_backup,
                   backup_address=backup_address, app_boot_validation_type=app_boot_validation,
-                  sd_boot_validation_type=sd_boot_validation, sd_file=softdevice, key_file=key_file)
+                  sd_boot_validation_type=sd_boot_validation, sd_file=softdevice, signer=signer)
     sett.tohexfile(hex_file)
 
     click.echo("\nGenerated Bootloader DFU settings .hex file and stored it in: {}".format(hex_file))
@@ -809,6 +816,7 @@ def generate(zipfile,
 
     if key_file is None:
         display_nokey_warning()
+        signer = None
     else:
         signer = Signing()
         default_key = signer.load_key(key_file)
@@ -869,7 +877,7 @@ def generate(zipfile,
                       softdevice,
                       sd_boot_validation,
                       app_boot_validation,
-                      key_file,
+                      signer,
                       inner_external_app,
                       zigbee,
                       zigbee_manufacturer_id,
@@ -903,7 +911,7 @@ def generate(zipfile,
                           None,
                           None,
                           None,
-                          key_file,
+                          signer,
                           True)
 
         package.generate_package(zipfile_path)

diff --git a/nordicsemi/dfu/bl_dfu_sett.py b/nordicsemi/dfu/bl_dfu_sett.py
@@ -173,7 +173,7 @@ def _calculate_crc32_from_hex(self, ih_object, start_addr=None, end_addr=None):
         return binascii.crc32(bytearray(list)) & 0xFFFFFFFF
 
     def generate(self, arch, app_file, app_ver, bl_ver, bl_sett_ver, custom_bl_sett_addr, no_backup,
-                 backup_address, app_boot_validation_type, sd_boot_validation_type, sd_file, key_file):
+                 backup_address, app_boot_validation_type, sd_boot_validation_type, sd_file, signer):
 
         self.set_arch(arch)
 
@@ -217,7 +217,7 @@ def generate(self, arch, app_file, app_ver, bl_ver, bl_sett_ver, custom_bl_sett_
                 self.app_boot_validation_bytes = Package.calculate_sha256_hash(self.app_bin)[::-1]
             elif app_boot_validation_type == 'VALIDATE_ECDSA_P256_SHA256':
                 self.app_boot_validation_type = 3 & 0xffffffff
-                self.app_boot_validation_bytes = Package.sign_firmware(key_file, self.app_bin)
+                self.app_boot_validation_bytes = Package.sign_firmware(signer, self.app_bin)
             else:  # This also covers 'NO_VALIDATION' case
                 self.app_boot_validation_type = 0 & 0xffffffff
                 self.app_boot_validation_bytes = bytes(0)
@@ -257,7 +257,7 @@ def generate(self, arch, app_file, app_ver, bl_ver, bl_sett_ver, custom_bl_sett_
                 self.sd_boot_validation_bytes = Package.calculate_sha256_hash(self.sd_bin)[::-1]
             elif sd_boot_validation_type == 'VALIDATE_ECDSA_P256_SHA256':
                 self.sd_boot_validation_type = 3 & 0xffffffff
-                self.sd_boot_validation_bytes = Package.sign_firmware(key_file, self.sd_bin)
+                self.sd_boot_validation_bytes = Package.sign_firmware(signer, self.sd_bin)
             else:  # This also covers 'NO_VALIDATION_CASE'
                 self.sd_boot_validation_type = 0 & 0xffffffff
                 self.sd_boot_validation_bytes = bytes(0)

diff --git a/nordicsemi/dfu/package.py b/nordicsemi/dfu/package.py
@@ -127,7 +127,7 @@ def __init__(self,
                  softdevice_fw=None,
                  sd_boot_validation=DEFAULT_BOOT_VALIDATION_TYPE,
                  app_boot_validation=DEFAULT_BOOT_VALIDATION_TYPE,
-                 key_file=None,
+                 signer=None,
                  is_external=False,
                  zigbee_format=False,
                  manufacturer_id=0,
@@ -148,7 +148,7 @@ def __init__(self,
         :param str app_fw: Path to application firmware file
         :param str bootloader_fw: Path to bootloader firmware file
         :param str softdevice_fw: Path to softdevice firmware file
-        :param str key_file: Path to Signing key file (PEM)
+        :param Signing signer: Instance of Signing() for Signing key file (PEM)
         :param int zigbee_ota_min_hw_version: Minimal zigbee ota hardware version
         :param int zigbee_ota_max_hw_version: Maximum zigbee ota hardware version
         :return: None
@@ -201,7 +201,8 @@ def __init__(self,
                                      boot_validation_type=sd_boot_validation_type,
                                      init_packet_data=init_packet_vars)
 
-        self.key_file = key_file
+        assert(not signer or isinstance(signer, Signing))
+        self.signer = signer
 
         self.work_dir = None
         self.manifest = None
@@ -436,9 +437,9 @@ def generate_package(self, filename, preserve_work_dir=False):
             for x in boot_validation_type_array:
                 if x  == ValidationTypes.VALIDATE_ECDSA_P256_SHA256:
                     if key == HexType.SD_BL:
-                        boot_validation_bytes_array.append(Package.sign_firmware(self.key_file, sd_bin_path))
+                        boot_validation_bytes_array.append(Package.sign_firmware(self.signer, sd_bin_path))
                     else:
-                        boot_validation_bytes_array.append(Package.sign_firmware(self.key_file, bin_file_path))
+                        boot_validation_bytes_array.append(Package.sign_firmware(self.signer, bin_file_path))
                 else:
                     boot_validation_bytes_array.append(b'')
 
@@ -458,10 +459,8 @@ def generate_package(self, filename, preserve_work_dir=False):
                             bl_size=bl_size,
                             sd_req=firmware_data[FirmwareKeys.INIT_PACKET_DATA][PacketField.REQUIRED_SOFTDEVICES_ARRAY])
 
-            if (self.key_file is not None):
-                signer = Signing()
-                signer.load_key(self.key_file)
-                signature = signer.sign(init_packet.get_init_command_bytes())
+            if (self.signer is not None):
+                signature = self.signer.sign(init_packet.get_init_command_bytes())
                 init_packet.set_signature(signature, SigningTypes.ECDSA_P256_SHA256)
 
             # Store the .dat file in the work directory
@@ -573,12 +572,11 @@ def calculate_crc(crc, firmware_filename):
             raise ValueError("Invalid CRC type")
 
     @staticmethod
-    def sign_firmware(key, firmware_filename):
+    def sign_firmware(signer, firmware_filename):
+        assert(isinstance(signer, Signing))
         data_buffer = b''
         with open(firmware_filename, 'rb') as firmware_file:
             data_buffer = firmware_file.read()
-        signer = Signing()
-        signer.load_key(key)
         return signer.sign(data_buffer)
 
     def create_manifest(self):

diff --git a/nordicsemi/dfu/tests/test_bl_dfu_sett.py b/nordicsemi/dfu/tests/test_bl_dfu_sett.py
@@ -41,6 +41,7 @@
 import unittest
 from nordicsemi.dfu.bl_dfu_sett import BLDFUSettings
 from nordicsemi.dfu.nrfhex import nRFArch
+from nordicsemi.dfu.signing import Signing
 
 
 class TestBLDFUSettingsV1(unittest.TestCase):
@@ -79,7 +80,7 @@ def test_generate_without_application_file(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(nRFArch.NRF52, settings.arch)
         self.assertEqual('nRF52', settings.arch_str)
@@ -107,7 +108,7 @@ def test_generate_with_application_file(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(nRFArch.NRF52, settings.arch)
         self.assertEqual('nRF52', settings.arch_str)
@@ -135,7 +136,7 @@ def test_generate_with_custom_start_address(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(settings.bl_sett_addr, 0x0006F000)
 
@@ -152,7 +153,7 @@ def test_generate_with_backup_page_check_size(self):
                              app_boot_validation_type=None,
                              sd_boot_validation_type=None,
                              sd_file=None,
-                             key_file=None)
+                             signer=None)
 
         settings = BLDFUSettings()
         settings.generate(arch='NRF52',
@@ -166,7 +167,7 @@ def test_generate_with_backup_page_check_size(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(len(list(settings.ihex.todict().keys())), len(list(settings_raw.ihex.todict().keys())) * 2)
 
@@ -183,7 +184,7 @@ def test_generate_with_backup_page_check_values(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         backup_dict = {(k + settings.bl_sett_backup_offset): v for k, v in list(settings.ihex.todict().items()) if k < settings.bl_sett_addr}
         settings_dict = {k: v for k, v in list(settings.ihex.todict().items()) if k >= settings.bl_sett_addr}
@@ -202,7 +203,7 @@ def test_generate_with_backup_page_custom_address(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(settings.backup_address, 0x0006F000)
         self.assertTrue(0x0006F000 in list(settings.ihex.todict().keys()))
@@ -220,7 +221,7 @@ def test_generate_with_backup_page_default_address(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(settings.backup_address, (0x0006F000 - settings.bl_sett_backup_offset))
         self.assertTrue((0x0006F000 - settings.bl_sett_backup_offset) in list(settings.ihex.todict().keys()))
@@ -267,7 +268,7 @@ def test_generate_without_application_or_sd_file(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(nRFArch.NRF52, settings.arch)
         self.assertEqual('nRF52', settings.arch_str)
@@ -301,7 +302,7 @@ def test_generate_with_application_and_sd_file(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type=None,
                           sd_file='firmwares/s132_nrf52_mini.hex',
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(nRFArch.NRF52, settings.arch)
         self.assertEqual('nRF52', settings.arch_str)
@@ -335,7 +336,7 @@ def test_generate_with_custom_start_address(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(settings.bl_sett_addr, 0x0006F000)
 
@@ -352,7 +353,7 @@ def test_generate_with_backup_page_check_size(self):
                              app_boot_validation_type=None,
                              sd_boot_validation_type=None,
                              sd_file=None,
-                             key_file=None)
+                             signer=None)
 
         settings = BLDFUSettings()
         settings.generate(arch='NRF52',
@@ -366,7 +367,7 @@ def test_generate_with_backup_page_check_size(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(len(list(settings.ihex.todict().keys())), len(list(settings_raw.ihex.todict().keys())) * 2)
 
@@ -383,7 +384,7 @@ def test_generate_with_backup_page_check_values(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         backup_dict = {(k + settings.bl_sett_backup_offset): v for k, v in list(settings.ihex.todict().items()) if k < settings.bl_sett_addr}
         settings_dict = {k: v for k, v in list(settings.ihex.todict().items()) if k >= settings.bl_sett_addr}
@@ -402,7 +403,7 @@ def test_generate_with_backup_page_custom_address(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(settings.backup_address, 0x0006F000)
         self.assertTrue(0x0006F000 in list(settings.ihex.todict().keys()))
@@ -420,7 +421,7 @@ def test_generate_with_backup_page_default_address(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(settings.backup_address, (0x0006F000 - settings.bl_sett_backup_offset))
         self.assertTrue((0x0006F000 - settings.bl_sett_backup_offset) in list(settings.ihex.todict().keys()))
@@ -438,7 +439,7 @@ def test_generate_with_app_boot_validation_crc(self):
                           app_boot_validation_type='VALIDATE_GENERATED_CRC',
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(0x1316CFD0, settings.crc)
         self.assertEqual(0x49A0F45A, settings.boot_validation_crc)
@@ -458,7 +459,7 @@ def test_generate_with_app_boot_validation_sha256(self):
                           app_boot_validation_type='VALIDATE_GENERATED_SHA256',
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(0x1316CFD0, settings.crc)
         self.assertEqual(0xF78E451E, settings.boot_validation_crc)
@@ -468,6 +469,10 @@ def test_generate_with_app_boot_validation_sha256(self):
 
     def test_generate_with_app_boot_validation_ecdsa(self):
         settings = BLDFUSettings()
+
+        signer = Signing()
+        signer.load_key('key.pem')
+
         settings.generate(arch='NRF52',
                           app_file='firmwares/s132_nrf52_mini.hex',
                           app_ver=1,
@@ -479,7 +484,7 @@ def test_generate_with_app_boot_validation_ecdsa(self):
                           app_boot_validation_type='VALIDATE_ECDSA_P256_SHA256',
                           sd_boot_validation_type=None,
                           sd_file=None,
-                          key_file='key.pem')
+                          signer=signer)
 
         # Since ECDSA contains a random component the signature will be different every time
         # it is generated. Therefore only overall structure of the boot validation will be checked.
@@ -499,7 +504,7 @@ def test_generate_with_sd_boot_validation_crc(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type='VALIDATE_GENERATED_CRC',
                           sd_file='firmwares/s132_nrf52_mini.hex',
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(0x4637019F, settings.crc)
         self.assertEqual(0xCB5F90FB, settings.boot_validation_crc)
@@ -519,7 +524,7 @@ def test_generate_with_sd_boot_validation_sha256(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type='VALIDATE_GENERATED_SHA256',
                           sd_file='firmwares/s132_nrf52_mini.hex',
-                          key_file=None)
+                          signer=None)
 
         self.assertEqual(0x4637019F, settings.crc)
         self.assertEqual(0x9C761426, settings.boot_validation_crc)
@@ -529,6 +534,10 @@ def test_generate_with_sd_boot_validation_sha256(self):
 
     def test_generate_with_sd_boot_validation_ecdsa(self):
         settings = BLDFUSettings()
+
+        signer = Signing()
+        signer.load_key('key.pem')
+
         settings.generate(arch='NRF52',
                           app_file=None,
                           app_ver=1,
@@ -540,7 +549,7 @@ def test_generate_with_sd_boot_validation_ecdsa(self):
                           app_boot_validation_type=None,
                           sd_boot_validation_type='VALIDATE_ECDSA_P256_SHA256',
                           sd_file='firmwares/s132_nrf52_mini.hex',
-                          key_file='key.pem')
+                          signer=signer)
 
         # Since ECDSA contains a random component the signature will be different every time
         # it is generated. Therefore only overall structure of the boot validation will be checked.