dcerpc/udp: Fix pairing of request response

So far, request and response were paired with serial number fields in
the header. This is incorrect. According to
https://pubs.opengroup.org/onlinepubs/9629399/chap12.htm,
"Together, the activity UUID and the sequence number uniquely identify
a remote procedure call."

Hence, add activity uuid and sequence number to the transaction and pair
the request accordingly. Remove incorrect handling of this and fix
tests.

rust/src/dcerpc/dcerpc.rs

@@ -179,6 +179,8 @@ pub struct DCERPCTransaction {
     pub resp_lost: bool,
     pub req_cmd: u8,
     pub resp_cmd: u8,
+    pub activityuuid: Vec<u8>,
+    pub seqnum: u32,
     pub tx_data: AppLayerTxData,
     pub de_state: Option<*mut core::DetectEngineState>,
 }
@@ -206,6 +208,8 @@ impl DCERPCTransaction {
             resp_lost: false,
             req_cmd: DCERPC_TYPE_REQUEST,
             resp_cmd: DCERPC_TYPE_RESPONSE,
+            activityuuid: Vec::new(),
+            seqnum: 0,
             tx_data: AppLayerTxData::new(),
             de_state: None,
         };

rust/src/dcerpc/dcerpc_udp.rs

@@ -20,10 +20,9 @@ use std::mem::transmute;
 use crate::applayer::{AppLayerResult, AppLayerTxData};
 use crate::core;
 use crate::dcerpc::dcerpc::{
-    DCERPCTransaction, DCERPCUuidEntry, DCERPC_TYPE_REQUEST, DCERPC_TYPE_RESPONSE, PFC_FIRST_FRAG,
+    DCERPCTransaction, DCERPCUuidEntry, DCERPC_TYPE_REQUEST, DCERPC_TYPE_RESPONSE, PFCL1_FRAG, PFCL1_LASTFRAG,
 };
 use crate::dcerpc::parser;
-use std::cmp;
 
 // Constant DCERPC UDP Header length
 pub const DCERPC_UDP_HDR_LEN: i32 = 80;
@@ -77,12 +76,12 @@ impl DCERPCUDPState {
         };
     }
 
-    fn create_tx(&mut self, serial_no: u16) -> DCERPCTransaction {
+    fn create_tx(&mut self,  hdr: &DCERPCHdrUdp) -> DCERPCTransaction {
         let mut tx = DCERPCTransaction::new();
-        let endianness = self.get_hdr_drep_0() & 0x10;
         tx.id = self.tx_id;
-        tx.call_id = serial_no as u32;
+        tx.endianness = hdr.drep[0] & 0x10;
-        tx.endianness = endianness;
+        tx.activityuuid = hdr.activityuuid.to_vec();
+        tx.seqnum = hdr.seqnum;
         self.tx_id += 1;
         tx
     }
@@ -108,208 +107,101 @@ impl DCERPCUDPState {
         }
     }
 
-    fn evaluate_serial_no(&mut self) -> u16 {
+    fn find_incomplete_tx(&mut self, hdr: &DCERPCHdrUdp) -> Option<&mut DCERPCTransaction> {
-        let mut serial_no: u16;
-        let mut serial_hi: u8 = 0;
-        let mut serial_lo: u8 = 0;
-        let endianness = self.get_hdr_drep_0();
-        if let Some(ref hdr) = &self.header {
-            serial_hi = hdr.serial_hi;
-            serial_lo = hdr.serial_lo;
-        }
-        if endianness & 0x10 == 0 {
-            serial_no = serial_lo as u16;
-            serial_no = serial_no.rotate_left(8) | serial_hi as u16;
-        } else {
-            serial_no = serial_hi as u16;
-            serial_no = serial_no.rotate_left(8) | serial_lo as u16;
-        }
-        serial_no
-    }
-
-    fn find_tx(&mut self, serial_no: u16) -> Option<&mut DCERPCTransaction> {
         for tx in &mut self.transactions {
-            let found = tx.call_id == (serial_no as u32);
+            if tx.seqnum == hdr.seqnum && tx.activityuuid == hdr.activityuuid {
-            if found {
+                if (hdr.pkt_type == DCERPC_TYPE_REQUEST && !tx.req_done) ||
-                return Some(tx);
+                   (hdr.pkt_type == DCERPC_TYPE_RESPONSE && !tx.resp_done) {
+                    SCLogDebug!("found tx id {}, last tx_id {}, {} {}", tx.id, self.tx_id, tx.seqnum, tx.activityuuid[0]);
+                    return Some(tx);
+                }
             }
         }
         None
     }
 
-    fn get_hdr_pkt_type(&self) -> Option<u8> {
+    pub fn handle_fragment_data(&mut self, hdr: &DCERPCHdrUdp, input: &[u8]) -> bool {
-        debug_validate_bug_on!(self.header.is_none());
+        if hdr.pkt_type != DCERPC_TYPE_REQUEST && hdr.pkt_type != DCERPC_TYPE_RESPONSE {
-        if let Some(ref hdr) = &self.header {
+            SCLogDebug!("Unrecognized packet type");
-            return Some(hdr.pkt_type);
+            return false;
         }
-        // Shouldn't happen
-        None
-    }
 
-    fn get_hdr_flags1(&self) -> Option<u8> {
+        let mut otx = self.find_incomplete_tx(hdr);
-        debug_validate_bug_on!(self.header.is_none());
+        if otx.is_none() {
-        if let Some(ref hdr) = &self.header {
+            let ntx = self.create_tx(hdr);
-            return Some(hdr.flags1);
+            SCLogDebug!("new tx id {}, last tx_id {}, {} {}", ntx.id, self.tx_id, ntx.seqnum, ntx.activityuuid[0]);
+            self.transactions.push(ntx);
+            otx = self.transactions.last_mut();
         }
-        // Shouldn't happen
-        None
-    }
 
-    fn get_hdr_drep_0(&self) -> u8 {
+        if let Some(tx) = otx {
-        debug_validate_bug_on!(self.header.is_none());
+            let done = (hdr.flags1 & PFCL1_FRAG) == 0 || (hdr.flags1 & PFCL1_LASTFRAG) != 0;
-        if let Some(ref hdr) = &self.header {
+
-            return hdr.drep[0];
+            match hdr.pkt_type {
+                DCERPC_TYPE_REQUEST => {
+                    tx.stub_data_buffer_ts.extend_from_slice(&input);
+                    tx.frag_cnt_ts += 1;
+                    if done {
+                        tx.req_done = true;
+                    }
+                    return true;
+                }
+                DCERPC_TYPE_RESPONSE => {
+                    tx.stub_data_buffer_tc.extend_from_slice(&input);
+                    tx.frag_cnt_tc += 1;
+                    if done {
+                        tx.resp_done = true;
+                    }
+                    return true;
+                }
+                _ => {
+                    // unreachable
+                }
+            }
         }
-        0
+        return false; // unreachable
     }
 
-    pub fn get_hdr_fraglen(&self) -> Option<u16> {
+    pub fn handle_input_data(&mut self, input: &[u8]) -> AppLayerResult {
-        debug_validate_bug_on!(self.header.is_none());
+        // Input length should at least be header length
-        if let Some(ref hdr) = &self.header {
+        if (input.len() as i32) < DCERPC_UDP_HDR_LEN {
-            return Some(hdr.fraglen);
+            return AppLayerResult::err();
-        }
-        // Shouldn't happen
-        None
-    }
-
-    pub fn handle_fragment_data(&mut self, input: &[u8], input_len: u16) -> u16 {
-        let retval: u16;
-        let hdrflags1 = self.get_hdr_flags1().unwrap_or(0);
-        let fraglenleft = self.fraglenleft;
-        let hdrtype = self.get_hdr_pkt_type().unwrap_or(0);
-        let serial_no = self.evaluate_serial_no();
-        let tx;
-        if let Some(transaction) = self.find_tx(serial_no) {
-            tx = transaction;
-        } else {
-            SCLogDebug!(
-                "No transaction found matching the serial number: {:?}",
-                serial_no
-            );
-            return 0;
-        }
-
-        // Update the stub params based on the packet type
-        match hdrtype {
-            DCERPC_TYPE_REQUEST => {
-                retval = evaluate_stub_params(
-                    input,
-                    input_len,
-                    hdrflags1,
-                    fraglenleft,
-                    &mut tx.stub_data_buffer_ts,
-                    &mut tx.stub_data_buffer_len_ts,
-                );
-                tx.req_done = true;
-                tx.frag_cnt_ts += 1;
-            }
-            DCERPC_TYPE_RESPONSE => {
-                retval = evaluate_stub_params(
-                    input,
-                    input_len,
-                    hdrflags1,
-                    fraglenleft,
-                    &mut tx.stub_data_buffer_tc,
-                    &mut tx.stub_data_buffer_len_tc,
-                );
-                tx.resp_done = true;
-                tx.frag_cnt_tc += 1;
-            }
-            _ => {
-                SCLogDebug!("Unrecognized packet type");
-                return 0;
-            }
         }
-        // Update the remaining fragment length
-        self.fraglenleft -= retval;
-
-        retval
-    }
 
-    pub fn process_header(&mut self, input: &[u8]) -> i32 {
+        // Call header parser first
         match parser::parse_dcerpc_udp_header(input) {
             Ok((leftover_bytes, header)) => {
                 if header.rpc_vers != 4 {
                     SCLogDebug!("DCERPC UDP Header did not validate.");
-                    return -1;
+                    return AppLayerResult::err();
+                }
+                if leftover_bytes.len() < header.fraglen as usize {
+                    SCLogDebug!("Insufficient data: leftover_bytes {}, fraglen {}", leftover_bytes.len(), header.fraglen);
+                    return AppLayerResult::err();
+                }
+                if !self.handle_fragment_data(&header, &leftover_bytes[..header.fraglen as usize]) {
+                    return AppLayerResult::err();
                 }
                 let mut uuidentry = DCERPCUuidEntry::new();
                 let auuid = header.activityuuid.to_vec();
                 uuidentry.uuid = auuid;
                 self.uuid_list.push(uuidentry);
-                self.header = Some(header);
-                (input.len() - leftover_bytes.len()) as i32
             }
             Err(nom::Err::Incomplete(_)) => {
                 // Insufficient data.
                 SCLogDebug!("Insufficient data while parsing DCERPC request");
-                -1
+                return AppLayerResult::err();
             }
             Err(_) => {
                 // Error, probably malformed data.
                 SCLogDebug!("An error occurred while parsing DCERPC request");
-                -1
+                return AppLayerResult::err();
-            }
-        }
-    }
-
-    pub fn handle_input_data(&mut self, input: &[u8]) -> AppLayerResult {
-        // Input length should at least be header length
-        if (input.len() as i32) < DCERPC_UDP_HDR_LEN {
-            return AppLayerResult::err();
-        }
-        // Call header parser first
-        let mut parsed = self.process_header(input);
-        if parsed == -1 {
-            return AppLayerResult::err();
-        }
-
-        let mut input_left = input.len() as i32 - parsed;
-        let fraglen = self.get_hdr_fraglen().unwrap_or(0);
-        self.fraglenleft = fraglen;
-        let serial_no = self.evaluate_serial_no();
-        let tx = self.create_tx(serial_no);
-        self.transactions.push(tx);
-        // Parse rest of the body
-        while parsed >= DCERPC_UDP_HDR_LEN && parsed < fraglen as i32 && input_left > 0 {
-            let retval = self.handle_fragment_data(&input[parsed as usize..], input_left as u16);
-            if retval > 0 && retval <= input_left as u16 {
-                parsed += retval as i32;
-                input_left -= retval as i32;
-            } else if input_left > 0 {
-                SCLogDebug!("Error parsing DCERPC UDP Fragment Data");
-                parsed -= input_left;
-                input_left = 0;
             }
         }
         return AppLayerResult::ok();
     }
 }
 
-fn evaluate_stub_params(
-    input: &[u8], input_len: u16, hdrflags: u8, lenleft: u16, stub_data_buffer: &mut Vec<u8>,
-    stub_data_buffer_len: &mut u32,
-) -> u16 {
-    let stub_len: u16;
-    stub_len = cmp::min(lenleft, input_len);
-    if stub_len == 0 {
-        return 0;
-    }
-    // If the UDP frag is the the first frag irrespective of it being a part of
-    // a multi frag PDU or not, it indicates the previous PDU's stub would
-    // have been buffered and processed and we can use the buffer to hold
-    // frags from a fresh request/response
-    if hdrflags & PFC_FIRST_FRAG > 0 {
-        *stub_data_buffer_len = 0;
-    }
-
-    let input_slice = &input[..stub_len as usize];
-    stub_data_buffer.extend_from_slice(&input_slice);
-    *stub_data_buffer_len += stub_len as u32;
-
-    stub_len
-}
-
 #[no_mangle]
 pub extern "C" fn rs_dcerpc_udp_parse(
     _flow: *mut core::Flow, state: &mut DCERPCUDPState, _pstate: *mut std::os::raw::c_void,
@@ -401,6 +293,7 @@ pub extern "C" fn rs_dcerpc_udp_get_alstate_progress_completion_status(_directio
 mod tests {
     use crate::applayer::AppLayerResult;
     use crate::dcerpc::dcerpc_udp::DCERPCUDPState;
+    use crate::dcerpc::parser;
 
     #[test]
     fn test_process_header_udp_incomplete_hdr() {
@@ -410,8 +303,12 @@ mod tests {
             0x1c, 0x7d, 0xcf, 0x11,
         ];
 
-        let mut dcerpcudp_state = DCERPCUDPState::new();
+        match parser::parse_dcerpc_udp_header(request) {
-        assert_eq!(-1, dcerpcudp_state.process_header(request));
+            Ok((_rem, _header)) => {
+                { assert!(false); }
+            }
+            _ => {}
+        }
     }
 
     #[test]
@@ -424,8 +321,13 @@ mod tests {
             0x79, 0xbe, 0x01, 0x34, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
             0xff, 0xff, 0xff, 0xff, 0x68, 0x00, 0x00, 0x00, 0x0a, 0x00,
         ];
-        let mut dcerpcudp_state = DCERPCUDPState::new();
+        match parser::parse_dcerpc_udp_header(request) {
-        assert_eq!(80, dcerpcudp_state.process_header(request));
+            Ok((rem, header)) => {
+                assert_eq!(4, header.rpc_vers);
+                assert_eq!(80, request.len() - rem.len());
+            }
+            _ => { assert!(false); }
+        }
     }
 
     #[test]
@@ -438,12 +340,14 @@ mod tests {
             0x79, 0xbe, 0x01, 0x34, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
             0xff, 0xff, 0xff, 0xff, 0x68, 0x00, 0x00, 0x00, 0x0a, 0x00,
         ];
-        let mut dcerpcudp_state = DCERPCUDPState::new();
+        match parser::parse_dcerpc_udp_header(request) {
-        assert_eq!(80, dcerpcudp_state.process_header(request));
+            Ok((rem, header)) => {
-        assert_eq!(
+                assert_eq!(4, header.rpc_vers);
-            0,
+                assert_eq!(80, request.len() - rem.len());
-            dcerpcudp_state.handle_fragment_data(request, request.len() as u16)
+                assert_eq!(0, rem.len());
-        );
+            }
+            _ => { assert!(false); }
+        }
     }
 
     #[test]
@@ -564,7 +468,7 @@ mod tests {
         assert_eq!(0, dcerpcudp_state.fraglenleft);
         assert_eq!(
             1392,
-            dcerpcudp_state.transactions[0].stub_data_buffer_len_ts
+            dcerpcudp_state.transactions[0].stub_data_buffer_ts.len()
         );
     }
 }