Review for Source Package: rust-hwlib
[Summary]
This Rust binary ("hwctl") queries the https://hw.ubuntu.com endpoint for
specific
CPU and hardware info, to check if the system runs on a certified device. It's
an Ubuntu/Canonical only solution. The output is structured JSON data, to be
consumed by "Ubuntu Pro". It's actively maintained in upstream/GitHub, but not
yet packaged in the archives. The Rust application code itself is fairly small
(~1k LoC), but comes with a huge pile of vendored crates (~1M LoC).
MIR team ACK under the constraint to resolve the below listed
required TODOs and as much as possible having a look at the
recommended TODOs.
This does need a security review, so I'll assign ubuntu-security
List of specific binary packages to be promoted to main: hwctl
Specific binary packages built, but NOT to be promoted to main: <None>
Notes:
#0 - Flagging for security review, due to parsing of system/device data and
parsing of HTTP responses from a centralized (trusted?) endpoint,
hw.ubuntu.com which can easily be modified at runtime via HW_API_URL
Required TODOs:
#1 - The binary is expected to run as root, but only needs access to specific
system data. Consider adding an apparmor profile for restricting it to the
mandatory data access. (I'm unclear about this, could potentially be
downgraded to a Recommended TODO.)
#2 - The autopkgtests is just a re-execution of the unit tests, including
re-compile, so not testing the actual, installed binaries. Adding a simple
integration tests, using a mock HTTP server to simulate hw.ubuntu.com and
make a test request using the real binary would be very valuable.
#3 - Please explain the release (upstream) release process, I see "Revision XXX"
as releases on GitHub, but this package (the hwlib client) uses versions
like v0.9.0 which are not referenced upstream.
#4 - Please explain the arch:amd64 limitation. I see arm64/rpi4b8g in test data
but the package is not built for arm64 or any other arch besides amd64.
Doesn't the Pro client need the data for devices of any architecture?
Recommended TODOs:
#5 - The package should get a team bug subscriber before being promoted
#6 - d/vendor-rust.sh: Consider using "cargo vendor-filterer" with the following
arguments "--platform '*-*-linux-gnu' --platform '*-*-linux-gnueabi'" to
avoid unnecessary windows crates
#7 - d/rules: Consider using a VENDOR_TARBALL for crates, see "vendor-tarball"
target in this example makefile:
https://github.com/canonical/ubuntu-mir/blob/main/vendoring/Rust.md#automation-via-debianrules
#8 - Consider fixing some lintian warnings, especially those (see below for more
warnings and details):
W: hwctl: synopsis-too-long
I: rust-hwlib source: override_dh_auto_test-does-not-check-DEB_BUILD_OPTIONS
[debian/rules:28]
P: rust-hwlib source: uses-debhelper-compat-file [debian/compat]
#9 - Consider fixing some build-time warnings, especially this one (see below
for more warnings and details):
warning: use of deprecated method `std::error::Error::description`: use the
Display impl or to_string()
[Rationale, Duplication and Ownership]
- There is no other package in main providing the same functionality.
=> Custom Canonical solution for checking hardware certification status.
- A team is committed to own long term maintenance of this package.
=> ~canonical-hw-cert
- The rationale given in the report seems valid and useful for Ubuntu.
[Dependencies]
OK:
- no other Dependencies to MIR due to this
- SRCPKG checked with `check-mir`
- all dependencies can be found in `seeded-in-ubuntu` (already in main)
- none of the (potentially auto-generated) dependencies (Depends
and Recommends) that are present after build are not in main
- no -dev/-debug/-doc packages that need exclusion
- No dependencies in main that are only superficially tested requiring
more tests now.
Problems: None
[Embedded sources and static linking]
OK:
- not a go package, no extra constraints to consider in that regard
- does not have unexpected Built-Using entries
- Rust package that has all dependencies vendored. It does neither
have *Built-Using (after build). Nor does the build log indicate
built-in sources that are missed to be reported as Built-Using.
- rust package using dh_cargo (dh ... --buildsystem cargo)
- Includes vendored code, the package has documented how to refresh this
code at ./README.md
Problems:
- embedded source present (vendored Rust dependencies)
- Rust, using static linking of the final binary
[Security]
OK:
- history of CVEs does not look concerning (new application, Jan 2024)
- does not run a daemon as root
- does not use webkit1,2 (references in
rust-vendor/vcpkg/../boost-build_1.67.0_x64-windows.list)
- does not use lib*v8 directly
- does not process arbitrary web content
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)
- does not deal with security attestation (secure boot, tpm, signatures)
Problems:
- does use centralized online accounts (services actually):
https://hw.ubuntu.com
- does parse data formats (cpuinfo.rs, hardware_info.rs, HW_API_URL) from
an untrusted source.
- does deal with cryptography (validation of signatures in JSON responses)
- does not expose any external endpoint (but utilizes "socket2" create via
"tokio" & "hyper-util")
- this makes appropriate (for its exposure) use of established risk
mitigation features (apparmor)
=> The binary is expected to run as root, but only needs access to specific
system data. Consider adding an apparmor profile for restricting it to the
mandatory data access.
[Common blockers]
OK:
- does not FTBFS currently
- does have a test suite that runs at build time
- test suite fails will fail the build upon error.
- This does not need special HW for build or test
- no new python2 dependency
- Not a Python package
- Not a Go package
Problems:
- does have a test suite that runs as autopkgtest, but it's just a re-execution
of the unit tests, including re-compile, so not testing the actual, installed
binaries. Adding a simple integration tests, using a mock HTTP server to
simulate hw.ubunut.com and make a test request using the real binary would
be very valuable.
[Packaging red flags]
OK:
- Ubuntu does carry a delta, but it is reasonable and maintenance under
control (Ubuntu only package)
- symbols tracking not applicable for this kind of code.
- debian/watch is not present but also not needed (e.g. native)
- Upstream update history is good
- promoting this does not seem to cause issues for MOTUs that so far
maintained the package
- debian/rules is rather clean (not clean, but OKish. As it needs to deal with
Rust vendoring)
- It is not on the lto-disabled list
Problems:
- Debian/Ubuntu update history is sporadic (not yet packaged)
- d/control: Architecture: amd64 (but raspi as certified device in tests)
- d/vendor-rust.sh: use cargo vendor-filterer --platform '*-*-linux-gnu'
--platform '*-*-linux-gnueabi' to avoid windows crates
- d/rules: consider using a VENDOR_TARBALL for crates, see "vendor-tarball"
target in this example makefile:
https://github.com/canonical/ubuntu-mir/blob/main/vendoring/Rust.md#automation-via-debianrules
- the current release is packaged
=>Yes, but it does not match the releases/Revisions of the Github project
- lintian warnings:
X: librust-hwlib-dev: package-contains-no-arch-dependent-files
I: rust-hwlib source: override_dh_auto_test-does-not-check-DEB_BUILD_OPTIONS
[debian/rules:28]
N: The debian/rules file for this package has an override_dh_auto_test target
N: that does not appear to check DEB_BUILD_OPTIONS against nocheck.
W: hwctl: synopsis-too-long
N: The first line of the "Description:" must be less than 80 characters long.
P: rust-hwlib source: uses-debhelper-compat-file [debian/compat]
N: debhelper has replaced debian/compat with the debhelper-compat virtual
N: package for most circumstances, e.g. Build-Depends: debhelper-compat (= 13)
I: hwctl: hardening-no-fortify-functions
[Upstream red flags]
OK:
- no Errors/warnings during the build
- no incautious use of malloc/sprintf (the language has no direct MM)
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH (usage is OK inside tests)
- no use of user nobody
- no use of setuid / setgid
- no important open bugs (crashers, etc) in Debian or Ubuntu
- no dependency on webkit, qtwebkit or libseed
- not part of the UI for extra checks
- no translation present, but none needed for this case (user visible)?
Problems:
- some usage of malloc/sprintf in rust-vendor/ (libc, openssl, ring/crypto,
pyo3-ffi)
- manpage: BUGS: Only amd64 architecture is currently supported.
- warnings during build:
warning: unexpected `cfg` condition value: ...
warning: `...` (lib) generated XX warning
warning: elided lifetime has a name
warning: trait `AssertSync` is never used
warning: ambiguous wide pointer comparison, the comparison includes metadata
which may not be expected
warning: lint `illegal_floating_point_literal_pattern` has been removed: no
longer a warning, float patterns behave the same as `==`
warning: use of deprecated method `std::error::Error::description`: use the
Display impl or to_string()
warning: unused import
** Changed in: Ubuntu Plucky
Status: New => Incomplete
** Changed in: Ubuntu Plucky
Status: Incomplete => Confirmed
** Changed in: Ubuntu Plucky
Assignee: Lukas Märdian (slyon) => Ubuntu Security Team (ubuntu-security)
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/2072561
Title:
[MIR] rust-hwlib
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+bug/2072561/+subscriptions
--
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
