Skip to content

fix(security): restrict keystore file permissions to 0o600 #399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yasinlex wants to merge 1 commit into
base: v0.40-dev
Choose a base branch
from
+25 −6
Open

fix(security): restrict keystore file permissions to 0o600 #399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/commands/account/export.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {BaseAction} from "../../lib/actions/BaseAction";
import {ethers} from "ethers";
import {writeFileSync, existsSync, readFileSync} from "fs";
import {writeFileSync, existsSync, readFileSync, chmodSync} from "fs";
import path from "path";

export interface ExportAccountOptions {
Expand Down Expand Up @@ -60,7 +60,12 @@ export class ExportAccountAction extends BaseAction {
const encryptedJson = await wallet.encrypt(password);

// Write standard web3 keystore format (compatible with geth, foundry, etc.)
writeFileSync(outputPath, encryptedJson);
writeFileSync(outputPath, encryptedJson, { mode: 0o600 });
try {
chmodSync(outputPath, 0o600);
} catch {
// chmod can fail on Windows (no POSIX permissions)
}

this.succeedSpinner(`Account '${accountName}' exported to: ${outputPath}`);
this.logInfo(`Address: ${wallet.address}`);
Expand Down
9 changes: 7 additions & 2 deletions src/commands/account/import.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {BaseAction} from "../../lib/actions/BaseAction";
import {ethers} from "ethers";
import {writeFileSync, existsSync, readFileSync} from "fs";
import {writeFileSync, existsSync, readFileSync, chmodSync} from "fs";

export interface ImportAccountOptions {
privateKey?: string;
Expand Down Expand Up @@ -62,7 +62,12 @@ export class ImportAccountAction extends BaseAction {
const encryptedJson = await wallet.encrypt(password);

// Write standard web3 keystore format directly
writeFileSync(keystorePath, encryptedJson);
writeFileSync(keystorePath, encryptedJson, { mode: 0o600 });
try {
chmodSync(keystorePath, 0o600);
} catch {
// chmod can fail on Windows (no POSIX permissions)
}

if (options.setActive !== false) {
this.setActiveAccount(options.name);
Expand Down
13 changes: 11 additions & 2 deletions src/lib/actions/BaseAction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export function resolveNetwork(stored: string | undefined, customNetworks?: Cust
}
}
import { ethers } from "ethers";
import { writeFileSync, existsSync, readFileSync } from "fs";
import { writeFileSync, existsSync, readFileSync, chmodSync } from "fs";

export class BaseAction extends ConfigFileManager {
private static readonly DEFAULT_ACCOUNT_NAME = "default";
Expand Down Expand Up @@ -219,7 +219,16 @@ export class BaseAction extends ConfigFileManager {

// Write standard web3 keystore format directly
const encryptedJson = await wallet.encrypt(password);
writeFileSync(keystorePath, encryptedJson);
writeFileSync(keystorePath, encryptedJson, { mode: 0o600 });
// Enforce restrictive permissions even if the file already existed (writeFileSync
// does not change the mode of an existing file, so an attacker who pre-created it
// with 0644 would keep world-read access to the encrypted private key).
try {
chmodSync(keystorePath, 0o600);
} catch {
// chmod can fail on Windows (no POSIX permissions) — the encrypted keystore
// still protects the key, and Windows ACLs govern access there anyway.
}
Comment on lines +222 to +231

@coderabbitai coderabbitai Bot Jul 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Locate files =="
git ls-files | rg '(^|/)(BaseAction|export|import)\.ts$|^src/(commands/account|lib/actions)/' || true

echo
echo "== Relevant snippets =="
for f in src/lib/actions/BaseAction.ts src/commands/account/export.ts src/commands/account/import.ts; do
  echo "--- $f $(wc -l < "$f") lines ---"
  sed -n '1,110p' "$f" | cat -n
  echo
done

echo
echo "== Search keystore permission/write patterns =="
rg -n "writeFileSync|chmodSync|chmod\\(|0o600|0600|encrypt|keystore" src -g '*.ts' || true

echo
echo "== Static verifier: writeFileSync+chmodSync failure handling =="
python3 - <<'PY'
from pathlib import Path
import re

files = [
    "src/lib/actions/BaseAction.ts",
    "src/commands/account/export.ts",
    "src/commands/account/import.ts",
]
for path in files:
    src = Path(path).read_text()
    write = list(re.finditer(r'writeFileSync\s*\(', src))
    chmod = list(re.finditer(r'chmodSync\s*\(', src))
    try_blocks = list(re.finditer(r'(?s)\btry\s*\{(?P<body>.*?)\}\s*\catch\s*\{', src))
    chmod_in_failing_try = []
    for m in try_blocks:
        if any(c.start() >= m.start() and c.start() <= m.end() for c in chmod):
            body = m.group('body')
            inside = bool(re.search(r'catch\s*\{.*?rethrow|\bthrow\s', body, re.S))
            chmod_in_failing_try.append((c.start(), c.start(), c.group().strip(), inside))
    print(f"\n{path}:")
    print(f"  writeFileSync count: {len(write)}")
    print(f"  chmodSync count: {len(chmod)}")
    for m in try_blocks:
        body = src[m.start():m.end()]
        if re.search(r'chmodSync\s*\(', body):
            has_rethrow = bool(re.search(r'catch\s*\{.*?rethrow|\bthrow\s', body, re.S))
            has_just_comment = bool(re.search(r'catch\s*\{[^}]*chmod can fail|chmod can fail[^}]*\}', body, re.S))
            print(f"  try/catch around chmod: has_throw_on_non_windows_reject={has_rethrow}, broad_catch_comment={has_just_comment}")
PY

Repository: genlayerlabs/genlayer-cli

Length of output: 34014

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== BaseAction tail around write/update and imports =="
sed -n '180,235p' src/lib/actions/BaseAction.ts | cat -n
sed -n '150,170p' src/lib/config/ConfigFileManager.ts | cat -n

echo
echo "== Import/export tail with static parser =="
python3 - <<'PY'
from pathlib import Path
import re

for path in ["src/lib/actions/BaseAction.ts", "src/commands/account/export.ts", "src/commands/account/import.ts"]:
    src = Path(path).read_text()
    matches = list(re.finditer(r'(?s)\btry\s*\{\s*chmodSync\s*\([^}]*\)\s*\}\s*\catch\s*\{', src))
    for m in matches:
        block = src[m.start():m.end()]
        try_rethrow = bool(re.search(r'\bthrow\s*;', block) or bool(re.search(r'\brethrow\b', block)))
        print(f"{path}: chmod try/catch: throw={try_rethrow}")
        print(block.replace("\n", " "))
    if not matches:
        print(f"{path}: no chmod try/catch matches")
PY

Repository: genlayerlabs/genlayer-cli

Length of output: 4928

Make keystore permission enforcement fail closed.

All three paths silently ignore chmodSync failures, so an existing keystore or exported file with broader permissions can keep the encrypted private key readable even though the command reports success.

  • src/lib/actions/BaseAction.ts#L447-L452: propagate non-Windows chmod failures and centralize the shared write logic.
  • src/commands/account/export.ts#L63-L68: fail closed when exported keystore permissions cannot be enforced.
  • src/commands/account/import.ts#L65-L70: fail closed when imported keystore permissions cannot be enforced.
📍 Affects 3 files
  • src/lib/actions/BaseAction.ts#L222-L231 (this comment)
  • src/commands/account/export.ts#L63-L68
  • src/commands/account/import.ts#L65-L70
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/actions/BaseAction.ts` around lines 222 - 231, Centralize keystore
writing and permission enforcement in BaseAction, ensuring chmodSync failures
propagate on non-Windows platforms instead of being silently ignored; retain
Windows-specific handling as appropriate. Apply the shared fail-closed behavior
at src/lib/actions/BaseAction.ts lines 222-231 and 447-452, and update
src/commands/account/export.ts lines 63-68 and src/commands/account/import.ts
lines 65-70 to fail when permissions cannot be enforced rather than reporting
success.


// Set as active account if no active account exists
if (!this.getActiveAccount()) {
Expand Down
Loading