Expose a safe interface for EVP_AEAD #68

initsecret · 2022-02-19T21:11:22Z

jyn514

This looks like a good start :) I left a lot of comments - none of them are critical, but it would be nice to address at least some before merging.

Could you add EVP_aead_aes_256_gcm_randnonce and EVP_aead_aes_128_gcm_randnonce under a cfg(feature = "fips") flag? EVP_aead_aes_128_ccm_bluetooth, EVP_aead_aes_128_ccm_bluetooth_8, and EVP_has_aes_hardware also seem to be missing, I think you can add those unconditionally.

boring/src/aead.rs

jyn514 · 2022-02-21T16:24:13Z

boring/src/aead.rs

+
+    /// Returns the length of nonces used with this AEAD.
+    pub fn nonce_len(&self) -> usize {
+        unsafe { EVP_AEAD_nonce_length(self.0) as usize }


Casting here doesn't seem correct - can you change this to return libc::size_t instead? Same for all the other functions returning usize.

I tried to maintain parity with symm.rs which uses this pattern when returning lengths:

boring/boring/src/symm.rs

Lines 187 to 191 in e6ddc40

/// Returns the length of keys used with this cipher.

#[allow(clippy::trivially_copy_pass_by_ref)]

pub fn key_len(&self) -> usize {

unsafe { EVP_CIPHER_key_length(self.0) as usize }

}

jyn514 · 2022-02-21T16:28:13Z

boring/src/aead.rs

+        unsafe { EVP_AEAD_nonce_length(self.0) as usize }
+    }
+
+    /// Returns the maximum ciphertext overhead with this AEAD.


Let's use the upstream documentation here, I'd prefer not to reuse the word "overhead" when defining what it means.

Suggested change

/// Returns the maximum ciphertext overhead with this AEAD.

/// Returns the maximum number of additional bytes added by the act of sealing data with `self`.

///

/// Corresponds to [`EVP_AEAD_max_overhead`](https://commondatastorage.googleapis.com/chromium-boringssl-docs/aead.h.html#EVP_AEAD_max_overhead).

jyn514 · 2022-02-21T16:30:05Z

boring/src/aead.rs

+    }
+
+    pub fn as_ptr(&self) -> *const ffi::EVP_AEAD {
+        self.0


boring does this elsewhere in the codebase with ForeignType - is there a reason you didn't use that here? https://docs.rs/boring/latest/boring/dh/struct.Dh.html#impl-ForeignType

I tried to follow the one in symm.rs.

boring/boring/src/symm.rs

Lines 182 to 185 in e6ddc40

#[allow(clippy::trivially_copy_pass_by_ref)]

pub fn as_ptr(&self) -> *const ffi::EVP_CIPHER {

self.0

}

jyn514 · 2022-02-21T16:38:17Z

boring/src/aead.rs

+}
+
+unsafe impl Sync for Aead {}
+unsafe impl Send for Aead {}


How do you know the cipher is thread-safe?

Good catch, I don't, I accidentally assumed that it was, and the the documentation doesn't seem explicit on this, so I'll remove the lines.

jyn514 · 2022-02-21T16:48:26Z

boring/src/aead.rs

+unsafe impl Send for Aead {}
+
+/// Represents an AEAD context.
+pub struct AeadCrypter {


you mentioned wanting to come up with a better name - I think AeadContext would be a good fit. But this also seems fine as-is.

jyn514 · 2022-02-21T16:51:02Z

boring/src/aead.rs

+/// Encrypts the provided data and authenticates the ciphertext and the provided aad.
+///
+/// Returns the sealed data as a vector.
+pub fn encrypt_aead(


should this be an inherent method on Aead maybe?

I agree. I defined them separately for parity with symm.rs.

boring/boring/src/symm.rs

Lines 635 to 685 in e6ddc40

/// Like `encrypt`, but for AEAD ciphers such as AES GCM.

///

/// Additional Authenticated Data can be provided in the `aad` field, and the authentication tag

/// will be copied into the `tag` field.

///

/// The size of the `tag` buffer indicates the required size of the tag. While some ciphers support

/// a range of tag sizes, it is recommended to pick the maximum size. For AES GCM, this is 16 bytes,

/// for example.

pub fn encrypt_aead(

t: Cipher,

key: &[u8],

iv: Option<&[u8]>,

aad: &[u8],

data: &[u8],

tag: &mut [u8],

) -> Result<Vec<u8>, ErrorStack> {

let mut c = Crypter::new(t, Mode::Encrypt, key, iv)?;

let mut out = vec![0; data.len() + t.block_size()];

c.aad_update(aad)?;

let count = c.update(data, &mut out)?;

let rest = c.finalize(&mut out[count..])?;

c.get_tag(tag)?;

out.truncate(count + rest);

Ok(out)

}

/// Like `decrypt`, but for AEAD ciphers such as AES GCM.

///

/// Additional Authenticated Data can be provided in the `aad` field, and the authentication tag

/// should be provided in the `tag` field.

pub fn decrypt_aead(

t: Cipher,

key: &[u8],

iv: Option<&[u8]>,

aad: &[u8],

data: &[u8],

tag: &[u8],

) -> Result<Vec<u8>, ErrorStack> {

let mut c = Crypter::new(t, Mode::Decrypt, key, iv)?;

let mut out = vec![0; data.len() + t.block_size()];

c.aad_update(aad)?;

let count = c.update(data, &mut out)?;

c.set_tag(tag)?;

let rest = c.finalize(&mut out[count..])?;

out.truncate(count + rest);

Ok(out)

}

jyn514 · 2022-02-21T16:51:38Z

boring/src/aead.rs

+    let mut aeadcrypter = AeadCrypter::new(aead, key)?;
+    let mut output = vec![0u8; data.len() + aead.max_ciphertext_overhead()];
+    let bytes_written_to_output = aeadcrypter.seal(nonce, aad, data, &mut output)?;
+    Ok(output[..bytes_written_to_output].to_vec())


Suggested change

Ok(output[..bytes_written_to_output].to_vec())

output.truncate(bytes_written_to_output);

Ok(output)

jyn514 · 2022-02-21T16:52:23Z

boring/src/aead.rs

+/// Authenticates the provided data and aad and decrypts the provided data.
+///
+/// Returns the unsealed data as a vector.
+pub fn decrypt_aead(


ditto, this seems better as an inherent method.

jyn514 · 2022-02-21T16:54:18Z

boring/src/aead.rs

+        let key = [0u8; 16];
+        let nonce = [0u8; 16];
+        let aad = [0u8; 16];
+        let data = [0u8; 16];


Could you use something other than zeros here? I'm not sure what boringssl does on zeros, and it seems better to be closer to a real scenario anyway.

initsecret · 2022-02-27T03:24:33Z

Thank you for detailed comments! I made a few of the suggested changes; the others seem to break parity with symm.rs which afaict seems okay, but I wanted to confirm with you before making the change.

initsecret added 3 commits December 1, 2021 19:43

Expose the EVP_AEAD Interface

49d046b

Merge remote-tracking branch 'upstream/master' into expose-evp-aead

4b7e090

add more tests

558a105

jyn514 changed the title ~~Expose the EVP_AEAD Interface~~ Expose a safe interface for EVP_AEAD Feb 21, 2022

jyn514 suggested changes Feb 21, 2022

View reviewed changes

initsecret added 3 commits February 26, 2022 22:12

address review comments - part 1

9a8b125

rename AeadCrypter to AeadContext

928a0f0

remove impls for send and sync

d452c33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose a safe interface for EVP_AEAD #68

Expose a safe interface for EVP_AEAD #68

initsecret commented Feb 19, 2022

jyn514 left a comment

jyn514 Feb 21, 2022

initsecret Feb 26, 2022

jyn514 Feb 21, 2022

jyn514 Feb 21, 2022

initsecret Feb 27, 2022

jyn514 Feb 21, 2022

initsecret Feb 27, 2022 •

edited

Loading

jyn514 Feb 21, 2022

jyn514 Feb 21, 2022

initsecret Feb 27, 2022

jyn514 Feb 21, 2022

jyn514 Feb 21, 2022

jyn514 Feb 21, 2022

initsecret commented Feb 27, 2022

	/// Returns the length of keys used with this cipher.
	#[allow(clippy::trivially_copy_pass_by_ref)]
	pub fn key_len(&self) -> usize {
	unsafe { EVP_CIPHER_key_length(self.0) as usize }
	}

-    /// Returns the maximum ciphertext overhead with this AEAD.
+    /// Returns the maximum number of additional bytes added by the act of sealing data with `self`.
+    ///
+    /// Corresponds to [`EVP_AEAD_max_overhead`](https://commondatastorage.googleapis.com/chromium-boringssl-docs/aead.h.html#EVP_AEAD_max_overhead).

	#[allow(clippy::trivially_copy_pass_by_ref)]
	pub fn as_ptr(&self) -> *const ffi::EVP_CIPHER {
	self.0
	}

	/// Like `encrypt`, but for AEAD ciphers such as AES GCM.
	///
	/// Additional Authenticated Data can be provided in the `aad` field, and the authentication tag
	/// will be copied into the `tag` field.
	///
	/// The size of the `tag` buffer indicates the required size of the tag. While some ciphers support
	/// a range of tag sizes, it is recommended to pick the maximum size. For AES GCM, this is 16 bytes,
	/// for example.
	pub fn encrypt_aead(
	t: Cipher,
	key: &[u8],
	iv: Option<&[u8]>,
	aad: &[u8],
	data: &[u8],
	tag: &mut [u8],
	) -> Result<Vec<u8>, ErrorStack> {
	let mut c = Crypter::new(t, Mode::Encrypt, key, iv)?;
	let mut out = vec![0; data.len() + t.block_size()];

	c.aad_update(aad)?;
	let count = c.update(data, &mut out)?;
	let rest = c.finalize(&mut out[count..])?;
	c.get_tag(tag)?;
	out.truncate(count + rest);
	Ok(out)
	}

	/// Like `decrypt`, but for AEAD ciphers such as AES GCM.
	///
	/// Additional Authenticated Data can be provided in the `aad` field, and the authentication tag
	/// should be provided in the `tag` field.
	pub fn decrypt_aead(
	t: Cipher,
	key: &[u8],
	iv: Option<&[u8]>,
	aad: &[u8],
	data: &[u8],
	tag: &[u8],
	) -> Result<Vec<u8>, ErrorStack> {
	let mut c = Crypter::new(t, Mode::Decrypt, key, iv)?;
	let mut out = vec![0; data.len() + t.block_size()];

	c.aad_update(aad)?;
	let count = c.update(data, &mut out)?;

	c.set_tag(tag)?;
	let rest = c.finalize(&mut out[count..])?;

	out.truncate(count + rest);
	Ok(out)
	}

	Ok(output[..bytes_written_to_output].to_vec())
	output.truncate(bytes_written_to_output);
	Ok(output)

Expose a safe interface for EVP_AEAD #68

Are you sure you want to change the base?

Expose a safe interface for EVP_AEAD #68

Conversation

initsecret commented Feb 19, 2022

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

initsecret Feb 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

initsecret commented Feb 27, 2022

initsecret Feb 27, 2022 •

edited

Loading