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

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
Contributor

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.

davidben added 2 commits July 30, 2019 09:38
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@deepak1556 deepak1556 force-pushed the backport_crypto_4_x branch from 08853ee to 5efd0aa Compare July 30, 2019 16:39
Copy link
Contributor

@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.

5 participants