Repository navigation
fix(core): zero out channel account secret key buffers after use (#428) - #728
Closed
ValJnr-dev1 wants to merge 5 commits into
Closed
ValJnr-dev1 wants to merge 5 commits into
ValJnr-dev1 wants to merge 5 commits into
Conversation
…oLabs#428) Implements issue TegoLabs#428: zero the Buffer backing a Stellar Keypair's raw secret key immediately after the signing operation completes, reducing (not eliminating) the window in which a heap dump or memory scrape can recover the key material. Changes ─────── src/core/channels.ts - Import Keypair from @stellar/stellar-sdk - Export zeroizeKeypair(keypair: Keypair): void — fills rawSecretKey() (which returns the same underlying ArrayBuffer, verified empirically) with zeros via Buffer.fill(0). Detailed doc comment explains both what this achieves and its limits (immutable JS strings, GC non-determinism). - fundChannels(): construct a local Keypair from masterSecretKey, wrap client.sendPayments() in try/finally, call zeroizeKeypair() in the finally block so the buffer is zeroed in success, failure, and throw paths. src/core/vault.ts - Add a memory-safety note to VaultResolver's class doc comment: the resolved secret is returned as a JS string (immutable, no Buffer to zero). Callers that derive a Keypair should use zeroizeKeypair() from channels.ts. The resolver does not cache the secret beyond the call. SECURITY.md - Add invariant TegoLabs#5 documenting the key-buffer zeroing requirement for any future code that creates a Keypair in core/channels.ts. Explicit ✅/❌ bullets state what the mitigation does and does not guarantee, per the issue's instruction not to oversell the guarantee. tests/core/key-zeroing.test.ts (new, 10 tests) - TDD: tests were written before implementation (confirmed 7/10 failing before the code changes landed). - zeroizeKeypair() suite: buffer is all-zeros after call, no throw on fresh keypair, idempotent, isolated to the target instance. - Functional regression guard: signing and verification work correctly before zeroing; buffer is all-zeros afterward. - fundChannels() zeroing suite: Keypair.fromSecret spy confirms the created keypair's buffer is zeroed after success and after a thrown error. What this does NOT guarantee ──────────────────────────── The original secret-key string (S… stroop) is a JS primitive. Strings are immutable and may be interned by V8; there is no portable way to zero them. This is defense-in-depth, not a hard cryptographic erasure guarantee. Refs: TegoLabs#428
|
@ValJnr-dev1 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The lockfile committed in 62f9ff0 was generated by an older npm version that omitted optional platform-specific @esbuild/* packages. GitHub Actions runners (Node 22/24) use a newer npm that treats this as an inconsistency and fails added 535 packages, and audited 536 packages in 21s 140 packages are looking for funding run `npm fund` for details 2 moderate severity vulnerabilities To address all issues, run: npm audit fix Run `npm audit` for details. with exit code 1. Regenerated with npm 11 / Node 24 so the lockfile is consistent with what CI installs.
…-zeroing tests vi.spyOn(Keypair, 'fromSecret') in fundChannels zeroing tests was leaking into budget_enforcement.test.ts, which mocks @stellar/stellar-sdk via vi.mock(). When vitest scheduled both files in the same worker, the spy overrode the module mock and caused the shared-budget-pool test to fail with 'expected false to be true' (one extension ran via the real SDK instead of the stubbed one). Fix: add afterEach(() => vi.restoreAllMocks()) to the fundChannels suite. Each vi.spyOn is now explicitly torn down after every test, regardless of pass or fail.
…ous substrings
Tests like expect(output).toContain('CRITICAL: 2') were failing because
chalk was applied only to the label word, leaving reset codes between the
label and the ': N' suffix. Moving the entire 'LABEL: N' string inside
the chalk template literal makes the token a single contiguous substring
that plain-string assertions can match.
Also add vitest isolate:true + pool:'forks' to prevent vi.mock() hoisting
in one worker from leaking into concurrently-scheduled test files
(budget_enforcement flake observed in full-suite runs).
…strumentation
Under v8 coverage, dynamic imports (used inside extension.ts for both
@stellar/stellar-sdk and the RPC client) bypass vi.mock() hoisting, so the
original mocks didn't take effect and the real Keypair.fromSecret() was called
with an invalid-checksum dummy key, producing 'invalid checksum' errors instead
of the expected budget-exceeded messages.
Fixes:
- Mirror the pattern from extension.test.ts: declare module-level vi.fn()
handles, mock '../../src/rpc/client.js' (with .js extension), and top-level
await import the module under test so the mock is hoisted correctly.
- Drop the vi.mock('@stellar/stellar-sdk') workaround entirely — with the RPC
client properly mocked, Keypair.fromSecret is only called on a real valid key.
- Use keypair_source: 'env:BUDGET_TEST_SECRET_KEY' with a real valid Stellar
secret key set in beforeEach/afterEach, matching how extension.test.ts works.
AbdulmalikAlayande
added a commit
that referenced
this pull request
Sep 6, 2026
… PR #728) Adds zeroizeKeypair(), which fills a Keypair's rawSecretKey() buffer with zeros. fundChannels() now constructs a local Keypair and zeroes it in a finally block around sendPayments, covering success, failure, and thrown-error paths. This is defense-in-depth, not full erasure: the original secret-key string is an immutable JS primitive that cannot be zeroed, and the separate Keypair instance signing inside sendPayments is untouched. Both limits are documented in the zeroizeKeypair() doc comment and SECURITY.md rather than oversold. Dropped unrelated commits from the source PR's branch (an npm lockfile regen, a fleet.ts/vitest.config.ts test-infra change, and a budget_enforcement.test.ts rewrite) — none needed against current main's test suite, which already passes cleanly without them.
Collaborator
|
Merged as 0a93176. Channel account secret key buffers are now zeroed after use. |
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #428
Implements defense-in-depth key-material zeroing for
ChannelAccountPoolinsrc/core/channels.ts. TheKeypair.rawSecretKey()Buffer is overwritten with zeros immediately after each signing operation completes, shortening the window in which a heap dump or memory scrape can recover the raw ed25519 seed.Changes
src/core/channels.tszeroizeKeypair(keypair: Keypair): void— fillsrawSecretKey()(verified to return the same underlyingArrayBufferreference each call) with zeros viaBuffer.fill(0).fundChannels(): construct a localKeypair, wrapclient.sendPayments()intry/finally, callzeroizeKeypair()in thefinallyblock — zeroing occurs on success, on failure response, and on thrown errors.zeroizeKeypairexplains both what it achieves and its runtime limits.src/core/vault.tsVaultResolver's class doc comment: the resolver returns secrets as JS strings (immutable, no Buffer to zero). Callers that derive aKeypairshould callzeroizeKeypair()after the signing operation.SECURITY.mdsrc/core/channels.tsthat constructs aKeypairfrom a secret must callzeroizeKeypair()in afinallyblock. Includes explicit ✅/❌ bullets documenting what the mitigation does and does not guarantee — per the issue's instruction not to oversell.tests/core/key-zeroing.test.ts(new — 10 tests, all passing)Tests were written before implementation (TDD). Covers:
zeroizeKeypair(): buffer is all-zeros after call; no throw; idempotent; isolated to the target instance; signing + verification work correctly before zeroing (regression guard).fundChannels()zeroing:Keypair.fromSecretspy confirms the created keypair's buffer is zeroed after a successful payment and after a thrown RPC error.What this does and does NOT guarantee
Keypairobject's backingBufferis overwritten with zeros immediately after useS…stroop) is a JS primitive — immutable, possibly V8-interned, no portable way to zero itThis is defense-in-depth. It meaningfully reduces (not eliminates) the exposure window. It is not described as a hard security guarantee anywhere in the code or docs.
Checklist
SECURITY.md, and this PR — not overstatedsrc/core/aws_secrets.tsaudited — holds no raw key buffers post-resolution, no changes neededsrc/core/extension.tsnot touched (out of scope per issue)