crypto: fix edge case in authenticated encryption - #22828
Conversation
Sorry, something went wrong.
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.
|
@tniessen build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/871/pipeline |
Sorry, something went wrong.
|
@addaleax I updated it so it matches the old behavior (which isn't optimal). |
Sorry, something went wrong.
| }; | ||
| 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…
Sorry, something went wrong.
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. 😉)
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you! Is there any difference in how we treat those states after they have been entered?
Sorry, something went wrong.
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.)
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
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 <daniel.bevenius@gmail.com>
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 <daniel.bevenius@gmail.com>
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 <daniel.bevenius@gmail.com>
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 <daniel.bevenius@gmail.com>
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. This change allows the implementation to distinguish between knowing the tag length and the tag itself using the auth_tag_state_ field which replaces the auth_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