Skip to content

pldm-common: bound wire version-string lengths by the destination - #21

Open
alanhc wants to merge 1 commit into
OpenPRoT:mainfrom
alanhc:fix/ver-str-length-bounds
Open

alanhc wants to merge 1 commit into
OpenPRoT:mainfrom
alanhc:fix/ver-str-length-bounds

Conversation

@alanhc

@alanhc alanhc commented Sep 10, 2026

Copy link
Copy Markdown

Fixes #11.

What

The version-string length fields are u8 values taken off the wire (0..=255),
but they index a fixed PLDM_FWUP_IMAGE_SET_VER_STR_MAX_LEN (32) byte
destination. Each decode site validated only the source buffer, never the
destination, so a peer reporting a length above 32 with a long enough
payload panics the decoder. This is remotely reachable on the firmware update
path.

Each decode site now rejects an out-of-range length with
PldmCodecError::InvalidData, leaving BufferTooShort to mean what it says:
the buffer really was too short.

Six sites, not four

The issue lists four. Sweeping every copy_from_slice in the crate turned up
two more with the identical shape, both of which panic on main:

# Site In issue?
1 RequestUpdateRequest::decode (request_update.rs:129) yes
2 RequestUpdateRequest::get_comp_image_set_ver_str (request_update.rs:83) yes
3 FirmwareParameters::decode, active string (get_fw_params.rs:178) yes
4 FirmwareParameters::decode, pending string (get_fw_params.rs:190) yes
5 PldmFirmwareString::decode (protocol/firmware_update.rs:627) no
6 PassComponentTableRequest::decode (pass_component.rs:122) no

Site 2 is a different bug and takes a different fix. It called
copy_from_slice on the whole 32-byte destination from a source of str_len
bytes, and copy_from_slice requires equal lengths, so it panicked for every
version string that was not exactly 32 bytes. The crate's own test uses a 9-byte
string but never calls the getter, so this stayed green. It now copies only the
prefix, clamped to the destination.

The new() constructors use the same call but are safe as written, because
str_data is always exactly 32 bytes. They are left alone.

Where the destination bound now precedes the copy, the redundant [..str_len]
reslice of the already-sliced source is dropped.

Tests

pldm-common/tests/ver_str_bounds.rs, 10 tests covering all six sites.

Tests that pass on patched code prove nothing on their own, so each one was run
against the reverted source. Seven fail there, at exactly the reported lines:

request_update.rs:129   request_update.rs:83   request_update.rs:84
get_fw_params.rs:178    get_fw_params.rs:190
firmware_update.rs:627  pass_component.rs:122

The remaining three pass both before and after by design; they are the
no-regression guards, including a round trip over every length from 0 to 32.

Clean build: cargo fmt --check clean, cargo clippy --all-targets --all-features -- -D warnings clean, 103 tests pass.

One open question

The issue does not say which error variant an over-long length should produce.
I chose InvalidData on the grounds that it is malformed input rather than a
truncated buffer. If you prefer BufferTooShort, it is a one-word change in six
places, happy to switch.

AI disclosure

Written with Claude Code. I directed the work, and reviewed every line of the
diff and the tests. The six sites were confirmed by executing the reproductions,
and the regression tests were verified to fail against the unpatched source
before being accepted. I did not find an AI policy in this repo; disclosing by
default in case you want one.

The version-string length fields are u8 values taken off the wire (0..=255),
but they index a fixed PLDM_FWUP_IMAGE_SET_VER_STR_MAX_LEN (32) byte
destination. Each decode site validated only the source buffer, never the
destination, so a peer reporting a length above 32 with a long enough payload
panicked the decoder. This is remotely reachable on the firmware update path.

Reject an out-of-range length with PldmCodecError::InvalidData at each decode
site: RequestUpdateRequest, PassComponentTableRequest, FirmwareParameters
(both the active and the pending string) and PldmFirmwareString.

RequestUpdateRequest::get_comp_image_set_ver_str is a separate case. It called
copy_from_slice on the whole 32-byte destination from a source of str_len
bytes, and copy_from_slice requires equal lengths, so it panicked for every
version string that was not exactly 32 bytes long. The crate's own test uses a
9-byte string but never calls the getter, so this stayed green. Copy only the
prefix, clamped to the destination.

Where the destination bound now precedes the copy, the redundant [..str_len]
reslice of the already-sliced source is dropped.

Add tests/ver_str_bounds.rs covering all six sites. Each new test panics
against the unpatched code at the reported line and passes after the fix;
round-trip tests over every length from 0 to 32 guard against regressing the
valid path.

Fixes: OpenPRoT#11

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@linux-foundation-easycla

linux-foundation-easycla Bot commented Sep 10, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: alanhc / name: Hung-Chun Tseng (cf17335)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wire-controlled version-string length is not bounded by the 32-byte destination: 4 panic sites in firmware_update decode

1 participant