Skip to content

fix: windows decrypt patch#1227

Merged
Nikhil (shadowfax92) merged 1 commit into
mainfrom
fix/unship-windows-updater-1
Jun 16, 2026
Merged

fix: windows decrypt patch#1227
Nikhil (shadowfax92) merged 1 commit into
mainfrom
fix/unship-windows-updater-1

Conversation

@shadowfax92

Copy link
Copy Markdown
Contributor

No description provided.

@shadowfax92 Nikhil (shadowfax92) merged commit 9484034 into main Jun 16, 2026
3 of 4 checks passed
@github-actions github-actions Bot added the fix label Jun 16, 2026
@greptile-apps

greptile-apps Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This patch replaces the Chromium-provided base/win/wincrypt_shim.h include with direct <windows.h> + <wincrypt.h> system includes in the Windows Chrome decryptor implementation. The rest of the DPAPI + AES-256-GCM decryption logic is unchanged.

  • The shim removal risks macro-conflict compilation failures: <wincrypt.h> defines symbols (X509_NAME, X509_CERT_PAIR, etc.) that collide with BoringSSL definitions pulled in transitively through crypto/aead.h.
  • DecryptWithDpapi leaks the DPAPI output buffer when CryptUnprotectData succeeds but returns cbData == 0LocalFree is never called in that branch.
  • A LOG(INFO) on the non-v10 decryption path fires for every unencrypted stored value and should be removed or demoted per the team's logging guidelines.

Confidence Score: 3/5

The include swap may re-introduce BoringSSL macro conflicts that the shim was designed to prevent; the decryption path should be validated against the exact Chromium target before merging.

The sole code change — swapping wincrypt_shim.h for direct Win32 headers — bypasses the Chromium shim that undefines wincrypt.h macros conflicting with BoringSSL. Because crypto/aead.h is included in the same translation unit, a BoringSSL macro collision could silently break compilation in certain build configurations. Additionally, the existing DecryptWithDpapi function leaks the DPAPI output buffer on a valid-but-empty result from CryptUnprotectData.

The single changed patch file chrome_decryptor_win.cc needs attention — specifically the include change and the DPAPI buffer-free logic.

Important Files Changed

Filename Overview
packages/browseros/chromium_patches/chrome/utility/importer/browseros/chrome_decryptor_win.cc Changes wincrypt_shim.h to direct <windows.h> + <wincrypt.h> includes; shim removal can introduce BoringSSL macro conflicts since crypto/aead.h is also included. Existing DecryptWithDpapi has a memory leak when DPAPI returns a non-null but zero-length buffer.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[DecryptChromeValue called] --> B{Has v10 prefix?}
    B -- No --> C[Return ciphertext as-is + LOG INFO]
    B -- Yes --> D[Strip v10 prefix]
    D --> E[DecryptAesGcm]
    E --> F{Key valid 32 bytes?}
    F -- No --> G[Return false]
    F -- Yes --> H{Ciphertext long enough?}
    H -- No --> I[Return false]
    H -- Yes --> J[Extract nonce first 12 bytes]
    J --> K[crypto::Aead AES_256_GCM Open]
    K --> L{Decrypt OK?}
    L -- No --> M[Return false]
    L -- Yes --> N[Return plaintext]

    subgraph ExtractChromeKey
        P[GetLocalStatePath] --> Q{Local State exists?}
        Q -- No --> R[kLocalStateNotFound]
        Q -- Yes --> S[Parse JSON / FindDict os_crypt]
        S --> T{Key found?}
        T -- No --> U[kLocalStateParseError]
        T -- Yes --> V[Base64 decode]
        V --> W[DecryptWithDpapi]
        W --> X{CryptUnprotectData OK?}
        X -- No --> Y[kDpapiDecryptFailed]
        X -- Yes --> Z{cbData > 0?}
        Z -- No --> AA[Memory leak — LocalFree skipped]
        Z -- Yes --> AB[LocalFree + return key]
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[DecryptChromeValue called] --> B{Has v10 prefix?}
    B -- No --> C[Return ciphertext as-is + LOG INFO]
    B -- Yes --> D[Strip v10 prefix]
    D --> E[DecryptAesGcm]
    E --> F{Key valid 32 bytes?}
    F -- No --> G[Return false]
    F -- Yes --> H{Ciphertext long enough?}
    H -- No --> I[Return false]
    H -- Yes --> J[Extract nonce first 12 bytes]
    J --> K[crypto::Aead AES_256_GCM Open]
    K --> L{Decrypt OK?}
    L -- No --> M[Return false]
    L -- Yes --> N[Return plaintext]

    subgraph ExtractChromeKey
        P[GetLocalStatePath] --> Q{Local State exists?}
        Q -- No --> R[kLocalStateNotFound]
        Q -- Yes --> S[Parse JSON / FindDict os_crypt]
        S --> T{Key found?}
        T -- No --> U[kLocalStateParseError]
        T -- Yes --> V[Base64 decode]
        V --> W[DecryptWithDpapi]
        W --> X{CryptUnprotectData OK?}
        X -- No --> Y[kDpapiDecryptFailed]
        X -- Yes --> Z{cbData > 0?}
        Z -- No --> AA[Memory leak — LocalFree skipped]
        Z -- Yes --> AB[LocalFree + return key]
    end
Loading

Comments Outside Diff (1)

  1. packages/browseros/chromium_patches/chrome/utility/importer/browseros/chrome_decryptor_win.cc, line 112-127 (link)

    P1 Memory leak when DPAPI returns empty output

    CryptUnprotectData can succeed (return TRUE) but produce cbData == 0 with a non-null pbData. The current guard if (output_blob.pbData && output_blob.cbData > 0) skips the LocalFree call in that case, leaking the Windows heap allocation. The LocalFree should be called unconditionally whenever CryptUnprotectData succeeds, regardless of whether the result is used.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: packages/browseros/chromium_patches/chrome/utility/importer/browseros/chrome_decryptor_win.cc
    Line: 112-127
    
    Comment:
    **Memory leak when DPAPI returns empty output**
    
    `CryptUnprotectData` can succeed (return `TRUE`) but produce `cbData == 0` with a non-null `pbData`. The current guard `if (output_blob.pbData && output_blob.cbData > 0)` skips the `LocalFree` call in that case, leaking the Windows heap allocation. The `LocalFree` should be called unconditionally whenever `CryptUnprotectData` succeeds, regardless of whether the result is used.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
packages/browseros/chromium_patches/chrome/utility/importer/browseros/chrome_decryptor_win.cc:112-127
**Memory leak when DPAPI returns empty output**

`CryptUnprotectData` can succeed (return `TRUE`) but produce `cbData == 0` with a non-null `pbData`. The current guard `if (output_blob.pbData && output_blob.cbData > 0)` skips the `LocalFree` call in that case, leaking the Windows heap allocation. The `LocalFree` should be called unconditionally whenever `CryptUnprotectData` succeeds, regardless of whether the result is used.

### Issue 2 of 3
packages/browseros/chromium_patches/chrome/utility/importer/browseros/chrome_decryptor_win.cc:13-15
**Direct `wincrypt.h` can conflict with BoringSSL macros**

`base/win/wincrypt_shim.h` exists in Chromium precisely because `<wincrypt.h>` defines macros (`X509_NAME`, `X509_CERT_PAIR`, `X509_EXTENSIONS`, `PKCS7_SIGNER_INFO`, etc.) that collide with BoringSSL definitions. The shim `#undef`s those symbols after including `<wincrypt.h>`. This file then includes `crypto/aead.h` (line 24), which pulls in BoringSSL transitively — with the shim removed, those macro conflicts can surface as compilation errors. If the shim path was the actual problem, the safer fix is to restore the shim and add a `#pragma comment(lib, "crypt32.lib")` in the build file rather than bypassing the shim altogether.

### Issue 3 of 3
packages/browseros/chromium_patches/chrome/utility/importer/browseros/chrome_decryptor_win.cc:232-233
**Verbose info log on hot decryption path**

`LOG(INFO)` here fires for every stored value that is not encrypted with the `v10` prefix — which could include a large number of cookies or passwords in practice. This matches the team rule to remove excessive logging after debugging. Consider removing or downgrading this to `VLOG(1)` to avoid flooding logs at `INFO` level during import.

Reviews (1): Last reviewed commit: "fix: windows decrypt patch" | Re-trigger Greptile

Comment on lines +13 to +15
+#include <windows.h>
+
+#include <wincrypt.h>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Direct wincrypt.h can conflict with BoringSSL macros

base/win/wincrypt_shim.h exists in Chromium precisely because <wincrypt.h> defines macros (X509_NAME, X509_CERT_PAIR, X509_EXTENSIONS, PKCS7_SIGNER_INFO, etc.) that collide with BoringSSL definitions. The shim #undefs those symbols after including <wincrypt.h>. This file then includes crypto/aead.h (line 24), which pulls in BoringSSL transitively — with the shim removed, those macro conflicts can surface as compilation errors. If the shim path was the actual problem, the safer fix is to restore the shim and add a #pragma comment(lib, "crypt32.lib") in the build file rather than bypassing the shim altogether.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/chromium_patches/chrome/utility/importer/browseros/chrome_decryptor_win.cc
Line: 13-15

Comment:
**Direct `wincrypt.h` can conflict with BoringSSL macros**

`base/win/wincrypt_shim.h` exists in Chromium precisely because `<wincrypt.h>` defines macros (`X509_NAME`, `X509_CERT_PAIR`, `X509_EXTENSIONS`, `PKCS7_SIGNER_INFO`, etc.) that collide with BoringSSL definitions. The shim `#undef`s those symbols after including `<wincrypt.h>`. This file then includes `crypto/aead.h` (line 24), which pulls in BoringSSL transitively — with the shim removed, those macro conflicts can surface as compilation errors. If the shim path was the actual problem, the safer fix is to restore the shim and add a `#pragma comment(lib, "crypt32.lib")` in the build file rather than bypassing the shim altogether.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant