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.

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

secure boot: lockdown, grub fallback, CI #2299

Merged
merged 6 commits into from
Sep 17, 2024
Merged

Conversation

jepio
Copy link
Member

@jepio jepio commented Sep 10, 2024

secure boot: lockdown, grub fallback, CI

How to use

[ describe what reviewers need to do in order to validate this PR ]

Testing done

Tested here:
amd64: http://jenkins.infra.kinvolk.io:8080/job/container/job/test/26834/cldsv/
arm64: http://jenkins.infra.kinvolk.io:8080/job/container/job/test/26780/cldsv/

  • Changelog entries added in the respective changelog/ directory (user-facing change, bug fix, security fix, update)
  • Inspected CI output for image differences: /boot and /usr size, packages, list files for any missing binaries, kernel modules, config files, kernel modules, etc.

Copy link
Contributor

@chewi chewi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM besides concerns already raised. I'm surprised these patches aren't mainline already.

@ader1990
Copy link
Contributor

As there is a work in progress and if the desire is to move to a new kernel soon-ish (#2300), I would suggest to have these patches only for the 6.10 or the new LTS 6.12, and to not have to maintain them for 6.6 too (less work if we maintain only for very up-to-date Linux versions). And maybe in the meantime find a way to upstream / gather some support for mainline.

Copy link

github-actions bot commented Sep 10, 2024

@jepio
Copy link
Member Author

jepio commented Sep 10, 2024

LGTM besides concerns already raised. I'm surprised these patches aren't mainline already.

They will never be mainline. You can see the full discussion here: https://lore.kernel.org/lkml/[email protected]/.

As there is a work in progress and if the desire is to move to a new kernel soon-ish (#2300), I would suggest to have these patches only for the 6.10 or the new LTS 6.12, and to not have to maintain them for 6.6 too (less work if we maintain only for very up-to-date Linux versions). And maybe in the meantime find a way to upstream / gather some support for mainline.

The aim is to submit a shim for signing in october so we can't risk it by coupling the required changes to other ones like the kernel upgrade. If we have to maintain patches on two kernel branches then so be it.

echo "++++ ${CIA_TESTSCRIPT}: Using existing ${bios} ++++"
if [ -f "${firmware}" ] ; then
echo "++++ ${CIA_TESTSCRIPT}: Using existing ${firmware} ++++"
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation seems off?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, fixed it

@jepio jepio force-pushed the jepio+sayan/sboot-lockdown branch 2 times, most recently from 0b3cefe to a9e42a8 Compare September 11, 2024 10:51
@jepio
Copy link
Member Author

jepio commented Sep 11, 2024

Jenkins CI ran successfully here: http://jenkins.infra.kinvolk.io:8080/job/container/job/packages_all_arches/4670/cldsv/
GHA CI failed, pushed a fix and rerunning.

Copy link
Member

@sayanchowdhury sayanchowdhury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jepio
Copy link
Member Author

jepio commented Sep 17, 2024

@ader1990 are you OK with merging?

jepio and others added 6 commits September 17, 2024 11:01
Shim signing for secure boot requires enforcing lockdown. There are three ways
we can do this:

1. setting CONFIG_LOCK_DOWN_KERNEL_FORCE_INTEGRITY=y. This unconditionally
   prevents loading unsigned kernel modules.
2. setting lockdown=integrity on the kernel cmdline from a signed Grub
   configuration. This would be OK, but Grub is not updated in the field right
   now, so we'd be stuck.
3. incorporate the secure-boot-lockdown patches that other major distros are using.

We're going to go with 3, because this only enforces lockdown when secure boot
is actually enabled and lets us change approach later on.

These patches are sourced from Debian:
https://sources.debian.org/src/linux/6.6.13-1~bpo12%2B1/debian/patches/features/all/lockdown/.

Signed-off-by: Jeremi Piotrowski <[email protected]>
This is a requirement of the shim signing process.

Signed-off-by: Jeremi Piotrowski <[email protected]>
With secure boot a failed shim signature check will leave us stuck in grub.
Enable automatic fallback in that case.
Signed-off-by: Sayan Chowdhury <[email protected]>
Signed-off-by: Jeremi Piotrowski <[email protected]>
@ader1990
Copy link
Contributor

@ader1990 are you OK with merging?

I suppose I am okay with merging, but I still think it is not an optimal way forward.

Because having this unreviewed (and soon rebased, conflicts need to be taken care of, etc) patches affect Flatcar images that will never be used in a secure boot environment, thus adding more possible security vulnerabilities. I suggested to have a more one-step at a time approach, activate this patch just for the OEMs that we know need the secure boot implementation now, see how it goes for a while, and then activate the patches for all the images.

I am saying this because some of the code from the patches executes independently of the kernel config option is set or not, which opens the possibility for unwanted behaviours.
If the patches were just an if check_kernel_config: do something, it would have been great. But it is not the case.

@jepio
Copy link
Member Author

jepio commented Sep 17, 2024

@ader1990 are you OK with merging?

I suppose I am okay with merging, but I still think it is not an optimal way forward.

Because having this unreviewed (and soon rebased, conflicts need to be taken care of, etc) patches affect Flatcar images that will never be used in a secure boot environment, thus adding more possible security vulnerabilities. I suggested to have a more one-step at a time approach, activate this patch just for the OEMs that we know need the secure boot implementation now, see how it goes for a while, and then activate the patches for all the images.

I am saying this because some of the code from the patches executes independently of the kernel config option is set or not, which opens the possibility for unwanted behaviours. If the patches were just an if check_kernel_config: do something, it would have been great. But it is not the case.

The patches don't do anything unless secure boot is enabled at runtime (the switch checks whether firmware set SECUREBOOT=1)

 		case efi_secureboot_mode_enabled:
 			set_bit(EFI_SECURE_BOOT, &efi.flags);
+#ifdef CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT
+			lock_kernel_down("EFI Secure Boot",
+					 LOCKDOWN_INTEGRITY_MAX);
+#endif

So lockdown is only triggered in a secure boot environment.

We can't ship a different kernel configuration for different OEMs because that would require preparing separate update payloads and nebraska can't handle that. These patches are part of standard Ubuntu, Debian, Fedora, RHEL, SuSe kernels so they have been well tested.

I would also rather not have them.

@ader1990
Copy link
Contributor

ader1990 commented Sep 17, 2024

@ader1990 are you OK with merging?

I suppose I am okay with merging, but I still think it is not an optimal way forward.
Because having this unreviewed (and soon rebased, conflicts need to be taken care of, etc) patches affect Flatcar images that will never be used in a secure boot environment, thus adding more possible security vulnerabilities. I suggested to have a more one-step at a time approach, activate this patch just for the OEMs that we know need the secure boot implementation now, see how it goes for a while, and then activate the patches for all the images.
I am saying this because some of the code from the patches executes independently of the kernel config option is set or not, which opens the possibility for unwanted behaviours. If the patches were just an if check_kernel_config: do something, it would have been great. But it is not the case.

The patches don't do anything unless secure boot is enabled at runtime (the switch checks whether firmware set SECUREBOOT=1)

 		case efi_secureboot_mode_enabled:
 			set_bit(EFI_SECURE_BOOT, &efi.flags);
+#ifdef CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT
+			lock_kernel_down("EFI Secure Boot",
+					 LOCKDOWN_INTEGRITY_MAX);
+#endif

So lockdown is only triggered in a secure boot environment.

We can't ship a different kernel configuration for different OEMs because that would require preparing separate update payloads and nebraska can't handle that. These patches are part of standard Ubuntu, Debian, Fedora, RHEL, SuSe kernels so they have been well tested.

I would also rather not have them.

efi_get_fdt_params is changed, also some small other places in the process. This code change runs irrespective of the secureboot kernel config.

If other operating systems use the same byte-wise patches, I suppose it is a smaller risk. Let's hope for the best.

I see, for example, that 6.10 also has the same level of support for the patches: https://sources.debian.org/data/main/l/linux/6.10.6-1~bpo12+1/debian/patches/features/all/lockdown/, which is great, as we can copy those over byte-wise. Maybe we could add a small .github ci automation to get these patches automatically on a kernel change, like we do with the gentoo upstream - to have the same source of truth Flatcar and Debian.

@jepio jepio merged commit a23e5bb into main Sep 17, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

6 participants