diff --git a/.changeset/podstring_utf8_soundness.md b/.changeset/podstring_utf8_soundness.md new file mode 100644 index 00000000..9347b974 --- /dev/null +++ b/.changeset/podstring_utf8_soundness.md @@ -0,0 +1,5 @@ +--- +pina_pod_primitives: fix +--- + +Remove the unsound `Deref` and `AsRef` impls from `PodString`. Because `PodString` is `bytemuck::Pod`, bytes loaded from untrusted account data may not be valid UTF-8, and the removed impls produced a `&str` via `from_utf8_unchecked` — undefined behavior. Use `try_as_str()` for validated access, or `as_str_unchecked()` (unsafe) for unchecked access. diff --git a/crates/pina_pod_primitives/src/string.rs b/crates/pina_pod_primitives/src/string.rs index ea2bb22d..d2a1da3e 100644 --- a/crates/pina_pod_primitives/src/string.rs +++ b/crates/pina_pod_primitives/src/string.rs @@ -103,11 +103,16 @@ impl PodString { N } - /// Returns the string as a `&str`. + /// Returns the string as a `&str` without validating UTF-8. + /// + /// This is the only way to obtain a `&str` view without validation — + /// `PodString` deliberately does not implement `Deref` or + /// `AsRef`, because the stored bytes may be arbitrary when the value + /// is loaded from untrusted account data via `Pod`. /// /// # Safety - /// This assumes the stored bytes are valid UTF-8. For untrusted account - /// data, use `try_as_str()` instead. + /// The stored bytes must be valid UTF-8. For untrusted account data, use + /// `try_as_str()` instead. #[inline] pub unsafe fn as_str_unchecked(&self) -> &str { unsafe { @@ -197,20 +202,6 @@ impl Default for PodString { } } -impl core::ops::Deref for PodString { - type Target = str; - - fn deref(&self) -> &str { - unsafe { self.as_str_unchecked() } - } -} - -impl AsRef for PodString { - fn as_ref(&self) -> &str { - unsafe { self.as_str_unchecked() } - } -} - impl AsRef<[u8]> for PodString { fn as_ref(&self) -> &[u8] { self.as_bytes() diff --git a/crates/pina_pod_primitives/src/tests/string.rs b/crates/pina_pod_primitives/src/tests/string.rs index a862ef6c..93f51314 100644 --- a/crates/pina_pod_primitives/src/tests/string.rs +++ b/crates/pina_pod_primitives/src/tests/string.rs @@ -56,3 +56,79 @@ fn pod_string_bytemuck_roundtrip() { let restored = unsafe { &*(bytes.as_ptr() as *const PodString<32>) }; assert_eq!(restored.try_as_str().unwrap(), "test"); } + +// --------------------------------------------------------------------------- +// UTF-8 soundness: PodString loaded from untrusted bytes +// --------------------------------------------------------------------------- + +/// A `PodString` loaded from arbitrary account data may contain invalid +/// UTF-8. `try_as_str()` must reject it rather than producing a `&str`. +#[test] +fn pod_string_invalid_utf8_rejected_by_try_as_str() { + // Length prefix 2, data bytes [0xff, 0xfe] — invalid UTF-8. + let bytes = [2u8, 0xff, 0xfe]; + let pod = try_from_bytes::>(&bytes) + .unwrap_or_else(|e| panic!("try_from_bytes failed for {bytes:?}: {e}")); + assert_eq!(pod.len(), 2); + assert!(matches!( + pod.try_as_str(), + Err(PodCollectionError::InvalidUtf8) + )); +} + +/// A truncated multi-byte sequence is also invalid UTF-8. +#[test] +fn pod_string_incomplete_utf8_rejected_by_try_as_str() { + // Length prefix 1, data byte 0xc3 — a lead byte with no continuation. + let bytes = [1u8, 0xc3]; + let pod = try_from_bytes::>(&bytes) + .unwrap_or_else(|e| panic!("try_from_bytes failed for {bytes:?}: {e}")); + assert!(matches!( + pod.try_as_str(), + Err(PodCollectionError::InvalidUtf8) + )); +} + +/// Safe traits must never panic or produce a `&str` from invalid UTF-8. +#[test] +fn pod_string_invalid_utf8_safe_traits_do_not_panic() { + let bytes = [2u8, 0xff, 0xfe]; + let pod = try_from_bytes::>(&bytes) + .unwrap_or_else(|e| panic!("try_from_bytes failed for {bytes:?}: {e}")); + + // Debug and Display fall back to a placeholder. + assert_eq!(std::format!("{pod:?}"), "PodString { len: 2 }"); + assert_eq!(std::format!("{pod}"), ""); + + // Byte access and byte-based comparison remain total and safe. + assert_eq!(pod.as_bytes(), &[0xff, 0xfe]); + assert_eq!( as AsRef<[u8]>>::as_ref(&pod), &[0xff, 0xfe]); + assert_eq!(pod, pod); + assert_ne!(pod, "valid"); +} + +/// Valid multi-byte UTF-8 survives a Pod round-trip. +#[test] +fn pod_string_valid_utf8_roundtrip_via_pod() { + let mut s = PodString::<32>::default(); + s.set("héllo wörld"); + let bytes = bytemuck::bytes_of(&s); + let restored = try_from_bytes::>(bytes) + .unwrap_or_else(|e| panic!("try_from_bytes failed: {e}")); + assert_eq!( + restored + .try_as_str() + .unwrap_or_else(|e| panic!("invalid UTF-8: {e}")), + "héllo wörld" + ); +} + +/// `as_str_unchecked` remains available as an explicit unsafe escape hatch. +#[test] +fn pod_string_as_str_unchecked_valid() { + let mut s = PodString::<32>::default(); + s.set("hello"); + // SAFETY: "hello" is valid UTF-8. + let s = unsafe { s.as_str_unchecked() }; + assert_eq!(s, "hello"); +}