fix(pdu,graphics): harden ClearCodec and fix shortVBarYOff interpretation

Security hardening informed by FreeRDP ClearCodec CVEs (GHSA-3frr,
GHSA-32q9, CVE-2026-26955, CVE-2026-23531) and three rounds of
targeted code review.

Fixes:
- Correct shortVBarYOff interpretation: 6-bit field is an absolute
  end-row offset, not a pixel count. Pixel count = yOff - yOn per
  MS-RDPEGFX 2.2.4.1.1.2.1.1.3. (Round 2)
- Validate shortVBarYOff >= shortVBarYOn and <= band_height (Round 2)
- Cap residual run_length iterations to output buffer size (Round 1)
- Cap decoder allocation to 8192x8192 pixels max (Round 3)
- Validate glyph index range 0-3999 per spec 2.2.4.1 (Round 2)
- Cap encoder input to available pixel data (Round 1)

Add integration tests in ironrdp-testsuite-core (35 tests) covering
codec round-trip, adversarial input (DoS, OOM, malformed streams),
bands layer compositing, cache state, compression quality, and
multi-frame session simulation. Inline unit tests (42 tests) in the
PDU and graphics crates cover wire format decode/encode.

Author: glamberson

crates/ironrdp-graphics/src/clearcodec/mod.rs

@@ -44,18 +44,25 @@ impl ClearCodecDecoder {
         let mut src = ReadCursor::new(data);
         let stream = ClearCodecBitmapStream::decode(&mut src)?;
 
-        // Validate sequence number
-        if stream.seq_number != self.expected_seq {
-            // Per spec, sequence mismatch means we should reset state.
-            // In practice, some servers restart sequences so we tolerate it.
-        }
+        // Per spec (2.2.4.1 seqNumber): must increment by 1, wrapping 0xFF -> 0x00.
+        // Mismatches indicate packet loss or server restart. We tolerate them
+        // (FreeRDP does too) but re-sync to the received sequence number.
+        // ironrdp-graphics has no tracing dependency, so callers that need
+        // diagnostics should track sequence numbers externally.
         self.expected_seq = stream.seq_number.wrapping_add(1);
 
         // Handle cache reset
         if stream.is_cache_reset() {
             self.vbar_cache.reset();
         }
 
+        // Validate glyph index range per spec: 0..3999 inclusive
+        if let Some(idx) = stream.glyph_index {
+            if idx >= GLYPH_CACHE_WRAP {
+                return Err(invalid_field_err!("glyphIndex", "glyph index out of range 0-3999"));
+            }
+        }
+
         let w = usize::from(width);
         let h = usize::from(height);
         let pixel_count = w
@@ -74,6 +81,15 @@ impl ClearCodecDecoder {
             return Ok(entry.pixels.clone());
         }
 
+        // Cap allocation to prevent OOM from adversarial dimensions.
+        // EGFX surfaces are capped at 32767x32767 by the spec, but a single
+        // ClearCodec tile should never approach that. 8192x8192 (256MB) is a
+        // generous upper bound for any reasonable tile size.
+        const MAX_DECODE_PIXELS: usize = 8192 * 8192;
+        if pixel_count > MAX_DECODE_PIXELS {
+            return Err(invalid_field_err!("dimensions", "pixel count exceeds decoder maximum"));
+        }
+
         // Decode composite payload
         let mut output = vec![0u8; pixel_count * 4];
 
@@ -109,19 +125,25 @@ impl ClearCodecDecoder {
     ) -> DecodeResult<()> {
         let w = usize::from(width);
 
-        // Layer 1: Residual (BGR RLE) - fills the entire output
+        // Layer 1: Residual (BGR RLE) - fills the entire output.
+        // Cap pixel writes to the output buffer size to prevent CPU-spin DoS
+        // from adversarial run_length values (FreeRDP CVE GHSA-32q9-m5qr-9j2v).
         if !composite.residual_data.is_empty() {
             let segments = decode_residual_layer(composite.residual_data)?;
+            let max_offset = output.len();
             let mut offset = 0;
             for seg in &segments {
-                for _ in 0..seg.run_length {
-                    if offset + 3 < output.len() {
-                        output[offset] = seg.blue;
-                        output[offset + 1] = seg.green;
-                        output[offset + 2] = seg.red;
-                        output[offset + 3] = 0xFF; // Alpha
-                        offset += 4;
-                    }
+                let pixels_remaining = (max_offset.saturating_sub(offset)) / 4;
+                let effective_run = u32::try_from(pixels_remaining).unwrap_or(u32::MAX).min(seg.run_length);
+                for _ in 0..effective_run {
+                    output[offset] = seg.blue;
+                    output[offset + 1] = seg.green;
+                    output[offset + 2] = seg.red;
+                    output[offset + 3] = 0xFF; // Alpha
+                    offset += 4;
+                }
+                if offset >= max_offset {
+                    break;
                 }
             }
         }
@@ -295,9 +317,11 @@ impl ClearCodecDecoder {
                 }
             }
             SubcodecId::NsCodec => {
-                // NSCodec: deferred to Phase A7. For now, treat as opaque.
-                // This is a valid conformance approach: the server can always
-                // choose to use Raw or RLEX subcodecs instead.
+                // NSCodec (MS-RDPNSC) decode not yet implemented.
+                // Encoder avoids generating NSCodec tiles, so this path only
+                // triggers when decoding streams from other implementations.
+                // The region is left at its current pixel values (transparent
+                // or whatever the residual layer filled in).
             }
         }
 
@@ -439,6 +463,10 @@ fn bgra_to_run_segments(bgra: &[u8], pixel_count: usize) -> Vec<RgbRunSegment> {
         return Vec::new();
     }
 
+    // Cap to the number of complete pixels actually present in the input
+    let available_pixels = bgra.len() / 4;
+    let pixel_count = pixel_count.min(available_pixels);
+
     let mut segments = Vec::new();
     let mut i = 0;
 

crates/ironrdp-pdu/src/codecs/clearcodec/bands.rs

@@ -58,9 +58,9 @@ pub enum VBar<'a> {
 /// Inline short V-bar data from a cache miss.
 #[derive(Debug, Clone)]
 pub struct ShortVBarCacheMiss<'a> {
-    /// First pixel row within the band where color data starts.
+    /// First pixel row within the band where color data starts (shortVBarYOn).
     pub y_on: u8,
-    /// Number of pixel rows with color data (6 bits, max 52).
+    /// Number of pixel rows with color data (`shortVBarYOff - shortVBarYOn`).
     pub y_off_delta: u8,
     /// Raw BGR pixel data: `y_off_delta * 3` bytes.
     pub pixel_data: &'a [u8],
@@ -124,7 +124,7 @@ fn decode_single_band<'a>(src: &mut ReadCursor<'a>) -> DecodeResult<Band<'a>> {
     })
 }
 
-fn decode_vbar<'a>(src: &mut ReadCursor<'a>, _band_height: u16) -> DecodeResult<VBar<'a>> {
+fn decode_vbar<'a>(src: &mut ReadCursor<'a>, band_height: u16) -> DecodeResult<VBar<'a>> {
     ensure_size!(ctx: "VBar", in: src, size: 2);
     let first_word = src.read_u16();
 
@@ -143,19 +143,32 @@ fn decode_vbar<'a>(src: &mut ReadCursor<'a>, _band_height: u16) -> DecodeResult<
     }
 
     // Both top bits clear: short V-bar cache miss
-    // first_word encodes: yOn (high 8 bits of the 14-bit field) and yOff delta (low 6 bits)
-    // Per spec: top byte = yOn, low 6 bits = pixel count
-    // Top 2 bits are clear (checked above), so first_word <= 0x3FFF and (first_word >> 6) <= 0xFF
+    // Per MS-RDPEGFX 2.2.4.1.1.2.1.1.3 (SHORT_VBAR_CACHE_MISS):
+    //   bits 13:6 = shortVBarYOn (8 bits): row where Short V-Bar begins
+    //   bits 5:0  = shortVBarYOff (6 bits): row where Short V-Bar ends
+    // Pixel count = shortVBarYOff - shortVBarYOn
     let y_on = u8::try_from(first_word >> 6).expect("top 2 bits are clear, so shifted value fits in u8");
-    let y_off_delta = u8::try_from(first_word & 0x3F).expect("masked to 6 bits, always fits in u8");
+    let y_off = u8::try_from(first_word & 0x3F).expect("masked to 6 bits, always fits in u8");
 
-    let pixel_byte_count = usize::from(y_off_delta) * 3;
+    if y_off < y_on {
+        return Err(invalid_field_err!("shortVBarCacheMiss", "shortVBarYOff < shortVBarYOn"));
+    }
+
+    if u16::from(y_off) > band_height {
+        return Err(invalid_field_err!(
+            "shortVBarCacheMiss",
+            "shortVBarYOff exceeds band height"
+        ));
+    }
+
+    let pixel_count = y_off - y_on;
+    let pixel_byte_count = usize::from(pixel_count) * 3;
     ensure_size!(ctx: "ShortVBarCacheMiss", in: src, size: pixel_byte_count);
     let pixel_data = src.read_slice(pixel_byte_count);
 
     Ok(VBar::ShortCacheMiss(ShortVBarCacheMiss {
         y_on,
-        y_off_delta,
+        y_off_delta: pixel_count,
         pixel_data,
     }))
 }
@@ -195,10 +208,10 @@ mod tests {
 
     #[test]
     fn decode_vbar_short_cache_miss() {
-        // Both top bits clear: yOn=2 (shifted left 6), pixel_count=3
+        // Both top bits clear: y_on=2, y_off=5, pixel_count = y_off - y_on = 3
         let y_on: u16 = 2;
-        let pixel_count: u16 = 3;
-        let first_word = (y_on << 6) | pixel_count;
+        let y_off: u16 = 5;
+        let first_word = (y_on << 6) | y_off;
         let mut data = Vec::new();
         data.extend_from_slice(&first_word.to_le_bytes());
         // 3 pixels * 3 bytes = 9 bytes BGR data
@@ -208,7 +221,7 @@ mod tests {
         match vbar {
             VBar::ShortCacheMiss(miss) => {
                 assert_eq!(miss.y_on, 2);
-                assert_eq!(miss.y_off_delta, 3);
+                assert_eq!(miss.y_off_delta, 3); // pixel_count = y_off - y_on = 5 - 2 = 3
                 assert_eq!(miss.pixel_data.len(), 9);
             }
             _ => panic!("expected ShortCacheMiss"),