From d95bb386ec37af1e8c4050180a0edeff74c15463 Mon Sep 17 00:00:00 2001 From: rabbitstack Date: Fri, 16 Oct 2026 12:48:02 +0200 Subject: [PATCH 01/13] fix(signature): Prevent leaks when opening the catalog Open acquires a catalog admin context and opens the file before it can fail. When the hash is not in any catalog, CryptCatalogAdminEnumCatalogFromHash returns 0 and CryptCatalogInfoFromContext fails. That is the normal, not catalog-signed outcome, so Open returns an error and Close is never called. This can leak: - admin context - open file handle - catalog context --- pkg/util/signature/signature.go | 2 +- pkg/util/signature/types.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/util/signature/signature.go b/pkg/util/signature/signature.go index 5df5ddb8b..bfd188784 100644 --- a/pkg/util/signature/signature.go +++ b/pkg/util/signature/signature.go @@ -442,10 +442,10 @@ func (s *Signature) check() error { // maybe the signature is in the catalog? catalog := sys.NewCatalog() + defer catalog.Close() if err := catalog.Open(s.Path); err != nil { return ErrNoSignature } - defer catalog.Close() if !catalog.IsCatalogSigned() { return ErrNoSignature diff --git a/pkg/util/signature/types.go b/pkg/util/signature/types.go index a29fd8319..97f889be6 100644 --- a/pkg/util/signature/types.go +++ b/pkg/util/signature/types.go @@ -306,9 +306,9 @@ cat: } // parse catalog certificate catalog := sys.NewCatalog() + defer catalog.Close() if err := catalog.Open(s.Path); err != nil { return nil, err } - defer catalog.Close() return catalog.ParseCertificate() } From 3d1b7b75a10d94497c7452a301603a1e989b370e Mon Sep 17 00:00:00 2001 From: rabbitstack Date: Fri, 16 Oct 2026 13:54:08 +0200 Subject: [PATCH 02/13] perf(signature): Cache catalog certificates Catalog certificate parsing is uncached and reads whole .cat files and then pkcs7.Parse on it, for every module. Cache the parsed cert per catalog file, keyed by path, size and mtime, and deduplicate concurrent parses. --- pkg/sys/trust.go | 193 ++++++++++++++++++++++++++++++++++++----------- 1 file changed, 148 insertions(+), 45 deletions(-) diff --git a/pkg/sys/trust.go b/pkg/sys/trust.go index 89a8f9ce4..68a7d972c 100644 --- a/pkg/sys/trust.go +++ b/pkg/sys/trust.go @@ -21,16 +21,22 @@ package sys import ( + libytes "bytes" + "crypto/x509" "encoding/hex" + "errors" "fmt" "io" + "io/fs" "os" - "reflect" "runtime" + "strings" "sync" "time" "unsafe" + "github.com/golang/groupcache/singleflight" + "github.com/hashicorp/golang-lru/v2/expirable" "github.com/rabbitstack/fibratus/pkg/util/bytes" "github.com/rabbitstack/fibratus/pkg/util/format" "github.com/rabbitstack/fibratus/pkg/util/multierror" @@ -394,71 +400,123 @@ func (c *Cat) Verify(filename string) (SignatureStatus, error) { return trust.VerifyCatalog(c.file.Fd(), filename, c.admin, c.catalogInfo, c.hash, c.size) } -// ParseCertificate parses the catalog certificate. -func (c *Cat) ParseCertificate() (*Cert, error) { - f, err := os.Open(c.catalogInfo.CatalogFile()) +// certKey is the cache key for parsed cert per catalog file. Keyed by path, size and modification time. +type certKey struct { + path string + size, mtime int64 +} + +// certEntry is the outcome of parsing a catalog certificate. +type certEntry struct { + cert *Cert + err error + expires time.Time // zero for successes, which only age out of the LRU +} + +const ( + certsLRUSize = 1024 + // certErrTTL bounds how long a failure to parse a catalog is remembered. + certErrTTL = 5 * time.Minute +) + +var ( + catalogGroup singleflight.Group + catalogCertCache = expirable.NewLRU[certKey, *certEntry](certsLRUSize, nil, time.Hour) + + pkcs7ParseMu sync.Mutex +) + +// parsePKCS7 parses the DER/BER encoded PKCS#7 package. It is a goroutine-safe +// wrapper around pkcs7.Parse. Always use it instead of calling pkcs7.Parse +// directly. +func parsePKCS7(data []byte) (*pkcs7.PKCS7, error) { + pkcs7ParseMu.Lock() + defer pkcs7ParseMu.Unlock() + return pkcs7.Parse(data) +} + +func lookupCert(k certKey) (*certEntry, bool) { + e, ok := catalogCertCache.Get(k) + if !ok { + return nil, false + } + // failures carry a deadline shorter than the LRU TTL + if !e.expires.IsZero() && time.Now().After(e.expires) { + catalogCertCache.Remove(k) + return nil, false + } + return e, true +} + +func parseCatalogCert(path string) (*Cert, error) { + f, err := os.Open(path) if err != nil { return nil, err } defer f.Close() + crt, err := io.ReadAll(f) if err != nil { return nil, err } - pkcs, err := pkcs7.Parse(crt) + pkcs, err := parsePKCS7(crt) if err != nil { return nil, err } - certInfo := &Cert{} - serialNumber := pkcs.Signers[0].IssuerAndSerialNumber.SerialNumber - for _, cert := range pkcs.Certificates { - if !reflect.DeepEqual(cert.SerialNumber, serialNumber) { - continue - } - - certInfo.SerialNumber = hex.EncodeToString(cert.SerialNumber.Bytes()) - - certInfo.NotAfter = cert.NotAfter - certInfo.NotBefore = cert.NotBefore - - // issuer information - if len(cert.Issuer.Country) > 0 { - certInfo.Issuer = cert.Issuer.Country[0] - } - - if len(cert.Issuer.Province) > 0 { - certInfo.Issuer += ", " + cert.Issuer.Province[0] - } + if len(pkcs.Signers) == 0 { + return nil, errors.New("catalog has no signers") + } + signer := pkcs.Signers[0].IssuerAndSerialNumber + if signer.SerialNumber == nil { + return nil, errors.New("catalog signer has no serial number") + } - if len(cert.Issuer.Locality) > 0 { - certInfo.Issuer += ", " + cert.Issuer.Locality[0] + for _, cert := range pkcs.Certificates { + if cert.SerialNumber != nil && + cert.SerialNumber.Cmp(signer.SerialNumber) == 0 && + libytes.Equal(cert.RawIssuer, signer.IssuerName.FullBytes) { + return NewCert(cert), nil } + } + return nil, errors.New("catalog signer certificate not found") +} - certInfo.Issuer += ", " + cert.Issuer.CommonName +// ParseCertificate parses the catalog certificate. +func (c *Cat) ParseCertificate() (*Cert, error) { + path := c.catalogInfo.CatalogFile() + fi, err := os.Stat(path) + if err != nil { + return nil, err + } - // subject information - if len(cert.Subject.Country) > 0 { - certInfo.Subject = cert.Subject.Country[0] - } + size := fi.Size() + mtime := fi.ModTime().Unix() + key := certKey{strings.ToLower(path), size, mtime} - if len(cert.Subject.Province) > 0 { - certInfo.Subject += ", " + cert.Subject.Province[0] - } + if e, ok := lookupCert(key); ok { + return e.cert, e.err + } - if len(cert.Subject.Locality) > 0 { - certInfo.Subject += ", " + cert.Subject.Locality[0] + v, _ := catalogGroup.Do(fmt.Sprint(key), func() (any, error) { + if e, ok := lookupCert(key); ok { + return e.cert, e.err } - - if len(cert.Subject.Organization) > 0 { - certInfo.Subject += ", " + cert.Subject.Organization[0] + cert, err := parseCatalogCert(path) + e := &certEntry{cert: cert, err: err} + if err != nil { + var pathErr *fs.PathError + if errors.As(err, &pathErr) { + return e, nil // I/O failure is not cached + } + e.expires = time.Now().Add(certErrTTL) } + catalogCertCache.Add(key, e) + return e, nil + }) - certInfo.Subject += ", " + cert.Subject.CommonName - - break - } - return certInfo, nil + e := v.(*certEntry) + return e.cert, e.err } func (c *Cat) Close() error { @@ -499,6 +557,51 @@ type Cert struct { SerialNumber string `json:"serial_number"` } +// NewCert builds the certificate info from the X.509 certificate. +func NewCert(cert *x509.Certificate) *Cert { + certInfo := &Cert{ + SerialNumber: hex.EncodeToString(cert.SerialNumber.Bytes()), + NotAfter: cert.NotAfter, + NotBefore: cert.NotBefore, + } + + // issuer information + if len(cert.Issuer.Country) > 0 { + certInfo.Issuer = cert.Issuer.Country[0] + } + + if len(cert.Issuer.Province) > 0 { + certInfo.Issuer += ", " + cert.Issuer.Province[0] + } + + if len(cert.Issuer.Locality) > 0 { + certInfo.Issuer += ", " + cert.Issuer.Locality[0] + } + + certInfo.Issuer += ", " + cert.Issuer.CommonName + + // subject information + if len(cert.Subject.Country) > 0 { + certInfo.Subject = cert.Subject.Country[0] + } + + if len(cert.Subject.Province) > 0 { + certInfo.Subject += ", " + cert.Subject.Province[0] + } + + if len(cert.Subject.Locality) > 0 { + certInfo.Subject += ", " + cert.Subject.Locality[0] + } + + if len(cert.Subject.Organization) > 0 { + certInfo.Subject += ", " + cert.Subject.Organization[0] + } + + certInfo.Subject += ", " + cert.Subject.CommonName + + return certInfo +} + // Marshal writes certificate info into a raw buffer. func (c *Cert) Marshal() []byte { b := make([]byte, 0) From 1adbd686f7f60544dfead86743a1e80274095198 Mon Sep 17 00:00:00 2001 From: rabbitstack Date: Fri, 16 Oct 2026 14:21:16 +0200 Subject: [PATCH 03/13] fix(signature): Eliminate singleflight race window Store the result inside singleflight callback to fix the race window after Do returns. --- pkg/util/signature/signature.go | 50 +++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/pkg/util/signature/signature.go b/pkg/util/signature/signature.go index bfd188784..ad53725d5 100644 --- a/pkg/util/signature/signature.go +++ b/pkg/util/signature/signature.go @@ -197,7 +197,7 @@ func (s *Signatures) DoRequest(key Key) *Signature { // queue full: fall back to inline check rather than // dropping a decision that has a security consequence. signatureAsyncRequestDrops.Add(1) - return s.getOrCheck(key) + return s.resolve(key) } r := <-ch return r @@ -286,13 +286,26 @@ func (s *Signatures) get(key Key) *Signature { return sig } +// peek looks up the signature without touching the access timestamp +// or the hit counter. +func (s *Signatures) peek(key Key) *Signature { + s.mux.RLock() + sig := s.signatures[key] + s.mux.RUnlock() + return sig +} + // contains returns true if the signature with the given key exists in the store. // If the signature doesn't exist, this method returns false. func (s *Signatures) contains(key Key) bool { return s.get(key) != nil } -func (s *Signatures) getOrCheck(key Key) *Signature { +// resolve returns the cached signature for the key, or verifies the +// module and stores the result. Concurrent callers for the same key +// share a single verification. The call blocks until the result is +// available. +func (s *Signatures) resolve(key Key) *Signature { if sig := s.get(key); sig != nil { return sig } @@ -301,27 +314,28 @@ func (s *Signatures) getOrCheck(key Key) *Signature { // concurrently, only one of them does the PE/catalog work, while the // rest block and share the result. This is critical during process // startup when many threads load the same DLL simultaneously - v, err := s.group.Do(key.String(), func() (any, error) { + v, _ := s.group.Do(key.String(), func() (any, error) { + // previous flight may have finished just before this one started + if sig := s.peek(key); sig != nil { + return sig, nil + } + sig := newUncheckedSignature(key.Path) if err := sig.check(); err != nil { - return sig, err + signatureCheckErrors.Add(err.Error(), 1) } - return sig, nil - }) - if err != nil { - signatureCheckErrors.Add(err.Error(), 1) - } + // store and count once per check, not once per sharer + s.mux.Lock() + s.signatures[key] = sig + s.mux.Unlock() + signatureCount.Add(1) + signatureCacheMisses.Add(1) - // put the signature in the store - s.mux.Lock() - sign := v.(*Signature) - s.signatures[key] = sign - s.mux.Unlock() - signatureCount.Add(1) - signatureCacheMisses.Add(1) + return sig, nil + }) - return sign + return v.(*Signature) } func (s *Signatures) runWorker() { @@ -347,7 +361,7 @@ func (s *Signatures) runWorker() { } func (s *Signatures) processRequest(req Request) { - sign := s.getOrCheck(req.Key) + sign := s.resolve(req.Key) if req.Response != nil { req.Response <- sign } From 660cfd5c7376439d9ea5827342f4a4916d53d51c Mon Sep 17 00:00:00 2001 From: rabbitstack Date: Fri, 16 Oct 2026 14:25:25 +0200 Subject: [PATCH 04/13] perf(signature): Bound concurrent inline checks with a semaphore --- pkg/util/signature/signature.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/pkg/util/signature/signature.go b/pkg/util/signature/signature.go index ad53725d5..cd2b56029 100644 --- a/pkg/util/signature/signature.go +++ b/pkg/util/signature/signature.go @@ -37,6 +37,10 @@ import ( // requestQueueSize is the signature request input channel size const requestQueueSize = 2048 +// maxInlineChecks bounds the number of signature checks that can run +// concurrently on caller goroutines when the request queue is saturated. +const maxInlineChecks = 2 + var signatureAsyncRequestDrops = expvar.NewInt("signature.async.request.drops") var signatureCiCount = expvar.NewInt("signature.ci.count") @@ -121,6 +125,9 @@ type Signatures struct { group singleflight.Group + // inline bounds concurrent checks run on caller goroutines. + inline chan struct{} + stop chan struct{} } @@ -153,6 +160,7 @@ func newSignatures() *Signatures { purger: time.NewTicker(time.Minute), requests: make(chan Request, requestQueueSize), certs: make(chan Request, requestQueueSize), + inline: make(chan struct{}, maxInlineChecks), stop: make(chan struct{}), } @@ -196,7 +204,14 @@ func (s *Signatures) DoRequest(key Key) *Signature { default: // queue full: fall back to inline check rather than // dropping a decision that has a security consequence. + // Also, bound concurrent inline checks with a semaphore signatureAsyncRequestDrops.Add(1) + select { + case s.inline <- struct{}{}: + defer func() { <-s.inline }() + case <-s.stop: + return newUncheckedSignature(key.Path) + } return s.resolve(key) } r := <-ch From f304f5984845ac6dada208d7ad98571c5367fbc0 Mon Sep 17 00:00:00 2001 From: rabbitstack Date: Fri, 16 Oct 2026 14:36:18 +0200 Subject: [PATCH 05/13] perf(signature): Deduplicate async requests with a pending set --- pkg/util/signature/signature.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/pkg/util/signature/signature.go b/pkg/util/signature/signature.go index cd2b56029..cd5e9c787 100644 --- a/pkg/util/signature/signature.go +++ b/pkg/util/signature/signature.go @@ -59,6 +59,8 @@ var signatureInvalidations = expvar.NewMap("signature.invalidations") var signatureCertParseErrors = expvar.NewMap("signature.cert.parse.errors") +var signatureAsyncRequestDeduped = expvar.NewInt("signature.async.request.deduped") + // Key stores the attributes used for mapping the executable/DLL to its signature info. type Key struct { // Path is the normalized NT device path of the executable/DLL file. @@ -125,9 +127,12 @@ type Signatures struct { group singleflight.Group - // inline bounds concurrent checks run on caller goroutines. + // inline bounds concurrent checks run on caller goroutines inline chan struct{} + // pending tracks keys that are enqueued or being checked + pending sync.Map + stop chan struct{} } @@ -226,9 +231,16 @@ func (s *Signatures) DoRequestAsync(key Key) { if s.contains(key) { return } + + if _, loaded := s.pending.LoadOrStore(key, struct{}{}); loaded { + signatureAsyncRequestDeduped.Add(1) + return + } + select { case s.requests <- Request{Key: key}: default: + s.pending.Delete(key) // release the claim so a later event can retry // drop rather than block the event processing thread signatureAsyncRequestDrops.Add(1) } @@ -279,6 +291,7 @@ func (s *Signatures) PutSignature(key Key, sigType Type, sigLevel Level) { s.mux.Lock() s.signatures[key] = newSignature(key.Path, sigType, sigLevel) s.mux.Unlock() + s.pending.Delete(key) signatureCount.Add(1) signatureCiCount.Add(1) @@ -322,6 +335,7 @@ func (s *Signatures) contains(key Key) bool { // available. func (s *Signatures) resolve(key Key) *Signature { if sig := s.get(key); sig != nil { + s.pending.Delete(key) return sig } @@ -347,6 +361,7 @@ func (s *Signatures) resolve(key Key) *Signature { signatureCount.Add(1) signatureCacheMisses.Add(1) + s.pending.Delete(key) return sig, nil }) From d5239dab3e5ceb4be1a1ba7b1e4810675b2234d3 Mon Sep 17 00:00:00 2001 From: rabbitstack Date: Fri, 16 Oct 2026 14:57:37 +0200 Subject: [PATCH 06/13] perf(signature): Reduce parallelism for crypto and I/O work --- pkg/util/signature/signature.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/util/signature/signature.go b/pkg/util/signature/signature.go index cd5e9c787..c458f23e0 100644 --- a/pkg/util/signature/signature.go +++ b/pkg/util/signature/signature.go @@ -169,7 +169,10 @@ func newSignatures() *Signatures { stop: make(chan struct{}), } - workerCount := max(runtime.NumCPU()/2, 4) + workerCount := 2 + if runtime.NumCPU() >= 16 { + workerCount = 4 + } for range workerCount { go sigs.runWorker() } From 84a55b3fb81d0818aeff405e0fdf44d00c52daff Mon Sep 17 00:00:00 2001 From: rabbitstack Date: Fri, 16 Oct 2026 15:04:41 +0200 Subject: [PATCH 07/13] fix(signature): Invert signature nil check condition --- pkg/util/signature/signature.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/util/signature/signature.go b/pkg/util/signature/signature.go index c458f23e0..d43553371 100644 --- a/pkg/util/signature/signature.go +++ b/pkg/util/signature/signature.go @@ -383,7 +383,7 @@ func (s *Signatures) runWorker() { s.processRequest(req) case req := <-s.certs: sig := s.get(req.Key) - if sig != nil { + if sig == nil { continue } s.parseCertificate(sig) From e96386dd335370a862900b3c86a4935193a9a07d Mon Sep 17 00:00:00 2001 From: rabbitstack Date: Fri, 16 Oct 2026 15:27:23 +0200 Subject: [PATCH 08/13] perf(signature): Use only the local cache for revocation checks --- pkg/sys/trust.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/sys/trust.go b/pkg/sys/trust.go index 68a7d972c..62c659608 100644 --- a/pkg/sys/trust.go +++ b/pkg/sys/trust.go @@ -76,6 +76,8 @@ const ( WtdStateActionClose = 0x00000002 // WtdSaferFlag is the trust provider flag WtdSaferFlag = 0x100 + // WtdCacheOnlyURLRetrieval use only the local cache for revocation checks. Prevents revocation checks over the network. + WtdCacheOnlyURLRetrieval = 0x1000 ) const ( @@ -155,7 +157,7 @@ func NewWintrustData(choice uint32) *WintrustData { UIChoice: WtdUINone, RevocationChecks: WtdRevokeNone, StateAction: WtdStateActionVerify, - ProviderFlags: WtdSaferFlag, + ProviderFlags: WtdSaferFlag | WtdCacheOnlyURLRetrieval, } } From 65edddfce14363a8e03b0ce519dfd71a627d4675 Mon Sep 17 00:00:00 2001 From: rabbitstack Date: Fri, 16 Oct 2026 17:18:02 +0200 Subject: [PATCH 09/13] fix(signature): Pass catalog structs by reference --- pkg/sys/trust.go | 12 ++++++------ pkg/util/signature/types.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/sys/trust.go b/pkg/sys/trust.go index 62c659608..b880c4f3e 100644 --- a/pkg/sys/trust.go +++ b/pkg/sys/trust.go @@ -205,7 +205,7 @@ func (t *WintrustData) VerifyCatalog( fd uintptr, filename string, catalogAdmin windows.Handle, - catalog CatalogInfo, + catalog *CatalogInfo, hash []byte, hashSize uint32, ) (SignatureStatus, error) { @@ -296,7 +296,7 @@ type CatalogInfo struct { } // CatalogFile returns the full path to the catalog file. -func (c CatalogInfo) CatalogFile() string { +func (c *CatalogInfo) CatalogFile() string { n := len(c.Name) / 2 p := unsafe.Slice((*uint16)(unsafe.Pointer(&c.Name[0])), n) return windows.UTF16ToString(p) @@ -327,7 +327,7 @@ type Cat struct { admin windows.Handle catalog windows.Handle file *os.File - catalogInfo CatalogInfo + catalogInfo *CatalogInfo hash []byte size uint32 @@ -336,8 +336,8 @@ type Cat struct { const initialHashBufSize uint32 = 32 // SHA-1 is 20, SHA-256 is 32; start at 32 // NewCatalog creates an instance of the catalog with default hash size. -func NewCatalog() Cat { - return Cat{size: initialHashBufSize, hash: make([]byte, initialHashBufSize)} +func NewCatalog() *Cat { + return &Cat{size: initialHashBufSize, hash: make([]byte, initialHashBufSize)} } // Open opens the catalog and acquires the hash for the given file. If the @@ -384,7 +384,7 @@ func (c *Cat) Open(filename string) error { c.size, 0, nil, ) c.catalogInfo.Size = uint32(unsafe.Sizeof(c.catalogInfo)) - return CryptCatalogInfoFromContext(c.catalog, &c.catalogInfo, 0) + return CryptCatalogInfoFromContext(c.catalog, c.catalogInfo, 0) } // IsCatalogSigned determines if the file is catalog-signed. diff --git a/pkg/util/signature/types.go b/pkg/util/signature/types.go index 97f889be6..38b13349c 100644 --- a/pkg/util/signature/types.go +++ b/pkg/util/signature/types.go @@ -270,7 +270,7 @@ func (s *Signature) verifyFile() error { } // verifyCatalog verifies the catalog-based file signature. -func (s *Signature) verifyCatalog(catalog sys.Cat) error { +func (s *Signature) verifyCatalog(catalog *sys.Cat) error { status, err := catalog.Verify(s.Path) s.setStatus(status) return err From b141dcfe835958c9d7f09cd7011ab2c0ff3a5a9b Mon Sep 17 00:00:00 2001 From: rabbitstack Date: Fri, 16 Oct 2026 17:44:27 +0200 Subject: [PATCH 10/13] perf(signature): Avoid cert parse retry loop Catalog-signed files, which are most of Windows, have no embedded certificate. Request's hot path re-runs a full PE parse on the caller's thread, on every request for that module. Introduce a new atomic field, that is set when the cert parsing is attempted, and will not try to parse the cert again. --- pkg/util/signature/signature.go | 10 ++++++++-- pkg/util/signature/types.go | 16 ++++++++-------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/pkg/util/signature/signature.go b/pkg/util/signature/signature.go index d43553371..7c28fc975 100644 --- a/pkg/util/signature/signature.go +++ b/pkg/util/signature/signature.go @@ -199,8 +199,11 @@ func (s *Signatures) DoRequest(key Key) *Signature { } else { // hot path returns immediately if the signature is already resolved if sig := s.get(key); sig != nil { - if sig.IsTrusted() && !sig.HasCertificate() { - s.parseCertificate(sig) + if sig.IsTrusted() && !sig.HasCertificate() && !sig.certTried.Load() { + select { + case s.certs <- Request{Key: key}: + default: + } } return sig } @@ -447,6 +450,9 @@ func (s *Signatures) gcSignatures() { } func (s *Signatures) parseCertificate(sig *Signature) { + if sig == nil || !sig.certTried.CompareAndSwap(false, true) { + return + } cert, err := sig.parseCertificate(false) if err != nil { signatureCertParseErrors.Add(err.Error(), 1) diff --git a/pkg/util/signature/types.go b/pkg/util/signature/types.go index 38b13349c..f89bbc40c 100644 --- a/pkg/util/signature/types.go +++ b/pkg/util/signature/types.go @@ -143,6 +143,8 @@ type Signature struct { // cert represents certificate information for the particular signature. cert atomic.Pointer[sys.Cert] + // certTried indicates if certificate parsing was attempted + certTried atomic.Bool // accessed the timestamp of the signature access by field extractor. accessed atomic.Int64 @@ -277,8 +279,8 @@ func (s *Signature) verifyCatalog(catalog *sys.Cat) error { } // parseCertificate parses the certificate data for catalog-based -// or PE signatures if the parameter is set to false. -func (s *Signature) parseCertificate(onlyCatalog bool) (*sys.Cert, error) { +// or PE signatures if the skipEmbedded parameter is set to false. +func (s *Signature) parseCertificate(skipEmbedded bool) (*sys.Cert, error) { if s == nil { return nil, ErrNilSignature } @@ -291,19 +293,17 @@ func (s *Signature) parseCertificate(onlyCatalog bool) (*sys.Cert, error) { return nil, ErrNoSignature } - if !onlyCatalog { + if !skipEmbedded { // parse PE certificate - f, err := pe.ParseFile(s.Path, pe.WithSecurity()) - if err != nil { - goto cat + if f, err := pe.ParseFile(s.Path, pe.WithSecurity()); err == nil && f.Cert != nil { + return f.Cert, nil } - return f.Cert, nil } -cat: if !sys.IsWintrustFound() { return nil, ErrWintrustUnavailable } + // parse catalog certificate catalog := sys.NewCatalog() defer catalog.Close() From bd0f295d622fd0c1e3ddb101d64f5edfcb56d513 Mon Sep 17 00:00:00 2001 From: rabbitstack Date: Sat, 17 Oct 2026 19:42:33 +0200 Subject: [PATCH 11/13] fix(signature): Keep WinVerifyTrust inputs on the heap until Close Every pointer the native side reads now lives in heap memory. The structs hang off a field of the heap-allocated WintrustData, so the compiler must treat them as escaping. The strings are separate heap buffers. A goroutine stack move can no longer invalidate them. The objects stay reachable until after the second WinVerifyTrust call. Before, they could be collected between VerifyCatalog returning and the deferred Close. --- pkg/sys/fs.go | 2 +- pkg/sys/trust.go | 95 +++++++++++------- pkg/util/signature/cert.go | 170 ++++++++++++++++++++++++++++++++ pkg/util/signature/cert_test.go | 56 +++++++++++ pkg/util/signature/signature.go | 9 +- pkg/util/signature/types.go | 5 +- 6 files changed, 290 insertions(+), 47 deletions(-) create mode 100644 pkg/util/signature/cert.go create mode 100644 pkg/util/signature/cert_test.go diff --git a/pkg/sys/fs.go b/pkg/sys/fs.go index bc776181b..f13186c29 100644 --- a/pkg/sys/fs.go +++ b/pkg/sys/fs.go @@ -126,7 +126,7 @@ func ReadFile(path string, size int, timeout time.Duration) ([]byte, error) { handle, err := windows.CreateFile( pathUTF16, windows.GENERIC_READ, - windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, nil, windows.OPEN_EXISTING, windows.FILE_FLAG_OVERLAPPED, diff --git a/pkg/sys/trust.go b/pkg/sys/trust.go index b880c4f3e..047d8c269 100644 --- a/pkg/sys/trust.go +++ b/pkg/sys/trust.go @@ -76,7 +76,8 @@ const ( WtdStateActionClose = 0x00000002 // WtdSaferFlag is the trust provider flag WtdSaferFlag = 0x100 - // WtdCacheOnlyURLRetrieval use only the local cache for revocation checks. Prevents revocation checks over the network. + // WtdCacheOnlyURLRetrieval use only the local cache for revocation checks. Prevents revocation + // checks over the network. WtdCacheOnlyURLRetrieval = 0x1000 ) @@ -130,6 +131,10 @@ func (s SignatureStatus) String() string { // WintrustActionGenericVerifyV2 is the action that indicates the file or object should be verified by using the Authenticode policy provider. var WintrustActionGenericVerifyV2 = windows.GUID{Data1: 0xaac56b, Data2: 0xcd44, Data3: 0x11d0, Data4: [8]byte{0x8c, 0xc2, 0x0, 0xc0, 0x4f, 0xc2, 0x95, 0xee}} +// wintrustDataSize covers only the C-visible prefix of WintrustData +var wintrustDataStructSize = uint32(unsafe.Offsetof(WintrustData{}.SignatureSettings) + + unsafe.Sizeof(uintptr(0))) + // WintrustData structure is used when calling WinVerifyTrust // to pass necessary information into the trust providers. type WintrustData struct { @@ -146,13 +151,18 @@ type WintrustData struct { ProviderFlags uint32 UIContext uint32 SignatureSettings *WintrustSignatureSettings + + // keep every object that is referenced through a uintptr field reachable until Close + fileInfo *WintrustFileInfo + catInfo *WintrustCatalogInfo + pins []any } // NewWintrustData creates a new instance of WintrustData prepared // to verify file or catalog trust. func NewWintrustData(choice uint32) *WintrustData { return &WintrustData{ - Size: uint32(unsafe.Sizeof(WintrustData{})), + Size: wintrustDataStructSize, UnionChoice: choice, UIChoice: WtdUINone, RevocationChecks: WtdRevokeNone, @@ -188,16 +198,16 @@ func (t *WintrustData) verifyTrust() (SignatureStatus, error) { // This is achieved by providing INVALID_HANDLE_VALUE as a first parameter // in the WinVerifyTrust call. func (t *WintrustData) VerifyFile(filename string) (SignatureStatus, error) { - fileinfo := &WintrustFileInfo{ + p, err := windows.UTF16PtrFromString(filename) + if err != nil { + return SignatureNotTrusted, err + } + t.fileInfo = &WintrustFileInfo{ Size: uint32(unsafe.Sizeof(WintrustFileInfo{})), - FilePath: windows.StringToUTF16Ptr(filename), + FilePath: p, } - t.Union = uintptr(unsafe.Pointer(fileinfo)) - - status, err := t.verifyTrust() - runtime.KeepAlive(fileinfo) - - return status, err + t.Union = uintptr(unsafe.Pointer(t.fileInfo)) + return t.verifyTrust() } // VerifyCatalog verifies the trust of the file signed by catalog-based certificate. @@ -205,20 +215,29 @@ func (t *WintrustData) VerifyCatalog( fd uintptr, filename string, catalogAdmin windows.Handle, - catalog *CatalogInfo, + catalogFile string, hash []byte, hashSize uint32, ) (SignatureStatus, error) { - // convert strings to UTF-16 and hold explicit Go references. - // Converting directly to uintptr in the struct literal would - // allow the GC to collect these before WinVerifyTrust reads them - s := format.BytesToHex(hash[:hashSize]) - tag := windows.StringToUTF16Ptr(s) - path := windows.StringToUTF16Ptr(filename) + if hashSize == 0 || int(hashSize) > len(hash) { + return SignatureNotTrusted, errors.New("invalid catalog member hash") + } + tag, err := windows.UTF16PtrFromString(format.BytesToHex(hash[:hashSize])) + if err != nil { + return SignatureNotTrusted, err + } + path, err := windows.UTF16PtrFromString(filename) + if err != nil { + return SignatureNotTrusted, err + } + catPath, err := windows.UTF16PtrFromString(catalogFile) + if err != nil { + return SignatureNotTrusted, err + } catinfo := &WintrustCatalogInfo{ Size: uint32(unsafe.Sizeof(WintrustCatalogInfo{})), - CatalogFilePath: uintptr(unsafe.Pointer(&catalog.Name[0])), + CatalogFilePath: uintptr(unsafe.Pointer(catPath)), MemberFilePath: uintptr(unsafe.Pointer(path)), MemberFile: windows.Handle(fd), MemberTag: uintptr(unsafe.Pointer(tag)), @@ -226,25 +245,17 @@ func (t *WintrustData) VerifyCatalog( FileHashSize: hashSize, CatalogAdmin: catalogAdmin, } - t.Union = uintptr(unsafe.Pointer(catinfo)) - status, err := t.verifyTrust() - - // KeepAlive pins every object whose address was passed into - // WinVerifyTrust via a uintptr field. Without these, the GC - // is permitted to collect them as soon as their last Go-visible - // use is seenm which may be before verifyTrust returns - runtime.KeepAlive(tag) - runtime.KeepAlive(path) - runtime.KeepAlive(hash) - runtime.KeepAlive(catalog) - runtime.KeepAlive(catinfo) + // everything the native side reads through a uintptr stays reachable until Close + t.pins = []any{tag, path, catPath, hash} + t.Union = uintptr(unsafe.Pointer(catinfo)) - return status, err + return t.verifyTrust() } -// Close disposes state data by specifying the corresponding action +// Close disposes state data by specifying the corresponding action. func (t *WintrustData) Close() error { + defer t.release() t.StateAction = WtdStateActionClose status, err := WinVerifyTrust(windows.InvalidHandle, &WintrustActionGenericVerifyV2, t) if err != nil { @@ -256,6 +267,11 @@ func (t *WintrustData) Close() error { return nil } +func (t *WintrustData) release() { + t.Union = 0 + t.fileInfo, t.catInfo, t.pins = nil, nil, nil +} + // WintrustSignatureSettings structure can be used to specify the signatures on a file. type WintrustSignatureSettings struct { Size uint32 @@ -327,7 +343,7 @@ type Cat struct { admin windows.Handle catalog windows.Handle file *os.File - catalogInfo *CatalogInfo + catalogInfo CatalogInfo hash []byte size uint32 @@ -384,7 +400,7 @@ func (c *Cat) Open(filename string) error { c.size, 0, nil, ) c.catalogInfo.Size = uint32(unsafe.Sizeof(c.catalogInfo)) - return CryptCatalogInfoFromContext(c.catalog, c.catalogInfo, 0) + return CryptCatalogInfoFromContext(c.catalog, &c.catalogInfo, 0) } // IsCatalogSigned determines if the file is catalog-signed. @@ -394,12 +410,15 @@ func (c *Cat) IsCatalogSigned() bool { // Verify verifies the signature of the given file against the catalog. func (c *Cat) Verify(filename string) (SignatureStatus, error) { - trust := NewWintrustData(WtdChoiceCatalog) - defer trust.Close() if c.file == nil { - panic("catalog is not opened") + return SignatureNotTrusted, errors.New("catalog is not opened") } - return trust.VerifyCatalog(c.file.Fd(), filename, c.admin, c.catalogInfo, c.hash, c.size) + defer runtime.KeepAlive(c.file) + trust := NewWintrustData(WtdChoiceCatalog) + defer trust.Close() + return trust.VerifyCatalog( + c.file.Fd(), filename, c.admin, c.catalogInfo.CatalogFile(), c.hash, c.size, + ) } // certKey is the cache key for parsed cert per catalog file. Keyed by path, size and modification time. diff --git a/pkg/util/signature/cert.go b/pkg/util/signature/cert.go new file mode 100644 index 000000000..917bc231d --- /dev/null +++ b/pkg/util/signature/cert.go @@ -0,0 +1,170 @@ +//go:build windows + +/* + * Copyright 2021-present by Nedim Sabic Sabic + * https://www.fibratus.io + * All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package signature + +import ( + "bytes" + "debug/pe" + "encoding/binary" + "encoding/hex" + "errors" + "fmt" + "reflect" + "time" + + "github.com/rabbitstack/fibratus/pkg/sys" + "go.mozilla.org/pkcs7" +) + +const ( + winCertHeaderSize = 8 // dwLength + wRevision + wCertificateType + winCertTypePKCSSignedData = 0x0002 // WIN_CERT_TYPE_PKCS_SIGNED_DATA + maxCertBlobSize = 4 << 20 + readSize = 8 * 1024 * 1024 // 8 MB +) + +// parseSecurityDirectory reads only the PE headers and the WIN_CERTIFICATE blob, +// of the executable or DLL file and returns true and the signier certificiate if +// the file has a valid signature. +func parseSecurityDirectory(path string) (bool, *sys.Cert, error) { + buf, err := sys.ReadFile(path, readSize, time.Millisecond*500) + if err != nil { + return false, nil, err + } + + r := bytes.NewReader(buf) + f, err := pe.NewFile(r) + if err != nil { + return false, nil, err + } + + var dir pe.DataDirectory + switch oh := f.OptionalHeader.(type) { + case *pe.OptionalHeader32: + if oh.NumberOfRvaAndSizes > pe.IMAGE_DIRECTORY_ENTRY_SECURITY { + dir = oh.DataDirectory[pe.IMAGE_DIRECTORY_ENTRY_SECURITY] + } + case *pe.OptionalHeader64: + if oh.NumberOfRvaAndSizes > pe.IMAGE_DIRECTORY_ENTRY_SECURITY { + dir = oh.DataDirectory[pe.IMAGE_DIRECTORY_ENTRY_SECURITY] + } + default: + return false, nil, errors.New("pe: no optional header") + } + + // for the security directory, VirtualAddress is a raw file offset, not an RVA + if dir.VirtualAddress == 0 || dir.Size == 0 { + return false, nil, nil + } + + off, size := int64(dir.VirtualAddress), int64(dir.Size) + if int(off+size) > len(buf) { + return false, nil, errors.New("security directory exceeds file size") + } + cert, err := readSignerCert(r, off, size) + return err == nil && cert != nil, cert, err +} + +func readSignerCert(r *bytes.Reader, off, size int64) (*sys.Cert, error) { + if size < winCertHeaderSize { + return nil, errors.New("security directory too small") + } + + var hdr [winCertHeaderSize]byte + if _, err := r.ReadAt(hdr[:], off); err != nil { + return nil, fmt.Errorf("read WIN_CERTIFICATE header: %w", err) + } + length := int64(binary.LittleEndian.Uint32(hdr[0:4])) + certType := binary.LittleEndian.Uint16(hdr[6:8]) + + switch { + case certType != winCertTypePKCSSignedData: + return nil, fmt.Errorf("unsupported certificate type %#x", certType) + case length < winCertHeaderSize || length > size: + return nil, fmt.Errorf("invalid certificate length %d", length) + case length-winCertHeaderSize > maxCertBlobSize: + return nil, fmt.Errorf("certificate blob too large: %d", length) + } + + blob := make([]byte, length-winCertHeaderSize) + if _, err := r.ReadAt(blob, off+winCertHeaderSize); err != nil { + return nil, fmt.Errorf("read certificate blob: %w", err) + } + + p7, err := pkcs7.Parse(blob) + if err != nil { + return nil, err + } + if len(p7.Signers) == 0 { + return nil, errors.New("signature has no signers") + } + + certInfo := &sys.Cert{} + serialNumber := p7.Signers[0].IssuerAndSerialNumber.SerialNumber + for _, cert := range p7.Certificates { + if !reflect.DeepEqual(cert.SerialNumber, serialNumber) { + continue + } + + certInfo.SerialNumber = hex.EncodeToString(cert.SerialNumber.Bytes()) + + certInfo.NotAfter = cert.NotAfter + certInfo.NotBefore = cert.NotBefore + + // issuer information + if len(cert.Issuer.Country) > 0 { + certInfo.Issuer = cert.Issuer.Country[0] + } + + if len(cert.Issuer.Province) > 0 { + certInfo.Issuer += ", " + cert.Issuer.Province[0] + } + + if len(cert.Issuer.Locality) > 0 { + certInfo.Issuer += ", " + cert.Issuer.Locality[0] + } + + certInfo.Issuer += ", " + cert.Issuer.CommonName + + // subject information + if len(cert.Subject.Country) > 0 { + certInfo.Subject = cert.Subject.Country[0] + } + + if len(cert.Subject.Province) > 0 { + certInfo.Subject += ", " + cert.Subject.Province[0] + } + + if len(cert.Subject.Locality) > 0 { + certInfo.Subject += ", " + cert.Subject.Locality[0] + } + + if len(cert.Subject.Organization) > 0 { + certInfo.Subject += ", " + cert.Subject.Organization[0] + } + + certInfo.Subject += ", " + cert.Subject.CommonName + + return certInfo, nil + } + + return nil, errors.New("signer certificate not found") +} diff --git a/pkg/util/signature/cert_test.go b/pkg/util/signature/cert_test.go new file mode 100644 index 000000000..0c1a699e6 --- /dev/null +++ b/pkg/util/signature/cert_test.go @@ -0,0 +1,56 @@ +//go:build windows + +/* + * Copyright 2021-present by Nedim Sabic Sabic + * https://www.fibratus.io + * All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package signature + +import ( + "path/filepath" + "testing" + + "github.com/rabbitstack/fibratus/pkg/pe" +) + +func TestReadSecurityMatchesParser(t *testing.T) { + t.Skip() + files, _ := filepath.Glob(`C:\Windows\System32\*.dll`) + if len(files) > 300 { + files = files[:300] + } + for _, path := range files { + old, err := pe.ParseFile(path, pe.WithSecurity()) + if err != nil || old == nil { + continue + } + signed, cert, err := parseSecurityDirectory(path) + if err != nil { + t.Errorf("%s: %v", path, err) + continue + } + if old.IsSigned() != signed { + t.Errorf("%s: signed old=%t new=%t", path, old.IsSigned(), signed) + } + if old.Cert != nil && (cert == nil || + old.Cert.Subject != cert.Subject || + old.Cert.Issuer != cert.Issuer || + old.Cert.SerialNumber != cert.SerialNumber) { + t.Errorf("%s: cert mismatch old=%+v new=%+v err=%v", path, old.Cert, cert, err) + } + } +} diff --git a/pkg/util/signature/signature.go b/pkg/util/signature/signature.go index 7c28fc975..cb77eda42 100644 --- a/pkg/util/signature/signature.go +++ b/pkg/util/signature/signature.go @@ -29,7 +29,6 @@ import ( "time" "github.com/golang/groupcache/singleflight" - "github.com/rabbitstack/fibratus/pkg/pe" "github.com/rabbitstack/fibratus/pkg/sys" log "github.com/sirupsen/logrus" ) @@ -474,15 +473,15 @@ func (s *Signature) check() error { } // check if the signature is embedded in PE - f, err := pe.ParseFile(s.Path, pe.WithSecurity()) + signed, cert, err := parseSecurityDirectory(s.Path) if err != nil { return err } - if f.IsSigned() { + if signed { s.setExists() s.setType(TypeEmbedded) - s.setCert(f.Cert) + s.setCert(cert) if !sys.IsWintrustFound() { return ErrWintrustUnavailable } @@ -509,7 +508,7 @@ func (s *Signature) check() error { if err := s.verifyCatalog(catalog); err != nil { return err } - cert, err := catalog.ParseCertificate() + cert, err = catalog.ParseCertificate() if err != nil { signatureCertParseErrors.Add(err.Error(), 1) return err diff --git a/pkg/util/signature/types.go b/pkg/util/signature/types.go index f89bbc40c..88f1f02d3 100644 --- a/pkg/util/signature/types.go +++ b/pkg/util/signature/types.go @@ -24,7 +24,6 @@ import ( "sync/atomic" "time" - "github.com/rabbitstack/fibratus/pkg/pe" "github.com/rabbitstack/fibratus/pkg/sys" ) @@ -295,8 +294,8 @@ func (s *Signature) parseCertificate(skipEmbedded bool) (*sys.Cert, error) { if !skipEmbedded { // parse PE certificate - if f, err := pe.ParseFile(s.Path, pe.WithSecurity()); err == nil && f.Cert != nil { - return f.Cert, nil + if _, cert, err := parseSecurityDirectory(s.Path); err == nil && cert != nil { + return cert, nil } } From 3db3532b7a05d92d7c8db8ad0b44e3f4f1380fbd Mon Sep 17 00:00:00 2001 From: rabbitstack Date: Mon, 19 Oct 2026 00:06:57 +0200 Subject: [PATCH 12/13] perf(signature): Faster PE security directory parsing --- pkg/util/signature/cert.go | 259 +++++++++---- pkg/util/signature/cert_test.go | 630 +++++++++++++++++++++++++++++++- 2 files changed, 788 insertions(+), 101 deletions(-) diff --git a/pkg/util/signature/cert.go b/pkg/util/signature/cert.go index 917bc231d..f43d3c741 100644 --- a/pkg/util/signature/cert.go +++ b/pkg/util/signature/cert.go @@ -22,74 +22,204 @@ package signature import ( "bytes" - "debug/pe" "encoding/binary" - "encoding/hex" "errors" "fmt" - "reflect" - "time" + "io" + "os" + "sync" "github.com/rabbitstack/fibratus/pkg/sys" - "go.mozilla.org/pkcs7" + "github.com/secDre4mer/pkcs7" ) const ( winCertHeaderSize = 8 // dwLength + wRevision + wCertificateType winCertTypePKCSSignedData = 0x0002 // WIN_CERT_TYPE_PKCS_SIGNED_DATA maxCertBlobSize = 4 << 20 - readSize = 8 * 1024 * 1024 // 8 MB ) -// parseSecurityDirectory reads only the PE headers and the WIN_CERTIFICATE blob, -// of the executable or DLL file and returns true and the signier certificiate if -// the file has a valid signature. +// Offsets and magic numbers of the PE headers that are needed to locate +// the certificate table. Nothing else in the image is ever read. +const ( + dosHeaderSize = 64 + dosMagic = 0x5a4d // "MZ" + dosNTHeaderOffset = 0x3c // e_lfanew + ntSignature = 0x00004550 // "PE\0\0" + fileHeaderSize = 20 + fileHeaderOptSizeOff = 16 // SizeOfOptionalHeader within IMAGE_FILE_HEADER + optionalMagicPE32 = 0x10b + optionalMagicPE32Plus = 0x20b + pe32DataDirsOffset = 96 // IMAGE_OPTIONAL_HEADER32.DataDirectory + pe32PlusDataDirsOffset = 112 // IMAGE_OPTIONAL_HEADER64.DataDirectory + dataDirEntrySize = 8 + dataDirSecurity = 4 // IMAGE_DIRECTORY_ENTRY_SECURITY +) + +// pkcs7ParseMu serializes every call into pkcs7.Parse in the process. +var pkcs7ParseMu sync.Mutex + +// ParsePKCS7 parses the DER/BER encoded PKCS#7 package. It is a goroutine-safe +// wrapper around pkcs7.Parse. Always use it instead of calling pkcs7.Parse +// directly. +func parsePKCS7(data []byte) (*pkcs7.PKCS7, error) { + pkcs7ParseMu.Lock() + defer pkcs7ParseMu.Unlock() + return pkcs7.Parse(data) +} + +// errNotPE is wrapped by the errors returned for files that aren't PE images. +var errNotPE = errors.New("not a PE image") + +// parseSecurityDirectory reads the PE headers and the WIN_CERTIFICATE blob of +// the executable or DLL, and returns the signer certificate of the embedded +// Authenticode signature. +// +// The returned boolean reports whether the image carries a certificate table. +// It says nothing about trust, which is the WinVerifyTrust's job. When the +// image is signed but the signature can't be decoded, the function returns +// true, a nil certificate, and the reason as the error. This matters for the +// caller: a signed image with a signature blob we can't decode must still be +// verified as an embedded signature, rather than be treated as unsigned and +// sent down the catalog lookup path. +// +// The certificate table is placed at the very end of the image, so the file is +// accessed with positioned reads. The cost is a few hundred bytes of headers plus +// the signature blob regardless of the image size. func parseSecurityDirectory(path string) (bool, *sys.Cert, error) { - buf, err := sys.ReadFile(path, readSize, time.Millisecond*500) + f, err := os.Open(path) if err != nil { return false, nil, err } + defer f.Close() - r := bytes.NewReader(buf) - f, err := pe.NewFile(r) + fi, err := f.Stat() if err != nil { return false, nil, err } - var dir pe.DataDirectory - switch oh := f.OptionalHeader.(type) { - case *pe.OptionalHeader32: - if oh.NumberOfRvaAndSizes > pe.IMAGE_DIRECTORY_ENTRY_SECURITY { - dir = oh.DataDirectory[pe.IMAGE_DIRECTORY_ENTRY_SECURITY] - } - case *pe.OptionalHeader64: - if oh.NumberOfRvaAndSizes > pe.IMAGE_DIRECTORY_ENTRY_SECURITY { - dir = oh.DataDirectory[pe.IMAGE_DIRECTORY_ENTRY_SECURITY] - } - default: - return false, nil, errors.New("pe: no optional header") + return parseSecurityDirectoryAt(f, fi.Size()) +} + +// parseSecurityDirectoryAt is parseSecurityDirectory for an image that is +// accessible through r, and has the given size in bytes. +func parseSecurityDirectoryAt(r io.ReaderAt, size int64) (bool, *sys.Cert, error) { + dir, err := securityDirectory(r) + if err != nil { + return false, nil, err } - // for the security directory, VirtualAddress is a raw file offset, not an RVA - if dir.VirtualAddress == 0 || dir.Size == 0 { + // for the security directory, the address is a raw file offset, not an RVA + if dir.offset == 0 || dir.size == 0 { return false, nil, nil } - off, size := int64(dir.VirtualAddress), int64(dir.Size) - if int(off+size) > len(buf) { - return false, nil, errors.New("security directory exceeds file size") + off, n := int64(dir.offset), int64(dir.size) + if off+n > size { + return true, nil, fmt.Errorf("security directory [%d, %d) exceeds file size %d", off, off+n, size) + } + + cert, err := readSignerCert(r, off, n) + if err != nil { + return true, nil, err + } + + return true, cert, nil +} + +type dataDirectory struct { + offset, size uint32 +} + +// securityDirectory walks the DOS, NT, and optional headers to find the +// IMAGE_DIRECTORY_ENTRY_SECURITY data directory. It performs a handful of tiny +// reads, and unlike debug/pe, it never touches the COFF symbol table which is +// potentially huge on unstripped binaries. +func securityDirectory(r io.ReaderAt) (dataDirectory, error) { + var dir dataDirectory + + var dos [dosHeaderSize]byte + if err := readFullAt(r, dos[:], 0); err != nil { + return dir, fmt.Errorf("%w: reading DOS header: %v", errNotPE, err) + } + if binary.LittleEndian.Uint16(dos[:]) != dosMagic { + return dir, fmt.Errorf("%w: bad DOS signature", errNotPE) + } + ntOff := int64(binary.LittleEndian.Uint32(dos[dosNTHeaderOffset:])) + + var nt [4 + fileHeaderSize]byte + if err := readFullAt(r, nt[:], ntOff); err != nil { + return dir, fmt.Errorf("%w: reading NT headers: %v", errNotPE, err) + } + if binary.LittleEndian.Uint32(nt[:]) != ntSignature { + return dir, fmt.Errorf("%w: bad NT signature", errNotPE) + } + + optOff := ntOff + int64(len(nt)) + optSize := int64(binary.LittleEndian.Uint16(nt[4+fileHeaderOptSizeOff:])) + if optSize < 2 { + return dir, fmt.Errorf("%w: no optional header", errNotPE) + } + + var magic [2]byte + if err := readFullAt(r, magic[:], optOff); err != nil { + return dir, fmt.Errorf("%w: reading optional header: %v", errNotPE, err) + } + var dirsOff int64 + switch m := binary.LittleEndian.Uint16(magic[:]); m { + case optionalMagicPE32: + dirsOff = pe32DataDirsOffset + case optionalMagicPE32Plus: + dirsOff = pe32PlusDataDirsOffset + default: + return dir, fmt.Errorf("%w: unknown optional header magic %#x", errNotPE, m) + } + + // an optional header too small to hold the security entry has no + // certificate table + entryOff := dirsOff + dataDirSecurity*dataDirEntrySize + if optSize < entryOff+dataDirEntrySize { + return dir, nil + } + + // NumberOfRvaAndSizes immediately precedes the data directory array + var n [4]byte + if err := readFullAt(r, n[:], optOff+dirsOff-4); err != nil { + return dir, fmt.Errorf("%w: reading data directories: %v", errNotPE, err) } - cert, err := readSignerCert(r, off, size) - return err == nil && cert != nil, cert, err + if binary.LittleEndian.Uint32(n[:]) <= dataDirSecurity { + return dir, nil + } + + var e [dataDirEntrySize]byte + if err := readFullAt(r, e[:], optOff+entryOff); err != nil { + return dir, fmt.Errorf("%w: reading security directory: %v", errNotPE, err) + } + dir.offset = binary.LittleEndian.Uint32(e[:4]) + dir.size = binary.LittleEndian.Uint32(e[4:]) + + return dir, nil } -func readSignerCert(r *bytes.Reader, off, size int64) (*sys.Cert, error) { +// readFullAt reads exactly len(buf) bytes at the given offset. +func readFullAt(r io.ReaderAt, buf []byte, off int64) error { + n, err := r.ReadAt(buf, off) + if n == len(buf) { + return nil + } + if err == nil || err == io.EOF { + err = io.ErrUnexpectedEOF + } + return err +} + +func readSignerCert(r io.ReaderAt, off, size int64) (*sys.Cert, error) { if size < winCertHeaderSize { return nil, errors.New("security directory too small") } var hdr [winCertHeaderSize]byte - if _, err := r.ReadAt(hdr[:], off); err != nil { + if err := readFullAt(r, hdr[:], off); err != nil { return nil, fmt.Errorf("read WIN_CERTIFICATE header: %w", err) } length := int64(binary.LittleEndian.Uint32(hdr[0:4])) @@ -105,65 +235,32 @@ func readSignerCert(r *bytes.Reader, off, size int64) (*sys.Cert, error) { } blob := make([]byte, length-winCertHeaderSize) - if _, err := r.ReadAt(blob, off+winCertHeaderSize); err != nil { + if err := readFullAt(r, blob, off+winCertHeaderSize); err != nil { return nil, fmt.Errorf("read certificate blob: %w", err) } - p7, err := pkcs7.Parse(blob) + // pkcs7.Parse isn't goroutine-safe, so it must go through the shared wrapper + p7, err := parsePKCS7(blob) if err != nil { - return nil, err + return nil, fmt.Errorf("parse PKCS#7 signature: %w", err) } if len(p7.Signers) == 0 { return nil, errors.New("signature has no signers") } - certInfo := &sys.Cert{} - serialNumber := p7.Signers[0].IssuerAndSerialNumber.SerialNumber + // The signer is identified by the issuer name AND the serial number. The + // serial number alone is unique only within a single issuer, and the + // certificates in the set aren't guaranteed to be ordered with the signer first. + signer := p7.Signers[0].IssuerAndSerialNumber + if signer.SerialNumber == nil { + return nil, errors.New("signer has no serial number") + } for _, cert := range p7.Certificates { - if !reflect.DeepEqual(cert.SerialNumber, serialNumber) { - continue + if cert.SerialNumber != nil && + cert.SerialNumber.Cmp(signer.SerialNumber) == 0 && + bytes.Equal(cert.RawIssuer, signer.IssuerName.FullBytes) { + return sys.NewCert(cert), nil } - - certInfo.SerialNumber = hex.EncodeToString(cert.SerialNumber.Bytes()) - - certInfo.NotAfter = cert.NotAfter - certInfo.NotBefore = cert.NotBefore - - // issuer information - if len(cert.Issuer.Country) > 0 { - certInfo.Issuer = cert.Issuer.Country[0] - } - - if len(cert.Issuer.Province) > 0 { - certInfo.Issuer += ", " + cert.Issuer.Province[0] - } - - if len(cert.Issuer.Locality) > 0 { - certInfo.Issuer += ", " + cert.Issuer.Locality[0] - } - - certInfo.Issuer += ", " + cert.Issuer.CommonName - - // subject information - if len(cert.Subject.Country) > 0 { - certInfo.Subject = cert.Subject.Country[0] - } - - if len(cert.Subject.Province) > 0 { - certInfo.Subject += ", " + cert.Subject.Province[0] - } - - if len(cert.Subject.Locality) > 0 { - certInfo.Subject += ", " + cert.Subject.Locality[0] - } - - if len(cert.Subject.Organization) > 0 { - certInfo.Subject += ", " + cert.Subject.Organization[0] - } - - certInfo.Subject += ", " + cert.Subject.CommonName - - return certInfo, nil } return nil, errors.New("signer certificate not found") diff --git a/pkg/util/signature/cert_test.go b/pkg/util/signature/cert_test.go index 0c1a699e6..6b882edfb 100644 --- a/pkg/util/signature/cert_test.go +++ b/pkg/util/signature/cert_test.go @@ -1,5 +1,3 @@ -//go:build windows - /* * Copyright 2021-present by Nedim Sabic Sabic * https://www.fibratus.io @@ -21,36 +19,628 @@ package signature import ( + "bytes" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + debugpe "debug/pe" + "encoding/binary" + "encoding/hex" + "errors" + "fmt" + "io" + "math/big" + "os" "path/filepath" + "strings" + "sync/atomic" "testing" + "time" + + "github.com/rabbitstack/fibratus/pkg/sys" + "go.mozilla.org/pkcs7" +) - "github.com/rabbitstack/fibratus/pkg/pe" +var ( + testNotBefore = time.Date(2024, 1, 2, 3, 4, 5, 0, time.UTC) + testNotAfter = time.Date(2034, 6, 7, 8, 9, 10, 0, time.UTC) ) -func TestReadSecurityMatchesParser(t *testing.T) { - t.Skip() - files, _ := filepath.Glob(`C:\Windows\System32\*.dll`) - if len(files) > 300 { - files = files[:300] +type identity struct { + cert *x509.Certificate + key *ecdsa.PrivateKey +} + +// newIdentity creates a certificate with the given name and serial. If parent +// is nil, the certificate is self-signed. +func newIdentity(t testing.TB, name pkix.Name, serial *big.Int, parent *identity) *identity { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + tmpl := &x509.Certificate{ + SerialNumber: serial, + Subject: name, + NotBefore: testNotBefore, + NotAfter: testNotAfter, + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageCodeSigning}, + BasicConstraintsValid: true, + IsCA: parent == nil, + } + issuer, issuerKey := tmpl, key + if parent != nil { + issuer, issuerKey = parent.cert, parent.key + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, issuer, &key.PublicKey, issuerKey) + if err != nil { + t.Fatal(err) + } + cert, err := x509.ParseCertificate(der) + if err != nil { + t.Fatal(err) + } + return &identity{cert: cert, key: key} +} + +func fullName(org, cn string) pkix.Name { + return pkix.Name{ + Country: []string{"US"}, + Province: []string{"WA"}, + Locality: []string{"Redmond"}, + Organization: []string{org}, + CommonName: cn, + } +} + +// signBlob produces the DER PKCS#7 SignedData signed by the signer. Any +// extra certificates are placed in the certificate set before the signer's. +func signBlob(t testing.TB, signer *identity, extra ...*x509.Certificate) []byte { + t.Helper() + sd, err := pkcs7.NewSignedData([]byte("authenticode")) + if err != nil { + t.Fatal(err) + } + sd.SetDigestAlgorithm(pkcs7.OIDDigestAlgorithmSHA256) + for _, c := range extra { + sd.AddCertificate(c) + } + if err := sd.AddSigner(signer.cert, signer.key, pkcs7.SignerInfoConfig{}); err != nil { + t.Fatal(err) + } + b, err := sd.Finish() + if err != nil { + t.Fatal(err) + } + return b +} + +const ntOffset = 0x80 // where the NT headers are placed in the synthetic images + +type segment struct { + off int64 + data []byte +} + +// image is a (possibly huge) file whose content is zero except for the +// segments. It implements io.ReaderAt and counts the I/O made against it. +type image struct { + segs []segment + size int64 + reads atomic.Int64 + read atomic.Int64 +} + +func (im *image) ReadAt(p []byte, off int64) (int, error) { + im.reads.Add(1) + if off < 0 { + return 0, errors.New("negative offset") + } + if off >= im.size { + return 0, io.EOF } - for _, path := range files { - old, err := pe.ParseFile(path, pe.WithSecurity()) - if err != nil || old == nil { + n := int64(len(p)) + var err error + if rem := im.size - off; n > rem { + n, err = rem, io.EOF + } + clear(p[:n]) + for _, s := range im.segs { + end := s.off + int64(len(s.data)) + if end <= off || s.off >= off+n { continue } + from, to := max(s.off, off), min(end, off+n) + copy(p[from-off:to-off], s.data[from-s.off:to-s.off]) + } + im.read.Add(n) + return int(n), err +} + +// bytes materializes the whole image in memory. +func (im *image) bytes() []byte { + buf := make([]byte, im.size) + _, _ = im.ReadAt(buf, 0) + im.reads.Store(0) + im.read.Store(0) + return buf +} + +// writeTo materializes the image on disk. The file is sparse where the +// file system supports it. +func (im *image) writeTo(t testing.TB, path string) { + t.Helper() + f, err := os.Create(path) + if err != nil { + t.Fatal(err) + } + defer f.Close() + if err := f.Truncate(im.size); err != nil { + t.Fatal(err) + } + for _, s := range im.segs { + if _, err := f.WriteAt(s.data, s.off); err != nil { + t.Fatal(err) + } + } +} + +const noDirectories = -1 // spec.numDirs value that requests zero data directories + +// spec describes the synthetic image. Zero values produce a well-formed +// PE32+ image. +type spec struct { + pe32 bool // 32-bit optional header layout + + numDirs int // NumberOfRvaAndSizes; 0 means 16, noDirectories means 0 + optSize uint16 // SizeOfOptionalHeader; 0 means natural size + optMagic uint16 // optional header magic; 0 means the one for the layout + dosMagic string // overrides "MZ" + ntSig uint32 // overrides "PE\0\0" + lfanewField uint32 // e_lfanew value; 0 means ntOffset + + table []byte // raw certificate table, the content of the security directory + padTo int64 // the table is placed at or after this file offset + dirOffset uint32 // overrides the directory address + dirSize uint32 // overrides the directory size + size int64 // minimum logical file size +} + +func buildImage(s spec) *image { + dirsOff, magic, machine := pe32PlusDataDirsOffset, uint16(optionalMagicPE32Plus), uint16(0x8664) + if s.pe32 { + dirsOff, magic, machine = pe32DataDirsOffset, optionalMagicPE32, 0x14c + } + if s.optMagic != 0 { + magic = s.optMagic + } + natural := dirsOff + 16*dataDirEntrySize + optSize := s.optSize + if optSize == 0 { + optSize = uint16(natural) + } + ndirs := uint32(16) + switch { + case s.numDirs > 0: + ndirs = uint32(s.numDirs) + case s.numDirs < 0: + ndirs = 0 + } + + head := make([]byte, ntOffset+4+fileHeaderSize+natural) + copy(head, "MZ") + if s.dosMagic != "" { + copy(head, s.dosMagic) + } + lfanew := s.lfanewField + if lfanew == 0 { + lfanew = ntOffset + } + binary.LittleEndian.PutUint32(head[dosNTHeaderOffset:], lfanew) + + sig := s.ntSig + if sig == 0 { + sig = ntSignature + } + binary.LittleEndian.PutUint32(head[ntOffset:], sig) + fh := head[ntOffset+4:] + binary.LittleEndian.PutUint16(fh, machine) + binary.LittleEndian.PutUint16(fh[fileHeaderOptSizeOff:], optSize) + opt := fh[fileHeaderSize:] + binary.LittleEndian.PutUint16(opt, magic) + binary.LittleEndian.PutUint32(opt[dirsOff-4:], ndirs) + + im := &image{segs: []segment{{0, head}}} + end := int64(len(head)) + + if len(s.table) > 0 { + off := max(int64(len(head)), s.padTo) + off = (off + 7) &^ 7 + im.segs = append(im.segs, segment{off, s.table}) + end = off + int64(len(s.table)) + + dirOffset, dirSize := uint32(off), uint32(len(s.table)) + if s.dirOffset != 0 { + dirOffset = s.dirOffset + } + if s.dirSize != 0 { + dirSize = s.dirSize + } + entry := opt[dirsOff+dataDirSecurity*dataDirEntrySize:] + binary.LittleEndian.PutUint32(entry, dirOffset) + binary.LittleEndian.PutUint32(entry[4:], dirSize) + } + im.size = max(end, s.size) + + return im +} + +// rawWinCert builds a WIN_CERTIFICATE with an arbitrary dwLength, padded to 8 bytes +// as the linker does. +func rawWinCert(length uint32, certType uint16, body []byte) []byte { + b := make([]byte, winCertHeaderSize+len(body)) + binary.LittleEndian.PutUint32(b, length) + binary.LittleEndian.PutUint16(b[4:], 0x0200) // WIN_CERT_REVISION_2_0 + binary.LittleEndian.PutUint16(b[6:], certType) + copy(b[winCertHeaderSize:], body) + if pad := (8 - len(b)%8) % 8; pad > 0 { + b = append(b, make([]byte, pad)...) + } + return b +} + +func winCert(blob []byte) []byte { + return rawWinCert(uint32(winCertHeaderSize+len(blob)), winCertTypePKCSSignedData, blob) +} + +func parse(im *image) (bool, *sys.Cert, error) { + return parseSecurityDirectoryAt(im, im.size) +} + +func wantCert(t *testing.T, got *sys.Cert, id *identity, issuer, subject string) { + t.Helper() + if got == nil { + t.Fatal("expected a certificate, got nil") + } + if want := hex.EncodeToString(id.cert.SerialNumber.Bytes()); got.SerialNumber != want { + t.Errorf("serial: got %q, want %q", got.SerialNumber, want) + } + if got.Issuer != issuer { + t.Errorf("issuer: got %q, want %q", got.Issuer, issuer) + } + if got.Subject != subject { + t.Errorf("subject: got %q, want %q", got.Subject, subject) + } + if !got.NotBefore.Equal(testNotBefore) || !got.NotAfter.Equal(testNotAfter) { + t.Errorf("validity: got [%s, %s], want [%s, %s]", got.NotBefore, got.NotAfter, testNotBefore, testNotAfter) + } +} + +func TestParseSecurityDirectoryUnsigned(t *testing.T) { + tests := []struct { + name string + spec spec + }{ + {"no certificate table", spec{}}, + {"no certificate table PE32", spec{pe32: true}}, + // the security entry is present but the loader only honours the first + // NumberOfRvaAndSizes entries + {"security entry beyond NumberOfRvaAndSizes", spec{table: winCert([]byte{1}), numDirs: 4}}, + {"no data directories", spec{table: winCert([]byte{1}), numDirs: noDirectories}}, + // the optional header can't hold the security entry + {"optional header too small for security entry", spec{table: winCert([]byte{1}), optSize: pe32PlusDataDirsOffset + 8}}, + {"optional header only holds the magic", spec{table: winCert([]byte{1}), optSize: 2}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + signed, cert, err := parse(buildImage(tt.spec)) + if err != nil || signed || cert != nil { + t.Fatalf("got (%t, %v, %v), want (false, nil, nil)", signed, cert, err) + } + }) + } +} + +func TestParseSecurityDirectorySigned(t *testing.T) { + signer := newIdentity(t, fullName("Fibratus Test Corp", "Fibratus Test Signer"), big.NewInt(0x7a7a7a7a7a7a7a7a), nil) + blob := signBlob(t, signer) + + for _, layout := range []struct { + name string + pe32 bool + }{{"PE32+", false}, {"PE32", true}} { + t.Run(layout.name, func(t *testing.T) { + signed, cert, err := parse(buildImage(spec{pe32: layout.pe32, table: winCert(blob)})) + if err != nil || !signed { + t.Fatalf("got (%t, _, %v), want (true, _, nil)", signed, err) + } + wantCert(t, cert, signer, + "US, WA, Redmond, Fibratus Test Signer", + "US, WA, Redmond, Fibratus Test Corp, Fibratus Test Signer") + }) + } +} + +// The certificate set isn't ordered with the signer first. The first +// certificate in the set is the issuing CA here, and it must not be reported. +func TestParseSecurityDirectoryPicksSignerNotFirstCertificate(t *testing.T) { + ca := newIdentity(t, fullName("Fibratus Test CA Org", "Fibratus Test CA"), big.NewInt(1), nil) + leaf := newIdentity(t, fullName("Fibratus Test Corp", "Fibratus Test Leaf"), big.NewInt(2), ca) + blob := signBlob(t, leaf, ca.cert) + + signed, cert, err := parse(buildImage(spec{table: winCert(blob)})) + if err != nil || !signed { + t.Fatalf("got (%t, _, %v), want (true, _, nil)", signed, err) + } + wantCert(t, cert, leaf, + "US, WA, Redmond, Fibratus Test CA", + "US, WA, Redmond, Fibratus Test Corp, Fibratus Test Leaf") +} + +// Serial numbers are only unique per issuer. Two certificates in the set +// share the serial, and the signer is the one issued by the second CA. +func TestParseSecurityDirectorySameSerialDifferentIssuer(t *testing.T) { + ca1 := newIdentity(t, fullName("CA One Org", "CA One"), big.NewInt(100), nil) + ca2 := newIdentity(t, fullName("CA Two Org", "CA Two"), big.NewInt(200), nil) + impostor := newIdentity(t, fullName("Impostor Corp", "Impostor"), big.NewInt(5), ca1) + signer := newIdentity(t, fullName("Real Corp", "Real Signer"), big.NewInt(5), ca2) + + blob := signBlob(t, signer, impostor.cert) + + signed, cert, err := parse(buildImage(spec{table: winCert(blob)})) + if err != nil || !signed { + t.Fatalf("got (%t, _, %v), want (true, _, nil)", signed, err) + } + wantCert(t, cert, signer, "US, WA, Redmond, CA Two", "US, WA, Redmond, Real Corp, Real Signer") +} + +// The strings are formatted as before the rewrite: a missing country leaves +// a leading separator, and the issuer doesn't include the organization. +func TestParseSecurityDirectoryCertFormatIsPreserved(t *testing.T) { + signer := newIdentity(t, pkix.Name{Organization: []string{"Org"}, CommonName: "CN"}, big.NewInt(9), nil) + + _, cert, err := parse(buildImage(spec{table: winCert(signBlob(t, signer))})) + if err != nil { + t.Fatal(err) + } + wantCert(t, cert, signer, ", CN", ", Org, CN") +} + +func TestParseSecurityDirectoryLargeImage(t *testing.T) { + signer := newIdentity(t, fullName("Big Corp", "Big Signer"), big.NewInt(77), nil) + table := winCert(signBlob(t, signer)) + + for _, size := range []int64{1 << 20, 8 << 20, 9 << 20, 64 << 20, 300 << 20} { + t.Run(fmt.Sprintf("%dMB", size>>20), func(t *testing.T) { + im := buildImage(spec{table: table, padTo: size}) + signed, cert, err := parse(im) + if err != nil || !signed { + t.Fatalf("got (%t, _, %v), want (true, _, nil)", signed, err) + } + wantCert(t, cert, signer, "US, WA, Redmond, Big Signer", "US, WA, Redmond, Big Corp, Big Signer") + + if limit := int64(len(table)) + 1024; im.read.Load() > limit { + t.Errorf("read %d bytes from a %d byte image, want at most %d", im.read.Load(), im.size, limit) + } + if im.reads.Load() > 12 { + t.Errorf("made %d reads, want a handful", im.reads.Load()) + } + }) + } +} + +func TestParseSecurityDirectoryFile(t *testing.T) { + signer := newIdentity(t, fullName("File Corp", "File Signer"), big.NewInt(88), nil) + table := winCert(signBlob(t, signer)) + + t.Run("signed file larger than 8MB", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "big.dll") + buildImage(spec{table: table, padTo: 12 << 20}).writeTo(t, path) + + signed, cert, err := parseSecurityDirectory(path) + if err != nil || !signed { + t.Fatalf("got (%t, _, %v), want (true, _, nil)", signed, err) + } + wantCert(t, cert, signer, "US, WA, Redmond, File Signer", "US, WA, Redmond, File Corp, File Signer") + }) + + t.Run("unsigned file", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "unsigned.dll") + buildImage(spec{}).writeTo(t, path) + + signed, cert, err := parseSecurityDirectory(path) + if err != nil || signed || cert != nil { + t.Fatalf("got (%t, %v, %v), want (false, nil, nil)", signed, cert, err) + } + }) + + t.Run("missing file", func(t *testing.T) { + signed, cert, err := parseSecurityDirectory(filepath.Join(t.TempDir(), "nope.dll")) + if !errors.Is(err, os.ErrNotExist) || signed || cert != nil { + t.Fatalf("got (%t, %v, %v), want (false, nil, ErrNotExist)", signed, cert, err) + } + }) + + t.Run("empty file", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "empty.dll") + if err := os.WriteFile(path, nil, 0o600); err != nil { + t.Fatal(err) + } signed, cert, err := parseSecurityDirectory(path) + if !errors.Is(err, errNotPE) || signed || cert != nil { + t.Fatalf("got (%t, %v, %v), want (false, nil, errNotPE)", signed, cert, err) + } + }) +} + +func TestParseSecurityDirectoryMalformed(t *testing.T) { + signer := newIdentity(t, fullName("Corp", "Signer"), big.NewInt(3), nil) + blob := signBlob(t, signer) + good := winCert(blob) + + tests := []struct { + name string + img *image + wantSigned bool // the image has a certificate table + wantErr string // substring of the error + wantNotPE bool + }{ + // not a usable PE: nothing can be said about a signature + {name: "10 bytes", img: &image{segs: []segment{{0, []byte("MZ12345678")}}, size: 10}, wantErr: "reading DOS header", wantNotPE: true}, + {name: "bad DOS magic", img: buildImage(spec{dosMagic: "ZM"}), wantErr: "bad DOS signature", wantNotPE: true}, + {name: "e_lfanew beyond EOF", img: buildImage(spec{lfanewField: 0x7fffffff}), wantErr: "reading NT headers", wantNotPE: true}, + {name: "bad NT signature", img: buildImage(spec{ntSig: 0x00004551}), wantErr: "bad NT signature", wantNotPE: true}, + {name: "no optional header", img: buildImage(spec{optSize: 1}), wantErr: "no optional header", wantNotPE: true}, + {name: "unknown optional magic", img: buildImage(spec{optMagic: 0x107}), wantErr: "unknown optional header magic", wantNotPE: true}, + + // a certificate table exists, but its content is unusable. The image + // is still signed, and the caller has to verify it as such + {name: "directory beyond EOF", img: buildImage(spec{table: good, dirOffset: 0x10000000}), wantSigned: true, wantErr: "exceeds file size"}, + {name: "directory size beyond EOF", img: buildImage(spec{table: good, dirSize: 0x10000000}), wantSigned: true, wantErr: "exceeds file size"}, + {name: "directory smaller than header", img: buildImage(spec{table: good, dirSize: 4}), wantSigned: true, wantErr: "too small"}, + {name: "unsupported certificate type", img: buildImage(spec{table: rawWinCert(uint32(winCertHeaderSize+len(blob)), 0x0001, blob)}), wantSigned: true, wantErr: "unsupported certificate type"}, + {name: "length larger than directory", img: buildImage(spec{table: rawWinCert(uint32(len(good))+64, winCertTypePKCSSignedData, blob)}), wantSigned: true, wantErr: "invalid certificate length"}, + {name: "length smaller than header", img: buildImage(spec{table: rawWinCert(4, winCertTypePKCSSignedData, blob)}), wantSigned: true, wantErr: "invalid certificate length"}, + {name: "blob too large", img: buildImage(spec{ + table: rawWinCert(winCertHeaderSize+maxCertBlobSize+1, winCertTypePKCSSignedData, nil), + dirSize: winCertHeaderSize + maxCertBlobSize + 1, + size: 16 << 20, + }), wantSigned: true, wantErr: "too large"}, + {name: "empty blob", img: buildImage(spec{table: rawWinCert(winCertHeaderSize, winCertTypePKCSSignedData, nil)}), wantSigned: true, wantErr: "parse PKCS#7"}, + {name: "garbage blob", img: buildImage(spec{table: winCert(bytes.Repeat([]byte{0xde, 0xad, 0xbe, 0xef}, 64))}), wantSigned: true, wantErr: "parse PKCS#7"}, + {name: "truncated blob", img: buildImage(spec{table: winCert(blob[:len(blob)/2])}), wantSigned: true, wantErr: "parse PKCS#7"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + signed, cert, err := parse(tt.img) + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("error: got %v, want it to contain %q", err, tt.wantErr) + } + if signed != tt.wantSigned { + t.Errorf("signed: got %t, want %t", signed, tt.wantSigned) + } + if cert != nil { + t.Errorf("certificate: got %+v, want nil", cert) + } + if errors.Is(err, errNotPE) != tt.wantNotPE { + t.Errorf("errors.Is(err, errNotPE): got %t, want %t", !tt.wantNotPE, tt.wantNotPE) + } + }) + } +} + +func TestParseSecurityDirectorySignatureWithoutSigners(t *testing.T) { + sd, err := pkcs7.NewSignedData([]byte("authenticode")) + if err != nil { + t.Fatal(err) + } + blob, err := sd.Finish() + if err != nil { + t.Fatal(err) + } + + signed, cert, err := parse(buildImage(spec{table: winCert(blob)})) + if !signed || cert != nil || err == nil || !strings.Contains(err.Error(), "no signers") { + t.Fatalf("got (%t, %v, %v), want (true, nil, \"no signers\")", signed, cert, err) + } +} + +// The signer's issuer and serial identify a certificate that isn't in the set. +func TestParseSecurityDirectorySignerCertificateMissing(t *testing.T) { + signer := newIdentity(t, fullName("Corp", "Signer"), big.NewInt(0x5b5b5b5b5b5b5b5b), nil) + blob := signBlob(t, signer) + + // The serial appears twice: in the certificate, and in the SignerInfo that + // follows the certificate set. Corrupt the last occurrence only. + needle := bytes.Repeat([]byte{0x5b}, 8) + if bytes.Count(blob, needle) != 2 { + t.Fatalf("expected the serial to occur twice, got %d", bytes.Count(blob, needle)) + } + i := bytes.LastIndex(blob, needle) + copy(blob[i:], bytes.Repeat([]byte{0x5c}, 8)) + + signed, cert, err := parse(buildImage(spec{table: winCert(blob)})) + if !signed || cert != nil || err == nil || !strings.Contains(err.Error(), "signer certificate not found") { + t.Fatalf("got (%t, %v, %v), want (true, nil, \"signer certificate not found\")", signed, cert, err) + } +} + +func TestParseSecurityDirectoryReaderErrors(t *testing.T) { + signer := newIdentity(t, fullName("Corp", "Signer"), big.NewInt(4), nil) + blob := signBlob(t, signer) + + t.Run("reader failing after the headers", func(t *testing.T) { + im := buildImage(spec{table: winCert(blob)}) + tableOff := im.segs[1].off + boom := errors.New("boom") + r := readerFunc(func(p []byte, off int64) (int, error) { + if off >= tableOff { + return 0, boom + } + return im.ReadAt(p, off) + }) + signed, cert, err := parseSecurityDirectoryAt(r, im.size) + if !errors.Is(err, boom) || !signed || cert != nil { + t.Fatalf("got (%t, %v, %v), want (true, nil, boom)", signed, cert, err) + } + }) + + t.Run("EOF together with a full read", func(t *testing.T) { + // the signature blob ends exactly at the end of the file (no padding), + // and the reader reports io.EOF alongside the last bytes, as os.File may + im := buildImage(spec{table: winCert(blob)[:winCertHeaderSize+len(blob)]}) + r := readerFunc(func(p []byte, off int64) (int, error) { + n, err := im.ReadAt(p, off) + if err == nil && off+int64(n) == im.size { + err = io.EOF + } + return n, err + }) + signed, cert, err := parseSecurityDirectoryAt(r, im.size) + if err != nil || !signed || cert == nil { + t.Fatalf("got (%t, %v, %v), want a certificate", signed, cert, err) + } + }) +} + +type readerFunc func(p []byte, off int64) (int, error) + +func (f readerFunc) ReadAt(p []byte, off int64) (int, error) { return f(p, off) } + +func TestSecurityDirectoryMatchesDebugPE(t *testing.T) { + table := winCert(signBlob(t, newIdentity(t, fullName("Corp", "Signer"), big.NewInt(6), nil))) + + for _, s := range []spec{ + {table: table}, + {table: table, pe32: true}, + {table: table, padTo: 3 << 20}, + {table: table, pe32: true, padTo: 5<<20 + 3}, + {}, + {pe32: true}, + } { + im := buildImage(s) + f, err := debugpe.NewFile(bytes.NewReader(im.bytes())) if err != nil { - t.Errorf("%s: %v", path, err) - continue + t.Fatalf("debug/pe rejected the synthetic image: %v", err) } - if old.IsSigned() != signed { - t.Errorf("%s: signed old=%t new=%t", path, old.IsSigned(), signed) + + var want debugpe.DataDirectory + switch oh := f.OptionalHeader.(type) { + case *debugpe.OptionalHeader32: + want = oh.DataDirectory[debugpe.IMAGE_DIRECTORY_ENTRY_SECURITY] + case *debugpe.OptionalHeader64: + want = oh.DataDirectory[debugpe.IMAGE_DIRECTORY_ENTRY_SECURITY] + } + + got, err := securityDirectory(im) + if err != nil { + t.Fatal(err) } - if old.Cert != nil && (cert == nil || - old.Cert.Subject != cert.Subject || - old.Cert.Issuer != cert.Issuer || - old.Cert.SerialNumber != cert.SerialNumber) { - t.Errorf("%s: cert mismatch old=%+v new=%+v err=%v", path, old.Cert, cert, err) + if got.offset != want.VirtualAddress || got.size != want.Size { + t.Errorf("spec %+v: got {%d %d}, debug/pe says {%d %d}", s.pe32, got.offset, got.size, want.VirtualAddress, want.Size) } } } From 234d23cae81aadb222b71eb03011f63c8df84d5d Mon Sep 17 00:00:00 2001 From: rabbitstack Date: Mon, 19 Oct 2026 15:48:51 +0200 Subject: [PATCH 13/13] test(signature): Cover correctness and bug fixes --- pkg/util/signature/signature_test.go | 321 +++++++++++++++++++++++++++ 1 file changed, 321 insertions(+) diff --git a/pkg/util/signature/signature_test.go b/pkg/util/signature/signature_test.go index adfa2fe9e..2851d522a 100644 --- a/pkg/util/signature/signature_test.go +++ b/pkg/util/signature/signature_test.go @@ -19,6 +19,8 @@ package signature import ( + "expvar" + "fmt" "os" "path/filepath" "sync" @@ -34,6 +36,25 @@ func systemDir(path string) string { return filepath.Join(os.Getenv("windir"), "system32", path) } +// delta returns a function reporting how much the counter grew since the call. +func delta(v *expvar.Int) func() int64 { + before := v.Value() + return func() int64 { return v.Value() - before } +} + +func mapTotal(m *expvar.Map) (n int64) { + m.Do(func(kv expvar.KeyValue) { + if v, ok := kv.Value.(*expvar.Int); ok { + n += v.Value() + } + }) + return n +} + +func missingModule(name string) Key { + return MakeKey(`c:\fibratus-test\missing\`+name+`.dll`, 0x1000, 0xABCD, 0x12345678) +} + // wellKnownDLLs are catalog-signed system DLLs present on every // supported Windows version. They provide a reliable test corpus // without requiring specific test fixtures. @@ -665,6 +686,306 @@ func TestDoRequestAsyncDropsWhenQueueFull(t *testing.T) { } } +// bareStore builds a store without workers or the GC loop, so a test decides +// what is consumed from the queues and when. The signature checks run against +// paths that don't exist: the check fails fast, and the result is still stored +// and counted, which is all these tests need. +func bareStore(t *testing.T, queueSize int) *Signatures { + t.Helper() + s := &Signatures{ + signatures: make(map[Key]*Signature), + purger: time.NewTicker(time.Hour), + requests: make(chan Request, queueSize), + certs: make(chan Request, queueSize), + inline: make(chan struct{}, maxInlineChecks), + stop: make(chan struct{}), + } + t.Cleanup(s.purger.Stop) + return s +} + +func TestGetOrCheckRunsOneCheckPerKey(t *testing.T) { + s := bareStore(t, 1) + checks := delta(signatureCacheMisses) + stored := delta(signatureCount) + + // Every round is a new key hit by concurrent callers released together. + // A check that is repeated because a caller missed the cache after the + // singleflight call finished, but before the result was stored, shows up + // as more than one check per key. + const rounds, callers = 200, 16 + for r := 0; r < rounds; r++ { + key := missingModule(fmt.Sprintf("round%d", r)) + + var wg sync.WaitGroup + start := make(chan struct{}) + results := make([]*Signature, callers) + for i := 0; i < callers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + <-start + results[i] = s.resolve(key) + }() + } + close(start) + wg.Wait() + + for i, sig := range results { + if sig == nil || sig != results[0] { + t.Fatalf("round %d: caller %d got a different signature than caller 0", r, i) + } + } + } + + if got := checks(); got != rounds { + t.Errorf("ran %d checks for %d keys, want one per key", got, rounds) + } + if got := stored(); got != rounds { + t.Errorf("signature count grew by %d, want %d", got, rounds) + } + if len(s.signatures) != rounds { + t.Errorf("store holds %d signatures, want %d", len(s.signatures), rounds) + } +} + +func TestGetOrCheckOnlyCountsRealCacheHits(t *testing.T) { + s := bareStore(t, 1) + key := missingModule("hits") + hits := delta(signatureCacheHits) + + first := s.resolve(key) + // the internal re-check inside the singleflight callback is not a hit + if got := hits(); got != 0 { + t.Fatalf("a first lookup produced %d cache hits, want 0", got) + } + + if second := s.resolve(key); second != first { + t.Fatal("the second lookup must return the stored signature") + } + if got := hits(); got != 1 { + t.Fatalf("got %d cache hits after the second lookup, want 1", got) + } +} + +func TestDoRequestAsyncDedupesPendingKeys(t *testing.T) { + s := bareStore(t, 8) + key := missingModule("async") + deduped := delta(signatureAsyncRequestDeduped) + + const goroutines = 32 + var wg sync.WaitGroup + for i := 0; i < goroutines; i++ { + wg.Add(1) + go func() { + defer wg.Done() + s.DoRequestAsync(key) + }() + } + wg.Wait() + + if got := len(s.requests); got != 1 { + t.Fatalf("%d requests were enqueued for the same key, want 1", got) + } + if got := deduped(); got != goroutines-1 { + t.Errorf("deduped %d requests, want %d", got, goroutines-1) + } + if _, ok := s.pending.Load(key); !ok { + t.Error("the key must stay claimed while its request is queued") + } + + // the worker finishes the request: the result is stored and the claim released + s.processRequest(<-s.requests) + + if s.peek(key) == nil { + t.Fatal("the signature must be stored after the request is processed") + } + if _, ok := s.pending.Load(key); ok { + t.Error("the claim must be released once the signature is stored") + } + + // a resolved module is answered from the store, nothing is enqueued + s.DoRequestAsync(key) + if got := len(s.requests); got != 0 { + t.Errorf("a resolved key was enqueued again (%d queued)", got) + } +} + +func TestDoRequestAsyncReleasesClaimWhenQueueIsFull(t *testing.T) { + s := bareStore(t, 1) + drops := delta(signatureAsyncRequestDrops) + + first, second := missingModule("first"), missingModule("second") + s.DoRequestAsync(first) // fills the queue + s.DoRequestAsync(second) + + if got := drops(); got != 1 { + t.Errorf("got %d drops, want 1", got) + } + if _, ok := s.pending.Load(second); ok { + t.Fatal("a dropped request must not keep its key claimed, or the module is never checked again") + } + + // once the queue has room, a later event for the same module goes through + <-s.requests + s.DoRequestAsync(second) + if got := len(s.requests); got != 1 { + t.Errorf("the retry was not enqueued (%d queued)", got) + } +} + +func TestPutSignatureReleasesPendingClaim(t *testing.T) { + s := bareStore(t, 8) + key := missingModule("put") + + s.DoRequestAsync(key) + if _, ok := s.pending.Load(key); !ok { + t.Fatal("the key should be claimed") + } + + s.PutSignature(key, TypeEmbedded, LevelWindows) + + if _, ok := s.pending.Load(key); ok { + t.Error("PutSignature must release the claim") + } + if sig := s.peek(key); sig == nil || !sig.IsTrusted() { + t.Error("PutSignature must store a trusted signature") + } +} + +func TestDoRequestInlineFallbackIsBounded(t *testing.T) { + // an unbuffered queue without a reader is always full + s := bareStore(t, 0) + key := missingModule("inline") + drops := delta(signatureAsyncRequestDrops) + + // all the inline slots are taken + for i := 0; i < maxInlineChecks; i++ { + s.inline <- struct{}{} + } + + done := make(chan *Signature, 1) + go func() { done <- s.DoRequest(key) }() + + select { + case <-done: + t.Fatal("the caller ran a check while every inline slot was taken") + case <-time.After(100 * time.Millisecond): + } + if s.peek(key) != nil { + t.Fatal("no check must have run while the caller waits for a slot") + } + + <-s.inline // free one slot + + select { + case sig := <-done: + if sig == nil || sig.Path != key.Path { + t.Fatalf("unexpected signature %+v", sig) + } + case <-time.After(5 * time.Second): + t.Fatal("the caller did not proceed after a slot was freed") + } + if s.peek(key) == nil { + t.Error("the inline check must store its result") + } + if got := len(s.inline); got != maxInlineChecks-1 { + t.Errorf("%d slots are taken, want %d: the caller must give its slot back", got, maxInlineChecks-1) + } + if drops() < 1 { + t.Error("the full queue must be counted as a drop") + } +} + +func TestDoRequestInlineFallbackGivesUpOnClose(t *testing.T) { + s := bareStore(t, 0) + key := missingModule("closing") + for i := 0; i < maxInlineChecks; i++ { + s.inline <- struct{}{} + } + + done := make(chan *Signature, 1) + go func() { done <- s.DoRequest(key) }() + time.Sleep(50 * time.Millisecond) + close(s.stop) + + select { + case sig := <-done: + if sig == nil || sig.Path != key.Path || sig.Exists() || sig.Type() != TypeNone { + t.Fatalf("a caller released by shutdown must get an unchecked signature, got %+v", sig) + } + case <-time.After(5 * time.Second): + t.Fatal("the caller stayed blocked after the store was closed") + } + if s.peek(key) != nil { + t.Error("an unchecked placeholder must not be cached") + } +} + +func TestDoRequestInlineFallbackUnderLoad(t *testing.T) { + s := bareStore(t, 0) + checks := delta(signatureCacheMisses) + + const keys = 64 + var wg sync.WaitGroup + for i := 0; i < keys; i++ { + key := missingModule(fmt.Sprintf("load%d", i%keys)) + // two callers per key + for j := 0; j < 2; j++ { + wg.Add(1) + go func() { + defer wg.Done() + if sig := s.DoRequest(key); sig == nil { + t.Error("DoRequest returned nil") + } + }() + } + } + wg.Wait() + + if got := checks(); got != keys { + t.Errorf("ran %d checks for %d keys", got, keys) + } + if got := len(s.inline); got != 0 { + t.Errorf("%d inline slots leaked", got) + } +} + +func TestWorkerParsesCertificatesOfPutSignatures(t *testing.T) { + s := bareStore(t, 8) + t.Cleanup(func() { close(s.stop) }) + + total := mapTotal(signatureCertParseErrors) + nilErrs := int64(0) + if v, ok := signatureCertParseErrors.Get(ErrNilSignature.Error()).(*expvar.Int); ok { + nilErrs = v.Value() + } + + // a cert request whose signature is gone from the store (evicted or + // invalidated) must be skipped, not parsed as a nil signature + s.certs <- Request{Key: missingModule("evicted")} + // a request for a stored signature must be parsed. The module doesn't + // exist, so the parse fails and lands in the error map + s.PutSignature(missingModule("stored"), TypeEmbedded, LevelWindows) + + go s.runWorker() + + // the only worker handles the requests in order, so once the stored one + // has been parsed, the evicted one is behind us + deadline := time.Now().Add(5 * time.Second) + for mapTotal(signatureCertParseErrors) == total && time.Now().Before(deadline) { + time.Sleep(5 * time.Millisecond) + } + time.Sleep(50 * time.Millisecond) + + if v, ok := signatureCertParseErrors.Get(ErrNilSignature.Error()).(*expvar.Int); ok && v.Value() != nilErrs { + t.Error("the worker parsed the certificate of a signature that is not in the store") + } + if got := mapTotal(signatureCertParseErrors) - total; got != 1 { + t.Errorf("got %d certificate parse attempts, want exactly 1, for the stored signature", got) + } +} + func isWintrustAvailable() bool { return sys.IsWintrustFound() }