diff --git a/lib/srv/desktop/rdp/rdpclient/src/rdpdr.rs b/lib/srv/desktop/rdp/rdpclient/src/rdpdr.rs index ca0efe4db83e8..c9b0f41aaaae3 100644 --- a/lib/srv/desktop/rdp/rdpclient/src/rdpdr.rs +++ b/lib/srv/desktop/rdp/rdpclient/src/rdpdr.rs @@ -1965,39 +1965,18 @@ impl ClientDeviceListAnnounceRequest { // We only need to announce the smartcard in this Client Device List Announce Request. // Drives (directories) can be announced at any time with a Client Drive Device List Announce. fn new_smartcard(device_id: u32) -> Self { - Self { - device_count: 1, - device_list: vec![DeviceAnnounceHeader { - device_type: DeviceType::RDPDR_DTYP_SMARTCARD, - device_id, - // This name is a constant defined by the spec. - preferred_dos_name: "SCARD".to_string(), - device_data_length: 0, - device_data: vec![], - }], - } + Self::new(vec![DeviceAnnounceHeader::new_smartcard(device_id)]) } /// A new drive can be announced at any time during RDP's operation. It is up to the caller - pub fn new_drive(device_id: u32, drive_name: String) -> Self { - // If the client supports DRIVE_CAPABILITY_VERSION_02 in the Drive Capability Set, - // then the full name MUST also be specified in the DeviceData field, as a null-terminated - // Unicode string. If the DeviceDataLength field is nonzero, the content of the - // PreferredDosName field is ignored. - // - // In the RDP spec, Unicode typically means null-terminated UTF-16LE, however empirically it - // appears that this field expects null-terminated UTF-8. - let device_data = util::to_utf8(&drive_name); + pub fn new_drive(device_id: u32, name: String) -> Self { + Self::new(vec![DeviceAnnounceHeader::new_drive(device_id, name)]) + } + fn new(device_list: Vec) -> Self { Self { - device_count: 1, - device_list: vec![DeviceAnnounceHeader { - device_type: DeviceType::RDPDR_DTYP_FILESYSTEM, - device_id, - preferred_dos_name: drive_name, - device_data_length: device_data.len() as u32, - device_data, - }], + device_count: device_list.len() as u32, + device_list, } } } @@ -2017,27 +1996,72 @@ impl Encode for ClientDeviceListAnnounceRequest { /// https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpefs/32e34332-774b-4ead-8c9d-5d64720d6bf9 #[derive(Debug)] struct DeviceAnnounceHeader { + name: String, device_type: DeviceType, device_id: u32, - preferred_dos_name: String, - device_data_length: u32, - device_data: Vec, } impl DeviceAnnounceHeader { + fn new_smartcard(device_id: u32) -> Self { + Self { + name: "".to_string(), // ignored for smartcards + device_type: DeviceType::RDPDR_DTYP_SMARTCARD, + device_id, + } + } + + fn new_drive(device_id: u32, name: String) -> Self { + Self { + name, + device_type: DeviceType::RDPDR_DTYP_FILESYSTEM, + device_id, + } + } + fn encode(&self) -> RdpResult { let mut w = vec![]; w.write_u32::(self.device_type.to_u32().unwrap())?; w.write_u32::(self.device_id)?; - let mut name: &str = &self.preferred_dos_name; - // See "PreferredDosName" at - // https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpefs/32e34332-774b-4ead-8c9d-5d64720d6bf9 - if name.len() > 7 { - name = &name[..7]; + // Initially preferred_dos_name was set to the first 7 ASCII characters (per the spec) + // of the drive name when the drive was of type DeviceType::RDPDR_DTYP_FILESYSTEM, + // however this was causing a panic when the user shared a directory with a name with + // non-ASCII characters (e.g. 中文测试). + let mut preferred_dos_name: &str = match self.device_type { + // In this case we can just use any random value, since it will be ignored in favor + // of the UTF-8 encoded drive name in the DeviceData field. + // + // We may need to revisit this if we ever support multiple drives + // (i.e. do "FILE1", "FILE2", etc). + DeviceType::RDPDR_DTYP_FILESYSTEM => "FILE", + // If DeviceType is set to RDPDR_DTYP_SMARTCARD, the PreferredDosName MUST be set to "SCARD". + // See: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpefs/32e34332-774b-4ead-8c9d-5d64720d6bf9 + DeviceType::RDPDR_DTYP_SMARTCARD => "SCARD", + _ => { + return Err(RdpError::TryError(format!( + "unsupported device type {device_type:?}", + device_type = self.device_type + ))) + } + }; + // Sanity check to make sure we don't write more than + // 8 bytes (including the null terminator) for PreferredDosName. + if preferred_dos_name.len() > 7 { + preferred_dos_name = &preferred_dos_name[..7]; } - w.extend_from_slice(&format!("{name:\x00<8}").into_bytes()); - w.write_u32::(self.device_data_length)?; - w.extend_from_slice(&self.device_data); + w.extend_from_slice(&format!("{preferred_dos_name:\x00<8}").into_bytes()); + + // If the client supports DRIVE_CAPABILITY_VERSION_02 in the Drive Capability Set (which we do), + // then the full name MUST also be specified in the DeviceData field, as a null-terminated + // Unicode string. If the DeviceDataLength field is nonzero, the content of the + // PreferredDosName field is ignored. + // + // In the RDP spec, Unicode typically means null-terminated UTF-16LE, however empirically it + // appears that this field expects null-terminated UTF-8. + let device_data = util::to_utf8(&self.name); + let device_data_length = device_data.len() as u32; + + w.write_u32::(device_data_length)?; + w.extend_from_slice(&device_data); Ok(w) } } diff --git a/lib/srv/desktop/rdp/rdpclient/src/rdpdr/tests.rs b/lib/srv/desktop/rdp/rdpclient/src/rdpdr/tests.rs index 5549ddf6a16d6..d6ae7ecadd836 100644 --- a/lib/srv/desktop/rdp/rdpclient/src/rdpdr/tests.rs +++ b/lib/srv/desktop/rdp/rdpclient/src/rdpdr/tests.rs @@ -306,16 +306,9 @@ fn test_handle_client_id_confirm() { }, vec![( PacketId::PAKID_CORE_DEVICELIST_ANNOUNCE, - Box::new(ClientDeviceListAnnounceRequest { - device_count: 1, - device_list: vec![DeviceAnnounceHeader { - device_type: DeviceType::RDPDR_DTYP_SMARTCARD, - device_id: 1, - preferred_dos_name: "SCARD".to_string(), - device_data_length: 0, - device_data: vec![], - }], - }), + Box::new(ClientDeviceListAnnounceRequest::new_smartcard( + SCARD_DEVICE_ID, + )), )], ); @@ -429,3 +422,18 @@ fn check_dir_sharing_methods_error_when_disabled() { } } } + +/// Checks that we can encode a DeviceAnnounceHeader with a non-ascii name, +/// which was causing a panic in the past. +#[test] +fn test_device_announce_header_encode_with_non_ascii() { + assert_eq!( + DeviceAnnounceHeader::new_drive(2, "中文测试".to_string()) + .encode() + .unwrap(), + vec![ + 8, 0, 0, 0, 2, 0, 0, 0, 70, 73, 76, 69, 0, 0, 0, 0, 13, 0, 0, 0, 228, 184, 173, 230, + 150, 135, 230, 181, 139, 232, 175, 149, 0 + ] + ) +}