From 8953ffe8b3d3248e831c98a58997c94330ba5c7e Mon Sep 17 00:00:00 2001 From: Joshua Blum Date: Thu, 16 Jul 2026 13:09:07 -0400 Subject: [PATCH 1/2] bug fixes --- corefoundation.go | 36 +++++++++---------- go.mod | 4 +-- go.sum | 4 +-- keychain.go | 11 ++++-- ...h_ietf1024_sha256_aes128_cbc_pkcs7_test.go | 31 ++++++++++++++++ secretservice/secretservice.go | 2 +- 6 files changed, 63 insertions(+), 25 deletions(-) diff --git a/corefoundation.go b/corefoundation.go index 224f1ee..6c63ee6 100644 --- a/corefoundation.go +++ b/corefoundation.go @@ -267,7 +267,7 @@ func Convert(ref C.CFTypeRef) (interface{}, error) { } return b, nil } else if typeID == C.CFNumberGetTypeID() { - return CFNumberToInterface(C.CFNumberRef(ref)), nil + return CFNumberToInterface(C.CFNumberRef(ref)) } else if typeID == C.CFBooleanGetTypeID() { if C.CFBooleanGetValue(C.CFBooleanRef(ref)) != 0 { return true, nil @@ -300,72 +300,72 @@ func ConvertCFDictionary(d C.CFDictionaryRef) (map[interface{}]interface{}, erro // CFNumberToInterface converts the CFNumberRef to the most appropriate numeric // type. // This code is from github.com/kballard/go-osx-plist. -func CFNumberToInterface(cfNumber C.CFNumberRef) interface{} { +func CFNumberToInterface(cfNumber C.CFNumberRef) (interface{}, error) { typ := C.CFNumberGetType(cfNumber) switch typ { case C.kCFNumberSInt8Type: var sint C.SInt8 C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&sint)) //nolint - return int8(sint) + return int8(sint), nil case C.kCFNumberSInt16Type: var sint C.SInt16 C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&sint)) //nolint - return int16(sint) + return int16(sint), nil case C.kCFNumberSInt32Type: var sint C.SInt32 C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&sint)) //nolint - return int32(sint) + return int32(sint), nil case C.kCFNumberSInt64Type: var sint C.SInt64 C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&sint)) //nolint - return int64(sint) + return int64(sint), nil case C.kCFNumberFloat32Type: var float C.Float32 C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&float)) //nolint - return float32(float) + return float32(float), nil case C.kCFNumberFloat64Type: var float C.Float64 C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&float)) //nolint - return float64(float) + return float64(float), nil case C.kCFNumberCharType: var char C.char C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&char)) //nolint - return byte(char) + return byte(char), nil case C.kCFNumberShortType: var short C.short C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&short)) //nolint - return int16(short) + return int16(short), nil case C.kCFNumberIntType: var i C.int C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&i)) //nolint - return int32(i) + return int32(i), nil case C.kCFNumberLongType: var long C.long C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&long)) //nolint - return int(long) + return int(long), nil case C.kCFNumberLongLongType: // This is the only type that may actually overflow us var longlong C.longlong C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&longlong)) //nolint - return int64(longlong) + return int64(longlong), nil case C.kCFNumberFloatType: var float C.float C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&float)) //nolint - return float32(float) + return float32(float), nil case C.kCFNumberDoubleType: var double C.double C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&double)) //nolint - return float64(double) + return float64(double), nil case C.kCFNumberCFIndexType: // CFIndex is a long var index C.CFIndex C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&index)) //nolint - return int(index) + return int(index), nil case C.kCFNumberNSIntegerType: // We don't have a definition of NSInteger, but we know it's either an int or a long var nsInt C.long C.CFNumberGetValue(cfNumber, typ, unsafe.Pointer(&nsInt)) //nolint - return int(nsInt) + return int(nsInt), nil } - panic("Unknown CFNumber type") + return nil, fmt.Errorf("unknown CFNumber type: %d", typ) } diff --git a/go.mod b/go.mod index c5f616d..e6f9cb9 100644 --- a/go.mod +++ b/go.mod @@ -1,13 +1,13 @@ module github.com/keybase/go-keychain -go 1.24.0 +go 1.25.0 toolchain go1.25.5 require ( github.com/keybase/dbus v0.0.0-20220506165403-5aa21ea2c23a github.com/stretchr/testify v1.11.1 - golang.org/x/crypto v0.46.0 + golang.org/x/crypto v0.54.0 ) require ( diff --git a/go.sum b/go.sum index d9d21af..d969757 100644 --- a/go.sum +++ b/go.sum @@ -6,8 +6,8 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= -golang.org/x/crypto v0.46.0 h1:cKRW/pmt1pKAfetfu+RCEvjvZkA9RimPbh7bhFjGVBU= -golang.org/x/crypto v0.46.0/go.mod h1:Evb/oLKmMraqjZ2iQTwDwvCtJkczlDuTmdJXoZVzqU0= +golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw= +golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= diff --git a/keychain.go b/keychain.go index 9217a58..efd5ff1 100644 --- a/keychain.go +++ b/keychain.go @@ -558,8 +558,15 @@ func convertResult(d C.CFDictionaryRef) (*QueryResult, error) { case AuthenticationTypeKey: result.AuthenticationType = CFStringToString(C.CFStringRef(v)) case PortKey: - val := CFNumberToInterface(C.CFNumberRef(v)) - result.Port = val.(int32) + val, err := CFNumberToInterface(C.CFNumberRef(v)) + if err != nil { + return nil, fmt.Errorf("failed to convert port number: %w", err) + } + port, ok := val.(int32) + if !ok { + return nil, fmt.Errorf("port value has unexpected type %T, expected int32", val) + } + result.Port = port case PathKey: result.Path = CFStringToString(C.CFStringRef(v)) case AccountKey: diff --git a/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7_test.go b/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7_test.go index be18633..f1a2fb6 100644 --- a/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7_test.go +++ b/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7_test.go @@ -82,3 +82,34 @@ func TestPKCS7(t *testing.T) { _, err = unpadPKCS7([]byte{1, 2, 3, 4, 1, 1, 1, 2}, 4) require.Error(t, err) } + +// TestDecryptionErrors tests that decryption errors are properly returned +// rather than being swallowed. This validates the fix for the security issue +// where decryption failures would return (nil, nil) instead of (nil, err). +func TestDecryptionErrors(t *testing.T) { + key := []byte("YELLOW SUBMARINE") + + // Test 1: Invalid padding should return an error + invalidIV := make([]byte, 16) + invalidCiphertext := []byte{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, + 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f} // 16 bytes, invalid padding + _, err := unauthenticatedAESCBCDecrypt(invalidIV, invalidCiphertext, key) + require.Error(t, err, "decryption with invalid padding should return an error") + + // Test 2: Wrong key should produce unpadding errors + plaintext := []byte("secret message") + iv, ciphertext, err := unauthenticatedAESCBCEncrypt(plaintext, key) + require.NoError(t, err) + + wrongKey := []byte("WRONG KEY HERE!!") + _, err = unauthenticatedAESCBCDecrypt(iv, ciphertext, wrongKey) + require.Error(t, err, "decryption with wrong key should return an error") + + // Test 3: Corrupted ciphertext should error + corruptedCiphertext := make([]byte, len(ciphertext)) + copy(corruptedCiphertext, ciphertext) + // Corrupt the last block (which contains padding) + corruptedCiphertext[len(corruptedCiphertext)-1] ^= 0xFF + _, err = unauthenticatedAESCBCDecrypt(iv, corruptedCiphertext, key) + require.Error(t, err, "decryption of corrupted ciphertext should return an error") +} diff --git a/secretservice/secretservice.go b/secretservice/secretservice.go index 870b58f..b9b0904 100644 --- a/secretservice/secretservice.go +++ b/secretservice/secretservice.go @@ -282,7 +282,7 @@ func (s *SecretService) GetSecret(item dbus.ObjectPath, session Session) (secret case AuthenticationDHAES: plaintext, err := unauthenticatedAESCBCDecrypt(secret.Parameters, secret.Value, session.AESKey) if err != nil { - return nil, nil + return nil, err } secretPlaintext = plaintext default: From 2d5243797e19b6e70c47115aa784126712007f62 Mon Sep 17 00:00:00 2001 From: Joshua Blum Date: Thu, 16 Jul 2026 13:48:21 -0400 Subject: [PATCH 2/2] x --- secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7_test.go b/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7_test.go index f1a2fb6..b137d8e 100644 --- a/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7_test.go +++ b/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7_test.go @@ -91,8 +91,10 @@ func TestDecryptionErrors(t *testing.T) { // Test 1: Invalid padding should return an error invalidIV := make([]byte, 16) - invalidCiphertext := []byte{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, - 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f} // 16 bytes, invalid padding + invalidCiphertext := []byte{ + 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, + 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, + } // 16 bytes, invalid padding _, err := unauthenticatedAESCBCDecrypt(invalidIV, invalidCiphertext, key) require.Error(t, err, "decryption with invalid padding should return an error")