-
Notifications
You must be signed in to change notification settings - Fork 293
reviewer guidelines
Here's a non-exhaustive set of things to look for when reviewing a shim submission.
Thanks to Paul Nardini for the initial text, now adapted and posted here!
First of all, there are some higher-level things to consider:
-
Does the developer have a good grasp of shim, the chain of trust etc.? We've had some submissions where people had not spent any time learning how to do shim/grub/kernel etc. (so we would not want to approve their shim for signing). In other cases, people have wanted to learn more, and the review process has acted as education. (We're working on better documentation to make this less painful.)
-
Does the vendor actually need to get a signed shim? Some are not clear about this. If they don't need to run on ~all machines, they can just sign things themselves. If they don't need to customise the kernel, they should reuse shim+grub+kernel from another signed distro instead.
-
Does the vendor look like they might actually be around to deal with security issues in the future? A tiny 1-man outfit may just go away without warning.
-
If the developer hasn't done what we ask for, then say so. This can cover a range of things with different severities, e.g.
-
Not using the correct upstream shim source is a no-no and a hard FAIL.
-
Patches applied from upstream should be obvious and easy to identify, whether in shim or grub or kernel. If this causes you to spend massively more effort in reviewing, then that is bad of course. Mention this kind of thing, and explain what the developer could do to make our lives (and hence their lives!) easier here.
-
Not providing a Dockerfile that works, or one that makes it hard for you to see what's going on.
-
Not providing all the details of their other binaries, or maybe being vague about them. It's easy to miss this kind of thing when submitting, and we should be looking for these. It's most likely just something's that been missed, but we should be expecting good, clear, accurate, complete submissions for shim review. Without that, we can't accept things safely.
-
Then we have some concrete things to look at technically:
Did the submitter answer all the questions on the review template and in their review repository’s README.md? If not, say so clearly and move on. If questions have not been answered clearly, ask for clarification. Ask for help if you're not sure on anything.
Is the submitter from a new vendor that has not previously submitted to the repository? If so, verification of submitter email address ownership needs to be performed before review can be completed.
The submission should include details of PGP keys for each of the contacts. Using those keys, send an encrypted message to each of the contacts to verify their identity.
A common thing to do here is to send a list of random words to each contact and ask them to copy them into the review issue. See Steve's script if you need help with this.
Is the submitter’s build reproducible? If you cannot reproduce their build and match the hashes listed in the submission, the submitter must make appropriate fixes and try again. Be clear about this in a review - show the expected/published sha256sum, and the result from the local build.
Notes
-
there used to be some known issues with reproducibility in arm-based shim submissions, which were accepted back then. This should no longer be the case - shim 15.7 and binutils 2.38 solved this problem.
-
A common issue is that submitted Dockerfiles may "bit-rot" over time. If they refer to
Ubuntu Jammy, for example, a new compiler version might appear in a future update and the build will differ. If this seems to be the case, clearly describe the issue in your review.
Is the submitter’s shim built from source code of known provenance, i.e. rhboot/shim? Any patches applied must be adequately identified and explained. If they do not come from known contributors (e.g. backports from the official rhboot/shim tree), then you should be extra-careful to review the changes. Ask for help if you're not sure.
Ideally, the build should be made using the exact source tarball referenced in the submission guidelines, but so long as the source is clearly the same, then it's acceptable to use a git repo instead.
We can no longer accept shim signing submissions for versions before 15.7.
To check the details of the cert:
$ openssl x509 -inform der -in <cert.der> -text -noout
Does the submitter’s embedded certificate match the organization they are submitting under, and can they explain why if not?
Does the submitter have reasonable access controls in place (e.g. HSM, etc.) for the private half of the public key embedded in their shim?
Does the embedded certificate have a reasonable validity period? For a straight certificate, 1 to 5 years is probably sensible. For an embedded CA cert, longer is fine (20 years?)
If the submitter is embedding a CA certificate, do they describe how the rest of their chain of certificates is handled?
SBAT is Secure Boot Advanced Targeting. You need to understand
how SBAT works, and you need to carefully check the submission SBAT
data:
Is SBAT data present in the provided binaries, and does it match what was provided in the answers to issue template questions?
Is the vendor extension sensible? A clear, unique name is good, something that's too likely to clash with another vendor is bad. Too long or too short an extension is not wanted, etc.
If a binary is derived from a build from another distro, it's recommended to also include the details of that build and distro in the SBAT. This will make it easier to revoke things if we find that a whole family of builds are all broken due to a shared vendor patch.
Does the submitter use grub as a bootloader? This is the default assumption in shim and review so far.
If grub is used:
- Does it have the patches stated by the issue template and README.md?
- Are there any custom patches applied? If so, are they explained by the submitter and well understood? This can be very time-consuming to do right - if a vendor is doing their own novel patches we may need to get more reviews.
- Which grub modules are built in? The smaller, the better here, for the sake of reduced attack surface. Some of the more obscure grub filesystem modules have patchy security history and are best left disabled.
If the submitter is not using grub, additional full review by someone who is capable of performing it will be required. Ask for help if you're not sure.
What kernel is loaded, and does it have all the patches stated by the issue template and README.md? Older kernels will need a lot of patches applying to make SB work well, but more modern kernels are safer out of the box.
If the submitter has already had a previous shim signed, then they must also explain how revocation is achieved for old, vulnerable binaries.
Does the submitter adequately address how secure boot is enforced in their boot stack and how their boot stack prevents execution of unauthenticated code?
Except for sending the contact verification mails (see above), please keep all communications in the issues.
We have a small set of labels that can be attached to review submissions to help us track things. These should be self-explanatory, I hope! The correct labels should also act to give clear information to submitters. If you're a known reviewer, you can add/remove/modify labels as you see fit.
We should never add the accepted label until a submission is
considered complete and ready for signing. Once a submission has been
signed, submitters should be encouraged to mark their shim-review
issues as closed.