Skip to content
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.

Sign up for GitHub

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

Jump to bottom

src: remove OCB support when OPENSSL_NO_OCB is defined #23635

Closed
wants to merge 1 commit into from
Closed

src: remove OCB support when OPENSSL_NO_OCB is defined #23635

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ using v8::Uint32;
using v8::Undefined;
using v8::Value;

#ifdef OPENSSL_NO_OCB
# define IS_OCB_MODE(mode) false
#else
# define IS_OCB_MODE(mode) ((mode) == EVP_CIPH_OCB_MODE)
#endif

struct StackOfX509Deleter {
void operator()(STACK_OF(X509)* p) const { sk_X509_pop_free(p, X509_free); }
Expand Down Expand Up @@ -2544,7 +2549,7 @@ int VerifyCallback(int preverify_ok, X509_STORE_CTX* ctx) {
static bool IsSupportedAuthenticatedMode(int mode) {
return mode == EVP_CIPH_CCM_MODE ||
mode == EVP_CIPH_GCM_MODE ||
mode == EVP_CIPH_OCB_MODE;
IS_OCB_MODE(mode);
Copy link
Contributor

Choose a reason for hiding this comment

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

The same comment as below applies here. IsSupportedAuthenticatedMode() is used to check for invalid API use. With this change, when the mode is not implemented by the crypto library, the checking for invalid or missing IV lengths done by CipherBase::InitIv() will be bypassed.

}

void CipherBase::Initialize(Environment* env, Local<Object> target) {
Expand Down Expand Up @@ -2769,7 +2774,7 @@ bool CipherBase::InitAuthenticated(const char* cipher_type, int iv_len,
}

const int mode = EVP_CIPHER_CTX_mode(ctx_.get());
if (mode == EVP_CIPH_CCM_MODE || mode == EVP_CIPH_OCB_MODE) {
if (mode == EVP_CIPH_CCM_MODE ||IS_OCB_MODE(mode)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will result in an odd behaviour. When attempting to use authenticated ciphers, there are certain arguments that are mandatory. Here, its the authTagLength that is checked for. If you don't provide it, this code will throw an error so you know you are calling the API incorrectly. With your change, if the OCB cipher mode isn't supported, it doesn't check for invalid API calls, and instead will (I assume) fail later, when the cipher runs, do to an unsupported cipher mode.

This doesn't seem ideal to me. It seems more reasonable to define EVP_CIPH_OCB_MODE correctly if it happens to be missing, and throw here. Of course, once they correct the code, it will still error, but later.

Copy link
Member

Choose a reason for hiding this comment

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

will (I assume) fail later, when the cipher runs, do to an unsupported cipher mode.

Not quite. You cannot construct an OCB cipher if OpenSSL does not support it: The mode can only be determined if the EVP_CIPHER* is known. Since EVP_get_cipherbyname(cipher_type) will fail for unsupported modes, InitAuthenticated is never called.

if (auth_tag_len == kNoAuthTagLength) {
char msg[128];
snprintf(msg, sizeof(msg), "authTagLength required for %s", cipher_type);
Expand Down Expand Up @@ -2885,7 +2890,7 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) {
} else {
// At this point, the tag length is already known and must match the
// length of the given authentication tag.
CHECK(mode == EVP_CIPH_CCM_MODE || mode == EVP_CIPH_OCB_MODE);
CHECK(mode == EVP_CIPH_CCM_MODE || IS_OCB_MODE(mode));
CHECK_NE(cipher->auth_tag_len_, kNoAuthTagLength);
is_valid = cipher->auth_tag_len_ == tag_len;
codebytere marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down