Package: ftp.debian.org
Severity: important
Tags: patch security
While studying some details of
https://salsa.debian.org/ftp-team/code-signing/-/blob/master/secure-boot-code-sign.py,
I noticed that the "Check that files.json doesn't use absolute paths" in
check_template doesn't seem quite sufficient. The current code is as
follows:
# Check that files.json doesn't use absolute paths
for pkg, metadata in job.bin_pkgs.items():
assert(re.match(r'[a-z0-9][a-z0-9.+-]+\Z', pkg))
for file in metadata['files']:
assert(any(file["file"].startswith(prefix) for prefix in ("boot/",
"lib/", "usr/")))
assert(".." not in file["file"])
assert(not os.path.isabs(file["file"]))
Consider packages that look like this:
foo:
./usr/lib -> ../../../../../etc
foo-signed-template:
./usr/share/code-signing/foo-signed-template/files.json:
{"foo": {"files": [{"sig_type": "efi", "file": "usr/lib/passwd"}]}}
This will pass, because the file name starts with "usr/", doesn't
contain "..", and isn't absolute. sign_and_log_files will then go on to
resolve those file names relative to the extracted binary package being
signed, following symlinks, and might be able to read any data
accessible to the user running secure-boot-code-sign.py.
(Incidentally, the check for ".." there is implemented weirdly. It
should check that none of the individual segments are "..", not that the
substring ".." appears nowhere in the path.)
Obviously this would never pass any kind of attentive manual check, and
there are probably limited ways in which you could actually use this to
extract information, so I can't really justify RC severity here; but
since you have these checks in place I'm assuming that you think it's
worth them being effective.
The following is a mostly untested patch (though I've tested some of the
individual bits of code in isolation). I generally find it easier to
get this stuff right using pathlib nowadays, so I converted the relevant
bits of code to that, on the assumption that you have at least Python
3.9 for Path.is_relative_to.
I considered filing this as a merge request, but since nobody's replied
to either of my fairly straightforward MRs in
https://salsa.debian.org/ftp-team/code-signing/-/merge_requests, I
suspect that the right people might not be subscribed to those.
diff --git a/secure-boot-code-sign.py b/secure-boot-code-sign.py
index 97f86c1..81ed844 100755
--- a/secure-boot-code-sign.py
+++ b/secure-boot-code-sign.py
@@ -24,6 +24,7 @@ import fcntl
import hashlib
import json
import os
+import pathlib
import re
import subprocess
import sys
@@ -160,7 +161,7 @@ def hash_file(f):
return m.hexdigest()
-def sign_kmod(key_config: Dict[str, str], module_path: str, signature_path:
str) -> str:
+def sign_kmod(key_config: Dict[str, str], module_path: pathlib.Path,
signature_path: pathlib.Path) -> str:
env = os.environ.copy()
if config['efi']['pin'] is not None:
env['KBUILD_SIGN_PIN'] = config['efi']['pin']
@@ -175,7 +176,7 @@ def sign_kmod(key_config: Dict[str, str], module_path: str,
signature_path: str)
stderr=subprocess.STDOUT
)
- os.rename(module_path + '.p7s', signature_path)
+ pathlib.Path(str(module_path) + '.p7s').rename(signature_path)
with open(signature_path, 'rb') as f:
fhash = hash_file(f)
@@ -183,7 +184,7 @@ def sign_kmod(key_config: Dict[str, str], module_path: str,
signature_path: str)
return fhash
-def sign_efi(key_config: Dict[str, str], efi_path: str, signature_path: str)
-> str:
+def sign_efi(key_config: Dict[str, str], efi_path: pathlib.Path,
signature_path: pathlib.Path) -> str:
env = os.environ.copy()
if config['efi']['pin'] is not None:
env['PESIGN_PIN'] = str(config['efi']['pin'])
@@ -309,12 +310,14 @@ def check_template(job):
# Check signatures folder doesn't exist
assert(not os.path.exists(os.path.join(job.template_source_dir, 'debian',
'signatures')))
# Check that files.json doesn't use absolute paths
+ # (sign_and_log_files has additional symlink checks)
for pkg, metadata in job.bin_pkgs.items():
assert(re.match(r'[a-z0-9][a-z0-9.+-]+\Z', pkg))
for file in metadata['files']:
- assert(any(file["file"].startswith(prefix) for prefix in ("boot/",
"lib/", "usr/")))
- assert(".." not in file["file"])
- assert(not os.path.isabs(file["file"]))
+ file_path = pathlib.PurePath(file["file"])
+ assert file_path.parts[0] in {"boot", "lib", "usr"}
+ assert ".." not in file_path.parts
+ assert not file_path.is_absolute()
# Check that all trusted certificates are whitelisted
trusted_certs = config['keys']['trusted_certs']
if '*' not in trusted_certs:
@@ -367,25 +370,26 @@ def sign_and_log_files(job):
key_config = config['signing-keys'][key_name]
for pkg, metadata in job.bin_pkgs.items():
maybe_interactive_print('Processing', pkg)
+ pkg_path = pathlib.Path(job.tempdir, build_pkg_full_name(job,
pkg)).resolve(strict=True)
for n, file in enumerate(metadata['files'], start=1):
- relative_file_path = os.path.join(build_pkg_full_name(job, pkg),
file["file"])
- file_path = os.path.join(job.tempdir, build_pkg_full_name(job,
pkg), file["file"])
- sig_path = os.path.join(job.template_source_dir, 'debian',
'signatures', pkg, file["file"] + ".sig")
+ relative_file_path = pathlib.PurePath(build_pkg_full_name(job,
pkg), file["file"])
+ file_path = pkg_path / file["file"]
+ sig_path = pathlib.Path(job.template_source_dir, 'debian',
'signatures', pkg, file["file"] + ".sig")
+ assert file_path.resolve(strict=True).is_relative_to(pkg_path)
with open(file_path, 'rb') as f:
fhash = hash_file(f)
- presign_id = log_presign(relative_file_path, fhash, pkg,
job.version, job.architecture)
+ presign_id = log_presign(str(relative_file_path), fhash, pkg,
job.version, job.architecture)
- if not os.path.exists(os.path.dirname(sig_path)):
- os.makedirs(os.path.dirname(sig_path))
+ sig_path.parent.mkdir(parents=True, exist_ok=True)
if file["sig_type"] == "efi":
sig_hash = sign_efi(key_config, file_path, sig_path)
elif file["sig_type"] == "linux-module":
sig_hash = sign_kmod(key_config, file_path, sig_path)
else:
raise ValueError("File Type Unknown")
- log_signature(relative_file_path, sig_hash, pkg, job.version,
presign_id, job.architecture, key_name)
+ log_signature(str(relative_file_path), sig_hash, pkg, job.version,
presign_id, job.architecture, key_name)
maybe_interactive_print('\rSigned', n, 'of',
len(metadata['files']), 'files', end='')
maybe_interactive_print()
Thanks,
--
Colin Watson (he/him) [cjwat...@debian.org]
