fix(bootloader-env): block all fw_setenv options, not only -s/--script - #679
Merged
JanZachmann merged 4 commits intoAug 5, 2026
Merged
Conversation
The set command compared key and value against '-s', '--script' and their '=' forms. getopt accepts attached values and abbreviations, so a value like '-sFILE' or '--scr=FILE' reached fw_setenv as script mode, and '-cFILE' as an attacker-chosen config, both writing files as root. The adu sudoers rules pass the value unfiltered for omnect_extra_bootargs and omnect_validate_extra_bootargs, so a crafted value could exploit this. Pass '--' instead: key and value are always data, which covers every option rather than a list of known ones. Same for unset and get, whose key reaches fw_setenv/fw_printenv the same way. Signed-off-by: Jan Zachmann <50990105+JanZachmann@users.noreply.github.com>
The dispatch called "${1} ${@:2}" unquoted, so a value with spaces was split
into words and rejoined inside set() via "${@:2}". Two consequences: repeated
whitespace collapsed to a single space, and a value containing glob characters
was pathname-expanded, e.g. "set omnect_extra_bootargs '*'" stored the listing
of the current directory. The argument count was checked against a global taken
before the split, which is why the split went unnoticed.
Quote the dispatch and take the value from "${2}", so each function gets its
arguments as they were passed and can count them itself.
Rename the commands to cmd_* as well. set and unset shadowed shell builtins,
which only works because bash prefers functions over builtins outside POSIX
mode - and set is a POSIX special builtin. The prefix also lets the dispatch
check the function exists instead of matching the command against a list as a
substring.
Signed-off-by: Jan Zachmann <50990105+JanZachmann@users.noreply.github.com>
A value of -n, -e or -E is an option of echo, so "get" printed nothing and the caller could not tell it apart from an unset key. printf takes no options after the format string, so the value is passed through unchanged. Signed-off-by: Jan Zachmann <50990105+JanZachmann@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors the bootloader_env.sh wrappers (u-boot and GRUB variants) to safely dispatch commands and preserve argument integrity (notably values containing whitespace or glob characters), while also avoiding conflicts with shell builtins.
Changes:
- Rename command functions to
cmd_*and update dispatch to call"cmd_${1}" "${@:2}"with a function-existence check (declare -F) instead of substring-matching against a list. - Stop re-joining value arguments via
"${@:2}";setnow takes the value from"${2}"(so a value with spaces must be passed as a single quoted argument). - Harden u-boot
fw_setenv/fw_printenvcalls by passing--so key/value are always treated as data.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| recipes-omnect/bootloader_env/bootloader-env/bootloader_env_u-boot.sh | Updates dispatch and argument handling; uses -- with fw_setenv/fw_printenv to prevent option injection. |
| recipes-omnect/bootloader_env/bootloader-env/bootloader_env_grub.sh | Updates dispatch and argument handling for GRUB env editing, including quoting and cmd_* dispatch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
HarryWaschkeit
left a comment
HarryWaschkeit
left a comment
Contributor
There was a problem hiding this comment.
see copilot comment
HarryWaschkeit
approved these changes
Aug 5, 2026
ods 0.45.1 carries the same fw_setenv fix for its own wrapper (sudo/fw_setenv_no_script.sh) that this branch applies to the layer's bootloader_env.sh, so both copies of the defect get fixed together. Recipe regenerated with cargo-bitbake; the crate set did not change. Signed-off-by: Jan Zachmann <50990105+JanZachmann@users.noreply.github.com> (cherry picked from commit 80d0500)
Contributor
Author
|
@HarryWaschkeit the review of #678 fixed code that was part of this PR as well. Thus I closed #678 and this one must be re-appropved |
HarryWaschkeit
approved these changes
Aug 5, 2026
HarryWaschkeit
left a comment
HarryWaschkeit
left a comment
Contributor
There was a problem hiding this comment.
imho the description should also mention the omnect-device-service update that is contained.
JanZachmann
changed the title
Contributor
Author
|
FYI: @mlilien |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Harden the u-boot bootloader_env.sh wrapper against
fw_setenv/fw_printenvoption injection, refactor the wrapper along the way, and pin omnect-device-service 0.45.1 which carries the same fix for its own wrappersudo/fw_setenv_no_script.sh.--tofw_setenv/fw_printenvso key and value are always treated as data. This blocks script mode and every other option, e.g. an attacker-chosen config file, which a flag blocklist would miss (getopt accepts attached values like-sFILEand abbreviations like--scr=FILE).set/unset/get/listtocmd_*so nothing shadows the bash builtins.cmd_*takes its own quoted arguments and checks its own argc. Top-level dispatcher looks the function up viadeclare -Finstead of an unquoted string match.getprints values withprintf '%s\n'and strips thekey=prefix with a quoted pattern, so values with a leading-n/-eand keys with glob metacharacters do not misbehave.Reason
The old blocklist covered
-s,--script,-s=*and--script=*.fw_setenvparses arguments withgetopt_longand the optstringVc:f:s:nhm:(libubootenv,src/fw_printenv.c), which accepts attached values (-sFILE) and abbreviations (--scr=FILE), so those forms passed the filter and reachedfw_setenvas options. GNU getopt also keeps scanning after the first positional argument, so the value position was enough to sneak in an option.recipes-azure-iot/iot-hub-device-update/iot-hub-device-update/adu-bootloader-envpins the key but passes the value unfiltered foromnect_extra_bootargsandomnect_validate_extra_bootargs, so theaduuser could write files as root — via a script file (-s) or an attacker-chosen config (-c), the latter not covered by a-s/--scriptblocklist at all.The same defect existed in ods's own wrapper
sudo/fw_setenv_no_script.sh(omnect/omnect-device-service#208). Both copies ship in the same image, so the ods pin belongs in this PR — otherwise the image would keep the unfixed wrapper.Supersedes #678.