Insufficient length validation on TPKT packets

https://github.com/citronneur/rdp-rs/blob/b4804d15f29556a5592803f6c0d98dd6051c558f/src/core/tpkt.rs#L124-L158

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 https://github.com/nccgroup/scrying/issues/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

citronneur / rdp-rs