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.

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

src: remove OCB support when OPENSSL_NO_OCB is defined #23635

Closed
wants to merge 1 commit into from

Conversation

@codebytere
Copy link
Member

commented Oct 13, 2018

This PR places OCB-related code under a OPENSSL_NO_OCB flag.

In Electron, we use BoringSSL instead of OpenSSL as a result of Chromium, and BoringSSL isn't licensed to use OCB. This PR improves support for BoringSSL without changing any default behaviors.

/cc @tniessen @nornagon

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@davidben

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2018

Just came across this idly. You know we could just define the EVP_CIPH_OCB_MODE constant unused in BoringSSL. That's the sort of thing we normally do to avoid too many unnecessary ifdefs in the world.

if (mode == EVP_CIPH_CCM_MODE || mode == EVP_CIPH_OCB_MODE) {
bool needs_auth_tag_length = mode == EVP_CIPH_CCM_MODE;
#ifndef OPENSSL_NO_OCB
needs_auth_tag_length |= mode == EVP_CIPH_OCB_MODE;

This comment has been minimized.

Copy link
@tniessen

tniessen Oct 13, 2018

Member

Some people won't be happy about using this operator I guess.

This comment has been minimized.

Copy link
@addaleax

addaleax Oct 13, 2018

Member

I guess the alternative would be something like

bool needs_auth_tag_length = mode == EVP_CIPH_CCM_MODE
#ifndef OPENSSL_NO_OCB
    || mode == EVP_CIPH_OCB_MODE;
#endif
  ;

? Not that much prettier ;)

This comment has been minimized.

Copy link
@tniessen

tniessen Oct 13, 2018

Member

I am fine with the change as it is, but other collaborators have expressed concerns about mixing int/bool operators in the past :)

This comment has been minimized.

Copy link
@sam-github

sam-github Oct 13, 2018

Member

I don't care about the operator mixing, but I very mildly prefer @addaleax's idiom above, and would use something similar to it for the block around line 2547 as well: a list of || blah statements, some of them with ifdef's around them.

if (mode == EVP_CIPH_CCM_MODE || mode == EVP_CIPH_OCB_MODE) {
bool needs_auth_tag_length = mode == EVP_CIPH_CCM_MODE;
#ifndef OPENSSL_NO_OCB
needs_auth_tag_length |= mode == EVP_CIPH_OCB_MODE;

This comment has been minimized.

Copy link
@addaleax

addaleax Oct 13, 2018

Member

I guess the alternative would be something like

bool needs_auth_tag_length = mode == EVP_CIPH_CCM_MODE
#ifndef OPENSSL_NO_OCB
    || mode == EVP_CIPH_OCB_MODE;
#endif
  ;

? Not that much prettier ;)

src/node_crypto.cc Show resolved Hide resolved
@addaleax

This comment has been minimized.

#ifndef OPENSSL_NO_OCB
|| mode == EVP_CIPH_OCB_MODE
#endif
); // NOLINT

This comment has been minimized.

Copy link
@addaleax

addaleax Oct 13, 2018

Member

Looks like the Windows compiler is confused by this in some way…?

16:44:06 src\node_crypto.cc(2899): error C2121: '#': invalid character: possibly the result of a macro expansion [c:\workspace\node-compile-windows\node_lib.vcxproj]
16:44:06 src\node_crypto.cc(2899): error C2146: syntax error: missing ')' before identifier 'ifndef' [c:\workspace\node-compile-windows\node_lib.vcxproj]
16:44:06 src\node_crypto.cc(2899): error C2143: syntax error: missing ';' before '{' [c:\workspace\node-compile-windows\node_lib.vcxproj]

@sam-github

This comment has been minimized.

Copy link
Member

commented Oct 13, 2018

Could we just defined EVP_CIPH_OCB_MODE to some unused value if there is no OCB? I assume attempts to use OCB would still fail later in the code path, but it would minimize the changes. I think @davidben was suggesting something like this, but in the boring ssl headers. We could do it locally, too:

#ifndef EVP_CIPH_OCB_MODE
#define EVP_CIPH_OCB_MODE 0x10003
#endif 

If not, would it make sense for the ifdefs be on the existence of EVP_CIPH_OCB_MODE instead of the OPENSSL_NO_OCB so as to test directly for what we care about and not assume a relationship between the two macros that isn't necessarily going to be true with other openssl-alike crypto libs.

@tniessen

This comment has been minimized.

Copy link
Member

commented Oct 13, 2018

The reason I told Shelley I would like to see this in core is that you can actually build OpenSSL without support for OCB, and OpenSSL relies on OPENSSL_NO_OCB, so this is not limited to BoringSSL.

#ifndef OPENSSL_NO_OCB
EVP_add_cipher(EVP_aes_128_ocb());
#endif

We can still take @sam-github's approach as long as the actual behavior matches the expected behavior (which is that createCipher / createCipheriv fails with "unknown cipher").

@@ -2542,9 +2542,12 @@ int VerifyCallback(int preverify_ok, X509_STORE_CTX* ctx) {
}

static bool IsSupportedAuthenticatedMode(int mode) {
#ifndef OPENSSL_NO_OCB
if (mode == EVP_CIPH_OCB_MODE)
return true;

This comment has been minimized.

Copy link
@mscdex

mscdex Oct 14, 2018

Contributor

This doesn't seem right. If there is no OCB support, we should not be returning true for OCB in a function that determines supported authenticated modes.

Also, I think we should replace this conditional with the same ifdef solution used in the other changes below for consistency.

This comment has been minimized.

Copy link
@tniessen

tniessen Oct 14, 2018

Member

#ifndef OPENSSL_NO_OCB implies that there is support for OCB, so this should be correct.

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2018

I think we should be failing as early as possible when OCB is not available and someone tries to use it (directly or indirectly).

@tniessen

This comment has been minimized.

Copy link
Member

commented Oct 14, 2018

I think we should go for consistency. How about this?

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

In my opinion, that's the cleanest solution, and I think that addresses all other review comments.

@davidben

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2018

FYI, I've uploaded https://boringssl-review.googlesource.com/c/boringssl/+/32464 to BoringSSL. (Note OpenSSL normally defines EVP_CIPH_OCB_MODE even if you build with OCB disabled.)

@@ -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)) {

This comment has been minimized.

Copy link
@sam-github

sam-github Oct 15, 2018

Member

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.

This comment has been minimized.

Copy link
@tniessen

tniessen Oct 15, 2018

Member

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.

@@ -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);

This comment has been minimized.

Copy link
@sam-github

sam-github Oct 15, 2018

Member

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.

@sam-github

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

@davidben why not set the defines to the same values as in openssl, so that they are unique? If someone is using the defines in a case statement, having them all be the same value could go awry.

@tniessen
Copy link
Member

left a comment

Still LGTM, thanks for following up on my suggestion @codebytere.

@tniessen

This comment has been minimized.

@davidben

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2018

@davidben why not set the defines to the same values as in openssl, so that they are unique? If someone is using the defines in a case statement, having them all be the same value could go awry.

No one seems to have used switch statements so far, but good point. :-) Switched them to distinct negative numbers.

I didn't match upstream's values because ours already don't match. Notably our current EVP_CIPH_MODE_MASK (0x3f) puts the bits all at the bottom whereas upstream is ABI-stable, so when they needed more mode bits, they took some upper ones (0xf0007). (If it turns out we need to match later, we can always go back and change that, but we don't generally want code to depend on exact values on constants.)

agl pushed a commit to google/boringssl that referenced this pull request Oct 16, 2018
Add a compatibility EVP_CIPH_OCB_MODE value.
Node references it these days. Also replace the no-op modes with negative
numbers rather than zero. Stream ciphers like RC4 report a "mode" of zero, so
code comparing the mode to a dummy value will get confused.

(I came across nodejs/node#23635, though we'd have run
into it sooner or later anyway. Better to just define the value and avoid ifdef
proliferation.)

Change-Id: I223f25663e138480ad83f35aa16f5218f1425563
Reviewed-on: https://boringssl-review.googlesource.com/c/32464
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
@tniessen

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

Landed in ca2eec4, thank you, Shelley! 😃

@tniessen tniessen closed this Oct 16, 2018

tniessen added a commit that referenced this pull request Oct 16, 2018
src: remove OCB support ifdef OPENSSL_NO_OCB
Electron uses BoringSSL which does not support OCB . It is also
possible to build OpenSSL without support for OCB for Node.js.
This commit disables OCB if OPENSSL_NO_OCB is defined.

PR-URL: #23635
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
burgerboydaddy added a commit to burgerboydaddy/node that referenced this pull request Oct 17, 2018
src: remove OCB support ifdef OPENSSL_NO_OCB
Electron uses BoringSSL which does not support OCB . It is also
possible to build OpenSSL without support for OCB for Node.js.
This commit disables OCB if OPENSSL_NO_OCB is defined.

PR-URL: nodejs#23635
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Oct 17, 2018
src: remove OCB support ifdef OPENSSL_NO_OCB
Electron uses BoringSSL which does not support OCB . It is also
possible to build OpenSSL without support for OCB for Node.js.
This commit disables OCB if OPENSSL_NO_OCB is defined.

PR-URL: #23635
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax added a commit that referenced this pull request Oct 20, 2018
src: remove OCB support ifdef OPENSSL_NO_OCB
Electron uses BoringSSL which does not support OCB . It is also
possible to build OpenSSL without support for OCB for Node.js.
This commit disables OCB if OPENSSL_NO_OCB is defined.

PR-URL: #23635
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins added a commit that referenced this pull request Oct 30, 2018
src: remove OCB support ifdef OPENSSL_NO_OCB
Electron uses BoringSSL which does not support OCB . It is also
possible to build OpenSSL without support for OCB for Node.js.
This commit disables OCB if OPENSSL_NO_OCB is defined.

PR-URL: #23635
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere referenced this pull request Nov 27, 2018
rvagg added a commit that referenced this pull request Nov 28, 2018
src: remove OCB support ifdef OPENSSL_NO_OCB
Electron uses BoringSSL which does not support OCB . It is also
possible to build OpenSSL without support for OCB for Node.js.
This commit disables OCB if OPENSSL_NO_OCB is defined.

PR-URL: #23635
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins added a commit that referenced this pull request Nov 29, 2018
src: remove OCB support ifdef OPENSSL_NO_OCB
Electron uses BoringSSL which does not support OCB . It is also
possible to build OpenSSL without support for OCB for Node.js.
This commit disables OCB if OPENSSL_NO_OCB is defined.

PR-URL: #23635
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere referenced this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.