From d2005b967cb0c644a702cf9dd9b20d4d9c85106d Mon Sep 17 00:00:00 2001 From: qmuntal Date: Mon, 25 Nov 2024 16:16:47 +0100 Subject: [PATCH 01/12] validate ECDH keys --- ec.go | 16 +++++++ ecdh.go | 56 ++++++++++++++++++++++- ecdh_test.go | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++ shims.h | 1 + 4 files changed, 197 insertions(+), 2 deletions(-) diff --git a/ec.go b/ec.go index 03c51e5a..ccaf5a4c 100644 --- a/ec.go +++ b/ec.go @@ -19,6 +19,22 @@ func curveNID(curve string) (C.int, error) { return 0, errUnknownCurve } +// curveSize returns the size of the curve in bytes. +func curveSize(curve string) int { + switch curve { + default: + panic("openssl: unknown curve " + curve) + case "P-224": + return 224 / 8 + case "P-256": + return 256 / 8 + case "P-384": + return 384 / 8 + case "P-521": + return (521 + 7) / 8 + } +} + // encodeEcPoint encodes pt. func encodeEcPoint(group C.GO_EC_GROUP_PTR, pt C.GO_EC_POINT_PTR) ([]byte, error) { // Get encoded point size. diff --git a/ecdh.go b/ecdh.go index 5b146749..69baa22a 100644 --- a/ecdh.go +++ b/ecdh.go @@ -6,6 +6,7 @@ package openssl import "C" import ( "errors" + "math/bits" "runtime" "unsafe" ) @@ -30,8 +31,8 @@ func (k *PrivateKeyECDH) finalize() { } func NewPublicKeyECDH(curve string, bytes []byte) (*PublicKeyECDH, error) { - if len(bytes) < 1 { - return nil, errors.New("NewPublicKeyECDH: missing key") + if len(bytes) != 1+2*curveSize(curve) { + return nil, errors.New("NewPublicKeyECDH: wrong key length") } pkey, err := newECDHPkey(curve, bytes, false) if err != nil { @@ -45,6 +46,9 @@ func NewPublicKeyECDH(curve string, bytes []byte) (*PublicKeyECDH, error) { func (k *PublicKeyECDH) Bytes() []byte { return k.bytes } func NewPrivateKeyECDH(curve string, bytes []byte) (*PrivateKeyECDH, error) { + if len(bytes) != curveSize(curve) { + return nil, errors.New("NewPrivateKeyECDH: wrong key length") + } pkey, err := newECDHPkey(curve, bytes, true) if err != nil { return nil, err @@ -161,6 +165,9 @@ func newECDHPkey1(nid C.int, bytes []byte, isPrivate bool) (pkey C.GO_EVP_PKEY_P return nil, newOpenSSLError("EC_KEY_set_public_key") } } + if C.go_openssl_EC_KEY_check_key(key) != 1 { + return nil, newOpenSSLError("EC_KEY_check_key") + } return newEVPPKEY(key) } @@ -311,3 +318,48 @@ func GenerateKeyECDH(curve string) (*PrivateKeyECDH, []byte, error) { runtime.SetFinalizer(k, (*PrivateKeyECDH).finalize) return k, bytes, nil } + +// isZero reports whether x is all zeroes in constant time. +func isZero(x []byte) bool { + var acc byte + for _, b := range x { + acc |= b + } + return acc == 0 +} + +// isECDHLess reports whether a < b, where a and b are big-endian buffers of the +// same length and shorter than 72 bytes. +func isECDHLess(a, b []byte) bool { + if len(a) != len(b) { + panic("crypto/ecdh: internal error: mismatched isLess inputs") + } + + // Copy the values into a fixed-size preallocated little-endian buffer. + // 72 bytes is enough for every scalar in this package, and having a fixed + // size lets us avoid heap allocations. + if len(a) > 72 { + panic("crypto/ecdh: internal error: isLess input too large") + } + bufA, bufB := make([]byte, 72), make([]byte, 72) + for i := range a { + bufA[i], bufB[i] = a[len(a)-i-1], b[len(b)-i-1] + } + + // Perform a subtraction with borrow. + var borrow uint64 + for i := 0; i < len(bufA); i += 8 { + limbA, limbB := leUint64(bufA[i:]), leUint64(bufB[i:]) + _, borrow = bits.Sub64(limbA, limbB, borrow) + } + + // If there is a borrow at the end of the operation, then a < b. + return borrow == 1 +} + +// leUint64 returns the little-endian uint64 value in b. +func leUint64(b []byte) uint64 { + _ = b[7] // bounds check hint to compiler; see golang.org/issue/14808 + return uint64(b[0]) | uint64(b[1])<<8 | uint64(b[2])<<16 | uint64(b[3])<<24 | + uint64(b[4])<<32 | uint64(b[5])<<40 | uint64(b[6])<<48 | uint64(b[7])<<56 +} diff --git a/ecdh_test.go b/ecdh_test.go index c83a347b..4b2c6bd3 100644 --- a/ecdh_test.go +++ b/ecdh_test.go @@ -3,6 +3,7 @@ package openssl_test import ( "bytes" "encoding/hex" + "strings" "testing" "github.com/golang-fips/openssl/v2" @@ -171,3 +172,128 @@ func BenchmarkECDH(b *testing.B) { } } } + +var invalidECDHPrivateKeys = map[string][]string{ + "P-256": { + // Bad lengths. + "", + "01", + "01010101010101010101010101010101010101010101010101010101010101", + "000101010101010101010101010101010101010101010101010101010101010101", + strings.Repeat("01", 200), + // Zero. + "0000000000000000000000000000000000000000000000000000000000000000", + // Order of the curve and above. + "ffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632551", + "ffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632552", + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", + }, + "P-384": { + // Bad lengths. + "", + "01", + "0101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101", + "00010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101", + strings.Repeat("01", 200), + // Zero. + "000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", + // Order of the curve and above. + "ffffffffffffffffffffffffffffffffffffffffffffffffc7634d81f4372ddf581a0db248b0a77aecec196accc52973", + "ffffffffffffffffffffffffffffffffffffffffffffffffc7634d81f4372ddf581a0db248b0a77aecec196accc52974", + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", + }, + "P-521": { + // Bad lengths. + "", + "01", + "0101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101", + "00010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101", + strings.Repeat("01", 200), + // Zero. + "000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", + // Order of the curve and above. + "01fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffa51868783bf2f966b7fcc0148f709a5d03bb5c9b8899c47aebb6fb71e91386409", + "01fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffa51868783bf2f966b7fcc0148f709a5d03bb5c9b8899c47aebb6fb71e9138640a", + "11fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffa51868783bf2f966b7fcc0148f709a5d03bb5c9b8899c47aebb6fb71e91386409", + "03fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff4a30d0f077e5f2cd6ff980291ee134ba0776b937113388f5d76df6e3d2270c812", + }, +} + +var invalidECDHPublicKeys = map[string][]string{ + "P-256": { + // Bad lengths. + "", + "04", + strings.Repeat("04", 200), + // Infinity. + "00", + // Compressed encodings. + "036b17d1f2e12c4247f8bce6e563a440f277037d812deb33a0f4a13945d898c296", + "02e2534a3532d08fbba02dde659ee62bd0031fe2db785596ef509302446b030852", + // Points not on the curve. + "046b17d1f2e12c4247f8bce6e563a440f277037d812deb33a0f4a13945d898c2964fe342e2fe1a7f9b8ee7eb4a7c0f9e162bce33576b315ececbb6406837bf51f6", + "0400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", + }, + "P-384": { + // Bad lengths. + "", + "04", + strings.Repeat("04", 200), + // Infinity. + "00", + // Compressed encodings. + "03aa87ca22be8b05378eb1c71ef320ad746e1d3b628ba79b9859f741e082542a385502f25dbf55296c3a545e3872760ab7", + "0208d999057ba3d2d969260045c55b97f089025959a6f434d651d207d19fb96e9e4fe0e86ebe0e64f85b96a9c75295df61", + // Points not on the curve. + "04aa87ca22be8b05378eb1c71ef320ad746e1d3b628ba79b9859f741e082542a385502f25dbf55296c3a545e3872760ab73617de4a96262c6f5d9e98bf9292dc29f8f41dbd289a147ce9da3113b5f0b8c00a60b1ce1d7e819d7a431d7c90ea0e60", + "04000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", + }, + "P-521": { + // Bad lengths. + "", + "04", + strings.Repeat("04", 200), + // Infinity. + "00", + // Compressed encodings. + "030035b5df64ae2ac204c354b483487c9070cdc61c891c5ff39afc06c5d55541d3ceac8659e24afe3d0750e8b88e9f078af066a1d5025b08e5a5e2fbc87412871902f3", + "0200c6858e06b70404e9cd9e3ecb662395b4429c648139053fb521f828af606b4d3dbaa14b5e77efe75928fe1dc127a2ffa8de3348b3c1856a429bf97e7e31c2e5bd66", + // Points not on the curve. + "0400c6858e06b70404e9cd9e3ecb662395b4429c648139053fb521f828af606b4d3dbaa14b5e77efe75928fe1dc127a2ffa8de3348b3c1856a429bf97e7e31c2e5bd66011839296a789a3bc0045c8a5fb42c7d1bd998f54449579b446817afbd17273e662c97ee72995ef42640c550b9013fad0761353c7086a272c24088be94769fd16651", + "04000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", + }, +} + +func TestECDHNewPrivateKeyECDH_Invalid(t *testing.T) { + for _, curve := range []string{"P-256", "P-384", "P-521"} { + t.Run(curve, func(t *testing.T) { + for _, input := range invalidECDHPrivateKeys[curve] { + k, err := openssl.NewPrivateKeyECDH(curve, hexDecode(t, input)) + if err == nil { + t.Errorf("unexpectedly accepted %q", input) + } else if k != nil { + t.Error("PrivateKey was not nil on error") + } else if strings.Contains(err.Error(), "boringcrypto") { + t.Errorf("boringcrypto error leaked out: %v", err) + } + } + }) + } +} + +func TestECDHNewPublicKeyECDH_Invalid(t *testing.T) { + for _, curve := range []string{"P-256", "P-384", "P-521"} { + t.Run(curve, func(t *testing.T) { + for _, input := range invalidECDHPublicKeys[curve] { + k, err := openssl.NewPublicKeyECDH(curve, hexDecode(t, input)) + if err == nil { + t.Errorf("unexpectedly accepted %q", input) + } else if k != nil { + t.Error("PublicKey was not nil on error") + } else if strings.Contains(err.Error(), "boringcrypto") { + t.Errorf("boringcrypto error leaked out: %v", err) + } + } + }) + } +} diff --git a/shims.h b/shims.h index 156d8e8a..003f3cef 100644 --- a/shims.h +++ b/shims.h @@ -345,6 +345,7 @@ DEFINEFUNC_LEGACY_1(const GO_BIGNUM_PTR, EC_KEY_get0_private_key, (const GO_EC_K DEFINEFUNC_LEGACY_1(const GO_EC_POINT_PTR, EC_KEY_get0_public_key, (const GO_EC_KEY_PTR arg0), (arg0)) \ DEFINEFUNC_LEGACY_1(GO_EC_KEY_PTR, EC_KEY_new_by_curve_name, (int arg0), (arg0)) \ DEFINEFUNC_LEGACY_1(int, EC_KEY_set_private_key, (GO_EC_KEY_PTR arg0, const GO_BIGNUM_PTR arg1), (arg0, arg1)) \ +DEFINEFUNC_LEGACY_1(int, EC_KEY_check_key, (const GO_EC_KEY_PTR key), (key)) \ DEFINEFUNC(GO_EC_POINT_PTR, EC_POINT_new, (const GO_EC_GROUP_PTR arg0), (arg0)) \ DEFINEFUNC(void, EC_POINT_free, (GO_EC_POINT_PTR arg0), (arg0)) \ DEFINEFUNC(int, EC_POINT_mul, (const GO_EC_GROUP_PTR group, GO_EC_POINT_PTR r, const GO_BIGNUM_PTR n, const GO_EC_POINT_PTR q, const GO_BIGNUM_PTR m, GO_BN_CTX_PTR ctx), (group, r, n, q, m, ctx)) \ From 9e7a20535b52bdfc0390273e2a6960d0fc652835 Mon Sep 17 00:00:00 2001 From: qmuntal Date: Mon, 25 Nov 2024 16:18:55 +0100 Subject: [PATCH 02/12] remove unused functions --- ecdh.go | 46 ---------------------------------------------- 1 file changed, 46 deletions(-) diff --git a/ecdh.go b/ecdh.go index 69baa22a..666e6c75 100644 --- a/ecdh.go +++ b/ecdh.go @@ -6,7 +6,6 @@ package openssl import "C" import ( "errors" - "math/bits" "runtime" "unsafe" ) @@ -318,48 +317,3 @@ func GenerateKeyECDH(curve string) (*PrivateKeyECDH, []byte, error) { runtime.SetFinalizer(k, (*PrivateKeyECDH).finalize) return k, bytes, nil } - -// isZero reports whether x is all zeroes in constant time. -func isZero(x []byte) bool { - var acc byte - for _, b := range x { - acc |= b - } - return acc == 0 -} - -// isECDHLess reports whether a < b, where a and b are big-endian buffers of the -// same length and shorter than 72 bytes. -func isECDHLess(a, b []byte) bool { - if len(a) != len(b) { - panic("crypto/ecdh: internal error: mismatched isLess inputs") - } - - // Copy the values into a fixed-size preallocated little-endian buffer. - // 72 bytes is enough for every scalar in this package, and having a fixed - // size lets us avoid heap allocations. - if len(a) > 72 { - panic("crypto/ecdh: internal error: isLess input too large") - } - bufA, bufB := make([]byte, 72), make([]byte, 72) - for i := range a { - bufA[i], bufB[i] = a[len(a)-i-1], b[len(b)-i-1] - } - - // Perform a subtraction with borrow. - var borrow uint64 - for i := 0; i < len(bufA); i += 8 { - limbA, limbB := leUint64(bufA[i:]), leUint64(bufB[i:]) - _, borrow = bits.Sub64(limbA, limbB, borrow) - } - - // If there is a borrow at the end of the operation, then a < b. - return borrow == 1 -} - -// leUint64 returns the little-endian uint64 value in b. -func leUint64(b []byte) uint64 { - _ = b[7] // bounds check hint to compiler; see golang.org/issue/14808 - return uint64(b[0]) | uint64(b[1])<<8 | uint64(b[2])<<16 | uint64(b[3])<<24 | - uint64(b[4])<<32 | uint64(b[5])<<40 | uint64(b[6])<<48 | uint64(b[7])<<56 -} From f1bc879e6f1c29ecf82b8e536e21e4b8e0dc24f0 Mon Sep 17 00:00:00 2001 From: qmuntal Date: Mon, 25 Nov 2024 16:43:05 +0100 Subject: [PATCH 03/12] fix TestECDHVectors for OpenSSL 1 --- ecdh.go | 72 +++++++++++++++++++++++---------------------------------- 1 file changed, 29 insertions(+), 43 deletions(-) diff --git a/ecdh.go b/ecdh.go index 666e6c75..5d79db6a 100644 --- a/ecdh.go +++ b/ecdh.go @@ -20,9 +20,8 @@ func (k *PublicKeyECDH) finalize() { } type PrivateKeyECDH struct { - _pkey C.GO_EVP_PKEY_PTR - curve string - hasPublicKey bool + _pkey C.GO_EVP_PKEY_PTR + curve string } func (k *PrivateKeyECDH) finalize() { @@ -52,20 +51,13 @@ func NewPrivateKeyECDH(curve string, bytes []byte) (*PrivateKeyECDH, error) { if err != nil { return nil, err } - k := &PrivateKeyECDH{pkey, curve, false} + k := &PrivateKeyECDH{pkey, curve} runtime.SetFinalizer(k, (*PrivateKeyECDH).finalize) return k, nil } func (k *PrivateKeyECDH) PublicKey() (*PublicKeyECDH, error) { defer runtime.KeepAlive(k) - if !k.hasPublicKey { - err := deriveEcdhPublicKey(k._pkey, k.curve) - if err != nil { - return nil, err - } - k.hasPublicKey = true - } var pkey C.GO_EVP_PKEY_PTR defer func() { C.go_openssl_EVP_PKEY_free(pkey) @@ -141,6 +133,7 @@ func newECDHPkey1(nid C.int, bytes []byte, isPrivate bool) (pkey C.GO_EVP_PKEY_P C.go_openssl_EC_KEY_free(key) } }() + group := C.go_openssl_EC_KEY_get0_group(key) if isPrivate { priv := C.go_openssl_BN_bin2bn(base(bytes), C.int(len(bytes)), nil) if priv == nil { @@ -150,8 +143,15 @@ func newECDHPkey1(nid C.int, bytes []byte, isPrivate bool) (pkey C.GO_EVP_PKEY_P if C.go_openssl_EC_KEY_set_private_key(key, priv) != 1 { return nil, newOpenSSLError("EC_KEY_set_private_key") } + pub, err := pointMult(group, priv) + if err != nil { + return nil, err + } + defer C.go_openssl_EC_POINT_free(pub) + if C.go_openssl_EC_KEY_set_public_key(key, pub) != 1 { + return nil, newOpenSSLError("EC_KEY_set_public_key") + } } else { - group := C.go_openssl_EC_KEY_get0_group(key) pub := C.go_openssl_EC_POINT_new(group) if pub == nil { return nil, newOpenSSLError("EC_POINT_new") @@ -196,39 +196,25 @@ func newECDHPkey3(nid C.int, bytes []byte, isPrivate bool) (C.GO_EVP_PKEY_PTR, e return newEvpFromParams(C.GO_EVP_PKEY_EC, selection, params) } +func pointMult(group C.GO_EC_GROUP_PTR, priv C.GO_BIGNUM_PTR) (C.GO_EC_POINT_PTR, error) { + // OpenSSL does not expose any method to generate the public + // key from the private key [1], so we have to calculate it here. + // [1] https://github.com/openssl/openssl/issues/18437#issuecomment-1144717206 + pt := C.go_openssl_EC_POINT_new(group) + if pt == nil { + return nil, newOpenSSLError("EC_POINT_new") + } + if C.go_openssl_EC_POINT_mul(group, pt, priv, nil, nil, nil) == 0 { + C.go_openssl_EC_POINT_free(pt) + return nil, newOpenSSLError("EC_POINT_mul") + } + return pt, nil +} + // deriveEcdhPublicKey sets the raw public key of pkey by deriving it from // the raw private key. func deriveEcdhPublicKey(pkey C.GO_EVP_PKEY_PTR, curve string) error { - derive := func(group C.GO_EC_GROUP_PTR, priv C.GO_BIGNUM_PTR) (C.GO_EC_POINT_PTR, error) { - // OpenSSL does not expose any method to generate the public - // key from the private key [1], so we have to calculate it here. - // [1] https://github.com/openssl/openssl/issues/18437#issuecomment-1144717206 - pt := C.go_openssl_EC_POINT_new(group) - if pt == nil { - return nil, newOpenSSLError("EC_POINT_new") - } - if C.go_openssl_EC_POINT_mul(group, pt, priv, nil, nil, nil) == 0 { - C.go_openssl_EC_POINT_free(pt) - return nil, newOpenSSLError("EC_POINT_mul") - } - return pt, nil - } switch vMajor { - case 1: - key := getECKey(pkey) - priv := C.go_openssl_EC_KEY_get0_private_key(key) - if priv == nil { - return newOpenSSLError("EC_KEY_get0_private_key") - } - group := C.go_openssl_EC_KEY_get0_group(key) - pub, err := derive(group, priv) - if err != nil { - return err - } - defer C.go_openssl_EC_POINT_free(pub) - if C.go_openssl_EC_KEY_set_public_key(key, pub) != 1 { - return newOpenSSLError("EC_KEY_set_public_key") - } case 3: var priv C.GO_BIGNUM_PTR if C.go_openssl_EVP_PKEY_get_bn_param(pkey, _OSSL_PKEY_PARAM_PRIV_KEY, &priv) != 1 { @@ -237,7 +223,7 @@ func deriveEcdhPublicKey(pkey C.GO_EVP_PKEY_PTR, curve string) error { defer C.go_openssl_BN_clear_free(priv) nid, _ := curveNID(curve) pubBytes, err := generateAndEncodeEcPublicKey(nid, func(group C.GO_EC_GROUP_PTR) (C.GO_EC_POINT_PTR, error) { - return derive(group, priv) + return pointMult(group, priv) }) if err != nil { return err @@ -313,7 +299,7 @@ func GenerateKeyECDH(curve string) (*PrivateKeyECDH, []byte, error) { if err := bnToBinPad(priv, bytes); err != nil { return nil, nil, err } - k = &PrivateKeyECDH{pkey, curve, true} + k = &PrivateKeyECDH{pkey, curve} runtime.SetFinalizer(k, (*PrivateKeyECDH).finalize) return k, bytes, nil } From 963cd39288824215881ceb56c16d638caf5c534a Mon Sep 17 00:00:00 2001 From: qmuntal Date: Mon, 25 Nov 2024 20:39:40 +0100 Subject: [PATCH 04/12] fix newECDHPkey3 --- ecdh.go | 40 +++++++++++++--------------------------- 1 file changed, 13 insertions(+), 27 deletions(-) diff --git a/ecdh.go b/ecdh.go index 5d79db6a..9d4d8dbd 100644 --- a/ecdh.go +++ b/ecdh.go @@ -181,7 +181,19 @@ func newECDHPkey3(nid C.int, bytes []byte, isPrivate bool) (C.GO_EVP_PKEY_PTR, e bld.addUTF8String(_OSSL_PKEY_PARAM_GROUP_NAME, C.go_openssl_OBJ_nid2sn(nid), 0) var selection C.int if isPrivate { - bld.addBin(_OSSL_PKEY_PARAM_PRIV_KEY, bytes, true) + priv := C.go_openssl_BN_bin2bn(base(bytes), C.int(len(bytes)), nil) + if priv == nil { + return nil, newOpenSSLError("BN_bin2bn") + } + defer C.go_openssl_BN_clear_free(priv) + pubBytes, err := generateAndEncodeEcPublicKey(nid, func(group C.GO_EC_GROUP_PTR) (C.GO_EC_POINT_PTR, error) { + return pointMult(group, priv) + }) + if err != nil { + return nil, err + } + bld.addOctetString(_OSSL_PKEY_PARAM_PUB_KEY, pubBytes) + bld.addBN(_OSSL_PKEY_PARAM_PRIV_KEY, priv) selection = C.GO_EVP_PKEY_KEYPAIR } else { bld.addOctetString(_OSSL_PKEY_PARAM_PUB_KEY, bytes) @@ -211,32 +223,6 @@ func pointMult(group C.GO_EC_GROUP_PTR, priv C.GO_BIGNUM_PTR) (C.GO_EC_POINT_PTR return pt, nil } -// deriveEcdhPublicKey sets the raw public key of pkey by deriving it from -// the raw private key. -func deriveEcdhPublicKey(pkey C.GO_EVP_PKEY_PTR, curve string) error { - switch vMajor { - case 3: - var priv C.GO_BIGNUM_PTR - if C.go_openssl_EVP_PKEY_get_bn_param(pkey, _OSSL_PKEY_PARAM_PRIV_KEY, &priv) != 1 { - return newOpenSSLError("EVP_PKEY_get_bn_param") - } - defer C.go_openssl_BN_clear_free(priv) - nid, _ := curveNID(curve) - pubBytes, err := generateAndEncodeEcPublicKey(nid, func(group C.GO_EC_GROUP_PTR) (C.GO_EC_POINT_PTR, error) { - return pointMult(group, priv) - }) - if err != nil { - return err - } - if C.go_openssl_EVP_PKEY_set1_encoded_public_key(pkey, base(pubBytes), C.size_t(len(pubBytes))) != 1 { - return newOpenSSLError("EVP_PKEY_set1_encoded_public_key") - } - default: - panic(errUnsupportedVersion()) - } - return nil -} - func ECDH(priv *PrivateKeyECDH, pub *PublicKeyECDH) ([]byte, error) { defer runtime.KeepAlive(priv) defer runtime.KeepAlive(pub) From 83fb30f6f91c4e8e382717e4329102ea57b53eef Mon Sep 17 00:00:00 2001 From: qmuntal Date: Mon, 25 Nov 2024 20:55:11 +0100 Subject: [PATCH 05/12] add ECDH key validations for OpenSSL 3 --- ecdh.go | 15 ++++++++++++++- shims.h | 2 ++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/ecdh.go b/ecdh.go index 9d4d8dbd..1cf02318 100644 --- a/ecdh.go +++ b/ecdh.go @@ -205,7 +205,20 @@ func newECDHPkey3(nid C.int, bytes []byte, isPrivate bool) (C.GO_EVP_PKEY_PTR, e return nil, err } defer C.go_openssl_OSSL_PARAM_free(params) - return newEvpFromParams(C.GO_EVP_PKEY_EC, selection, params) + pkey, err := newEvpFromParams(C.GO_EVP_PKEY_EC, selection, params) + if err != nil { + return nil, err + } + if isPrivate { + if C.go_openssl_EVP_PKEY_private_check(pkey) != 1 { + return nil, errors.New("crypto/ecdh: invalid private key") + } + } else { + if C.go_openssl_EVP_PKEY_public_check_quick(pkey) != 1 { + return nil, errors.New("crypto/ecdh: invalid public key") + } + } + return pkey, nil } func pointMult(group C.GO_EC_GROUP_PTR, priv C.GO_BIGNUM_PTR) (C.GO_EC_POINT_PTR, error) { diff --git a/shims.h b/shims.h index 003f3cef..e4381754 100644 --- a/shims.h +++ b/shims.h @@ -310,6 +310,8 @@ DEFINEFUNC(int, EVP_PKEY_sign, (GO_EVP_PKEY_CTX_PTR arg0, unsigned char *arg1, s DEFINEFUNC(int, EVP_PKEY_derive_init, (GO_EVP_PKEY_CTX_PTR ctx), (ctx)) \ DEFINEFUNC(int, EVP_PKEY_derive_set_peer, (GO_EVP_PKEY_CTX_PTR ctx, GO_EVP_PKEY_PTR peer), (ctx, peer)) \ DEFINEFUNC(int, EVP_PKEY_derive, (GO_EVP_PKEY_CTX_PTR ctx, unsigned char *key, size_t *keylen), (ctx, key, keylen)) \ +DEFINEFUNC(int, EVP_PKEY_public_check_quick, (GO_EVP_PKEY_CTX_PTR ctx), (ctx)) \ +DEFINEFUNC(int, EVP_PKEY_private_check, (GO_EVP_PKEY_CTX_PTR ctx), (ctx)) \ DEFINEFUNC_LEGACY_1_0(void*, EVP_PKEY_get0, (GO_EVP_PKEY_PTR pkey), (pkey)) \ DEFINEFUNC_LEGACY_1_1(GO_EC_KEY_PTR, EVP_PKEY_get0_EC_KEY, (GO_EVP_PKEY_PTR pkey), (pkey)) \ DEFINEFUNC_LEGACY_1_1(GO_DSA_PTR, EVP_PKEY_get0_DSA, (GO_EVP_PKEY_PTR pkey), (pkey)) \ From 5a8ed0c52c717de396884583022ed4520689d6c8 Mon Sep 17 00:00:00 2001 From: qmuntal Date: Mon, 25 Nov 2024 21:01:50 +0100 Subject: [PATCH 06/12] move key validation into newEvpFromParams --- dsa.go | 2 +- ecdh.go | 22 +++++++--------------- ecdsa.go | 2 +- evp.go | 15 ++++++++++++++- rsa.go | 2 +- shims.h | 4 ++-- 6 files changed, 26 insertions(+), 21 deletions(-) diff --git a/dsa.go b/dsa.go index c56071f5..72cbb7a4 100644 --- a/dsa.go +++ b/dsa.go @@ -280,7 +280,7 @@ func newDSA3(params DSAParameters, x, y BigInt) (C.GO_EVP_PKEY_PTR, error) { return nil, err } defer C.go_openssl_OSSL_PARAM_free(bldparams) - pkey, err := newEvpFromParams(C.GO_EVP_PKEY_DSA, selection, bldparams) + pkey, err := newEvpFromParams(C.GO_EVP_PKEY_DSA, selection, bldparams, false) if err != nil { return nil, err } diff --git a/ecdh.go b/ecdh.go index 1cf02318..495529c7 100644 --- a/ecdh.go +++ b/ecdh.go @@ -165,7 +165,12 @@ func newECDHPkey1(nid C.int, bytes []byte, isPrivate bool) (pkey C.GO_EVP_PKEY_P } } if C.go_openssl_EC_KEY_check_key(key) != 1 { - return nil, newOpenSSLError("EC_KEY_check_key") + // Match upstream error message. + if isPrivate { + return nil, errors.New("crypto/ecdh: invalid private key") + } else { + return nil, errors.New("crypto/ecdh: invalid public key") + } } return newEVPPKEY(key) } @@ -205,20 +210,7 @@ func newECDHPkey3(nid C.int, bytes []byte, isPrivate bool) (C.GO_EVP_PKEY_PTR, e return nil, err } defer C.go_openssl_OSSL_PARAM_free(params) - pkey, err := newEvpFromParams(C.GO_EVP_PKEY_EC, selection, params) - if err != nil { - return nil, err - } - if isPrivate { - if C.go_openssl_EVP_PKEY_private_check(pkey) != 1 { - return nil, errors.New("crypto/ecdh: invalid private key") - } - } else { - if C.go_openssl_EVP_PKEY_public_check_quick(pkey) != 1 { - return nil, errors.New("crypto/ecdh: invalid public key") - } - } - return pkey, nil + return newEvpFromParams(C.GO_EVP_PKEY_EC, selection, params, true) } func pointMult(group C.GO_EC_GROUP_PTR, priv C.GO_BIGNUM_PTR) (C.GO_EC_POINT_PTR, error) { diff --git a/ecdsa.go b/ecdsa.go index f85782a6..16b360a1 100644 --- a/ecdsa.go +++ b/ecdsa.go @@ -207,5 +207,5 @@ func newECDSAKey3(nid C.int, bx, by, bd C.GO_BIGNUM_PTR) (C.GO_EVP_PKEY_PTR, err return nil, err } defer C.go_openssl_OSSL_PARAM_free(params) - return newEvpFromParams(C.GO_EVP_PKEY_EC, selection, params) + return newEvpFromParams(C.GO_EVP_PKEY_EC, selection, params, false) } diff --git a/evp.go b/evp.go index 91296a93..90900d2d 100644 --- a/evp.go +++ b/evp.go @@ -502,7 +502,7 @@ func getECKey(pkey C.GO_EVP_PKEY_PTR) (key C.GO_EC_KEY_PTR) { return key } -func newEvpFromParams(id C.int, selection C.int, params C.GO_OSSL_PARAM_PTR) (C.GO_EVP_PKEY_PTR, error) { +func newEvpFromParams(id C.int, selection C.int, params C.GO_OSSL_PARAM_PTR, validate bool) (C.GO_EVP_PKEY_PTR, error) { ctx := C.go_openssl_EVP_PKEY_CTX_new_id(id, nil) if ctx == nil { return nil, newOpenSSLError("EVP_PKEY_CTX_new_id") @@ -515,5 +515,18 @@ func newEvpFromParams(id C.int, selection C.int, params C.GO_OSSL_PARAM_PTR) (C. if C.go_openssl_EVP_PKEY_fromdata(ctx, &pkey, selection, params) != 1 { return nil, newOpenSSLError("EVP_PKEY_fromdata") } + if validate { + if selection == C.GO_EVP_PKEY_KEYPAIR { // Private key + if C.go_openssl_EVP_PKEY_private_check(ctx) != 1 { + // Match upstream error message. + return nil, errors.New("crypto/ecdh: invalid private key") + } + } else { // Public key + if C.go_openssl_EVP_PKEY_public_check_quick(ctx) != 1 { + // Match upstream error message. + return nil, errors.New("crypto/ecdh: invalid public key") + } + } + } return pkey, nil } diff --git a/rsa.go b/rsa.go index da5c7636..7e64bec0 100644 --- a/rsa.go +++ b/rsa.go @@ -404,5 +404,5 @@ func newRSAKey3(isPriv bool, n, e, d, p, q, dp, dq, qinv BigInt) (C.GO_EVP_PKEY_ if isPriv { selection = C.GO_EVP_PKEY_KEYPAIR } - return newEvpFromParams(C.GO_EVP_PKEY_RSA, C.int(selection), params) + return newEvpFromParams(C.GO_EVP_PKEY_RSA, C.int(selection), params, false) } diff --git a/shims.h b/shims.h index e4381754..d16759d6 100644 --- a/shims.h +++ b/shims.h @@ -310,8 +310,8 @@ DEFINEFUNC(int, EVP_PKEY_sign, (GO_EVP_PKEY_CTX_PTR arg0, unsigned char *arg1, s DEFINEFUNC(int, EVP_PKEY_derive_init, (GO_EVP_PKEY_CTX_PTR ctx), (ctx)) \ DEFINEFUNC(int, EVP_PKEY_derive_set_peer, (GO_EVP_PKEY_CTX_PTR ctx, GO_EVP_PKEY_PTR peer), (ctx, peer)) \ DEFINEFUNC(int, EVP_PKEY_derive, (GO_EVP_PKEY_CTX_PTR ctx, unsigned char *key, size_t *keylen), (ctx, key, keylen)) \ -DEFINEFUNC(int, EVP_PKEY_public_check_quick, (GO_EVP_PKEY_CTX_PTR ctx), (ctx)) \ -DEFINEFUNC(int, EVP_PKEY_private_check, (GO_EVP_PKEY_CTX_PTR ctx), (ctx)) \ +DEFINEFUNC_3_0(int, EVP_PKEY_public_check_quick, (GO_EVP_PKEY_CTX_PTR ctx), (ctx)) \ +DEFINEFUNC_3_0(int, EVP_PKEY_private_check, (GO_EVP_PKEY_CTX_PTR ctx), (ctx)) \ DEFINEFUNC_LEGACY_1_0(void*, EVP_PKEY_get0, (GO_EVP_PKEY_PTR pkey), (pkey)) \ DEFINEFUNC_LEGACY_1_1(GO_EC_KEY_PTR, EVP_PKEY_get0_EC_KEY, (GO_EVP_PKEY_PTR pkey), (pkey)) \ DEFINEFUNC_LEGACY_1_1(GO_DSA_PTR, EVP_PKEY_get0_DSA, (GO_EVP_PKEY_PTR pkey), (pkey)) \ From 2644a7e91ca83716441c7c2c9baa69f30b900651 Mon Sep 17 00:00:00 2001 From: qmuntal Date: Tue, 26 Nov 2024 09:15:17 +0100 Subject: [PATCH 07/12] fix openssl 3 --- dsa.go | 2 +- ecdh.go | 23 ++++++++++++++++++++++- ecdsa.go | 2 +- evp.go | 15 +-------------- rsa.go | 2 +- 5 files changed, 26 insertions(+), 18 deletions(-) diff --git a/dsa.go b/dsa.go index 72cbb7a4..c56071f5 100644 --- a/dsa.go +++ b/dsa.go @@ -280,7 +280,7 @@ func newDSA3(params DSAParameters, x, y BigInt) (C.GO_EVP_PKEY_PTR, error) { return nil, err } defer C.go_openssl_OSSL_PARAM_free(bldparams) - pkey, err := newEvpFromParams(C.GO_EVP_PKEY_DSA, selection, bldparams, false) + pkey, err := newEvpFromParams(C.GO_EVP_PKEY_DSA, selection, bldparams) if err != nil { return nil, err } diff --git a/ecdh.go b/ecdh.go index 495529c7..9f2b57bb 100644 --- a/ecdh.go +++ b/ecdh.go @@ -210,7 +210,28 @@ func newECDHPkey3(nid C.int, bytes []byte, isPrivate bool) (C.GO_EVP_PKEY_PTR, e return nil, err } defer C.go_openssl_OSSL_PARAM_free(params) - return newEvpFromParams(C.GO_EVP_PKEY_EC, selection, params, true) + pkey, err := newEvpFromParams(C.GO_EVP_PKEY_EC, selection, params) + if err != nil { + return nil, err + } + ctx := C.go_openssl_EVP_PKEY_CTX_new(pkey, nil) + if ctx == nil { + return nil, newOpenSSLError("EVP_PKEY_CTX_new") + } + defer C.go_openssl_EVP_PKEY_CTX_free(ctx) + if isPrivate { + if C.go_openssl_EVP_PKEY_private_check(ctx) != 1 { + // Match upstream error message. + return nil, errors.New("crypto/ecdh: invalid private key") + } + } else { + // Upstream Go does a partial check here, so do we. + if C.go_openssl_EVP_PKEY_public_check_quick(ctx) != 1 { + // Match upstream error message. + return nil, errors.New("crypto/ecdh: invalid public key") + } + } + return pkey, nil } func pointMult(group C.GO_EC_GROUP_PTR, priv C.GO_BIGNUM_PTR) (C.GO_EC_POINT_PTR, error) { diff --git a/ecdsa.go b/ecdsa.go index 16b360a1..f85782a6 100644 --- a/ecdsa.go +++ b/ecdsa.go @@ -207,5 +207,5 @@ func newECDSAKey3(nid C.int, bx, by, bd C.GO_BIGNUM_PTR) (C.GO_EVP_PKEY_PTR, err return nil, err } defer C.go_openssl_OSSL_PARAM_free(params) - return newEvpFromParams(C.GO_EVP_PKEY_EC, selection, params, false) + return newEvpFromParams(C.GO_EVP_PKEY_EC, selection, params) } diff --git a/evp.go b/evp.go index 90900d2d..91296a93 100644 --- a/evp.go +++ b/evp.go @@ -502,7 +502,7 @@ func getECKey(pkey C.GO_EVP_PKEY_PTR) (key C.GO_EC_KEY_PTR) { return key } -func newEvpFromParams(id C.int, selection C.int, params C.GO_OSSL_PARAM_PTR, validate bool) (C.GO_EVP_PKEY_PTR, error) { +func newEvpFromParams(id C.int, selection C.int, params C.GO_OSSL_PARAM_PTR) (C.GO_EVP_PKEY_PTR, error) { ctx := C.go_openssl_EVP_PKEY_CTX_new_id(id, nil) if ctx == nil { return nil, newOpenSSLError("EVP_PKEY_CTX_new_id") @@ -515,18 +515,5 @@ func newEvpFromParams(id C.int, selection C.int, params C.GO_OSSL_PARAM_PTR, val if C.go_openssl_EVP_PKEY_fromdata(ctx, &pkey, selection, params) != 1 { return nil, newOpenSSLError("EVP_PKEY_fromdata") } - if validate { - if selection == C.GO_EVP_PKEY_KEYPAIR { // Private key - if C.go_openssl_EVP_PKEY_private_check(ctx) != 1 { - // Match upstream error message. - return nil, errors.New("crypto/ecdh: invalid private key") - } - } else { // Public key - if C.go_openssl_EVP_PKEY_public_check_quick(ctx) != 1 { - // Match upstream error message. - return nil, errors.New("crypto/ecdh: invalid public key") - } - } - } return pkey, nil } diff --git a/rsa.go b/rsa.go index 7e64bec0..da5c7636 100644 --- a/rsa.go +++ b/rsa.go @@ -404,5 +404,5 @@ func newRSAKey3(isPriv bool, n, e, d, p, q, dp, dq, qinv BigInt) (C.GO_EVP_PKEY_ if isPriv { selection = C.GO_EVP_PKEY_KEYPAIR } - return newEvpFromParams(C.GO_EVP_PKEY_RSA, C.int(selection), params, false) + return newEvpFromParams(C.GO_EVP_PKEY_RSA, C.int(selection), params) } From 8f91a67c88cb786e8cb6e293a0f4a13cc454e904 Mon Sep 17 00:00:00 2001 From: qmuntal Date: Tue, 26 Nov 2024 09:22:32 +0100 Subject: [PATCH 08/12] fix memory leak --- ecdh.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ecdh.go b/ecdh.go index 9f2b57bb..0ea8fb59 100644 --- a/ecdh.go +++ b/ecdh.go @@ -221,12 +221,14 @@ func newECDHPkey3(nid C.int, bytes []byte, isPrivate bool) (C.GO_EVP_PKEY_PTR, e defer C.go_openssl_EVP_PKEY_CTX_free(ctx) if isPrivate { if C.go_openssl_EVP_PKEY_private_check(ctx) != 1 { + C.go_openssl_EVP_PKEY_free(pkey) // Match upstream error message. return nil, errors.New("crypto/ecdh: invalid private key") } } else { // Upstream Go does a partial check here, so do we. if C.go_openssl_EVP_PKEY_public_check_quick(ctx) != 1 { + C.go_openssl_EVP_PKEY_free(pkey) // Match upstream error message. return nil, errors.New("crypto/ecdh: invalid public key") } From d2d0288f7287928f62c5cf940ab0c0e3d4895de9 Mon Sep 17 00:00:00 2001 From: qmuntal Date: Tue, 26 Nov 2024 09:48:54 +0100 Subject: [PATCH 09/12] fix memory leak --- ec.go | 39 +++++++++++++++++++++++++++++++-------- ecdh.go | 27 +++++---------------------- ecdsa.go | 5 +---- evp.go | 5 +---- 4 files changed, 38 insertions(+), 38 deletions(-) diff --git a/ec.go b/ec.go index ccaf5a4c..efc004e0 100644 --- a/ec.go +++ b/ec.go @@ -4,26 +4,26 @@ package openssl // #include "goopenssl.h" import "C" +import "errors" -func curveNID(curve string) (C.int, error) { +func curveNID(curve string) C.int { switch curve { case "P-224": - return C.GO_NID_secp224r1, nil + return C.GO_NID_secp224r1 case "P-256": - return C.GO_NID_X9_62_prime256v1, nil + return C.GO_NID_X9_62_prime256v1 case "P-384": - return C.GO_NID_secp384r1, nil + return C.GO_NID_secp384r1 case "P-521": - return C.GO_NID_secp521r1, nil + return C.GO_NID_secp521r1 + default: + panic("openssl: unknown curve " + curve) } - return 0, errUnknownCurve } // curveSize returns the size of the curve in bytes. func curveSize(curve string) int { switch curve { - default: - panic("openssl: unknown curve " + curve) case "P-224": return 224 / 8 case "P-256": @@ -32,6 +32,8 @@ func curveSize(curve string) int { return 384 / 8 case "P-521": return (521 + 7) / 8 + default: + panic("openssl: unknown curve " + curve) } } @@ -65,3 +67,24 @@ func generateAndEncodeEcPublicKey(nid C.int, newPubKeyPointFn func(group C.GO_EC defer C.go_openssl_EC_POINT_free(pt) return encodeEcPoint(group, pt) } + +func checkPkey(pkey C.GO_EVP_PKEY_PTR, isPrivate bool) error { + ctx := C.go_openssl_EVP_PKEY_CTX_new(pkey, nil) + if ctx == nil { + return newOpenSSLError("EVP_PKEY_CTX_new") + } + defer C.go_openssl_EVP_PKEY_CTX_free(ctx) + if isPrivate { + if C.go_openssl_EVP_PKEY_private_check(ctx) != 1 { + // Match upstream error message. + return errors.New("invalid private key") + } + } else { + // Upstream Go does a partial check here, so do we. + if C.go_openssl_EVP_PKEY_public_check_quick(ctx) != 1 { + // Match upstream error message. + return errors.New("invalid public key") + } + } + return nil +} diff --git a/ecdh.go b/ecdh.go index 0ea8fb59..d8c72f62 100644 --- a/ecdh.go +++ b/ecdh.go @@ -107,10 +107,7 @@ func (k *PrivateKeyECDH) PublicKey() (*PublicKeyECDH, error) { } func newECDHPkey(curve string, bytes []byte, isPrivate bool) (C.GO_EVP_PKEY_PTR, error) { - nid, err := curveNID(curve) - if err != nil { - return nil, err - } + nid := curveNID(curve) switch vMajor { case 1: return newECDHPkey1(nid, bytes, isPrivate) @@ -214,24 +211,10 @@ func newECDHPkey3(nid C.int, bytes []byte, isPrivate bool) (C.GO_EVP_PKEY_PTR, e if err != nil { return nil, err } - ctx := C.go_openssl_EVP_PKEY_CTX_new(pkey, nil) - if ctx == nil { - return nil, newOpenSSLError("EVP_PKEY_CTX_new") - } - defer C.go_openssl_EVP_PKEY_CTX_free(ctx) - if isPrivate { - if C.go_openssl_EVP_PKEY_private_check(ctx) != 1 { - C.go_openssl_EVP_PKEY_free(pkey) - // Match upstream error message. - return nil, errors.New("crypto/ecdh: invalid private key") - } - } else { - // Upstream Go does a partial check here, so do we. - if C.go_openssl_EVP_PKEY_public_check_quick(ctx) != 1 { - C.go_openssl_EVP_PKEY_free(pkey) - // Match upstream error message. - return nil, errors.New("crypto/ecdh: invalid public key") - } + + if err := checkPkey(pkey, isPrivate); err != nil { + C.go_openssl_EVP_PKEY_free(pkey) + return nil, errors.New("crypto/ecdh: " + err.Error()) } return pkey, nil } diff --git a/ecdsa.go b/ecdsa.go index f85782a6..bc5f1117 100644 --- a/ecdsa.go +++ b/ecdsa.go @@ -122,10 +122,7 @@ func HashVerifyECDSA(pub *PublicKeyECDSA, h crypto.Hash, msg, sig []byte) bool { } func newECDSAKey(curve string, x, y, d BigInt) (C.GO_EVP_PKEY_PTR, error) { - nid, err := curveNID(curve) - if err != nil { - return nil, err - } + nid := curveNID(curve) var bx, by, bd C.GO_BIGNUM_PTR defer func() { C.go_openssl_BN_free(bx) diff --git a/evp.go b/evp.go index 91296a93..17d040a4 100644 --- a/evp.go +++ b/evp.go @@ -175,10 +175,7 @@ func generateEVPPKey(id C.int, bits int, curve string) (C.GO_EVP_PKEY_PTR, error } } if curve != "" { - nid, err := curveNID(curve) - if err != nil { - return nil, err - } + nid := curveNID(curve) if C.go_openssl_EVP_PKEY_CTX_ctrl(ctx, id, -1, C.GO_EVP_PKEY_CTRL_EC_PARAMGEN_CURVE_NID, nid, nil) != 1 { return nil, newOpenSSLError("EVP_PKEY_CTX_ctrl failed") } From 2035279b2dff086228a1332f4a2459b191d95f6e Mon Sep 17 00:00:00 2001 From: qmuntal Date: Tue, 26 Nov 2024 11:08:29 +0100 Subject: [PATCH 10/12] fix memory leak --- ec.go | 22 ---------------------- ecdh.go | 3 ++- evp.go | 27 +++++++++++++++++++++++++++ 3 files changed, 29 insertions(+), 23 deletions(-) diff --git a/ec.go b/ec.go index efc004e0..734c14b9 100644 --- a/ec.go +++ b/ec.go @@ -4,7 +4,6 @@ package openssl // #include "goopenssl.h" import "C" -import "errors" func curveNID(curve string) C.int { switch curve { @@ -67,24 +66,3 @@ func generateAndEncodeEcPublicKey(nid C.int, newPubKeyPointFn func(group C.GO_EC defer C.go_openssl_EC_POINT_free(pt) return encodeEcPoint(group, pt) } - -func checkPkey(pkey C.GO_EVP_PKEY_PTR, isPrivate bool) error { - ctx := C.go_openssl_EVP_PKEY_CTX_new(pkey, nil) - if ctx == nil { - return newOpenSSLError("EVP_PKEY_CTX_new") - } - defer C.go_openssl_EVP_PKEY_CTX_free(ctx) - if isPrivate { - if C.go_openssl_EVP_PKEY_private_check(ctx) != 1 { - // Match upstream error message. - return errors.New("invalid private key") - } - } else { - // Upstream Go does a partial check here, so do we. - if C.go_openssl_EVP_PKEY_public_check_quick(ctx) != 1 { - // Match upstream error message. - return errors.New("invalid public key") - } - } - return nil -} diff --git a/ecdh.go b/ecdh.go index d8c72f62..ad392dca 100644 --- a/ecdh.go +++ b/ecdh.go @@ -7,6 +7,7 @@ import "C" import ( "errors" "runtime" + "slices" "unsafe" ) @@ -36,7 +37,7 @@ func NewPublicKeyECDH(curve string, bytes []byte) (*PublicKeyECDH, error) { if err != nil { return nil, err } - k := &PublicKeyECDH{pkey, append([]byte(nil), bytes...)} + k := &PublicKeyECDH{pkey, slices.Clone(bytes)} runtime.SetFinalizer(k, (*PublicKeyECDH).finalize) return k, nil } diff --git a/evp.go b/evp.go index 17d040a4..e7ccdee7 100644 --- a/evp.go +++ b/evp.go @@ -510,7 +510,34 @@ func newEvpFromParams(id C.int, selection C.int, params C.GO_OSSL_PARAM_PTR) (C. } var pkey C.GO_EVP_PKEY_PTR if C.go_openssl_EVP_PKEY_fromdata(ctx, &pkey, selection, params) != 1 { + if vMinor < 2 { + // OpenSSL 3.0.1 and 3.0.2 have a bug where EVP_PKEY_fromdata + // does not free the internally allocated EVP_PKEY on error. + // See https://github.com/openssl/openssl/issues/17407. + C.go_openssl_EVP_PKEY_free(pkey) + } return nil, newOpenSSLError("EVP_PKEY_fromdata") } return pkey, nil } + +func checkPkey(pkey C.GO_EVP_PKEY_PTR, isPrivate bool) error { + ctx := C.go_openssl_EVP_PKEY_CTX_new(pkey, nil) + if ctx == nil { + return newOpenSSLError("EVP_PKEY_CTX_new") + } + defer C.go_openssl_EVP_PKEY_CTX_free(ctx) + if isPrivate { + if C.go_openssl_EVP_PKEY_private_check(ctx) != 1 { + // Match upstream error message. + return errors.New("invalid private key") + } + } else { + // Upstream Go does a partial check here, so do we. + if C.go_openssl_EVP_PKEY_public_check_quick(ctx) != 1 { + // Match upstream error message. + return errors.New("invalid public key") + } + } + return nil +} From 6744ee34a3865c6eaab0105923adbb5bd22e730f Mon Sep 17 00:00:00 2001 From: qmuntal Date: Tue, 26 Nov 2024 11:41:24 +0100 Subject: [PATCH 11/12] remove unnecessary test check --- ecdh_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/ecdh_test.go b/ecdh_test.go index 4b2c6bd3..da1e37f3 100644 --- a/ecdh_test.go +++ b/ecdh_test.go @@ -273,8 +273,6 @@ func TestECDHNewPrivateKeyECDH_Invalid(t *testing.T) { t.Errorf("unexpectedly accepted %q", input) } else if k != nil { t.Error("PrivateKey was not nil on error") - } else if strings.Contains(err.Error(), "boringcrypto") { - t.Errorf("boringcrypto error leaked out: %v", err) } } }) @@ -290,8 +288,6 @@ func TestECDHNewPublicKeyECDH_Invalid(t *testing.T) { t.Errorf("unexpectedly accepted %q", input) } else if k != nil { t.Error("PublicKey was not nil on error") - } else if strings.Contains(err.Error(), "boringcrypto") { - t.Errorf("boringcrypto error leaked out: %v", err) } } }) From 4a82bb4e8ea748a792d444be0447bc39134bc291 Mon Sep 17 00:00:00 2001 From: Quim Muntal Date: Thu, 28 Nov 2024 09:37:43 +0100 Subject: [PATCH 12/12] Update evp.go Co-authored-by: Davis Goodin --- evp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evp.go b/evp.go index e7ccdee7..85c84b8b 100644 --- a/evp.go +++ b/evp.go @@ -510,7 +510,7 @@ func newEvpFromParams(id C.int, selection C.int, params C.GO_OSSL_PARAM_PTR) (C. } var pkey C.GO_EVP_PKEY_PTR if C.go_openssl_EVP_PKEY_fromdata(ctx, &pkey, selection, params) != 1 { - if vMinor < 2 { + if vMajor == 3 && vMinor <= 2 { // OpenSSL 3.0.1 and 3.0.2 have a bug where EVP_PKEY_fromdata // does not free the internally allocated EVP_PKEY on error. // See https://github.com/openssl/openssl/issues/17407.