search

rhboot / shim-review

Reviews of shim
66 stars 131 forks source link

Shim 15.7 for 10ZiG Linux #326

Closed ClaudioGranatiero-10zig closed 8 months ago

ClaudioGranatiero-10zig 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/ClaudioGranatiero-10zig/shim-review/tree/10zig-shim-x64-20231120

What is the SHA256 hash of your final SHIM binary?

102a7ba88a13c3bc88cd6d4c30e39d78946c62776779bc228a5d309edb4a84d8

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

N/A

dennis-tseng99 commented 1 year ago
ClaudioGranatiero-10zig commented 1 year ago

Hi Dennis, thanks for your time. Yes, you're right, usually the validity period is shorter, but since Secure Boot does not consider it, I have taken a conservative position. But if that is beyond the specifications, no problem in changing to a more acceptable value.

ClaudioGranatiero-10zig commented 1 year ago

Hi all,

I just changed the certificate (as reported by https://github.com/dennis-tseng99), changing the validity from 100 years to 10 years. The new tag is https://github.com/ClaudioGranatiero-10zig/shim-review/tree/10zig-shim-x64-20230328.

If I understood correctly, we need to provide proof of our PGP identity, so we're waiting for a verification message. Please, let me know if something else is needed to proceed with the review.

Thank you. Claudio

ClaudioGranatiero-10zig commented 1 year ago

Hi all,

please, can someone (@dennis-tseng99, maybe) check our contact references, so we can advance on the review queue? Let me know if something is missing. Thank you.

aronowski commented 1 year ago

If I understand correctly, you're using Debian's GRUB2 implementation as well as the mainline 5.15 kernel with only these tweaks:

- support for a specific type of EMMC present in our hardware
- 10ZiG Boot Logo
- lockdown: also lock down previous kgdb use (eadb2f47a3ced5c64b23b90fd2a3463f63726066)

If this is correct, then I'd like to remind that they need to have NX support as issue #307 says.

Debian shall have this in their GRUB2 builds soon so keep an eye out for it, but the mainline kernel 5.15 will need to have an additional patch.

Use the IMAGE_DLL_CHARACTERISTICS_NX_COMPAT definition (or hardcode 0x100) in arch/x86/boot/header.S instead of the 0 in the line with the # DllCharacteristics comment and that should do it.
Reference is here.

Also, I know that there is indeed a demand for the Debian's GRUB2 implementation to support NX and just FYI, I've been asking around, what can be done to speed up the process.

ClaudioGranatiero-10zig commented 1 year ago

Thank you @aronowski! Yes, you are correct, these are our patch at the moment. Surely I'll take a look at kernel for implement NX_COMPAT (I was thinking it was already in the mainline). As soon as Debian would support NX in GRUB2, I'll give a try.

ClaudioGranatiero-10zig commented 1 year ago

Forced NX_COMPAT in kernel 5.15.39:

objdump -x kernel/linux-5.15.39/arch/x86/boot/bzImage |grep DllCharacteristics
DllCharacteristics  00000100

Updated README.md and referenced tag. Thanks again @aronowski !

ClaudioGranatiero-10zig commented 1 year ago

@aronowski , do you think a binary-modified GRUB is a viable solution in the meantime? I've found a way to change the DllCharacteristics of GRUB2 from the extern (with this program: https://blog.didierstevens.com/2010/10/17/setdllcharacteristics/). So I took the GRUB2 binary extracted from Debian Bookworm and executed setdllcharacteristics +n on that.

./setdllcharacteristics +n grubx64.efi
Original DLLCHARACTERISTICS = 0x0000
 DYNAMIC_BASE    = 0
 NX_COMPAT       = 0
 FORCE_INTEGRITY = 0
Updated  DLLCHARACTERISTICS = 0x0100
 DYNAMIC_BASE    = 0
 NX_COMPAT       = 1
 FORCE_INTEGRITY = 0

root@RPOS165:/opt/Linux-16/src/SecureBoot/shim# objdump -x grubx64.efi |grep DllChar
DllCharacteristics  00000100

Do you (or any other reviewer) think that this is acceptable as a temporary solution until Debian implements it natively?

julian-klode commented 1 year ago

Both the kernel and grub need very substantial patch sets to enable NX support.

ClaudioGranatiero-10zig commented 1 year ago

@julian-klode , thanks to comment. So, if I understand, it's not so simple as changing the DllCharacteristics? Can you please give me some hints on where can I found some informations about all this and go further on?

aronowski commented 1 year ago

@ClaudioGranatiero-10zig I think it's best to just wait for Debian's implementation as they know the best, how it should look like from their side, as well as wait for the review's approval.

Debian got their shims signed 2 months ago so the conclusion is that the support needs to be promised to be added in the future.

Or, if that's not acceptable, a permission needs to be obtained from Microsoft like Oracle got in their reviews (1, 2).

THS-on commented 1 year ago

You might run into issues with your embedded certificate because:

Also as already mentioned the validity is rather long. While no component to my knowledge enforces the time validation, can you limit it 20-30 years, which seems to be common.

ClaudioGranatiero-10zig commented 1 year ago

Hi @THS-on, thanks for taking the time to look at our request. As for you concern about validity, maybe you were looking at the original commit. After an analog observation from @dennis-tseng99 in March we've changed the duration to 10 years, as for https://github.com/ClaudioGranatiero-10zig/shim-review/tree/10zig-shim-x64-20230328. The latest tag https://github.com/ClaudioGranatiero-10zig/shim-review/tree/10zig-shim-x64-20230427 should contain the new certificate. Concerning the other two observations:

Thanks.

THS-on commented 1 year ago

@ClaudioGranatiero-10zig indeed the new certificate has a lifetime of 10 years.

Regarding CA certificates) What you refer to are EV certificates. Those are used to prove that your legal entity exists, e.g. for singing software. You'll need one for submitting the shim to MS. A CA certificate can be used to issue different certificates that then in return can be used for signing the components (e.g. shim, GRUB, kernel). The advantage of that approach is that you can revoke or retire certificates without needing to change the CA certificate embedded in the shim.

How did you generate the certificates? Here is an example OpenSSL configuration that sets the attributes:

[ req ]
default_bits           = 2048
digest                 = sha256
distinguished_name     = req_distinguished_name
days                   = 3650
prompt                 = no
copy_extensions        = copyall

[ req_distinguished_name ]
CN                     = TEST code singing cert

[ cert_ext ]
subjectKeyIdentifier   = hash
authorityKeyIdentifier = keyid:always,issuer:always
keyUsage               = Digital Signature
extendedKeyUsage       = codeSigning

Now to generate an test key and certificate which is self-signed from that you can use:

openssl req -newkey rsa:2048 -nodes -keyout key.pem -x509 -outform DER  -out simple.crt  -extensions cert_ext -config /path/to/conf.cnf

I've tested a shim + GRUB2 using such a certificate using QEMU and EDK2 and it seems to work fine.

ClaudioGranatiero-10zig commented 1 year ago

@THS-on thank you for your exposition on certificates, I'm quite new to all this, but I'm trying to learn. I'll try to recap everything: some months are passed since I worked on all this, so I'll need some time to realloc my current tasks and get back with my mind on this. I'll do some tests as you suggest, and as soon as I have something new, I'll post it here. Thanks again.

By the way, I'm trying to arrange to be in Brusselles for Fosdem in February, hope to see some of you guys there.

ClaudioGranatiero-10zig commented 1 year ago

Please @THS-on (or everyone else reviewing), take a look at https://github.com/ClaudioGranatiero-10zig/shim-review/tree/10zig-shim-x64-20231006, where I updated the certificate as suggested. Thanks for your help.

THS-on commented 1 year ago

@ClaudioGranatiero-10zig thanks for updating the certificate.

Doing an initial look at the submission, can you explain in more detail why you cannot use the shim + GRUB2 + kernel from another distribution?

ClaudioGranatiero-10zig commented 1 year ago

Sure: the reasons for a custom kernel are mainly three:

  1. we need to include some patch for some specific hardware on legacy products;
  2. we are currently using a simple "rootfs in RAM" mechanism, and so we cannot just add every possible module in the rootfs (it will fill the RAM). Hopefully we would change to a more usual and modern "root on physical device" sooner or later, but for now we are stuck on this.
  3. we have two main line of products: the appliance-type (thin client on proprietary hardware) and the generic "install-everywhere" software product (derived from the other one), so we need the best (or the worst, you can say) of the two worlds. For the later (the software solution) we need a generic Secure Boot solution able to install on every customer's hardware, for the first we need custom patches. Sure, we can split the development in two different solutions, but we choose to maintain a single production line for both.
THS-on commented 1 year ago

Yeah I think point 3 is the main thing here. There is a trade-off between splitting the products in two and maintaining your own builds of shim, grub and kernel.

ClaudioGranatiero-10zig commented 1 year ago

Please @THS-on, there's something else I need to answer? I see the tag "question" is still there...

THS-on commented 1 year ago

For now that's all. I'll try to do a full review in the next couple of days.

THS-on commented 1 year ago

Review for 10zig-shim-x64-20231006

OK

Questions and issues

Small note that forcing the NX flag on the kernel is likely not sufficient alone. Newer kernels (>=6.2) should have all the required patches.

ClaudioGranatiero-10zig commented 1 year ago

Hi @THS-on, sorry for the inconvenience regarding the cert's validity, I missed the "-days" parameter on generating the new cert... Amended on the new commit.

Regarding GRUB: as you suggested, I downloaded the package 2.06-13+deb12u1 from debian repositories (http://security.debian.org/debian-security/pool/updates/main/g/grub2/grub-efi-amd64-bin_2.06-13+deb12u1_amd64.deb ): I extracted and signed directly the monolithic binary as is, no need to change modules or recompile, just the binary. I add this to git. The SBAT is the original one, but here it is:

grubx64.efi: file format pei-x86-64

Contents of section .sbat:
 3ff000 73626174 2c312c53 42415420 56657273  sbat,1,SBAT Vers
 3ff010 696f6e2c 73626174 2c312c68 74747073  ion,sbat,1,https
 3ff020 3a2f2f67 69746875 622e636f 6d2f7268  ://github.com/rh
 3ff030 626f6f74 2f736869 6d2f626c 6f622f6d  boot/shim/blob/m
 3ff040 61696e2f 53424154 2e6d640a 67727562  ain/SBAT.md.grub
 3ff050 2c342c46 72656520 536f6674 77617265  ,4,Free Software
 3ff060 20466f75 6e646174 696f6e2c 67727562   Foundation,grub
 3ff070 2c322e30 362c6874 7470733a 2f2f7777  ,2.06,https://ww
 3ff080 772e676e 752e6f72 672f736f 66747761  w.gnu.org/softwa
 3ff090 72652f67 7275622f 0a677275 622e6465  re/grub/.grub.de
 3ff0a0 6269616e 2c342c44 65626961 6e2c6772  bian,4,Debian,gr
 3ff0b0 7562322c 322e3036 2d31332b 64656231  ub2,2.06-13+deb1
 3ff0c0 3275312c 68747470 733a2f2f 74726163  2u1,https://trac
 3ff0d0 6b65722e 64656269 616e2e6f 72672f70  ker.debian.org/p
 3ff0e0 6b672f67 72756232 0a000000 00000000  kg/grub2........

And this is the shimx64 one:

shimx64.efi:     file format pei-x86-64

Contents of section .sbat:
 d2000 73626174 2c312c53 42415420 56657273  sbat,1,SBAT Vers
 d2010 696f6e2c 73626174 2c312c68 74747073  ion,sbat,1,https
 d2020 3a2f2f67 69746875 622e636f 6d2f7268  ://github.com/rh
 d2030 626f6f74 2f736869 6d2f626c 6f622f6d  boot/shim/blob/m
 d2040 61696e2f 53424154 2e6d640a 7368696d  ain/SBAT.md.shim
 d2050 2c332c55 45464920 7368696d 2c736869  ,3,UEFI shim,shi
 d2060 6d2c312c 68747470 733a2f2f 67697468  m,1,https://gith
 d2070 75622e63 6f6d2f72 68626f6f 742f7368  ub.com/rhboot/sh
 d2080 696d0a73 68696d2e 31307a69 672c312c  im.shim.10zig,1,
 d2090 31305a69 47205465 63686e6f 6c6f6779  10ZiG Technology
 d20a0 2c736869 6d2c3135 2e372c6d 61696c3a  ,shim,15.7,mail:
 d20b0 73656375 7265626f 6f744031 307a6967  secureboot@10zig
 d20c0 2e636f6d 0a                          .com.

As for the kernel source: at the moment we don't have a public repository for sources, but I think that if it's needed we can arrange something. The kernel now is the vanilla 6.3.7 with only the already indicated patches applied.

Please, update your review to the latest 10zig-shim-x64-20231017 tag.

Thank you Thore for your time and patience.

THS-on commented 1 year ago
ClaudioGranatiero-10zig commented 1 year ago

Hi @THS-on, thanks for the review. I added an answer to the question about ephemeral keys in README, I hope it is acceptable. Corrected the certificate, now it's a CA and the validity is 10 years. About publishing the kernel sources: we are working on that, we need some days to have all online (our team is splitted between time zones, so I need to wait the Phoenix collegues). Please, refer to tag 10zig-shim-x64-20231030.

ClaudioGranatiero-10zig commented 1 year ago

I just amended the README.md with the link to our Linux kernel repo (https://github.com/10ZiG-Technology/linux). Please refer to tag 10zig-shim-x64-20231031.

ClaudioGranatiero-10zig commented 1 year ago

@THS-on, there is something else I can do to go further on? Thanks.

THS-on commented 1 year ago

@ClaudioGranatiero-10zig I'll try to take a closer look hopefully this week again. What you can do is to look at the issues with "extra review wanted" and check their submissions (is the shim reproducible, does the SBAT entries look good, where does the GRUB come from, does the certificate match and how are keys managed etc.).

ClaudioGranatiero-10zig commented 1 year ago

@THS-on: message received, I'll try to squeeze some review between my "official" tasks.

THS-on commented 12 months ago

Review of 10zig-shim-x64-20231031

HASH

#25 0.396 102a7ba88a13c3bc88cd6d4c30e39d78946c62776779bc228a5d309edb4a84d8  /build/output/shimx64.efi

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.10zig,1,10ZiG Technology,shim,15.7,mail:secureboot@10zig.com

Question/Notes

ClaudioGranatiero-10zig commented 12 months ago

Thore, thanks for the updated review.

  1. updated README.md with info about SBAT
  2. thank you, I've already tested in my test environment with a leaf certificate generated from our CA, and it's working good.
  3. the .config we released in the public repo is the actual one for our current production line, not yet ready for Secure Boot (as we lack a signed shim). I just tested it on an internal release in our test environment. As soon as the Secure Boot stack is completed, we will add the option in .config
ClaudioGranatiero-10zig commented 12 months ago

For my contact verification:

computerization committing overstatements clouded menus condemning semiprofessional tablespoon ranked burger's

THS-on commented 12 months ago

Resend contact verification for Kevin Greenway keving@10zig.com with new PGP key (1D7E 0F09 AF6C 117F 9914 BFF3 4AFD D3B9 069C D9C2)

ClaudioGranatiero-10zig commented 12 months ago

Kevin's verification:

savers presented trap's authorize spotlessness characteristic heuristics intersession's mystification invoice

ClaudioGranatiero-10zig commented 11 months ago

Please, can some of the peer reviewers take a final look at this issue? We're waiting only for an extra review before we're ready to go... thanks to all for the work already done.

aronowski commented 11 months ago

@ClaudioGranatiero-10zig, I'll try to write a review this week.

Have been struggling with several personal things, but most of them have been resolved and I should have more time for that.

Thank you for the updates and for helping out with peer-reviewing other applications. I appreciate that.

aronowski commented 11 months ago

Review done, checksum matches, seems alright! Accepting.

Thank you for the patience.

ClaudioGranatiero-10zig commented 11 months ago

Thank you all for your effort! I hope to see some of you at FOSDEM in February.

THS-on commented 9 months ago

What is the status of this? Did you get a signed shim back or are you creating a new submission for 15.8?

ClaudioGranatiero-10zig commented 9 months ago

Hi Thore,

not yet uploaded to Microsoft (we're trying to obtain an EV certificate and have a bunch of other priorities). So, I think we'll create a new submission as soon as the EV cert arrives.

ClaudioGranatiero-10zig commented 9 months ago

New submission created here: https://github.com/rhboot/shim-review/issues/376 Should I close this one?

THS-on commented 8 months ago

Yes I'll close this one.