From 878d860c31f2c233aa88e86d2218c45158c07da1 Mon Sep 17 00:00:00 2001 From: Steve McIntyre <93sam@debian.org> Date: Fri, 3 May 2019 01:41:52 +0100 Subject: VLogError(): Avoid NULL pointer dereferences in (V)Sprint calls Backport of upstream fix: VLogError() calculates the size of format strings by using calls to SPrint and VSPrint with a StrSize of 0 and NULL for an output buffer. Unfortunately, this is an incorrect usage of (V)Sprint. A StrSize of "0" is special-cased to mean "there is no limit". So, we end up writing our string to address 0x0. This was discovered because it causes a crash on ARM where, unlike x86, it does not necessarily have memory mapped at 0x0. Avoid the (V)Sprint calls altogether by using (V)PoolPrint, which handles the size calculation and allocation for us. Signed-off-by: Peter Jones Fixes: 25f6fd08cd26 ("try to show errors more usefully.") [dannf: commit message ] Signed-off-by: dann frazier --- debian/changelog | 8 +++++ debian/patches/avoid_null_vsprint.patch | 59 +++++++++++++++++++++++++++++++++ debian/patches/series | 1 + 3 files changed, 68 insertions(+) create mode 100644 debian/patches/avoid_null_vsprint.patch diff --git a/debian/changelog b/debian/changelog index 45eadbff..95d6dbc5 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,11 @@ +shim (15+1533136590.3beb971-7) UNRELEASED; urgency=medium + + [ Steve McIntyre ] + * Backport needed crash fixes: + + VLogError(): Avoid NULL pointer dereferences in (V)Sprint calls + + -- Steve McIntyre <93sam@debian.org> Fri, 03 May 2019 01:39:34 +0100 + shim (15+1533136590.3beb971-6) unstable; urgency=medium [ Steve McIntyre ] diff --git a/debian/patches/avoid_null_vsprint.patch b/debian/patches/avoid_null_vsprint.patch new file mode 100644 index 00000000..cb056d6a --- /dev/null +++ b/debian/patches/avoid_null_vsprint.patch @@ -0,0 +1,59 @@ +commit 20e731f423a438f53738de73af9ef3d67c4cba2f +Author: Peter Jones +Date: Tue Feb 12 18:04:49 2019 -0500 + + VLogError(): Avoid NULL pointer dereferences in (V)Sprint calls + + VLogError() calculates the size of format strings by using calls to + SPrint and VSPrint with a StrSize of 0 and NULL for an output buffer. + Unfortunately, this is an incorrect usage of (V)Sprint. A StrSize + of "0" is special-cased to mean "there is no limit". So, we end up + writing our string to address 0x0. This was discovered because it + causes a crash on ARM where, unlike x86, it does not necessarily + have memory mapped at 0x0. + + Avoid the (V)Sprint calls altogether by using (V)PoolPrint, which + handles the size calculation and allocation for us. + + Signed-off-by: Peter Jones + Fixes: 25f6fd08cd26 ("try to show errors more usefully.") + [dannf: commit message ] + Signed-off-by: dann frazier + +diff --git a/errlog.c b/errlog.c +index 18be482..eebb266 100644 +--- a/errlog.c ++++ b/errlog.c +@@ -14,29 +14,20 @@ EFI_STATUS + VLogError(const char *file, int line, const char *func, CHAR16 *fmt, va_list args) + { + va_list args2; +- UINTN size = 0, size2; + CHAR16 **newerrs; + +- size = SPrint(NULL, 0, L"%a:%d %a() ", file, line, func); +- va_copy(args2, args); +- size2 = VSPrint(NULL, 0, fmt, args2); +- va_end(args2); +- + newerrs = ReallocatePool(errs, (nerrs + 1) * sizeof(*errs), + (nerrs + 3) * sizeof(*errs)); + if (!newerrs) + return EFI_OUT_OF_RESOURCES; + +- newerrs[nerrs] = AllocatePool(size*2+2); ++ newerrs[nerrs] = PoolPrint(L"%a:%d %a() ", file, line, func); + if (!newerrs[nerrs]) + return EFI_OUT_OF_RESOURCES; +- newerrs[nerrs+1] = AllocatePool(size2*2+2); ++ va_copy(args2, args); ++ newerrs[nerrs+1] = VPoolPrint(fmt, args2); + if (!newerrs[nerrs+1]) + return EFI_OUT_OF_RESOURCES; +- +- SPrint(newerrs[nerrs], size*2+2, L"%a:%d %a() ", file, line, func); +- va_copy(args2, args); +- VSPrint(newerrs[nerrs+1], size2*2+2, fmt, args2); + va_end(args2); + + nerrs += 2; diff --git a/debian/patches/series b/debian/patches/series index 01e6063f..9cae2bbf 100644 --- a/debian/patches/series +++ b/debian/patches/series @@ -1,2 +1,3 @@ fixup_git.patch uname.patch +avoid_null_vsprint.patch -- cgit v1.2.3 From 315e87677ba8af638402c25b0c0e8c1c8f3da46f Mon Sep 17 00:00:00 2001 From: Steve McIntyre <93sam@debian.org> Date: Fri, 3 May 2019 01:44:29 +0100 Subject: Fix OBJ_create() to tolerate a NULL sn and ln Cherry-picked fix from upstream MR at https://github.com/rhboot/shim/pull/174/commits/3a9e237b1baddf0d3192755406befb3e9fa5ca80 From: https://github.com/openssl/openssl/commit/f13615c5b828aeb8e3d9bf2545c803633d1c684f Apply an upstream patch from OpenSSL to tolerate a NULL sn. This avoids a NULL pointer reference in shim.c:verify_eku(). This was discovered because it causes a crash on ARM where, unlike x86, it does not necessarily have memory mapped at 0x0. Fixes: 6c180c6004ac ("shim: verify Extended Key Usage flags") Signed-off-by: dann frazier --- debian/changelog | 1 + debian/patches/check_null_sn_ln.patch | 30 ++++++++++++++++++++++++++++++ debian/patches/series | 1 + 3 files changed, 32 insertions(+) create mode 100644 debian/patches/check_null_sn_ln.patch diff --git a/debian/changelog b/debian/changelog index 95d6dbc5..33926fe3 100644 --- a/debian/changelog +++ b/debian/changelog @@ -3,6 +3,7 @@ shim (15+1533136590.3beb971-7) UNRELEASED; urgency=medium [ Steve McIntyre ] * Backport needed crash fixes: + VLogError(): Avoid NULL pointer dereferences in (V)Sprint calls + + Fix OBJ_create() to tolerate a NULL sn and ln -- Steve McIntyre <93sam@debian.org> Fri, 03 May 2019 01:39:34 +0100 diff --git a/debian/patches/check_null_sn_ln.patch b/debian/patches/check_null_sn_ln.patch new file mode 100644 index 00000000..b0ee4c4a --- /dev/null +++ b/debian/patches/check_null_sn_ln.patch @@ -0,0 +1,30 @@ +commit 3a9e237b1baddf0d3192755406befb3e9fa5ca80 +Author: dann frazier +Date: Thu Mar 7 19:55:42 2019 -0700 + + Fix OBJ_create() to tolerate a NULL sn and ln + + From: https://github.com/openssl/openssl/commit/f13615c5b828aeb8e3d9bf2545c803633d1c684f + + Apply an upstream patch from OpenSSL to tolerate a NULL sn. This avoids + a NULL pointer reference in shim.c:verify_eku(). This was discovered + because it causes a crash on ARM where, unlike x86, it does not necessarily + have memory mapped at 0x0. + + Fixes: 6c180c6004ac ("shim: verify Extended Key Usage flags") + Signed-off-by: dann frazier + +diff --git a/Cryptlib/OpenSSL/crypto/objects/obj_dat.c b/Cryptlib/OpenSSL/crypto/objects/obj_dat.c +index 259851b..9b850ed 100644 +--- a/Cryptlib/OpenSSL/crypto/objects/obj_dat.c ++++ b/Cryptlib/OpenSSL/crypto/objects/obj_dat.c +@@ -685,7 +685,8 @@ int OBJ_create(const char *oid, const char *sn, const char *ln) + int ok = 0; + + /* Check to see if short or long name already present */ +- if (OBJ_sn2nid(sn) != NID_undef || OBJ_ln2nid(ln) != NID_undef) { ++ if ((sn != NULL && OBJ_sn2nid(sn) != NID_undef) ++ || (ln != NULL && OBJ_ln2nid(ln) != NID_undef)) { + OBJerr(OBJ_F_OBJ_CREATE, OBJ_R_OID_EXISTS); + return 0; + } diff --git a/debian/patches/series b/debian/patches/series index 9cae2bbf..22d2577e 100644 --- a/debian/patches/series +++ b/debian/patches/series @@ -1,3 +1,4 @@ fixup_git.patch uname.patch avoid_null_vsprint.patch +check_null_sn_ln.patch -- cgit v1.2.3 From 839af42e06287aba287ac192d0e37a739e1884ae Mon Sep 17 00:00:00 2001 From: Steve McIntyre <93sam@debian.org> Date: Fri, 3 May 2019 01:53:51 +0100 Subject: Update VCS-* fields in debian/control --- debian/changelog | 1 + debian/control | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/debian/changelog b/debian/changelog index 33926fe3..d4e6dceb 100644 --- a/debian/changelog +++ b/debian/changelog @@ -4,6 +4,7 @@ shim (15+1533136590.3beb971-7) UNRELEASED; urgency=medium * Backport needed crash fixes: + VLogError(): Avoid NULL pointer dereferences in (V)Sprint calls + Fix OBJ_create() to tolerate a NULL sn and ln + * Update VCS-* fields in debian/control -- Steve McIntyre <93sam@debian.org> Fri, 03 May 2019 01:39:34 +0100 diff --git a/debian/control b/debian/control index bedff821..8ee1560a 100644 --- a/debian/control +++ b/debian/control @@ -5,8 +5,8 @@ Maintainer: Debian EFI team Uploaders: Steve Langasek , Steve McIntyre <93sam@debian.org> Standards-Version: 4.3.0 Build-Depends: debhelper (>= 9), gnu-efi (>= 3.0u), sbsigntool, openssl, libelf-dev -Vcs-Browser: https://salsa.debian.org/vorlon/shim -Vcs-Git: https://salsa.debian.org/vorlon/shim.git +Vcs-Browser: https://salsa.debian.org/efi-team/shim +Vcs-Git: https://salsa.debian.org/efi-team/shim.git Package: shim-unsigned Architecture: amd64 arm64 i386 -- cgit v1.2.3 From e17b0af4664eff964d36090143fd6f91e07416c5 Mon Sep 17 00:00:00 2001 From: Steve McIntyre <93sam@debian.org> Date: Fri, 3 May 2019 01:56:07 +0100 Subject: Build using gcc-7 To get better control of reproducibility during the lifetime of Buster --- debian/changelog | 2 ++ debian/control | 2 +- debian/rules | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/debian/changelog b/debian/changelog index d4e6dceb..396351b4 100644 --- a/debian/changelog +++ b/debian/changelog @@ -5,6 +5,8 @@ shim (15+1533136590.3beb971-7) UNRELEASED; urgency=medium + VLogError(): Avoid NULL pointer dereferences in (V)Sprint calls + Fix OBJ_create() to tolerate a NULL sn and ln * Update VCS-* fields in debian/control + * Build using gcc-7 to get better control of reproducibility during the + lifetime of Buster. -- Steve McIntyre <93sam@debian.org> Fri, 03 May 2019 01:39:34 +0100 diff --git a/debian/control b/debian/control index 8ee1560a..dfad5e2f 100644 --- a/debian/control +++ b/debian/control @@ -4,7 +4,7 @@ Priority: optional Maintainer: Debian EFI team Uploaders: Steve Langasek , Steve McIntyre <93sam@debian.org> Standards-Version: 4.3.0 -Build-Depends: debhelper (>= 9), gnu-efi (>= 3.0u), sbsigntool, openssl, libelf-dev +Build-Depends: debhelper (>= 9), gnu-efi (>= 3.0u), sbsigntool, openssl, libelf-dev, gcc-7 Vcs-Browser: https://salsa.debian.org/efi-team/shim Vcs-Git: https://salsa.debian.org/efi-team/shim.git diff --git a/debian/rules b/debian/rules index daaed62e..0f125340 100755 --- a/debian/rules +++ b/debian/rules @@ -36,6 +36,7 @@ COMMON_OPTIONS += \ VENDOR_CERT_FILE=$(cert) \ EFIDIR=$(distributor) \ CROSS_COMPILE=$(DEB_HOST_GNU_TYPE)- \ + CC=$(DEB_HOST_GNU_TYPE)-gcc-7 \ $(NULL) %: -- cgit v1.2.3 From 6cf246a5c9bb035467fafedfd18408bc4ae78f6c Mon Sep 17 00:00:00 2001 From: Steve McIntyre <93sam@debian.org> Date: Sat, 4 May 2019 18:52:08 +0100 Subject: Generate a vendor dbx file at build time This allow us to block executing binaries with specific checksums. Generate the dbx list at runtime from a simple list of sha256 hashes, so we can update this easily. If we need to also blacklist a cert later, we'll need to update this code to add that option too. Add a build-dep on pesign to get the needed efisiglist program. --- debian/control | 2 +- debian/rules | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/debian/control b/debian/control index dfad5e2f..db164bb9 100644 --- a/debian/control +++ b/debian/control @@ -4,7 +4,7 @@ Priority: optional Maintainer: Debian EFI team Uploaders: Steve Langasek , Steve McIntyre <93sam@debian.org> Standards-Version: 4.3.0 -Build-Depends: debhelper (>= 9), gnu-efi (>= 3.0u), sbsigntool, openssl, libelf-dev, gcc-7 +Build-Depends: debhelper (>= 9), gnu-efi (>= 3.0u), sbsigntool, openssl, libelf-dev, gcc-7, pesign Vcs-Browser: https://salsa.debian.org/efi-team/shim Vcs-Git: https://salsa.debian.org/efi-team/shim.git diff --git a/debian/rules b/debian/rules index 0f125340..2a37327e 100755 --- a/debian/rules +++ b/debian/rules @@ -15,6 +15,9 @@ else distributor=debian endif +export DBX_LIST = dbx.esl +export DBX_HASHES = debian/$(distributor)-dbx.hashes + include /usr/share/dpkg/architecture.mk ifeq ($(DEB_HOST_ARCH),amd64) @@ -34,18 +37,29 @@ COMMON_OPTIONS += \ EFI_PATH=/usr/lib \ ENABLE_HTTPBOOT=true \ VENDOR_CERT_FILE=$(cert) \ + VENDOR_DBX_FILE=$(DBX_LIST) \ EFIDIR=$(distributor) \ CROSS_COMPILE=$(DEB_HOST_GNU_TYPE)- \ CC=$(DEB_HOST_GNU_TYPE)-gcc-7 \ $(NULL) +$(DBX_LIST): + if [ -f ${DBX_HASHES} ]; then \ + for HASH in $$(grep -E [[:xdigit:]]{32} ${DBX_HASHES}); do \ + efisiglist -o ${DBX_LIST} -a -h $$HASH; \ + done; \ + else \ + touch ${DBX_LIST}; \ + fi + %: dh $@ --parallel override_dh_auto_clean: dh_auto_clean -- MAKELEVEL=0 + rm -f $(DBX_LIST) -override_dh_auto_build: +override_dh_auto_build: $(DBX_LIST) dh_auto_build -- $(COMMON_OPTIONS) override_dh_auto_install: -- cgit v1.2.3 From 88a7a6505b6c502c0180c9980081fb97a224b72f Mon Sep 17 00:00:00 2001 From: Steve McIntyre <93sam@debian.org> Date: Sat, 4 May 2019 18:57:01 +0100 Subject: Add initial file with test checksums for the dbx list --- debian/changelog | 4 ++++ debian/control | 2 +- debian/debian-dbx.hashes | 18 ++++++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 debian/debian-dbx.hashes diff --git a/debian/changelog b/debian/changelog index 396351b4..492d35e5 100644 --- a/debian/changelog +++ b/debian/changelog @@ -7,6 +7,10 @@ shim (15+1533136590.3beb971-7) UNRELEASED; urgency=medium * Update VCS-* fields in debian/control * Build using gcc-7 to get better control of reproducibility during the lifetime of Buster. + * Build in a dbx list to blacklist binaries that we know to not be + secure. Build-depend on a new (bug-fixed) version of pesign to + generate that list at build time, using a list of known bad hashes. + * Initial list of known bad hashes is just my personal test binary. -- Steve McIntyre <93sam@debian.org> Fri, 03 May 2019 01:39:34 +0100 diff --git a/debian/control b/debian/control index db164bb9..5f82c5c4 100644 --- a/debian/control +++ b/debian/control @@ -4,7 +4,7 @@ Priority: optional Maintainer: Debian EFI team Uploaders: Steve Langasek , Steve McIntyre <93sam@debian.org> Standards-Version: 4.3.0 -Build-Depends: debhelper (>= 9), gnu-efi (>= 3.0u), sbsigntool, openssl, libelf-dev, gcc-7, pesign +Build-Depends: debhelper (>= 9), gnu-efi (>= 3.0u), sbsigntool, openssl, libelf-dev, gcc-7, pesign (>= 0.112-5) Vcs-Browser: https://salsa.debian.org/efi-team/shim Vcs-Git: https://salsa.debian.org/efi-team/shim.git diff --git a/debian/debian-dbx.hashes b/debian/debian-dbx.hashes new file mode 100644 index 00000000..494f09df --- /dev/null +++ b/debian/debian-dbx.hashes @@ -0,0 +1,18 @@ +# debian-dbx.hashes +# +# This file contains the sha256 sums of the binaries that we want to +# blacklist directly in our signed shim. Add entries below, with comments +# to explain each entry (where possible). +# +# Format of this file: put hex-encoded sha256 checksums on lines on +# their own. I'm using shell-style comments just for clarity. +# +# The hashes are generated using: +# +# pesign --hash -in +# +# on *either* the signed or unsigned binary, pesign doesn't care +# which. + +# Sledge's test arm64 grub binary +d0555468007c31bd75c1f1c984e5b4adbb464bc68e5dedd670535ee97acc7dd9 -- cgit v1.2.3 From 549f650b3d9e630b185ed7f00b55cef4a55460d3 Mon Sep 17 00:00:00 2001 From: Steve McIntyre <93sam@debian.org> Date: Mon, 6 May 2019 13:07:00 +0100 Subject: Add more hashes that we want to blacklist signed arm64 grub binaries that allow use of the devicetree command, as found in grub-efi-arm64-signed_1+2.02+dfsg1+16_arm64.deb grub-efi-arm64-signed_1+2.02+dfsg1+17_arm64.deb --- debian/debian-dbx.hashes | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/debian/debian-dbx.hashes b/debian/debian-dbx.hashes index 494f09df..568ce611 100644 --- a/debian/debian-dbx.hashes +++ b/debian/debian-dbx.hashes @@ -16,3 +16,21 @@ # Sledge's test arm64 grub binary d0555468007c31bd75c1f1c984e5b4adbb464bc68e5dedd670535ee97acc7dd9 + +# Files from grub-efi-arm64-signed_1+2.02+dfsg1+16_arm64.deb +# (allows use of the devicetree command in secure mode) +# grubaa64.efi.signed +1c88f32ebd6ecd1a84d83940f78b0d69168a4ff57a1f57fd070e7307899bbc99 +# grubnetaa64.efi.signed +7f5074f42eb92f183fa8748c92992bae8b23a963bc9f8b85692ee7116dc3dcb0 +# gcdaa64.efi.signed +d9e95ad9ea0e0df522f7f41ef9fd9cb5e65cfb0c285465aabd077023e6edb6ba + +# Files from grub-efi-arm64-signed_1+2.02+dfsg1+17_arm64.deb +# (allows use of the devicetree command in secure mode) +# grubaa64.efi.signed +77fb2b05450520eecdbbfa3070fb26405d151b576cd6dffc8cab6a2f0bcff10c +# grubnetaa64.efi.signed +d70b41ea19ea19836198252ac90b1b6fca40ad6baa3911b4a27e886e6423426a +# gcdaa64.efi.signed +44f20309d8b0f661da2c3cf225ba9c0f7b8a2d6ff3885ecba710e5907870ee24 -- cgit v1.2.3