Shim 15.8 + extra patch for EgoSecure (Matrix42) #353
Comments
|
|
Hi, thank you for the response!
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 :
Thanks for looking into this! |
|
Hi @MuthuvelKuppusamy, @THS-on, could we have a review over the latest change? We are trying to fix current problems for our customers. Thanks! |
|
@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). |
|
@THS-on - sure I've went through the reviewer-guidelines.md, hopefully I'm not missing anything. Thank you for you're effort! 👍 |
|
I haven't checked the shim binary checksums and reproducibility, since the Please, fix it and I'll perform further review. If you need help with this, feel free to ping me here. |
aronowski
added
the
bug
@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. |
|
@BogdanAriton, some mistakes are shown in the following: |
|
@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. |
|
@aronowski - thanks for the help! I've made the update. I've also checked the .sbatlevel and it looks fine now. |
|
@aronowski - any chance we can have a review? 💯 , we're trying to solve some of the issues we have for our clients. Thank you! |
|
@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. |
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.
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? |
|
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. Yes, it's the And now the things that need fixing: 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. 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). 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. |
aronowski
added
the
contact verification needed
|
@aronowski - Thank you for the notes and the review! (also very good points from @kukrimate )
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.
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)
Can you elaborate further, which commit are you referring to?
Ups! I updated in the readme, but left the old one in the template. (will make the update as soon as possible)
Thank you! |
|
Verification emails sent. |
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.
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
I know that feeling - will try my best to make the thing compliant with the current requirements.
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.
If the update was to happen today, there should be a tag named
👍 |
|
@aronowski - Hi, and thanks for all the help and effort with our submission. |
|
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. |
|
@aronowski - here are the my words from the verification email: |
|
@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. |
aronowski
removed
the
contact verification needed
|
@AndreiVoicuM42 what the status? Please update the submission to 15.8 or create a new one. |
|
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. |
|
@THS-on , @aronowski - I've updated the review. Please let us know if we should change or update anything. |
steve-mcintyre
added
extra review wanted
and removed
question
OK, sounds good!
Let's hope you don't need too many - DBX updates are expensive. For future shims, you might be better off with an embedded I'm not going to say you need to do this now, but it's an In the meantime, I think you're good here today. I'm happy! You'll need a second reviewer to approve this now; I've just tagged |
This sounds interesting, will explore this option for next review. Thank you so much for the time and effort spent here! |
|
@julian-klode you raised concerns about this back in December. Any update there please? |
1 similar comment
|
@julian-klode you raised concerns about this back in December. Any update there please? |
|
Hi @steve-mcintyre , I think we made a great progress since @julian-klode 's comment in December, and addressed all the concerns raised back then. But it is not clear to us, did you discuss this further internally with him? Do we also need him to review this submission in the current state and approve it, or any other second approval would suffice? |
|
I could re-review the application, apart from any custom patches, as I mentioned some time ago. I myself am impressed with all the descriptions and clarifications. I raised the discussion part already - please ping me, if no answer arrives in the next days. |
|
Some sideline comments as @SherifNagy asked for opinions from a broader audience. I find it a bit weird that we are reviewing proprietary DRM solutions. The purpose of this review is not to boot a Linux distribution on a Secure Boot enabled machine for a consumer. It's to enable some convoluted DRM solution where Linux is a cheap iPXE bootloader for a consumer booting a... kiosk/terminal thing? I don't think we, as the Linux distro community, should be signing products from companies like this. It seems more like a cheap way to get this signed, then something that belongs with the mandate that the Linux UEFI/shim community has. It's also weird that we focus on reproducible builds, but allow proprietary components in this build chain. If we not enforcing a F/OSS bootchain already, we should ensure this is written down and checked for future reviews. |
|
Adding a blocked label for now, we are still waiting some info from MSFT folks and will update the ticket once we hear back. |
|
Hi @SherifNagy, I hope you're doing well. We wanted to check in and see if there's been any update from MSFT and if there's anything we can do to support the process. Additionally, we wanted to bring to your attention that on August 13, Microsoft released KB5041580 (OS Builds 19044.4780 and 19045.4780), which impacts users of previous versions of SHIM and also affects earlier releases of our product. |
|
Hi @SherifNagy , @steve-mcintyre . It has been almost 3 weeks since this was blocked, is there no update from MSFT side? Thank you, |
|
@SherifNagy , @steve-mcintyre - Since we've encountered delays with the review, and the issue seems related to the custom second-stage boot loader, we believe it is worth exploring the removal of our second stage entirely, replacing it with GRUB2. I've created a diagram that outlines our proposed approach:
In summary, we are removing esbootmg.efi and replacing it with GRUB2. GRUB2 will handle loading either our custom GUI/Text interface, Linux, or Windows based on flags stored in an encrypted KEK sector, which will be saved in memory. The new EFI app, manage_kek.efi , will provide controlled access to the KEK sector and manage partition start and end values for GRUB via predefined callbacks. This approach helps to avoid introducing unnecessary logic into GRUB itself. The only custom code added to GRUB will be a module that utilizes the predefined callbacks from manage_kek.efi . Please let us know if this proposal would help with the current status of the review. Thank you! |
|
@BogdanAriton I think part of the issue here is that nothing is actually open-source, thus we can only trust what you are telling us on the flow diagrams, we have way to validate nor review what we are actually approving. |
@Foxboron - I'm not sure I follow, which part would be the issue? (Grub/Shim/Linux are open source. The GUI/Text app (esboot), the manage_kek.efi and Windows, of course, will not be open source). The part where grub decides what to load will be opened (will be a simple if/then/else logic). |
That is the problem. You are at that point not operating with the same rules as the linux distroes where everything is free and open-source. I don't think shim-reviewers should be in the business of rubber stamping proprietary solutions when everyone else is operating by the customs of the Free and Open-Source communities. |
@Foxboron - I really appreciate your input, and, in general I agree with you, but I would like to narrow down on what the best approach should here. Since esboot.efi "is now" (or it will be) an endpoint like windows, then manage_kek.efi remains the problem? Eventually grub will be the only boot loader here. |
I don't know. I don't think We shouldn't be signing chainloaders for Windows, shim was written to ensure Linux distributions can continue to book on Windows enabled laptops for consumers to easily install Linux. Not as a way for companies to circumvent the paid signature process from Microsoft directly. |
|
We are still waiting on feedback from MSFT, but yes, as @Foxboron was saying, we are trying to figure out if this is something shim-reviewers should review in first place or not |
|
Hi @SherifNagy , could you shed some light on the discussion with MSFT? I think you can understand the frustration from our point of view, as we get back the exact same answer for months now. Is it an active discussion ongoing, with daily/weekly activity, or is this discussion stagnating? We also reached out to [email protected], explaining the situation here (including the open source/closed source concerns, and linking to this review) and asked for clarifications/guidance. This is the answer we received:
Additionally, @Foxboron suggests in his answers that we want to get this SHIM signed by the community, which I think deserves some clarification to ensure we are all on the same page and address any possible misunderstanding:
To our understanding, the SHIM board does not need to actually sign our shim, just review it before we submit it to MSFT directly. If we understood anything wrong, then please clarify and advise. And @Foxboron, could you expand on the paid option for signing you are referring to, as we could find no mention about it, neither in the article above or in the Partner Center. Thanks, |
|
Could you provide an update on the current status? Additionally, I would appreciate responses to the questions I asked three weeks ago. |
|
Hi @Foxboron, @SherifNagy, @THS-on, @aronowski. I'm trying to understand the differences between this approved review (#385) and our review (same closed source second stage that loads windows). Could you help clarify? Thanks! |
|
Hello.
Regarding that application, which I accepted (and I suppose that's the reason,
why I'm pinged now): the difference is what I already answered a few months ago,
i.e. that I have no such expertise to be able to help with reviewing custom
patches, whereas these in the linked application have been historically accepted
(meaning that I just trust they are safe).
|
|
1) We are trying to follow the process outlined here:
https://techcommunity.microsoft.com/t5/hardware-dev-center/updated-uefi-signing-requirements/ba-p/1062916
, which I am sure you are all familiar with, and it specifies at point 12.
that ANY shim needs to first be reviewed by this SHIM board, hence our
submission.
2) Once we would get the approval here, we will submit our SHIM for review and
signing to Microsoft directly, through the MSFT Partner Center, as it was done
before.
Yes! More on this below.
To our understanding, the SHIM board does not need to actually sign our shim,
just review it before we submit it to MSFT directly. If we understood anything
wrong, then please clarify and advise.
The point of the shim-review repository is to help vendors maintain their
shim-backed bootloader chains in a secure manner and correct potential errors
(in shim-review submissions) for the vendors' shims to be later signed as part
of a separate application (submission) filed in the Microsoft Hardware Dev
Center File Signing Services portal. They are separate entities, even though the
shim-review venue is endorsed by Microsoft and required by and for Microsoft to
sign vendors' shims.
I suppose that the idea of a manual review by Microsoft was meant as part of a
custom bootloader chain, that doesn't rely on shim at all, but instead the first
stage is a custom binary made by Matrix42, signed by Microsoft directly, which
then loads further components, also proprietary.
[...] suggests in his answers that we want to get this SHIM signed by the
community
I can provide a shim binary signed with my own key, if anyone wants a
community-signed shim.
Any jokes aside, it's naturally possible to replace the default
Microsoft-provided EFI signature lists with custom ones and manage everything by
oneself, but then is shim even needed in such a setup, if no further components
rely on it, e.g. on the shim_lock protocol?
|
|
Hi @aronowski - thank you so much for your replay! (lately we had replies few and far between)
We've been learning from these submissions throughout the year and we really appreciate your time and effort! |
|
On 2024.10.28 03:48:27, Bogdan Ariton wrote:
Hi @aronowski - thank you so much for your replay! (lately we had replies few and far between)
I can provide some help, but only in the domain of my experience - clarifying
the situation may count here, but only in regard to what I know, than jumping
into conclusions. More on that below.
1. Regarding the comparison between our application and the one mentioned in
my previous comment: I understand your answer about the patches, but, since
we've received already an approval for our application, the problem presented
was the fact that our second stage loader is not open source (grabbing one of
the more relevant quotes regarding this issue:
#353 (comment)) - my
point with the submission #385 was
that, from what I could gather, it's the exact same situation and this wasn't
an issue for this specific review. (was just something I wanted to clarify and
learn from)
If those same patches, which code I'm not competent to review, have historically
been approved as part of another application, let me know and point to the
place, where that happened, so I'm sure they are fine. I'll keep that in mind
when re-reviewing the current application.
The discussion was also about the second-stage bootloader's licensing, what I
hope to be clarified in the future - I can't speak for others, but once there's
a consensus and Microsoft gets involved, there should be an official
announcement (e.g. a pinned message).
2. Regarding the submission to Microsoft directly - this is actually something
we're trying to figure out since the entire application was setup to work with
the shim we need to redesign most of our application in order to move forward
with this idea.
Since I can't see the sources, I suppose that it does use the shim_lock protocol
or similar mechanisms, that would need to be ported for the new design. I can't
help with that, but maybe, in the meantime, some clarifications on Microsoft
requirements for such setup could be asked and answered, as to be sure it's OK
for the signing service. Note that this is completely outside of our domain, as
it doesn't involve shim at all.
We've been learning from these submissions throughout the year and we really
appreciate your time and effort!
Hopefully most of the confusing stuff has been clarified, but nevertheless I
know there's still a room for improving the process (working on some
contributions in my free time), but also the Microsoft shim signing requirements
may change with time, alongside the shim community consensuses and requirements,
the most notable being reflected in the README.md changes.
…
--
Reply to this email directly or view it on GitHub:
#353 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
steve-mcintyre
added
Accredited review needed
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)?
#199
#169
#7
The text was updated successfully, but these errors were encountered: