Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions credentialsd-common/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions credentialsd-common/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
pub mod client;
pub mod memfd;
pub mod model;
pub mod server;
118 changes: 118 additions & 0 deletions credentialsd-common/src/memfd.rs
Original file line number Diff line number Diff line change
@@ -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<String, std::io::Error> {
// Get pin length
let len = {
let mut stat_buf = MaybeUninit::<libc::stat>::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<u8> = 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<u8>, max_len: usize) -> Result<OwnedFd, std::io::Error> {
let bytes_len = if secret.len() <= max_len {
secret.len() as u8

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

max_len here is CTAP_HYBRID_QR_CODE_MAX_LEN (4k). Won't fit in u8.

usize?

Alternatively we could reduce CTAP_HYBRID_QR_CODE_MAX_LEN until PQ, I think we're comfortably below 255 right now.

} 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());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaks fd. Could you wrap fd in OwnedFd::from_raw_fd early on, so it's safe to drop at any point?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can still use owned_fd.as_raw_fd() where you need it.

}

let ptr = unsafe {
let ptr = mmap(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Ideally wrap in a struct with Drop so we can add more early exit branches safely here

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)
}
51 changes: 28 additions & 23 deletions credentialsd-common/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
use std::{collections::HashMap, fmt::Display};

use serde::{
Deserialize, Serialize,
de::{DeserializeSeed, Error, Visitor},
Deserialize, Serialize,
};
use zvariant::{
self, Array, DeserializeDict, DynamicDeserialize, Fd, NoneValue, Optional, OwnedFd, OwnedValue,
SerializeDict, Signature, Str, Structure, StructureBuilder, Type, Value, signature::Fields,
self, signature::Fields, Array, DeserializeDict, DynamicDeserialize, Fd, NoneValue, Optional,
OwnedFd, OwnedValue, SerializeDict, Signature, Str, Structure, StructureBuilder, Type, Value,
};

use crate::model::{Device, Operation, RequestId, UserInteractedEvent};
Expand Down Expand Up @@ -55,7 +55,7 @@ const USER_INTERACTED_EVENT_CREDENTIAL_SELECTED: u32 = 0x05;
const USER_INTERACTED_EVENT_REQUEST_CANCELLED: u32 = 0x06;

/// Flattened enum BackgroundEvent for sending across D-Bus.
#[derive(Debug, Clone, PartialEq)]
#[derive(Debug, PartialEq)]
pub enum BackgroundEvent {
CeremonyCompleted,
NeedsPin { attempts_left: Option<u32> },
Expand All @@ -64,7 +64,7 @@ pub enum BackgroundEvent {
SelectingCredential { creds: Vec<Credential> },

HybridIdle,
HybridStarted(String),
HybridStarted(OwnedFd),
HybridConnecting,
HybridConnected,

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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::<Fd>()?.try_to_owned()?;
Ok(Self::HybridStarted(qr_data_fd.into()))
}
BACKGROUND_EVENT_HYBRID_CONNECTING => Ok(Self::HybridConnecting),
BACKGROUND_EVENT_HYBRID_CONNECTED => Ok(Self::HybridConnected),
Expand Down Expand Up @@ -636,9 +636,11 @@ fn tag_value_to_struct(tag: u32, value: Option<Value<'_>>) -> Structure<'static>

#[cfg(test)]
mod test {
use std::os::fd::{FromRawFd, OwnedFd};

use zvariant::{
Type,
serialized::{Context, Data, Format},
Type,
};

use super::{BackgroundEvent, Credential};
Expand All @@ -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]
Expand Down
1 change: 0 additions & 1 deletion credentialsd-ui/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
76 changes: 5 additions & 71 deletions credentialsd-ui/src/client.rs
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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");
Expand Down Expand Up @@ -66,61 +58,3 @@ impl FlowControlClient {
}
}
}

fn write_secret(secret: String) -> Result<OwnedFd, std::io::Error> {
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)
}
Loading
Loading