Skip to content

Validate fixed PDU lengths and encoded item length overflow - #799

Draft
atlex00 wants to merge 2 commits into
Enet4:masterfrom
atlex00:fix/ul-pdu-length-validation
Draft

atlex00 wants to merge 2 commits into
Enet4:masterfrom
atlex00:fix/ul-pdu-length-validation

Conversation

@atlex00

@atlex00 atlex00 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

read_pdu did not enforce the required PDU-length value of 4 for A-ASSOCIATE-RJ, A-RELEASE-RQ, A-RELEASE-RP, and A-ABORT PDUs. Declared lengths below 4 already failed field-length validation, but lengths above 4 could be accepted: the decoder parsed the defined four bytes and discarded the additional bytes included in the declared PDU body. The chunk writers also converted encoded payload lengths with truncating as u16/as u32 casts, so payloads exceeding the corresponding length field could produce a truncated length value while the complete payload was written.

DICOM PS3.8 specifies a PDU-length of 00000004H for these PDUs (§9.3.4, §9.3.6, §9.3.7, §9.3.8) and defines variable PDU and item lengths as the number of bytes in their following content. This change rejects any other declared length for those fixed PDUs, replaces the length casts with checked conversions that report overflow, and adds the corresponding read and write error variants.

@atlex00
atlex00 force-pushed the fix/ul-pdu-length-validation branch from 3205ae4 to 5d1e32f Compare June 22, 2026 23:49
@atlex00
atlex00 marked this pull request as ready for review June 22, 2026 23:54
@atlex00
atlex00 marked this pull request as draft June 29, 2026 11:11
@atlex00

This comment was marked as resolved.

@atlex00
atlex00 force-pushed the fix/ul-pdu-length-validation branch 2 times, most recently from 15fd94d to 056ca0d Compare June 29, 2026 12:06
@atlex00
atlex00 marked this pull request as ready for review June 29, 2026 21:48
@Enet4 Enet4 added A-lib Area: library C-ul Crate: dicom-ul labels Jun 30, 2026
@atlex00
atlex00 force-pushed the fix/ul-pdu-length-validation branch from 056ca0d to 5bbfa3b Compare August 2, 2026 20:48
@atlex00
atlex00 changed the base branch from main to master August 2, 2026 20:49
@Enet4
Enet4 self-requested a review August 3, 2026 11:49
@Enet4 Enet4 added C-storescp Crate: dicom-storescp A-tool Area: tooling C-storescu Crate: dicom-storescu labels Aug 10, 2026

@Enet4 Enet4 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your pull request. Can you look into the two concerns inline?

Comment thread storescu/src/main.rs
Comment on lines +57 to +58
#[arg(long)]
reject_trailing_fixed_pdu_bytes: bool,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option could be shortened a bit. How about this?

Suggested change
#[arg(long)]
reject_trailing_fixed_pdu_bytes: bool,
#[arg(long = "no-trailing-fixed-pdu-bytes")]
reject_trailing_fixed_pdu_bytes: bool,

// }
// Recieved a valid TLS message, means the server expects TLS
return super::TlsNotSupportedSnafu.fail();
return Err(e);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that this fixes a false positive in which a client would erroneously claim to be communicating with a TLS server? We can incorporate that, but this either needs to be documented differently in the PR, or split into a separate one.

@atlex00
atlex00 force-pushed the fix/ul-pdu-length-validation branch from 2d30262 to 252ef74 Compare August 25, 2026 15:57
@atlex00
atlex00 force-pushed the fix/ul-pdu-length-validation branch from 252ef74 to 50c0c08 Compare August 25, 2026 16:02
@atlex00
atlex00 marked this pull request as draft August 25, 2026 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-lib Area: library A-tool Area: tooling C-storescp Crate: dicom-storescp C-storescu Crate: dicom-storescu C-ul Crate: dicom-ul

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants