Two soundness fixes based on KEYCTL_READ interface misunderstanding

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<T: AsMut<[u8]>>(&self, buffer: &mut T) -> Result<usize, KeyError> {
+    pub fn read<T: AsMut<[u8]> + ?Sized>(&self, buffer: &mut T) -> Result<usize, KeyError> {
+        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<T: AsRef<[u8]>>(&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());
+    }
 }

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::<KeySerialId>()) as _
         )? as usize;
 
+        // The len is the actual size of the ring, not the consumed buffer.
+        let kernel_len = len / core::mem::size_of::<KeySerialId>();
+
         // Set the size of the results
         unsafe {
-            buffer.set_len(len / core::mem::size_of::<KeySerialId>());
+            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);
+    }
 }