search

rhboot / shim-review

Reviews of shim
67 stars 127 forks source link

Shim 15.8 + extra patch for EgoSecure (Matrix42) #353

Open BogdanAriton opened 10 months ago

BogdanAriton commented 10 months ago

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

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

https://github.com/Matrix42AG/shim-review/releases/tag/2024-07-31_15-20-09

What is the SHA256 hash of your final SHIM binary?

7409c799415b69dfc6222a844b072f79aa14f5b57f1bea07a442c17d74390b33

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

https://github.com/rhboot/shim-review/issues/199 https://github.com/rhboot/shim-review/issues/169 https://github.com/rhboot/shim-review/issues/7

MuthuvelKuppusamy commented 10 months ago

  1. This recommended patch (0001-Enable-the-NX-compatibility-flag-by-default.patch) is missing.

  2. Custom patch included.

  3. Build & sha256sum is good.

  4. f75973853cbdbb95190efdc61fd7a044fc6dd61f0138fbeaa0e3ec44d69211ff shimx64.efi

  5. Contents of section .sbat: d5000 73626174 2c312c53 42415420 56657273 sbat,1,SBAT Vers d5010 696f6e2c 73626174 2c312c68 74747073 ion,sbat,1,https d5020 3a2f2f67 69746875 622e636f 6d2f7268 ://github.com/rh d5030 626f6f74 2f736869 6d2f626c 6f622f6d boot/shim/blob/m d5040 61696e2f 53424154 2e6d640a 7368696d ain/SBAT.md.shim d5050 2c332c55 45464920 7368696d 2c736869 ,3,UEFI shim,shi d5060 6d2c312c 68747470 733a2f2f 67697468 m,1,https://gith d5070 75622e63 6f6d2f72 68626f6f 742f7368 ub.com/rhboot/sh d5080 696d0a73 68696d2e 65676f73 65637572 im.shim.egosecur d5090 652c312c 4d617472 69783432 20476d62 e,1,Matrix42 Gmb d50a0 482c7368 696d2c31 352e342c 68747470 H,shim,15.4,http d50b0 733a2f2f 6d617472 69783432 2e636f6d s://matrix42.com d50c0 0a .

  6. Contents of section .sbatlevel: 88000 00000000 08000000 26000000 73626174 ........&...sbat 88010 2c00312c 00323032 32303532 34303000 ,.1,.2022052400. 88020 0a006772 75622c32 0a007362 61742c00 ..grub,2..sbat,. 88030 312c0032 30323231 31313530 30000a00 1,.2022111500... 88040 7368696d 2c320a67 7275622c 330a00 shim,2.grub,3..

BogdanAriton commented 10 months ago

Hi, thank you for the response!

  1. This recommended patch (0001-Enable-the-NX-compatibility-flag-by-default.patch) is missing.

I've updated the repo with the recommended patch and I've rebuild the shim and updated README and ISSUE_TEMPLATE with the latest sha256 value.

I've also update the sbat value from :

shim.egosecure | 1 | Matrix42 GmbH | shim | 15.4 | https://matrix42.com to shim.egosecure | 1 | Matrix42 GmbH | shim | 15.7 | https://matrix42.com

Thanks for looking into this!

BogdanAriton commented 9 months ago

Hi @MuthuvelKuppusamy, @THS-on, could we have a review over the latest change? We are trying to fix current problems for our customers. Thanks!

THS-on commented 9 months ago

@BogdanAriton there was quite a big queue of submissions and we are still working through it, mostly in our spare time. I'll try to have a look at it, once most of the old submissions got some updates and I find some free time :)

In the meantime it would be awesome, if you can help with the submission process by taking a look at other submissions and adding an (unofficial) review (see here for guidelines https://github.com/rhboot/shim-review/blob/main/docs/reviewer-guidelines.md or have look at the accepted submissions).

BogdanAriton commented 9 months ago

@THS-on - sure I've went through the reviewer-guidelines.md, hopefully I'm not missing anything. Thank you for you're effort! 👍 [Edit] - I read this on the diagonal :) Sure, I can take a look over other submissions and help with the process.

aronowski commented 9 months ago

I haven't checked the shim binary checksums and reproducibility, since the .sbatlevel section has the binutils bug.

Please, fix it and I'll perform further review. If you need help with this, feel free to ping me here.

BogdanAriton commented 9 months ago

I haven't checked the shim binary checksums and reproducibility, since the .sbatlevel section has the binutils bug.

Please, fix it and I'll perform further review. If you need help with this, feel free to ping me here.

@aronowski - I need help here because I'm not sure what I need to fix. The issue you are referencing has been merged a while back, so it should be part of the current version I presume.

dennis-tseng99 commented 9 months ago

@BogdanAriton, some mistakes are shown in the following: 

00000000 08000000 26000000 73626174 ........&...sbat
2c00312c 00323032 32303532 34303000 ,.1,.2022052400.  <-- should be 2c31
0a006772 75622c32 0a007362 61742c00 ..grub,2..sbat,.
312c0032 30323231 31313530 30000a00 1,.2022111500...  <-- should be 2c32
7368696d 2c320a67 7275622c 330a00   shim,2.grub,3..
aronowski commented 9 months ago

@BogdanAriton, it's a matter of integrating the changes, I linked to, to your build process.

I did something like this by porting that patch, naming it buggy-binutils.patch and adding to my review here.

The integration part might be what I'd need to dive deeper into, as I'm more familiar with the Fedora/RHEL family, rather than Debian.

BogdanAriton commented 9 months ago

@aronowski - thanks for the help! I've made the update. I've also checked the .sbatlevel and it looks fine now.

objdump shim.efi -s -j .sbatlevel

Contents of section .sbatlevel:
 88000 00000000 08000000 22000000 73626174  ........"...sbat
 88010 2c312c32 30323230 35323430 300a6772  ,1,2022052400.gr
 88020 75622c32 0a007362 61742c31 2c323032  ub,2..sbat,1,202
 88030 32313131 3530300a 7368696d 2c320a67  2111500.shim,2.g
 88040 7275622c 330a00                      rub,3..
BogdanAriton commented 9 months ago

@aronowski - any chance we can have a review? 💯 , we're trying to solve some of the issues we have for our clients. Thank you!

aronowski commented 9 months ago

@BogdanAriton, yes, I'll try my best, but considering that the bootchain is different from what I know and that I'm not a low-level programmer to have an authority to speak about the shim PR 620, it's not going to be easy for me to verify everything thoroughly.

First, I'll just scratch the surface and focus on the easier parts. The good thing is that some older shims were already signed and the applications accepted.

kukrimate commented 9 months ago

We do not employ ephemeral keys. Our practice does not involve granting shell access to the kernel. Instead, we utilize a minimal kernel configuration with a login dialog exclusively for user authentication purposes.

Why is not providing shell access to your users relevant to your kernel's implication on the secure boot attack surface.

If your kernel is signed with a key inside your shim, a hyptohetical malware author can take your shim and kernel by themselves and glue any userspace on it.

This is true unless your kernel is only executable as part of a signed UKI-like image containing an initrd and cmdline. Otherwise all command line options, the entire initrd, and syscalls from userspace are part of your attack surface.

It's worth noting that we only utilize the Linux kernel to display a login dialog for user authentication, allowing them to proceed to boot into Windows. We do not grant shell access or permit any actions that involve modifying kernel modules or similar activities.

Does this mean that you execute a Linux based environment prior to Windows as a login dialog, then you chainload Windows?

If so, how do you verify that your bootloader cannot be used to load backdoored copies of Windows?

aronowski commented 9 months ago

OK, I did scratch the surface as promised. Some entries will need fixing, but let's start with the good ones:

Shim build reproduces file, checksum matches. Its characteristics are alright, but there are ongoing discussions about the handling of the bootchain in the context of NX support. If it turns out it shall not be present in this case, it's just a matter of rolling back the patch that applies the related flag.

*******************************************************************************
### If your SHIM launches any other components, please provide further details on what is launched.
*******************************************************************************
The shim loads our custom loader (the DEFAULT_LOADER provided to the shim). We use a custom second-stage loader. According to business logic, the loader can start Microsoft Windows Boot Manager or own Linux Kernel image. All of the files we load are code-signed with the EV vendor certificate provided to the shim.
To start the loader we create our own EFI boot entry.

Yes, it's the esbootmg.efi file. While I have never worked with it, the same software resulted in a successful previous application. Therefore, I'd say this is OK.

And now the things that need fixing:

*******************************************************************************
### Do you use an ephemeral key for signing kernel modules?
### If not, please describe how you ensure that one kernel build does not load modules built for another kernel.
*******************************************************************************
We do not employ ephemeral keys. Our practice does not involve granting shell access to the kernel. Instead, we utilize a minimal kernel configuration with a login dialog exclusively for user authentication purposes.

A proper strategy will be needed to be implemented if using ephemeral keys is not possible. It's required.

You can base one on these proposals by @THS-on and ask for help if needed.

*******************************************************************************
### If your boot chain of trust includes a Linux kernel:
### Is upstream commit [1957a85b0032a81e6482ca4aab883643b8dae06e "efi: Restrict efivar_ssdt_load when the kernel is locked down"](https://git.kernel.org/pub/scm
/linux/kernel/git/torvalds/linux.git/commit/?id=1957a85b0032a81e6482ca4aab883643b8dae06e) applied?
### Is upstream commit [75b0cea7bf307f362057cc778efe89af4c615354 "ACPI: configfs: Disallow loading ACPI tables when locked down"](https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75b0cea7bf307f362057cc778efe89af4c615354) applied?
### Is upstream commit [eadb2f47a3ced5c64b23b90fd2a3463f63726066 "lockdown: also lock down previous kgdb use"](https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eadb2f47a3ced5c64b23b90fd2a3463f63726066) applied?
*******************************************************************************
We are currently running Kernel version 5.5.7. Our intention is to upgrade to the latest kernel version and apply the most recent security patches. However, our current priority is to address the issue involving a certificate mismatch under the shim. Resolving this problem is crucial for unblocking our clients.

It's worth noting that we only utilize the Linux kernel to display a login dialog for user authentication, allowing them to proceed to boot into Windows. We do not grant shell access or permit any actions that involve modifying kernel modules or similar activities.

As far as I can see, only the first commit would be present in that kernel. Please, upgrade to the most recent version possible, as these patches will be required in most cases (most common configurations).

*******************************************************************************
### What patches are being applied and why:
*******************************************************************************
[We need to check if "loader_str" is an actual path or not](https://github.com/rhboot/shim/commit/fc26b47924e6c46e6391552014fc6b4f716f6e65)
Reason for the patch: Because our shim is replacing the windows boot loader: bootmgfw.efi, the paramters sent to windows boot loader cannot be considered a path to secondary boot loader. Our default boot loader is set while building the shim.

I don't have the experience needed to verify, if this patch won't sometimes be causing a behavior that Microsoft wouldn't approve - I'll leave this one for the experts here.

There's also a curiosity that wasn't there in the last time you got your successful application - there's no tag referring to the commit to be reviewed. I suppose this makes it easier for Microsoft to handle things, so please add it and edit the issue's entry message, so the links point to it.

Also remember to update the answer to the "What is the SHA256 hash of your final SHIM binary?" question in that message.

I can see the contacts changed since the last review - I'll send verification emails soon.

BogdanAriton commented 9 months ago

@aronowski - Thank you for the notes and the review! (also very good points from @kukrimate ) I'm out for a couple of days and will try to address everything when I come back. I'll just add a few things that I can address right now:

It's worth noting that we only utilize the Linux kernel to display a login dialog for user authentication, allowing them to proceed to boot into Windows. We do not grant shell access or permit any actions that involve modifying kernel modules or similar activities.


As far as I can see, only the first commit would be present in that kernel. Please, upgrade to the most recent version possible, as these patches will be required in most cases (most common configurations).

Upgrading the Kernel will potentially impact the product and, as I've mentioned in the template, the chain of trust got broken at some point because the old cert expired and part of the pipeline that builds the product just updated the cert we used to sign our efi apps and other cab files, but the shim got left behind. This happened at the start of the year and we have a lot of clients that can't enable secure boot because of this problem.

What patches are being applied and why:

We need to check if "loader_str" is an actual path or not Reason for the patch: Because our shim is replacing the windows boot loader: bootmgfw.efi, the paramters sent to windows boot loader cannot be considered a path to secondary boot loader. Our default boot loader is set while building the shim.



I don't have the experience needed to verify, if this patch won't sometimes be causing a behavior that Microsoft wouldn't approve - I'll leave this one for the experts here.

This particular patch is in debate, there is another method I can use and that is to avoid loading options all together, until an actual patch can be made for this particular issue and I will make that change when I come back. ( I can add more details about the patch if you think is necessary)

There's also a curiosity that wasn't there in the last time you got your successful application - there's no tag referring to the commit to be reviewed. I suppose this makes it easier for Microsoft to handle things, so please add it and edit the issue's entry message, so the links point to it.

Can you elaborate further, which commit are you referring to?

Also remember to update the answer to the "What is the SHA256 hash of your final SHIM binary?" question in that message.

Ups! I updated in the readme, but left the old one in the template. (will make the update as soon as possible)

I can see the contacts changed since the last review - I'll send verification emails soon.

Thank you!

aronowski commented 9 months ago

Verification emails sent.

aronowski commented 9 months ago

@BogdanAriton,

I'm out for a couple of days and will try to address everything when I come back.

Unsure if this will be a well-deserved rest or dealing with other tough stuff, but hopefully the former - this review process and the software maintenance is hard already.

Upgrading the Kernel will potentially impact the product and, as I've mentioned in the template, the chain of trust got broken at some point because the old cert expired and part of the pipeline that builds the product just updated the cert we used to sign our efi apps and other cab files, but the shim got left behind.

Unless someone at Microsoft or from the committee says otherwise, I think that maybe that kernel could be patched with the "75b0cea7bf307f362057cc778efe89af4c615354 "ACPI: configfs: Disallow loading ACPI tables when locked down"" patch and a laboratory setting arranged to see if the proof-of-concept exploit linked there works or does not work. If I had more time and a trial access to EgoSecure Full Disk Encryption, I'd test this out myself.

The issue that "eadb2f47a3ced5c64b23b90fd2a3463f63726066 "lockdown: also lock down previous kgdb use"" fixes can be, as far as I'm concerned, fixed by simply setting CONFIG_KDB_DEFAULT_ENABLE=0x0 for the building process. Although don't believe me blindly and verify things with the kernel experts we have here. I can ping one if you want. ;-)

This happened at the start of the year and we have a lot of clients that can't enable secure boot because of this problem.

I know that feeling - will try my best to make the thing compliant with the current requirements.

This particular patch is in debate, there is another method I can use and that is to avoid loading options all together, until an actual patch can be made for this particular issue and I will make that change when I come back. ( I can add more details about the patch if you think is necessary)

We can give it a shot, but still, for a proper verification I'd need to become a low-level developer or prepare such a laboratory setting that could provide me with a definite answer.

Can you elaborate further, which commit are you referring to?

a8da6c3fbc2630a2034ffde4391172d7a5e875df - right now it's the tip of the branch main and no tags point to it.

If the update was to happen today, there should be a tag named egosecure-shim-x64-20231211 like in the earlier application.

Ups! I updated in the readme, but left the old one in the template. (will make the update as soon as possible)

:+1:

AndreiVoicuM42 commented 9 months ago

@aronowski - Hi, and thanks for all the help and effort with our submission. Here are my words from the verification email: Alice relearn fries laboring giantess industries cacophony endeavor spunk export

julian-klode commented 9 months ago

Reviewers: Please refrain from accepting this for the time being, we are having some additional discussions in the keybase about this to make sure that MS is aware of this use case and comfortable with signing it this way. Because we're not sure this is well highlighted to them via this process, and it's not clear how the previous shim ended up signed despite strong rejection from multiple reviewers for the first submission.

Regarding the kernel patches, we do not care if this shim is crucial for unlocking your customers or not, including the patches is mandatory to get the shim signed, there is no discussion to be had there. I'd be open to signing the shim with an older sbat level that was valid at the time the patches were not included but I don't think this provides you a lot of value.

The shim also sets the NX bit as far as I can tell from the patch, so an NX-compatible kernel is mandatory, which we believe I think 6.7 now is, but due to lack of real actual hardware to test this for most people it's all a bit fuzzy, so that's another no-go. #359 has more details on this, and yes I'm aware this is a 360° turn from what @MuthuvelKuppusamy asked in his review, but sadly it's the reality that stuff with NX is not going well.

BogdanAriton commented 9 months ago

@aronowski - here are the my words from the verification email: "topples prefabricated heckling outmoded tailcoat waylays escapade whomever evaporate deployed"

AndreiVoicuM42 commented 9 months ago

@julian-klode Thanks for your input, we discussed internally, we will address the issues brought up asap and get back to you with an updated version for review.

THS-on commented 6 months ago

@AndreiVoicuM42 what the status? Please update the submission to 15.8 or create a new one.

BogdanAriton commented 6 months ago

Hi @THS-on , thanks for the message, we've update our source to use Linux 6.6.9 (with the latest CVE patches and without the NX bit) and we're in the late stages of testing. Will make the necessary updates to the README as soon as possible. We will continue on this review if that's OK with you.

BogdanAriton commented 6 months ago

@THS-on , @aronowski - I've updated the review. Please let us know if we should change or update anything.

AndreiVoicuM42 commented 6 months ago

@julian-klode We addressed all the issues brought up, both by you and other reviewers, are we good to move forward with this submission?

aronowski commented 6 months ago

Reviewing from commit 8d4214eaf0581e1f427da457ef72b2a657e14e27.

Shim doesn't have NX bit set - OK, because:

Ephemeral key is used - OK.

Kernel 6.6.9 is said to be used, so the 3 important upstream commits are already there - OK.

Why is the patch patches/0001-bypass_boot_options.patch used? The answer in the current application revision only says this

This patch will bypass boot options code.

, so is it a simple replacement of the older patches/0002-Fix-failed-to-load-image-with-invalid-parameter.patch patch? Or is there something else going on?

The custom bootloader doesn't have its SBAT entries dumped and pasted to the application. Why is that?

Furthermore, the answer is meant to be a dump of the exact entries from the binaries. Let's take a look at your shim binary and how it was meant to look like:

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.egosecure,1,Matrix42 GmbH,shim,15.8,https://matrix42.com

I ran the command objcopy --only-section .sbat -O binary shim.efi /dev/stdout to get this result. Please, fix the current entry and do the same for your custom bootloader.

Note: I haven't yet checked the kernel patches. This will take me some time, if only I'll have the required experience to do so.

BogdanAriton commented 6 months ago

Hi @aronowski , thank you for taking the time to review.

I'm not entirely sure where I should have put this information.

Please let me know if I should address anything related to the above.

aronowski commented 5 months ago

The bootloader binary's SBAT section should look different. Something similar to:

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
esbootmg,1,Matrix42 GmbH,FDE,1,https://matrix42.com

This is just an example I just invented, so the actual entries may have other names, but the issue still remains.

The patch in question (0001-bypass_boot_options.patch) is a replacement for the old patch. We don't need the load-option functionality and it also causes problems for us because we replace the windows boot loader with the shim and since the windows boot loader receives certain load options that are meaningless to the shim we simply ignore them, otherwise there will be an Error on screen until it falls back to fallback loader.

I'll check things out later. Please, be patient.

BogdanAriton commented 5 months ago

I'm not sure exactly what to do about the SBAT information. Here is what we have:

The sbat data we have setup under the shim review is: shim.egosecure,1,Matrix42 GmbH,shim,15.8,https://matrix42.com

Our second stage boot loader sbat has this information: shim.egosecure,1,Matrix42 GmbH,FDE,1,https://matrix42.com

I may be missing something, but it's probably because I don't fully understand what you are asking. Maybe some more background in this area would be nice.

Thank you! Bogdan

aronowski commented 5 months ago

Your second-stage bootloader binary should have "its"* name referenced in the SBAT section, but it doesn't right now. So, your stage boot loader may have a section like this:

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
esbootmg,1,Matrix42 GmbH,FDE,1,https://matrix42.com

See the second line.

This is a simple summary of the SBAT.example.md document - I don't recommend studying it right now, though.

* This is also a simplification - that should be something sensible that represents your second-stage bootloader in the whole shim community.

BogdanAriton commented 5 months ago

Hi @aronowski, I've made the update with the sbat info for both our shim build and our second stage. (https://github.com/Matrix42AG/shim-review/blob/matrix42ag-shim-x64-20240304/README.md#do-you-add-a-vendor-specific-sbat-entry-to-the-sbat-section-in-each-binary-that-supports-sbat-metadata--grub2-fwupd-fwupdate-systemd-boot-systemd-stub-shim--all-child-shim-binaries-)

aronowski commented 5 months ago

The current SBAT listings look good!

Regarding the patches, I've read them but don't have the experience to tell, if things are safe, after they've been applied. Ping another person, who does have such experience and can help.

Also waiting for an update from Julian on the discussions of this use case.

AndreiVoicuM42 commented 5 months ago

Hi Aronowski, thank you for taking the time to review and check.

@julian-klode could we get an update from your side?

BogdanAriton commented 5 months ago

hi @aronowski , @julian-klode - any new questions on concerns? Anything we can do to help?

aronowski commented 5 months ago

I'd appreciate some help with other applications to make the job easier, especially if there are some problems in them that need to be fixed.

In case of your application, someone will have to review the patches, which I wasn't able to review. Also, still waiting for an update from Julian. In case there's no update soon, I'll raise the issue during the next call.

-- Reply to this email directly or view it on GitHub: https://github.com/rhboot/shim-review/issues/353#issuecomment-2059119543 You are receiving this because you were mentioned.

Message ID: @.***>

aronowski commented 5 months ago

On 2024.03.19 05:47:33, Bogdan Ariton wrote:

  • The patch in question (0001-bypass_boot_options.patch) is a replacement for the old patch. We don't need the load-option functionality and it also causes problems for us because we replace the windows boot loader with the shim and since the windows boot loader receives certain load options that are meaningless to the shim we simply ignore them, otherwise there will be an Error on screen until it falls back to fallback loader.

Update: this patch should be safe, as it historically has already been approved and a signed shim with it received from Microsoft - https://github.com/rhboot/shim-review/issues/257

The kernel patches are another story - I don't have the experience to review them. I wish there was a point in history where they already were approved - would make things easier.

-- Reply to this email directly or view it on GitHub: https://github.com/rhboot/shim-review/issues/353#issuecomment-2007085146 You are receiving this because you were mentioned.

Message ID: @.***>

BogdanAriton commented 4 months ago

On 2024.03.19 05:47:33, Bogdan Ariton wrote: * The patch in question (0001-bypass_boot_options.patch) is a replacement for the old patch. We don't need the load-option functionality and it also causes problems for us because we replace the windows boot loader with the shim and since the windows boot loader receives certain load options that are meaningless to the shim we simply ignore them, otherwise there will be an Error on screen until it falls back to fallback loader. Update: this patch should be safe, as it historically has already been approved and a signed shim with it received from Microsoft - #257 The kernel patches are another story - I don't have the experience to review them. I wish there was a point in history where they already were approved - would make things easier. … -- Reply to this email directly or view it on GitHub: #353 (comment) You are receiving this because you were mentioned. Message ID: @.***>

The kernel patches are not new (I just had to adapt them to the new kernel code), but I don't see any mention of them in past reviews, most likely this wasn't a requirement back then.

I've added more details to each patch (except for the first two since they have very small changes), hopefully it will make more sense about what they do and what are they used for: https://github.com/Matrix42AG/shim-review/blob/matrix42ag-shim-x64-20240304/README.md#do-you-build-your-signed-kernel-with-additional-local-patches-what-do-they-do

I could also be available for a call if you want 👍 and we could spend some time looking at these patches together.

Also, in regards to helping out with other applications I will start to get familiar with the review process/guidelines and help out as much as I can. Thanks again!

aronowski commented 4 months ago

The presence of the patches back then would have made reviewing easier, as I would be able to refer to an authority on their safety. Thank you for the descriptions - I'll ask around for help and will try (no promises though) to analyze them on my own soon, preferably once I have an opportunity to do such work in an office with air conditioning and no external obstructive noise.

Regarding the help with other applications, we've recently added the "easy to review" tag - I'd start with such applications first, as they could prove useful for learning. Furthermore, this quote from my public notes:

The venue is open for those who'd like to contribute - don't be afraid of reviewing other applications even if you feel you may not know everything. No one does. A review may be done only in regard to the area you're comfortable with and still help others, as well as be a way to get some of the aforementioned comfort in the long term.

Thank you - I appreciate it.

BogdanAriton commented 3 months ago

Hi @aronowski , just wanted to let you guys know that I've added my review for this ticket: 412. (I have to say that even though this was easy-to-review it does take a lot of time to make sure everything is covered). Will pick another review at the end of the week.

aronowski commented 3 months ago

On 2024.05.22 01:06:39, Bogdan Ariton wrote:

Hi @aronowski , just wanted to let you guys know that I've added my review for this ticket: 412. (I have to say that even though this was easy-to-review it does take a lot of time to make sure everything is covered)

Thank you! I so much appreciate that! The thing is, reviewing additional security patches is not something anyone can do - hence why I'm so grateful to see some expertise right here.

Checking out things thoroughly isn't easy on it's own - I recall one application missing one out of several security backports, what I noticed after about 7 hours of verifying the code.

I'll raise the issue of this application being stalled, as you've been waiting for a long time - hopefully a response will arrive soon.

-- Reply to this email directly or view it on GitHub: https://github.com/rhboot/shim-review/issues/353#issuecomment-2124134993 You are receiving this because you were mentioned.

Message ID: @.***>

SherifNagy commented 3 months ago

We are waiting some information from MSFT about approving a custom stage boot loader such as yours, in meantime, would you be able to provide the following information? " just some question / thoughts came up in a chat "

steve-mcintyre commented 3 months ago

Although you've been accepted in the past, we've never done formal contact verification. Let's do that now.

Mails on the way.

BogdanAriton commented 3 months ago

Hi @steve-mcintyre, here are the words from the verification email:

squashiest Lana Dominguez reversal Adderley flagstones fatalism Steuben Pan= ama snip

@SherifNagy - thanks for the review, I'll try to address these questions as soon as possible; (I was out yesterday and most of today)

AndreiVoicuM42 commented 3 months ago

Hi @steve-mcintyre, @aronowski already did the contact verification earlier (https://github.com/rhboot/shim-review/issues/353#issuecomment-1850396777), but here is the confirmation for you as well:

heisting deters hotels disestablish unbroken Gino Cavendish substantiates s= pecious Presidents

Thank you for taking over our submission.

BogdanAriton commented 3 months ago

@SherifNagy - thank you for your review (I've added my answers below)

We are waiting some information from MSFT about approving a custom stage boot loader such as yours, in meantime, would you be able to provide the following information? " just some question / thoughts came up in a chat "

* I can see that you have SBAT entry for esbootmg, I assume you do have full sbat support in your esbootmg?
  1. I'm not entirely sure what you mean by "full sbat support" but we add the sbat section using objcopy tool in our makefile. This is how the lines in our makefile look like: 
    
sbat.%.csv : sbat/sbat.%.csv
dos2unix -r -l -F -f -n $< $@
tail -c1 $@ | read -r _ || echo >> $@ # ensure a trailing newline

VENDOR_SBATS := $(sort $(foreach x,$(wildcard sbat/sbat.*.csv),$(notdir $(x))))

define add-vendor-sbat $(OBJCOPY) --add-section ".$(patsubst %.csv,%,$(1))=$(1)" $(2)

endef

sbat_data.o : | $(VENDOR_SBATS) sbat_data.o : /dev/null $(CC) $(CFLAGS) -x c -c -o $@ $< $(OBJCOPY) --add-section .sbat=sbat/sbat.csv \ --set-section-flags .sbat=contents,alloc,load,readonly,data \ $@ $(foreach vs,$(VENDOR_SBATS),$(call add-vendor-sbat,$(vs),$@))



> 
>     * We can see that there was couple of DBX revocation for EgoSecure in July but we don't know which product / binary was that, can you clarify which product and binary is that? this might impact your SBAT entry within your esbootmg
> 
> 
> ```
> 23EBFBC7BC286CEFC68B4920784B926EC28D7965815238325FBD17892177D6F3    340DA32B58331C8E2B561BAF300CA9DFD6B91CD2270EE0E2A34958B1C6259E85        64-bit    EgoSecure    CVE-2020-10713; CVE-2020-14308; CVE-2020-14309; CVE-2020-14310; CVE-2020-14311; CVE-2020-15705; CVE-2020-15706; CVE-2020-15707    20-Jul
> 5C39F0E5E0E7FA3BE05090813B13D161ACAF48494FDE6233B452C416D29CDDBE    C452AB846073DF5ACE25CCA64D6B7A09D906308A1A65EB5240E3C4EBCAA9CC        64-bit    EgoSecure    CVE-2020-10713; CVE-2020-14308; CVE-2020-14309; CVE-2020-14310; CVE-2020-14311; CVE-2020-15705; CVE-2020-15706; CVE-2020-15707    20-Jul
> ```

2. This is a good point, we found this information as well when we started to gather information for this submission of shim-review. These DBX entries are most likely related to esbootmg.efi (our second stage) and they were added in 2020. Our first shim-review (https://github.com/rhboot/shim-review/issues/169) was approved in 2021 and during this review the first SBAT section was added to esbootmg.efi.

> 
>     * What license is your esbootmgr has? can we get a link to the source code or at least the binary?

3. I've uploaded the latest released version of esbootmg.efi (version 24.0.0) here: https://github.com/Matrix42AG/shim-review/blob/matrix42ag-shim-x64-20240304/esbootmg.efi. Small note here, the sbat section will not be the same as the one specified in the README from this submission since we haven't merged our changes related to this review yet.
AndreiVoicuM42 commented 3 months ago

Hi @SherifNagy , @steve-mcintyre , not to pester you :) , but did you hear back from MSFT regarding our custom second stage bootloader? And did you have time to review our answers, or do you need additional information from our side?

SherifNagy commented 3 months ago

@AndreiVoicuM42 We haven't heard anything yet from MSFT side, however I have just question that I am curious about actually:

steve-mcintyre commented 3 months ago

Review of Shim 15.8 + extra patch for EgoSecure (Matrix42)

OK

Issues / queries

Sorry, there's quite a few of these:

steve-mcintyre commented 3 months ago

Also, as @aronowskimentioned - I don't see any git tags on your submission. It's fine that you're using a branch when making changes, but we want to see tags too so that we have a better indication of an unchanging submission.

BogdanAriton commented 1 month ago

Hi @steve-mcintyre , thank you for the review! Sorry for the delayed response, I'll try to answer things as best as possible:

* Easy one first! You say you're changing to a new CA certificate, but
  you're not embedding a CA certificate and you didn't previously (in
  [Shim 15.4 + extra patch for EgoSecure #199](https://github.com/rhboot/shim-review/issues/199)). Just a misunderstanding here, I think?

Right, this is just a mistake on my part. It's not a CA certificate; it's an EV certificate. I've updated the README to reflect this as well. https://github.com/Matrix42AG/shim-review/commit/a5bc73571f48d4f33b86a1e04def70425eda50d8

* The claimed SBAT data for `esbootmg.efi` in your README.md does not
  match what's in the binary. What you describe in the README.md
  includes the SBAT spec link
  `sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md`
  but that's not in the `esbootmg.efi` binary you've given us. You
  mention this in the comments in your submission. Have you fixed
  this, please? Without this, this SBAT data is invalid.

Last time, I uploaded the latest release binary, which didn't include the most recent changes from our shim-branch. This was because a link to the binary or code was requested, and I didn't realize the SBAT section would be needed in that context. Now, I've uploaded the latest binaries from the shim-branch where we're making the shim-related changes: https://github.com/Matrix42AG/shim-review/commit/a5bc73571f48d4f33b86a1e04def70425eda50d8

* If you've had DBX entries added for previous EgoSecure products, it
  would be good to see some discussion of what's happened there. You
  suggested that these might be for esbootmg, but all the CVEs I can
  see tagged there are GRUB CVEs:

  * [CVE-2020-10713](https://github.com/advisories/GHSA-m2fm-gm84-v5jq)
  * [CVE-2020-14308](https://github.com/advisories/GHSA-6f8f-rchr-mhp3)
  * [CVE-2020-14309](https://github.com/advisories/GHSA-8whj-mpcj-4jv6)
  * [CVE-2020-14310](https://github.com/advisories/GHSA-849w-6cw8-9p7m)
  * [CVE-2020-14311](https://github.com/advisories/GHSA-2jpf-4r7j-42qr)
  * [CVE-2020-15705](https://github.com/advisories/GHSA-w739-fjv8-98pq)
  * [CVE-2020-15706](https://github.com/advisories/GHSA-gmqm-wgp3-r69q)
  * [CVE-2020-15707](https://github.com/advisories/GHSA-mf72-cf87-p3p2)
    I didn't think you were using GRUB?

I had to delve deeper into the product's history to uncover this information. It appears that prior to the first shim submission, EgoSecure utilized GRUB on older BIOS machines. The team at that time decided to transition to a UEFI Stub kernel build, allowing the kernel to be loaded directly from our boot loader. The CVEs are related to GRUB and were discovered in 2020. Our first successful shim submission was in 2021, by which time we had already switched to loading the Linux kernel directly from our EFI bootloader. (There was a submission made in 2017, but it was never resolved.)

* @SherifNagy has asked already, but I'd like to see an answer too:
  how are you preventing booting of old/vulnerable bootloader and
  kernel binaries? As part of this: how exactly does esbootmg load the
  kernel or Windows boot manager? We can't see the source, so we have
  to ask. Is it loading and verifying further EFI programs directly,
  or using underlying firmware and/or shim-provided functions? How do
  things work, precisely?

Whether secure-boot is on or off, booting happens the same way, but normally secure-boot should be turned on and this is the scenario we are considering. (will try to point out differences as best as possible)

The diagram below describes the boot process for EgoSecure. Let's break it down:

  1. The shim contains the certificate that can be used to validate all loaded apps: esbootmg.efi, esboot.efi, pba.efi, fde.efi that were signed in our build process. • esbootmg.efi - is our second stage loader which simply loads and verifies esboot.efi • esboot.efi - is the main application that decided which user interface to load based on previously saved data • pba.efi - is our linux bzimage based on linux.6.6.9. During our build we rename the bzimage-uefi to pba.efi. • fde.efi - it's an app that checks the integrity of the system so that, if the system is encrypted, we need certain values to determine if we need to decrypt and makes sure the system is in a valid state

  2. We start with esbootmg.efi which is the default loader built under the shim (DEFAULT_LOADER=esbootmg.efi)

  3. esbootmg.efi will decide to load esboot.efi in secure mode or otherwise, based on the current system state (whether or not secure boot is on) - if secure boot is on, and this is the scenario of interest, we make use of the shim_lock->Verify callback to check if the image is valid or has the correct signature

  4. esboot.efi will perform the more complex operations: • It will decide which user interface to load - information will be found under our KEK sector which an in-memory storage available for both UEFI and Windows. • If we're loading Simple PBA (defined by this value: EfiBootParamSimplePbaGui), which is the default, the GUI defined directly under esboot.efi will be loaded, which in turn will load the windows boot loader: ebootmgfw.efi. • The same applies for Text-Bases PBA (which is defined by this value: EfiBootParamSimplePba) • If the value is set to EfiBootParamPBA then we load the Linux Based PBA - which is a more complex graphical user interface under Linux. For this, we need to load the Linux kernel (pba.efi) directly from the efi app using the following command: pba.efi root=/dev/sda%d espartstart=%d espartlength=%d %d %d %d, and the loaded kernel is also verified via shim_lock->Verify callback.

  5. The kernel will load its user-space via /etc/inittab and will execute the files described in the diagram:

FDE_boot

When loading either esboot, pba or ebootmgfw we’re using gnu-efi-> uefi_call_wrapper(…) – so basically, we’re using EFI code that loads the image in memory. Signature verification is done via the shim_lock->Verify callback.

We sign all our binaries which when secure boot is active this should prevent loading anything down the chain if the signature doesn’t match. We have IMA enabled as part of our kernel compilation:


CONFIG_IMA=y
CONFIG_IMA_MEASURE_PCR_IDX=10
CONFIG_IMA_NG_TEMPLATE=y
CONFIG_IMA_DEFAULT_TEMPLATE="ima-ng"
CONFIG_IMA_DEFAULT_HASH_SHA256=y
CONFIG_IMA_DEFAULT_HASH="sha256"
CONFIG_IMA_WRITE_POLICY=y
CONFIG_IMA_READ_POLICY=y
CONFIG_IMA_APPRAISE=y
CONFIG_IMA_APPRAISE_BOOTPARAM=y

We have a runtime policy that will measure and appraise everything except for certain file systems like tmpfs, debugfs, procfs, sysfs, cgroups …, and a few log files we need. Having IMA enabled we can make sure that the user-space stays the same.

Please let me know if I should add anything else.