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

crypto: cast oaepLabel to unsigned char* #30917

Closed
wants to merge 1 commit into from

Conversation

codebytere
Copy link
Member

This PR fixes an incompatibility between BoringSSL and OpenSSL in compilation through Electron.

OpenSSL has the signature

int EVP_PKEY_CTX_set0_rsa_oaep_label(EVP_PKEY_CTX *ctx, unsigned char *label, int len);

whereas BoringSSL has:

int EVP_PKEY_CTX_set0_rsa_oaep_label(EVP_PKEY_CTX *ctx, uint8_t *label, size_t label_len);

Changing this in BoringSSL would not have the correct effect, since label (as returned by OpenSSL_memdup) is a void* & C++ doesn't cast from void* to T* without an explicit cast. OpenSSL has a lot of functions/macros that aren't typesafe which means we don't get the usual C++ type-checking and so this fails to compile without this patch.

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Dec 12, 2019
@davidben
Copy link
Contributor

davidben commented Dec 12, 2019

Correction: this doesn't have anything to do with those signatures. We required unsigned char and uint8_t to be the same type, and it is true in every platform I'm aware of. The signatures are identical.

Rather it's that OpenSSL doesn't use any type signature at all. It's documented with that signature, but it's really a macro without any type-checking. The cast is needed to conform Node to the OpenSSL's documentation because C++ does not implicitly cast void*.

@nodejs-github-bot

This comment has been minimized.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 12, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

codebytere added a commit that referenced this pull request Dec 14, 2019
OpenSSL uses a macro without typechecking; since C++ does not
implicitly cast void* this is needed to conform Node.js to the
OpenSSL documentation.

PR-URL: #30917
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere
Copy link
Member Author

Landed in cb76f27.

@codebytere codebytere closed this Dec 14, 2019
@codebytere codebytere deleted the cast-label branch December 14, 2019 18:55
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
OpenSSL uses a macro without typechecking; since C++ does not
implicitly cast void* this is needed to conform Node.js to the
OpenSSL documentation.

PR-URL: #30917
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 17, 2019
targos pushed a commit that referenced this pull request Jan 14, 2020
OpenSSL uses a macro without typechecking; since C++ does not
implicitly cast void* this is needed to conform Node.js to the
OpenSSL documentation.

PR-URL: #30917
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
OpenSSL uses a macro without typechecking; since C++ does not
implicitly cast void* this is needed to conform Node.js to the
OpenSSL documentation.

PR-URL: #30917
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 15, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 18, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
* chore: bump node in DEPS to v12.16.0

* Fixup asar support setup patch

nodejs/node#30862

* Fixup InternalCallbackScope patch

nodejs/node#30236

* Fixup GN buildfiles patch

nodejs/node#30755

* Fixup low-level hooks patch

nodejs/node#30466

* Fixup globals require patch

nodejs/node#31643

* Fixup process stream patch

nodejs/node#30862

* Fixup js2c modification patch

nodejs/node#30755

* Fixup internal fs override patch

nodejs/node#30610

* Fixup context-aware warn patch

nodejs/node#30336

* Fixup Node.js with ltcg config

nodejs/node#29388

* Fixup oaepLabel patch

nodejs/node#30917

* Remove redundant ESM test patch

nodejs/node#30997

* Remove redundant cli flag patch

nodejs/node#30466

* Update filenames.json

* Remove macro generation in GN build files

nodejs/node#30755

* Fix some compilation errors upstream

* Add uvwasi to deps

nodejs/node#30258

* Fix BoringSSL incompatibilities

* Fixup linked module patch

nodejs/node#30274

* Add missing sources to GN uv build

libuv/libuv#2347

* Patch some uvwasi incompatibilities

* chore: bump Node.js to v12.6.1

* Remove mark_arraybuffer_as_untransferable.patch

nodejs/node#30549

* Fix uvwasi build failure on win

* Fixup --perf-prof cli option error

* Fixup early cjs module loading

* fix: initialize diagnostics properly

nodejs/node#30025

* Disable new esm syntax specs

nodejs/node#30219

* Fixup v8 weakref hook spec

nodejs/node#29874

* Fix async context timer issue

* Disable monkey-patch-main spec

It relies on nodejs/node#29777, and we don't
override prepareStackTrace.

* Disable new tls specs

nodejs/node#23188

We don't support much of TLS owing to schisms between BoringSSL and
OpenSSL.

Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants