Skip to content

Commit

Permalink
Fix output buffer check introduced in #1733
Browse files Browse the repository at this point in the history 
Sadly the condition used to relax output buffer checks that depended
on the `num` parameter does not really hold so this change effectively
reverts PR #1733.

As clarified on the OpenSSL mailing list [0] and during integration tests
the `num` parameter does not reflect the internal buffer cache size thus
one needs to pessimistically assume that each call to `cipher_update`
will need sufficient size to contain one additional block. Streaming
ciphers are not affected by this revert.

[0]: https://mta.openssl.org/pipermail/openssl-users/2022-December/015727.html
  • Loading branch information
@wiktor-k
wiktor-k committed Dec 20, 2022
1 parent d780a8f commit 71013f7
Showing 1 changed file with 10 additions and 145 deletions.
155 changes: 10 additions & 145 deletions openssl/src/cipher_ctx.rs
View file
Expand Up @@ -379,49 +379,6 @@ impl CipherCtxRef {
unsafe { ffi::EVP_CIPHER_CTX_num(self.as_ptr()) as usize }
}

/// Returns number of bytes cached in partial block update.
#[cfg(ossl110)]
fn used_block_size(&self) -> usize {
self.num()
}

/// Returns maximum number of bytes that could be cached.
#[cfg(not(ossl110))]
fn used_block_size(&self) -> usize {
self.block_size()
}

/// Calculate the minimal size of the output buffer given the
/// input buffer size.
///
/// For streaming ciphers the minimal output size is the same as
/// the input size. For block ciphers the minimal output size
/// additionally depends on the partial blocks that might have
/// been written in previous calls to [`Self::cipher_update`].
///
/// This function takes into account the number of partially
/// written blocks for block ciphers for supported targets
/// (OpenSSL >= 1.1). For unsupported targets the number of
/// partially written bytes is assumed to contain one full block
/// (pessimistic case).
///
/// # Panics
///
/// Panics if the context has not been initialized with a cipher.
pub fn minimal_output_size(&self, inlen: usize) -> usize {
let block_size = self.block_size();
if block_size > 1 {
// block cipher
let num = self.used_block_size();
let total_size = inlen + num;
let num_blocks = total_size / block_size;
num_blocks * block_size
} else {
// streaming cipher
inlen
}
}

/// Sets the length of the IV expected by this context.
///
/// Only some ciphers support configurable IV lengths.
Expand Down Expand Up @@ -569,7 +526,11 @@ impl CipherCtxRef {
output: Option<&mut [u8]>,
) -> Result<usize, ErrorStack> {
if let Some(output) = &output {
let min_output_size = self.minimal_output_size(input.len());
let mut block_size = self.block_size();
if block_size == 1 {
block_size = 0;
}
let min_output_size = input.len() + block_size;
assert!(
output.len() >= min_output_size,
"Output buffer size should be at least {} bytes.",
Expand All @@ -588,16 +549,13 @@ impl CipherCtxRef {
///
/// This function is the same as [`Self::cipher_update`] but with the
/// output size check removed. It can be used when the exact
/// buffer size control is maintained by the caller and the
/// underlying cryptographic library doesn't expose exact block
/// cache data (e.g. OpenSSL < 1.1, BoringSSL, LibreSSL).
/// buffer size control is maintained by the caller.
///
/// SAFETY: The caller is expected to provide `output` buffer
/// large enough to contain correct number of bytes. For streaming
/// ciphers the output buffer size should be at least as big as
/// the input buffer. For block ciphers the size of the output
/// buffer depends on the state of partially updated blocks (see
/// [`Self::minimal_output_size`]).
/// buffer depends on the state of partially updated blocks.
#[corresponds(EVP_CipherUpdate)]
pub unsafe fn cipher_update_unchecked(
&mut self,
Expand Down Expand Up @@ -757,75 +715,6 @@ mod test {
aes_128_cbc(cipher);
}

#[test]
#[cfg(ossl110)]
fn partial_block_updates() {
test_block_cipher_for_partial_block_updates(Cipher::aes_128_cbc());
test_block_cipher_for_partial_block_updates(Cipher::aes_256_cbc());
test_block_cipher_for_partial_block_updates(Cipher::des_ede3_cbc());
}

#[cfg(ossl110)]
fn test_block_cipher_for_partial_block_updates(cipher: &'static CipherRef) {
let mut key = vec![0; cipher.key_length()];
rand_bytes(&mut key).unwrap();
let mut iv = vec![0; cipher.iv_length()];
rand_bytes(&mut iv).unwrap();

let mut ctx = CipherCtx::new().unwrap();

ctx.encrypt_init(Some(cipher), Some(&key), Some(&iv))
.unwrap();
ctx.set_padding(false);

let block_size = cipher.block_size();
assert!(block_size > 1, "Need a block cipher, not a stream cipher");

// update cipher with non-full block
// expect no output until a block is complete
let outlen = ctx
.cipher_update(&vec![0; block_size - 1], Some(&mut [0; 0]))
.unwrap();
assert_eq!(0, outlen);

// update cipher with missing bytes from the previous block
// and one additional block, output should contain two blocks
let mut two_blocks = vec![0; block_size * 2];
let outlen = ctx
.cipher_update(&vec![0; block_size + 1], Some(&mut two_blocks))
.unwrap();
assert_eq!(block_size * 2, outlen);

ctx.cipher_final_vec(&mut vec![0; 0]).unwrap();

// try to decrypt
ctx.decrypt_init(Some(cipher), Some(&key), Some(&iv))
.unwrap();
ctx.set_padding(false);

// update cipher with non-full block
// expect no output until a block is complete
let outlen = ctx
.cipher_update(&two_blocks[0..block_size - 1], Some(&mut [0; 0]))
.unwrap();
assert_eq!(0, outlen);

// update cipher with missing bytes from the previous block
// and one additional block, output should contain two blocks
let mut two_blocks_decrypted = vec![0; block_size * 2];
let outlen = ctx
.cipher_update(
&two_blocks[block_size - 1..],
Some(&mut two_blocks_decrypted),
)
.unwrap();
assert_eq!(block_size * 2, outlen);

ctx.cipher_final_vec(&mut vec![0; 0]).unwrap();
// check if the decrypted blocks are the same as input (all zeros)
assert_eq!(two_blocks_decrypted, vec![0; block_size * 2]);
}

#[test]
fn test_stream_ciphers() {
test_stream_cipher(Cipher::aes_192_ctr());
Expand Down Expand Up @@ -894,43 +783,19 @@ mod test {
}

#[test]
#[should_panic(expected = "Output buffer size should be at least 16 bytes.")]
#[cfg(ossl110)]
fn full_block_updates_aes_128() {
output_buffer_too_small(Cipher::aes_128_cbc());
}

#[test]
#[should_panic(expected = "Output buffer size should be at least 16 bytes.")]
#[cfg(ossl110)]
fn full_block_updates_aes_256() {
output_buffer_too_small(Cipher::aes_256_cbc());
}

#[test]
#[should_panic(expected = "Output buffer size should be at least 8 bytes.")]
#[cfg(ossl110)]
fn full_block_updates_3des() {
output_buffer_too_small(Cipher::des_ede3_cbc());
}

#[test]
#[should_panic(expected = "Output buffer size should be at least 32 bytes.")]
#[cfg(not(ossl110))]
#[should_panic(expected = "Output buffer size should be at least 33 bytes.")]
fn full_block_updates_aes_128() {
output_buffer_too_small(Cipher::aes_128_cbc());
}

#[test]
#[should_panic(expected = "Output buffer size should be at least 32 bytes.")]
#[cfg(not(ossl110))]
#[should_panic(expected = "Output buffer size should be at least 33 bytes.")]
fn full_block_updates_aes_256() {
output_buffer_too_small(Cipher::aes_256_cbc());
}

#[test]
#[should_panic(expected = "Output buffer size should be at least 16 bytes.")]
#[cfg(not(ossl110))]
#[should_panic(expected = "Output buffer size should be at least 17 bytes.")]
fn full_block_updates_3des() {
output_buffer_too_small(Cipher::des_ede3_cbc());
}
Expand Down

2 comments on commit 71013f7

@Audrius-GR
Copy link

Choose a reason for hiding this comment

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

This should be better documented in the API, as it was not obvious why this was needed when migrating from a different crypto library, and caused a lot of confusion.

@wiktor-k
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I was the one who introduced the check and then removed it since it didn't actually do anything good. So I definitely was caught by that.

I'm not sure what do you mean by "better documented in the API" though - do you mean this behavior (that is OpenSSL buffering stuff if not in streaming mode?). I think that's covered by the OpenSSL docs and this crate merely wraps it.

Maybe you can create an issue and if it gets "acked" follow-up with a PR? (Just trying to find some constructive ground here... 😅 )

Please sign in to comment.