Skip to content

boundary handling and decompression limits in CompressedRTF#1071

Open
jmestwa-coder wants to merge 1 commit into
apache:trunkfrom
jmestwa-coder:compressed-rtf-boundary-validation
Open

boundary handling and decompression limits in CompressedRTF#1071
jmestwa-coder wants to merge 1 commit into
apache:trunkfrom
jmestwa-coder:compressed-rtf-boundary-validation

Conversation

@jmestwa-coder
Copy link
Copy Markdown
Contributor

Summary

This patch strengthens CompressedRTF boundary handling and decompression validation for both compressed and uncompressed RTF streams.

Changes

  • Enforce maximum decompressed size validation using IOUtils.safelyAllocateCheck.
  • Add bounded output enforcement to prevent expansion beyond the declared decompressed size plus documented protocol padding.
  • Fix uncompressed RTF handling to:
    • copy only the declared payload bytes,
    • stop at the correct stream boundary,
    • and avoid falling through into LZW decompression.
  • Bound compressed input reads to the declared compressed payload size using BoundedInputStream.
  • Add documentation for the MS-OXRTFCP trailing padding behavior.

Tests

Added tests covering:

  • rejection of oversized declared decompressed sizes,
  • rejection of malformed compressed streams expanding beyond allowed padding limits.

@jmestwa-coder jmestwa-coder force-pushed the compressed-rtf-boundary-validation branch from a90480a to fc05283 Compare May 13, 2026 17:22
"\\fmodern \\fscript \\fdecor MS Sans SerifSymbolArialTimes New RomanCourier" +
"{\\colortbl\\red0\\green0\\blue0\n\r\\par \\pard\\plain\\f0\\fs20\\b\\i\\u\\tab\\tx";

private static final int DEFAULT_MAX_RECORD_LENGTH = 50_000_000;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why this length as opposed to some other value?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I mainly wanted a limit that was large enough to avoid rejecting legitimate larger RTF payloads, while still enforcing a reasonable upper bound for malformed or inconsistent size declarations.

The 50MB value was meant as a conservative default rather than a format-defined maximum. Happy to align it with an existing POI-wide limit pattern, or reduce/remove the configurable default if you’d prefer.

@jmestwa-coder jmestwa-coder force-pushed the compressed-rtf-boundary-validation branch from fc05283 to 63cf1a3 Compare May 15, 2026 18:59
// Nope, nothing fancy to do
IOUtils.copy(src, res);
copyCompressedPayload(src, limited);
return;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

don't use return like this - also copyCompressedPayload has its own limit checks, quite untidy to do this

}
}

private static final class LimitedOutputStream extends OutputStream {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

2 participants