From 79284c28734bf854f44106835b5578ead75eb547 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Mon, 2 Mar 2020 15:52:56 -0500 Subject: [PATCH] [dev.boringcrypto] crypto/internal/boring: make accesses to RSA types with finalizers safer RSA key types have a finalizer that will free the underlying C value when the Go one is garbage collected. It's important that the finalizer doesn't run while a cgo call is using the underlying C value, so they require runtime.KeepAlive calls after each use. This is error prone, so replace it with a closure that provides access to the underlying C value and then automatically calls KeepAlive. AES, HMAC, and ECDSA also need KeepAlives, but they have much fewer call sites, so avoid the complexity for now. Change-Id: I6d6f38297cd1cf384a1639974d9739a939cbdbcc Reviewed-on: https://go-review.googlesource.com/c/go/+/221822 Run-TryBot: Filippo Valsorda TryBot-Result: Gobot Gobot Reviewed-by: Katie Hockman --- src/crypto/internal/boring/rsa.go | 111 +++++++++++++++++++----------- 1 file changed, 69 insertions(+), 42 deletions(-) diff --git a/src/crypto/internal/boring/rsa.go b/src/crypto/internal/boring/rsa.go index 1ebf6044ba..9f4f53e0d8 100644 --- a/src/crypto/internal/boring/rsa.go +++ b/src/crypto/internal/boring/rsa.go @@ -45,7 +45,8 @@ func GenerateKeyRSA(bits int) (N, E, D, P, Q, Dp, Dq, Qinv *big.Int, err error) } type PublicKeyRSA struct { - key *C.GO_RSA + // _key MUST NOT be accessed directly. Instead, use the withKey method. + _key *C.GO_RSA } func NewPublicKeyRSA(N, E *big.Int) (*PublicKeyRSA, error) { @@ -57,21 +58,26 @@ func NewPublicKeyRSA(N, E *big.Int) (*PublicKeyRSA, error) { !bigToBn(&key.e, E) { return nil, fail("BN_bin2bn") } - k := &PublicKeyRSA{key: key} - // Note: Because of the finalizer, any time k.key is passed to cgo, - // that call must be followed by a call to runtime.KeepAlive(k), - // to make sure k is not collected (and finalized) before the cgo - // call returns. + k := &PublicKeyRSA{_key: key} runtime.SetFinalizer(k, (*PublicKeyRSA).finalize) return k, nil } func (k *PublicKeyRSA) finalize() { - C._goboringcrypto_RSA_free(k.key) + C._goboringcrypto_RSA_free(k._key) +} + +func (k *PublicKeyRSA) withKey(f func(*C.GO_RSA) C.int) C.int { + // Because of the finalizer, any time _key is passed to cgo, that call must + // be followed by a call to runtime.KeepAlive, to make sure k is not + // collected (and finalized) before the cgo call returns. + defer runtime.KeepAlive(k) + return f(k._key) } type PrivateKeyRSA struct { - key *C.GO_RSA + // _key MUST NOT be accessed directly. Instead, use the withKey method. + _key *C.GO_RSA } func NewPrivateKeyRSA(N, E, D, P, Q, Dp, Dq, Qinv *big.Int) (*PrivateKeyRSA, error) { @@ -89,20 +95,24 @@ func NewPrivateKeyRSA(N, E, D, P, Q, Dp, Dq, Qinv *big.Int) (*PrivateKeyRSA, err !bigToBn(&key.iqmp, Qinv) { return nil, fail("BN_bin2bn") } - k := &PrivateKeyRSA{key: key} - // Note: Because of the finalizer, any time k.key is passed to cgo, - // that call must be followed by a call to runtime.KeepAlive(k), - // to make sure k is not collected (and finalized) before the cgo - // call returns. + k := &PrivateKeyRSA{_key: key} runtime.SetFinalizer(k, (*PrivateKeyRSA).finalize) return k, nil } func (k *PrivateKeyRSA) finalize() { - C._goboringcrypto_RSA_free(k.key) + C._goboringcrypto_RSA_free(k._key) +} + +func (k *PrivateKeyRSA) withKey(f func(*C.GO_RSA) C.int) C.int { + // Because of the finalizer, any time _key is passed to cgo, that call must + // be followed by a call to runtime.KeepAlive, to make sure k is not + // collected (and finalized) before the cgo call returns. + defer runtime.KeepAlive(k) + return f(k._key) } -func setupRSA(gokey interface{}, key *C.GO_RSA, +func setupRSA(withKey func(func(*C.GO_RSA) C.int) C.int, padding C.int, h hash.Hash, label []byte, saltLen int, ch crypto.Hash, init func(*C.GO_EVP_PKEY_CTX) C.int) (pkey *C.GO_EVP_PKEY, ctx *C.GO_EVP_PKEY_CTX, err error) { defer func() { @@ -122,12 +132,11 @@ func setupRSA(gokey interface{}, key *C.GO_RSA, if pkey == nil { return nil, nil, fail("EVP_PKEY_new") } - if C._goboringcrypto_EVP_PKEY_set1_RSA(pkey, key) == 0 { + if withKey(func(key *C.GO_RSA) C.int { + return C._goboringcrypto_EVP_PKEY_set1_RSA(pkey, key) + }) == 0 { return nil, nil, fail("EVP_PKEY_set1_RSA") } - // key is freed by the finalizer on gokey, which is a PrivateKeyRSA or a - // PublicKeyRSA. Ensure it doesn't run until after the cgo calls that use key. - runtime.KeepAlive(gokey) ctx = C._goboringcrypto_EVP_PKEY_CTX_new(pkey, nil) if ctx == nil { return nil, nil, fail("EVP_PKEY_CTX_new") @@ -174,13 +183,13 @@ func setupRSA(gokey interface{}, key *C.GO_RSA, return pkey, ctx, nil } -func cryptRSA(gokey interface{}, key *C.GO_RSA, +func cryptRSA(withKey func(func(*C.GO_RSA) C.int) C.int, padding C.int, h hash.Hash, label []byte, saltLen int, ch crypto.Hash, init func(*C.GO_EVP_PKEY_CTX) C.int, crypt func(*C.GO_EVP_PKEY_CTX, *C.uint8_t, *C.size_t, *C.uint8_t, C.size_t) C.int, in []byte) ([]byte, error) { - pkey, ctx, err := setupRSA(gokey, key, padding, h, label, saltLen, ch, init) + pkey, ctx, err := setupRSA(withKey, padding, h, label, saltLen, ch, init) if err != nil { return nil, err } @@ -199,27 +208,27 @@ func cryptRSA(gokey interface{}, key *C.GO_RSA, } func DecryptRSAOAEP(h hash.Hash, priv *PrivateKeyRSA, ciphertext, label []byte) ([]byte, error) { - return cryptRSA(priv, priv.key, C.GO_RSA_PKCS1_OAEP_PADDING, h, label, 0, 0, decryptInit, decrypt, ciphertext) + return cryptRSA(priv.withKey, C.GO_RSA_PKCS1_OAEP_PADDING, h, label, 0, 0, decryptInit, decrypt, ciphertext) } func EncryptRSAOAEP(h hash.Hash, pub *PublicKeyRSA, msg, label []byte) ([]byte, error) { - return cryptRSA(pub, pub.key, C.GO_RSA_PKCS1_OAEP_PADDING, h, label, 0, 0, encryptInit, encrypt, msg) + return cryptRSA(pub.withKey, C.GO_RSA_PKCS1_OAEP_PADDING, h, label, 0, 0, encryptInit, encrypt, msg) } func DecryptRSAPKCS1(priv *PrivateKeyRSA, ciphertext []byte) ([]byte, error) { - return cryptRSA(priv, priv.key, C.GO_RSA_PKCS1_PADDING, nil, nil, 0, 0, decryptInit, decrypt, ciphertext) + return cryptRSA(priv.withKey, C.GO_RSA_PKCS1_PADDING, nil, nil, 0, 0, decryptInit, decrypt, ciphertext) } func EncryptRSAPKCS1(pub *PublicKeyRSA, msg []byte) ([]byte, error) { - return cryptRSA(pub, pub.key, C.GO_RSA_PKCS1_PADDING, nil, nil, 0, 0, encryptInit, encrypt, msg) + return cryptRSA(pub.withKey, C.GO_RSA_PKCS1_PADDING, nil, nil, 0, 0, encryptInit, encrypt, msg) } func DecryptRSANoPadding(priv *PrivateKeyRSA, ciphertext []byte) ([]byte, error) { - return cryptRSA(priv, priv.key, C.GO_RSA_NO_PADDING, nil, nil, 0, 0, decryptInit, decrypt, ciphertext) + return cryptRSA(priv.withKey, C.GO_RSA_NO_PADDING, nil, nil, 0, 0, decryptInit, decrypt, ciphertext) } func EncryptRSANoPadding(pub *PublicKeyRSA, msg []byte) ([]byte, error) { - return cryptRSA(pub, pub.key, C.GO_RSA_NO_PADDING, nil, nil, 0, 0, encryptInit, encrypt, msg) + return cryptRSA(pub.withKey, C.GO_RSA_NO_PADDING, nil, nil, 0, 0, encryptInit, encrypt, msg) } // These dumb wrappers work around the fact that cgo functions cannot be used as values directly. @@ -248,12 +257,15 @@ func SignRSAPSS(priv *PrivateKeyRSA, h crypto.Hash, hashed []byte, saltLen int) if saltLen == 0 { saltLen = -1 } - out := make([]byte, C._goboringcrypto_RSA_size(priv.key)) + var out []byte var outLen C.size_t - if C._goboringcrypto_RSA_sign_pss_mgf1(priv.key, &outLen, base(out), C.size_t(len(out)), base(hashed), C.size_t(len(hashed)), md, nil, C.int(saltLen)) == 0 { + if priv.withKey(func(key *C.GO_RSA) C.int { + out = make([]byte, C._goboringcrypto_RSA_size(key)) + return C._goboringcrypto_RSA_sign_pss_mgf1(key, &outLen, base(out), C.size_t(len(out)), + base(hashed), C.size_t(len(hashed)), md, nil, C.int(saltLen)) + }) == 0 { return nil, fail("RSA_sign_pss_mgf1") } - runtime.KeepAlive(priv) return out[:outLen], nil } @@ -266,22 +278,27 @@ func VerifyRSAPSS(pub *PublicKeyRSA, h crypto.Hash, hashed, sig []byte, saltLen if saltLen == 0 { saltLen = -2 // auto-recover } - if C._goboringcrypto_RSA_verify_pss_mgf1(pub.key, base(hashed), C.size_t(len(hashed)), md, nil, C.int(saltLen), base(sig), C.size_t(len(sig))) == 0 { + if pub.withKey(func(key *C.GO_RSA) C.int { + return C._goboringcrypto_RSA_verify_pss_mgf1(key, base(hashed), C.size_t(len(hashed)), + md, nil, C.int(saltLen), base(sig), C.size_t(len(sig))) + }) == 0 { return fail("RSA_verify_pss_mgf1") } - runtime.KeepAlive(pub) return nil } func SignRSAPKCS1v15(priv *PrivateKeyRSA, h crypto.Hash, hashed []byte) ([]byte, error) { - out := make([]byte, C._goboringcrypto_RSA_size(priv.key)) if h == 0 { // No hashing. + var out []byte var outLen C.size_t - if C._goboringcrypto_RSA_sign_raw(priv.key, &outLen, base(out), C.size_t(len(out)), base(hashed), C.size_t(len(hashed)), C.GO_RSA_PKCS1_PADDING) == 0 { + if priv.withKey(func(key *C.GO_RSA) C.int { + out = make([]byte, C._goboringcrypto_RSA_size(key)) + return C._goboringcrypto_RSA_sign_raw(key, &outLen, base(out), C.size_t(len(out)), + base(hashed), C.size_t(len(hashed)), C.GO_RSA_PKCS1_PADDING) + }) == 0 { return nil, fail("RSA_sign_raw") } - runtime.KeepAlive(priv) return out[:outLen], nil } @@ -290,16 +307,22 @@ func SignRSAPKCS1v15(priv *PrivateKeyRSA, h crypto.Hash, hashed []byte) ([]byte, return nil, errors.New("crypto/rsa: unsupported hash function: " + strconv.Itoa(int(h))) } nid := C._goboringcrypto_EVP_MD_type(md) + var out []byte var outLen C.uint - if C._goboringcrypto_RSA_sign(nid, base(hashed), C.uint(len(hashed)), base(out), &outLen, priv.key) == 0 { + if priv.withKey(func(key *C.GO_RSA) C.int { + out = make([]byte, C._goboringcrypto_RSA_size(key)) + return C._goboringcrypto_RSA_sign(nid, base(hashed), C.uint(len(hashed)), + base(out), &outLen, key) + }) == 0 { return nil, fail("RSA_sign") } - runtime.KeepAlive(priv) return out[:outLen], nil } func VerifyRSAPKCS1v15(pub *PublicKeyRSA, h crypto.Hash, hashed, sig []byte) error { - size := int(C._goboringcrypto_RSA_size(pub.key)) + size := int(pub.withKey(func(key *C.GO_RSA) C.int { + return C.int(C._goboringcrypto_RSA_size(key)) + })) if len(sig) < size { // BoringCrypto requires sig to be same size as RSA key, so pad with leading zeros. zsig := make([]byte, size) @@ -309,13 +332,15 @@ func VerifyRSAPKCS1v15(pub *PublicKeyRSA, h crypto.Hash, hashed, sig []byte) err if h == 0 { var outLen C.size_t out := make([]byte, size) - if C._goboringcrypto_RSA_verify_raw(pub.key, &outLen, base(out), C.size_t(len(out)), base(sig), C.size_t(len(sig)), C.GO_RSA_PKCS1_PADDING) == 0 { + if pub.withKey(func(key *C.GO_RSA) C.int { + return C._goboringcrypto_RSA_verify_raw(key, &outLen, base(out), + C.size_t(len(out)), base(sig), C.size_t(len(sig)), C.GO_RSA_PKCS1_PADDING) + }) == 0 { return fail("RSA_verify") } if subtle.ConstantTimeCompare(hashed, out[:outLen]) != 1 { return fail("RSA_verify") } - runtime.KeepAlive(pub) return nil } md := cryptoHashToMD(h) @@ -323,9 +348,11 @@ func VerifyRSAPKCS1v15(pub *PublicKeyRSA, h crypto.Hash, hashed, sig []byte) err return errors.New("crypto/rsa: unsupported hash function") } nid := C._goboringcrypto_EVP_MD_type(md) - if C._goboringcrypto_RSA_verify(nid, base(hashed), C.size_t(len(hashed)), base(sig), C.size_t(len(sig)), pub.key) == 0 { + if pub.withKey(func(key *C.GO_RSA) C.int { + return C._goboringcrypto_RSA_verify(nid, base(hashed), C.size_t(len(hashed)), + base(sig), C.size_t(len(sig)), key) + }) == 0 { return fail("RSA_verify") } - runtime.KeepAlive(pub) return nil } -- 2.50.0