Add user facing message when SBAT self check fails #671
Conversation
shim.c
Outdated
| * Provide additional information when the SBAT self check fails | ||
| */ | ||
| if ( msg == SBAT_SELF_CHECK ) { | ||
| console_print(L"\nThe current shim has likely been revoked. " |
There was a problem hiding this comment.
Can we be sure here? "likely" isn't great?
There was a problem hiding this comment.
Unfortunately not as verify_sbat_section 1 returns EFI_SECURITY_VIOLATION on different errors: SBAT validation, .sbat section is missing, overflow check.
We could add a flag to differentiate those, but that would be a bigger change.
There was a problem hiding this comment.
Thank you for this PR!
The other option is to have the message clearly cover all the cases that could land you here:
- You are somehow trying to boot an otherwise valid out of date OS image and your shim has been revoked.
- You are somehow booting an out of date shim (perhaps from the wrong device?)
- You are somehow trying to boot an old file that was inserted maliciously.
- You are dealing with a file or EFI variable that was somehow corrupted
- We ran into an internal error (bug in the shim code)
Although that's enough cases that it may make sense to split them up and handle them appropriately. Certainly verify_sbat_section() could differentiate between EFI_SECURITY_VIOLATION and EFI_ERROR, which would group 1-3 and 4&5.
That wording could probably use some more work, but let me know what you think.
I do want to get something like this merged soon, especially since Windows pre-installed systems are very likely to have SbatLevel pre-applied.
There was a problem hiding this comment.
@jsetje my plan is then the following:
- differentiate errors in verify_sbat_section() (thanks for the hint with EFI_ERROR)
- propagate which SBAT rule check failed: Something like: expected at least level 5 for "shim" got "4"
- Output error messages similar to the ones you suggested depending on the information (sbat is wrong, something else failed)
There was a problem hiding this comment.
Printing the exact levels may be more than we need here. In the end a customer needs to know which package to update, and the package versions have nothing to do with the sbat generation.
I'll take a look at your next pass as soon as you have it.
There was a problem hiding this comment.
I now updated the error message handling to differentiate between likely .sbat section or internal errors and when an actual revocation happens.
As I had most of the code for showing the component name and expected and found SBAT levels already done, I also now included it in the error message shown to the user.
Error message for revocation looks like this:
Error message for everything else (in this case I just set the .sbat section to nothing):
I think this gives users and also distros enough info if something happens.
There was a problem hiding this comment.
I'd be very much in favor of rewriting the error messages for several reasons, and here are some of them that I can spot at the first glance, especially since the PR is about those who have no experience with shim and its environment and terminology.
The text "Something has gone seriously wrong" already gives me a sense of dread and I'd assume the same can happen with someone, who sees the message for the first time, when attempting to install a system. The same with words like "corrupted".
Addressing the errors to a user of some distro, I'd avoid the modern mentality of saying "Something went wrong. Try again later", and instead opt for a clear message on what to do with it, than just stare at the screen, e.g.
Your UEFI Shim is not compliant with your machine's UEFI security policy - your Shim is at level 4, but should be at least 5.
Report the following information to your system vendor at [email protected] for further assistance:
"SBAT self-check failed - expected `shim,5` or higher, but got `shim,4`"
or
Your UEFI Shim is not compliant with your machine's UEFI security policy - your Shim is at level 0, but should be at least 5.
Report the following information to your system vendor at [email protected] for further assistance:
"SBAT self-check failed - expected `shim,5` or higher, but the `.sbat` section couldn't be parsed"
or
Your UEFI Shim is unable to start.
Report the following information to your system vendor at [email protected] for further assistance:
"SBAT self-check failed - internal error. Report this to the shim developers at https://github.com/rhboot/shim/issues."
There was a problem hiding this comment.
On the one hand I don't love "Something has gone seriously wrong", on the other hand it does convey that we did not want anyone to end up in that situation and it's worth escalating so that we can fix anything that may have automatically put it there, such as a dependency check before applying a revocation. While it made headlines recently, I didn't actually see complaints about that string, just that there wasn't additional useful information.
There was a problem hiding this comment.
There is two types of information that we want to convey: what went wrong (e.g. shim was revoked via SBAT) and what to do for the user.
The first one should contain enough information for us or the distributions to see what is wrong and the second something that a user can easily understand.
We already print a decent amount of technical error messages, that I wouldn't want to hide, because those are helpful to debug issues. Maybe something like this:
[...]
Something has gone seriously wrong: [...]
SBAT self-check failed! Expected "shim,5" or higher. Got :"shim,4".
Your shim has been revoked via the UEFI Secure Boot SBAT policy on your machine. Please update to a compliant version.
[Vendor custom string here]
We can add the option for distributions to include some information where to redirect people, this is then something we would need to audit during reviews and have policies around. @jsetje what do you think? Also wouldn't direct users directly to upstream as most errors should go through the distribution first.
There was a problem hiding this comment.
From a UX perspective I'd change the order of the text, e.g.
[...]
Your shim has been revoked via the UEFI Secure Boot SBAT policy on your machine. Please update to a compliant version.
[Vendor custom string here]
Technical details:
SBAT self-check failed! Expected "shim,5" or higher. Got: "shim,4".
It should provide further hints (or even better: a clear message, considering the vendor's custom string*) for an end user on what to do, e.g. take a photo of their screen and report back to their vendor in the first place. Then (only later, rather than earlier), if the end user is adventurous enough, they could diagnose the SBAT self-check on their own, and potentially even learn some knowledge from a foreign domain (for such an end user in this context the boot processes are indeed something foreign).
* That's some reminiscences of mine regarding good video game design: If you can't understand what the game is asking you to do, this is not a game, but a challenge in frustration. We're here to make the users' systems boot securely, rather than frustrate them, after all.
On the one hand I don't love "Something has gone seriously wrong", on the other hand it does convey that we did not want anyone to end up in that situation and [...] I didn't actually see complaints about that string, just that there wasn't additional useful information.
Now's the first time to see a complaint, I suppose. ;-)
It might have to do with the background of mine, and that I know how any visual oddities may become associated with malware for end-users (Source: QubesOS/qubes-issues#6425), especially if suddenly the system is unable to start as intended, only for a screen like this to be present and the machine shutting down immediately, without any meaningful feedback on what to do.
The end user in this context is someone, who knows nothing about boot processes of the system they use, and if I was one, with the knowledge they have, I'd think I got compromised with some sort of ransomware, especially when seeing texts like "Something has gone seriously wrong". Or think about, why there's no ASCII art of a skull and crossbones, and that message instead.
The vendor string idea, instead of that scary message, looks like a good proposal to me, but needs some polishing to be implemented optimally: thinking, how to solve the thing, where companies are merged/bought, as per rhboot/shim-review#345 (comment).
This exports the component name and found and expected SBAT level from verify_sbat_section for us to display to the user why validation failed. Further we now differentiate between errors regarding .sbat section related errors and SBAT revocations. Signed-off-by: Thore Sommer <[email protected]>
This allows the user more time to read the error messages. Signed-off-by: Thore Sommer <[email protected]>
THS-on
force-pushed
the
revocation-message
branch
from
c2a22f9
to
7b98f75
Compare
As with the upcoming versions we are going to revoke more and more older shims, I've been testing this more extensively and have found that the current message is not ideal for users who don't normally interact with the shim.
This is an initial attempt at adding additional message with more context. We could also include an URL with additional information, but the question here is if this should be generic or distro specific?