Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Shim 15.8 for IGEL OS (x86_64) #434

Open
8 tasks done
af-kulow opened this issue Jul 30, 2024 · 20 comments
Open
8 tasks done

Shim 15.8 for IGEL OS (x86_64) #434

af-kulow opened this issue Jul 30, 2024 · 20 comments
Assignees
@jsetje
Labels
1 review needed Needs 1 (additional) successful review before being accepted Accredited review needed Needs a successful review by an accredited reviewer contacts verified OK Contact verification is complete here (or in an earlier submission)

Comments

@af-kulow
Copy link

af-kulow commented Jul 30, 2024 •
edited
Loading

Confirm the following are included in your repo, checking each box:

  • completed README.md file with the necessary information
  • shim.efi to be signed
  • public portion of your certificate(s) embedded in shim (the file passed to VENDOR_CERT_FILE)
  • binaries, for which hashes are added to vendor_db ( if you use vendor_db and have hashes allow-listed )
  • any extra patches to shim via your own git tree
  • any extra patches to grub via your own git tree or as files
  • build logs
  • a Dockerfile to reproduce the build of the provided shim EFI binaries

What is the link to your tag in a repo cloned from rhboot/shim-review?

https://github.com/IGEL-Technology/shim-review/tree/IGEL-shim-x64-20240730-1

EDIT: In the meantime, linux kernel diffs, as well as additional sbat sections were requested in the review (see comments below). We included the information in the following tag of the review:
https://github.com/IGEL-Technology/shim-review/releases/tag/IGEL-shim-x64-20240807

What is the SHA256 hash of your final SHIM binary?

90ce33685c5ac241b582bdf27d0ae872b72263a5ae8271ef7f738bd1d13e8e63 shimx64.efi

What is the link to your previous shim review request (if any, otherwise N/A)?

#11

If no security contacts have changed since verification, what is the link to your request, where they've been verified (if any, otherwise N/A)?

N/A
@steve-mcintyre
Copy link
Collaborator

Contact verification mails sent to:

@steve-mcintyre steve-mcintyre added new vendor This is a new vendor contact verification pending Contact verification emails have been sent, waiting on response labels Jul 30, 2024
@tina-igel
Copy link

Contact verification for [email protected]:

seethes demarcating softwood tuning imbecilities perfunctory bobbling voyager Wyoming bulwark

@af-kulow
Copy link
Author

contact verification for [email protected]:

tines buttons noiseless atrocity Kempis Versailles forgot gambolled comprises colorfast

@steve-mcintyre
Copy link
Collaborator

Contact responses good!

@steve-mcintyre steve-mcintyre added contacts verified OK Contact verification is complete here (or in an earlier submission) and removed contact verification pending Contact verification emails have been sent, waiting on response labels Jul 30, 2024
@jclab-joseph
Copy link

Review for IGEL-shim-x64-20240730-1

review helper : https://github.com/jclab-joseph/other-shim-reviews/tree/master/20240802-IGEL-shim-x64-20240730-1

shim

Patches

$ git clone https://github.com/IGEL-Technology/shim.git igel-shim
=> commit id: 2eed136ae574185f1ed1fab0babb566bb19de3cb
$ diff -urN shim-15.8 igel-shim/ | grep -A3 -E "^diff " | grep -v '\.git'

diff -urN shim-15.8/debian/block_signed_deb igel-shim/debian/block_signed_deb
--- shim-15.8/debian/block_signed_deb	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/block_signed_deb	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,73 @@
--
diff -urN shim-15.8/debian/changelog igel-shim/debian/changelog
--- shim-15.8/debian/changelog	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/changelog	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,534 @@
--
diff -urN shim-15.8/debian/check_nx igel-shim/debian/check_nx
--- shim-15.8/debian/check_nx	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/check_nx	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,32 @@
--
diff -urN shim-15.8/debian/control igel-shim/debian/control
--- shim-15.8/debian/control	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/control	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,46 @@
--
diff -urN shim-15.8/debian/copyright igel-shim/debian/copyright
--- shim-15.8/debian/copyright	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/copyright	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,415 @@
--
diff -urN shim-15.8/debian/generate_dbx_list igel-shim/debian/generate_dbx_list
--- shim-15.8/debian/generate_dbx_list	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/generate_dbx_list	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,54 @@
--
diff -urN shim-15.8/debian/igel-dbx.hashes igel-shim/debian/igel-dbx.hashes
--- shim-15.8/debian/igel-dbx.hashes	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/igel-dbx.hashes	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,21 @@
--
diff -urN shim-15.8/debian/patches/0001-sbat-Add-grub.peimage-2-to-latest-CVE-2024-2312.patch igel-shim/debian/patches/0001-sbat-Add-grub.peimage-2-to-latest-CVE-2024-2312.patch
--- shim-15.8/debian/patches/0001-sbat-Add-grub.peimage-2-to-latest-CVE-2024-2312.patch	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/patches/0001-sbat-Add-grub.peimage-2-to-latest-CVE-2024-2312.patch	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,42 @@
--
diff -urN shim-15.8/debian/patches/0002-sbat-Also-bump-latest-for-grub-4-and-to-todays-date.patch igel-shim/debian/patches/0002-sbat-Also-bump-latest-for-grub-4-and-to-todays-date.patch
--- shim-15.8/debian/patches/0002-sbat-Also-bump-latest-for-grub-4-and-to-todays-date.patch	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/patches/0002-sbat-Also-bump-latest-for-grub-4-and-to-todays-date.patch	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,47 @@
--
diff -urN shim-15.8/debian/patches/IGEL-change-default-loader-to-igel.diff igel-shim/debian/patches/IGEL-change-default-loader-to-igel.diff
--- shim-15.8/debian/patches/IGEL-change-default-loader-to-igel.diff	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/patches/IGEL-change-default-loader-to-igel.diff	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,39 @@
--
diff -urN shim-15.8/debian/patches/series igel-shim/debian/patches/series
--- shim-15.8/debian/patches/series	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/patches/series	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,5 @@
--
diff -urN shim-15.8/debian/rules igel-shim/debian/rules
--- shim-15.8/debian/rules	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/rules	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,106 @@
--
diff -urN shim-15.8/debian/salsa-ci.yml igel-shim/debian/salsa-ci.yml
--- shim-15.8/debian/salsa-ci.yml	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/salsa-ci.yml	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,3 @@
--
diff -urN shim-15.8/debian/sbat.debian.csv.in igel-shim/debian/sbat.debian.csv.in
--- shim-15.8/debian/sbat.debian.csv.in	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/sbat.debian.csv.in	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1 @@
--
diff -urN shim-15.8/debian/sbat.igel.csv.in igel-shim/debian/sbat.igel.csv.in
--- shim-15.8/debian/sbat.igel.csv.in	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/sbat.igel.csv.in	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1 @@
--
diff -urN shim-15.8/debian/shim-helpers-amd64-signed-template.lintian-overrides igel-shim/debian/shim-helpers-amd64-signed-template.lintian-overrides
--- shim-15.8/debian/shim-helpers-amd64-signed-template.lintian-overrides	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/shim-helpers-amd64-signed-template.lintian-overrides	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1 @@
--
diff -urN shim-15.8/debian/shim-helpers-arm64-signed-template.lintian-overrides igel-shim/debian/shim-helpers-arm64-signed-template.lintian-overrides
--- shim-15.8/debian/shim-helpers-arm64-signed-template.lintian-overrides	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/shim-helpers-arm64-signed-template.lintian-overrides	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1 @@
--
diff -urN shim-15.8/debian/shim-helpers-i386-signed-template.lintian-overrides igel-shim/debian/shim-helpers-i386-signed-template.lintian-overrides
--- shim-15.8/debian/shim-helpers-i386-signed-template.lintian-overrides	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/shim-helpers-i386-signed-template.lintian-overrides	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1 @@
--
diff -urN shim-15.8/debian/shim-unsigned.dirs igel-shim/debian/shim-unsigned.dirs
--- shim-15.8/debian/shim-unsigned.dirs	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/shim-unsigned.dirs	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1 @@
--
diff -urN shim-15.8/debian/shim-unsigned.install igel-shim/debian/shim-unsigned.install
--- shim-15.8/debian/shim-unsigned.install	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/shim-unsigned.install	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,4 @@
--
diff -urN shim-15.8/debian/signing-template/@[email protected] igel-shim/debian/signing-template/@[email protected]
--- shim-15.8/debian/signing-template/@[email protected]	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/signing-template/@[email protected]	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,93 @@
--
diff -urN shim-15.8/debian/signing-template/@[email protected] igel-shim/debian/signing-template/@[email protected]
--- shim-15.8/debian/signing-template/@[email protected]	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/signing-template/@[email protected]	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,57 @@
--
diff -urN shim-15.8/debian/signing-template/README.source igel-shim/debian/signing-template/README.source
--- shim-15.8/debian/signing-template/README.source	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/signing-template/README.source	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,4 @@
--
diff -urN shim-15.8/debian/signing-template/changelog.in igel-shim/debian/signing-template/changelog.in
--- shim-15.8/debian/signing-template/changelog.in	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/signing-template/changelog.in	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,11 @@
--
diff -urN shim-15.8/debian/signing-template/compat igel-shim/debian/signing-template/compat
--- shim-15.8/debian/signing-template/compat	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/signing-template/compat	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1 @@
--
diff -urN shim-15.8/debian/signing-template/control.in igel-shim/debian/signing-template/control.in
--- shim-15.8/debian/signing-template/control.in	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/signing-template/control.in	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,25 @@
--
diff -urN shim-15.8/debian/signing-template/copyright igel-shim/debian/signing-template/copyright
--- shim-15.8/debian/signing-template/copyright	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/signing-template/copyright	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,51 @@
--
diff -urN shim-15.8/debian/signing-template/rules igel-shim/debian/signing-template/rules
--- shim-15.8/debian/signing-template/rules	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/signing-template/rules	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,18 @@
--
diff -urN shim-15.8/debian/signing-template/source/format igel-shim/debian/signing-template/source/format
--- shim-15.8/debian/signing-template/source/format	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/signing-template/source/format	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1 @@
--
diff -urN shim-15.8/debian/signing-template.generate igel-shim/debian/signing-template.generate
--- shim-15.8/debian/signing-template.generate	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/signing-template.generate	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,43 @@
--
diff -urN shim-15.8/debian/signing-template.json.in igel-shim/debian/signing-template.json.in
--- shim-15.8/debian/signing-template.json.in	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/signing-template.json.in	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,11 @@
--
diff -urN shim-15.8/debian/source/format igel-shim/debian/source/format
--- shim-15.8/debian/source/format	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/source/format	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1 @@
--
diff -urN shim-15.8/debian/source/include-binaries igel-shim/debian/source/include-binaries
--- shim-15.8/debian/source/include-binaries	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/source/include-binaries	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,2 @@
--
diff -urN shim-15.8/debian/tests/01_sanity_tests.py igel-shim/debian/tests/01_sanity_tests.py
--- shim-15.8/debian/tests/01_sanity_tests.py	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/tests/01_sanity_tests.py	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,54 @@
--
diff -urN shim-15.8/debian/tests/05_signature_tests.py igel-shim/debian/tests/05_signature_tests.py
--- shim-15.8/debian/tests/05_signature_tests.py	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/tests/05_signature_tests.py	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,91 @@
--
diff -urN shim-15.8/debian/tests/10_uefi_boot_tests.py igel-shim/debian/tests/10_uefi_boot_tests.py
--- shim-15.8/debian/tests/10_uefi_boot_tests.py	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/tests/10_uefi_boot_tests.py	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,51 @@
--
diff -urN shim-15.8/debian/tests/control igel-shim/debian/tests/control
--- shim-15.8/debian/tests/control	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/tests/control	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,47 @@
--
diff -urN shim-15.8/debian/tests/uefi_tests_base.py igel-shim/debian/tests/uefi_tests_base.py
--- shim-15.8/debian/tests/uefi_tests_base.py	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/tests/uefi_tests_base.py	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,261 @@
--
diff -urN shim-15.8/debian/upstream/metadata igel-shim/debian/upstream/metadata
--- shim-15.8/debian/upstream/metadata	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/upstream/metadata	2024-08-02 15:48:22.091442471 +0900
@@ -0,0 +1,2 @@
--
diff -urN shim-15.8/debian/upstream/signing-key.asc igel-shim/debian/upstream/signing-key.asc
--- shim-15.8/debian/upstream/signing-key.asc	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/upstream/signing-key.asc	2024-08-02 15:48:22.095442243 +0900
@@ -0,0 +1,465 @@
--
diff -urN shim-15.8/debian/watch igel-shim/debian/watch
--- shim-15.8/debian/watch	1970-01-01 09:00:00.000000000 +0900
+++ igel-shim/debian/watch	2024-08-02 15:48:22.095442243 +0900
@@ -0,0 +1,5 @@

certificate

  • Not After: Mar 19 08:55:59 2054 GMT
  • self-signed 4096 bit cert and valid for almost 10 years
  • The keys are generated on a NitroKey HSM, which is stored in a safe in the company's facility.

NEED MORE CHECKS

We generally backport fixes and features from development kernels to our LTS kernels. For example, we're currently on 6.6.x but have quite a few backports from 6.8+
We apply various patches to support also the most recent hardware, e.g.

  • MeteorLake processor generation
  • HP mt645
  • Surface tablets
    We have IGEL OS-specific features in the kernel
  • IGEL Flash Driver, a kind of logical volume manager optimized for small flash memory devices providing checksum validation, encryption, etc.

@af-kulow
Copy link
Author

af-kulow commented Aug 5, 2024

I will add the SBAT entries of the binaries we boot also in the questionnaire, for quick reference, here they are:

igelx64.efi

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md 
grub,4,Free Software Foundation,grub,2.06,https://www.gnu.org/software/grub/ 
grub.debian,4,Debian,grub2,2.06-13+deb12u1,https://tracker.debian.org/pkg/grub2 
grub.igel,4,Igel,grub2,2.06-13+deb12u1igel1721912063,https://www.igel.com

fwupdx64.efi

sbat,1,UEFI shim,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
fwupd-efi,1,Firmware update daemon,fwupd-efi,1.6,https://github.com/fwupd/fwupd-efi
fwupd-efi.debian,1,Debian,fwupd,1:1.6-1,https://tracker.debian.org/pkg/fwupd

Further, we are preparing the Kernel patches for review.

@af-kulow
Copy link
Author

af-kulow commented Aug 7, 2024

We now have included the patches to the linux kernel as well as the kernel build config in the shim-review repo (linux-patches/).
I created a new tag for the updated files as well as the updated, previously stated, sbat sections: https://github.com/IGEL-Technology/shim-review/releases/tag/IGEL-shim-x64-20240807

The source code of the kernel, initramfs et al. is available here for reviewers: https://github.com/IGEL-Technology

@zeetim
Copy link

zeetim commented Sep 4, 2024 •
edited
Loading

Review of IGEL-shim-x64-20240807

I am not an authorized reviewer but I want to help

  • shim sources sha256 sum matches in docker image:
# sha256sum shim-15.8.tar.bz2 
a79f0a9b89f3681ab384865b1a46ab3f79d88b11b4ca59aa040ab03fffae80a9  shim-15.8.tar.bz2
  • shim patches:
# diff shim shim-15.8
Only in shim: .git
Common subdirectories: shim/.github and shim-15.8/.github
Common subdirectories: shim/Cryptlib and shim-15.8/Cryptlib
diff shim/Make.defaults shim-15.8/Make.defaults
31c31
< DEFAULT_LOADER        ?= \\\\igel$(ARCH_SUFFIX).efi
---
> DEFAULT_LOADER        ?= \\\\grub$(ARCH_SUFFIX).efi
Common subdirectories: shim/data and shim-15.8/data
Only in shim: dbx.esl
Only in shim: debian
Common subdirectories: shim/gnu-efi and shim-15.8/gnu-efi
Common subdirectories: shim/include and shim-15.8/include
Common subdirectories: shim/lib and shim-15.8/lib
Only in shim: sbat.igel.csv
diff shim/shim.h shim-15.8/shim.h
73c73
< #define DEFAULT_LOADER L"\\igelx64.efi"
---
> #define DEFAULT_LOADER L"\\grubx64.efi"
76c76
< #define DEFAULT_LOADER_CHAR "\\igelx64.efi"
---
> #define DEFAULT_LOADER_CHAR "\\grubx64.efi"
88c88
< #define DEFAULT_LOADER L"\\igelia32.efi"
---
> #define DEFAULT_LOADER L"\\grubia32.efi"
91c91
< #define DEFAULT_LOADER_CHAR "\\igelia32.efi"
---
> #define DEFAULT_LOADER_CHAR "\\grubia32.efi"
Common subdirectories: shim/test-data and shim-15.8/test-data
  • build is reproducible:
# sha256sum build/output/shimx64.efi 
90ce33685c5ac241b582bdf27d0ae872b72263a5ae8271ef7f738bd1d13e8e63  build/output/shimx64.efi
  • NX is not set:
# objdump -x /build/output/shimx64.efi | grep DllCharacteristics
DllCharacteristics      00000000
  • shim sbat looks fine
# objcopy --only-section .sbat -O binary /build/output/shimx64.efi /dev/stdout
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,4,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.igel,1,Igel,shim,15.8,https://www.igel.com
  • Certificate:
    • valid for 30 years
    • RSA 4096 bits key
    • Is a CA certificate
# openssl x509 -noout -inform DER -in igel-uefi-ca.der -text
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            0e:88:b9:8d:0b:7e:00:6a:92:9c:9a:be:39:4e:af:54:57:f8:bc:85
        Signature Algorithm: sha256WithRSAEncryption
        Issuer: C = DE, ST = Bremen, L = Bremen, O = IGEL Technology GmbH, OU = IGEL Technology GmbH Certificate Authority, CN = IGEL Technology GmbH Root CA
        Validity
            Not Before: Mar 26 08:55:59 2024 GMT
            Not After : Mar 19 08:55:59 2054 GMT
        Subject: C = DE, ST = Bremen, L = Bremen, O = IGEL Technology GmbH, OU = IGEL Technology GmbH Certificate Authority, CN = IGEL Technology GmbH Root CA
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                Public-Key: (4096 bit)

Note:

I think you should split your kernel patch to make things easier to review.

@steve-mcintyre steve-mcintyre added 2 reviews needed Needs 2 (additional) successful reviews before being accepted Accredited review needed Needs a successful review by an accredited reviewer labels Nov 13, 2024
@tina-igel
Copy link

I've split the single patch file into individual diffs to make it easier to get an idea of what has been patched in our kernel. Please let me know if you would like them to be organized in a different way.

@af-kulow
Copy link
Author

For information, I updated my original comment, providing a link to our first SHIM submission to the board, which was accepted.

Sorry for the inconvenience/confusion for stating N/A at first.

@igelboot
Copy link

igelboot commented Dec 4, 2024

Hi Steve,
Dear Review Board,

I still see the 'new vendor' label here. But we're in fact the same team that submitted #11 (via this account). So we're not new, and this is not our first submission.

Sincerely,
Mathias

@steve-mcintyre steve-mcintyre added 1 review needed Needs 1 (additional) successful review before being accepted and removed new vendor This is a new vendor 2 reviews needed Needs 2 (additional) successful reviews before being accepted labels Dec 16, 2024
@steve-mcintyre
Copy link
Collaborator

Not a new vendor, so updated labels and requirements appropriately.

@aronowski
Copy link
Collaborator

Review as per IGEL-shim-x64-20240807.

Looks alright at a first glance. No ephemeral keys yet, but planned in the future.

Please note that I'm not competent to review the custom GRUB2 and kernel patches. I'll raise this at the next call - please, wait for now, and ping me if no answer arrives by the end of the month.

@aronowski aronowski self-assigned this Dec 22, 2024
@aronowski aronowski removed their assignment Jan 13, 2025
@igelboot
Copy link

Dear reviewers,

On behalf of the IGEL OS team, I would like to ask for a status/progress update. Our team member @af-kulow has helped with the reviews of #433 and #431 to show our good will.
We really don't know where our review stands at present, as we are seeing no activity in this Github issue. Please let us know.

Sincerely,
Mathias

@jsetje jsetje self-assigned this Jan 15, 2025
@jsetje
Copy link
Collaborator

jsetje commented Jan 15, 2025

Hi, it sounds like there is a concern about custom kernel and GRUB patches. I'm happy to take a look, especially at any GRUB patches. I assume they are for booting from your custom volume manager?

If your kernel changes don't impact the EFI stub or any other code that could run before we exit boot services, I'm not terribly concerned about them, although it would be nice if you can explicitly state that you thought about any lockdown implications.

I took a quick glance and didn't see a pointer to patches or a repo. If someone could supply one, I'd appreciate it.

@tina-igel
Copy link

Hi, this issue refers to specific tags for the SHIM and GRUB patches at the top but there have been additions to the repo in the meantime, specifically a new subdirectory, linux-patches/patches, in which we split the single diff file into one diff per source file to make them easier to review. If you look at https://github.com/IGEL-Technology/shim-review/tree/main/ you'll find the latest version including all GRUB and Linux patches.

The Linux patches are primarily for back-ports of modules into the LTS kernels we're using or for fixes/workarounds we need for specific hardware. For example, we once had an issue where a new Intel graphics driver changed the order of display ports but our customers use centrally managed configurations, including the monitor order, and we need to make sure that something like a public passenger information display doesn't all of a sudden display the back-office desktop.

We don't have many security-relevant patches, none of which touching the lockdown concept. Security is one of our top concerns, thus we are very careful when we do things which might impact security.

Security is also part of the GRUB patches. For example, we have a fixed boot menu with a few debug and recovery options but we don't want to give people full access to the kernel command line at boot time. We also need to make GRUB aware of our custom volume manager, correct, and we have automatic recovery features in case a system update failed and we need to switch back to the previous system partition.

@jsetje
Copy link
Collaborator

jsetje commented Jan 16, 2025

Oh, I see you included patches in directories in the review repo. I completely missed that by looking for a link.

I'm obviously unable to do a full review, but I did try to find relevant changes. In general the intent appears to be in-line with creating a more secure system.

From a shim review perspective, the custom partitioning work is interesting. Is this a superset of GPT or truly custom? Either way, is there somewhere that describes how this works, especially if and how it interacts with the UEFI system partition?

In general, I don't have the base repo that these patches apply to, is debian in fact carrying the igelfs and related code?

It would be nice to understand why you needed to add signature validation to what appears to be a new code path. And if that code exists in other downstream shims, does it need to be fixed there as well?

Finally, I'm missing context, but is it actually OK to make goto fail conditional here?
https://github.com/IGEL-Technology/shim-review/blob/main/grub-patches/IGEL-fix-secure-boot-loading-of-unsigned-kernel.diff

Looking at the kernel I didn't spot anything obviously problematic even if I was astounded by the amount of one off graphics fixes.

@tina-igel
Copy link

tina-igel commented Jan 17, 2025 •
edited
Loading

The partition format is completely custom. It's similar to LVM in that it divides the available space in sections of 256KB which can be allocated to IGEL partitions in arbitrary order, i.e. partitions don't have to be consecutive and can be spread across the available free space. It started a long time ago with thin clients and small flash devices, hence the relatively small section size. IGEL OS uses separate partitions its components, e.g. there's one for the base system, one for each app, .... All of these are read-only squashfs partitions with full integrity checks across meta and file data. Read-write data partitions can be allocated, too - those are usually encrypted.

The base repo for the current set of patches is GRUB 2.06-13 (Debian Bookworm). However, we moved to GRUB 2.12-1 (Debian Bookworm Backports) recently due to a compatibility issue with a pre-production device connected to a 4K monitor. The new version doesn't require the EFI patch you mentioned anymore. I'll upload a new set of patches for GRUB 2.12-1 ASAP and let you know when I've done so.

And yes, there are quite a few patches for graphics issues...

@jsetje
Copy link
Collaborator

jsetje commented Jan 17, 2025 •
edited
Loading

This is going to be a shim that trusts a fair amount of vendor specific code while boot services are active. And while this isn't typical, it's also not unique. If you ever do find anything in your code that could be leveraged into a secure boot bypass please make sure to communicate that issue to the UEFI CA signers (Microsoft) in an appropriate manner.

@jsetje jsetje added accepted Submission is ready for sysdev 1 review needed Needs 1 (additional) successful review before being accepted Accredited review needed Needs a successful review by an accredited reviewer and removed 1 review needed Needs 1 (additional) successful review before being accepted Accredited review needed Needs a successful review by an accredited reviewer accepted Submission is ready for sysdev labels Jan 17, 2025
@jsetje
Copy link
Collaborator

jsetje commented Jan 17, 2025

Before we accept this, can you explain how and why the signature verification appears here:

https://github.com/IGEL-Technology/shim-review/blob/main/grub-patches/IGEL-check-kernel-signature-in-failsafe-mode.diff

Is support for you partition in fact in the debian downstream branches, and if so, should they carry this newer patch as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jsetje jsetje

Labels
1 review needed Needs 1 (additional) successful review before being accepted Accredited review needed Needs a successful review by an accredited reviewer contacts verified OK Contact verification is complete here (or in an earlier submission)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@igelboot @steve-mcintyre @jsetje @jclab-joseph @aronowski @zeetim @tina-igel @af-kulow