cli: dbus: inhibit property path traversal in DBus property access by Sinan-Karakaya · Pull Request #163 · canonical/fpgad

Sinan-Karakaya · 2026-03-26T10:22:50Z

Summary

This PR fixes a path traversal gap in DBus property access, which could lead to execution of unwanted executables. More details can be found in #88

The solution was to forbid the usage of '..' in paths, by adding the function validate_property_path

Copilot

Pull request overview

This PR closes a path traversal gap in daemon D-Bus property access by introducing a shared validator, and expands automated coverage across daemon (unit + universal integration) and CLI parsing/helpers.

Changes:

Add validate_property_path() in the daemon D-Bus module and reuse it in read/write property methods.
Add universal daemon test cases that assert traversal paths are rejected for read/write property operations.
Refactor CLI status parsing and property-path building into helper functions with new unit tests and CLI parse tests.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`daemon/tests/universal/status/read_property.rs`	Adds a universal integration case asserting traversal paths are rejected for read_property.
`daemon/tests/universal/control/write_property.rs`	Adds a universal integration case asserting traversal paths are rejected for write_property.
`daemon/tests/universal/control/set_bytes.rs`	Adds a universal integration case asserting traversal paths are rejected for write_property_bytes.
`daemon/src/comm/dbus/control_interface.rs`	Switches property writes to use the shared `validate_property_path()` guard.
`daemon/src/comm/dbus.rs`	Introduces `validate_property_path()` and unit tests; routes `fs_read_property()` through the validator.
`cli/src/status.rs`	Extracts string parsing into helpers and adds unit tests for parsing behavior.
`cli/src/set.rs`	Extracts sysfs property path construction into `build_property_path()` and adds unit tests.
`cli/src/main.rs`	Adds CLI argument parsing tests for several subcommands and error cases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cli/src/set.rs

artiepoole · 2026-03-26T10:49:58Z

This looks like it solves #162 but it does not address #88 fully. #88 expects all functions in system_io.rs to be tested e.g.

write to a file and read it back - check bytes and strings are handled
ensure that write fails if create isn't enabled, etc.
check that the correct error type is reported
test extract_path_and_filename

I'll update the issue to reflect this expectation

artiepoole

Looking good so far, just a few thoughts

cli/src/status.rs

cli/src/set.rs

cli/src/status.rs

daemon/src/comm/dbus.rs

artiepoole · 2026-03-26T15:43:00Z

Please add docstrings to all new functions following the style used in the (currently being merged) PRs such as this one ^.^

Sorry for any merge conflicts as a result of #149 #148 or #145

Sinan-Karakaya · 2026-03-27T09:22:28Z

Please add docstrings to all new functions following the style used in the (currently being merged) PRs such as this one ^.^

Sorry for any merge conflicts as a result of #149 #148 or #145

All done now :)

artiepoole

Sorry for not spotting the &Path thing earlier 🫣

daemon/src/comm/dbus.rs

cli/src/set.rs

daemon/src/comm/dbus.rs

artiepoole · 2026-03-27T11:03:47Z

Note: the failing test is a lab related error, not a problem with the code

+ sudo env DEBIAN_FRONTEND=noninteractive apt install build-essential -y
  
  WARNING: apt does not have a stable CLI interface. Use with caution in scripts.
  
  Waiting on output...
  E: Unable to acquire the dpkg frontend lock (/var/lib/dpkg/lock-frontend), is another process using it?

artiepoole · 2026-03-27T12:15:25Z

Ah I didn't anticipate this. The fpga flags variable is a symlink and therefore the test fails

  stderr:	[2026-03-27T11:46:41Z ERROR cli] org.freedesktop.DBus.Error.InvalidArgs: FpgadError::Argument: Cannot access property /sys/class/fpga_manager/fpga0/flags: resolved path /sys/devices/platform/firmware:zynqmp-firmware/firmware:zynqmp-firmware:pcap/fpga_manager/fpga0/flags is outside /sys/class/fpga_manager
  Error: MethodError(OwnedErrorName("org.freedesktop.DBus.Error.InvalidArgs"), Some("FpgadError::Argument: Cannot access property /sys/class/fpga_manager/fpga0/flags: resolved path /sys/devices/platform/firmware:zynqmp-firmware/firmware:zynqmp-firmware:pcap/fpga_manager/fpga0/flags is outside /sys/class/fpga_manager"), Msg { type: Error, serial: 38, sender: UniqueName(":1.59"), reply-serial: 4, body: Str, fds: [] })

also the snap test isn't running. This is an error which then results in the test showing passed even though it failed. I wonder how long this has been a problem @talhaHavadar 👀

./tests/coverage_test.sh: line 20: RUSTFLAGS: unbound variable

artiepoole · 2026-03-27T12:45:43Z

Attempting to fix the test issues in #165

edit - merged

…rage This change hardens property path handling and expands test coverage across daemon integration tests and CLI unit tests. Security and behavior changes: - Added shared property path validation in the DBus layer to ensure paths are inside /sys/class/fpga_manager. - Explicitly rejects any parent traversal segments such as .. in property paths. Applied the shared validator to: - read_property - write_property - write_property_bytes Test coverage changes: - Added daemon integration test cases for traversal attempts in: - write_property - write_property_bytes - read_property Added daemon unit tests for the new property-path validator. Added CLI parser tests for representative command forms and invalid invocation. Added CLI unit tests for: - platform/overlay line parsing helpers - property path construction helper Validation: cargo test -p cli passed (9 tests). cargo test --workspace --no-run succeeded. Signed-off-by: Sinan KARAKAYA <sinan.karakaya.canonical.com>

Revert commit 605af48, removed tests as they were out of scopes for the PR. Signed-off-by: Sinan KARAKAYA <sinan.karakaya.canonical.com>

Refactored build_property_path to return a Result<String, zbus::Error>. Added sanitize_segment to check that device_handle and attribute are not containing any illegal statements ('..', root dir etc...). Added tests containing illegal statements. Signed-off-by: Sinan KARAKAYA <sinan.karakaya.canonical.com>

Added helper functions and tests in status.rs to ensure that we check that the correct error type is returned and formatted correctly. Signed-off-by: Sinan KARAKAYA <sinan.karakaya.canonical.com>

Added validate_property_path_with_base to canonicalize the property path and reject paths that escape FPGA_MANAGERS_DIR (including symlink escapes). Signed-off-by: Sinan KARAKAYA <sinan.karakaya.canonical.com>

Signed-off-by: Sinan KARAKAYA <sinan.karakaya.canonical.com>

Changed validate_property_path{_with_base} parameter property_path_str to property_path of type &Path Signed-off-by: Sinan KARAKAYA <sinan.karakaya.canonical.com>

validate_property_path_with_base switched canonicalize to absolute path resolve, to ignore symlinks. Signed-off-by: Sinan KARAKAYA <sinan.karakaya.canonical.com>

artiepoole

LGTM, thank you Sinan!

Sinan-Karakaya requested a review from artiepoole March 26, 2026 10:22

Sinan-Karakaya self-assigned this Mar 26, 2026

Copilot AI review requested due to automatic review settings March 26, 2026 10:22

github-actions bot added the cli label Mar 26, 2026

github-actions bot requested a review from talhaHavadar March 26, 2026 10:23

Copilot started reviewing on behalf of Sinan-Karakaya March 26, 2026 10:23 View session

Sinan-Karakaya added the daemon label Mar 26, 2026

Sinan-Karakaya linked an issue Mar 26, 2026 that may be closed by this pull request

tests: daemon: the write_property methods are not checking paths safely #162

Closed

github-actions bot removed the daemon label Mar 26, 2026

Copilot AI reviewed Mar 26, 2026

View reviewed changes

cli/src/set.rs Outdated Show resolved Hide resolved

Sinan-Karakaya changed the title ~~Fix property path traversal in DBus property access; add daemon and CLI tests~~ Draft: Fix property path traversal in DBus property access Mar 26, 2026

Sinan-Karakaya marked this pull request as draft March 26, 2026 11:16

artiepoole reviewed Mar 26, 2026

View reviewed changes

cli/src/status.rs Show resolved Hide resolved

cli/src/set.rs Show resolved Hide resolved

cli/src/status.rs Show resolved Hide resolved

daemon/src/comm/dbus.rs Outdated Show resolved Hide resolved

Sinan-Karakaya requested a review from artiepoole March 26, 2026 15:17

Sinan-Karakaya marked this pull request as ready for review March 26, 2026 15:19

Sinan-Karakaya changed the title ~~Draft: Fix property path traversal in DBus property access~~ Fix property path traversal in DBus property access Mar 26, 2026

artiepoole changed the title ~~Fix property path traversal in DBus property access~~ cli: dbus: fix property path traversal in DBus property access Mar 26, 2026

artiepoole changed the title ~~cli: dbus: fix property path traversal in DBus property access~~ cli: dbus: restrict property path traversal in DBus property access Mar 26, 2026

artiepoole changed the title ~~cli: dbus: restrict property path traversal in DBus property access~~ cli: dbus: inhibit property path traversal in DBus property access Mar 26, 2026

Sinan-Karakaya force-pushed the sinan/integration-test-for-system-io branch 2 times, most recently from 083537a to 833bbd3 Compare March 27, 2026 09:39

artiepoole reviewed Mar 27, 2026

View reviewed changes

daemon/src/comm/dbus.rs Outdated Show resolved Hide resolved

cli/src/set.rs Show resolved Hide resolved

daemon/src/comm/dbus.rs Outdated Show resolved Hide resolved

github-actions bot requested a review from artiepoole March 27, 2026 11:17

Sinan-Karakaya added 7 commits March 30, 2026 09:11

cli: removed tests

f1be222

Revert commit 605af48, removed tests as they were out of scopes for the PR. Signed-off-by: Sinan KARAKAYA <sinan.karakaya.canonical.com>

cli: added tests that fails in status.rs

b707621

Added helper functions and tests in status.rs to ensure that we check that the correct error type is returned and formatted correctly. Signed-off-by: Sinan KARAKAYA <sinan.karakaya.canonical.com>

dbus: validate_property_path now resolve and validate full path

e0a2f0e

Added validate_property_path_with_base to canonicalize the property path and reject paths that escape FPGA_MANAGERS_DIR (including symlink escapes). Signed-off-by: Sinan KARAKAYA <sinan.karakaya.canonical.com>

cli: added docstring comments to new functions

6b4c4b5

Signed-off-by: Sinan KARAKAYA <sinan.karakaya.canonical.com>

dbus: changed validate_property_... parameters from &str to &Path

0ce9b45

Changed validate_property_path{_with_base} parameter property_path_str to property_path of type &Path Signed-off-by: Sinan KARAKAYA <sinan.karakaya.canonical.com>

dbus: removed symlink resolve

aff8ba3

validate_property_path_with_base switched canonicalize to absolute path resolve, to ignore symlinks. Signed-off-by: Sinan KARAKAYA <sinan.karakaya.canonical.com>

Sinan-Karakaya force-pushed the sinan/integration-test-for-system-io branch from 6f54abe to aff8ba3 Compare March 30, 2026 07:12

artiepoole approved these changes Mar 30, 2026

View reviewed changes

artiepoole enabled auto-merge (squash) March 30, 2026 07:51

artiepoole merged commit 3030564 into main Mar 30, 2026
9 checks passed

artiepoole deleted the sinan/integration-test-for-system-io branch March 30, 2026 08:02

Conversation

Sinan-Karakaya commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

artiepoole commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artiepoole left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artiepoole commented Mar 26, 2026

Uh oh!

Sinan-Karakaya commented Mar 27, 2026

Uh oh!

artiepoole left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artiepoole commented Mar 27, 2026

Uh oh!

artiepoole commented Mar 27, 2026

Uh oh!

artiepoole commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artiepoole left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Sinan-Karakaya commented Mar 26, 2026 •

edited

Loading

artiepoole commented Mar 26, 2026 •

edited

Loading

artiepoole commented Mar 27, 2026 •

edited

Loading