|
Bugzilla – Full Text Bug Listing |
| Summary: | grub2-once prints spurious error message | ||
|---|---|---|---|
| Product: | [openSUSE] openSUSE Distribution | Reporter: | Henryk Hecht <nvbugs> |
| Component: | Bootloader | Assignee: | Bootloader Maintainers <bootloader-maintainers> |
| Status: | NEW --- | QA Contact: | E-mail List <qa-bugs> |
| Severity: | Normal | ||
| Priority: | P5 - None | ||
| Version: | Leap 15.5 | ||
| Target Milestone: | --- | ||
| Hardware: | x86-64 | ||
| OS: | openSUSE Leap 15.5 | ||
| Whiteboard: | |||
| Found By: | --- | Services Priority: | |
| Business Priority: | Blocker: | --- | |
| Marketing QA Status: | --- | IT Deployment: | --- |
This was reported against the grub2 package with version 2.06-150500.27.4. In 2.06-150500.29.3.1, released in late July, sh_test() gets this:
diff tmp/grub2/usr/sbin/grub2-once /usr/sbin/grub2-once
25a26,31
> # Don't test grub command return status from linux shell, this often results
> # in command not found error. In such case the expression often has no
> # opening bracket and just returning false here to signify -ENOCMD error.
> return 0 if ( $exp =~ m{^\s*[^\[]});
I find the comment mystifying; I doubt whether it says what the author intended. The guard does prevent it from trying to run things like terminal_output, but I don't know if it suffices to prevent other sorts of mischief. Maybe it is safer to exec /usr/bin/test directly rather than run a full shell that could be tricked into doing something else? This is made difficult by another bug in the script.
On looking at what sh_test() runs, it often passes along unexpanded variables, as so:
[ -z "${config_directory}" -a -f $prefix/custom.cfg ]
The shell that sh_test() runs will not have values for these, so the wrong value may be returned. Conversely, it may have a value for a variable that would not be visible in grub.cfg. On the other hand, expansion of the above variable happens in ${config_directory}/grubenv (to /boot/grub2/grubenv). I think the loop that begins (around line 118):
while ( m'(?:[^\\])(\$(?:{([^}]+?)}|([A-Za-z0-9_]+)))' ) {
is meant to do variable expansion in these expressions, but it clearly doesn't work correctly: e.g., $var and ${var} are sometimes expanded, sometimes not (I think they are expanded so long as a value exists in %E?); "$var" and "${var}" never are. In order to pick the correct branches, variable expansion needs to be fixed. Doubly so if one wants to run /usr/bin/test instead of sh.
In any case, 29.3.1 is both a cosmetic and safety improvement, but sh_test() is still buggy (and possibly dangerous). The exposure could be mitigated by not running a full shell. A proper fix unfortunately seems to entail the parser in read_cfg(), not just sh_test() itself.
|
When run with an argument, e.g.: grub2-once 0 one gets on stderr: sh: terminal_output: command not found The script subsequently succeeds. What seems to be tripping it up is this stanza in /boot/grub2/grub.cfg, included from /etc/grub.d/00_header: for i in gfxterm; do if [ x${use_append} = xtrue ]; then terminal_output --append $i elif terminal_output $i; then use_append=true; fi done This hits grub2-once:84: $state = 0b010 + sh_test( $1); # if-? This is mostly a cosmetic bug as encountered, and hence minor. On the other hand, the hand-built state machine parser causes sh_test and thence Perl's qx (popen(), more or less?) to be run on a large number of things that are not actually shell tests. This indicates the parser is fundamentally faulty, and the result is essentially undefined behavior running as root, as the intersection of what the parser sends to sh_test and what may be in /boot/grub2/grub.cfg (and grubenv) is hard to reason about. I would bet that a clever person could e.g. name a boot option such that it does rm -rf /. This is markedly less alarming since grub.cfg is 600, but I mention it to illustrate that the problem is likely more serious than its cosmetic effect, and adding 2>/dev/null or 2>&1 (cf. bug #1190473) is probably not good enough. If sh_test is meant evaluate shell test expressions, then it should not receive anything else. In my debug run, most of what it was called with was not appropriate. Fortunately, it did not delete my root directory; mostly it just tried to run a large number of non-existent commands. I don't know if the parser defects are new or have simply gone unnoticed until now. In any event, it requires little imagination to see that arbitrary damage could occur, especially as it can "run" menuentry names.