From 3741ca345ff717477765a80f0fefdc0222d375cd Mon Sep 17 00:00:00 2001 From: moonD4rk Date: Sat, 25 Apr 2026 21:46:49 +0800 Subject: [PATCH 1/2] refactor(keyretriever): reduce duplicate WARN logs from master-key chain - Demote in-chain retriever failures to Debug - GcoredumpRetriever logs at Debug and falls through silently - Dedupe master-key WARN per browser instead of per profile - Clarify "exit status 128 ()" with empty stderr message Closes #588 --- browser/chromium/chromium.go | 29 ++++++++-------------- crypto/keyretriever/keyretriever.go | 2 +- crypto/keyretriever/keyretriever_darwin.go | 28 ++++++++++++++++----- 3 files changed, 34 insertions(+), 25 deletions(-) diff --git a/browser/chromium/chromium.go b/browser/chromium/chromium.go index 50e56776..c475d725 100644 --- a/browser/chromium/chromium.go +++ b/browser/chromium/chromium.go @@ -3,6 +3,7 @@ package chromium import ( "os" "path/filepath" + "sync" "time" "github.com/moond4rk/hackbrowserdata/crypto/keyretriever" @@ -51,17 +52,7 @@ func NewBrowsers(cfg types.BrowserConfig) ([]*Browser, error) { return browsers, nil } -// SetKeyRetrievers wires the per-tier master-key retrievers used by Extract. Each slot -// (V10 / V11 / V20) is populated only on platforms where that cipher tier is used: -// -// - Windows: V10 (DPAPI) + V20 (ABE). V11 nil — Chromium does not emit v11 prefix on Windows. -// - Linux: V10 ("peanuts" kV10Key) + V11 (D-Bus Secret Service kV11Key). V20 nil. -// - macOS: V10 (Keychain chain). V11 and V20 nil. -// -// Slots are independent — a failure or absence in one tier does not affect others. A single -// Chromium profile can carry mixed cipher-prefix ciphertexts (the motivation for issue #578), so -// every configured retriever runs at extract time and decryptValue picks the matching key per -// ciphertext. +// SetKeyRetrievers wires the per-tier master-key retrievers (V10/V11/V20) used by Extract; unused tiers stay nil. func (b *Browser) SetKeyRetrievers(r keyretriever.Retrievers) { b.retrievers = r } @@ -178,12 +169,10 @@ func (b *Browser) acquireFiles(session *filemanager.Session, categories []types. return tempPaths } -// getMasterKeys retrieves the Chromium master keys for every configured tier. Chrome mixes -// cipher tiers on the same profile — v20 for new cookies alongside v10 passwords on Windows; v10 -// (peanuts) alongside v11 (keyring) on Linux after session-mode changes — so every retriever in -// b.retrievers runs independently and keyretriever.NewMasterKeys assembles the results. Any tier -// key may be nil if its retriever failed or is not configured for this platform; decryptValue -// treats a missing tier key as "that tier cannot decrypt" so partial success is still reported. +// warnedMasterKeyFailure dedupes "master key retrieval" WARN per browser; profiles share one Safe Storage entry. +var warnedMasterKeyFailure sync.Map + +// getMasterKeys retrieves master keys for all configured cipher tiers. func (b *Browser) getMasterKeys(session *filemanager.Session) keyretriever.MasterKeys { label := b.BrowserName() + "/" + b.ProfileName() @@ -207,7 +196,11 @@ func (b *Browser) getMasterKeys(session *filemanager.Session) keyretriever.Maste keys, err := keyretriever.NewMasterKeys(b.retrievers, b.cfg.Storage, localStateDst) if err != nil { - log.Warnf("%s: master key retrieval: %v", label, err) + if _, already := warnedMasterKeyFailure.LoadOrStore(b.BrowserName(), struct{}{}); !already { + log.Warnf("%s: master key retrieval: %v", b.BrowserName(), err) + } else { + log.Debugf("%s: master key retrieval: %v", label, err) + } } return keys } diff --git a/crypto/keyretriever/keyretriever.go b/crypto/keyretriever/keyretriever.go index c6472e22..29a5f2ad 100644 --- a/crypto/keyretriever/keyretriever.go +++ b/crypto/keyretriever/keyretriever.go @@ -48,7 +48,7 @@ func (c *ChainRetriever) RetrieveKey(storage, localStatePath string) ([]byte, er return key, nil } if err != nil { - log.Warnf("keyretriever %T failed: %v", r, err) + log.Debugf("keyretriever %T failed: %v", r, err) errs = append(errs, fmt.Errorf("%T: %w", r, err)) } } diff --git a/crypto/keyretriever/keyretriever_darwin.go b/crypto/keyretriever/keyretriever_darwin.go index ecc0d570..31333325 100644 --- a/crypto/keyretriever/keyretriever_darwin.go +++ b/crypto/keyretriever/keyretriever_darwin.go @@ -14,6 +14,8 @@ import ( "time" "github.com/moond4rk/keychainbreaker" + + "github.com/moond4rk/hackbrowserdata/log" ) // https://source.chromium.org/chromium/chromium/src/+/master:components/os_crypt/os_crypt_mac.mm;l=157 @@ -37,18 +39,25 @@ type GcoredumpRetriever struct { err error } +// RetrieveKey logs internal failures at Debug and returns (nil, nil) so ChainRetriever falls +// through to the next retriever silently. The most common failure ("requires root privileges") +// is documented expected behavior, not a warning-worthy condition; surfacing it on every profile +// would drown out genuine warnings. The same pattern is used by ABERetriever (see abe_windows.go). func (r *GcoredumpRetriever) RetrieveKey(storage, _ string) ([]byte, error) { r.once.Do(func() { r.records, r.err = DecryptKeychainRecords() - if r.err != nil { - r.err = fmt.Errorf("gcoredump: %w", r.err) - } }) if r.err != nil { - return nil, r.err + log.Debugf("gcoredump: %v", r.err) + return nil, nil //nolint:nilerr // intentional silent fallthrough } - return findStorageKey(r.records, storage) + key, err := findStorageKey(r.records, storage) + if err != nil { + log.Debugf("gcoredump: %v", err) + return nil, nil //nolint:nilerr // intentional silent fallthrough + } + return key, nil } // loadKeychainRecords opens login.keychain-db and unlocks it with the given @@ -141,7 +150,14 @@ func (r *SecurityCmdRetriever) retrieveKeyOnce(storage string) ([]byte, error) { if errors.Is(ctx.Err(), context.DeadlineExceeded) { return nil, fmt.Errorf("security command timed out after %s", securityCmdTimeout) } - return nil, fmt.Errorf("security command: %w (%s)", err, strings.TrimSpace(stderr.String())) + // `security find-generic-password` exits non-zero with empty stderr when the user denies + // the keychain access prompt or enters the wrong password. Surface that explicitly so the + // error message is actionable instead of the cryptic "exit status 128 ()". + stderrStr := strings.TrimSpace(stderr.String()) + if stderrStr == "" { + return nil, fmt.Errorf("security command: %w (likely keychain access denied or wrong password)", err) + } + return nil, fmt.Errorf("security command: %w (%s)", err, stderrStr) } if stderr.Len() > 0 { return nil, fmt.Errorf("keychain: %s", strings.TrimSpace(stderr.String())) From 9386ca6ee03137cd130bf5e52f26aa33f847eefc Mon Sep 17 00:00:00 2001 From: moonD4rk Date: Sat, 25 Apr 2026 21:57:37 +0800 Subject: [PATCH 2/2] refactor(chromium): dedupe master-key WARN per installation, not just per browser name Address Copilot review: glob-expanded configs (e.g., MSIX/UWP browsers with publisher hash suffixes) yield multiple installations sharing a BrowserName but with different UserDataDir. Include UserDataDir in the dedupe key so each installation still emits its first WARN. --- browser/chromium/chromium.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/browser/chromium/chromium.go b/browser/chromium/chromium.go index c475d725..96dee9d1 100644 --- a/browser/chromium/chromium.go +++ b/browser/chromium/chromium.go @@ -169,7 +169,8 @@ func (b *Browser) acquireFiles(session *filemanager.Session, categories []types. return tempPaths } -// warnedMasterKeyFailure dedupes "master key retrieval" WARN per browser; profiles share one Safe Storage entry. +// warnedMasterKeyFailure dedupes "master key retrieval" WARN per installation (BrowserName + UserDataDir); +// profiles share one Safe Storage entry, but glob-expanded configs may yield multiple installations of the same browser. var warnedMasterKeyFailure sync.Map // getMasterKeys retrieves master keys for all configured cipher tiers. @@ -196,7 +197,8 @@ func (b *Browser) getMasterKeys(session *filemanager.Session) keyretriever.Maste keys, err := keyretriever.NewMasterKeys(b.retrievers, b.cfg.Storage, localStateDst) if err != nil { - if _, already := warnedMasterKeyFailure.LoadOrStore(b.BrowserName(), struct{}{}); !already { + installKey := b.BrowserName() + "|" + b.cfg.UserDataDir + if _, already := warnedMasterKeyFailure.LoadOrStore(installKey, struct{}{}); !already { log.Warnf("%s: master key retrieval: %v", b.BrowserName(), err) } else { log.Debugf("%s: master key retrieval: %v", label, err)