From 23fd798ce1acabb7c8b397f58217efc724a1b9c2 Mon Sep 17 00:00:00 2001 From: Isaiah Inuwa Date: Sun, 21 Jun 2026 19:12:48 -0500 Subject: [PATCH 1/4] common: Move memfd ops to shared lib --- Cargo.lock | 10 +-- credentialsd-common/Cargo.toml | 2 + credentialsd-common/src/lib.rs | 1 + credentialsd-common/src/memfd.rs | 118 ++++++++++++++++++++++++++ credentialsd-ui/Cargo.toml | 1 - credentialsd-ui/src/client.rs | 76 ++--------------- credentialsd/Cargo.toml | 1 - credentialsd/src/dbus/flow_control.rs | 72 ++-------------- 8 files changed, 140 insertions(+), 141 deletions(-) create mode 100644 credentialsd-common/src/memfd.rs diff --git a/Cargo.lock b/Cargo.lock index 3ac56848..fb8fcaff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -799,7 +799,6 @@ dependencies = [ "futures", "futures-lite", "gio 0.21.5", - "libc", "libwebauthn", "nfc1", "rand 0.9.2", @@ -817,7 +816,9 @@ name = "credentialsd-common" version = "0.2.0" dependencies = [ "futures-lite", + "libc", "serde", + "zeroize", "zvariant", ] @@ -832,7 +833,6 @@ dependencies = [ "gettext-rs", "gio-unix", "gtk4", - "libc", "qrcode", "serde", "tracing", @@ -3184,7 +3184,7 @@ dependencies = [ "once_cell", "socket2", "tracing", - "windows-sys 0.52.0", + "windows-sys 0.60.2", ] [[package]] @@ -5210,9 +5210,9 @@ dependencies = [ [[package]] name = "zeroize" -version = "1.8.2" +version = "1.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b97154e67e32c85465826e8bcc1c59429aaaf107c1e4a9e53c8d8ccd5eff88d0" +checksum = "e13c156562582aa81c60cb29407084cdb54c4164760106ab78e6c5b0858cf64e" dependencies = [ "zeroize_derive", ] diff --git a/credentialsd-common/Cargo.toml b/credentialsd-common/Cargo.toml index 66f1c2c9..fc0490a9 100644 --- a/credentialsd-common/Cargo.toml +++ b/credentialsd-common/Cargo.toml @@ -7,5 +7,7 @@ license = "LGPL-3.0-only" [dependencies] futures-lite.workspace = true +libc.workspace = true serde = { workspace = true, features = ["derive"] } +zeroize = "1.9.0" zvariant.workspace = true diff --git a/credentialsd-common/src/lib.rs b/credentialsd-common/src/lib.rs index cd7f939f..f6ad9561 100644 --- a/credentialsd-common/src/lib.rs +++ b/credentialsd-common/src/lib.rs @@ -1,3 +1,4 @@ pub mod client; +pub mod memfd; pub mod model; pub mod server; diff --git a/credentialsd-common/src/memfd.rs b/credentialsd-common/src/memfd.rs new file mode 100644 index 00000000..52cfb0cf --- /dev/null +++ b/credentialsd-common/src/memfd.rs @@ -0,0 +1,118 @@ +use std::{ + io::{self, ErrorKind}, + mem::MaybeUninit, + os::{ + fd::{AsRawFd, FromRawFd, OwnedFd}, + raw::c_void, + }, +}; + +use libc::{ + ftruncate, mmap, off_t, SYS_memfd_secret, MAP_SHARED, O_CLOEXEC, PROT_READ, PROT_WRITE, +}; +use zeroize::Zeroize; + +pub fn read_secret(pin_fd: std::os::fd::OwnedFd) -> Result { + // Get pin length + let len = { + let mut stat_buf = MaybeUninit::::uninit(); + let res = unsafe { libc::fstat(pin_fd.as_raw_fd(), stat_buf.as_mut_ptr()) }; + if res == -1 { + return Err(io::Error::last_os_error()); + } + let stat_buf = unsafe { stat_buf.assume_init() }; + usize::try_from(stat_buf.st_size) + .map_err(|_| io::Error::new(ErrorKind::FileTooLarge, "pin is too large"))? + }; + + // map the memory from the file descriptor + let ptr = unsafe { + let ptr = libc::mmap( + std::ptr::null_mut(), + 4096, + PROT_READ | PROT_WRITE, + MAP_SHARED, + pin_fd.as_raw_fd(), + 0, + ); + if ptr == usize::MAX as *mut c_void { + return Err(std::io::Error::last_os_error()); + } + ptr as *const u8 + }; + + // Copy the bytes. + let buf = unsafe { + let mut buf: Vec = Vec::with_capacity(len); + ptr.copy_to_nonoverlapping(buf.as_mut_ptr().cast(), len); + buf.set_len(len); + buf + }; + + // Clean up mapping + unsafe { + if libc::munmap(ptr as *mut c_void, 4096) == -1 { + return Err(std::io::Error::last_os_error()); + } + } + drop(pin_fd); + + String::from_utf8(buf).map_err(|_| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + "invalid UTF-8 data found in buffer", + ) + }) +} + +pub fn write_secret(mut secret: Vec, max_len: usize) -> Result { + let bytes_len = if secret.len() <= max_len { + secret.len() as u8 + } else { + return Err(io::Error::new( + ErrorKind::FileTooLarge, + "value is too large", + )); + }; + + // Open memfd_secret + let ret: i64 = unsafe { libc::syscall(SYS_memfd_secret, O_CLOEXEC) }; + if ret == -1 { + return Err(std::io::Error::last_os_error()); + } + let fd = i32::try_from(ret).map_err(|_| std::io::Error::other("invalid file descriptor"))?; + if unsafe { ftruncate(fd, bytes_len as off_t) } == -1 { + return Err(std::io::Error::last_os_error()); + } + + let ptr = unsafe { + let ptr = mmap( + std::ptr::null_mut(), + 4096, + PROT_READ | PROT_WRITE, + MAP_SHARED, + fd, + 0, + ); + if ptr == usize::MAX as *mut c_void { + return Err(std::io::Error::last_os_error()); + } + // ptr as *mut u8 + ptr + }; + + // Copy the data + unsafe { + ptr.copy_from_nonoverlapping(secret.as_ptr().cast(), secret.len()); + } + + // Cleanup + if unsafe { libc::munmap(ptr, 4096) } == -1 { + return Err(std::io::Error::last_os_error()); + } + secret.zeroize(); + drop(secret); + + let owned_fd = unsafe { OwnedFd::from_raw_fd(fd) }; + Ok(owned_fd) +} diff --git a/credentialsd-ui/Cargo.toml b/credentialsd-ui/Cargo.toml index b28c98c2..79eeaa23 100644 --- a/credentialsd-ui/Cargo.toml +++ b/credentialsd-ui/Cargo.toml @@ -16,7 +16,6 @@ futures-lite.workspace = true gettext-rs = { version = "0.7", features = ["gettext-system"] } gtk = { version = "0.10.3", package = "gtk4", features = ["v4_14"] } gdk-wayland = { version = "0.10.3", package = "gdk4-wayland", optional = true } -libc.workspace = true qrcode = "0.14.1" serde.workspace = true tracing.workspace = true diff --git a/credentialsd-ui/src/client.rs b/credentialsd-ui/src/client.rs index c421bb86..52cd93bb 100644 --- a/credentialsd-ui/src/client.rs +++ b/credentialsd-ui/src/client.rs @@ -1,21 +1,13 @@ -use std::{ - io::ErrorKind, - os::{ - fd::{FromRawFd, OwnedFd}, - raw::c_void, - }, -}; - use async_std::{ channel::{Receiver, Sender}, sync::Mutex as AsyncMutex, }; -use libc::{ - MAP_SHARED, O_CLOEXEC, PROT_READ, PROT_WRITE, SYS_memfd_secret, ftruncate, mmap, off_t, + +use credentialsd_common::{ + memfd::write_secret, model::UserInteractedEvent, server::BackgroundEvent, }; -use zeroize::Zeroize; -use credentialsd_common::{model::UserInteractedEvent, server::BackgroundEvent}; +const CTAP_CLIENT_SECRET_MAX_LEN: usize = 63; #[derive(Debug)] pub struct FlowControlClient { @@ -29,7 +21,7 @@ impl FlowControlClient { } pub async fn enter_client_pin(&mut self, pin: String) -> Result<(), ()> { - let fd = match write_secret(pin) { + let fd = match write_secret(pin.into_bytes(), CTAP_CLIENT_SECRET_MAX_LEN) { Ok(fd) => fd, Err(err) => { tracing::error!(%err, "Failed to write secret to file descriptor"); @@ -66,61 +58,3 @@ impl FlowControlClient { } } } - -fn write_secret(secret: String) -> Result { - let mut bytes = secret.into_bytes(); - // CTAP pins are maximum of 63 bytes, so they should all fit in a u8. - let bytes_len = if bytes.len() <= 63 { - bytes.len() as u8 - } else { - return Err(std::io::Error::new( - ErrorKind::FileTooLarge, - "value is too large", - )); - }; - - // Open memfd_secret - let ret: i64 = unsafe { libc::syscall(SYS_memfd_secret, O_CLOEXEC) }; - if ret == -1 { - tracing::debug!("Failed to create memfd_secret"); - return Err(std::io::Error::last_os_error()); - } - let fd = i32::try_from(ret).map_err(|_| std::io::Error::other("invalid file descriptor"))?; - if unsafe { ftruncate(fd, bytes_len as off_t) } == -1 { - tracing::debug!("Failed to ftruncate memfd_secret"); - return Err(std::io::Error::last_os_error()); - } - - let ptr = unsafe { - let ptr = mmap( - std::ptr::null_mut(), - 4096, - PROT_READ | PROT_WRITE, - MAP_SHARED, - fd, - 0, - ); - if ptr == usize::MAX as *mut c_void { - tracing::debug!("Failed to mmap memfd_secret"); - return Err(std::io::Error::last_os_error()); - } - // ptr as *mut u8 - ptr - }; - - // Copy the data - unsafe { - ptr.copy_from_nonoverlapping(bytes.as_ptr().cast(), bytes.len()); - } - - // Cleanup - if unsafe { libc::munmap(ptr, 4096) } == -1 { - tracing::debug!("Failed to unmap memfd_secret"); - return Err(std::io::Error::last_os_error()); - } - bytes.zeroize(); - drop(bytes); - - let owned_fd = unsafe { OwnedFd::from_raw_fd(fd) }; - Ok(owned_fd) -} diff --git a/credentialsd/Cargo.toml b/credentialsd/Cargo.toml index e8703b7a..10885c01 100644 --- a/credentialsd/Cargo.toml +++ b/credentialsd/Cargo.toml @@ -12,7 +12,6 @@ base64 = "0.22.1" credentialsd-common = { path = "../credentialsd-common" } futures = "0.3.32" futures-lite.workspace = true -libc.workspace = true libwebauthn = { version = "0.8.0", features = ["nfc-backend-libnfc", "nfc-backend-pcsc", "reqwest-related-origins-source"] } # 0.6.1 fails to build with non-vendored library. # https://github.com/alexrsagen/rs-nfc1/issues/15 diff --git a/credentialsd/src/dbus/flow_control.rs b/credentialsd/src/dbus/flow_control.rs index ce883674..3da16f6f 100644 --- a/credentialsd/src/dbus/flow_control.rs +++ b/credentialsd/src/dbus/flow_control.rs @@ -3,20 +3,20 @@ use std::{ fmt::Debug, - io::{self, ErrorKind}, - mem::MaybeUninit, - os::{fd::AsRawFd, raw::c_void}, + os::fd::OwnedFd, sync::{Arc, Mutex}, }; use async_trait::async_trait; -use credentialsd_common::model::{ - Error as CredentialServiceError, Operation, PortalBackendOptions, UserInteractedEvent, - WebAuthnError, -}; use credentialsd_common::server::{BackgroundEvent, WindowHandle}; +use credentialsd_common::{ + memfd::read_secret, + model::{ + Error as CredentialServiceError, Operation, PortalBackendOptions, UserInteractedEvent, + WebAuthnError, + }, +}; use futures_lite::{Stream, StreamExt}; -use libc::{MAP_SHARED, PROT_READ, PROT_WRITE}; use tokio::sync::mpsc::Receiver; use tokio::sync::oneshot; use tokio::sync::{mpsc::Sender, Mutex as AsyncMutex}; @@ -198,7 +198,7 @@ async fn handle { - let pin_fd = std::os::fd::OwnedFd::from(pin_fd); + let pin_fd = OwnedFd::from(pin_fd); let pin = match read_secret(pin_fd.into()) { Ok(pin) => pin, // TODO: need to send an error to the UI, cancel the request and terminate the loop. @@ -314,60 +314,6 @@ impl CredentialRequestController for CredentialRequestControllerClient { } } -fn read_secret(pin_fd: std::os::fd::OwnedFd) -> Result { - // Get pin length - let len = { - let mut stat_buf = MaybeUninit::::uninit(); - let res = unsafe { libc::fstat(pin_fd.as_raw_fd(), stat_buf.as_mut_ptr()) }; - if res == -1 { - return Err(io::Error::last_os_error()); - } - let stat_buf = unsafe { stat_buf.assume_init() }; - usize::try_from(stat_buf.st_size) - .map_err(|_| io::Error::new(ErrorKind::FileTooLarge, "pin is too large"))? - }; - - // map the memory from the file descriptor - let ptr = unsafe { - let ptr = libc::mmap( - std::ptr::null_mut(), - 4096, - PROT_READ | PROT_WRITE, - MAP_SHARED, - pin_fd.as_raw_fd(), - 0, - ); - if ptr == usize::MAX as *mut c_void { - return Err(std::io::Error::last_os_error()); - } - ptr as *const u8 - }; - - // Copy the bytes. - let buf = unsafe { - // let len = ptr.read() as usize; - let mut buf: Vec = Vec::with_capacity(len); - ptr.copy_to_nonoverlapping(buf.as_mut_ptr().cast(), len); - buf.set_len(len); - buf - }; - - // Clean up mapping - unsafe { - if libc::munmap(ptr as *mut c_void, 4096) == -1 { - return Err(std::io::Error::last_os_error()); - } - } - drop(pin_fd); - - String::from_utf8(buf).map_err(|_| { - std::io::Error::new( - std::io::ErrorKind::InvalidData, - "invalid UTF-8 data found in buffer", - ) - }) -} - #[cfg(test)] pub mod test { use std::{ From f52e6d44894713ea82d6f33716078aa9a7f65ff9 Mon Sep 17 00:00:00 2001 From: Isaiah Inuwa Date: Sun, 21 Jun 2026 19:12:48 -0500 Subject: [PATCH 2/4] common: Use memfd_secret for Hybrid QR code --- credentialsd-common/src/memfd.rs | 29 +++++------ credentialsd-common/src/server.rs | 51 ++++++++++--------- credentialsd-ui/src/client.rs | 6 ++- credentialsd-ui/src/gui/view_model/mod.rs | 8 ++- credentialsd/src/credential_service/hybrid.rs | 15 ++++-- 5 files changed, 60 insertions(+), 49 deletions(-) diff --git a/credentialsd-common/src/memfd.rs b/credentialsd-common/src/memfd.rs index 52cfb0cf..7ea80c9f 100644 --- a/credentialsd-common/src/memfd.rs +++ b/credentialsd-common/src/memfd.rs @@ -65,23 +65,24 @@ pub fn read_secret(pin_fd: std::os::fd::OwnedFd) -> Result, max_len: usize) -> Result { - let bytes_len = if secret.len() <= max_len { - secret.len() as u8 - } else { +pub fn write_secret(mut secret: Vec) -> Result { + if secret.len() > 4096 { return Err(io::Error::new( ErrorKind::FileTooLarge, "value is too large", )); - }; + } // Open memfd_secret - let ret: i64 = unsafe { libc::syscall(SYS_memfd_secret, O_CLOEXEC) }; - if ret == -1 { - return Err(std::io::Error::last_os_error()); - } - let fd = i32::try_from(ret).map_err(|_| std::io::Error::other("invalid file descriptor"))?; - if unsafe { ftruncate(fd, bytes_len as off_t) } == -1 { + let fd = { + let ret = unsafe { libc::syscall(SYS_memfd_secret, O_CLOEXEC) }; + if ret == -1 { + return Err(std::io::Error::last_os_error()); + } + unsafe { OwnedFd::from_raw_fd(ret as i32) } + }; + + if unsafe { ftruncate(fd.as_raw_fd(), secret.len() as off_t) } == -1 { return Err(std::io::Error::last_os_error()); } @@ -91,13 +92,12 @@ pub fn write_secret(mut secret: Vec, max_len: usize) -> Result, max_len: usize) -> Result }, @@ -64,7 +64,7 @@ pub enum BackgroundEvent { SelectingCredential { creds: Vec }, HybridIdle, - HybridStarted(String), + HybridStarted(OwnedFd), HybridConnecting, HybridConnected, @@ -138,7 +138,7 @@ impl From<&BackgroundEvent> for Structure<'_> { Some(Value::U32(attempts_left.map(u32::from).unwrap_or(u32::MAX))) } BackgroundEvent::SelectingCredential { creds } => Some(Value::Array(creds.into())), - BackgroundEvent::HybridStarted(qr_data) => Some(Value::Str(qr_data.into())), + BackgroundEvent::HybridStarted(qr_data_fd) => Some(Value::Fd(qr_data_fd.into())), // Empty BackgroundEvent::CeremonyCompleted => None, BackgroundEvent::NeedsUserPresence => None, @@ -215,8 +215,8 @@ impl TryFrom<&Structure<'_>> for BackgroundEvent { BACKGROUND_EVENT_HYBRID_IDLE => Ok(Self::HybridIdle), BACKGROUND_EVENT_HYBRID_STARTED => { - let qr_data = value.downcast_ref::<&str>()?; - Ok(Self::HybridStarted(qr_data.to_string())) + let qr_data_fd = value.downcast_ref::()?.try_to_owned()?; + Ok(Self::HybridStarted(qr_data_fd.into())) } BACKGROUND_EVENT_HYBRID_CONNECTING => Ok(Self::HybridConnecting), BACKGROUND_EVENT_HYBRID_CONNECTED => Ok(Self::HybridConnected), @@ -636,9 +636,11 @@ fn tag_value_to_struct(tag: u32, value: Option>) -> Structure<'static> #[cfg(test)] mod test { + use std::os::fd::{FromRawFd, OwnedFd}; + use zvariant::{ - Type, serialized::{Context, Data, Format}, + Type, }; use super::{BackgroundEvent, Credential}; @@ -656,25 +658,28 @@ mod test { #[test] fn test_round_trip_background_hybrid_event() { - let event1 = BackgroundEvent::HybridStarted("FIDO:/1234".to_string()); + let mut fds = [0; 2]; + unsafe { + libc::pipe(fds.as_mut_ptr()); + } + // Wrap the raw fds into safe OwnedFd instances + let mock_fd = unsafe { OwnedFd::from_raw_fd(fds[0]) }; + let _mock_fd2 = unsafe { OwnedFd::from_raw_fd(fds[1]) }; + + println!("mock_fd: {mock_fd:?}"); + let event1 = BackgroundEvent::HybridStarted(mock_fd.into()); let ctx = zvariant::serialized::Context::new_dbus(zvariant::BE, 0); assert_eq!("(uv)", BackgroundEvent::SIGNATURE.to_string()); let data = zvariant::to_bytes(ctx, &event1).unwrap(); - let expected = b"\x00\x00\x00\x21\x01s\0\0\0\0\0\x0aFIDO:/1234\0"; + println!("data: {data:?}"); + // handle value is 0_u32 because it's the index into the list of fds in the message. Since + // there's only one, it will be 0. + let expected = b"\x00\x00\x00\x21\x01h\0\0\0\0\0\0"; assert_eq!(expected, data.bytes()); let event2 = data.deserialize().unwrap().0; - assert_eq!(event1, event2); - } - - #[test] - fn test_deserialize_background_hybrid_event() { - let bytes = b"\x00\x00\x00\x21\x01s\0\0\0\0\0\x0aFIDO:/1234\0"; - let data = Data::new(bytes, Context::new(Format::DBus, zvariant::BE, 0)); - let event: BackgroundEvent = data.deserialize().unwrap().0; - assert!(matches!( - event, - BackgroundEvent::HybridStarted(ref s) if s == "FIDO:/1234" - )); + // I believe that the fd is `dup()`'d through the serialization/deserialization process, so + // we can't compare the numbers for equality. + assert!(matches!(event2, BackgroundEvent::HybridStarted(_))); } #[test] diff --git a/credentialsd-ui/src/client.rs b/credentialsd-ui/src/client.rs index 52cd93bb..a18de293 100644 --- a/credentialsd-ui/src/client.rs +++ b/credentialsd-ui/src/client.rs @@ -21,7 +21,11 @@ impl FlowControlClient { } pub async fn enter_client_pin(&mut self, pin: String) -> Result<(), ()> { - let fd = match write_secret(pin.into_bytes(), CTAP_CLIENT_SECRET_MAX_LEN) { + if pin.len() > CTAP_CLIENT_SECRET_MAX_LEN { + tracing::error!("PIN is too long"); + return Err(()); + } + let fd = match write_secret(pin.into_bytes()) { Ok(fd) => fd, Err(err) => { tracing::error!(%err, "Failed to write secret to file descriptor"); diff --git a/credentialsd-ui/src/gui/view_model/mod.rs b/credentialsd-ui/src/gui/view_model/mod.rs index 3a5cb64b..70fcea45 100644 --- a/credentialsd-ui/src/gui/view_model/mod.rs +++ b/credentialsd-ui/src/gui/view_model/mod.rs @@ -7,6 +7,7 @@ use async_std::{ channel::{Receiver, Sender}, sync::Mutex as AsyncMutex, }; +use credentialsd_common::memfd::read_secret; use credentialsd_common::server::{BackgroundEvent, Credential}; use gettextrs::gettext; use serde::{Deserialize, Serialize}; @@ -33,10 +34,7 @@ pub(crate) struct ViewModel { devices: Vec, selected_device: Option, - // providers: Vec, - hybrid_qr_state: HybridState, hybrid_qr_code_data: Option>, - // hybrid_linked_state: HybridState, } impl ViewModel { @@ -67,7 +65,6 @@ impl ViewModel { subtitle: String::default(), devices, selected_device: None, - hybrid_qr_state: HybridState::default(), hybrid_qr_code_data: None, } } @@ -300,7 +297,8 @@ impl ViewModel { Event::Background(BackgroundEvent::HybridIdle) => { self.hybrid_qr_code_data = None; } - Event::Background(BackgroundEvent::HybridStarted(qr_code)) => { + Event::Background(BackgroundEvent::HybridStarted(qr_code_fd)) => { + let qr_code = read_secret(qr_code_fd.into()).unwrap(); self.hybrid_qr_code_data = Some(qr_code.clone().into_bytes()); self.tx_update .send(ViewUpdate::HybridNeedsQrCode(qr_code)) diff --git a/credentialsd/src/credential_service/hybrid.rs b/credentialsd/src/credential_service/hybrid.rs index c166865b..24d14351 100644 --- a/credentialsd/src/credential_service/hybrid.rs +++ b/credentialsd/src/credential_service/hybrid.rs @@ -2,10 +2,11 @@ use core::panic; use std::fmt::Debug; use async_stream::stream; -use credentialsd_common::server::BackgroundEvent; use futures_lite::Stream; -use tokio::sync::broadcast; -use tokio::sync::mpsc::{self, Sender}; +use tokio::sync::{ + broadcast, + mpsc::{self, Sender}, +}; use tracing::{debug, error}; use libwebauthn::transport::cable::channel::{CableUpdate, CableUxUpdate}; @@ -15,7 +16,7 @@ use libwebauthn::transport::cable::qr_code_device::{ use libwebauthn::transport::{Channel, ChannelSettings, Device}; use libwebauthn::webauthn::{Error as WebAuthnError, WebAuthn}; -use credentialsd_common::model::Error; +use credentialsd_common::{memfd::write_secret, model::Error, server::BackgroundEvent}; use crate::model::CredentialRequest; @@ -240,7 +241,11 @@ impl From for credentialsd_common::model::HybridState { impl From<&HybridState> for BackgroundEvent { fn from(value: &HybridState) -> Self { match value { - HybridState::Init(qr_code) => BackgroundEvent::HybridStarted(qr_code.to_string()), + HybridState::Init(qr_code) => { + let fd = write_secret(qr_code.clone().into_bytes()).unwrap(); + BackgroundEvent::HybridStarted(fd.into()) + } + HybridState::Connecting => BackgroundEvent::HybridConnecting, HybridState::Connected => BackgroundEvent::HybridConnected, HybridState::Completed => BackgroundEvent::CeremonyCompleted, From 159ccd5d659946045843a43e3e44e0e7e7570fe5 Mon Sep 17 00:00:00 2001 From: Isaiah Inuwa Date: Wed, 24 Jun 2026 23:09:47 -0500 Subject: [PATCH 3/4] ui: Set maximum width for client PIN entry --- credentialsd-ui/data/resources/ui/window.blp | 1 + 1 file changed, 1 insertion(+) diff --git a/credentialsd-ui/data/resources/ui/window.blp b/credentialsd-ui/data/resources/ui/window.blp index 6528bd87..7236515a 100644 --- a/credentialsd-ui/data/resources/ui/window.blp +++ b/credentialsd-ui/data/resources/ui/window.blp @@ -196,6 +196,7 @@ template $CredentialsUiWindow: ApplicationWindow { visible: bind template.view-model as <$CredentialManagerViewModel>.usb_nfc_pin_entry_visible; placeholder-text: _("Enter your device PIN"); can-focus: true; + max-width-chars: 63; } }; } From 5f9a2be621f87256d145335cfaaaafc66df02f04 Mon Sep 17 00:00:00 2001 From: Isaiah Inuwa Date: Thu, 25 Jun 2026 15:12:52 -0500 Subject: [PATCH 4/4] common: Structured drop for memfd_secret and mmap --- Cargo.lock | 1 + credentialsd-common/Cargo.toml | 1 + credentialsd-common/src/memfd.rs | 279 ++++++++++++------ credentialsd-common/src/server.rs | 8 +- credentialsd-ui/src/gui/view_model/mod.rs | 3 +- credentialsd/src/credential_service/hybrid.rs | 8 +- credentialsd/src/dbus/flow_control.rs | 8 +- 7 files changed, 216 insertions(+), 92 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fb8fcaff..535be0d2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -818,6 +818,7 @@ dependencies = [ "futures-lite", "libc", "serde", + "tracing", "zeroize", "zvariant", ] diff --git a/credentialsd-common/Cargo.toml b/credentialsd-common/Cargo.toml index fc0490a9..3a67f9c9 100644 --- a/credentialsd-common/Cargo.toml +++ b/credentialsd-common/Cargo.toml @@ -9,5 +9,6 @@ license = "LGPL-3.0-only" futures-lite.workspace = true libc.workspace = true serde = { workspace = true, features = ["derive"] } +tracing.workspace = true zeroize = "1.9.0" zvariant.workspace = true diff --git a/credentialsd-common/src/memfd.rs b/credentialsd-common/src/memfd.rs index 7ea80c9f..04060d48 100644 --- a/credentialsd-common/src/memfd.rs +++ b/credentialsd-common/src/memfd.rs @@ -1,117 +1,226 @@ use std::{ - io::{self, ErrorKind}, - mem::MaybeUninit, + io::{self, ErrorKind, Read, Write}, + mem::{ManuallyDrop, MaybeUninit}, os::{ fd::{AsRawFd, FromRawFd, OwnedFd}, raw::c_void, }, + ptr::{self, NonNull}, }; use libc::{ - ftruncate, mmap, off_t, SYS_memfd_secret, MAP_SHARED, O_CLOEXEC, PROT_READ, PROT_WRITE, + MAP_SHARED, MS_SYNC, O_CLOEXEC, PROT_READ, PROT_WRITE, SYS_memfd_secret, fstat, ftruncate, + mmap, msync, munmap, off_t, syscall, }; use zeroize::Zeroize; -pub fn read_secret(pin_fd: std::os::fd::OwnedFd) -> Result { - // Get pin length - let len = { - let mut stat_buf = MaybeUninit::::uninit(); - let res = unsafe { libc::fstat(pin_fd.as_raw_fd(), stat_buf.as_mut_ptr()) }; - if res == -1 { - return Err(io::Error::last_os_error()); - } - let stat_buf = unsafe { stat_buf.assume_init() }; - usize::try_from(stat_buf.st_size) - .map_err(|_| io::Error::new(ErrorKind::FileTooLarge, "pin is too large"))? - }; - - // map the memory from the file descriptor - let ptr = unsafe { - let ptr = libc::mmap( - std::ptr::null_mut(), - 4096, - PROT_READ | PROT_WRITE, - MAP_SHARED, - pin_fd.as_raw_fd(), - 0, - ); - if ptr == usize::MAX as *mut c_void { - return Err(std::io::Error::last_os_error()); - } - ptr as *const u8 - }; - - // Copy the bytes. - let buf = unsafe { - let mut buf: Vec = Vec::with_capacity(len); - ptr.copy_to_nonoverlapping(buf.as_mut_ptr().cast(), len); - buf.set_len(len); - buf - }; +/// On most architectures, the minimum page size is 4KB, so we use that as a baseline for creating +/// memory-mapped files. If the page size is larger, the OS will just map a larger page than we +/// need. +const MIN_PAGE_SIZE: usize = 4096; - // Clean up mapping - unsafe { - if libc::munmap(ptr as *mut c_void, 4096) == -1 { - return Err(std::io::Error::last_os_error()); - } - } - drop(pin_fd); - - String::from_utf8(buf).map_err(|_| { - std::io::Error::new( - std::io::ErrorKind::InvalidData, - "invalid UTF-8 data found in buffer", - ) - }) +/// Read a secret from a memory-mapped file. +pub fn read_secret(fd: OwnedFd) -> io::Result> { + let mut reader = MmapReader::from_fd(fd)?; + let mut buf = Vec::new(); + reader.read_to_end(&mut buf)?; + Ok(buf) } -pub fn write_secret(mut secret: Vec) -> Result { - if secret.len() > 4096 { +/// Write a secret to a memfd_secret and return its file descriptor. The secret will be zeroized +/// from memory. +pub fn write_secret(mut secret: Vec) -> io::Result { + // For now, we're only accepting values that fit within a single page. + // This can be raised in the future if needed. + if secret.len() > MIN_PAGE_SIZE { return Err(io::Error::new( ErrorKind::FileTooLarge, "value is too large", )); } - // Open memfd_secret - let fd = { - let ret = unsafe { libc::syscall(SYS_memfd_secret, O_CLOEXEC) }; - if ret == -1 { - return Err(std::io::Error::last_os_error()); + let memfd_secret = open_memfd_secret(secret.len())?; + let mut mem = Mmap::from_fd(memfd_secret)?; + mem.write_all(&secret)?; + secret.zeroize(); + drop(secret); + + Ok(mem.into_fd()) +} + +struct Mmap { + inner: NonNull, + fd: OwnedFd, + size: usize, + pos: usize, +} + +impl Mmap { + fn from_fd(fd: OwnedFd) -> io::Result { + let size = MIN_PAGE_SIZE; + let ptr = unsafe { + let ptr = mmap( + ptr::null_mut(), + size, + PROT_READ | PROT_WRITE, + MAP_SHARED, + fd.as_raw_fd(), + 0, + ); + if ptr == usize::MAX as *mut c_void { + let err = io::Error::last_os_error(); + tracing::error!(%err, "mmap failed"); + return Err(err); + } + NonNull::new(ptr as *mut u8) + .ok_or_else(|| io::Error::other("mmap returned NULL pointer"))? + }; + + return Ok(Self { + inner: ptr, + fd, + size, + pos: 0, + }); + } + + fn into_fd(self) -> OwnedFd { + let mmap = ManuallyDrop::new(self); + assert!(unsafe { munmap(mmap.inner.as_ptr().cast(), 4096) != -1 }); + unsafe { ptr::read(&mmap.fd) } + } +} + +impl Write for Mmap { + fn write(&mut self, buf: &[u8]) -> io::Result { + let remaining = self.size - self.pos; + if remaining == 0 { + return Ok(0); } - unsafe { OwnedFd::from_raw_fd(ret as i32) } - }; - if unsafe { ftruncate(fd.as_raw_fd(), secret.len() as off_t) } == -1 { - return Err(std::io::Error::last_os_error()); + let bytes_to_write = usize::min(remaining, buf.len()); + unsafe { + self.inner + .as_ptr() + .wrapping_add(self.pos) + .copy_from_nonoverlapping(buf.as_ptr(), bytes_to_write) + }; + self.pos += bytes_to_write; + + Ok(bytes_to_write) } - let ptr = unsafe { - let ptr = mmap( - std::ptr::null_mut(), - 4096, - PROT_READ | PROT_WRITE, - MAP_SHARED, - fd.as_raw_fd(), - 0, - ); - if ptr == usize::MAX as *mut c_void { - return Err(std::io::Error::last_os_error()); + fn flush(&mut self) -> io::Result<()> { + // No-op if there's no bytes to flush, prevents errors on call to msyc + if self.pos == 0 { + return Ok(()); } - ptr - }; - // Copy the data - unsafe { - ptr.copy_from_nonoverlapping(secret.as_ptr().cast(), secret.len()); + if unsafe { msync(self.inner.as_ptr().cast(), self.pos, MS_SYNC) == -1 } { + let err = io::Error::last_os_error(); + // msync is invalid on some file descriptors, so we ignore the error if called on one of those. + let ErrorKind::InvalidInput = err.kind() else { + tracing::error!("Failed to flush bytes"); + return Err(err); + }; + } + Ok(()) } +} - // Cleanup - if unsafe { libc::munmap(ptr, 4096) } == -1 { - return Err(std::io::Error::last_os_error()); +impl Drop for Mmap { + fn drop(&mut self) { + unsafe { + assert!(munmap(self.inner.as_ptr().cast(), MIN_PAGE_SIZE) != -1); + } } - secret.zeroize(); - drop(secret); +} + +struct MmapReader { + inner: NonNull, + _fd: OwnedFd, + size: usize, + pos: usize, +} + +impl MmapReader { + fn from_fd(fd: OwnedFd) -> io::Result { + let ptr = unsafe { + let size = MIN_PAGE_SIZE; + let flags = PROT_READ; + let ptr = mmap(ptr::null_mut(), size, flags, MAP_SHARED, fd.as_raw_fd(), 0); + if ptr == usize::MAX as *mut c_void { + let err = io::Error::last_os_error(); + tracing::error!(%err, "mmap failed"); + return Err(err); + } + NonNull::new(ptr as *mut u8) + .ok_or_else(|| io::Error::other("mmap returned NULL pointer"))? + }; + + // actual size of the data in the file + let size = { + let mut stat_buf = MaybeUninit::::uninit(); + let res = unsafe { fstat(fd.as_raw_fd(), stat_buf.as_mut_ptr()) }; + if res == -1 { + tracing::error!("fstat failed"); + return Err(io::Error::last_os_error()); + } + let stat_buf = unsafe { stat_buf.assume_init() }; + usize::try_from(stat_buf.st_size) + .map_err(|_| io::Error::new(ErrorKind::FileTooLarge, "file is too large"))? + }; + Ok(Self { + inner: ptr, + _fd: fd, + size, + pos: 0, + }) + } +} + +impl Drop for MmapReader { + fn drop(&mut self) { + unsafe { + assert!(munmap(self.inner.as_ptr().cast(), MIN_PAGE_SIZE) != -1); + } + } +} +impl Read for MmapReader { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + let remaining = self.size - self.pos; + if remaining == 0 { + return Ok(0); + } + let bytes_to_read = usize::min(remaining, buf.len()); + unsafe { + self.inner + .as_ptr() + .wrapping_add(self.pos) + .copy_to_nonoverlapping(buf.as_mut_ptr(), bytes_to_read); + } + self.pos += bytes_to_read; + Ok(bytes_to_read) + } +} + +fn open_memfd_secret(len: usize) -> io::Result { + let len = off_t::try_from(len) + .map_err(|_| io::Error::new(ErrorKind::FileTooLarge, "File is too large"))?; + + // Open memfd_secret + let fd = { + let ret = unsafe { syscall(SYS_memfd_secret, O_CLOEXEC) }; + if ret == -1 { + return Err(io::Error::last_os_error()); + } + unsafe { OwnedFd::from_raw_fd(ret as i32) } + }; + + // Set length on fd. We have to do this before memory-mapping it. + if unsafe { ftruncate(fd.as_raw_fd(), len) } == -1 { + return Err(io::Error::last_os_error()); + } Ok(fd) } diff --git a/credentialsd-common/src/server.rs b/credentialsd-common/src/server.rs index f785acb8..c670e75f 100644 --- a/credentialsd-common/src/server.rs +++ b/credentialsd-common/src/server.rs @@ -3,12 +3,12 @@ use std::{collections::HashMap, fmt::Display}; use serde::{ - de::{DeserializeSeed, Error, Visitor}, Deserialize, Serialize, + de::{DeserializeSeed, Error, Visitor}, }; use zvariant::{ - self, signature::Fields, Array, DeserializeDict, DynamicDeserialize, Fd, NoneValue, Optional, - OwnedFd, OwnedValue, SerializeDict, Signature, Str, Structure, StructureBuilder, Type, Value, + self, Array, DeserializeDict, DynamicDeserialize, Fd, NoneValue, Optional, OwnedFd, OwnedValue, + SerializeDict, Signature, Str, Structure, StructureBuilder, Type, Value, signature::Fields, }; use crate::model::{Device, Operation, RequestId, UserInteractedEvent}; @@ -639,8 +639,8 @@ mod test { use std::os::fd::{FromRawFd, OwnedFd}; use zvariant::{ - serialized::{Context, Data, Format}, Type, + serialized::{Context, Data, Format}, }; use super::{BackgroundEvent, Credential}; diff --git a/credentialsd-ui/src/gui/view_model/mod.rs b/credentialsd-ui/src/gui/view_model/mod.rs index 70fcea45..25390a3b 100644 --- a/credentialsd-ui/src/gui/view_model/mod.rs +++ b/credentialsd-ui/src/gui/view_model/mod.rs @@ -298,7 +298,8 @@ impl ViewModel { self.hybrid_qr_code_data = None; } Event::Background(BackgroundEvent::HybridStarted(qr_code_fd)) => { - let qr_code = read_secret(qr_code_fd.into()).unwrap(); + let qr_code_bytes = read_secret(qr_code_fd.into()).unwrap(); + let qr_code = String::from_utf8(qr_code_bytes).unwrap(); self.hybrid_qr_code_data = Some(qr_code.clone().into_bytes()); self.tx_update .send(ViewUpdate::HybridNeedsQrCode(qr_code)) diff --git a/credentialsd/src/credential_service/hybrid.rs b/credentialsd/src/credential_service/hybrid.rs index 24d14351..b531f9f8 100644 --- a/credentialsd/src/credential_service/hybrid.rs +++ b/credentialsd/src/credential_service/hybrid.rs @@ -242,7 +242,13 @@ impl From<&HybridState> for BackgroundEvent { fn from(value: &HybridState) -> Self { match value { HybridState::Init(qr_code) => { - let fd = write_secret(qr_code.clone().into_bytes()).unwrap(); + let fd = match write_secret(qr_code.clone().into_bytes()) { + Ok(fd) => fd, + Err(err) => { + tracing::error!(%err, "Failed to write QR code secret"); + panic!("Failed to write QR code secret"); + } + }; BackgroundEvent::HybridStarted(fd.into()) } diff --git a/credentialsd/src/dbus/flow_control.rs b/credentialsd/src/dbus/flow_control.rs index 3da16f6f..3f4659f0 100644 --- a/credentialsd/src/dbus/flow_control.rs +++ b/credentialsd/src/dbus/flow_control.rs @@ -199,7 +199,13 @@ async fn handle { let pin_fd = OwnedFd::from(pin_fd); - let pin = match read_secret(pin_fd.into()) { + let pin = match read_secret(pin_fd.into()) + .map_err(|err| format!("Could not read from file descriptor: {err}")) + .and_then(|bytes| { + String::from_utf8(bytes).map_err(|err| { + format!("Invalid UTF-8 data retrieved from pin: {err}") + }) + }) { Ok(pin) => pin, // TODO: need to send an error to the UI, cancel the request and terminate the loop. Err(err) => {