More suggestions from @martinthomson

fuzz/fuzz_targets/packet.rs

@@ -14,8 +14,7 @@ fuzz_target!(|data: &[u8]| {
     neqo_crypto::init().unwrap();
 
     // Run the fuzzer
-    let mut d = data.to_vec();
-    _ = PublicPacket::decode(&mut d, decoder);
+    _ = PublicPacket::decode(&mut data.to_vec(), decoder);
 });
 
 #[cfg(any(not(fuzzing), windows))]

neqo-common/src/datagram.rs

@@ -96,7 +96,7 @@ impl<D: AsRef<[u8]>> std::fmt::Debug for Datagram<D> {
 
 impl<'a> Datagram<&'a mut [u8]> {
     #[must_use]
-    pub fn from_slice(src: SocketAddr, dst: SocketAddr, tos: IpTos, d: &'a mut [u8]) -> Self {
+    pub const fn from_slice(src: SocketAddr, dst: SocketAddr, tos: IpTos, d: &'a mut [u8]) -> Self {
         Self { src, dst, tos, d }
     }
 

neqo-crypto/src/aead.rs

@@ -6,7 +6,7 @@
 
 use std::{
     fmt,
-    ops::{Deref, DerefMut, Range},
+    ops::{Deref, DerefMut},
     os::raw::{c_char, c_uint},
     ptr::null_mut,
 };
@@ -18,7 +18,6 @@ use crate::{
     p11::{PK11SymKey, SymKey},
     scoped_ptr,
     ssl::{PRUint16, PRUint64, PRUint8, SSLAeadContext},
-    Error,
 };
 
 experimental_api!(SSL_MakeAead(
@@ -124,49 +123,36 @@ impl RealAead {
                 c_uint::try_from(output.len())?,
             )
         }?;
-        Ok(&output[0..(l.try_into()?)])
+        Ok(&output[..l.try_into()?])
     }
 
-    /// Encrypt `data` consisting of `aad` and plaintext `input` in place.
+    /// Encrypt `data` consisting of `aad` and plaintext `data` in place.
     ///
     /// The space provided in `data` needs to allow `Aead::expansion` more bytes to be appended.
     ///
     /// # Errors
     ///
-    /// If the input can't be protected or any input is too large for NSS, or `aad` or `input` are
-    /// not valid ranges into `data`
-    pub fn encrypt_in_place(
-        &self,
-        count: u64,
-        aad: Range<usize>,
-        input: Range<usize>,
-        data: &[u8],
-    ) -> Res<usize> {
-        let aad = data.get(aad).ok_or(Error::AeadError)?;
-        let input = data.get(input).ok_or(Error::AeadError)?;
+    /// If the input can't be protected or any input is too large for NSS.
+    pub fn encrypt_in_place(&self, count: u64, aad: &[u8], data: &[u8]) -> Res<usize> {
         let mut l: c_uint = 0;
         unsafe {
             SSL_AeadEncrypt(
                 *self.ctx,
                 count,
                 aad.as_ptr(),
                 c_uint::try_from(aad.len())?,
-                input.as_ptr(),
-                c_uint::try_from(input.len())?,
-                input.as_ptr(),
+                data.as_ptr(),
+                c_uint::try_from(data.len() - self.expansion())?,
+                data.as_ptr(),
                 &mut l,
-                c_uint::try_from(input.len() + self.expansion())?,
+                c_uint::try_from(data.len())?,
             )
         }?;
         Ok(l.try_into()?)
     }
 
     /// Decrypt a ciphertext.
     ///
-    /// Note that NSS insists upon having extra space available for decryption, so
-    /// the buffer for `output` should be the same length as `input`, even though
-    /// the final result will be shorter.
-    ///
     /// # Errors
     ///
     /// If the input isn't authenticated or any input is too large for NSS.
@@ -179,6 +165,9 @@ impl RealAead {
     ) -> Res<&'a [u8]> {
         let mut l: c_uint = 0;
         unsafe {
+            // Note that NSS insists upon having extra space available for decryption, so
+            // the buffer for `output` should be the same length as `input`, even though
+            // the final result will be shorter.
             SSL_AeadDecrypt(
                 *self.ctx,
                 count,
@@ -191,24 +180,20 @@ impl RealAead {
                 c_uint::try_from(output.len())?,
             )
         }?;
-        Ok(&output[0..(l.try_into()?)])
+        Ok(&output[..l.try_into()?])
     }
 
     /// Decrypt a ciphertext in place.
     ///
     /// # Errors
     ///
-    /// If the input isn't authenticated or any input is too large for NSS, or `aad` or `input` are
-    /// not valid ranges into `data`
+    /// If the input isn't authenticated or any input is too large for NSS.
     pub fn decrypt_in_place<'a>(
         &self,
         count: u64,
-        aad: Range<usize>,
-        input: Range<usize>,
+        aad: &[u8],
         data: &'a mut [u8],
     ) -> Res<&'a mut [u8]> {
-        let aad = data.get(aad).ok_or(Error::AeadError)?;
-        let inp = data.get(input.clone()).ok_or(Error::AeadError)?;
         let mut l: c_uint = 0;
         unsafe {
             // Note that NSS insists upon having extra space available for decryption, so
@@ -219,14 +204,14 @@ impl RealAead {
                 count,
                 aad.as_ptr(),
                 c_uint::try_from(aad.len())?,
-                inp.as_ptr(),
-                c_uint::try_from(inp.len())?,
-                inp.as_ptr(),
+                data.as_ptr(),
+                c_uint::try_from(data.len())?,
+                data.as_ptr(),
                 &mut l,
-                c_uint::try_from(inp.len())?,
+                c_uint::try_from(data.len())?,
             )
         }?;
-        Ok(&mut data[input.start..input.start + usize::try_from(l)?])
+        Ok(&mut data[..l.try_into()?])
     }
 }
 

neqo-crypto/src/aead_null.rs

@@ -4,7 +4,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use std::{fmt, ops::Range};
+use std::fmt;
 
 use crate::{
     constants::{Cipher, Version},
@@ -47,15 +47,10 @@ impl AeadNull {
     }
 
     #[allow(clippy::missing_errors_doc)]
-    pub fn encrypt_in_place(
-        &self,
-        _count: u64,
-        _aad: Range<usize>,
-        input: Range<usize>,
-        data: &mut [u8],
-    ) -> Res<usize> {
-        data[input.end..input.end + self.expansion()].copy_from_slice(AEAD_NULL_TAG);
-        Ok(input.len() + self.expansion())
+    pub fn encrypt_in_place(&self, _count: u64, _aad: &[u8], data: &mut [u8]) -> Res<usize> {
+        let pos = data.len() - self.expansion();
+        data[pos..].copy_from_slice(AEAD_NULL_TAG);
+        Ok(data.len())
     }
 
     fn decrypt_check(&self, _count: u64, _aad: &[u8], input: &[u8]) -> Res<usize> {
@@ -95,18 +90,11 @@ impl AeadNull {
     pub fn decrypt_in_place<'a>(
         &self,
         count: u64,
-        aad: Range<usize>,
-        input: Range<usize>,
+        aad: &[u8],
         data: &'a mut [u8],
     ) -> Res<&'a mut [u8]> {
-        let aad = data
-            .get(aad)
-            .ok_or_else(|| Error::from(SEC_ERROR_BAD_DATA))?;
-        let inp = data
-            .get(input.clone())
-            .ok_or_else(|| Error::from(SEC_ERROR_BAD_DATA))?;
-        self.decrypt_check(count, aad, inp)
-            .map(move |len| &mut data[input.start..input.start + len])
+        self.decrypt_check(count, aad, data)
+            .map(move |len| &mut data[..len])
     }
 }
 

neqo-transport/src/crypto.rs

@@ -635,32 +635,30 @@ impl CryptoDxState {
         self.used_pn.end
     }
 
-    pub fn encrypt(
-        &mut self,
-        pn: PacketNumber,
-        hdr: Range<usize>,
-        body: Range<usize>,
-        data: &mut [u8],
-    ) -> Res<usize> {
+    pub fn encrypt(&mut self, pn: PacketNumber, hdr: Range<usize>, data: &mut [u8]) -> Res<usize> {
         debug_assert_eq!(self.direction, CryptoDxDirection::Write);
         qtrace!(
             "[{self}] encrypt_in_place pn={pn} hdr={} body={}",
             hex(data[hdr.clone()].as_ref()),
-            hex(data[body.clone()].as_ref())
+            hex(data[hdr.end..].as_ref())
         );
 
         // The numbers in `Self::limit` assume a maximum packet size of `LIMIT`.
         // Adjust them as we encounter larger packets.
-        debug_assert!(body.len() < 65536);
-        if body.len() > self.largest_packet_len {
+        let body_len = data.len() - hdr.len() - self.aead.expansion();
+        debug_assert!(body_len <= u16::MAX.into());
+        if body_len > self.largest_packet_len {
             let new_bits = usize::leading_zeros(self.largest_packet_len - 1)
-                - usize::leading_zeros(body.len() - 1);
+                - usize::leading_zeros(body_len - 1);
             self.invocations >>= new_bits;
-            self.largest_packet_len = body.len();
+            self.largest_packet_len = body_len;
         }
         self.invoked()?;
 
-        let len = self.aead.encrypt_in_place(pn, hdr, body, data)?;
+        let (prev, data) = data.split_at_mut(hdr.end);
+        // `prev` may have already-encrypted packets this one is being coalesced with.
+        // Use only the actual current header for AAD.
+        let len = self.aead.encrypt_in_place(pn, &prev[hdr], data)?;
 
         qtrace!("[{self}] encrypt ct={}", hex(data));
         debug_assert_eq!(pn, self.next_pn());
@@ -677,17 +675,17 @@ impl CryptoDxState {
         &mut self,
         pn: PacketNumber,
         hdr: Range<usize>,
-        body: Range<usize>,
         data: &'a mut [u8],
     ) -> Res<&'a mut [u8]> {
         debug_assert_eq!(self.direction, CryptoDxDirection::Read);
         qtrace!(
             "[{self}] decrypt_in_place pn={pn} hdr={} body={}",
             hex(data[hdr.clone()].as_ref()),
-            hex(data[body.clone()].as_ref())
+            hex(data[hdr.end..].as_ref())
         );
         self.invoked()?;
-        let data = self.aead.decrypt_in_place(pn, hdr, body, data)?;
+        let (hdr, data) = data.split_at_mut(hdr.end);
+        let data = self.aead.decrypt_in_place(pn, hdr, data)?;
         self.used(pn)?;
         Ok(data)
     }

neqo-transport/src/packet/mod.rs

@@ -423,12 +423,7 @@ impl PacketBuilder {
         let data_end = self.encoder.len();
         self.pad_to(data_end + crypto.expansion(), 0);
 
-        let ciphertext_len = crypto.encrypt(
-            self.pn,
-            self.header.clone(),
-            self.header.end..data_end,
-            self.encoder.as_mut(),
-        )?;
+        let ciphertext_len = crypto.encrypt(self.pn, self.header.clone(), self.encoder.as_mut())?;
 
         // Calculate the mask.
         let ciphertext = &self.encoder.as_mut()[self.header.end..self.header.end + ciphertext_len];
@@ -812,7 +807,7 @@ impl<'a> PublicPacket<'a> {
     fn decrypt_header(
         &mut self,
         crypto: &CryptoDxState,
-    ) -> Res<(bool, PacketNumber, Range<usize>, Range<usize>)> {
+    ) -> Res<(bool, PacketNumber, Range<usize>)> {
         assert_ne!(self.packet_type, PacketType::Retry);
         assert_ne!(self.packet_type, PacketType::VersionNegotiation);
 
@@ -856,12 +851,7 @@ impl<'a> PublicPacket<'a> {
         let key_phase = self.packet_type == PacketType::Short
             && (first_byte & PACKET_BIT_KEY_PHASE) == PACKET_BIT_KEY_PHASE;
         let pn = Self::decode_pn(crypto.next_pn(), pn_encoded, pn_len);
-        Ok((
-            key_phase,
-            pn,
-            hdrbytes,
-            self.header_len + pn_len..self.data.len(),
-        ))
+        Ok((key_phase, pn, hdrbytes))
     }
 
     /// # Errors
@@ -883,13 +873,13 @@ impl<'a> PublicPacket<'a> {
             // This is OK in this case because we the only reason this can
             // fail is if the cryptographic module is bad or the packet is
             // too small (which is public information).
-            let (key_phase, pn, header, body) = self.decrypt_header(rx)?;
+            let (key_phase, pn, header) = self.decrypt_header(rx)?;
             qtrace!("[{rx}] decoded header: {header:?}");
             let Some(rx) = crypto.rx(version, cspace, key_phase) else {
                 return Err(Error::DecryptError);
             };
             let version = rx.version(); // Version fixup; see above.
-            let d = rx.decrypt(pn, header, body, self.data)?;
+            let d = rx.decrypt(pn, header, self.data)?;
             // If this is the first packet ever successfully decrypted
             // using `rx`, make sure to initiate a key update.
             if rx.needs_update() {
@@ -1024,7 +1014,7 @@ mod tests {
         // The spec uses PN=1, but our crypto refuses to skip packet numbers.
         // So burn an encryption:
         let mut burn = [0; 16];
-        prot.encrypt(0, 0..0, 0..0, &mut burn).expect("burn OK");
+        prot.encrypt(0, 0..0, &mut burn).expect("burn OK");
         assert_eq!(burn.len(), prot.expansion());
 
         let mut builder = PacketBuilder::long(