crypto: fix edge case in authenticated encryption #22828
Conversation
Restricting the authentication tag length and calling update or setAAD before setAuthTag caused an incorrect authentication tag to be passed to OpenSSL: The auth_tag_len_ field was already set, so the implementation assumed that the tag itself was known as well.
nodejs-github-bot
added
c++
|
@addaleax I updated it so it matches the old behavior (which isn't optimal). |
src/node_crypto.h
Outdated
| @@ -363,6 +363,12 @@ class CipherBase : public BaseObject { | |||
| kErrorMessageSize, | |||
| kErrorState | |||
| }; | |||
| enum AuthTagState { | |||
| kAuthTagUnknown, | |||
| kAuthTagLengthKnown, | |||
There was a problem hiding this comment.
Is there currently a difference between these two states? Sorry for asking questions, but I’m not sure I’m fully qualified to review this…
There was a problem hiding this comment.
Yes, you can use GCM (e.g. aes-128-gcm) without specifying the authentication tag length, then the state will be kAuthTagUnknown, but other modes such as CCM (aes-128-ccm) and OCB (aes-128-ocb) require the length of the authentication tag in advance (via the authTagLength option), so the cipher will start in the state kAuthTagLengthKnown. Once the user calls setAuthTag, the state becomes kAuthTagKnown, which implies that the length is known as well.
(Never apologize for asking questions. 😉)
There was a problem hiding this comment.
Thank you! Is there any difference in how we treat those states after they have been entered?
There was a problem hiding this comment.
Technically, we do. But:
If auth_tag_state_ is either kAuthTagUnknown or kAuthTagLengthKnown, then auth_tag_state_ == kAuthTagUnknown iff auth_tag_len_ == kNoAuthTagLength. So you are right, we could totally drop the second state! (Or remove kNoAuthTagLength.)
Restricting the authentication tag length and calling update or setAAD before setAuthTag caused an incorrect authentication tag to be passed to OpenSSL: The auth_tag_len_ field was already set, so the implementation assumed that the tag itself was known as well. PR-URL: #22828 Reviewed-By: Daniel Bevenius <[email protected]>
Restricting the authentication tag length and calling update or setAAD before setAuthTag caused an incorrect authentication tag to be passed to OpenSSL: The auth_tag_len_ field was already set, so the implementation assumed that the tag itself was known as well. PR-URL: #22828 Reviewed-By: Daniel Bevenius <[email protected]>
Restricting the authentication tag length and calling update or setAAD before setAuthTag caused an incorrect authentication tag to be passed to OpenSSL: The auth_tag_len_ field was already set, so the implementation assumed that the tag itself was known as well. PR-URL: #22828 Reviewed-By: Daniel Bevenius <[email protected]>
Restricting the authentication tag length and calling update or setAAD before setAuthTag caused an incorrect authentication tag to be passed to OpenSSL: The auth_tag_len_ field was already set, so the implementation assumed that the tag itself was known as well. PR-URL: #22828 Reviewed-By: Daniel Bevenius <[email protected]>
Restricting the authentication tag length and calling
updateorsetAADbeforesetAuthTagcaused an incorrect authentication tag to be passed to OpenSSL: Theauth_tag_len_field was already set, so the implementation assumed that the tag itself was known as well. This change allows the implementation to distinguish between knowing the tag length and the tag itself using theauth_tag_state_field which replaces theauth_tag_set_field.This also allows #22538 to work for OCB! Sadly, CCM won't work without changes within OpenSSL and they probably won't change that due to a more or less related NIST recommendation.
This should finally fix #22421.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes