Skip to content

Follow-ups to the certificate searcher: bounds-checked property reads, store handle leaks #1091

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.

Sign up for GitHub

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

Merged
dopey merged 6 commits into from
Jul 29, 2026
Merged

Follow-ups to the certificate searcher: bounds-checked property reads, store handle leaks #1091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
422f48b
capi: read the key container name out of the property buffer
darkfronza Jul 28, 2026
60c5bf6
capi: report certificates with unreadable key metadata
darkfronza Jul 28, 2026
f5ef4fd
capi: cover the key container name reader on Windows
darkfronza Jul 28, 2026
d68b10f
capi: close certificate store handles
darkfronza Jul 28, 2026
ddbf37c
capi: centralize the CRYPT_E_NOT_FOUND check, drop a dead branch
darkfronza Jul 28, 2026
2961412
platform: reject a nil SearchCertificates request
darkfronza Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion kms/apiv1/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ type SearchableKeyManager interface {
// returned response may still be non-nil and hold the certificates
// enumerated before the failure, together with a non-nil error describing
// what went wrong. Callers wanting those partial results must check the
// response before (or regardless of) the error.
// response before (or regardless of) the error. A failure to read one
// certificate's key metadata is reported on that result's Err field rather
// than failing the search or dropping the certificate.
//
// # Experimental
//
Expand Down
8 changes: 8 additions & 0 deletions kms/apiv1/requests.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,14 @@ type SearchCertificatesRequest struct {
type SearchCertificateResult struct {
Certificate *x509.Certificate
KeyContainerName string

// Err is non-nil when the certificate's key-provider metadata could not
// be read. Certificate is still valid and KeyContainerName is empty. It
// distinguishes a certificate with no private-key association at all
// (Err == nil, KeyContainerName == "") from one whose association exists
// but cannot be interpreted — a certificate a caller sweeping for broken
// credentials most likely wants to see rather than have hidden.
Err error
}

// SearchCertificatesResponse is the response of a SearchCertificates call.
Expand Down
196 changes: 78 additions & 118 deletions kms/capi/capi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"io"
"math/big"
"net/url"
"runtime"
"strconv"
"strings"
"time"
Expand Down Expand Up @@ -405,35 +406,64 @@ func (k *CAPIKMS) Close() error {
return nil
}

// getCertContext returns a pointer to a X.509 certificate context based on the provided URI
// callers are responsible for freeing the context
func (k *CAPIKMS) getCertContext(u *uriAttributes) (*windows.CertContext, error) {
// The hash argument is a SHA-1
if len(u.hash) > 0 && len(u.hash) != 20 {
return nil, fmt.Errorf("decoded %s has length %d; expected 20 bytes for SHA-1", HashArg, len(u.hash))
}

// openCertStore opens the system certificate store selected by u. The caller
// owns the returned handle and must close it with windows.CertCloseStore.
//
// Closing it with no flags only drops a reference: a certificate context
// obtained from a store holds its own reference, so the context stays valid
// after the store is closed and the store is released once the last context is
// freed. Callers may therefore close with a defer even when they hand a
// context back to their own caller. Passing CERT_CLOSE_STORE_FORCE_FLAG would
// not be safe there, as it closes the store regardless of outstanding
// contexts.
func openCertStore(u *uriAttributes) (windows.Handle, error) {
var certStoreLocation uint32
switch u.storeLocation {
case UserStoreLocation:
certStoreLocation = certStoreCurrentUser
case MachineStoreLocation:
certStoreLocation = certStoreLocalMachine
default:
return nil, fmt.Errorf("invalid cert store location %q", u.storeLocation)
return 0, fmt.Errorf("invalid cert store location %q", u.storeLocation)
}

// CertOpenStore takes the store name as a uintptr, so the conversion below
// leaves no Go pointer referring to the string: the garbage collector is
// free to reclaim it while the call is in flight. Hold it in a variable and
// keep it alive across the call. (The compiler's special case that makes
// this safe applies only to a conversion written inside a call to
// syscall.Syscall itself, which this is not.)
storeName := wide(u.storeName)
st, err := windows.CertOpenStore(
certStoreProvSystem,
0,
0,
certStoreLocation,
uintptr(unsafe.Pointer(wide(u.storeName))),
uintptr(unsafe.Pointer(storeName)),
)
runtime.KeepAlive(storeName)
if err != nil {
return nil, fmt.Errorf("CertOpenStore for the %q store %q returned: %w", u.storeLocation, u.storeName, err)
return 0, fmt.Errorf("CertOpenStore for the %q store %q returned: %w", u.storeLocation, u.storeName, err)
}

return st, nil
}

// getCertContext returns a pointer to a X.509 certificate context based on the provided URI
// callers are responsible for freeing the context
func (k *CAPIKMS) getCertContext(u *uriAttributes) (*windows.CertContext, error) {
// The hash argument is a SHA-1
if len(u.hash) > 0 && len(u.hash) != 20 {
return nil, fmt.Errorf("decoded %s has length %d; expected 20 bytes for SHA-1", HashArg, len(u.hash))
}

st, err := openCertStore(u)
if err != nil {
return nil, err
}
// Safe to close while returning a context: see openCertStore.
defer func() { _ = windows.CertCloseStore(st, 0) }()

// if issuer + any of the other fields in the list below is provided, then attempt a second certificate lookup when
// lookup by KeyID fails (not found).
canLookupByIssuer := u.issuerName != "" && (u.serialNumber != nil || u.subjectCN != "" || u.friendlyName != "" || u.description != "")
Expand Down Expand Up @@ -865,26 +895,11 @@ func (k *CAPIKMS) FindCertificatesByIssuer(req *apiv1.LoadCertificateRequest, ra
return nil, fmt.Errorf("%q is required", IssuerNameArg)
}

var certStoreLocation uint32
switch u.storeLocation {
case UserStoreLocation:
certStoreLocation = certStoreCurrentUser
case MachineStoreLocation:
certStoreLocation = certStoreLocalMachine
default:
return nil, fmt.Errorf("invalid cert store location %q", u.storeLocation)
}

st, err := windows.CertOpenStore(
certStoreProvSystem,
0,
0,
certStoreLocation,
uintptr(unsafe.Pointer(wide(u.storeName))),
)
st, err := openCertStore(u)
if err != nil {
return nil, fmt.Errorf("CertOpenStore for the %q store %q returned: %w", u.storeLocation, u.storeName, err)
return nil, err
}
defer func() { _ = windows.CertCloseStore(st, 0) }()

var (
certs []*x509.Certificate
Expand Down Expand Up @@ -926,8 +941,10 @@ func (k *CAPIKMS) FindCertificatesByIssuer(req *apiv1.LoadCertificateRequest, ra
// certificate, reads the CNG/CAPI key container recorded in its
// CERT_KEY_PROV_INFO property. It never opens a key handle, so a certificate
// whose associated key no longer exists is still returned, with its recorded
// container name; a certificate is skipped when its properties cannot be read
// at all, or when its DER encoding cannot be parsed.
// container name. A certificate whose key-provider metadata cannot be read is
// returned too, with the failure recorded on the result's Err field; only a
// certificate whose DER encoding cannot be parsed is skipped, there being no
// certificate to report.
func (k *CAPIKMS) SearchCertificates(req *apiv1.SearchCertificatesRequest) (*apiv1.SearchCertificatesResponse, error) {
if req == nil {
return nil, errors.New("searchCertificatesRequest cannot be nil")
Expand All @@ -941,29 +958,10 @@ func (k *CAPIKMS) SearchCertificates(req *apiv1.SearchCertificatesRequest) (*api
return nil, err
}

var certStoreLocation uint32
switch u.storeLocation {
case UserStoreLocation:
certStoreLocation = certStoreCurrentUser
case MachineStoreLocation:
certStoreLocation = certStoreLocalMachine
default:
return nil, fmt.Errorf("invalid cert store location %q", u.storeLocation)
}

st, err := windows.CertOpenStore(
certStoreProvSystem,
0,
0,
certStoreLocation,
uintptr(unsafe.Pointer(wide(u.storeName))),
)
st, err := openCertStore(u)
if err != nil {
return nil, fmt.Errorf("CertOpenStore for the %q store %q returned: %w", u.storeLocation, u.storeName, err)
return nil, err
}
// Unlike the other store-opening call sites in this file, SearchCertificates
// runs on every certificate issuance in long-running callers, so an
// unclosed store handle here is an unbounded leak rather than a one-off.
defer func() { _ = windows.CertCloseStore(st, 0) }()

var (
Expand All @@ -972,36 +970,36 @@ func (k *CAPIKMS) SearchCertificates(req *apiv1.SearchCertificatesRequest) (*api
enumErr error
)
for {
// CertEnumCertificatesInStore returns a nil context, and so a non-nil
// error, both at the end of the store and on failure; either way it has
// already freed prevCert.
certHandle, err := windows.CertEnumCertificatesInStore(st, prevCert)
if err != nil {
if errno, ok := err.(windows.Errno); ok && uint32(errno) == CRYPT_E_NOT_FOUND {
// End of store; prevCert was freed by this call per the Windows API contract.
break
if isNotFound(err) {
break // end of store
}
// A real enumeration failure: prevCert was still freed by this call, but
// the store may hold unvisited certificates, so what we have is partial.
// A real enumeration failure: the store may hold unvisited
// certificates, so what we have is partial.
enumErr = fmt.Errorf("CertEnumCertificatesInStore failed: %w", err)
break
}
if certHandle == nil {
// prevCert was freed by this call per the Windows API contract.
break
}
prevCert = certHandle // freed on next CertEnumCertificatesInStore call

x509Cert, err := certContextToX509(certHandle)
if err != nil {
continue
}

// A certificate with no key association at all reads back as ("", nil),
// so an error here means the association exists but could not be
// interpreted — precisely the kind of broken credential a caller
// sweeping the store is looking for. Report it on the result instead of
// dropping the certificate.
containerName, err := cryptFindCertificateKeyContainerName(certHandle)
if err != nil {
continue
}

results = append(results, apiv1.SearchCertificateResult{
Certificate: x509Cert,
KeyContainerName: containerName,
Err: err,
})
}

Expand All @@ -1014,15 +1012,14 @@ func (k *CAPIKMS) StoreCertificate(req *apiv1.StoreCertificateRequest) error {
return err
}

var certStoreLocation uint32
switch u.storeLocation {
case UserStoreLocation:
certStoreLocation = certStoreCurrentUser
case MachineStoreLocation:
certStoreLocation = certStoreLocalMachine
default:
return fmt.Errorf("invalid cert store location %q", u.storeLocation)
// Opened before the certificate context is built so an unusable store
// location fails before any key association work, which may prompt for a
// smart card.
st, err := openCertStore(u)
if err != nil {
return err
}
defer func() { _ = windows.CertCloseStore(st, 0) }()

certContext, err := windows.CertCreateCertificateContext(
encodingX509ASN|encodingPKCS7,
Expand Down Expand Up @@ -1077,16 +1074,6 @@ func (k *CAPIKMS) StoreCertificate(req *apiv1.StoreCertificateRequest) error {
cryptSetCertificateDescription(certContext, u.description)
}

st, err := windows.CertOpenStore(
certStoreProvSystem,
0,
0,
certStoreLocation,
uintptr(unsafe.Pointer(wide(u.storeName))))
if err != nil {
return fmt.Errorf("CertOpenStore for the %q store %q returned: %w", u.storeLocation, u.storeName, err)
}

// Add the cert context to the system certificate store
if err = windows.CertAddCertificateContextToStore(st, certContext, windows.CERT_STORE_ADD_ALWAYS, nil); err != nil {
return fmt.Errorf("CertAddCertificateContextToStore returned: %w", err)
Expand Down Expand Up @@ -1171,25 +1158,13 @@ func (k *CAPIKMS) DeleteCertificate(req *apiv1.DeleteCertificateRequest) error {
return err
}

var certStoreLocation uint32
switch u.storeLocation {
case UserStoreLocation:
certStoreLocation = certStoreCurrentUser
case MachineStoreLocation:
certStoreLocation = certStoreLocalMachine
default:
return fmt.Errorf("invalid cert store location %q", u.storeLocation)
}

st, err := windows.CertOpenStore(
certStoreProvSystem,
0,
0,
certStoreLocation,
uintptr(unsafe.Pointer(wide(u.storeName))))
st, err := openCertStore(u)
if err != nil {
return fmt.Errorf("CertOpenStore for the %q store %q returned: %w", u.storeLocation, u.storeName, err)
return err
}
// Safe to close while the delete paths below still hold a context: see
// openCertStore.
defer func() { _ = windows.CertCloseStore(st, 0) }()

var certHandle *windows.CertContext

Expand Down Expand Up @@ -1325,26 +1300,11 @@ func (k *CAPIKMS) CleanupCredentials(req *apiv1.CleanupCredentialsRequest) error
return fmt.Errorf("%q is required", IssuerNameArg)
}

var certStoreLocation uint32
switch u.storeLocation {
case UserStoreLocation:
certStoreLocation = certStoreCurrentUser
case MachineStoreLocation:
certStoreLocation = certStoreLocalMachine
default:
return fmt.Errorf("invalid cert store location %q", u.storeLocation)
}

st, err := windows.CertOpenStore(
certStoreProvSystem,
0,
0,
certStoreLocation,
uintptr(unsafe.Pointer(wide(u.storeName))),
)
st, err := openCertStore(u)
if err != nil {
return fmt.Errorf("CertOpenStore for the %q store %q returned: %w", u.storeLocation, u.storeName, err)
return err
}
defer func() { _ = windows.CertCloseStore(st, 0) }()

now := time.Now()
var errs []error
Expand Down
Loading