search

rhboot / shim-review

Reviews of shim
65 stars 124 forks source link

shim 15.7 for Certus Software S.R.L. #285

Closed eduardacatrinei closed 6 months ago

eduardacatrinei commented 1 year 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/cssrl/shim-review/tree/CertusSoftware-shim-x64-20231218

What is the SHA256 hash of your final SHIM binary?

d0178270f72fc6070fe1e19cc1d66b65c1386fdc6bb7c80fa094cee3a3532db7 shimx64.efi

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

N/A

eduardacatrinei commented 1 year ago

Change grub2 version 2.06 (upstream) to 2.06-3 (Debian). https://github.com/cssrl/shim-review/releases/tag/CertusSoftware-shim-x64-20221101

frozencemetery commented 1 year ago

The main reason for this is to easier the effort of Certus Erasure user when booting the software products on machines where he needs to securely remove the data from the attached storage devices. The most affected users are the technicians working with the ITAD companies which are required to remove the data stored on a great amount of machines, on a daily basis.

This doesn't sound like you're doing anything requiring customization of the boot stack. Could you just reuse the signed shim+grub2+kernel from another distro (in your case, probably Debian's)?

eduardacatrinei commented 1 year ago

We can not use a kernel signed from another distro because we require our custom kernel config. In our custom kernel config we enabled drivers for some very old storage controllers that I have not found in other distros that we considered for reusing.

frozencemetery commented 1 year ago

Can you be more specific? What drivers/controllers?

eduardacatrinei commented 1 year ago

Can you be more specific? What drivers/controllers?

The need for kernel customization is caused by:

A. The need for hardware support

All Certus Erasure products are required to boot and run on a very broad range of hardware, both old and very new alike. The support for the old hardware is usually missing from the most of the existing boot stacks and it requires to be added. The newest hardware is usually added but with consistent delays. The below list contains a few examples of what is needed and it is not included on Debian distro's kernel:

A.1. Storage

A.2. Peripherals

A.3. TPM

A.4. Network

B. The need for AUFS support

We have also a dependency on AUFS. We've planned to migrate to OVERLAYFS in the future, but for the moment we need to manage what we have and the modern distros doesn't support this anymore.

Also, being the first review, I created a new tag in which I updated shim to 15.7 and grub to 2.06-5. https://github.com/cssrl/shim-review/releases/tag/CertusSoftware-shim-x64-20221123

eduardacatrinei commented 1 year ago

https://github.com/cssrl/shim-review/tree/CertusSoftware-shim-x64-20221220 Address binutils issue rhboot/shim#533.

eduardacatrinei commented 1 year ago

https://github.com/cssrl/shim-review/tree/CertusSoftware-shim-x64-20230119 Enabled NX support.

cssrl-amp commented 1 year ago

Can you be more specific? What drivers/controllers?

Hi Robbie (@frozencemetery), Would it be possible to have a look at the info provided by Eduard (@eduardacatrinei)? Is there anything we need to do in order to go ahead? Thank you very much.

frozencemetery commented 1 year ago

Please note https://github.com/rhboot/shim-review/issues/307 (it's not mentioned in your README)

eduardacatrinei commented 1 year ago

Please note #307 (it's not mentioned in your README)

Added missing patch note to readme.

ClaudioGranatiero-10zig commented 1 year ago
steve-mcintyre commented 10 months ago

Apologies for keeping you waiting, catching up on backlog now...

steve-mcintyre commented 10 months ago

Contact verification mails sent

eduardacatrinei commented 10 months ago

Contact verification mails sent

daubers astutest hatchets Triassic genres ambergris extirpated scarves sunfishes Wynn

steve-mcintyre commented 10 months ago

Review of Certus CertusSoftware-shim-x64-20230216

OK

Issues / queries / outstanding

cssrl-amp commented 10 months ago

Contact verification mails sent

Quote: "founts forestalling gobbledygook Kodiak hackney tows clayier juiciness poultry ovoids".

eduardacatrinei commented 10 months ago

@steve-mcintyre

  • shim build matches the binary in the repo, but doesn't match the checksum in README.md, as mentioned by @ClaudioGranatiero-10zig (thanks!). Did you miss updating the README.md file?

Indeed the README.md file was not updated, but now it is. Thanks!

  • You're referencing Debian's GRUB SBAT data, but with an old version - please bump to the current value of "4" rather than "1"

I've changed the value in the README.md file. Thanks.

  • Could you point us at your kernel sources please? Do you have any lockdown patches (etc.) applied?

The upstream kernel sources are used and the below lockdown patches from Debian.

arm64-add-kernel-config-option-to-lock-down-when.patch
efi-add-an-efi_secure_boot-flag-to-indicate-secure-b.patch
efi-lock-down-the-kernel-if-booted-in-secure-boot-mo.patch
mtd-disable-slram-and-phram-when-locked-down.patch

There are also AUFS patches applied (https://github.com/sfjro/aufs-standalone).

  • Your list of grub modules looks very short, especially if you've borrowed from Debian. Could you confirm your list please?

The list of GRUB modules was borrowed from Slackware, a while back. If you consider that there is any issue with the existing or missing module, we are open to address it. Thanks.

eduardacatrinei commented 10 months ago

@steve-mcintyre What further actions are necessary for approval? Thank you for your assistance.

THS-on commented 9 months ago

Review of CertusSoftware-shim-x64-20230906

Hashes

#23 [19/20] RUN sha256sum /shimx64.efi /shim/shimx64.efi
#23 0.625 547e7129ddf97a16a5ce03f50b5577280b070fb2a26f597e11235be934157fcd  /shimx64.efi
#23 0.635 547e7129ddf97a16a5ce03f50b5577280b070fb2a26f597e11235be934157fcd  /shim/shimx64.efi
#23 DONE 0.7s

SBAT

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.certus,1,Certus Software S.R.L.,shim,15.7,mail:security@certussoftware.ro

Notes/questions

eduardacatrinei commented 8 months ago

@THS-on Thanks for the review.

Notes/questions

  • aufs patches are mentioned in the comments, but not the README.md. Please add them.

Done. Thanks!

  • Debian lockdown patches are used. Because the kernel source is not linked, can you verify that the the kernel option LOCK_DOWN_IN_EFI_SECURE_BOOT is actually set, to enable it?

I confirm that the kernel option CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT is enabled.

  • Please update to the 2.06-13+deb12u1 release with the NTFS patches and upstream SBAT level 4. While the old GRUB2 versions are not blocked yet and your builds seem not to be affected. They will likely blocked at some point.

Updated to the 2.06-13+deb12u1 release (with all patches) and upstream SBAT level 4.

  • We recently added a question how kernel modules are signed (

The kernel module signing facility option CONFIG_MODULE_SIG_FORCE (require modules to be validly signed) is enabled. A unique (ephemeral) key is automatically generated during each build. It is then used to sign the kernel modules and it is deleted at the end of the process. We have included the answer also in the README.md.

THS-on commented 8 months ago

CertusSoftware-shim-x64-20231110 now LGTM. Let's wait for @steve-mcintyre to confirm the contact verification and one more reviewer before accepting.

ClaudioGranatiero-10zig commented 8 months ago

I'm not an authorized reviewer, I'm just trying to help and learn.

sha256:

547e7129ddf97a16a5ce03f50b5577280b070fb2a26f597e11235be934157fcd  shimx64.efi

Build is reproducible, sha256 is confirmed.

Obj Alignment:

shimx64.efi

SectionAlignment    00001000
DllCharacteristics  00000100

Alignement is ok.

DllCharacteristics:

shimx64.efi

            DllCharacteristics:        256         0x100  NX_COMPAT

NX_COMPAT is enabled.

Sections:

shimx64.efi


=== SECTIONS ===

  NAME          RVA      VSZ   RAW_SZ  RAW_PTR  nREL  REL_PTR nLINE LINE_PTR     FLAGS
  "/4"         5000    1d9fc    1da00      400     0        0     0        0  40400040  R-- IDATA
  .text       23000    5e839    5ea00    1de00     0        0     0        0  60300020  R-X CODE
  .reloc      82000        a      200    7c800     0        0     0        0  42100040  R-- IDATA DISCARDABLE
  "/14"       84000       88      200    7ca00     0        0     0        0  c0600040  RW- IDATA
  "/26"       85000       47      200    7cc00     0        0     0        0  40300040  R-- IDATA
  .data       86000    2cef4    2d000    7ce00     0        0     0        0  c0600040  RW- IDATA
  "/37"       b3000      7e6      800    a9e00     0        0     0        0  40300040  R-- IDATA
  .dynamic    b4000      100      200    aa600     0        0     0        0  c0400040  RW- IDATA
  .rela       b5000    1b468    1b600    aa800     0        0     0        0  40400040  R-- IDATA
  .sbat       d1000       d2      200    c5e00     0        0     0        0  40100040  R-- IDATA

Code section is not writable: OK

SBAT:

shimx64.efi


shimx64.efi:     file format pei-x86-64

Contents of section .sbat:
 d1000 73626174 2c312c53 42415420 56657273  sbat,1,SBAT Vers
 d1010 696f6e2c 73626174 2c312c68 74747073  ion,sbat,1,https
 d1020 3a2f2f67 69746875 622e636f 6d2f7268  ://github.com/rh
 d1030 626f6f74 2f736869 6d2f626c 6f622f6d  boot/shim/blob/m
 d1040 61696e2f 53424154 2e6d640a 7368696d  ain/SBAT.md.shim
 d1050 2c332c55 45464920 7368696d 2c736869  ,3,UEFI shim,shi
 d1060 6d2c312c 68747470 733a2f2f 67697468  m,1,https://gith
 d1070 75622e63 6f6d2f72 68626f6f 742f7368  ub.com/rhboot/sh
 d1080 696d0a73 68696d2e 63657274 75732c31  im.shim.certus,1
 d1090 2c436572 74757320 536f6674 77617265  ,Certus Software
 d10a0 20532e52 2e4c2e2c 7368696d 2c31352e   S.R.L.,shim,15.
 d10b0 372c6d61 696c3a73 65637572 69747940  7,mail:security@
 d10c0 63657274 7573736f 66747761 72652e72  certussoftware.r
 d10d0 6f0a                                 o.              

shimx64.efi:     file format pei-x86-64

Contents of section .sbatlevel:
 85000 00000000 08000000 22000000 73626174  ........"...sbat
 85010 2c312c32 30323230 35323430 300a6772  ,1,2022052400.gr
 85020 75622c32 0a007362 61742c31 2c323032  ub,2..sbat,1,202
 85030 32313131 3530300a 7368696d 2c320a67  2111500.shim,2.g
 85040 7275622c 330a00                      rub,3..         
shim.certus,1,Certus Software S.R.L.,shim,15.7,mail:security@certussoftware.ro

SBAT seems ok to me.

Certificate:

Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            0e:fd:01:eb:79:80:a7:c5:63:78:21:6b:4b:b5:05:8f
        Signature Algorithm: sha256WithRSAEncryption
        Issuer: C = US, O = "DigiCert, Inc.", CN = DigiCert Trusted G4 Code Signing RSA4096 SHA384 2021 CA1
        Validity
            Not Before: Oct 12 00:00:00 2022 GMT
            Not After : Oct 11 23:59:59 2025 GMT
        Subject: jurisdictionC = RO, businessCategory = Private Organization, serialNumber = J22/1532/2016, C = RO, L = Iasi, O = Certus Software S.R.L., CN = Certus Software S.R.L.
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                Public-Key: (4096 bit)
                Modulus:
                    00:db:0a:16:52:53:68:a0:a8:a5:ba:8e:13:67:81:
                    a5:b0:39:ef:5a:60:cc:75:b9:7f:30:59:89:77:82:
                    48:21:0c:83:cc:46:1b:83:70:67:16:1f:bb:12:5e:
                    6c:4d:8b:2a:0f:c1:e7:72:74:b3:c1:07:da:ff:39:
                    13:74:c3:d3:70:0f:aa:43:93:03:70:ba:cd:62:89:
                    61:ad:04:84:ee:b3:80:cf:a0:ca:1c:65:fc:54:af:
                    d8:1b:fe:80:34:91:b6:e5:e2:d4:cf:76:d1:9c:9f:
                    81:eb:4c:20:aa:e0:4a:78:1a:a4:6a:a7:b5:1a:c8:
                    21:31:4e:f7:ea:9e:26:10:14:72:01:a0:48:62:c3:

Certificate it's not a CA, but it's an EV certificate signed by DigiCert. Certificate is ok.

aronowski commented 7 months ago

@eduardacatrinei, apologies for the late reply. I'll try to write a review by the end of this week - making your application top priority, as you've been waiting for a very long time.

@steve-mcintyre, pinging you here to confirm the successful contact verification.

aronowski commented 7 months ago

Review done, checksum matches, seems alright!

@THS-on, I just now noticed that Steve added thumbs up emoji reactions to both replies with shim words for contact verification. In this case, what's the thing we should do now?

In the meantime I was thinking about further improvements to the process, to prevent this kind of situation from happening. Something like if a contact verification has been initiated, the official reviewer who initiated it has one week to respond - otherwise another reviewer takes the initiative. I may add this soon to the thread with proposals, but right here I'd like to already accept this application without further waiting - the people behind it have been waiting for a very long time.

eduardacatrinei commented 7 months ago

Thanks, everyone! If the only remaining step is contact verification, we're ready to redo that without any issues.

aronowski commented 7 months ago

Since there's been no activity for over a week, I'll send the verification emails again. Say what you want, but I'd rather double-check that everything is correct*, than treat uncertainties as absolute truths. Especially considering my potential lack of knowledge of certain cultural differences and my personal situation, which makes me extra careful. The lack of sleep and the amount of responsibilities do this to me.

Furthermore, we recently got a newsflash, that the whole bootchain shall be NX-compatible for the shim binary to have the proper bit set, therefore what I'd recommend is to just recompile the binary without the NX support patch, put the appropriate changes to the README, as well as edit the opening post of this GitHub issue (new repository tag and shim binary checksum), and ping me, so I can quickly rebuild it myself and accept the application after such a long time.

Yes, I know how frustrating this might be - I'll try my best to review the updated application ASAP.

* I should write a guide, how to accomplish this too, but let's just say that I confirmed the public keys from 2 independent sources with 2 independent Internet connections/IP addresses.

aronowski commented 7 months ago

Verification emails sent - attempt no. 2.

cssrl-amp commented 7 months ago

Verification emails sent - attempt no. 2.

Thanks @aronowski. Below is the content of the second attempt email verification. morrows padlocks rationality foreswore repartee voice comported ginger tavern cheekbone

eduardacatrinei commented 7 months ago

Verification emails sent - attempt no. 2.

discrediting Cray reprobate wigs prognosticate noblest nurseryman sacrificed antiquarian desist

aronowski commented 7 months ago

@eduardacatrinei, @cssrl-amp, thank you! Contact verification successful.

Once the updated application is present, ping me and I'll review it with high priority. After all, it should be just about the new binary and therefore quick and easy.

In regard to the current NX bit situation, I wrote some proposals here, so that people would be aware of, what's happening and the message being as straightforward as possible in the future.

eduardacatrinei commented 7 months ago

Once the updated application is present, ping me and I'll review it with high priority. After all, it should be just about the new binary and therefore quick and easy.

@aronowski Thank you for your time! https://github.com/cssrl/shim-review/tree/CertusSoftware-shim-x64-20231218

aronowski commented 7 months ago

Review done, checksum matches, new binary seems OK - no NX support, since the whole chain is not fully NX-compatible, i.e. compliant with the requirements (the Microsoft exception) mentioned in PR https://github.com/rhboot/shim-review/pull/359.

Accepting. Thank you for your patience. I'll try my best to make the process more optimal in the future.

eduardacatrinei commented 6 months ago

Thank you! We have received the signed shim from Microsoft.