Skip to content

Conversation

mondain
Copy link
Member

@mondain mondain commented Jul 27, 2025

Fix 1 - Chunk Size Validation in RTMP

  • Constants MIN_CHUNK_SIZE=128 and MAX_CHUNK_SIZE=65536 properly defined
  • Both setReadChunkSize() and setWriteChunkSize() methods validate bounds
  • Throws IllegalArgumentException for invalid sizes

Fix 2 - Type 3 Header Validation in RTMPProtocolDecoder

  • Validates previous header exists before inheritance
  • Detects suspicious Type 3 usage for new messages
  • Proper null check for savedHeader before processing Type 3 headers
  • Removes unnecessary XOR bit manipulation
  • Reads 32-bit extended timestamps directly from the buffer
  • Error logging and null return prevents stream confusion attacks
  • Code structure matches expected security pattern

Fix 3 - Extended Timestamp Rollover in RTMPProtocolEncoder

  • calculateTimestampDelta() method handles 32-bit timestamp rollover correctly
  • RTMPUtils.diffTimestamps deprecated and calls replaced with secure implementation
  • Prevents timestamp corruption during 49.7-day rollover periods

@chushiyun2015
Copy link

chushiyun2015 commented Jul 27, 2025 via email

@mondain mondain requested review from Copilot and Andy--S August 25, 2025 15:38
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements comprehensive RTMP security fixes for Red5 Server to address chunk size validation, Type 3 header validation, and extended timestamp rollover handling. The changes enhance security while maintaining compatibility with popular streaming clients like OBS Studio and ffmpeg.

  • Added chunk size validation with RTMP spec bounds checking and librtmp compatibility
  • Implemented Type 3 header validation to prevent stream confusion attacks with graceful error handling
  • Fixed extended timestamp rollover handling for 32-bit wraparound protection

Reviewed Changes

Copilot reviewed 38 out of 41 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
common/.../codec/RTMP.java Added chunk size constants and validation methods with bounds checking
common/.../codec/RTMPProtocolDecoder.java Enhanced Type 3 header validation and fixed extended timestamp processing
common/.../codec/RTMPProtocolEncoder.java Added calculateTimestampDelta method for rollover handling and chunk size validation
common/.../event/Ping.java Refactored ping types from constants to enum for type safety
common/.../RTMPUtils.java Deprecated diffTimestamps method in favor of secure implementation
tests/.../ExtendedTimestampTest.java Added comprehensive test coverage for extended timestamp scenarios
Multiple test files Updated Ping constant references to use new PingType enum

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +653 to +656
// Validate chunk size according to RTMP spec and librtmp compatibility
// librtmp uses default chunk size of 128, max of 65536
if (chunkSize < 1 || chunkSize > 65536) {
throw new ProtocolException("Invalid chunk size: " + chunkSize + ". Must be between 1 and 65536 for librtmp compatibility.");
Copy link
Preview

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

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

The error message mentions librtmp compatibility but the validation logic in RTMP.java allows values up to 16777215. Consider making the validation consistent between decoder and RTMP class, or updating the error message to reflect the actual validation logic.

Suggested change
// Validate chunk size according to RTMP spec and librtmp compatibility
// librtmp uses default chunk size of 128, max of 65536
if (chunkSize < 1 || chunkSize > 65536) {
throw new ProtocolException("Invalid chunk size: " + chunkSize + ". Must be between 1 and 65536 for librtmp compatibility.");
// Validate chunk size according to RTMP specification (1 to 16777215)
if (chunkSize < 1 || chunkSize > 16777215) {
throw new ProtocolException("Invalid chunk size: " + chunkSize + ". Must be between 1 and 16777215 as per RTMP specification.");

Copilot uses AI. Check for mistakes.

Comment on lines +620 to +622
if (size < 1 || size > 65536) {
log.warn("Invalid chunk size: {}. Clamping to librtmp-compatible range [128, 65536]", size);
size = Math.max(128, Math.min(size, 65536));
Copy link
Preview

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

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

The chunk size clamping logic silently modifies invalid values rather than rejecting them. This could mask legitimate configuration errors. Consider throwing an exception for clearly invalid values while only clamping values in the acceptable range.

Suggested change
if (size < 1 || size > 65536) {
log.warn("Invalid chunk size: {}. Clamping to librtmp-compatible range [128, 65536]", size);
size = Math.max(128, Math.min(size, 65536));
if (size < 128 || size > 65536) {
throw new IllegalArgumentException("Invalid chunk size: " + size + ". Must be in range [128, 65536]");

Copilot uses AI. Check for mistakes.

@mondain mondain merged commit 753198d into main Aug 27, 2025
@mondain mondain deleted the rtmpclient branch August 27, 2025 15:42
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.

3 participants