Improving the robustness of value retention for the variable second_stage #626
Conversation
|
After some research I found a similar solution. It starts at exactly the same point as I did and checks whether the value of this variable represents a valid path before overwriting the variable "second_stage": Various checks are implemented there, but the check whether the first character contains a non-printable ASCII character is omitted. If you are looking for a general solution to this problem, perhaps a combination of both solutions: mine and the one just mentioned would be a better solution that takes several eventualities into account. But in my opinion, one should make a fundamental consideration whether the function parse_load_options() has any justification at all. |
The link above is broken, it looks like you are linking to a commit that does not exist in this repository. I guess you are referring to some other PR that got updated since, so the commit object got replaced. If that is the case, have you tried to raise this point on such PR? Also, as a general practice, it is better to link to PRs instead of commits, since PR numbers are persistent. Also, the commits on this PR do not have a commit message and the PR does not pass CI. Adding a good commit message makes it easier to review changes. |
|
@jprvita Hi, thank you very much for your comment. Ok but the main issue is that the function parse_load_options() does not always return a valid path for the variable second_stage. My improvement is actually that the value after the parse_load_options() function is checked for non-printable ASCII characters. This reduces the probability of an error situation occurring at this point. |
|
@AndrewSav Thank you very much! That's exactly the link for Commit I was looking for! |
|
Although this solution technically safe (meaning that it won't open any new security holes), I don't think you should use isprint on CHAR16, which is not even ASCII. Did you actually check what the buffer contains and where it comes from? Maybe it's the same BCDEdit problem as #621? The characters in that issue are perfectly printable if the font supports them, so if isprint fixes the issue, it's mostly good luck. |
|
@Metabolix Hi, thank you very much for your opinion. regarding isprint, I found the following: It seems that the problem is slightly different as in case from #621. However, what seems to be common is that the function parse_load_options() does not always return the valid value for path. What I found out is the following: the first character after the parse_load_options() function is ASCII character dec 1, so this is a non-printable ASCII character. Non-printable ASCII characters are characters dec from 0 to 32. Of course, we have not solved all problems with the isprint function, but as in our case, at least non-printable characters can be recognized and so the variable second_stage can keep its default value "\grubx64.efi". With this function we only ensure that if such a situation arises (non-printable character) then the default value is not overwritten. |
|
C actually allows locale-dependent charset instead of just ASCII, see https://en.cppreference.com/w/c/string/byte/isprint But luckily shim seems to define isprint simply as:
So it's ok to use. The side effect is that your file name can now only start with an ASCII character. That's probably not a problem in practice but still a slight limitation, ruling out non-English characters. On the other hand, you will still have a problem if the first character happens to be good but the rest are garbage. Why not check the whole buffer up to the Have you determined where the bad data comes from in your case? Is it a bad boot entry or a firmware problem, or what? The function doesn't make it up by itself. |
|
@Metabolix Hi, thank you for review and your suggestions! |
|
@Metabolix Hi Lauri, |
Unfortunately not. This second_stage / parse_load_options is the only part I know, based on thoroughly investigating a similar issue. I have nothing to say about signing or certificates. |
|
Can you please explain how you create the boot entry (what OS, what tool) and what are the exact contents of the boot entry (for example, hexdump the entry from Linux /sys/firmware/efi/efivars/Boot000*). That would make it easier to verify the best solution for the case. |
|
@Metabolix Hi, many thanks for your ideas! A boot entry is stored in the firmware's boot manager, which can be accessed during the installation of OS or modified just after the installation. The prerequisite for this is that the operating system remains permanently installed on the hardware. However, our boot environment is located on the separate boot medium and the Linux environment is started from there. This means that we have no way of permanently binding our boot environment to a specific hardware. Consequently, if our Linux boot environment is repeatedly started on the other hardware, we will always have different boot entry contents that we are unable to customize accordingly. This means that the contents of /sys/firmware/efi/efivars/Boot000* will always be unpredictably different. Therefore, from my point of view, the solution to the problem for our case is to improve the code of Shim. Specifically: the original code is missing the check of the output of the split_load_options() function. This is where the check should take place to determine whether the string has a valid path or not. The original code only checks whether the output is not NULL. This is too little, especially for the case like ours. |
|
Just to get this straight: do you create an external boot medium, like a live cd, which is booted by selecting "boot from cd" or "boot from usb" or some such? So that you don't have an own boot entry at all? This would mean that any extra data passed to shim is generated by the firmware. I don't doubt that your patch works for you, but to convince the maintainers to accept a patch upstream, it's probably necessary to have more data than just "works for me". It would be instructive to get your exact work flow (where is shim and how do you boot it) and a complete hex dump of the whole load_options. Maybe you could create the dump in parse_load_options for debugging purposes. As I noted earlier, simply limiting the first character to printable ASCII doesn't reliably block all possible bad paths (with bad data at a later point), and it blocks also valid non-ascii paths, so it's not a trivially perfect solution. |
|
@Metabolix Hi, thanks for your comments!
That is exactly our case. Our Linux environment is located on the CD, USB stick or USB drive. The boot process is initiated by selecting Boot Device from the boot menu. Enclosed, I have put together the necessary hexdumps. All were found as you suggested in /sys/firmware/efi/efivars/Boot000*: Boot0000-8be4df61-93ca-11d2-aa0d-00e098032b8c: 0000000 0007 0000 000a 0000 0025 0050 0030 003a Boot0002-8be4df61-93ca-11d2-aa0d-00e098032b8c: 0000000 0007 0000 000a 0000 0025 0054 004f 0053 Boot0004-8be4df61-93ca-11d2-aa0d-00e098032b8c: 0000000 0007 0000 000a 0000 0025 0050 0031 003a Boot0005-8be4df61-93ca-11d2-aa0d-00e098032b8c: 0000000 0007 0000 0001 0000 0060 0055 0045 0046 Boot0006-8be4df61-93ca-11d2-aa0d-00e098032b8c: 0000000 0007 0000 0001 0000 0094 0055 0045 0046 BootCurrent-8be4df61-93ca-11d2-aa0d-00e098032b8c: 0000000 0006 0000 0006 BootOptionSupport-8be4df61-93ca-11d2-aa0d-00e098032b8c: 0000000 0006 0000 0001 0000 BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c: 0000000 0007 0000 0005 0006 0000 0002 0004 I hope this information is sufficient, if not please let me know. |
|
Thanks for the clarification. Like you said, the NVRAM boot entries are not used in your case. I was hoping for the data which parse_load_options tries to parse. Here's a small patch 0001-debug-dump.txt which you could apply to shim to get the dump visible on screen during boot and also store the data (temporarily) into |
|
@Metabolix Hi, after changing the code according to the patch 0001-debug-dump.txt and recompiling, I will send you the content of the file /sys/firmware/efi/efivars/DebugShimLoadOptions-*. This content was extracted with hexdump -C: 00000000 06 00 00 00 00 00 42 4f |......BO| Or as file: |
|
Thanks! That's indeed invalid data. One more question, now that I digged around a bit more: Have you defined DISABLE_REMOVABLE_LOAD_OPTIONS, as instructed in BUILDING, which seems to be exactly meant for your use case? "This prevents boot failures because of unexpected data in boot entries automatically generated by firmware." |
|
@Metabolix Hi, many thanks for your hint! That actually worked! If I set DISABLE_REMOVABLE_LOAD_OPTIONS=y then the error with “Failed to open \EFI\BOOT\ - Invalid Parameter” no longer occurs. This means you can switch off the parsing load options when invoked as boot*.efi. This is OK. So my problem is actually solved without the patch #626. But purely academically, do you really think that the checking whether the output of the split_load_options() function has a valid path is not necessary? Perhaps there are other constellations in the real world where a similar error can occur? |
|
My use case would be covered by #621. A sanity check for the path would be nice and would fix the same problem, but I don't know what's the preferred way to do it.
|
|
@Metabolix Hi, So I would approach this problem like this, first of all hypothetically, two phases of sanity-check must be carried out:
Phase 2
Only the path that passes both phases of the check can be considered valid. |
|
|
@Metabolix I agree with 1, but only partially. Therefore the 2 phases test. I don't think deleting the error messages is a good idea. After all, they are there to report certain incorrect constellations. Finally, after parsing the boot options, it is important to know whether you can use the path-to-boot-loader or not. My goal is definitely not to remove the error message, my goal is to have clean code that can handle all eventualities. Unfortunately, I didn't understand your last question:
I tried to define the default loader (I used the DEFAULT_LOADER option). But this did not lead to a change, and the error “Invalid Parameter” remained. Is that what you mean? |
|
Yes, you will get a similar error message even if the file name (configured in NVRAM boot entry) is valid but the file just does not exist. I don't remember now if it's "invalid parameter" or some other text but the messages are there anyway. If your definition of "valid path" requires that the file exists, then there will be no way to report an error message about a missing file. |
|
@Metabolix Sorry that I may not have expressed myself clearly. but by “valid-path” I mean “valid-path-to-bootloader”, which means that if we can check that the boot-loader can be found and is accessible, then we can call the whole path, i.e. “path-to-bootloader”, valid. If the message about the boot-loader file is important, then we can check both. First the path alone, and if the path is valid, then the accessibility of the boot loader, i.e. path + boot loader. |
|
This boils down to the question, how do you know the difference between invalid (= should be ignored) and mistake (= should be reported). Non-printable chars are probably invalid (like in your case). Wrong arch (like grubaa64.efi instead of grubx64.efi) is probably a mistake and should be reported. Correct arch but missing file is even more likely a mistake. What about a file name ending in What happens with a strange but valid relative path like Where do you draw the line? How complex heuristics do you make? Is there a case where you would consider the data good if the file exists but silently ignore as invalid if it doesn't? I think that the printable ASCII check is a reasonable (simple but effective) heuristic to detect true invalid data to be ignored, if restricting the path to ASCII is generally acceptable. Personally I wouldn't add any other checks, because I think they would be useless and complex and it's so easy to come up with possible mistakes which then would be silently ignored. If you suggest some other check, then at least explain why the check is needed (i.e. where would you get such invalid data by accident), and what happens in the examples I made above. If you don't care about mistakes and just want to ignore all non-bootable paths, that gets us back to the question, why would you implement a new check for it when it's basically the same as just removing the error messages. |
|
@Metabolix Instead of answering individual questions, I would suggest that I present a scheme for the code structure.
|
|
I'll answer to your numbering:
|
|
@Metabolix Regarding point 3, I only included this because you wanted to distinguish between two error situations in your message (= should be ignored) and (= should be reported). If this is not important, then point 3. can be ignored. In this code schema description, I did not want to specify any assignment to specific functions and their localization. It was just an idea of how Sanity-Check should basically look. If you think that the check for non-printable ASCII characters would be sufficient, then, from my point of view, that would be a step in the right direction. |
|
@Metabolix Do you think my suggestion for checking non-printable ASCII characters could be accepted and included in the official Shim version? |
Like I said before, I think it would be more useful to disallow all non-ASCII characters along with the non-nrintable ASCII characters. That is, only allow printable ASCII characters. A clear benefit is that the default configuration would work out of the box for more use cases (like most of these removable media cases). However I can't speak for the shim project. |
|
@vathpela Hello, we discussed together with @Metabolix about the sense of ignoring second stage path in load options and replacing it with default path if it contains non-printable or non-ASCII characters. Please see our discussion on this topic. Do you think it makes sense to introduce this check in the official Shim version? |
|
Am I right in thinking that 0287c6b obviates the need for this PR? |
Probably not, because that commit checks for empty string but in this pr's case the string is not empty but contains non-printable and/or non-ascii characters. This PR would intentionally remove support for Unicode file names containing non-ascii characters (like åäö or cyrillic). If this idea is acceptable, then the pr should be fixed to check the whole string, not only the first char, to be consistent and more robust. |
|
I want to point out that I generally fixed the issue in b437584 (PR #399) back in 2021 already; because Ubuntu was facing similar issues, by making shim entirely ignore load options if loaded from the removable media path. It just so happens this is an option (DISABLE_REMOVABLE_LOAD_OPTIONS=1), that is not set by default. but like set in the Ubuntu shim. I don't know why we put it in an option rather than make it the default behavior, but the past 3ish years have shown us that this solves all the issues. |
|
Surprisingly in #393 I also made it fallback to the default loader if the parsed loader does not exist as you can see in the screenshot, so regardless, this only is a visual issue, as opposed to a functional one. |
in function: shim_init(void)
in function: EFI_STATUS set_second_stage (EFI_HANDLE image_handle)
second_stage is set to \grubx64.efi
in the further course of time:
in function: efi_status = init_grub(image_handle);
EFI_STATUS init_grub(EFI_HANDLE image_handle)
second_stage variable - no longer has any value !!! the value \grubx64.efi has been lost
see attached screen foto:
We have localized this problem:
in the file load-options.c in function parse_load_options(EFI_LOADED_IMAGE *li) the variable loader_str was initialized as CHAR16 *loader_str = NULL;
In the same file line Nr 445 the variable loader_str has received a non-printable ascii character from the function split_load_options(). This means that the variable does not have a NULL value anymore but have got the value "non-printable ascii character". After further check (line 454): if (loader_str) if variable has still NULL value the result was positive (loader_str is not NULL and it has a value) and the variable second_stage (\grubx64.efi) has been overwritten by a non-printable ascii character, as you can see on the screen-foto.
To improve this situation we have introduced the additional check of the first character of the variable loader_str in order to be sure, that at least first character is not a non-printable ascii character:
Line 454 if (loader_str && isprint(loader_str[0])). For this purpose we use standard function isprint(). This function returns true if the character is printable ascii character.
In this way we have solved the problem with non-printable ascii characters and overwriting the originally set value \grubx64.efi for second_stage through the non printable char. I think this improvement will be useful for all users of Shim boot-loader because this situation with non printable characters, as we see, can happen.