Insufficient length validation on TPKT packets

Lines 124 to 158 in b4804d1

    
           pub fn read(&mut self) -> RdpResult<Payload> { 
        
               let mut buffer = Cursor::new(self.transport.read(2)?); 
        
               let mut action: u8 = 0; 
        
               action.read(&mut buffer)?; 
        
               if action == Action::FastPathActionX224 as u8 { 
        
                   // read padding 
        
                   let mut padding: u8 = 0; 
        
                   padding.read(&mut buffer)?; 
        
                   // now wait extended header 
        
                   buffer = Cursor::new(self.transport.read(2)?); 
        
                   let mut size = U16::BE(0); 
        
                   size.read(&mut buffer)?; 
        
                   // now wait for body 
        
                   Ok(Payload::Raw(Cursor::new(self.transport.read(size.inner() as usize - 4)?))) 
        
               } else { 
        
                   // fast path 
        
                   let sec_flag = (action >> 6) & 0x3; 
        
                   let mut short_length: u8 = 0; 
        
                   short_length.read(&mut buffer)?; 
        
                   if short_length & 0x80 != 0 { 
        
                       let mut hi_length: u8 = 0; 
        
                       hi_length.read(&mut Cursor::new(self.transport.read(1)?))?; 
        
                       let length :u16 = ((short_length & !0x80) as u16) << 8; 
        
                       let length = length | hi_length as u16; 
        
                       Ok(Payload::FastPath(sec_flag, Cursor::new(self.transport.read(length as usize - 3)?))) 
        
                   } 
        
                   else { 
        
                       Ok(Payload::FastPath(sec_flag, Cursor::new(self.transport.read(short_length as usize - 2)?))) 
        
                   } 
        
                } 
        
           }

Hi, I've found a couple of panics in the TPKT parsing code that happen when the TPKT header specifies a length of zero. I've observed this happening due to network errors over in nccgroup/scrying#26 and have come up with some local reproductions.

Here are the test cases I used:

#[cfg(test)]
mod repro {
    use rdp::core::tpkt::Client;
    use rdp::model::link::{Link, Stream};
    use std::io::Cursor;

    fn process(data: &[u8]) {
        let cur = Cursor::new(data.to_vec());
        let link = Link::new(Stream::Raw(cur));
        let mut client = Client::new(link);
        let _ = client.read();
    }

    #[test]
    fn case_1() {
        let buf = b"\x00\x00\x03\x00\x00\x00";
        process(buf);
    }

    #[test]
    fn case_2() {
        let buf = b"\x00\x80\x00\x00\x00\x00";
        process(buf);
    }

    #[test]
    fn case_3() {
        let buf = b"\x03\xe8\x00\x00\x80\x00";
        process(buf);
    }
}

Analysis

First case

0000 0300 0000

---- repro::case_1 stdout ----
thread 'repro::case_1' panicked at 'attempt to subtract with overflow', /home/david/gits/rdp-rs-sciguy16/src/core/tpkt.rs:155:80
stack backtrace:
   0: rust_begin_unwind
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/std/src/panicking.rs:495:5
   1: core::panicking::panic_fmt
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/panicking.rs:92:14
   2: core::panicking::panic
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/panicking.rs:50:5
   3: rdp::core::tpkt::Client<S>::read
             at /home/david/gits/rdp-rs-sciguy16/src/core/tpkt.rs:155:80
   4: rdp_afl::repro::process
             at ./src/main.rs:32:17
   5: rdp_afl::repro::case_1
             at ./src/main.rs:38:9
   6: rdp_afl::repro::case_1::{{closure}}
             at ./src/main.rs:36:5
   7: core::ops::function::FnOnce::call_once
             at /home/david/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
   8: core::ops::function::FnOnce::call_once
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/ops/function.rs:227:5

action = 0
-> fast path branch on line 142
short_length = 0
-> else branch on 154
Panic on line 155 short_length as usize - 2 subtraction with overflow. In release mode this attempts to allocate a 2^64-2 byte buffer to read into, which fails

Second case

0080 0000 0000

---- repro::case_3 stdout ----
thread 'repro::case_3' panicked at 'attempt to subtract with overflow', /home/david/gits/rdp-rs-sciguy16/src/core/tpkt.rs:152:80
stack backtrace:
   0: rust_begin_unwind
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/std/src/panicking.rs:495:5
   1: core::panicking::panic_fmt
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/panicking.rs:92:14
   2: core::panicking::panic
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/panicking.rs:50:5
   3: rdp::core::tpkt::Client<S>::read
             at /home/david/gits/rdp-rs-sciguy16/src/core/tpkt.rs:152:80
   4: rdp_afl::repro::process
             at ./src/main.rs:32:17
   5: rdp_afl::repro::case_3
             at ./src/main.rs:50:9
   6: rdp_afl::repro::case_3::{{closure}}
             at ./src/main.rs:48:5
   7: core::ops::function::FnOnce::call_once
             at /home/david/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
   8: core::ops::function::FnOnce::call_once
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/ops/function.rs:227:5

action = 0
-> fast path branch on line 142
short_length = 0x80
0x80 & 0x80 != 0 (line 147)
hi_length = 0
length = (short_length & !0x80) | hi_length = 0
Panic on line 152 length as usize - 3 subtraction with overflow. In release mode this attempts to allocate a 2^64-3 byte buffer to read into, which fails

Third case

03e8 0000 8000

---- repro::case_4 stdout ----
thread 'repro::case_4' panicked at 'attempt to subtract with overflow', /home/david/gits/rdp-rs-sciguy16/src/core/tpkt.rs:140:61
stack backtrace:
   0: rust_begin_unwind
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/std/src/panicking.rs:495:5
   1: core::panicking::panic_fmt
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/panicking.rs:92:14
   2: core::panicking::panic
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/panicking.rs:50:5
   3: rdp::core::tpkt::Client<S>::read
             at /home/david/gits/rdp-rs-sciguy16/src/core/tpkt.rs:140:61
   4: rdp_afl::repro::process
             at ./src/main.rs:32:17
   5: rdp_afl::repro::case_4
             at ./src/main.rs:56:9
   6: rdp_afl::repro::case_4::{{closure}}
             at ./src/main.rs:54:5
   7: core::ops::function::FnOnce::call_once
             at /home/david/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
   8: core::ops::function::FnOnce::call_once
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/ops/function.rs:227:5

action = 0x03
FastPathActionX224 branch on line 128
padding = 0xe8
buffer = [0x00, 0x00]
size = 0
Panic on line 140 size.inner() as usize - 4 subtraction with overflow. In release mode this attempts to allocate a 2^64-4 byte buffer to read into, which fails

According to the MS documentation the minimum length of a TPKT packet should be 7, and adding a check for this should resolve the issue here.

Excerpt from [2], section 8:

Since an X.224 TPDU occupies at least 3 octets, the minimum value of TPKT length is 7. The maximum value is 65535. This permits a maximum TPDU size of 65531 octets.

Refs:
[1] https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/18a27ef9-6f9a-4501-b000-94b1fe3c2c10
[2] https://www.itu.int/rec/T-REC-T.123-200701-I/en

Thanks, check and test was added.
Thanks a lot!

Looks good! Thanks for fixing it :)

	pub fn read(&mut self) -> RdpResult<Payload> {
	let mut buffer = Cursor::new(self.transport.read(2)?);
	let mut action: u8 = 0;
	action.read(&mut buffer)?;
	if action == Action::FastPathActionX224 as u8 {

	// read padding
	let mut padding: u8 = 0;
	padding.read(&mut buffer)?;
	// now wait extended header
	buffer = Cursor::new(self.transport.read(2)?);

	let mut size = U16::BE(0);
	size.read(&mut buffer)?;

	// now wait for body
	Ok(Payload::Raw(Cursor::new(self.transport.read(size.inner() as usize - 4)?)))

	} else {
	// fast path
	let sec_flag = (action >> 6) & 0x3;
	let mut short_length: u8 = 0;
	short_length.read(&mut buffer)?;
	if short_length & 0x80 != 0 {
	let mut hi_length: u8 = 0;
	hi_length.read(&mut Cursor::new(self.transport.read(1)?))?;
	let length :u16 = ((short_length & !0x80) as u16) << 8;
	let length = length \| hi_length as u16;
	Ok(Payload::FastPath(sec_flag, Cursor::new(self.transport.read(length as usize - 3)?)))
	}
	else {
	Ok(Payload::FastPath(sec_flag, Cursor::new(self.transport.read(short_length as usize - 2)?)))
	}
	}
	}