fix(playstation): Add compression limit - #6211
Conversation
| } | ||
|
|
||
| let decoder = lz4_flex::frame::FrameDecoder::new(Cursor::new(bytes)); | ||
| let mut decoder = decoder.take(limit.saturating_add(1) as u64); |
There was a problem hiding this comment.
Mimics the existing logic here:
relay/relay-server/src/endpoints/minidump.rs
Line 178 in 84e052a
| let data = relay_prosperoconv::extract_data(&prosperodump.payload()).map_err(|err| { | ||
| ProcessingError::InvalidPlaystationDump(format!("Failed to extract data: {err}")) | ||
| })?; | ||
| let data = uncompress( |
There was a problem hiding this comment.
Did extract_data previously do the compression? If so, wouldn't it make more sense to pass a limit parameter into extract_data, so it can not be abused from (hypothetical) other call sites?
There was a problem hiding this comment.
Yes as pointed out in the above comment the old logic is here. At the time it was written it felt like putting it there made sense, but now I don't really see a point of having it outside of relay. This code here is the only call side so would remove it afterwards.
…playstation-compression-limit
| # Video size exceeds attachment size limits - we expect this to be ignored | ||
| video_content = "1" * (len(playstation_dump) + 2 * 1024 * 1024) |
There was a problem hiding this comment.
This test was changed a couple month ago without updating the comments and logic here, the video now gets uploaded to objectstore regardless of the max_attachment_size as such update the test to be less misleading.
Dav1dde
left a comment
Dav1dde
left a comment
There was a problem hiding this comment.
Goal is to then remove it decompression from the prospero crate?
| } | ||
|
|
||
| #[cfg(all(sentry, feature = "processing"))] | ||
| fn uncompress(bytes: Bytes, limit: usize) -> Result<Bytes, CompressionError> { |
There was a problem hiding this comment.
I think this is better suited in utils::playstation.
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| ctx.processing.config.max_attachment_size(), | ||
| ).map_err(ProcessingError::from)?; | ||
| let prospero_dump = relay_prosperoconv::ProsperoDump::parse(&data).map_err(|err| { |
There was a problem hiding this comment.
ProsperoDump::parse has no caller-side depth bound on attacker-controlled data
Attacker-controlled decompressed PlayStation dump data is passed to relay_prosperoconv::ProsperoDump::parse without a visible recursion-depth limit; max_attachment_size bounds memory but not parser depth.
Evidence
prosperodump.payload()at line 66 is attacker-controlled input.uncompress()bounds decompressed output tomax_attachment_sizebytes (line 67), bounding memory but not parsing depth.relay_prosperoconv::ProsperoDump::parse(&data)at line 69 parses the decompressed data without a caller-side depth limit.relay-prosperoconvsource is cloned from thetempestrepository at build time and is not inspectable here.relay-prosperoconvdepends onrmpv, which decodes msgpack recursively; absent a visiblemax_depthcall the parser is vulnerable to stack overflow from a deeply-nested payload.- Precedent in this codebase:
src/utils/rmp.rs:21setsmax_depthon other msgpack deserializers.
Identified by Warden · wrdn-dos-review · B4G-L2V
| use bytes::Bytes; | ||
| #[cfg(all(sentry, feature = "processing"))] |
There was a problem hiding this comment.
PlayStation decompression bounds memory but not post-decompress CPU
The uncompress function added in this PR caps decompressed memory via .take(limit), yet a tiny LZ4 payload can still inflate to max_attachment_size (default 200 MiB) before relay_prosperoconv::ProsperoDump::parse and write_dump process the full buffer with no caller-side time or depth budget, leaving a CPU-amplification vector on the shared worker.
Evidence
relay-server/src/processing/errors/errors/playstation.rs:269definesuncompress(bytes, limit)which useslz4_flex::frame::FrameDecoder::new(...).take(limit.saturating_add(1) as u64), bounding only memory.playstation.rs:67callsuncompress(prosperodump.payload(), ctx.processing.config.max_attachment_size())where the limit defaults to 200 MiB (relay-config/src/config.rs:726).playstation.rs:71passes the full decompressed buffer torelay_prosperoconv::ProsperoDump::parse(&data)with no caller-side depth limit, time budget, or item-count cap.playstation.rs:74then callsrelay_prosperoconv::write_dump(&prospero_dump), again with no CPU/time guard.relay-prosperoconvsource is cloned from an external git repo at build time (relay-prosperoconv/build.rs), so parser recursion/depth bounds cannot be verified statically; treating it as unbounded per fail-safe review policy.- Sibling path
relay-server/src/processing/errors/errors/nswitch.rs:23hardcodesMAX_DECOMPRESSED_SIZE = 100 KiB; the PlayStation path uses the much larger configurablemax_attachment_sizewith no post-decompression CPU guard.
Also found at 1 additional location
relay-server/src/processing/errors/errors/playstation.rs:269
Identified by Warden · wrdn-dos-review · X4Q-XCB
3 participants
Fix: INGEST-994 (More detail in the comments on the ticket).