fix(sudo): block all fw_setenv options, not only -s/--script

[JanZachmann]

Summary

Pass -- to fw_setenv so key and value are always treated as data, instead of comparing them against a list of known flags.

Reason

The wrapper blocked -s, --script, -s=* and --script=*. fw_setenv (libubootenv 0.3.5, src/fw_printenv.c:76) parses its arguments with getopt_long and the optstring Vc:f:s:nhm:, which accepts attached values (-sFILE) and abbreviations (--scr=FILE), so those forms passed the filter and reached fw_setenv as options. -c (attacker-chosen config) was not covered by the blocklist at all. GNU getopt also keeps scanning after the first positional argument, so the value position is enough. With --, all three arrive as positional data instead.

The sudoers rules (sudo/omnect-device-service-uboot) pin the key but pass the value unfiltered for factory-reset, omnect_extra_bootargs and omnect_validate_extra_bootargs. There is no live path from the twin into those values today, so this is defense in depth, not a fix for an exploitable escalation:

• factory-reset: the value is serde_json::to_string(&cmd)? (src/twin/factory_reset.rs:259) and always starts with {, so getopt cannot read it as an option. The preserve topics are validated against factory_reset_keys() first.
• omnect_extra_bootargs / omnect_validate_extra_bootargs: the value is merge_bootargs() over /boot/omnect_extra_bootargs_omnect and /boot/omnect_extra_bootargs_custom (src/twin/firmware_update/mod.rs:490-497) — files from the update image, not twin properties.

What the wrapper does have to hold is the argument count. sudo matches command line arguments as one concatenated string (man 5 sudoers, "Wildcards in command arguments"), so a rule like fw_setenv_no_script.sh omnect_extra_bootargs * also matches extra arguments. [[ $# -ne 2 ]] rejects those; -- covers what arrives as key or value.

tests/fw_setenv_wrapper.rs pins the argument contract against a stub fw_setenv, so dropping -- later fails the test. Two cases are worth naming: a value with spaces stays one argument, which is the shape the sudo concatenation rule is about; and an empty value must still be forwarded, because without the quotes around $VALUE the call would become fw_setenv -- KEY, a variable delete that only the separate unset: rules are meant to do. libubootenv's own parsing stays out of scope — only a real fw_setenv can show it, which needs a device test.

This is the repo's first tests/ target, and the documented lint command does not reach it. project-context.md now lists cargo clippy --features mock --all-targets -- -D warnings alongside the existing one, and records why it has to be mock: the self dev-dependency in Cargo.toml turns that feature on, so pairing --all-targets with a bootloader feature gives the build script both features at once and trips its mutual-exclusion check. The same file gains an entry for the tests/ directory and a Test location constraint, since until now every test lived in a #[cfg(test)] mod next to the code.

The wrapper's own contract is unchanged: two arguments, script mode not permitted — it is now structurally impossible rather than filtered, so the file name still applies and no caller, sudoers rule or recipe needs a change.

[JanZachmann]

FYI: @mlilien

[JoergZeidler]

The change itself is right and I would take it. Two things need a fix before merge, both outside the diff.

1. The Reason section overstates the exposure

Those values come from the twin, so a crafted value could write files as root

The sudoers part is correct, the twin part is not, for all three keys:

• factory-reset: the value is serde_json::to_string(&cmd)? (src/twin/factory_reset.rs:259). It always starts with {, so getopt can never read it as an option. The preserve topics are validated against factory_reset_keys() before that.
• omnect_extra_bootargs / omnect_validate_extra_bootargs: the value is merge_bootargs() over /boot/omnect_extra_bootargs_omnect and /boot/omnect_extra_bootargs_custom (src/twin/firmware_update/mod.rs:490-497, src/twin/firmware_update/common.rs:67-79). Those are files on the boot partition from the update image, not twin properties.

So there is no live twin to root-write path today. The change is still worth doing as defense in depth, but please reword the section so it does not read as an exploitable escalation. Related: whoever can write /boot/omnect_extra_bootargs_custom already controls the kernel command line, which is worse than fw_setenv -c.

2. sudo/omnect-device-service-uboot:1-2 is now wrong

# note: use /usr/bin/fw_setenv_no_script.sh wrapper instead of fw_setenv for setting.
#       it checks if the value string includes script parameters

The wrapper no longer checks the value. The PR says no sudoers rule needs a change, which is true for the rules, but this comment does.

3. sudo/fw_setenv_no_script.sh:9 (outside the diff, so no inline comment)

usage() still prints Script-file mode is not permitted., but it is now only reached on a wrong argument count. Before, script mode had its own error message. Drop the line, or keep only the Usage: line.

Verification I ran

I reproduced the getopt table with a C program using the same optstring and long options under glibc getopt_long - all three rows match. I also ran the wrapper against a stub fw_setenv: two arguments give -- KEY VALUE, zero, one or three or more give usage and exit 1. cargo fmt -- --check and cargo audit pass.

Not verified: libubootenv 0.3.5, src/fw_printenv.c:76 and the optstring itself - I only checked that this optstring behaves as claimed, not that fw_setenv uses it. cargo clippy and cargo test did not run here (azure-iot-sdk-dev.pc missing); no Rust changed, so low risk.

Minor, no change needed: the file name and FW_SETENV_NO_SCRIPT_BIN now describe a subset of what the wrapper does. Keeping the name to avoid a recipe change is fine, just noting the drift is permanent.

Signed-off-by: Joerg Zeidler joerg.zeidler@conplement.de

[JanZachmann]

Checked all three items, all correct.

1. Reason section — reworded

Verified both claims:

• factory-reset: value is serde_json::to_string(&cmd)? over the FactoryResetCommand struct (src/twin/factory_reset.rs:54-57, :259), so it always starts with {.
• bootargs: grepped every bootloader_env::set/unset caller. OMNECT_EXTRA_BOOTARGS and OMNECT_VALIDATE_EXTRA_BOOTARGS are only written from merge_bootargs() over the two /boot/omnect_extra_bootargs_* files, or from fw_printenv output in finalize_bootargs(). No twin property reaches any of them.

The section now says defense in depth and lists both reasons. I kept the sudoers part and added what is actually load-bearing: sudo matches command line arguments as one concatenated string (man 5 sudoers, "Wildcards in command arguments"), so fw_setenv_no_script.sh omnect_extra_bootargs * also matches extra arguments — [[ $# -ne 2 ]] rejects those, -- covers what arrives as key or value.

The same wrong claim is in f35f3c1's commit message ("so a twin-supplied value could exploit this"). That commit is pushed, so I left it as is rather than rewriting the branch — the corrected wording lives in the PR body.

2. sudo/omnect-device-service-uboot:1-2

Already fixed in 6c6c064 — your review was submitted against 1d6b663, one commit earlier. Current text:

# note: use /usr/bin/fw_setenv_no_script.sh wrapper instead of fw_setenv for setting.
#       it ends option parsing, so key and value can never become fw_setenv options

3. usage() script-file line

Dropped in 9af2b93. Grepped for the string first — no doc, recipe or test refers to it (tests/fw_setenv_wrapper.rs only asserts Usage:).

Verification

cargo fmt -- --check, cargo clippy --features bootloader_grub -- -D warnings and cargo test --features mock (177 tests, including the 4 wrapper tests) pass here.

On the name drift (fw_setenv_no_script.sh, FW_SETENV_NO_SCRIPT_BIN): agreed, keeping it to avoid a recipe change.

[JoergZeidler]

All three items are addressed, and the reworded Reason section matches the code - I re-checked both claims.

Verified this round

• Replayed every case from tests/fw_setenv_wrapper.rs against the current wrapper with a stub fw_setenv: the four option-like values, the option-like key and all four wrong argument counts behave as asserted.
• man 5 sudoers, "Wildcards in command arguments": "Command line arguments are matched as a single, concatenated string." The new comment on line 13 states this correctly.
• tempfile = "3.27" is already in [dev-dependencies] and src/lib.rs exists, so the integration test links.
• The relative path in WRAPPER_SRC matches how the existing tests reach testfiles/, so no change needed there.
• cargo fmt -- --check passes. cargo clippy and cargo test still cannot run on my side (azure-iot-sdk-dev.pc missing) - taking your run for those.

One item left, plus two inline

project-context.md does not mention the new tests/ directory. Section 3 lists testfiles/ and src/twin/mod_test.rs, and until now every test lived in a #[cfg(test)] mod inside the file under test. This PR opens a second location for tests that have no Rust module to live in; please add a line so the next contributor finds it.

On the commit message of f35f3c1

No action needed. The repository allows squash merge only (allow_merge_commit and allow_rebase_merge are both off), so the PR body becomes the commit message on main and the old wording never lands there. Leaving the branch as is was the right call.

Signed-off-by: Joerg Zeidler joerg.zeidler@conplement.de

[JoergZeidler]

Both suggestions applied verbatim, both test cases in. I pulled the libubootenv sources this round, which also closes the item I had listed as unverified in my first review - details in the test thread.

One item to correct, two small ones inline

Note that root CLAUDE.md carries a generated copy of the same section and is currently stale for an unrelated reason (it still shows the pre-split src/twin/feature/mod.rs entry). That drift predates this PR, so I left it out of scope.

That is not what I see. CLAUDE.md is gitignored (.gitignore:10) and untracked, and diffing section 3 of my generated copy against project-context.md gives exactly one difference - the line this PR adds:

17a18
> - `tests/` — integration tests for things with no Rust module to live in, …

Both files carry the src/twin/feature/mod.rs entry identically, so there is no older drift. The drift is the one this PR creates, and it clears with a local setup-ai.sh run. Nothing to commit since the file is ignored, but the "predates this PR" reasoning does not hold - worth knowing before the same argument is used to skip a regeneration that does matter.

Verified this round

• src/fw_printenv.c:77 on master: char *options = "Vc:f:s:nhm:"; - the optstring in the PR body is correct.
• Replayed both new cases against the wrapper: console=ttyS0 root=/dev/sda2 arrives as one argument, "" arrives as [].
• cargo fmt -- --check passes. cargo clippy and cargo test still cannot run on my side (azure-iot-sdk-dev.pc missing) - taking your run for those.

Signed-off-by: Joerg Zeidler joerg.zeidler@conplement.de

[JanZachmann]

Both inline items applied verbatim in a6d29b2.

On the CLAUDE.md claim

You are right that "predates this PR" was the wrong reasoning, and it is worth pinning down why our measurements differ.

My local copy really does carry the pre-split entry — but because it is old, not because the repo drifted:

$ stat -c %y CLAUDE.md
2026-07-09 13:53:29 +0200

Section 3 of my CLAUDE.md against project-context.md gives three differences, not one:

< - `src/twin/feature/mod.rs` — `Feature` trait definition, `Command` enum (all dispatchable operations), `CommandRequest` types
---
> - `src/twin/feature/mod.rs` — `Feature` trait definition and `DynFeature` (re-exports from `command.rs` and `fs_watcher.rs`)
> - `src/twin/feature/command.rs` — `Command` enum, `CommandRequest` types, `parse_payload` helper, `interval_stream`
> - `src/twin/feature/fs_watcher.rs` — centralized `FsWatcher` (inotify-based, per-watch debounce, oneshot support)

The feature/ split landed in 762ae5f, after my last setup-ai.sh run, so my copy never picked it up. Yours is freshly generated, which is why you only see the line this PR adds.

The conclusion is yours, though: since CLAUDE.md is gitignored (.gitignore:10), per-checkout staleness is expected and says nothing about the repo. There is no repo-level drift to point at, so it was not a reason to skip anything — the correct reason is simply that the file is not tracked. I will not use that argument again.

One thing I could not verify

cargo clippy --features bootloader_grub --all-targets does not build in this repo — the build script's compile_error! for the mutually exclusive bootloader features fires when the build script target is compiled:

error: Either feature 'bootloader_grub' xor 'bootloader_uboot' xor 'mock' must be enabled.

I confirmed this is pre-existing by stashing my changes and re-running it on dc51128. So the documented lint command (cargo clippy --features bootloader_grub, no --all-targets) never covered the new tests/ target. cargo clippy --features mock --tests -- -D warnings does cover it and passes. Not fixing the build script here — flagging it since this PR is what introduces a target the documented command misses.

Verification

cargo fmt -- --check, cargo clippy --features bootloader_grub -- -D warnings, cargo clippy --features mock --tests -- -D warnings and cargo test --features mock pass: 173 unit + 6 wrapper tests, 179 total.

running 6 tests
test empty_value_is_forwarded_instead_of_deleting ... ok
test key_and_value_are_forwarded_behind_double_dash ... ok
test option_like_key_stays_data ... ok
test option_like_value_stays_data ... ok
test value_with_spaces_stays_one_argument ... ok
test wrong_argument_count_is_rejected ... ok

[JanZachmann]

Correction to my previous comment: the evidence I gave for the --all-targets failure being pre-existing was not evidence.

I wrote that I confirmed it by stashing my changes and re-running on dc51128. But dc51128 already contains tests/ — the stash only dropped two uncommitted doc edits, so that run could not tell "pre-existing" apart from "caused by the new test target".

Re-checked properly on 1d6b663, the last commit before 020e922 added tests/:

$ git worktree add --detach /tmp/wt 1d6b663 && cd /tmp/wt
$ ls tests
ls: cannot access 'tests': No such file or directory
$ cargo clippy --features bootloader_grub --all-targets -- -D warnings
error: could not compile `omnect-device-service` (build script) due to 1 previous error

The conclusion holds, and the mechanism is now clear: --all-targets lints the build script as a target, and cargo does not set feature cfgs for build scripts (they get CARGO_FEATURE_* env vars instead), so the #[cfg(not(any(feature = ...)))] guard in src/build.rs is always true there and its compile_error! fires. src/build.rs is byte-identical on 1d6b663, so nothing about this branch is involved.

That also means --all-targets was never usable in this repo, so no lint coverage was lost — it just never existed for a target type the repo did not have until now. cargo clippy --features mock --tests -- -D warnings covers the new target and passes.

[JoergZeidler]

The --all-targets mechanism does not hold up

The corrected evidence is better, but the explanation still points at the wrong cause. I could not run it in the repo (azure-iot-sdk-dev.pc missing here), so I built a crate that mirrors the structure: build = "src/build.rs", the same cfg-guarded compile_error!, lib plus bin, a tests/ target, an optional dependency behind the feature, edition 2024, toolchain 1.93.0.

$ cargo clippy --features grub --all-targets -v | grep -- --crate-name build_script_build
--cfg 'feature="default"'
--cfg 'feature="grub"'

Cargo does pass the feature cfgs when compiling the build script, including under clippy --all-targets, and the whole run with -D warnings passes. The error text only appears when the feature is left off. So "cargo does not set feature cfgs for build scripts" is not what is happening on this toolchain.

What the pasted output shows is only the last line:

error: could not compile `omnect-device-service` (build script) due to 1 previous error

That is also the line you get when --all-targets lints src/build.rs and -D warnings promotes a clippy warning to an error - src/build.rs:29-33 has three .unwrap() calls. One command tells the two apart:

cargo clippy --features bootloader_grub --all-targets     # no -D warnings

If that passes, it was a lint, not the compile_error!. The line directly above the error: in your original run would settle it too.

This matters for the conclusion, not for the branch: "--all-targets was never usable, so no lint coverage was lost" only follows if the cause really is the build script guard.

project-context.md:73,76 - the lint commands do not cover tests/

This part of your finding is right, and I confirmed it in the same crate: with a warning in tests/it.rs, cargo clippy --features grub -- -D warnings passes and --tests fails. This PR adds the first tests/ target and already edits project-context.md, so the entry belongs here rather than in a follow-up.

Use the mock variant, not bootloader_grub --all-targets: the unit tests in src/** need mock, for example clear_mock() is gated #[cfg(all(not(any(feature = "bootloader_grub", feature = "bootloader_uboot")), test))] (src/bootloader_env/mod.rs:72-76) and does not exist otherwise.

- **Lint:** `cargo clippy --features bootloader_grub -- -D warnings` and `cargo clippy --features mock --all-targets -- -D warnings`
- **Pre-commit check:** `cargo fmt && cargo clippy --features bootloader_grub -- -D warnings && cargo clippy --features mock --all-targets -- -D warnings`

cargo fmt -- --check passes.

Signed-off-by: Joerg Zeidler joerg.zeidler@conplement.de

[JanZachmann]

You were right that my mechanism was wrong. It is not a promoted clippy lint either — it is feature unification.

Not -D warnings

Your discriminating command, run here:

$ cargo clippy --features bootloader_grub --all-targets
error: Either feature 'bootloader_grub' xor 'bootloader_uboot' xor 'mock' must be enabled.
  --> src/build.rs:17:5

Still fails without -D warnings, so the .unwrap() calls in src/build.rs:29-33 are not it.

The line number is the tell

src/build.rs:17 is not the "no feature set" guard — that one is at line 9. Line 17 belongs to:

#[cfg(all(feature = "bootloader_grub", feature = "mock"))]
compile_error!(...);

Both features are set at once. Your -v check was sound, it just could not show this, because the cause is in our Cargo.toml, not in cargo's build-script handling:

[dev-dependencies]
omnect-device-service = { path = ".", features = ["mock"] }   # Cargo.toml:86

A self dev-dependency that turns mock on. Once --all-targets brings dev-dependencies into the graph, feature unification gives the crate both bootloader_grub (from the command line) and mock (from that entry), and the build script sees the pair:

$ cargo clippy --features bootloader_grub --all-targets -v | grep -- --crate-name build_script_build
--cfg 'feature="bootloader_grub"' --cfg 'feature="default"' --cfg 'feature="mock"'

$ cargo clippy --features bootloader_grub -v | grep -- --crate-name build_script_build
--cfg 'feature="bootloader_grub"' --cfg 'feature="default"'

So cargo does pass the feature cfgs to build scripts, exactly as your crate showed. My sentence about CARGO_FEATURE_* was wrong and I have dropped it.

Still pre-existing: Cargo.toml:86 is unchanged on 1d6b663, and --all-targets fails there at the same src/build.rs:17:5.

--features mock --all-targets is the right command

Confirmed, and your reason for preferring mock holds — clear_mock() is gated #[cfg(all(not(any(feature = "bootloader_grub", feature = "bootloader_uboot")), test))] at src/bootloader_env/mod.rs:72-76. It also sidesteps the conflict above, since mock unified with mock is no conflict.

Both suggestions applied verbatim in 7b05a21. I added one thing you did not ask for, in section 4 next to the feature-flag constraint, because "use mock, not bootloader_grub" is otherwise unexplainable:

`Cargo.toml` declares a self dev-dependency with `features = ["mock"]`, so any
command covering test targets already pulls `mock` in — pair `--all-targets`
with `mock`, or `src/build.rs` rejects the combination.

Test location

Applied verbatim. Counted before doing so: 15 files with an inline mod tests, and src/twin/mod_test.rs the only *_test.rs — 15 to 1, as you said.

Ran here

cargo fmt -- --check, cargo clippy --features bootloader_grub -- -D warnings, cargo clippy --features mock --all-targets -- -D warnings, cargo test --features mock (173 unit + 6 wrapper) all pass.

I also dropped the Verification section from the PR description. Under squash merge the body becomes the commit message on main, and a list of green commands does not belong there — verification stays in these comments instead. The two facts from it that are about the change moved into Reason.