From e673c5c2d904d821719b2d21746ef91482acf8b4 Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Fri, 5 May 2023 06:22:57 -0400 Subject: [PATCH 1/4] ukify: typo in doc and print when package is missing If a package is missing, a subprocess is started with None as command argument. Error raised by subprocess is therefore not helpful at all to understand what needs to be done to fix that error. Also fix doc since systemd-stub will look for .cmdline files, and not .cmdline.efi files. Signed-off-by: Emanuele Giuseppe Esposito --- man/ukify.xml | 2 +- src/ukify/ukify.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/man/ukify.xml b/man/ukify.xml index 6aa136298d..cc711190fa 100644 --- a/man/ukify.xml +++ b/man/ukify.xml @@ -435,7 +435,7 @@ Phases=enter-initrd:leave-initrd --secureboot-private-key=sb.key \ --secureboot-certificate=sb.cert \ --cmdline='debug' \ - --output=debug.cmdline.efi + --output=debug.cmdline This creates a signed PE binary that contains the additional kernel command line parameter diff --git a/src/ukify/ukify.py b/src/ukify/ukify.py index 7e9c7cc9ae..9db84afdbb 100755 --- a/src/ukify/ukify.py +++ b/src/ukify/ukify.py @@ -353,8 +353,10 @@ def find_tool(name, fallback=None, opts=None): if shutil.which(name) is not None: return name - return fallback + if fallback is None: + print(f"Tool {name} not installed!") + return fallback def combine_signatures(pcrsigs): combined = collections.defaultdict(list) From c1e8d1727b64cc38821140312c7c3348300d81a0 Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Thu, 4 May 2023 11:48:47 -0400 Subject: [PATCH 2/4] ukify: support pesign as alternative to sbsign sbsign is not available everywhere, for example RHEL does not have it. Add pesign as alternative to it. pesign will use options "--secureboot-certificate-name" (mandatory) and "--secureboot-certificate-dir" (optional), while sbsign will use "--secureboot-private-key" and "--secureboot-certificate". By default, use sbsign. If no key/cert is provided or sbsign is not found, try pesign. Signed-off-by: Emanuele Giuseppe Esposito --- man/ukify.xml | 32 ++++++++- src/ukify/ukify.py | 160 +++++++++++++++++++++++++++++++-------------- 2 files changed, 141 insertions(+), 51 deletions(-) diff --git a/man/ukify.xml b/man/ukify.xml index cc711190fa..f5a2fcc3e8 100644 --- a/man/ukify.xml +++ b/man/ukify.xml @@ -253,13 +253,23 @@ system. + + SecureBootSigningTool=SIGNER + + + Whether to use sbsign or pesign. + Depending on this choice, different parameters are required in order to sign an image. + Defaults to sbsign. + + SecureBootPrivateKey=SB_KEY A path to a private key to use for signing of the resulting binary. If the SigningEngine=/ option is used, this may also be - an engine-specific designation. + an engine-specific designation. This option is required by + SecureBootSigningTool=sbsign/. @@ -268,7 +278,25 @@ A path to a certificate to use for signing of the resulting binary. If the SigningEngine=/ option is used, this may also - be an engine-specific designation. + be an engine-specific designation. This option is required by + SecureBootSigningTool=sbsign/. + + + + SecureBootCertificateDir=SB_PATH + + + A path to a nss certificate database directory to use for signing of the resulting binary. + Takes effect when SecureBootSigningTool=pesign/ is used. + Defaults to /etc/pki/pesign. + + + + SecureBootCertificateName=SB_CERTNAME + + + The name of the nss certificate database entry to use for signing of the resulting binary. + This option is required by SecureBootSigningTool=pesign/. diff --git a/src/ukify/ukify.py b/src/ukify/ukify.py index 9db84afdbb..d87670eb24 100755 --- a/src/ukify/ukify.py +++ b/src/ukify/ukify.py @@ -550,50 +550,93 @@ def pe_add_sections(uki: UKI, output: str): pe.write(output) +def signer_sign(cmd): + print('+', shell_join(cmd)) + subprocess.check_call(cmd) + +def find_sbsign(opts=None): + return find_tool('sbsign', opts=opts) + +def sbsign_sign(sbsign_tool, input_f, output_f, opts=None): + sign_invocation = [ + sbsign_tool, + '--key', opts.sb_key, + '--cert', opts.sb_cert, + input_f, + '--output', output_f, + ] + if opts.signing_engine is not None: + sign_invocation += ['--engine', opts.signing_engine] + signer_sign(sign_invocation) + +def find_pesign(opts=None): + return find_tool('pesign', opts=opts) + +def pesign_sign(pesign_tool, input_f, output_f, opts=None): + sign_invocation = [ + pesign_tool, '-s', '--force', + '-n', opts.sb_certdir, + '-c', opts.sb_cert_name, + '-i', input_f, + '-o', output_f, + ] + signer_sign(sign_invocation) + +SBVERIFY = { + 'name': 'sbverify', + 'option': '--list', + 'output': 'No signature table present', +} + +PESIGCHECK = { + 'name': 'pesign', + 'option': '-i', + 'output': 'No signatures found.', + 'flags': '-S' +} + +def verify(tool, opts): + verify_tool = find_tool(tool['name'], opts=opts) + cmd = [ + verify_tool, + tool['option'], + opts.linux, + ] + if 'flags' in tool: + cmd.append(tool['flags']) + + print('+', shell_join(cmd)) + info = subprocess.check_output(cmd, text=True) + + return tool['output'] in info def make_uki(opts): # kernel payload signing - sbsign_tool = find_tool('sbsign', opts=opts) - sbsign_invocation = [ - sbsign_tool, - '--key', opts.sb_key, - '--cert', opts.sb_cert, - ] + sign_tool = None + if opts.signtool == 'sbsign': + sign_tool = find_sbsign(opts=opts) + sign = sbsign_sign + verify_tool = SBVERIFY + else: + sign_tool = find_pesign(opts=opts) + sign = pesign_sign + verify_tool = PESIGCHECK - if opts.signing_engine is not None: - sbsign_invocation += ['--engine', opts.signing_engine] + sign_args_present = opts.sb_key or opts.sb_cert_name + + if sign_tool is None and sign_args_present: + raise ValueError(f'{opts.signtool}, required for signing, is not installed') sign_kernel = opts.sign_kernel - if sign_kernel is None and opts.linux is not None and opts.sb_key: + if sign_kernel is None and opts.linux is not None and sign_args_present: # figure out if we should sign the kernel - sbverify_tool = find_tool('sbverify', opts=opts) - - cmd = [ - sbverify_tool, - '--list', - opts.linux, - ] - - print('+', shell_join(cmd)) - info = subprocess.check_output(cmd, text=True) - - # sbverify has wonderful API - if 'No signature table present' in info: - sign_kernel = True + sign_kernel = verify(verify_tool, opts) if sign_kernel: linux_signed = tempfile.NamedTemporaryFile(prefix='linux-signed') linux = linux_signed.name - - cmd = [ - *sbsign_invocation, - opts.linux, - '--output', linux, - ] - - print('+', shell_join(cmd)) - subprocess.check_call(cmd) + sign(sign_tool, opts.linux, linux, opts=opts) else: linux = opts.linux @@ -641,7 +684,7 @@ def make_uki(opts): if linux is not None: uki.add_section(Section.create('.linux', linux, measure=True)) - if opts.sb_key: + if sign_args_present: unsigned = tempfile.NamedTemporaryFile(prefix='uki') output = unsigned.name else: @@ -651,20 +694,14 @@ def make_uki(opts): # UKI signing - if opts.sb_key: - cmd = [ - *sbsign_invocation, - unsigned.name, - '--output', opts.output, - ] - print('+', shell_join(cmd)) - subprocess.check_call(cmd) + if sign_args_present: + sign(sign_tool, unsigned.name, opts.output, opts=opts) # We end up with no executable bits, let's reapply them os.umask(umask := os.umask(0)) os.chmod(opts.output, 0o777 & ~umask) - print(f"Wrote {'signed' if opts.sb_key else 'unsigned'} {opts.output}") + print(f"Wrote {'signed' if sign_args_present else 'unsigned'} {opts.output}") @dataclasses.dataclass(frozen=True) @@ -913,18 +950,39 @@ CONFIG_ITEMS = [ help = 'OpenSSL engine to use for signing', config_key = 'UKI/SigningEngine', ), + ConfigItem( + '--signtool', + choices = ('sbsign', 'pesign'), + dest = 'signtool', + default = 'sbsign', + help = 'whether to use sbsign or pesign. Default is sbsign.', + config_key = 'UKI/SecureBootSigningTool', + ), ConfigItem( '--secureboot-private-key', dest = 'sb_key', - help = 'path to key file or engine-specific designation for SB signing', + help = 'required by --signtool=sbsign. Path to key file or engine-specific designation for SB signing', config_key = 'UKI/SecureBootPrivateKey', ), ConfigItem( '--secureboot-certificate', dest = 'sb_cert', - help = 'path to certificate file or engine-specific designation for SB signing', + help = 'required by --signtool=sbsign. sbsign needs a path to certificate file or engine-specific designation for SB signing', config_key = 'UKI/SecureBootCertificate', ), + ConfigItem( + '--secureboot-certificate-dir', + dest = 'sb_certdir', + default = '/etc/pki/pesign', + help = 'required by --signtool=pesign. Path to nss certificate database directory for PE signing. Default is /etc/pki/pesign', + config_key = 'UKI/SecureBootCertificateDir', + ), + ConfigItem( + '--secureboot-certificate-name', + dest = 'sb_cert_name', + help = 'required by --signtool=pesign. pesign needs a certificate nickname of nss certificate database entry to use for PE signing', + config_key = 'UKI/SecureBootCertificateName', + ), ConfigItem( '--sign-kernel', @@ -1091,16 +1149,20 @@ def finalize_options(opts): if opts.sb_cert: opts.sb_cert = pathlib.Path(opts.sb_cert) - if bool(opts.sb_key) ^ bool(opts.sb_cert): - raise ValueError('--secureboot-private-key= and --secureboot-certificate= must be specified together') + if opts.signtool == 'sbsign': + if bool(opts.sb_key) ^ bool(opts.sb_cert): + raise ValueError('--secureboot-private-key= and --secureboot-certificate= must be specified together when using --signtool=sbsign') + else: + if not bool(opts.sb_cert_name): + raise ValueError('--certificate-name must be specified when using --signtool=pesign') - if opts.sign_kernel and not opts.sb_key: - raise ValueError('--sign-kernel requires --secureboot-private-key= and --secureboot-certificate= to be specified') + if opts.sign_kernel and not opts.sb_key and not opts.sb_cert_name: + raise ValueError('--sign-kernel requires either --secureboot-private-key= and --secureboot-certificate= (for sbsign) or --secureboot-certificate-name= (for pesign) to be specified') if opts.output is None: if opts.linux is None: raise ValueError('--output= must be specified when building a PE addon') - suffix = '.efi' if opts.sb_key else '.unsigned.efi' + suffix = '.efi' if opts.sb_key or opts.sb_cert_name else '.unsigned.efi' opts.output = opts.linux.name + suffix for section in opts.sections: From ff5618c7c030990974bd9c479d4ca4e9d5a721b6 Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Mon, 8 May 2023 08:23:11 -0400 Subject: [PATCH 3/4] src/ukify/test/test_ukify: add pesign unit test Signed-off-by: Emanuele Giuseppe Esposito --- src/ukify/test/test_ukify.py | 55 ++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/src/ukify/test/test_ukify.py b/src/ukify/test/test_ukify.py index d221825019..7af4c7d0c2 100755 --- a/src/ukify/test/test_ukify.py +++ b/src/ukify/test/test_ukify.py @@ -219,12 +219,15 @@ def test_config_priority(tmp_path): DeviceTree = some/path2 Splash = some/path3 Uname = 1.2.3 - EFIArch=arm + EFIArch = arm Stub = some/path4 PCRBanks = sha512,sha1 SigningEngine = engine1 + SignTool = pesign SecureBootPrivateKey = some/path5 SecureBootCertificate = some/path6 + SecureBootCertificateDir = some/path7 + SecureBootCertificateName = some/name1 SignKernel = no [PCRSignature:NAME] @@ -246,8 +249,11 @@ def test_config_priority(tmp_path): '--pcr-public-key=PKEY2', '--pcr-banks=SHA1,SHA256', '--signing-engine=ENGINE', + '--signtool=pesign', '--secureboot-private-key=SBKEY', '--secureboot-certificate=SBCERT', + '--secureboot-certificate-dir=SBPATH', + '--secureboot-certificate-name=SBNAME', '--sign-kernel', '--no-sign-kernel', '--tools=TOOLZ///', @@ -277,8 +283,11 @@ def test_config_priority(tmp_path): pathlib.Path('some/path8')] assert opts.pcr_banks == ['SHA1', 'SHA256'] assert opts.signing_engine == 'ENGINE' + assert opts.signtool == 'pesign' assert opts.sb_key == 'SBKEY' assert opts.sb_cert == 'SBCERT' + assert opts.sb_certdir == 'SBPATH' + assert opts.sb_cert_name == 'SBNAME' assert opts.sign_kernel is False assert opts.tools == [pathlib.Path('TOOLZ/')] assert opts.output == pathlib.Path('OUTPUT') @@ -408,7 +417,7 @@ def test_uname_scraping(kernel_initrd): uname = ukify.Uname.scrape(kernel_initrd[0]) assert re.match(r'\d+\.\d+\.\d+', uname) -def test_efi_signing(kernel_initrd, tmpdir): +def test_efi_signing_sbsign(kernel_initrd, tmpdir): if kernel_initrd is None: pytest.skip('linux+initrd not found') if not shutil.which('sbsign'): @@ -445,6 +454,48 @@ def test_efi_signing(kernel_initrd, tmpdir): assert 'Signature verification OK' in dump +def test_efi_signing_pesign(kernel_initrd, tmpdir): + if kernel_initrd is None: + pytest.skip('linux+initrd not found') + if not shutil.which('pesign'): + pytest.skip('pesign not found') + + nss_db = f'{tmpdir}/nss_db' + name = 'Test_Secureboot' + author = 'systemd' + + subprocess.check_call(['mkdir', '-p', nss_db]) + cmd = f'certutil -N --empty-password -d {nss_db}'.split(' ') + subprocess.check_call(cmd) + cmd = f'efikeygen -d {nss_db} -S -k -c CN={author} -n {name}'.split(' ') + subprocess.check_call(cmd) + + output = f'{tmpdir}/signed.efi' + opts = ukify.parse_args([ + *kernel_initrd, + f'--output={output}', + '--uname=1.2.3', + '--signtool=pesign', + '--cmdline=ARG1 ARG2 ARG3', + f'--secureboot-certificate-name={name}', + f'--secureboot-certificate-dir={nss_db}', + ]) + + try: + ukify.check_inputs(opts) + except OSError as e: + pytest.skip(str(e)) + + ukify.make_uki(opts) + + # let's check that sbverify likes the resulting file + dump = subprocess.check_output([ + 'pesign', '-S', + '-i', output, + ], text=True) + + assert f"The signer's common name is {author}" in dump + def test_pcr_signing(kernel_initrd, tmpdir): if kernel_initrd is None: pytest.skip('linux+initrd not found') From 4e906270a31ccfe402e4bb645398dd4de6df94cc Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Mon, 8 May 2023 08:41:04 -0400 Subject: [PATCH 4/4] src/ukify/test/test_ukify: fix skipped tests Some tests are skipped because initrd extracted from bootctl is "/boot/initramfs-5.14.0-284.el9.x86_64.img $tuned_initrd" and not just "/boot/initramfs-5.14.0-284.el9.x86_64.img". Therefore split and remove the additional garbage. Signed-off-by: Emanuele Giuseppe Esposito --- src/ukify/test/test_ukify.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ukify/test/test_ukify.py b/src/ukify/test/test_ukify.py index 7af4c7d0c2..983a67aad6 100755 --- a/src/ukify/test/test_ukify.py +++ b/src/ukify/test/test_ukify.py @@ -321,7 +321,7 @@ def kernel_initrd(): for item in items: try: linux = f"{item['root']}{item['linux']}" - initrd = f"{item['root']}{item['initrd'][0]}" + initrd = f"{item['root']}{item['initrd'][0].split(' ')[0]}" except (KeyError, IndexError): continue return [linux, initrd]