-
-
Notifications
You must be signed in to change notification settings - Fork 759
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correct off-by-one in minimum output buffer size computation #2088
Conversation
If there was a full block in the buffer, it would have already been returned.
} | ||
let min_output_size = input.len() + block_size; | ||
let block_size = self.block_size(); | ||
let min_output_size = input.len() + block_size - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this is correct? The documentation says:
The amount of data written depends on the block alignment of the encrypted data. For most ciphers and modes, the amount of data written can be anything from zero bytes to (inl + cipher_block_size - 1) bytes. For wrap cipher modes, the amount of data written can be anything from zero bytes to (inl + cipher_block_size) bytes. For stream ciphers, the amount of data written can be anything from zero bytes to inl bytes. Thus, out should contain sufficient room for the operation being performed.
https://www.openssl.org/docs/man3.1/man3/EVP_DecryptUpdate.html
Really the problem is that EVP_CIPHER
is simply not suitable to be wrapped generically like this. rust-openssl's strategy of "exposing the underlying library" is unsound here. You have to know details about the cipher to accurate size it. (I'm going to file a bug with OpenSSL about this. This is a terrible API.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure it's correct -- we've used the same math for pyca/cryptography for a gazillion years :-)
If you think about it, it makes sense: the buffer will always contain less than one bock (if it had a whole block, it would have returned it previously).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on which modes are reachable here. As it says, cipher_block_size-1 is correct for most ciphers, but not all. The wrap mode ciphers have a block sizes of 8...
https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/crypto/evp/e_aes.c#L3872
...but they write inlen + 8
bytes:
https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/crypto/evp/e_aes.c#L3857
https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/crypto/modes/wrap128.c#L80
Unfortunately, since this crate exposed EVP_get_cipherbyname
(I definitely wouldn't have...), you have to worry about all possible EVP_CIPHER
s. There's probably also a future-proofing problem too. Based on OpenSSL's documentation, it's unclear whether they've promised that they will never add a new kind of EVP_CIPHER
that requires an even larger output. Saying "you have to know what kind of cipher you've got" would be perfectly coherent, but incompatible with rust-openssl's API decisions.
…, r=weihanglo chore(deps): update rust crate openssl to 0.10.60 [security] [![Mend Renovate logo banner](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [openssl](https://github.com/sfackler/rust-openssl) | workspace.dependencies | patch | `0.10.57` -> `0.10.60` | ### GitHub Vulnerability Alerts #### [GHSA-xphf-cx8h-7q9g](https://github.com/sfackler/rust-openssl/issues/2096) This function returned a reference into an OpenSSL datastructure, but there was no way to ensure OpenSSL would not mutate the datastructure behind one's back. Use of this function should be replaced with `X509StoreRef::all_certificates`. --- ### Release Notes <details> <summary>sfackler/rust-openssl (openssl)</summary> ### [`v0.10.60`](https://github.com/sfackler/rust-openssl/releases/tag/openssl-v0.10.60) [Compare Source](https://github.com/sfackler/rust-openssl/compare/openssl-v0.10.59...openssl-v0.10.60) #### What's Changed - Correct off-by-one in minimum output buffer size computation by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2088](https://github.com/sfackler/rust-openssl/pull/2088) - Expose a few more (bad) ciphers in cipher::Cipher by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2084](https://github.com/sfackler/rust-openssl/pull/2084) - add temp key bindings by [`@​jmayclin](https://github.com/jmayclin)` in [https://github.com/sfackler/rust-openssl/pull/2076](https://github.com/sfackler/rust-openssl/pull/2076) - Expose ChaCha20 on LibreSSL by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2093](https://github.com/sfackler/rust-openssl/pull/2093) - Revert "Correct off-by-one in minimum output buffer size computation" by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2090](https://github.com/sfackler/rust-openssl/pull/2090) - Added `update_unchecked` to `symm::Crypter` by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2100](https://github.com/sfackler/rust-openssl/pull/2100) - fixes [#​2096](https://github.com/sfackler/rust-openssl/issues/2096) -- deprecate `X509StoreRef::objects`, it is unsound by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2099](https://github.com/sfackler/rust-openssl/pull/2099) - Don't leak when overwriting ex data by [`@​sfackler](https://github.com/sfackler)` in [https://github.com/sfackler/rust-openssl/pull/2102](https://github.com/sfackler/rust-openssl/pull/2102) - Release openssl v0.10.60 and openssl-sys v0.9.96 by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2104](https://github.com/sfackler/rust-openssl/pull/2104) **Full Changelog**: sfackler/rust-openssl@openssl-v0.10.59...openssl-v0.10.60 ### [`v0.10.59`](https://github.com/sfackler/rust-openssl/releases/tag/openssl-v0.10.59) [Compare Source](https://github.com/sfackler/rust-openssl/compare/openssl-v0.10.58...openssl-v0.10.59) #### What's Changed - Add binding to NID of Chacha20-Poly1305 cipher by [`@​Arnavion](https://github.com/Arnavion)` in [https://github.com/sfackler/rust-openssl/pull/2081](https://github.com/sfackler/rust-openssl/pull/2081) - Fixed cfg for RSA_PSS by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2079](https://github.com/sfackler/rust-openssl/pull/2079) - fixes [#​2050](https://github.com/sfackler/rust-openssl/issues/2050) -- build and test on libressl 3.8.2 by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2082](https://github.com/sfackler/rust-openssl/pull/2082) - Release openssl v0.10.59 and openssl-sys v0.9.95 by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2083](https://github.com/sfackler/rust-openssl/pull/2083) #### New Contributors - [`@​Arnavion](https://github.com/Arnavion)` made their first contribution in [https://github.com/sfackler/rust-openssl/pull/2081](https://github.com/sfackler/rust-openssl/pull/2081) **Full Changelog**: sfackler/rust-openssl@openssl-v0.10.58...openssl-v0.10.59 ### [`v0.10.58`](https://github.com/sfackler/rust-openssl/releases/tag/openssl-v0.10.58) [Compare Source](https://github.com/sfackler/rust-openssl/compare/openssl-v0.10.57...openssl-v0.10.58) #### What's Changed - LibreSSL 3.8.1 support by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2035](https://github.com/sfackler/rust-openssl/pull/2035) - Update vendored version to openssl 3 by [`@​amousset](https://github.com/amousset)` in [https://github.com/sfackler/rust-openssl/pull/1925](https://github.com/sfackler/rust-openssl/pull/1925) - Test against 3.2.0-alpha1 by [`@​sfackler](https://github.com/sfackler)` in [https://github.com/sfackler/rust-openssl/pull/2037](https://github.com/sfackler/rust-openssl/pull/2037) - Removed reference to non-existent method by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2039](https://github.com/sfackler/rust-openssl/pull/2039) - Bump CI to 1.1.1w by [`@​sfackler](https://github.com/sfackler)` in [https://github.com/sfackler/rust-openssl/pull/2040](https://github.com/sfackler/rust-openssl/pull/2040) - \[openssl-sys] Add X509\_check\_{host,email,ip,ip_asc} fns by [`@​jgallagher](https://github.com/jgallagher)` in [https://github.com/sfackler/rust-openssl/pull/2042](https://github.com/sfackler/rust-openssl/pull/2042) - Expose CBC mode for several more (bad) ciphers by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2045](https://github.com/sfackler/rust-openssl/pull/2045) - Expose two additional Pkey IDs by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2046](https://github.com/sfackler/rust-openssl/pull/2046) - Add support for CRL extensions and the Authority Information Access e… by [`@​AdmiralGT](https://github.com/AdmiralGT)` in [https://github.com/sfackler/rust-openssl/pull/2003](https://github.com/sfackler/rust-openssl/pull/2003) - Fix clippy warnings produced by newer Rust by [`@​wiktor-k](https://github.com/wiktor-k)` in [https://github.com/sfackler/rust-openssl/pull/2052](https://github.com/sfackler/rust-openssl/pull/2052) - Use osslconf on BoringSSL by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2056](https://github.com/sfackler/rust-openssl/pull/2056) - Make X509\_ALGOR opaque for LibreSSL by [`@​botovq](https://github.com/botovq)` in [https://github.com/sfackler/rust-openssl/pull/2060](https://github.com/sfackler/rust-openssl/pull/2060) - Don't ignore ECDSA tests without GF2m support by [`@​botovq](https://github.com/botovq)` in [https://github.com/sfackler/rust-openssl/pull/2061](https://github.com/sfackler/rust-openssl/pull/2061) - Clarify 'possible LibreSSL bug' by [`@​botovq](https://github.com/botovq)` in [https://github.com/sfackler/rust-openssl/pull/2062](https://github.com/sfackler/rust-openssl/pull/2062) - Enable BN_mod_sqrt() for upcoming LibreSSL 3.8.2 by [`@​botovq](https://github.com/botovq)` in [https://github.com/sfackler/rust-openssl/pull/2063](https://github.com/sfackler/rust-openssl/pull/2063) - Enable SHA-3 for LibreSSL 3.8.0 by [`@​botovq](https://github.com/botovq)` in [https://github.com/sfackler/rust-openssl/pull/2064](https://github.com/sfackler/rust-openssl/pull/2064) - Remove DH_generate_parameters for LibreSSL 3.8.2 by [`@​botovq](https://github.com/botovq)` in [https://github.com/sfackler/rust-openssl/pull/2065](https://github.com/sfackler/rust-openssl/pull/2065) - Use EVP_MD_CTX\_{new,free}() in LibreSSL 3.8.2 by [`@​botovq](https://github.com/botovq)` in [https://github.com/sfackler/rust-openssl/pull/2067](https://github.com/sfackler/rust-openssl/pull/2067) - Enable HKDF support for LibreSSL >= 3.6.0 by [`@​botovq](https://github.com/botovq)` in [https://github.com/sfackler/rust-openssl/pull/2066](https://github.com/sfackler/rust-openssl/pull/2066) - Two build script fixes for LibreSSL by [`@​botovq](https://github.com/botovq)` in [https://github.com/sfackler/rust-openssl/pull/2068](https://github.com/sfackler/rust-openssl/pull/2068) - Respect OPENSSL_NO_OCB on AES functions by [`@​GuyLewin](https://github.com/GuyLewin)` in [https://github.com/sfackler/rust-openssl/pull/2070](https://github.com/sfackler/rust-openssl/pull/2070) - Support OPENSSL_NO_SCRYPT by [`@​GuyLewin](https://github.com/GuyLewin)` in [https://github.com/sfackler/rust-openssl/pull/2071](https://github.com/sfackler/rust-openssl/pull/2071) - Bump 3.2.0 beta by [`@​sfackler](https://github.com/sfackler)` in [https://github.com/sfackler/rust-openssl/pull/2073](https://github.com/sfackler/rust-openssl/pull/2073) - add security level bindings by [`@​jmayclin](https://github.com/jmayclin)` in [https://github.com/sfackler/rust-openssl/pull/2074](https://github.com/sfackler/rust-openssl/pull/2074) - Release openssl v0.10.58 and openssl-sys v0.9.94 by [`@​alex](https://github.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2078](https://github.com/sfackler/rust-openssl/pull/2078) #### New Contributors - [`@​amousset](https://github.com/amousset)` made their first contribution in [https://github.com/sfackler/rust-openssl/pull/1925](https://github.com/sfackler/rust-openssl/pull/1925) - [`@​jgallagher](https://github.com/jgallagher)` made their first contribution in [https://github.com/sfackler/rust-openssl/pull/2042](https://github.com/sfackler/rust-openssl/pull/2042) - [`@​AdmiralGT](https://github.com/AdmiralGT)` made their first contribution in [https://github.com/sfackler/rust-openssl/pull/2003](https://github.com/sfackler/rust-openssl/pull/2003) - [`@​botovq](https://github.com/botovq)` made their first contribution in [https://github.com/sfackler/rust-openssl/pull/2060](https://github.com/sfackler/rust-openssl/pull/2060) - [`@​GuyLewin](https://github.com/GuyLewin)` made their first contribution in [https://github.com/sfackler/rust-openssl/pull/2070](https://github.com/sfackler/rust-openssl/pull/2070) - [`@​jmayclin](https://github.com/jmayclin)` made their first contribution in [https://github.com/sfackler/rust-openssl/pull/2074](https://github.com/sfackler/rust-openssl/pull/2074) **Full Changelog**: sfackler/rust-openssl@openssl-v0.10.57...openssl-v0.10.58 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/rust-lang/cargo). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy41OS44IiwidXBkYXRlZEluVmVyIjoiMzcuNTkuOCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->
If there was a full block in the buffer, it would have already been returned.