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.

Sign up for GitHub

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

Jump to bottom

Shim 15.8 for FortiOS #356

Closed
8 tasks done
fgtvm opened this issue Nov 24, 2023 · 19 comments
Closed
8 tasks done

Shim 15.8 for FortiOS #356

fgtvm opened this issue Nov 24, 2023 · 19 comments
Labels
accepted Submission is ready for sysdev new vendor This is a new vendor

Comments

@fgtvm
Copy link

fgtvm commented Nov 24, 2023 •
edited
Loading

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

  • completed README.md file with the necessary information
  • shim.efi to be signed
  • public portion of your certificate(s) embedded in shim (the file passed to VENDOR_CERT_FILE)
  • binaries, for which hashes are added to vendor_db ( if you use vendor_db and have hashes allow-listed )
  • any extra patches to shim via your own git tree or as files
  • any extra patches to grub via your own git tree or as files
  • build logs
  • a Dockerfile to reproduce the build of the provided shim EFI binaries

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

https://github.com/fortinet/shim-review/tree/fortinet-shim-x86_64_aarch64-20240208

What is the SHA256 hash of your final SHIM binary?

eb7f324221e23f94fa92193c495b35ed4bde274aab9c0f761ec9c0c37c9f90b0 shimx64.efi 0d25eecddf7306bff58f9739194bccca0a94d4f7bd7cb5d6097228a9fe4caf60 shimaa64.efi

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

N/A
@THS-on THS-on added new vendor This is a new vendor contact verification needed Contact verification is needed for this review labels Nov 28, 2023
@Blarse
Copy link

Blarse commented Dec 1, 2023

I'm not an authorized reviewer, but I'd like to contribute and help
@frozencemetery @steve-mcintyre @julian-klode:

Build reproducibility

  • Reproducible according to Dockfile: OK
aarch64-linux-gnu-objcopy -D -j .text -j .sdata -j .data \
	-j .dynamic -j .rodata -j .rel* \
	-j .rela* -j .dyn -j .reloc -j .eh_frame -j .sbat \
	-j .sbatlevel \
	-j .debug_info -j .debug_abbrev -j .debug_aranges \
	-j .debug_line -j .debug_str -j .debug_ranges \
	-j .note.gnu.build-id \
	fbaa64.so fbaa64.efi.debug
aarch64-linux-gnu-objcopy -D -j .text -j .sdata -j .data -j .data.ident \
	-j .dynamic -j .rodata -j .rel* \
	-j .rela* -j .dyn -j .reloc -j .eh_frame \
	-j .vendor_cert -j .sbat -j .sbatlevel \
	--target efi-app-aarch64 mmaa64.so mmaa64.efi
./post-process-pe -vv mmaa64.efi
aarch64-linux-gnu-objcopy -D -j .text -j .sdata -j .data -j .data.ident \
	-j .dynamic -j .rodata -j .rel* \
	-j .rela* -j .dyn -j .reloc -j .eh_frame \
	-j .vendor_cert -j .sbat -j .sbatlevel \
	--target efi-app-aarch64 fbaa64.so fbaa64.efi
./post-process-pe -vv fbaa64.efi
make: Leaving directory '/build/shim/shim-15.7/build-aarch64'
--> 03ff16493da5
STEP 16/17: RUN sha256sum build-x86_64/shimx64.efi
476c554e0c06c419083f2868bd58e2e9c3774a763f4f37b935b3537f450843d2  build-x86_64/shimx64.efi
--> 237b68bc7ba0
STEP 17/17: RUN sha256sum build-aarch64/shimaa64.efi
ed64851489b8c77a54a79aba5f9969cfffaf3f47455008f11511c45225c69ea5  build-aarch64/shimaa64.efi
COMMIT fortinet-shim-x86_64_aarch64-20231123:latest
--> 18c729976992
Successfully tagged localhost/fortinet-shim-x86_64_aarch64-20231123:latest
18c72997699209f13050b35073fad1a908a42b8db05d1360a2088897e1e380f8
  • Hash is matched for shimx64.efi and shimaa64.efi OK
$podman run -it --rm fortinet-shim-x86_64_aarch64-20231123:latest sha256sum build-x86_64/shimx64.efi build-aarch64/shimaa64.efi
476c554e0c06c419083f2868bd58e2e9c3774a763f4f37b935b3537f450843d2  build-x86_64/shimx64.efi
ed64851489b8c77a54a79aba5f9969cfffaf3f47455008f11511c45225c69ea5  build-aarch64/shimaa64.efi

Shim source

Certificates

  • The certificate matches the organization; OK
  • The keys are stored on HSM with restricted access. OK
  • Embedded cert is valid until 2056 (32 years); OK
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 0 (0x0)
        Signature Algorithm: sha256WithRSAEncryption
        Issuer: C = US, ST = California, L = Sunnyvale, O = Fortinet, OU = Certificate Authority, CN = fortinet-ca2, emailAddress = [email protected]
        Validity
            Not Before: Jun  6 20:27:39 2016 GMT
            Not After : May 27 20:27:39 2056 GMT
        Subject: C = US, ST = California, L = Sunnyvale, O = Fortinet, OU = Certificate Authority, CN = fortinet-ca2, emailAddress = [email protected]

SBAT

  • SBAT seems okay for shimx64.efi: OK
$objdump -j .sbat -s ./build-x86_64/shimx64.efi

./build-x86_64/shimx64.efi:     file format pei-x86-64

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 666f7274 696f732c  im.shim.fortios,
 d5090 312c466f 7274696e 65742c73 68696d2c  1,Fortinet,shim,
 d50a0 31352e37 2c687474 70733a2f 2f676974  15.7,https://git
 d50b0 6875622e 636f6d2f 666f7274 696e6574  hub.com/fortinet
 d50c0 2f736869 6d2d7265 76696577 0a        /shim-review.
  • SBAT seems okay for shimaa64.efi: OK
$aarch64-linux-gnu-objdump -j .sbat -s build-aarch64/shimaa64.efi

build-aarch64/shimaa64.efi:     file format pei-aarch64-little

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 666f7274 696f732c  im.shim.fortios,
 d5090 312c466f 7274696e 65742c73 68696d2c  1,Fortinet,shim,
 d50a0 31352e37 2c687474 70733a2f 2f676974  15.7,https://git
 d50b0 6875622e 636f6d2f 666f7274 696e6574  hub.com/fortinet
 d50c0 2f736869 6d2d7265 76696577 0a        /shim-review.

GRUB

Couldn't find sources and/or patches for GRUB2 NOT OK

See: https://github.com/rhboot/shim-review/blob/main/docs/submitting.md#35-sources-and-patches-for-other-components

As stated in README.md:

  • Upstream GRUB2 2.06 with shim_lock verifier is used OK
  • All requierd security patches are applied OK
  • Moules built into GRUB2 image: "normal search configfile part_msdos part_gpt fat ext2 linux tpm all_video gfxterm terminal echo" OK

Kernel

As with GRUB2, can't verify.

As stated in README.md:

Our kernel is based on 4.19 with lockdown patch and enforces lockdown.

@aronowski
Copy link
Collaborator

I'll send verification emails first, and once the verification is successful, I'll then proceed with the application.

@aronowski
Copy link
Collaborator

Verification emails sent.

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

lingboz commented Dec 19, 2023

exhalations adolescence Tartars casters traded spacesuits hubbies
Ariosto when attributes

@fgtvm
Copy link
Author

fgtvm commented Dec 20, 2023

contradicts shrubberies accusing Minnie flask molding hasted kenning
wastrels hydrogenates

@aronowski aronowski removed the contact verification needed Contact verification is needed for this review label Dec 22, 2023
@aronowski
Copy link
Collaborator

Reviewing.

  • build reproduces, the artifacts are the exact same
  • .sbat OK in both binaries
  • No NX support patch, as the whole bootchain is not NX compatible
  • .sbatlevel OK, no binutils bug

I myself can't verify, what local kernel patches have been added, as well as how the 1957a85b0032a81e6482ca4aab883643b8dae06e, 75b0cea7bf307f362057cc778efe89af4c615354, eadb2f47a3ced5c64b23b90fd2a3463f63726066 commits have been backported to the 4.19 kernel, since I don't have access to it both in binary and source form.

I don't know, what the committee or Microsoft are to say about this. If it must be verified, I'd need access to the product and/or its sources, to see if some proof-of-concept exploits work with the kernel shipped, and once they don't, then I could provide a more detailed answer, that the ports are indeed fine.

A similar scenario is about GRUB2 - I can only check the upstream gnu.org grub-2.06 sources snapshot or the upstream development process at savannah.gnu.org, but not the manual porting process.

I don't suppose I could request a demo by contacting the sales department and explaining, that it's needed to make this review complete. ;-)

@aronowski aronowski added the question Reviewer(s) waiting on response label Dec 24, 2023
@fgtvm
Copy link
Author

fgtvm commented Dec 25, 2023 •
edited
Loading

grub and kernel lockdown patches are in
https://github.com/fortinet/shim-review/tree/main/grub-patch
https://github.com/fortinet/shim-review/tree/main/kernel-patch
CONFIG_SECURITY_LOCKDOWN is defined for all FortiOS VM models and security_locked_down always return -EPERM.
only FortiOS VM models uses shim bootloader.

Please let me know if you need more information.
Thanks,

@aronowski
Copy link
Collaborator

Thank you for the patches!

Here is my analysis of the GRUB2 patches.

cve-2023-10.patch:

cve-2022-11.patch:

cve-2022-06.patch:

I can see the code related to the SECURITY PATCH 23/30 (just 4 lines changed) has not been incorporated. Was this done on purpose? Or am I'm overanalyzing it, as you ship a version, which mitigates CVE-2022-28734 without these changes?

(Info: skipping mentions of CVE-2022-28737, as it was a shim vulnerability)

The kernel-related patch seems to have these lockdown-related fixes applied, just like the description in its comment says, and I assume that the security team has internally tested that proof-of-concept exploits like this one do not work.

The patch in its uploaded form won't work for me, as I don't have access to the complete sources, so I won't be able to compile a kernel with options like CONFIG_FORTINET_LSM. I could try editing it out, using a common configuration with the other options present, and the fixes applied, but this kernel would not be the same artifact that you have. I can only base on the code, which has been uploaded, which looks alright!

Maybe in this case I'll ask more experienced reviewers, what's the best thing to do here.

@fgtvm
Copy link
Author

fgtvm commented Dec 27, 2023

You are right, we missed SECURITY PATCH 23/30, will add it, or if we use grub-2.12, will it meet all requirements of shim-review?

For kernel, our kernel maybe difficult to test with normal script like
https://git.zx2c4.com/american-unsigned-language/tree/american-unsigned-language-2.sh
Would it possible to review just code patch?

@aronowski
Copy link
Collaborator

I myself understand that question in the application template as making sure the GRUB2 release shipped in one's bootchain is not vulnerable to certain CVEs, security patches being one way to mitigate issues (there's a similar question regarding kernel patches, where the RHEL-based releases do not apply one patch, but use a config, which mitigates the issue). So the version should not matter, as long as the artifacts being loaded are not vulnerable.

Similarly, an SBAT generation number may be bumped along with porting the appropriate patches to the current product version or by updating to a newer version, which is not vulnerable.

I can take a closer look at the kernel patch to confirm, that it implements the related code correctly and myself could assume that no issues are present in the final artifact that's being loaded. But I don't know if Microsoft could share that assumption with me, and, after all, they sign the shim binary.
Maybe I could ask @mheese for help. What's the right thing to do?

@fgtvm
Copy link
Author

fgtvm commented Jan 25, 2024

Hi, is there any update on kernel patch review?
If we want to use new shim15.8 version, can we just use this issue?

@aronowski
Copy link
Collaborator

Hello.

If we want to use new shim15.8 version, can we just use this issue?

Yes! Simply update the appropriate entries and I'll re-review them ASAP.

Hi, is there any update on kernel patch review?

I'm still worried that I might miss something and don't want to be held accountable for some issues that I won't be able to find. Sorry - not yet. I think the best bet is to try and ping someone else who has more experience with the kernel than me.

@fgtvm
Copy link
Author

fgtvm commented Feb 8, 2024

Hello,
We updated shim and grub version, created new tag
https://github.com/fortinet/shim-review/tree/fortinet-shim-x86_64_aarch64-20240208

Could your please review?
Thanks,

@aronowski
Copy link
Collaborator

LGTM! Let's wait for another official review and let me wish you all the best!

Some loose notes from the reviewing part:

Kind of a nitpick, but the SHA256 listings mention the build-x86_64 and build-aarch64 directories, which are present only as part of the Docker-based rebuild, rather than as part of the application. - Still OK as the checksums match and the build reproduces.

GRUB2 updated to upstream 2.12 - OK.

At last, I did review the current kernel patch and it (the patch - I don't have access to the complete sources) seems alright. Notes to myself:

  • The kernel gets compiled with CONFIG_SECURITY_LOCKDOWN=y
  • There is the security_locked_down function ported to the linux-4.19.13/include/linux/security.h file
  • Both writing to and reading from memory get restricted

@aronowski aronowski added extra review wanted and removed question Reviewer(s) waiting on response labels Feb 13, 2024
@aronowski aronowski removed their assignment Feb 13, 2024
@aronowski
Copy link
Collaborator

Just a moment after posting this comment I had a revelation: please update the initial post and the issue's title to reflect the current status, i.e. shim 15.8, the current git tag and the current checksums.

@fgtvm fgtvm changed the title Shim 15.7 for FortiOS Shim 15.8 for FortiOS Feb 14, 2024
@steve-mcintyre
Copy link
Collaborator

Review of Shim 15.8 for FortiOS: fortinet-shim-x86_64_aarch64-20240208

OK

  • Contact verification already done - OK
  • shim build reproduces here - OK
  • Builds from 15.8 upstream, with no patches applied - OK
  • NX bit not set - OK
  • Includes a CA key expiring in 2056 - OK
  • No previous shims signed, so revocation is easy - OK
  • GRUB2 from upstream 2.12 with minimal patching - OK
  • HSM for key management - OK
  • small list of grub modules is fine - OK
  • Only loads grub and linux, no other binaries - OK
  • kernel 4.19 with lockdown patches - OK
  • local kernel patches for firewalling
  • SBAT data looks fine for shim and grub - OK
  • reasonable further answers to questions about grub and linux
    • extra trivial grub patches are fine

Issues / queries

  • nothing

Excellent submission, and thanks to @Blarse for reviewing too 👍

Marking as accepted

@steve-mcintyre steve-mcintyre added accepted Submission is ready for sysdev and removed extra review wanted labels Feb 18, 2024
@fgtvm
Copy link
Author

fgtvm commented Feb 19, 2024

Great, thank you all for the review!

@THS-on
Copy link
Collaborator

THS-on commented Jun 21, 2024

@fgtvm did you get a signed shim back?

@THS-on
Copy link
Collaborator

THS-on commented Jul 22, 2024

closing as there hasn't been any response for a month

@THS-on THS-on closed this as completed Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
accepted Submission is ready for sysdev new vendor This is a new vendor
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@lingboz @THS-on @Blarse @steve-mcintyre @fgtvm @aronowski