Use OpenSSL native OCB-AES implementation #1196
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@bbarenblat and @davidben could you please take a look? |
|
Reproduced the CI failure on bionic: |
|
CI corrected. There was (seemingly) some UB that was only exposed on Ubuntu 18.04, but reproduced across clang and gcc. Fixed. |
davidben
reviewed
Jun 9, 2022
|
Ugh I've no idea why GitHub decided to double up my comments... :-/ |
bbarenblat
requested changes
Jun 14, 2022
There was a problem hiding this comment.
@davidben is certainly the cryptography expert here, but for what it’s worth, this LGTM from a cryptography perspective. I have a few minor comments, but substantively, this PR all looks correct.
bbarenblat
approved these changes
Jun 14, 2022
davidben
approved these changes
Jun 14, 2022
achernya
commented
Jun 14, 2022
davidben
reviewed
Jun 14, 2022
|
LGTM |
OpenSSL 3.0 deprecated many of the functions that ocb.cc used to implement OCB-AES, causing a build failure when -Wdeprecated collided with -Werror. Debian temporarily fixed this by suppressing the error in mobile-shell#1191. Since mosh 1.4 will be the next stable release of mosh, it should not depend on deprecated functions in OpenSSL. Since version 1.1.0, OpenSSL natively supports OCB-AES through the EVP_CIPHER API. @cgull started early support for this in mobile-shell#924. This change extends upon the previous work by @cgull in a few ways * EVP_CipherInit_ex is called in ae_init to set up the EVP_CIPHER_CTX. It is later called in ae_encrypt and ae_decrypt just to load nonce (IV in OpenSSL EVP parlance), which reduces the amount of initialization done per-packet. However, due to OpenSSL API limitations, two copies of the EVP_CIPHER_CTX are kept: one for encryption, and one for decryption. * Adds missing support for an external tag, rather than just one appended to the ciphertext * Support for non-default-sized tags as well as some improved error handling. Note that this change raises the minimum OpenSSL version for Mosh to 1.1.0. OpenSSL does not provide security support for versions prior to 1.1 at this time, so this is in principle reasonable dependency. If we want to continue to support distributions (such as RHEL7) which continue to be supported by their vendor but use an unsupported OpenSSL, then some future work will have to restore the ocb.cc implementation that uses the deprecated functions. Bugs: mobile-shell#1174
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
OpenSSL 3.0 deprecated many of the functions that ocb.cc used to
implement OCB-AES, causing a build failure when -Wdeprecated collided
with -Werror. Debian temporarily fixed this by suppressing the error
in #1191.
Since mosh 1.4 will be the next stable release of mosh, it should not
depend on deprecated functions in OpenSSL. Since version 1.1.0,
OpenSSL natively supports OCB-AES through the EVP_CIPHER API. @cgull
started early support for this in #924.
This change extends upon the previous work by @cgull in a few ways
EVP_EncryptInit_ex is called in ae_init to set up the
EVP_CIPHER_CTX. It is later called in ae_encrypt and ae_decrypt
just to load the key and nonce (IV in OpenSSL EVP parlance), which
reduces the amount of initialization done per-packet.
Adds missing support for an external tag, rather than just one
appended to the ciphertext
Support for non-default-sized tags
as well as some improved error handling.
Note that this change raises the minimum OpenSSL version for Mosh to
1.1.0. OpenSSL does not provide security support for versions prior to
1.1 at this time, so this is in principle reasonable dependency. If we
want to continue to support distributions (such as RHEL7) which
continue to be supported by their vendor but use an unsupported
OpenSSL, then some future work will have to restore the ocb.cc
implementation that uses the deprecated functions.
Bugs: #1174