hypervisor: reject config-injection chars in controller-supplied fields

[europaul]

Description

The QEMU (-readconfig) and Xen (xl) config files that domainmgr generates are line-oriented: every value sits on its own key = "value" line and a new [section]/directive begins on a fresh line. Many controller-supplied strings are rendered verbatim into these files with no escaping — boot args, kernel/ramdisk/device-tree/bootloader paths, VNC password, domain name, disk file location/WWN/vdev, bridge/vif names, device-tree and iomem list entries, and the PCI address / IRQ / ioports / serial / USB attributes of every passthrough device assigned to the domain.

A field containing a newline therefore lets a malicious controller terminate the intended directive and smuggle in entirely new config lines — for example an extra vfio-pci host-device passthrough or a debug backend that exposes guest memory — none of which EVE ever authorized. This is a config injection.

This PR adds a single validation choke point (validateDomainConfig) invoked at the top of both the KVM and Xen CreateDomConfig, before any config is written, that checks every string reaching either config file and rejects newline, carriage-return and NUL — the characters that actually enable a new-directive break-out. To keep the check from rotting as the config templates evolve, it aims to cover the full set of rendered strings rather than a hand-picked subset:

• Numeric and enum fields (memory, vcpus, disk format, PCI/disk/SATA IDs) and the already-parsed MAC address cannot carry these characters and are skipped.
• The free-form cloud-init payload is skipped too, because it legitimately contains newlines and is written to a separate cloud-init image, not this config file.
• Quotes and commas are not rejected — they are legitimate in kernel command lines and file paths and, confined to a single line, cannot introduce a new directive on their own.

How to test and validate this PR

Covered by an automated unit test (pkg/pillar/hypervisor/kvm_injection_test.go): it renders a real QEMU config with a malicious ExtraArgs value that tries to break out of the append="..." line and inject an extra [device "injected"] vfio-pci section, and asserts the injected section is no longer emitted (the build is now refused).

Run it with:

make -C pkg/pillar test
# or, inside `make shell`:
go test -tags kvm ./hypervisor/ -run TestCreateDomConfigInjection -v

Manual check: configure an app whose boot args (or any path field) contain a newline via the controller; domainmgr refuses to build the domain config and logs refusing to build domain config for <name>: invalid character ... instead of writing a config file with attacker-controlled extra directives.

Changelog notes

Hardened the hypervisor against a malicious controller injecting unauthorized QEMU/Xen configuration (e.g. extra device passthrough) through newline characters in app configuration fields. No user-facing changes for legitimate configurations.

PR Backports

• 16.0-stable: To be backported.
• 14.5-stable: To be backported.
• 13.4-stable: To be backported.

Checklist

• I've provided a proper description

• I've added the proper documentation

• I've tested my PR on amd64 device

• I've tested my PR on arm64 device

• I've written the test verification instructions

• I've set the proper labels to this PR

• I've checked the boxes above, or I've provided a good reason why I didn't
  check them.

[codecov]

Codecov Report

❌ Patch coverage is 64.51613% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 23.01%. Comparing base (40d245c) to head (e2b9301).
⚠️ Report is 46 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/pillar/hypervisor/config_validate.go	65.51%	11 Missing and 9 partials ⚠️
pkg/pillar/hypervisor/xen.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6208      +/-   ##
==========================================
+ Coverage   22.61%   23.01%   +0.40%     
==========================================
  Files         507      518      +11     
  Lines       93223    95001    +1778     
==========================================
+ Hits        21084    21869     +785     
- Misses      70413    71213     +800     
- Partials     1726     1919     +193     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[eriknordmark]

Can you ask claude to be more succinct in comments across the board?

[europaul]

Yes, it's very annoying

[eriknordmark]

Less verbose here as well please.

Also, with the fix the demonstration should be that it can NOT feed in extra stuff, right?

[europaul]

of course :)

[eriknordmark]

Run tests

[eriknordmark]

But it does rot/go stale if we add a new field in DomainConfig which gets rendered into a new field in the QEMU/Xen config, right?

If all we care about is CR/LF/NUL in the strings in the DomainConfig, can't we build a more general validator at that level using golang's ability to do a deep walk of that structure and look for things declared as strings?
If things like the cloud init userdata appears as a string we'd need to add an exception to allow CR/LF in there, but what's in DomainConfig is presumably a base64 encoded encrypted blob and the userdata file we put in the ISO filesystem and meta-data server is the decrypted output. My point is that the number of exceptions/skips is very few - might be zero.

[europaul]

Here is my honest opinion on these checks: we are risking more than we might gain from them.

With every check we add we are risking making a DevConfig that's currently in use unusable. On the other side we gain some protection against a malevolent controller. But isn't our security model that the controller has full control over the devices? That the controller should be able to deploy the apps as it wants them? If the controller is truly malicious then I think it would be way easier to mess with EVE just by pushing a malicious EVE update.

That's why I think that the checks are nice-to-have - they protect EVE against mistakes in DevConfig and they cover most fields that need to be covered. That's why I don't think we should take this "rot" too seriously.
Adding a check for all string fields might destroy more than we gain: also when someone adds a new field in the future that needs a line break and then wonders why it doesn't work.

I want us to clarify our security model before we decide how complex we want to make the DevConfig validator.

[eriknordmark]

See my comment - this needs some design-level review and care.