sneak/AutistMask
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

security review #6

New Issue
Closed
opened 2026-02-27 18:32:00 +01:00 by sneak · 8 comments
sneak commented 2026-02-27 18:32:00 +01:00
Owner
Copy Link

Audit this entire codebase for security bugs. Can malicious webpages inject code into our flows? Identify and list any security issues you can find in this codebase. Leave findings as a comment on this issue.

Audit this entire codebase for security bugs. Can malicious webpages inject code into our flows? Identify and list any security issues you can find in this codebase. Leave findings as a comment on this issue.
clawbot was assigned by sneak 2026-02-27 18:32:00 +01:00
clawbot commented 2026-02-27 18:34:01 +01:00
Collaborator
Copy Link

Security Review — AutistMask

Reviewed: content scripts, inpage provider, background service worker, vault, wallet, state management, popup views, manifests, and build pipeline.

CRITICAL

1. DEBUG mode ships a hardcoded mnemonic

File: src/shared/constants.js

DEBUG = true and DEBUG_MNEMONIC is a fixed 12-word phrase. generateMnemonic() in wallet.js returns this fixed mnemonic when DEBUG is true. The build pipeline (build.js) does NOT override this constant via esbuild define. If this ships, every wallet uses the same mnemonic. Anyone reading the source can derive all private keys.

Impact: Total loss of all funds for every user.
Fix: Remove the DEBUG mnemonic, or ensure build sets DEBUG = false via esbuild define.

2. postMessage uses "*" target origin

File: src/content/index.js

Responses are posted with window.postMessage(..., "*"). While the event.source !== window check prevents cross-origin frame injection, the "*" target means responses (including account addresses and RPC results) are visible to ALL frames on the page, including malicious iframes.

Fix: Use location.origin instead of "*" in all postMessage calls.

HIGH

3. No password strength enforcement beyond 8-char minimum

Both addWallet.js and importKey.js only check pw.length < 8. For a wallet holding funds, this is dangerously low. Argon2id INTERACTIVE params provide ~0.5s KDF work — still brute-forceable for weak 8-char passwords.

Recommendation: Minimum 12 chars or integrate zxcvbn-style entropy check.

4. Approval IDs are sequential integers (predictable)

let nextApprovalId = 1; — passed in popup URL as ?approval=N. A malicious page that can predict the ID could attempt to interact with the approval flow. Use crypto.randomUUID() instead.

5. eth_sign is enabled — allows signing arbitrary hashes

eth_sign signs raw 32-byte hashes (can be transaction hashes). A malicious dApp can trick users into signing fund-draining transactions. MetaMask shows strong warnings for this. Consider removing eth_sign support or adding a prominent danger warning.

6. Password sent via extension messaging in plaintext

When approving tx/sign, the password is sent popup-to-background via runtime.sendMessage({ password }). While extension messaging is internal, consider decrypting in the popup and sending derived key/signer output instead.

MEDIUM

7. connectedSites key collision risk

Uses origin + ":" + activeAddress as key. Theoretical collision if origin contains ":" followed by an address. Use a Map with structured keys.

8. No rate limiting on RPC proxy

Any page can fire unlimited RPC calls through the extension proxy, potentially exhausting the user's RPC endpoint rate limits.

9. _metamask.isUnlocked() always returns true

DApps using this to check lock state will skip their own lock-check flows.

10. No auto-lock / session timeout

No mechanism to require re-authentication after inactivity. A stolen/unlocked browser gives full access to wallet UI, addresses, balances, and site permissions.

11. ENS cache in localStorage without integrity check

ENS names cached in localStorage. Page-context scripts can write to localStorage for web pages (not extension pages). Verify inpage script does not access this cache.

LOW / INFORMATIONAL

  • web_accessible_resources exposes inpage.js — fingerprinting vector
  • No CSP in popup HTML — should add Content-Security-Policy meta tag
  • host_permissions: ["<all_urls>"] — very broad, typical for wallets but notable
  • Token holder threshold (1000) hardcoded in multiple files — should be a constant
  • escapeHtml uses DOM — safe but will crash if called from service worker (no DOM)

Summary

Severity Count Key Issues
Critical 2 DEBUG mnemonic ships to users; postMessage "*" target
High 4 Weak password policy; predictable approval IDs; eth_sign; plaintext password
Medium 5 No rate limiting; no auto-lock; ENS cache; etc.
Low 5 Fingerprinting; no CSP; broad permissions

The DEBUG mnemonic issue (#1) is the most urgent — if this ships, it is a total loss scenario.

# Security Review — AutistMask Reviewed: content scripts, inpage provider, background service worker, vault, wallet, state management, popup views, manifests, and build pipeline. ## CRITICAL ### 1. DEBUG mode ships a hardcoded mnemonic **File:** `src/shared/constants.js` `DEBUG = true` and `DEBUG_MNEMONIC` is a fixed 12-word phrase. `generateMnemonic()` in wallet.js returns this fixed mnemonic when DEBUG is true. The build pipeline (`build.js`) does NOT override this constant via esbuild `define`. If this ships, **every wallet uses the same mnemonic**. Anyone reading the source can derive all private keys. **Impact:** Total loss of all funds for every user. **Fix:** Remove the DEBUG mnemonic, or ensure build sets `DEBUG = false` via esbuild define. ### 2. postMessage uses "*" target origin **File:** `src/content/index.js` Responses are posted with `window.postMessage(..., "*")`. While the `event.source !== window` check prevents cross-origin frame injection, the "*" target means responses (including account addresses and RPC results) are visible to ALL frames on the page, including malicious iframes. **Fix:** Use `location.origin` instead of "*" in all postMessage calls. ## HIGH ### 3. No password strength enforcement beyond 8-char minimum Both addWallet.js and importKey.js only check `pw.length < 8`. For a wallet holding funds, this is dangerously low. Argon2id INTERACTIVE params provide ~0.5s KDF work — still brute-forceable for weak 8-char passwords. **Recommendation:** Minimum 12 chars or integrate zxcvbn-style entropy check. ### 4. Approval IDs are sequential integers (predictable) `let nextApprovalId = 1;` — passed in popup URL as `?approval=N`. A malicious page that can predict the ID could attempt to interact with the approval flow. Use `crypto.randomUUID()` instead. ### 5. eth_sign is enabled — allows signing arbitrary hashes eth_sign signs raw 32-byte hashes (can be transaction hashes). A malicious dApp can trick users into signing fund-draining transactions. MetaMask shows strong warnings for this. Consider removing eth_sign support or adding a prominent danger warning. ### 6. Password sent via extension messaging in plaintext When approving tx/sign, the password is sent popup-to-background via `runtime.sendMessage({ password })`. While extension messaging is internal, consider decrypting in the popup and sending derived key/signer output instead. ## MEDIUM ### 7. connectedSites key collision risk Uses `origin + ":" + activeAddress` as key. Theoretical collision if origin contains ":" followed by an address. Use a Map with structured keys. ### 8. No rate limiting on RPC proxy Any page can fire unlimited RPC calls through the extension proxy, potentially exhausting the user's RPC endpoint rate limits. ### 9. `_metamask.isUnlocked()` always returns true DApps using this to check lock state will skip their own lock-check flows. ### 10. No auto-lock / session timeout No mechanism to require re-authentication after inactivity. A stolen/unlocked browser gives full access to wallet UI, addresses, balances, and site permissions. ### 11. ENS cache in localStorage without integrity check ENS names cached in localStorage. Page-context scripts can write to localStorage for web pages (not extension pages). Verify inpage script does not access this cache. ## LOW / INFORMATIONAL - **web_accessible_resources** exposes inpage.js — fingerprinting vector - **No CSP in popup HTML** — should add Content-Security-Policy meta tag - **host_permissions: ["<all_urls>"]** — very broad, typical for wallets but notable - **Token holder threshold (1000) hardcoded** in multiple files — should be a constant - **escapeHtml uses DOM** — safe but will crash if called from service worker (no DOM) ## Summary | Severity | Count | Key Issues | |----------|-------|------------| | Critical | 2 | DEBUG mnemonic ships to users; postMessage "*" target | | High | 4 | Weak password policy; predictable approval IDs; eth_sign; plaintext password | | Medium | 5 | No rate limiting; no auto-lock; ENS cache; etc. | | Low | 5 | Fingerprinting; no CSP; broad permissions | **The DEBUG mnemonic issue (#1) is the most urgent — if this ships, it is a total loss scenario.**
clawbot removed their assignment 2026-02-27 18:34:08 +01:00
sneak was assigned by clawbot 2026-02-27 18:34:08 +01:00
clawbot commented 2026-02-27 18:34:25 +01:00
Collaborator
Copy Link

Starting security audit now. Will post findings here.

Starting security audit now. Will post findings here.
clawbot commented 2026-02-27 18:37:29 +01:00
Collaborator
Copy Link

AutistMask Security Audit Report

Auditor: clawbot (automated)
Date: 2026-02-27
Scope: All files in src/, manifests, package.json

Critical Findings

C1: DEBUG Mode Hardcoded to true — Deterministic Mnemonic in Production

  • Severity: Critical
  • File: src/shared/constants.js:1-2
  • Description: DEBUG is hardcoded to true and DEBUG_MNEMONIC is a fixed 12-word phrase. In wallet.js:generateMnemonic(), when DEBUG is true, the function returns DEBUG_MNEMONIC instead of generating random entropy. Every wallet created by a user of this extension uses the same seed phrase.
  • Attack scenario: Anyone who reads the source code (it's on Gitea) knows the mnemonic for every wallet created with the extension. They can derive all addresses and steal all funds.
  • Recommendation: Set DEBUG = false for production builds. Better: remove the DEBUG_MNEMONIC entirely and use a build-time environment variable. Add a CI check that ensures DEBUG is false in release artifacts.

High Findings

H1: Content Script Origin Spoofing — origin Comes From Content Script Message

  • Severity: High
  • File: src/content/index.js:18, src/background/index.js (message listener)
  • Description: The content script sends origin: location.origin as part of the AUTISTMASK_RPC message to the background script. The background script uses this msg.origin for all permission checks (allowed/denied sites, connection status). However, Chrome's runtime.onMessage provides sender.origin and sender.tab.url which are browser-verified and cannot be spoofed by web content. The current approach trusts the content script's self-reported origin.
  • Attack scenario: While a standard content script running in an ISOLATED world cannot have its location.origin spoofed by the page, in Firefox MV2 there are historical edge cases. More importantly, if another extension injects a content script into a page, it could call chrome.runtime.sendMessage with a forged origin field. The background script does not verify sender.origin or sender.tab.url against the claimed msg.origin.
  • Recommendation: In the background runtime.onMessage handler, ignore msg.origin and instead derive the origin from sender.origin (Chrome MV3) or sender.tab.url (Firefox MV2). This is the browser-attested origin.

H2: No Sender Validation on Internal Messages

  • Severity: High
  • File: src/background/index.js (entire runtime.onMessage listener)
  • Description: The background script processes AUTISTMASK_TX_RESPONSE, AUTISTMASK_SIGN_RESPONSE, AUTISTMASK_APPROVAL_RESPONSE, and AUTISTMASK_GET_APPROVAL messages from any sender. It does not check that the sender is the popup page (via sender.url or sender.tab). Any content script (running on any webpage) can send these messages.
  • Attack scenario: A malicious webpage's content script (injected by another extension or via a vulnerability) sends AUTISTMASK_GET_APPROVAL with a guessed/brute-forced approval ID to learn the approval details (hostname, txParams), then sends AUTISTMASK_APPROVAL_RESPONSE with approved: true to auto-approve a site connection without user interaction. Approval IDs are sequential integers starting from 1, making them trivially guessable.
  • Recommendation: For popup-only messages (AUTISTMASK_GET_APPROVAL, AUTISTMASK_APPROVAL_RESPONSE, AUTISTMASK_TX_RESPONSE, AUTISTMASK_SIGN_RESPONSE), validate that sender.url matches the extension's popup URL. Use cryptographically random approval IDs (e.g., crypto.randomUUID()) instead of sequential integers.

H3: Password Sent in Plaintext via runtime.sendMessage

  • Severity: High
  • File: src/popup/views/approval.js → src/background/index.js
  • Description: For transaction and signature approvals, the user's vault password is sent from the popup to the background script via runtime.sendMessage({ type: "AUTISTMASK_TX_RESPONSE", password: password }). Combined with H2, any content script that can send messages to the extension runtime can intercept the approval flow. Even without H2, the password transits through the extension messaging system.
  • Recommendation: Perform decryption and signing in the popup itself (it has access to storage), or use a more secure channel. At minimum, fix H2 first.

H4: Decrypted Secret Not Cleared From Memory

  • Severity: High
  • File: src/background/index.js:AUTISTMASK_TX_RESPONSE handler, src/popup/views/confirmTx.js:btn-modal-confirm handler
  • Description: After decrypting the mnemonic/private key with decryptWithPassword(), the plaintext secret is stored in a local variable (decryptedSecret/decrypted) but never explicitly zeroed. JavaScript strings are immutable and cannot be reliably zeroed, but the variable reference persists in closure scope.
  • Attack scenario: A memory dump, heap snapshot, or speculative execution attack could recover the plaintext mnemonic/private key from the JS heap.
  • Recommendation: While JS makes this hard to fully mitigate, minimize the window: null out variables immediately after use, avoid storing in closures, and consider using Uint8Array for secrets (which can be zeroed). Document the inherent limitation.

Medium Findings

M1: XSS via innerHTML with Partially-Escaped User Data (Settings Hostname Display)

  • Severity: Medium
  • File: src/popup/views/settings.js:renderSiteList()
  • Description: In renderSiteList(), hostnames from stored site permissions are interpolated into HTML via string concatenation: html += \${hostname}`. Hostnames are derived from new URL(origin).hostname` which normally produces safe values, but the storage could be corrupted or an edge-case URL could produce unexpected hostname values.
  • Attack scenario: If an attacker can influence the stored hostname (e.g., through a specially crafted origin processed by a browser bug, or by directly modifying extension storage via a vulnerability), they could inject HTML/JS into the popup.
  • Recommendation: Use escapeHtml(hostname) consistently. The escapeHtml helper exists and is used elsewhere but not here.

M2: <all_urls> Host Permission is Overly Broad

  • Severity: Medium
  • File: manifest/chrome.json, manifest/firefox.json
  • Description: The extension requests <all_urls> host permissions, giving it access to make requests to any URL. This is needed for custom RPC endpoints but grants more access than necessary for most users.
  • Attack scenario: If the extension is compromised, the broad permissions allow it to exfiltrate data from any site.
  • Recommendation: Document why <all_urls> is required (custom RPC endpoints). Consider whether activeTab alone would suffice for content script injection, with an optional host permission for the RPC URL.

M3: No CSP Defined for Extension Pages

  • Severity: Medium
  • File: manifest/chrome.json, manifest/firefox.json
  • Description: Neither manifest defines a content_security_policy. Chrome MV3 has a reasonable default CSP, but Firefox MV2 does not restrict inline scripts by default.
  • Recommendation: Add explicit CSP: "content_security_policy": "script-src 'self'; object-src 'none'" (MV2) or the MV3 equivalent. This provides defense-in-depth against XSS.

M4: Malicious RPC Endpoint Can Return Arbitrary Data

  • Severity: Medium
  • File: src/background/index.js:proxyRpc(), various consumers
  • Description: RPC responses from the configured endpoint are not validated beyond checking for json.error. A malicious RPC server can return crafted responses for eth_call, eth_getBalance, eth_estimateGas, etc. This could cause the UI to display incorrect balances, incorrect gas estimates (leading to stuck or overpaying transactions), or trigger unexpected behavior in ethers.js parsing.
  • Attack scenario: User configures a malicious RPC. The server returns inflated balances, causing the user to believe they have more funds. Or returns a very low gas estimate, causing transactions to fail. More seriously, crafted responses to eth_call (used for ERC-20 name()/symbol()) could return malicious strings that get rendered in the UI.
  • Recommendation: Validate RPC response shapes. For token name/symbol, apply length limits and character filtering. Document the trust model for RPC endpoints.

M5: postMessage with "*" Target Origin

  • Severity: Medium
  • File: src/content/index.js:26, src/content/index.js:40
  • Description: The content script uses window.postMessage({...}, "*") to send responses and events back to the page. Using "*" means any frame (including cross-origin iframes) on the page can receive these messages.
  • Attack scenario: A cross-origin iframe on a dApp page could listen for AUTISTMASK_RESPONSE messages and learn the user's address or intercept RPC responses.
  • Recommendation: Use window.postMessage({...}, location.origin) or target the specific origin of the requesting page.

Low Findings

L1: Sequential Approval IDs Are Predictable

  • Severity: Low (escalates to High when combined with H2)
  • File: src/background/index.js:nextApprovalId
  • Description: Approval IDs are sequential integers (nextApprovalId++). If sender validation (H2) is absent, these are trivially guessable.
  • Recommendation: Use crypto.randomUUID() for approval IDs.

L2: No Rate Limiting on Password Attempts

  • Severity: Low
  • File: src/popup/views/confirmTx.js, src/popup/views/approval.js
  • Description: The password modal and approval password fields have no rate limiting or lockout. An attacker with physical access can brute-force the password.
  • Recommendation: Implement exponential backoff or lockout after N failed attempts. Argon2id key derivation provides some protection via computational cost, but it's set to INTERACTIVE (fast) parameters.

L3: _metamask.isUnlocked() Always Returns True

  • Severity: Low
  • File: src/content/inpage.js:_metamask.isUnlocked()
  • Description: This always returns true, leaking information that the wallet is present and "unlocked" to any page, even before the user has approved a connection.
  • Recommendation: Return a meaningful value based on wallet state, or return false when no site connection is established.

L4: EIP-6963 Provider UUID is Hardcoded

  • Severity: Low
  • File: src/content/inpage.js:providerInfo.uuid
  • Description: The EIP-6963 UUID should be unique per extension instance. A hardcoded UUID means all AutistMask installations share the same identity, which could cause conflicts.
  • Recommendation: Generate the UUID at install time and persist it.

L5: Token Name/Symbol From RPC Not Length-Limited

  • Severity: Low
  • File: src/shared/balances.js (lookupTokenInfo), various UI renderers
  • Description: When adding a custom token, the name and symbol are fetched from the contract via RPC and stored/displayed without length limits. A malicious token contract could return extremely long strings.
  • Recommendation: Truncate token name to ~64 chars and symbol to ~12 chars.

Info Findings

I1: Dependencies Not Pinned to Exact Versions

  • Severity: Info
  • File: package.json
  • Description: Dependencies use caret ranges (e.g., ^6.16.0). A compromised or buggy minor/patch release could be pulled in.
  • Recommendation: Pin to exact versions or use a lockfile (yarn.lock is present, which helps). Consider running npm audit / yarn audit in CI.

I2: No Automated Security Tests

  • Severity: Info
  • Description: The test suite (jest) exists but there are no tests specifically for security-sensitive paths (message validation, origin checking, approval flows, encryption/decryption).
  • Recommendation: Add tests for: message origin validation, approval flow (cannot approve without user action), encryption round-trip, password rejection.

I3: web_accessible_resources Exposes inpage.js

  • Severity: Info
  • File: manifest/chrome.json, manifest/firefox.json
  • Description: inpage.js is listed as a web-accessible resource, making it loadable by any webpage. This is necessary for Firefox injection but allows fingerprinting (any site can detect AutistMask by trying to load this resource).
  • Recommendation: This is inherent to the architecture. Document the fingerprinting implication. In Chrome MV3, since inpage.js runs as a MAIN world content script, it may not need to be web-accessible.

Summary

Severity Count
Critical 1
High 4
Medium 5
Low 5
Info 3

The most urgent issue is C1 (DEBUG mode) — if this extension is shipped as-is, every user gets the same wallet. This must be fixed before any release.

H1+H2 together are severe: a malicious page can auto-approve site connections by sending forged messages to the background script with guessed approval IDs. Fix by validating sender in the message listener and using random approval IDs.

Overall architecture is sound: Argon2id+XSalsa20 for vault encryption, proper use of ethers.js for key derivation, approval popups for sensitive operations. The main gaps are in message authentication between extension components and the DEBUG flag.

# AutistMask Security Audit Report **Auditor**: clawbot (automated) **Date**: 2026-02-27 **Scope**: All files in `src/`, manifests, `package.json` --- ## Critical Findings ### C1: DEBUG Mode Hardcoded to `true` — Deterministic Mnemonic in Production - **Severity**: Critical - **File**: `src/shared/constants.js:1-2` - **Description**: `DEBUG` is hardcoded to `true` and `DEBUG_MNEMONIC` is a fixed 12-word phrase. In `wallet.js:generateMnemonic()`, when `DEBUG` is true, the function returns `DEBUG_MNEMONIC` instead of generating random entropy. **Every wallet created by a user of this extension uses the same seed phrase.** - **Attack scenario**: Anyone who reads the source code (it's on Gitea) knows the mnemonic for every wallet created with the extension. They can derive all addresses and steal all funds. - **Recommendation**: Set `DEBUG = false` for production builds. Better: remove the DEBUG_MNEMONIC entirely and use a build-time environment variable. Add a CI check that ensures `DEBUG` is `false` in release artifacts. --- ## High Findings ### H1: Content Script Origin Spoofing — `origin` Comes From Content Script Message - **Severity**: High - **File**: `src/content/index.js:18`, `src/background/index.js` (message listener) - **Description**: The content script sends `origin: location.origin` as part of the `AUTISTMASK_RPC` message to the background script. The background script uses this `msg.origin` for all permission checks (allowed/denied sites, connection status). However, Chrome's `runtime.onMessage` provides `sender.origin` and `sender.tab.url` which are browser-verified and cannot be spoofed by web content. The current approach trusts the content script's self-reported origin. - **Attack scenario**: While a standard content script running in an ISOLATED world cannot have its `location.origin` spoofed by the page, in Firefox MV2 there are historical edge cases. More importantly, if another extension injects a content script into a page, it could call `chrome.runtime.sendMessage` with a forged `origin` field. The background script does not verify `sender.origin` or `sender.tab.url` against the claimed `msg.origin`. - **Recommendation**: In the background `runtime.onMessage` handler, ignore `msg.origin` and instead derive the origin from `sender.origin` (Chrome MV3) or `sender.tab.url` (Firefox MV2). This is the browser-attested origin. ### H2: No Sender Validation on Internal Messages - **Severity**: High - **File**: `src/background/index.js` (entire `runtime.onMessage` listener) - **Description**: The background script processes `AUTISTMASK_TX_RESPONSE`, `AUTISTMASK_SIGN_RESPONSE`, `AUTISTMASK_APPROVAL_RESPONSE`, and `AUTISTMASK_GET_APPROVAL` messages from any sender. It does not check that the sender is the popup page (via `sender.url` or `sender.tab`). Any content script (running on any webpage) can send these messages. - **Attack scenario**: A malicious webpage's content script (injected by another extension or via a vulnerability) sends `AUTISTMASK_GET_APPROVAL` with a guessed/brute-forced approval ID to learn the approval details (hostname, txParams), then sends `AUTISTMASK_APPROVAL_RESPONSE` with `approved: true` to auto-approve a site connection without user interaction. Approval IDs are sequential integers starting from 1, making them trivially guessable. - **Recommendation**: For popup-only messages (`AUTISTMASK_GET_APPROVAL`, `AUTISTMASK_APPROVAL_RESPONSE`, `AUTISTMASK_TX_RESPONSE`, `AUTISTMASK_SIGN_RESPONSE`), validate that `sender.url` matches the extension's popup URL. Use cryptographically random approval IDs (e.g., `crypto.randomUUID()`) instead of sequential integers. ### H3: Password Sent in Plaintext via `runtime.sendMessage` - **Severity**: High - **File**: `src/popup/views/approval.js` → `src/background/index.js` - **Description**: For transaction and signature approvals, the user's vault password is sent from the popup to the background script via `runtime.sendMessage({ type: "AUTISTMASK_TX_RESPONSE", password: password })`. Combined with H2, any content script that can send messages to the extension runtime can intercept the approval flow. Even without H2, the password transits through the extension messaging system. - **Recommendation**: Perform decryption and signing in the popup itself (it has access to storage), or use a more secure channel. At minimum, fix H2 first. ### H4: Decrypted Secret Not Cleared From Memory - **Severity**: High - **File**: `src/background/index.js:AUTISTMASK_TX_RESPONSE handler`, `src/popup/views/confirmTx.js:btn-modal-confirm handler` - **Description**: After decrypting the mnemonic/private key with `decryptWithPassword()`, the plaintext secret is stored in a local variable (`decryptedSecret`/`decrypted`) but never explicitly zeroed. JavaScript strings are immutable and cannot be reliably zeroed, but the variable reference persists in closure scope. - **Attack scenario**: A memory dump, heap snapshot, or speculative execution attack could recover the plaintext mnemonic/private key from the JS heap. - **Recommendation**: While JS makes this hard to fully mitigate, minimize the window: null out variables immediately after use, avoid storing in closures, and consider using `Uint8Array` for secrets (which can be zeroed). Document the inherent limitation. --- ## Medium Findings ### M1: XSS via `innerHTML` with Partially-Escaped User Data (Settings Hostname Display) - **Severity**: Medium - **File**: `src/popup/views/settings.js:renderSiteList()` - **Description**: In `renderSiteList()`, hostnames from stored site permissions are interpolated into HTML via string concatenation: `html += \`<span>${hostname}</span>\``. Hostnames are derived from `new URL(origin).hostname` which normally produces safe values, but the storage could be corrupted or an edge-case URL could produce unexpected hostname values. - **Attack scenario**: If an attacker can influence the stored hostname (e.g., through a specially crafted origin processed by a browser bug, or by directly modifying extension storage via a vulnerability), they could inject HTML/JS into the popup. - **Recommendation**: Use `escapeHtml(hostname)` consistently. The `escapeHtml` helper exists and is used elsewhere but not here. ### M2: `<all_urls>` Host Permission is Overly Broad - **Severity**: Medium - **File**: `manifest/chrome.json`, `manifest/firefox.json` - **Description**: The extension requests `<all_urls>` host permissions, giving it access to make requests to any URL. This is needed for custom RPC endpoints but grants more access than necessary for most users. - **Attack scenario**: If the extension is compromised, the broad permissions allow it to exfiltrate data from any site. - **Recommendation**: Document why `<all_urls>` is required (custom RPC endpoints). Consider whether `activeTab` alone would suffice for content script injection, with an optional host permission for the RPC URL. ### M3: No CSP Defined for Extension Pages - **Severity**: Medium - **File**: `manifest/chrome.json`, `manifest/firefox.json` - **Description**: Neither manifest defines a `content_security_policy`. Chrome MV3 has a reasonable default CSP, but Firefox MV2 does not restrict inline scripts by default. - **Recommendation**: Add explicit CSP: `"content_security_policy": "script-src 'self'; object-src 'none'"` (MV2) or the MV3 equivalent. This provides defense-in-depth against XSS. ### M4: Malicious RPC Endpoint Can Return Arbitrary Data - **Severity**: Medium - **File**: `src/background/index.js:proxyRpc()`, various consumers - **Description**: RPC responses from the configured endpoint are not validated beyond checking for `json.error`. A malicious RPC server can return crafted responses for `eth_call`, `eth_getBalance`, `eth_estimateGas`, etc. This could cause the UI to display incorrect balances, incorrect gas estimates (leading to stuck or overpaying transactions), or trigger unexpected behavior in ethers.js parsing. - **Attack scenario**: User configures a malicious RPC. The server returns inflated balances, causing the user to believe they have more funds. Or returns a very low gas estimate, causing transactions to fail. More seriously, crafted responses to `eth_call` (used for ERC-20 `name()`/`symbol()`) could return malicious strings that get rendered in the UI. - **Recommendation**: Validate RPC response shapes. For token name/symbol, apply length limits and character filtering. Document the trust model for RPC endpoints. ### M5: `postMessage` with `"*"` Target Origin - **Severity**: Medium - **File**: `src/content/index.js:26`, `src/content/index.js:40` - **Description**: The content script uses `window.postMessage({...}, "*")` to send responses and events back to the page. Using `"*"` means any frame (including cross-origin iframes) on the page can receive these messages. - **Attack scenario**: A cross-origin iframe on a dApp page could listen for `AUTISTMASK_RESPONSE` messages and learn the user's address or intercept RPC responses. - **Recommendation**: Use `window.postMessage({...}, location.origin)` or target the specific origin of the requesting page. --- ## Low Findings ### L1: Sequential Approval IDs Are Predictable - **Severity**: Low (escalates to High when combined with H2) - **File**: `src/background/index.js:nextApprovalId` - **Description**: Approval IDs are sequential integers (`nextApprovalId++`). If sender validation (H2) is absent, these are trivially guessable. - **Recommendation**: Use `crypto.randomUUID()` for approval IDs. ### L2: No Rate Limiting on Password Attempts - **Severity**: Low - **File**: `src/popup/views/confirmTx.js`, `src/popup/views/approval.js` - **Description**: The password modal and approval password fields have no rate limiting or lockout. An attacker with physical access can brute-force the password. - **Recommendation**: Implement exponential backoff or lockout after N failed attempts. Argon2id key derivation provides some protection via computational cost, but it's set to INTERACTIVE (fast) parameters. ### L3: `_metamask.isUnlocked()` Always Returns True - **Severity**: Low - **File**: `src/content/inpage.js:_metamask.isUnlocked()` - **Description**: This always returns `true`, leaking information that the wallet is present and "unlocked" to any page, even before the user has approved a connection. - **Recommendation**: Return a meaningful value based on wallet state, or return `false` when no site connection is established. ### L4: EIP-6963 Provider UUID is Hardcoded - **Severity**: Low - **File**: `src/content/inpage.js:providerInfo.uuid` - **Description**: The EIP-6963 UUID should be unique per extension instance. A hardcoded UUID means all AutistMask installations share the same identity, which could cause conflicts. - **Recommendation**: Generate the UUID at install time and persist it. ### L5: Token Name/Symbol From RPC Not Length-Limited - **Severity**: Low - **File**: `src/shared/balances.js` (lookupTokenInfo), various UI renderers - **Description**: When adding a custom token, the name and symbol are fetched from the contract via RPC and stored/displayed without length limits. A malicious token contract could return extremely long strings. - **Recommendation**: Truncate token name to ~64 chars and symbol to ~12 chars. --- ## Info Findings ### I1: Dependencies Not Pinned to Exact Versions - **Severity**: Info - **File**: `package.json` - **Description**: Dependencies use caret ranges (e.g., `^6.16.0`). A compromised or buggy minor/patch release could be pulled in. - **Recommendation**: Pin to exact versions or use a lockfile (yarn.lock is present, which helps). Consider running `npm audit` / `yarn audit` in CI. ### I2: No Automated Security Tests - **Severity**: Info - **Description**: The test suite (`jest`) exists but there are no tests specifically for security-sensitive paths (message validation, origin checking, approval flows, encryption/decryption). - **Recommendation**: Add tests for: message origin validation, approval flow (cannot approve without user action), encryption round-trip, password rejection. ### I3: `web_accessible_resources` Exposes `inpage.js` - **Severity**: Info - **File**: `manifest/chrome.json`, `manifest/firefox.json` - **Description**: `inpage.js` is listed as a web-accessible resource, making it loadable by any webpage. This is necessary for Firefox injection but allows fingerprinting (any site can detect AutistMask by trying to load this resource). - **Recommendation**: This is inherent to the architecture. Document the fingerprinting implication. In Chrome MV3, since inpage.js runs as a MAIN world content script, it may not need to be web-accessible. --- ## Summary | Severity | Count | |----------|-------| | Critical | 1 | | High | 4 | | Medium | 5 | | Low | 5 | | Info | 3 | **The most urgent issue is C1 (DEBUG mode)** — if this extension is shipped as-is, every user gets the same wallet. This must be fixed before any release. **H1+H2 together are severe**: a malicious page can auto-approve site connections by sending forged messages to the background script with guessed approval IDs. Fix by validating `sender` in the message listener and using random approval IDs. Overall architecture is sound: Argon2id+XSalsa20 for vault encryption, proper use of ethers.js for key derivation, approval popups for sensitive operations. The main gaps are in message authentication between extension components and the DEBUG flag.
sneak commented 2026-02-27 20:32:40 +01:00
Author
Owner
Copy Link

DEBUG is only for development and testing - the hardcoded mnemonic is intentional. The production build will not have DEBUG mode enabled. It even says "insecure" in a red warning in the header. Don't worry about that one.

@clawbot make me a PR to fix all of the High severity items.

DEBUG is only for development and testing - the hardcoded mnemonic is intentional. The production build will not have DEBUG mode enabled. It even says "insecure" in a red warning in the header. Don't worry about that one. @clawbot make me a PR to fix all of the High severity items.
clawbot commented 2026-02-27 20:33:25 +01:00
Collaborator
Copy Link

Noted on DEBUG — removing from scope.

High severity items to fix:

  1. Password min 8→12 chars (addWallet.js, importKey.js)
  2. Predictable approval IDs → crypto.randomUUID() (background/index.js)
  3. eth_sign: add danger warning before signing arbitrary hashes
  4. Password plaintext via messaging — decrypt in popup instead of sending password to background
  5. Origin spoofing — use sender.origin/sender.tab.url instead of msg.origin (background/index.js)
  6. No sender validation — verify popup-only messages come from popup URL
  7. Decrypted secret not cleared — null out variables after use, use Uint8Array where possible

Creating PR now.

Noted on DEBUG — removing from scope. High severity items to fix: 1. **Password min 8→12 chars** (addWallet.js, importKey.js) 2. **Predictable approval IDs** → `crypto.randomUUID()` (background/index.js) 3. **eth_sign: add danger warning** before signing arbitrary hashes 4. **Password plaintext via messaging** — decrypt in popup instead of sending password to background 5. **Origin spoofing** — use `sender.origin`/`sender.tab.url` instead of `msg.origin` (background/index.js) 6. **No sender validation** — verify popup-only messages come from popup URL 7. **Decrypted secret not cleared** — null out variables after use, use Uint8Array where possible Creating PR now.
sneak commented 2026-02-27 20:36:32 +01:00
Author
Owner
Copy Link

Make me a PR for all the Low severity ones except the L2 rate limit - that one's not real. Someone with physical access has other ways of bruteforcing the password.

Make me a PR for all the Low severity ones except the L2 rate limit - that one's not real. Someone with physical access has other ways of bruteforcing the password.
clawbot commented 2026-02-27 20:37:05 +01:00
Collaborator
Copy Link

Working on the Low severity fixes (excluding L2 rate limit). Fixing:

  • L1: Sequential approval IDs → crypto.randomUUID()
  • L3: _metamask.isUnlocked() → return meaningful value based on connection state
  • L4: EIP-6963 provider UUID → generate at install time, persist in storage
  • L5: Token name/symbol from RPC → truncate (name ≤64 chars, symbol ≤12 chars)

Spawning agent now.

Working on the Low severity fixes (excluding L2 rate limit). Fixing: - **L1**: Sequential approval IDs → `crypto.randomUUID()` - **L3**: `_metamask.isUnlocked()` → return meaningful value based on connection state - **L4**: EIP-6963 provider UUID → generate at install time, persist in storage - **L5**: Token name/symbol from RPC → truncate (name ≤64 chars, symbol ≤12 chars) Spawning agent now.
clawbot commented 2026-02-27 20:39:19 +01:00
Collaborator
Copy Link

Working on the Low severity fixes now (excluding L2 rate limiting per your note). Will create a separate PR.

Working on the Low severity fixes now (excluding L2 rate limiting per your note). Will create a separate PR.
sneak closed this issue 2026-02-27 20:56:43 +01:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels   
bot

Agent's turn to work

  
merge-ready

   
needs-checks

   
needs-rebase

   
needs-review

   
needs-rework
No Label
bot
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
sneak
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#6
No description provided.
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?