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

Convert big.Int to bignum via word slice #25

Merged
merged 1 commit into from May 12, 2022
Merged

Convert big.Int to bignum via word slice #25

merged 1 commit into from May 12, 2022

Conversation

qmuntal
Copy link
Contributor

@qmuntal qmuntal commented May 12, 2022

This PR is an almost direct port of https://go-review.googlesource.com/c/go/+/395875/10. In our case there is no visible performance win, which was the motivation of the original CL, but it paves the path to removing big.Int from go-crypto-openssl, which is necessary for go1.19.

@jaredpar
Copy link
Member

What is the advantage of removing big.Int from the crypto layer?

@@ -180,6 +180,9 @@ DEFINEFUNC(void, BN_clear_free, (GO_BIGNUM_PTR arg0), (arg0)) \
DEFINEFUNC(int, BN_num_bits, (const GO_BIGNUM_PTR arg0), (arg0)) \
DEFINEFUNC(GO_BIGNUM_PTR, BN_bin2bn, (const unsigned char *arg0, int arg1, GO_BIGNUM_PTR arg2), (arg0, arg1, arg2)) \
DEFINEFUNC(int, BN_bn2bin, (const GO_BIGNUM_PTR arg0, unsigned char *arg1), (arg0, arg1)) \
/* bn_lebin2bn and bn_bn2lebinpad are not exported in any OpenSSL 1.0.2, but they exist. */ \
/*check:from=1.1.0*/ DEFINEFUNC_RENAMED_1_1(GO_BIGNUM_PTR, BN_lebin2bn, bn_lebin2bn, (const unsigned char *s, int len, GO_BIGNUM_PTR ret), (s, len, ret)) \
/*check:from=1.1.0*/ DEFINEFUNC_RENAMED_1_1(int, BN_bn2lebinpad, bn_bn2lebinpad, (const GO_BIGNUM_PTR a, unsigned char *to, int tolen), (a, to, tolen)) \
Copy link
Member

Choose a reason for hiding this comment

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

/*check:from=1.1.0*/

What impact does this have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checkheader won't validate that the signature defined here matches the one in the official OpenSSL headers when testing with OpenSSL 1.0.2. We can only trust on the developer copy-past skills.

It does not have any runtime impact.

@qmuntal
Copy link
Contributor Author

qmuntal commented May 12, 2022

What is the advantage of removing big.Int from the crypto layer?

The Go security team is trying to reduce the usage of big.Int from crypto code, as it is slow, not constant time and not designed for being used in the cryptography foundations.

This is the commit were big.Int was removed from boring: https://go-review.googlesource.com/c/go/+/395877/12

And these are other issues related to removing big.Int from other packages or fixing it:

In our case, it will make security reviews a little bit easier, as by removing big.Int from our code there won't be any doubt we are really delegating big integer operations to OpenSSL.

@@ -180,6 +180,9 @@ DEFINEFUNC(void, BN_clear_free, (GO_BIGNUM_PTR arg0), (arg0)) \
DEFINEFUNC(int, BN_num_bits, (const GO_BIGNUM_PTR arg0), (arg0)) \
DEFINEFUNC(GO_BIGNUM_PTR, BN_bin2bn, (const unsigned char *arg0, int arg1, GO_BIGNUM_PTR arg2), (arg0, arg1, arg2)) \
DEFINEFUNC(int, BN_bn2bin, (const GO_BIGNUM_PTR arg0, unsigned char *arg1), (arg0, arg1)) \
/* bn_lebin2bn and bn_bn2lebinpad are not exported in any OpenSSL 1.0.2, but they exist. */ \
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, I found that these were ported to 1.0.2 in 2019 by openssl/openssl#9793 to support a constant-time fix.

@qmuntal qmuntal merged commit 6ddecf2 into main May 12, 2022
@qmuntal qmuntal deleted the dev/qmuntal/bbig branch May 12, 2022 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants