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

Use BN_clear_free in places where BN_free is being used #4072

Merged
merged 4 commits into from Jan 6, 2018
Merged

Use BN_clear_free in places where BN_free is being used #4072

merged 4 commits into from Jan 6, 2018

Conversation

tuxxy
Copy link
Contributor

@tuxxy tuxxy commented Jan 5, 2018

What this does:

  1. Implements BN_clear_free in places where BN_free was being used on private values. (See Why not use EC_POINT_clear_free and BN_clear_free? #4065)
  2. Keeps the use of BN_free on public values.

Why?

  1. BN_clear_free is a security improvement. By replacing BN_free with this, whenever the garbage collector is called it will: 1) clear the memory with zeros, then 2) free the memory.

Copy link
Member

@alex alex left a comment

Choose a reason for hiding this comment

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

Besides these comments, looks good.

@@ -353,7 +353,7 @@ def generate_rsa_private_key(self, public_exponent, key_size):
rsa_cdata = self._ffi.gc(rsa_cdata, self._lib.RSA_free)

bn = self._int_to_bn(public_exponent)
bn = self._ffi.gc(bn, self._lib.BN_free)
bn = self._ffi.gc(bn, self._lib.BN_clear_free)
Copy link
Member

Choose a reason for hiding this comment

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

Not needed here, this is a public value

@@ -1499,8 +1499,8 @@ def _ec_key_set_public_key_affine_coordinates(self, ctx, x, y):
"Invalid EC key. Both x and y must be non-negative."
)

x = self._ffi.gc(self._int_to_bn(x), self._lib.BN_free)
y = self._ffi.gc(self._int_to_bn(y), self._lib.BN_free)
x = self._ffi.gc(self._int_to_bn(x), self._lib.BN_clear_free)
Copy link
Member

Choose a reason for hiding this comment

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

These are also public

@@ -695,7 +695,7 @@ def _asn1_to_der(backend, asn1_type):
def _asn1_integer_to_int(backend, asn1_int):
bn = backend._lib.ASN1_INTEGER_to_BN(asn1_int, backend._ffi.NULL)
backend.openssl_assert(bn != backend._ffi.NULL)
bn = backend._ffi.gc(bn, backend._lib.BN_free)
bn = backend._ffi.gc(bn, backend._lib.BN_clear_free)
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure these are always public, since they're from an X.509 cert -- does that sound right @reaperhulk ?

Copy link
Member

Choose a reason for hiding this comment

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

Every point where we utilize this is within X509 and is public, yes. I guess that doesn't preclude us using it for non-public things in the future, although I can't currently think of any scenarios where we'd want to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I'll take care of this then.

@@ -29,7 +29,7 @@ def _encode_asn1_int(backend, x):
# machine's native integer limits (note: `int_to_bn` doesn't automatically
# GC).
i = backend._int_to_bn(x)
i = backend._ffi.gc(i, backend._lib.BN_free)
i = backend._ffi.gc(i, backend._lib.BN_clear_free)
Copy link
Member

Choose a reason for hiding this comment

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

Same.

@tuxxy
Copy link
Contributor Author

tuxxy commented Jan 5, 2018

Ah, it looks like I mistakenly threw in my branch for adding EC_POINT_clear_free here.

I can remove this and push it to another PR, or we can keep it in. @alex @reaperhulk Your call..

@@ -1391,10 +1391,10 @@ def derive_elliptic_curve_private_key(self, private_value, curve):

point = self._lib.EC_POINT_new(group)
self.openssl_assert(point != self._ffi.NULL)
point = self._ffi.gc(point, self._lib.EC_POINT_free)
point = self._ffi.gc(point, self._lib.EC_POINT_clear_free)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mind you that this is also a public value. Other than that, not sure if I see a use for EC_POINT_clear_free if I'm going off of the logic from the implementation of BN_clear_free.

@alex
Copy link
Member

alex commented Jan 5, 2018

If you rebase this branch on master it should clean it up.

@alex alex merged commit 110398e into pyca:master Jan 6, 2018
@tuxxy tuxxy deleted the patch-use_bn_clear_free branch January 6, 2018 01:12
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants