feat(pdu): add arbitrary feature for structure-aware fuzzing (#1272)

Author: glamberson

Cargo.lock

@@ -18,7 +18,7 @@ test = false
 
 [features]
 default = []
-arbitrary = ["dep:arbitrary"]
+arbitrary = ["dep:arbitrary", "ironrdp-pdu/arbitrary"]
 qoi = ["ironrdp-pdu/qoi"]
 qoiz = ["ironrdp-pdu/qoiz"]
 

crates/ironrdp-connector/Cargo.toml

@@ -18,7 +18,6 @@ use crate::{
 };
 
 #[derive(Debug)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct ConnectionResult {
     pub io_channel_id: u16,
     pub user_channel_id: u16,
@@ -34,7 +33,6 @@ pub struct ConnectionResult {
 
 #[derive(Default, Debug)]
 #[non_exhaustive]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub enum ClientConnectorState {
     #[default]
     Consumed,
@@ -122,7 +120,6 @@ impl State for ClientConnectorState {
 }
 
 #[derive(Debug)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct ClientConnector {
     pub config: Config,
     pub state: ClientConnectorState,

crates/ironrdp-connector/src/connection.rs

@@ -34,6 +34,7 @@ pub use crate::license_exchange::LicenseCache;
 
 /// Provides user-friendly error messages for RDP negotiation failures
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct NegotiationFailure(ironrdp_pdu::nego::FailureCode);
 
 impl NegotiationFailure {
@@ -97,6 +98,7 @@ pub struct BitmapConfig {
 }
 
 #[derive(Debug, Clone)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct SmartCardIdentity {
     /// DER-encoded X509 certificate
     pub certificate: Vec<u8>,
@@ -111,6 +113,7 @@ pub struct SmartCardIdentity {
 }
 
 #[derive(Debug, Clone)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub enum Credentials {
     UsernamePassword {
         username: String,
@@ -233,6 +236,11 @@ pub struct Config {
     pub enable_audio_playback: bool,
     pub performance_flags: PerformanceFlags,
 
+    // `Arc<dyn LicenseCache>` cannot be generated by `arbitrary`. Skipped via
+    // `arbitrary(default)` which yields `None`. License-cache-dependent paths
+    // are not exercised under fuzz; a fuzz-mode `LicenseCache` impl is a
+    // possible follow-up.
+    #[cfg_attr(feature = "arbitrary", arbitrary(default))]
     pub license_cache: Option<Arc<dyn LicenseCache>>,
 
     // For Timezone Redirection to sync the server's timezone with the client's.
@@ -296,6 +304,7 @@ impl State for () {
 }
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub enum Written {
     Nothing,
     Size(core::num::NonZeroUsize),
@@ -342,6 +351,8 @@ pub type ConnectorResult<T> = Result<T, ConnectorError>;
 
 #[non_exhaustive]
 #[derive(Debug)]
+// ConnectorErrorKind carries foreign error types (sspi::Error, ironrdp_error::Error)
+// that do not implement Arbitrary, so this enum is intentionally not annotated.
 pub enum ConnectorErrorKind {
     Encode(ironrdp_core::EncodeError),
     Decode(ironrdp_core::DecodeError),

crates/ironrdp-connector/src/lib.rs

@@ -56,7 +56,6 @@ impl State for LicenseExchangeState {
 ///
 /// [3.1.5.3.1]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpele/8f9b860a-3687-401d-b3bc-7e9f5d4f7528
 #[derive(Debug)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct LicenseExchangeSequence {
     pub state: LicenseExchangeState,
     pub io_channel_id: u16,
@@ -66,6 +65,29 @@ pub struct LicenseExchangeSequence {
     pub license_cache: Arc<dyn LicenseCache>,
 }
 
+// `license_cache` is `Arc<dyn LicenseCache>` which cannot be derived. The hand-rolled impl
+// hardcodes `NoopLicenseCache`, so license-cache-dependent paths are not exercised under fuzz.
+// Reaching the license sequence is still useful coverage (MS-RDPELE is a historically
+// interesting target). A fuzz-mode `LicenseCache` impl is a possible follow-up.
+#[cfg(feature = "arbitrary")]
+impl<'a> arbitrary::Arbitrary<'a> for LicenseExchangeSequence {
+    fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result<Self> {
+        Ok(Self {
+            state: LicenseExchangeState::arbitrary(u)?,
+            io_channel_id: u16::arbitrary(u)?,
+            username: String::arbitrary(u)?,
+            domain: Option::<String>::arbitrary(u)?,
+            hardware_id: [
+                u32::arbitrary(u)?,
+                u32::arbitrary(u)?,
+                u32::arbitrary(u)?,
+                u32::arbitrary(u)?,
+            ],
+            license_cache: Arc::new(NoopLicenseCache),
+        })
+    }
+}
+
 // Use RefUnwindSafe so that types that embed LicenseCache remain UnwindSafe
 pub trait LicenseCache: Sync + Send + Debug + RefUnwindSafe {
     fn get_license(&self, license_info: LicenseInformation) -> ConnectorResult<Option<Vec<u8>>>;

crates/ironrdp-connector/src/license_exchange.rs

@@ -22,11 +22,13 @@ std = ["alloc", "ironrdp-error/std", "ironrdp-core/std"]
 alloc = ["ironrdp-core/alloc", "ironrdp-error/alloc"]
 qoi = []
 qoiz = ["qoi"]
+arbitrary = ["alloc", "dep:arbitrary", "bitflags/arbitrary"]
 
 [dependencies]
 bitflags = "2.11"
 ironrdp-core = { path = "../ironrdp-core", version = "0.1", features = ["std"] } # public
 ironrdp-error = { path = "../ironrdp-error", version = "0.1" } # public
+arbitrary = { version = "1", features = ["derive"], optional = true }
 tap = "1"
 
 # TODO: get rid of these dependencies (related code should probably go into another crate)

crates/ironrdp-pdu/Cargo.toml

@@ -468,6 +468,15 @@ are considered to be known and defined. In such cases, `from_bits` also never fa
 precisely the same as `from_bits_retain`, except it’s less ergonomic because it returns a `Result`
 which must be needlessly handled.
 
+## Feature Flags
+
+- `std` (depends on `alloc`): enables `std` integration in `ironrdp-error` and `ironrdp-core`.
+- `alloc`: enables allocation-dependent code paths while keeping `no_std`-compatible.
+- `qoi`, `qoiz`: optional [QOI image format](https://qoiformat.org/) bitmap codec support.
+- `arbitrary` (depends on `alloc`): enables [`arbitrary::Arbitrary`](https://docs.rs/arbitrary)
+  implementations on PDU types for use under structure-aware fuzzing. See
+  [`fuzz/README.md`](../../fuzz/README.md) for the fuzz target organization.
+
 This crate is part of the [IronRDP] project.
 
 [IronRDP]: https://github.com/Devolutions/IronRDP

crates/ironrdp-pdu/README.md

@@ -17,6 +17,7 @@ const FIRST_ROW_SIZE_VALUE: u16 = 0;
 
 /// TS_UPDATE_BITMAP_DATA
 #[derive(Debug, Clone, PartialEq, Eq)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct BitmapUpdateData<'a> {
     pub rectangles: Vec<BitmapData<'a>>,
 }
@@ -85,6 +86,7 @@ impl<'de> Decode<'de> for BitmapUpdateData<'de> {
 
 /// TS_BITMAP_DATA
 #[derive(Clone, PartialEq, Eq)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct BitmapData<'a> {
     pub rectangle: InclusiveRectangle,
     pub width: u16,
@@ -197,6 +199,7 @@ impl Debug for BitmapData<'_> {
 
 /// TS_CD_HEADER
 #[derive(Debug, Clone, PartialEq, Eq)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct CompressedDataHeader {
     pub main_body_size: u16,
     pub scan_width: u16,
@@ -267,6 +270,7 @@ impl Encode for CompressedDataHeader {
 
 bitflags! {
     #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+    #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
     pub struct BitmapFlags: u16{
         const BITMAP_UPDATE_TYPE = 0x0001;
 
@@ -276,6 +280,7 @@ bitflags! {
 
 bitflags! {
     #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+    #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
     pub struct Compression: u16 {
        const BITMAP_COMPRESSION = 0x0001;
        const NO_BITMAP_COMPRESSION_HDR = 0x0400;

crates/ironrdp-pdu/src/basic_output/bitmap/mod.rs

@@ -6,6 +6,7 @@ use ironrdp_core::{
 const NON_RLE_PADDING_SIZE: usize = 1;
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub enum ColorPlaneDefinition {
     Argb,
     AYCoCg {
@@ -15,6 +16,7 @@ pub enum ColorPlaneDefinition {
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct BitmapStreamHeader {
     pub enable_rle_compression: bool,
     pub use_alpha: bool,
@@ -93,6 +95,7 @@ impl Encode for BitmapStreamHeader {
 
 /// Represents `RDP6_BITMAP_STREAM` structure described in [MS-RDPEGDI] 2.2.2.5.1
 #[derive(Debug, Clone)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct BitmapStream<'a> {
     pub header: BitmapStreamHeader,
     pub color_planes: &'a [u8],

crates/ironrdp-pdu/src/basic_output/bitmap/rdp6.rs

@@ -24,6 +24,7 @@ use crate::rdp::headers::{CompressionFlags, SHARE_DATA_HEADER_COMPRESSION_MASK};
     reason = "this structure is used in the match expression in the integration tests"
 )]
 #[derive(Debug, Clone, PartialEq, Eq)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct FastPathHeader {
     pub flags: EncryptionFlags,
     pub data_length: usize,
@@ -116,6 +117,7 @@ impl<'de> Decode<'de> for FastPathHeader {
 
 /// TS_FP_UPDATE
 #[derive(Debug, Clone, PartialEq, Eq)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct FastPathUpdatePdu<'a> {
     pub fragmentation: Fragmentation,
     pub update_code: UpdateCode,
@@ -217,6 +219,7 @@ impl<'de> Decode<'de> for FastPathUpdatePdu<'de> {
 
 /// TS_FP_UPDATE data
 #[derive(Debug, Clone, PartialEq, Eq)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub enum FastPathUpdate<'a> {
     SurfaceCommands(Vec<SurfaceCommand<'a>>),
     Bitmap(BitmapUpdateData<'a>),
@@ -329,6 +332,7 @@ impl Encode for FastPathUpdate<'_> {
 
 #[repr(u8)]
 #[derive(Debug, Copy, Clone, PartialEq, Eq, FromPrimitive)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub enum UpdateCode {
     Orders = 0x0,
     Bitmap = 0x1,
@@ -375,6 +379,7 @@ impl From<&FastPathUpdate<'_>> for UpdateCode {
 
 #[repr(u8)]
 #[derive(Debug, Copy, Clone, PartialEq, Eq, FromPrimitive)]
+#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub enum Fragmentation {
     Single = 0x0,
     Last = 0x1,
@@ -394,6 +399,7 @@ impl Fragmentation {
 
 bitflags! {
     #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+    #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
     pub struct EncryptionFlags: u8 {
         const SECURE_CHECKSUM = 0x1;
         const ENCRYPTED = 0x2;
@@ -404,6 +410,7 @@ bitflags! {
 
 bitflags! {
     #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+    #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
     pub struct Compression: u8 {
         const COMPRESSION_USED = 0x2;