fix(bootloader-env): block all fw_setenv options, not only -s/--script - #678
Closed
JanZachmann wants to merge 3 commits into
Closed
fix(bootloader-env): block all fw_setenv options, not only -s/--script#678JanZachmann wants to merge 3 commits into
JanZachmann wants to merge 3 commits into
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>
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>
Contributor
Author
|
FYI: @mlilien |
There was a problem hiding this comment.
Pull request overview
This PR hardens the u-boot bootloader_env.sh wrapper against fw_setenv/fw_printenv option injection by terminating option parsing with --, and updates the Yocto recipe to pull a matching fix in omnect-device-service.
Changes:
- Pass
--tofw_setenv(set/unset) andfw_printenv(get) so keys/values cannot be interpreted as options. - Replace the previous
-s/--scriptflag blocklist with the more robust--option terminator approach. - Bump
omnect-device-servicerecipeSRCREVto the 0.45.1 commit carrying the same wrapper fix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| recipes-omnect/omnect-device-service/omnect-device-service_0.45.1.bb | Updates the pinned upstream commit to include the related wrapper hardening fix. |
| recipes-omnect/bootloader_env/bootloader-env/bootloader_env_u-boot.sh | Ensures fw_setenv/fw_printenv treat key/value strictly as data by using -- and removing the fragile flag blocklist. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Follow-up on the fw_setenv option-injection fix. get() still had unquoted
expansions:
- fw_printenv -- ${key} word-splits/globs the key.
- ${value#${key}=} treats the key as a glob pattern.
- echo ${value} eats leading -n/-e/-- and word-splits.
None are exploitable today (sudoers pins the key to a fixed literal, and
values are already attacker-owned), but printf '%s\n' fixes a real
read-back bug: values starting with -n or -e are truncated when
swupdate_handler_v2_u-boot.sh reads them back through get.
Signed-off-by: Jan Zachmann <50990105+JanZachmann@users.noreply.github.com>
HarryWaschkeit
approved these changes
Aug 5, 2026
JanZachmann
added a commit
that referenced
this pull request
Aug 5, 2026
#679) ## Summary Harden the u-boot bootloader_env.sh wrapper against `fw_setenv`/`fw_printenv` option injection, refactor the wrapper along the way, and pin omnect-device-service 0.45.1 which carries the same fix for its own wrapper `sudo/fw_setenv_no_script.sh`. - Pass `--` to `fw_setenv`/`fw_printenv` so 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 `-sFILE` and abbreviations like `--scr=FILE`). - Rename `set`/`unset`/`get`/`list` to `cmd_*` so nothing shadows the bash builtins. - Each `cmd_*` takes its own quoted arguments and checks its own argc. Top-level dispatcher looks the function up via `declare -F` instead of an unquoted string match. - `get` prints values with `printf '%s\n'` and strips the `key=` prefix with a quoted pattern, so values with a leading `-n`/`-e` and keys with glob metacharacters do not misbehave. - Bump omnect-device-service to 0.45.1. Recipe regenerated with cargo-bitbake; crate set unchanged. ## Reason The old blocklist covered `-s`, `--script`, `-s=*` and `--script=*`. `fw_setenv` parses arguments with `getopt_long` and the optstring `Vc:f:s:nhm:` (libubootenv, `src/fw_printenv.c`), which accepts attached values (`-sFILE`) and abbreviations (`--scr=FILE`), so those forms passed the filter and reached `fw_setenv` as 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-env` pins the key but passes the value unfiltered for `omnect_extra_bootargs` and `omnect_validate_extra_bootargs`, so the `adu` user could write files as root — via a script file (`-s`) or an attacker-chosen config (`-c`), the latter not covered by a `-s`/`--script` blocklist 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. --------- Signed-off-by: Jan Zachmann <50990105+JanZachmann@users.noreply.github.com>
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
bootloader_env.sh set(u-boot variant) passes--tofw_setenvinstead of comparingkey and value against a list of known flags. Same for
unsetandget, whose key reachesfw_setenv/fw_printenvthe same way.Also updates omnect-device-service to 0.45.1, which carries the same fix for its own wrapper
sudo/fw_setenv_no_script.sh. Recipe regenerated with cargo-bitbake; the crate set did not change.Reason
The blocklist covered
-s,--script,-s=*and--script=*.fw_setenv(libubootenv,
src/fw_printenv.c) parses its arguments withgetopt_longand theoptstring
Vc:f:s:nhm:, 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 is enough.
recipes-azure-iot/iot-hub-device-update/iot-hub-device-update/adu-bootloader-envpins the key but passes the value unfiltered for
omnect_extra_bootargsandomnect_validate_extra_bootargs, so theaduuser could write files as root — via ascript file (
-s) or an attacker-chosen config (-c), the latter not covered by a-s/--scriptblocklist at all.This is the same defect that was fixed in omnect-device-service's
fw_setenv_no_script.sh(omnect/omnect-device-service#208); this script is the second copy of it. Both copies ship in the same image, so the ods pin belongs here — otherwise the image keeps the unfixed wrapper.