Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix: malloc mismatch in node_crypto (electron-4.x) #112

Open
wants to merge 2 commits into
base: electron-node-v10.11.0-V8-6.9
Choose a base branch
from

Conversation

deepak1556
Copy link
Member

Backports the crypto patches mentioned in #110, minimizing the original PR surface.

@deepak1556 deepak1556 requested a review from a team July 29, 2019 07:26
@nornagon
Copy link
Member

Didn't we switch to using patch files for node?

@deepak1556
Copy link
Member Author

Thats what i thought, didn't get into 4.x. @MarshallOfSound are you gonna add it ? If not I can do it and follow up with this patch over there.

EC_KEY_key2buf returns an OPENSSL_malloc'd pointer so it shouldn't be
passed into Buffer::New, which expect a libc malloc'd pointer. Instead,
factor out the ECDH::GetPublicKey code which uses EC_POINT_point2oct.

This preserves the existing behavior where encoding failures are
silently ignored, but it is probably safe to CHECK fail them instead.

PR-URL: nodejs/node#25717
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
SSL_set_tlsext_status_ocsp_resp expects the data to be allocated with
OPENSSL_malloc, not libc malloc, so use OpenSSLMalloc.

Additionally, though OpenSSL doesn't type-check due to it being a macro,
the function is documented to take an unsigned char pointer:
https://www.openssl.org/docs/man1.1.0/ssl/SSL_set_tlsext_status_ocsp_resp.html

(By default, OPENSSL_malloc is the same as libc malloc, but it is
possible to customize this.)

PR-URL: nodejs/node#25706
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

The backport itself looks good to me.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants