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

Shim 15.7 for Miray Software #355

Closed
8 tasks done
miray-ar opened this issue Nov 21, 2023 · 11 comments
Closed
8 tasks done

Shim 15.7 for Miray Software #355

miray-ar opened this issue Nov 21, 2023 · 11 comments
Labels
extra review wanted Initial review(s) look good, another review desired

Comments

@miray-ar
Copy link

miray-ar commented Nov 21, 2023

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/MiraySoftware/shim-review/tree/miraysoftware-shim-x64+aa64-20240110


What is the SHA256 hash of your final SHIM binary?


shim_mirayx64.efi: 9618b067b7b43621f5121394bd3cdebfdde9123317c91dafb003420bedbfc1c4

shim_mirayaa64.efi: e9e5843dd4d339fcf797d70f76a05833e7ff7286d980c1ca86d43979d38591ff


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


Last request, superseeded by this request:
#351

Last accepted:
#247

@miray-ar miray-ar changed the title Shim 15.7 for Miray Software (2nd submission) Shim 15.7 for Miray Software Nov 30, 2023
@miray-tf
Copy link

@MuthuvelKuppusamy We added the patch that was missing in the commit in #351.
Could you please review this updated submission.

@aronowski aronowski self-assigned this Dec 19, 2023
@miray-tf
Copy link

@aronowski

Do we need to provide any additional information?

@aronowski
Copy link
Collaborator

@miray-tf, you've already had several shims signed, so it's a great thing, but I'm worried about the evolving requirements and if everything is correct - people, myself included, make mistakes, and it's fairly easy to miss something important, and thorough peer-reviews do help here.

I'll carefully check the sources of the patches you listed, but it'll take some time - I think maybe I'll have the initial review finished by the end of the next week.
I take into consideration the environment and surroundings, where the work will be taking presence, as I've assigned myself to this application last week, but I'm picking it up just now - it's way harder to double-check an application especially for the correctness of the patches, than e.g. write a document with casual thoughts on the improvements of this initiative.

There's at least one thing that I can already notice: recently the NX requirements have changed and most likely you'll need to remove the NX-compatibility patch for Microsoft to sign your binaries - we've had a discussion to make this venue more user-friendly here - I suggested some hints there as well, to prevent confusion.

It's about the complete chain implementing NX support, so until GRUB2 does not implement it, then Shim, Linux and Symobi shall not have the NX compatibility bit set.

I'll perform the shim rebuilding process, once that Microsoft requirement is satisfied - after all, the checksums will change.

@aronowski aronowski added the question Reviewer(s) waiting on response label Dec 29, 2023
@aronowski
Copy link
Collaborator

Reviewing a bit - as much as I could this day.

Here is the diff between the last accepted application and the current one ($ git diff miraysoftware-shim-x64+aa64-20220613 miraysoftware-shim-x64+aa64-20231121 --ignore-space-at-eol -- README.md):

 Shim:
 sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
-shim,2,UEFI shim,shim,1,https://github.com/rhboot/shim
-shim.miray,2,Miray Software,shim,miray-15.6,https://www.miray-software.com
+shim,3,UEFI shim,shim,1,https://github.com/rhboot/shim
+shim.miray,2,Miray Software,shim,miray-15.7,https://www.miray-software.com

Was there any signed shim that had an entry similar to the one below (X being a placeholder)?

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,3,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.miray,1,Miray Software,shim,miray-15.X,https://www.miray-software.com

If yes, then it's alright to have the shim.miray,2 entry in the current README.
If no, then the sbat.miray.csv file should be changed, so the entry says shim.miray,1 rather than shim.miray,2, as you're deriving from upstream after they've bumped their generation number.


I'll need help with the GRUB2 SBAT section. Here's what happened:

The required CVE-related patches have been applied and FSF's entry got changed to grub.4. Furthermore:

Changes for Grub:
- Changed base to fedora-39 and rebased our own patches on top of that branch
- Added 'grub.rh,2,Red Hat,grub2,2.06-104.fc39,mailto:[email protected]' to GRUB Sbat to reflect that change

But here's the curiosity: the Fedora/RHEL section still ships grub.3 as far as I can see. So by bumping FSF's entry, I suppose your SBAT section should look more like:

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.miray,1,Miray Software,grub2,sysload_$SYSLOAD_VERSION$,https://github.com/MiraySoftware/grub2

and, once Fedora/RHEL starts shipping a section like:

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,4,Free Software Foundation,grub,@@VERSION@@,https://www.gnu.org/software/grub/
grub.rh,1,Red Hat,grub2,@@VERSION_RELEASE@@,mailto:[email protected]

, then it will be alright to derive from that exact section, so then yours would be similar to:

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,4,Free Software Foundation,grub,@@VERSION@@,https://www.gnu.org/software/grub/
grub.rh,1,Red Hat,grub2,@@VERSION_RELEASE@@,mailto:[email protected]
grub.miray,1,Miray Software,grub2,sysload_$SYSLOAD_VERSION$,https://github.com/MiraySoftware/grub2

so the security generation numbers chain is alright. If I'm wrong here, please point out the errors.

Furthermore, I'm aware of the hints in this application section, i.e.:

Where your code is only slightly modified from an upstream vendor's, please also preserve their SBAT entries to simplify revocation.

If you are using a downstream implementation of GRUB2 (e.g. from Fedora or Debian), please
preserve the SBAT entry from those distributions and only append your own.

and where the confusion might be coming from - yes, this can be improved and I'd welcome suggestions, on how to write the hints more clearly, with open arms. We also have a thread for them here.


  • Shim patches should be alright apart from the NX-related one due to the changes of Microsoft requirements. After all, they have been also provided in an earlier application, which got accepted.
  • I won't be able to review Symobi, if all the new Microsoft requirements are satisfied, but it's been accepted earlier, so I suppose things should be fine here.
  • I haven't yet taken a look at the patches, but I suppose I could clone the rhboot/grub2.git repository, and checkout the commit 4bcf6f747c3ab0b998c6f5a361804e38bc9c4334 to act as the source of truth when comparing your fork at the sysload_2_8 branch? Is this correct?

@miray-tf
Copy link

miray-tf commented Jan 3, 2024

Thank you for your comments

sysload_2_8 is the correct branch from , we branched it from fedora-39 at commit 10f8ffc133553209ec1ddaadc6f4a8a25d3dea4e "Fix missing #include in ofdisk.c" Which was the latest commit at that time.


Regarding the SBAT numbers in Shim:
While bumping the SBAT number for shim.miray originally was a mistake, the SBAT numbers in a program should never go down:
Image a program A with vendor X. Initially it would have the SBAT entries

A,1
A.X,1

If A.X,1 gets revoked then a revocation entry will be added for it. (Update 1)
After fixing the bug the new SBAT entries in the program will be

A,1
A.X,2

Now if A,1 gets revoked then a revocation entry for A,1 will be added. (Update 2)

If the SBAT entries where to be updated to

A,2
A.X,1

Then this program would be considered revoked on all systems still on update 1.
So the SBAT entries would need to be

A,2
A.X,2

I think we should keep the 'grub.rh' entry in the SBAT section of our Grub:
If there ever is a bug in the Fedora/RedHat-patches that makes a revocation necessary then all Grub-variants derived from it are probably affected as well. All Grub-variants that have the grub.rh entry in the SBAT section will then get revoked together with the upstream Fedora/RedHat Grub.
Leaving the entry out would mean that our Grub would need its own revocation entry for grub.miray for the same revocation event.
The request to preserve SBAT-Entries was added to avoid such situations and also to keep the revocation information as small as possible.


For updating the Shim without the NX-patch:
Do you prefer if we update this request? Or should we close it and add open a new one?

@aronowski
Copy link
Collaborator

sysload_2_8 is the correct branch from , we branched it from fedora-39 at commit 10f8ffc133553209ec1ddaadc6f4a8a25d3dea4e "Fix missing #include in ofdisk.c" Which was the latest commit at that time.

Thank you. I'll check it out soon - things have been tough for me the last two days.


the SBAT numbers in a program should never go down

The SBAT-related environment may be confusing, so I'll quote some parts of the long SBAT.md document - the current revision. Then, I'll apply them to the current application's case.

A product-specific generation number is needed if a CVE is fixed in code that **only** exists in a specific product's branch. This would either be something like product-specific patches, or a mis-merge that only occurred in that product. Setting a product-specific generation number for such an event eliminates the need for other vendors to have to re-release the binaries for their products with an incremented global number.

However, once the global number is bumped for the next upstream CVE fix there
will be no further need to carry that product-specific generation number.
Satisfying the check of the global number will also exclude any of the older
product-specific binaries.

For example: There is a global CVE disclosure and all vendors coordinate to
release fixed components on the disclosure date. This release bumps the global
generation number for GRUB to 4.

SBAT revocation data would then require a GRUB with a global generation number
of 4.

However, Vendor C mis-merges the patches into one of their products and does
not become aware of the fact that this mis-merge created an additional
vulnerability until after they have published a signed binary in that,
vulnerable, state.

Vendor C's GRUB binary can now be used to compromise anyone's system.

To remedy this, Vendor C will release a fixed binary with the same global
generation number and the product-specific generation number set to 1.

SBAT revocation data would then require a GRUB with a global generation number
of 4, as well as a product-specific generation number of 1 for the product that
had the vulnerable binary.

If and when there is another upstream fix for a CVE that would bump the global
number, this product-specific number can be dropped from the UEFI revocation
variable.

If this same Vendor C has a similar event after the global number is
incremented, they would again set their product-specific or version-specific
number
to 1. If they have a second event on the same component, they would
set their product-specific or version-specific number to 2.

In such an event, a vendor would set the product-specific or version-specific
generation number based on whether the mis-merge occurred in all of their
branches or in just a subset of them. The goal is generally to limit end
customer impact with as few re-releases as possible, while not creating an
unnecessarily large UEFI revocation variable payload.

prior to
disclosure*
after
disclosure
after Vendor C's
first update
after Vendor C's
second update
after next global
disclosure
GRUB global
generation number in
artifacts .sbat section
3 4 4 4 5
Vendor C's product-specific
generation number in artifact's
.sbat section
1 1 2 3 1
GRUB global
generation number in
UEFI SBAT revocation variable
3 4 4 4 5
Vendor C's product-specific
generation number in
UEFI SBAT revocation variable
not set not set 2 3 not set

So, while shim.miray,2 at the following point

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,2,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.miray,2,Miray Software,shim,miray-15.6,https://www.miray-software.com

might have been just an error and it stayed like this for historical reasons, once shim,2 is denylisted (due to what the quoted example represents as disclosure) and shim,3 gets to the UEFI SBAT revocation variable (i.e. shim,3 being the minimum required number) the downstream shim.miray entry shall start from being shim.miray,1 again, as shim.miray,2 is no longer in the UEFI SBAT revocation variable, i.e. all shim.miray entries are allowed.

This is analogical to the quoted example, where the downstream Vendor C had their grub.vendorc,3 entry and once grub.4 gets denied, grub.5 is the minimal required, and therefore Vendor C's entry starts from grub.vendorc,1 again (grub.vendorc,3 is no longer denylisted).

I'll explain it also with the Product A example from the recent comment.

Image a program A with vendor X. Initially it would have the SBAT entries

A,1
A.X,1

That's OK.

If A.X,1 gets revoked then a revocation entry will be added for it. (Update 1)
After fixing the bug the new SBAT entries in the program will be

A,1
A.X,2

Then Update 1 adds A.X,2 to the UEFI SBAT revocation variable and these SBAT entries I just quoted are OK.

Now if A,1 gets revoked then a revocation entry for A,1 will be added. (Update 2)

Then Update 2 adds A,2 to the UEFI SBAT revocation variable and removes all entries downstream from Product A from the UEFI SBAT revocation variable: A.X,2 gets removed from the UEFI SBAT revocation variable, i.e. this is allowed:

A,2
A.X,1

and that's how Vendor X's Product A's .sbat section should look like once Vendor X applies all the required patches that satisfy the requirement of security generation 2 of the upstream Product A.

Confusing? Hard to grasp? Yes, I agree - I'll think of making this more inclusive in the future.


If there ever is a bug in the Fedora/RedHat-patches that makes a revocation necessary then all Grub-variants derived from it are probably affected as well. All Grub-variants that have the grub.rh entry in the SBAT section will then get revoked together with the upstream Fedora/RedHat Grub.
Leaving the entry out would mean that our Grub would need its own revocation entry for grub.miray for the same revocation event.

I'm aware of the hassle, which might arise from the need to maintain, as quoted, own revocation entry for grub.miray. That said, I'm still convinced that there might be a necessity for this: while this work is based on the patches from Fedora/RHEL, your GRUB2 is based on the security generation grub,4, rather than the generations:

grub,3
grub.rh,2

so I wouldn't be surprised if the demand from Microsoft was to (as long as Fedora/RHEL does not update their products to use grub.4) update grub.miray each time a grub.rh generation number should be updated (not counting internal Miray customizations). Furthermore,

The request to preserve SBAT-Entries was added to avoid such situations and also to keep the revocation information as small as possible.

this is understandable, but I've talked about this with Robbie, when they still were revieving the applications, from this point - while the idea of simplification is alright, this will mostly not work out.


For updating the Shim without the NX-patch:
Do you prefer if we update this request? Or should we close it and add open a new one?

I think it's OK to update this one. Once the NX support patch is no longer here, I'll build the shim binary and report back.

@miray-tf
Copy link

miray-tf commented Jan 5, 2024

Ok, we will drop the grub.rh line.

shim.miray and grub.miray were not used for revocation. So I think it would be possible to reset them both to 1.
But I would like to discuss that point again before doing that:

Regarding the SBAT.md file.
You quoted part of the 'Generation-Based Revocation Scenarios' which supports your point to reset the vendor generation numers.
But the SBAT.md file contradicts itself at the start of the 'Someone was wrong on the Internet and bug 2' example. There the grub generation number is bumped from 2 to 3, but the grub.debian generation number is kept at 2.
One of those two sections is wrong and should be corrected.


I'll come back to what I wanted to explain with my example:

UEFI revocation update 1:
Adds revocation for A.X,1

UEFI revocation update 2:
Adds revocation for A,1 and drops the previous revocation for A.X,1

Now, if we have a binary with
A,2
A.X,1

On a system with update 2 this is accepted, because 'A,2' is bigger than the revoked 'A,1'

On a system with has not been updated and still has the the older update 1 this binary will be rejected, because the 'A.X,1' entry in the binaries SBAT section will match the revocation entry 'A.X,1' from update 1.

@aronowski
Copy link
Collaborator

Ok, we will drop the grub.rh line.

Alright, thank you! 👍

If my reasoning turns out to be wrong, I'll request more clarifications on this matter - documenting some common scenarios, when the downstream entries should always remain, may optionally remain, and when they should be removed.


Regarding the SBAT.md file.
You quoted part of the 'Generation-Based Revocation Scenarios' which supports your point to reset the vendor generation numers.
But the SBAT.md file contradicts itself at the start of the 'Someone was wrong on the Internet and bug 2' example. There the grub generation number is bumped from 2 to 3, but the grub.debian generation number is kept at 2.
One of those two sections is wrong and should be corrected.

The table I quoted shall be the correct answer, as I did some cleanups to this file and running git blame SBAT.md shows that I myself touched that section on April 21, 2023, which got accepted to be included in the main branch.

I'll also show, how this table used to look like in the past, compared to the current revision. Let's run git diff c98a10c519e78c3c7abf727cf89dcbbf05fa1f4a dbbe3c84bd0e7683d4b81c1794a112a6853b80ee -- SBAT.md and look just at that table's diff:

-|                                                                                      | prior to<br>disclosure | after<br>disclosure | after Vendor C's<br>first update | after Vendor C's<br>second update | after next global<br>disclosure |
-|--------------------------------------------------------------------------------------|------------------------|---------------------|----------------------------------|----------------------------------|---------------------------------|
-| GRUB global<br>generation number in<br>artifacts .sbat section                       | 3                      | 4                   | 4                                | 4                                | 5                               |
-| Vendor C's product-specific<br>generation number in artifact's<br>.sbat section      | 1                      | 1                   | 5                                | 6                                | 1                               |
-| GRUB global<br>generation number in<br>UEFI SBAT revocation variable                 | 3                      | 4                   | 4                                | 4                                | 5                               |
-| Vendor C's product-specific<br>generation number in<br>UEFI SBAT revocation variable | not set                | not set             | 5                                | 6                                | not set                         |
+|                                                                                      | prior to<br>disclosure\* | after<br>disclosure | after Vendor C's<br>first update | after Vendor C's<br>second update | after next global<br>disclosure |
+|--------------------------------------------------------------------------------------|--------------------------|---------------------|----------------------------------|-----------------------------------|---------------------------------|
+| GRUB global<br>generation number in<br>artifacts .sbat section                       | 3                        | 4                   | 4                                | 4                                 | 5                               |
+| Vendor C's product-specific<br>generation number in artifact's<br>.sbat section      | 1                        | 1                   | 2                                | 3                                 | 1                               |
+| GRUB global<br>generation number in<br>UEFI SBAT revocation variable                 | 3                        | 4                   | 4                                | 4                                 | 5                               |
+| Vendor C's product-specific<br>generation number in<br>UEFI SBAT revocation variable | not set                  | not set             | 2                                | 3                                 | not set                         |
+
+\* A disclosure is the event/date where a CVE and fixes for it are made public.

It used to be "wrong" too - or maybe that's how the SBAT mechanism was supposed to behave at that time, when I was not involved in this initiative. I'd say the text from the early 2021 shall be rewritten and adapted to what is happening today.

Why wasn't this fixed earlier? Because not only was I learning the practical part on my own, but also because contributions were being verified thoroughly and therefore slowly, even some that I made still haven't made it into this project.

And this does not account for the guidelines like these:

I see a worrying tendency from the new reviewers to try to be helpful and streamline the submission process. This is counter productive. The questions must be vague enough that it is not clear what the right answer is, otherwise we cannot judge if they really know what they're doing.

The checklists we have are very basic and only a part of establishing that this is a trust-able vendor. You also need to rely on your instincts and feelings, and not just accept something that looks sketchy because it ticks all the boxes in the checklist.

You need to assume that every new vendor is just here to p0wn systems and you are the last line of defense.

And I would say this guideline is satisfied right now, as lots of mistakes are being made, and individuals have to basically reverse-engineer the whole initiative before posting an application.

Furthermore, continuing on this thread,

shim.miray and grub.miray were not used for revocation. So I think it would be possible to reset them both to 1.
But I would like to discuss that point again before doing that

Even though the possibility should be there (I'll explain it even more later on - see the next section of this comment), I'd like to discuss that too, especially with the SBAT.md document authors, and also about preserving-or-not-preserving upstream entries, like the grub.rh one I was writing about in the beginning of this comment. I can try pinging @vathpela here - if there's no answer, I'm blaming only the managers for orchestrating employees' work like this.


I'll come back to what I wanted to explain with my example:

UEFI revocation update 1:
Adds revocation for A.X,1

UEFI revocation update 2:
Adds revocation for A,1 and drops the previous revocation for A.X,1

Now, if we have a binary with
A,2
A.X,1

On a system with update 2 this is accepted, because 'A,2' is bigger than the revoked 'A,1'

On a system with has not been updated and still has the the older update 1 this binary will be rejected, because the 'A.X,1' entry in the binaries SBAT section will match the revocation entry 'A.X,1' from update 1.

I understand, but my answer was referring to what's happening with shim.miray not being listed as a separate product in the SbatLevel variable. If it was, then it would be understandable to do something like:

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
shim,3,UEFI shim,shim,1,https://github.com/rhboot/shim
shim.miray,2,Miray Software,shim,miray-15.7,https://www.miray-software.com

Reminder - in here shim and shim.miray are treated as distinct products. I don't know, why your shim implementation should even land in this variable in the first place (you do not add patches, that would make it vulnerable to the extent of granting an entry in this variable, do you?), but let's keep the examples going further on.

For instance, let's take a look at the current Debian implementation. @steve-mcintyre added grub.debian as a separate product to the revocation variable:

$ git blame include/sbat_var_defs.h | grep 'Steve McIntyre'
cca3933f (Steve McIntyre 2023-01-30 18:15:36 +0000 38) /*
cca3933f (Steve McIntyre 2023-01-30 18:15:36 +0000 39)  * Debian's grub.3 update was broken - some binaries included the SBAT
cca3933f (Steve McIntyre 2023-01-30 18:15:36 +0000 40)  * data update but not the security patches :-(
cca3933f (Steve McIntyre 2023-01-30 18:15:36 +0000 41)  */
cca3933f (Steve McIntyre 2023-01-30 18:15:36 +0000 42) #define SBAT_VAR_LATEST_DATE "2023012900"
cca3933f (Steve McIntyre 2023-01-30 18:15:36 +0000 43) #define SBAT_VAR_LATEST_REVOCATIONS "shim,2\ngrub,3\ngrub.debian,4\n"

And the current implementation contains:

sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
grub,4,Free Software Foundation,grub,@UPSTREAM_VERSION@,https://www.gnu.org/software/grub/
grub.debian,4,Debian,grub2,@DEB_VERSION@,https://tracker.debian.org/pkg/grub2
[...]

Reminder: grub and grub.debian are treated as distinct products. And the rest should behave like in the table I quoted and could get git-blamed about.

If you have some ideas of making this more accessible, like a flowchart or something, feel free to suggest it.

@miray-tf
Copy link

We added the new tag miraysoftware-shim-x64+aa64-20240110 (71b43de) with the suggested changes.
The review request is updated to reflect the changes.

@aronowski aronowski removed the question Reviewer(s) waiting on response label Jan 11, 2024
@aronowski
Copy link
Collaborator

I'm analyzing the GRUB2 patches and they seem fine at first glance.

You've been maintaining the whole bootchain for some time and had shims signed, so there's a lot of trust, that these tweaks were carefully picked, e.g. the replacement of grub_efi_secure_boot with upstream grub lockdown verification.

Also amazed how you ported the upstream (7-Zip) LZMA implementation from the end of 2021.


Build reproduces, checksums match.

Checked both binaries, .sbat and .sbatlevel seem fine.


LGTM! Please ping another official reviewer for a further review.

@aronowski aronowski added the extra review wanted Initial review(s) look good, another review desired label Jan 18, 2024
@aronowski aronowski removed their assignment Jan 18, 2024
@miray-ar
Copy link
Author

miray-ar commented Feb 2, 2024

Superseded by #367

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extra review wanted Initial review(s) look good, another review desired
Projects
None yet
Development

No branches or pull requests

3 participants