diff --git a/.gitignore b/.gitignore index d787b70..a24af7a 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,3 @@ /target /result +librust_out.rlib diff --git a/improvements.md b/improvements.md deleted file mode 100644 index e16a060..0000000 --- a/improvements.md +++ /dev/null @@ -1,78 +0,0 @@ -# USB/IP Rust Implementation — Audit & Improvements - -## CRITICAL: Security Hardening (untrusted client) - -### 1. Endpoint number not validated 0-15 -- **File:** `lib/src/lib.rs:508` -- **Issue:** `header.ep` is a `u32` from the wire, cast to `u8` at line 525. The kernel validates `epnum` is 0-15 (`stub_rx.c:344`). A malicious client can send any value; truncation to `u8` is silent. -- **Fix:** Reject if `header.ep > 15`. -- **Status:** [x] DONE — Added validation rejecting `header.ep > 15` before processing. - -### 2. ISO `total_len` overflow in host passthrough -- **File:** `lib/src/host.rs:419-421` -- **Issue:** `num_packets * packet_size` can overflow or produce a huge allocation. Should use `transfer_buffer_length` (already validated) instead of computing from first descriptor's length. -- **Status:** [x] DONE — `total_len` now uses `request.transfer_buffer_length as usize`. - -### 3. `try_read().expect()` panic in `find_ep` -- **File:** `lib/src/device.rs:238` -- **Issue:** If a concurrent `SET_INTERFACE` holds the write lock, this panics the host. -- **Fix:** Use `.read().await` or return an error. -- **Status:** [x] DONE — `try_read()` now returns `None`/skips on contention instead of panicking. Also fixed `to_bytes_with_interfaces()`. - -### 4. Unbounded in-flight URBs (DoS) -- **File:** `lib/src/lib.rs:463` -- **Issue:** A malicious client can flood CMD_SUBMIT without waiting for responses, spawning unbounded tasks and HashMap entries. -- **Fix:** Cap concurrent in-flight URBs at a reasonable limit (e.g., 256). -- **Status:** [x] DONE — Added `MAX_IN_FLIGHT_URBS = 256` constant; rejects with EBUSY when exceeded. - -## HIGH: USB Spec Conformance & Data Integrity - -### 5. String descriptor `bLength` overflow -- **File:** `lib/src/device.rs:521` -- **Issue:** If a string has >126 UTF-16 code units, `bytes.len() as u8 * 2` wraps around, producing a malformed descriptor. -- **Fix:** Cap or use `u16` arithmetic, truncate string if needed. -- **Status:** [x] DONE — Truncate UTF-16 to max 126 code units before computing bLength. - -### 6. No `devid` validation -- **File:** `lib/src/lib.rs:505` -- **Issue:** The kernel checks `pdu->base.devid` matches the imported device. This implementation ignores `devid`. -- **Fix:** Validate devid matches the imported device. -- **Status:** [x] DONE — Compute expected devid from device bus_num/dev_num; reject mismatches in CMD_SUBMIT and CMD_UNLINK. - -### 7. Missing standard control requests (simulated devices) -- **File:** `lib/src/device.rs:346-778` -- **Issue:** `GetStatus`, `ClearFeature`, `SetFeature`, `GetConfiguration`, `SetAddress` are unhandled for simulated devices. -- **Status:** [x] DONE — Added GetConfiguration, GetStatus (device/interface/endpoint), ClearFeature, SetFeature, SetAddress handlers. - -### 8. ISO `actual_length` sum not validated -- **File:** `lib/src/lib.rs:551-552` -- **Issue:** No overflow check on the sum of per-packet `actual_length`. Should use `checked_add` and cap at `transfer_buffer_length`. -- **Status:** [x] DONE — Use `saturating_add` fold capped at `transfer_buffer_length`. - -### 9. Response data length not capped to `transfer_buffer_length` -- **File:** `lib/src/lib.rs:569-578` -- **Issue:** For IN transfers, response `data` from the handler is sent as-is without verifying it doesn't exceed `transfer_buffer_length`. -- **Fix:** Truncate response data to `transfer_buffer_length`. -- **Status:** [x] DONE — Truncate `urb_resp.data` if it exceeds `transfer_buffer_length` for IN transfers. - -## MEDIUM: Reliability - -### 10. `server()` panics on bind failure -- **File:** `lib/src/lib.rs:773` -- **Issue:** `expect("bind to addr")` panics instead of returning `Result`. -- **Status:** [x] DONE — Changed return type to `Result<()>`, replaced `expect` with `?`. - -### 11. Error status mapping via string matching -- **File:** `lib/src/lib.rs:584-592` -- **Issue:** Fragile string-based error classification depends on nusb/rusb message text. -- **Status:** [x] DONE — Preserve nusb `TransferError` through `io::Error`; classify via `ErrorKind` instead of string matching. - -### 12. Poisoned mutex panics -- **Files:** `host.rs`, `device.rs` (multiple locations) -- **Issue:** All `Mutex::lock().unwrap()` calls panic if the mutex is poisoned. -- **Status:** [x] DONE — Replaced 21 production `.lock().unwrap()` with `.lock().unwrap_or_else(|e| e.into_inner())`. - -### 13. `debug_assert` for path/bus_id length -- **File:** `lib/src/device.rs:252-260` -- **Issue:** `debug_assert!` compiled out in release; silent truncation if path > 256 bytes. -- **Status:** [x] DONE — Replaced `debug_assert!` + `resize` with `truncate` + `resize`.