From a540fe45587287be9d61c8b0d2de9ba4a1c19162 Mon Sep 17 00:00:00 2001 From: Sirio Balmelli Date: Wed, 5 Aug 2020 14:54:52 +0200 Subject: [PATCH] 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 NordicSemiconductor/pc-nrfutil#327 Signed-off-by: Sirio Balmelli --- nordicsemi/__main__.py | 20 ++++++--- nordicsemi/dfu/bl_dfu_sett.py | 6 +-- nordicsemi/dfu/package.py | 22 +++++----- nordicsemi/dfu/tests/test_bl_dfu_sett.py | 53 ++++++++++++++---------- nordicsemi/dfu/tests/test_package.py | 26 +++++++++--- 5 files changed, 79 insertions(+), 48 deletions(-) diff --git a/nordicsemi/__main__.py b/nordicsemi/__main__.py index 7e5aa17..169930b 100755 --- 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 index 527120a..75fccd9 100644 --- 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 index dcccdcb..8bc72f9 100644 --- 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 index 0265588..61688e5 100644 --- 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. diff --git a/nordicsemi/dfu/tests/test_package.py b/nordicsemi/dfu/tests/test_package.py index 18340a8..d3a486c 100644 --- a/nordicsemi/dfu/tests/test_package.py +++ b/nordicsemi/dfu/tests/test_package.py @@ -43,6 +43,7 @@ import shutil from nordicsemi.dfu.package import Package +from nordicsemi.dfu.signing import Signing class TestPackage(unittest.TestCase): @@ -53,10 +54,13 @@ def tearDown(self): shutil.rmtree(self.work_directory, ignore_errors=True) def test_generate_package_application(self): + signer = Signing() + signer.load_key('key.pem') + self.p = Package(app_version=100, sd_req=[0x1000, 0xfffe], app_fw="firmwares/bar.hex", - key_file="key.pem" + signer=signer ) pkg_name = "mypackage.zip" @@ -83,11 +87,14 @@ def test_generate_package_application(self): self.assertTrue('bootloader' not in _json['manifest']) def test_generate_package_sd_bl(self): + signer = Signing() + signer.load_key('key.pem') + self.p = Package(app_version=100, sd_req=[0x1000, 0xfffe], softdevice_fw="firmwares/foo.hex", bootloader_fw="firmwares/bar.hex", - key_file="key.pem") + signer=signer) pkg_name = "mypackage.zip" @@ -112,10 +119,13 @@ def test_generate_package_sd_bl(self): self.assertEqual('sd_bl.dat', _json['manifest']['softdevice_bootloader']['dat_file']) def test_unpack_package_a(self): + signer = Signing() + signer.load_key('key.pem') + self.p = Package(app_version=100, sd_req=[0x1000, 0xffff], softdevice_fw="firmwares/bar.hex", - key_file="key.pem") + signer=signer) pkg_name = os.path.join(self.work_directory, "mypackage.zip") self.p.generate_package(pkg_name, preserve_work_dir=False) @@ -127,10 +137,13 @@ def test_unpack_package_a(self): # self.assertIsNotNone(manifest.softdevice.init_packet_data.firmware_crc16) def test_unpack_package_b(self): + signer = Signing() + signer.load_key('key.pem') + self.p = Package(app_version=100, sd_req=[0x1000, 0xffff], softdevice_fw="firmwares/bar.hex", - key_file="key.pem") + signer=signer) pkg_name = os.path.join(self.work_directory, "mypackage.zip") self.p.generate_package(pkg_name, preserve_work_dir=False) @@ -140,10 +153,13 @@ def test_unpack_package_b(self): self.assertEqual('bar.bin', manifest.softdevice.bin_file) def test_unpack_package_c(self): + signer = Signing() + signer.load_key('key.pem') + self.p = Package(app_version=100, sd_req=[0x1000, 0xffff], softdevice_fw="firmwares/bar.hex", - key_file="key.pem") + signer=signer) pkg_name = os.path.join(self.work_directory, "mypackage.zip") self.p.generate_package(pkg_name, preserve_work_dir=False)