Skip to content

Harden frame indexing against malformed BOT offsets#36

Merged
ThalesMMS merged 1 commit intocompression-supportfrom
codex/fix-comments-nre8gq
Sep 15, 2025
Merged

Harden frame indexing against malformed BOT offsets#36
ThalesMMS merged 1 commit intocompression-supportfrom
codex/fix-comments-nre8gq

Conversation

@ThalesMMS
Copy link
Owner

Summary

  • enforce monotonic Basic Offset Table offsets and per-frame size limits when building encapsulated frame indexes
  • guard native pixel frame sizing against overflow and oversized frames, adding explicit error cases and regression tests
  • make the performance monitor no-op on platforms without os.signpost so the package continues to build on Linux

Testing

  • swift test (fails on Linux: CoreGraphics module is unavailable)

https://chatgpt.com/codex/tasks/task_e_68c886be19cc832e92473c68ec6c550c

@ThalesMMS
Copy link
Owner Author

@claude

@claude
Copy link

claude bot commented Sep 15, 2025

Claude finished @ThalesMMS's task —— View job


Code Review Complete ✅

I've reviewed the changes in this PR that harden frame indexing against malformed BOT offsets. Overall, these are excellent defensive programming improvements that significantly enhance the robustness of the DICOM frame handling.

Summary

The changes effectively address potential security and stability issues by:

  • Adding proper validation for Basic Offset Table (BOT) offsets
  • Implementing frame size limits to prevent resource exhaustion
  • Guarding against integer overflow in frame size calculations
  • Fixing Linux compatibility for the performance monitor

Detailed Review

FrameIndex.swift - Excellent defensive improvements

Strengths:

  1. Frame size limit (512MB) - Good choice that prevents resource exhaustion while allowing legitimate medical images
  2. BOT validation - The new validateBasicOffsetTable method properly ensures:
    • Offsets are non-negative
    • Offsets are within bounds
    • Offsets are strictly monotonic (increasing)
  3. Overflow protection - Using addingReportingOverflow prevents integer overflow vulnerabilities
  4. Error handling - Clear, specific error cases added (invalidBOTOffsets, frameTooLarge)
  5. 64-bit arithmetic - Using Int64 for intermediate calculations prevents overflow in frame size computation

Minor suggestions:

  • Consider documenting why 512MB was chosen as the max frame size (typical medical image constraints)
  • The validation could potentially log warnings for suspicious but technically valid patterns

DcmSwiftPerformanceMonitor.swift - Clean Linux compatibility fix

Strengths:

  1. Conditional compilation - Properly uses #if canImport for platform-specific features
  2. Graceful degradation - Monitor becomes no-op on Linux rather than failing to compile
  3. Separate checks - Distinguishes between os and os.signpost availability

Implementation quality:

  • Clean separation of platform-specific code
  • No functional changes for supported platforms
  • Maintains the same public API

FrameIndexTests.swift - Comprehensive test coverage

Strengths:

  1. Edge case testing - Tests for non-monotonic BOT offsets
  2. Overflow testing - Validates handling of oversized frames
  3. Error specificity - Tests verify exact error types thrown
  4. Realistic scenarios - Tests cover both valid and malformed DICOM data

Test coverage includes:

  • Invalid BOT offset sequences
  • Frame size exceeding limits
  • Integer overflow scenarios
  • All new error conditions

Security Considerations

The changes effectively mitigate several potential security issues:

  • DoS prevention: Frame size limits prevent memory exhaustion attacks
  • Integer overflow: Proper overflow checking prevents undefined behavior
  • Malformed data: BOT validation prevents processing of corrupted/malicious offsets

Performance Impact

Minimal - the validation adds negligible overhead compared to actual frame processing.

Recommendation

Approve and merge - These changes significantly improve the robustness of the DICOM frame handling without breaking existing functionality. The defensive programming measures are well-thought-out and properly tested.


Great work on hardening the frame indexing! The changes strike a good balance between security and compatibility. 👍

@ThalesMMS ThalesMMS merged commit 6244887 into compression-support Sep 15, 2025
1 check failed
@ThalesMMS ThalesMMS deleted the codex/fix-comments-nre8gq branch September 15, 2025 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant