From c9cdb86c220ada6b88e81f3395a8558d4bca3704 Mon Sep 17 00:00:00 2001 From: "A. Molzer" <5550310+197g@users.noreply.github.com> Date: Tue, 3 Nov 2026 09:50:53 +0100 Subject: [PATCH 1/2] Fix: Key::{read,update} unsoundness+documentation The safe traits AsMut/AsRef place no strict requirements on their implementations to provide a stable result, i.e. dereference to the same buffer when called multiple times. It's thus always wrong to recombine the results from two separate calls, especially address and length data when their Target is a slice. Instead, the buffer must be queried once and that slice be retained. --- src/key.rs | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/src/key.rs b/src/key.rs index d753702..064953f 100644 --- a/src/key.rs +++ b/src/key.rs @@ -60,18 +60,20 @@ impl Key { /// Read the payload data of a key into a provided mutable slice. /// - /// The returned usize is the number of bytes read into the slice. + /// The returned usize is the size of the key in bytes. The buffer is filled as much as + /// possible. If the buffer is too small for the key, the bytes have an unspecified value. /// /// The key must either grant the caller read permission, or grant /// the caller search permission when searched for from the process /// keyrings (i.e., the key is possessed). - pub fn read>(&self, buffer: &mut T) -> Result { + pub fn read + ?Sized>(&self, buffer: &mut T) -> Result { + let buffer = buffer.as_mut(); // TODO: alternate key types? Currenlty we don't support KeyType::BigKey let len = ffi::keyctl!( KeyCtlOperation::Read, self.0.as_raw_id() as libc::c_ulong, - buffer.as_mut().as_mut_ptr() as _, - buffer.as_mut().len() as _ + buffer.as_mut_ptr() as _, + buffer.len() as _ )? as usize; Ok(len) } @@ -108,11 +110,12 @@ impl Key { /// A negatively instantiated key (see the description of [Key::reject]) /// can be positively instantiated with this operation. pub fn update>(&self, update: &T) -> Result<(), KeyError> { + let update = update.as_ref(); _ = ffi::keyctl!( KeyCtlOperation::Update, self.0.as_raw_id() as libc::c_ulong, - update.as_ref().as_ptr() as _, - update.as_ref().len() as _ + update.as_ptr() as _, + update.len() as _ )?; Ok(()) } @@ -356,4 +359,24 @@ mod tests { assert_eq!("wow".as_bytes(), &buf[..len]); key.invalidate().unwrap() } + + #[test] + fn test_buffer_sizing() { + let secret = "Test Data"; + + // Obtain the Process keyring, create for the test process if necessary. + let ring = KeyRing::from_special_id(KeyRingIdentifier::Process, true).unwrap(); + + // Create the key + let key = ring.add_key("my-super-secret-test-key", secret).unwrap(); + + let apparent_len = key.read(&mut []).unwrap(); + assert_eq!(apparent_len, secret.len()); + + // A buffer that is ensured to be zeroed when + // out of scope + let mut buf = Zeroizing::new([0u8; 4096]); + let len = key.read(&mut buf).unwrap(); + assert_eq!(len, secret.len()); + } } From 6b4c84364ea966f0cc2a1614eb26498c835182c4 Mon Sep 17 00:00:00 2001 From: "A. Molzer" <5550310+197g@users.noreply.github.com> Date: Tue, 3 Nov 2026 10:07:53 +0100 Subject: [PATCH 2/2] Fix keyring get_links unsoundness Two counts: the size of the buffer passed was not its length in bytes but its item count, making the kernel only fill it at most partially. The kernel enforces that the length is a multiple of the identifier size for keyrings (it won't partially write IDs with unclear endianess). More crucially, we'd trust the returned length when in reality the *actual* length of the keyring is returned and *not* the filled size as with a call to `read`. This would lead to a `Vec`'s length exceeding its capacity and thus an out of bounds reference in the later iteration. --- src/keyring.rs | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/keyring.rs b/src/keyring.rs index 005d220..e826192 100644 --- a/src/keyring.rs +++ b/src/keyring.rs @@ -169,12 +169,15 @@ impl KeyRing { KeyCtlOperation::Read, self.id.as_raw_id() as libc::c_ulong, buffer.as_mut_ptr() as _, - buffer.capacity() as _ + (buffer.capacity() * core::mem::size_of::()) as _ )? as usize; + // The len is the actual size of the ring, not the consumed buffer. + let kernel_len = len / core::mem::size_of::(); + // Set the size of the results unsafe { - buffer.set_len(len / core::mem::size_of::()); + buffer.set_len(kernel_len.min(max)); } // Remap the results to complete keys @@ -447,6 +450,7 @@ mod test { let items = sess.get_links(200).unwrap(); assert!(!items.contains(&thread)); } + #[test] fn test_link_unlink_keyrings_with_id() { // Get a couple of unlinked keyrings to test @@ -522,4 +526,21 @@ mod test { let items = ring.get_links(200).unwrap(); assert!(!items.contains(&key)); } + + #[test] + fn get_linked_maximum() { + // Ensure a ring that is definitely filled with at least the thread ring link. + let ring = KeyRing::from_special_id(KeyRingIdentifier::Thread, true).unwrap(); + assert!(ring.id.as_raw_id() > 0); + let _key0 = ring.add_key("test_read_key0", b"test").unwrap(); + + // We must not get any items if we did not request them. + let items = ring.get_links(0).unwrap(); + assert_eq!(items.len(), 0); + + // The first link we read might not be a linked item, we skip it. + let items = ring.get_links(1).unwrap(); + assert!(items.len() <= 1); + assert!(items.capacity() >= 1); + } }