Repository navigation
fix(sdk-core): bind GG18 offline shares to signable, enforce single use #10126
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.
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
vibhavgo
wants to merge
1
commit into
master
Choose a base branch
from
vibhavgopalkrishna/wal-2459-critical-bitgojs-gg18-external-signer-s-share-helper-accepts
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,181
−38
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,171 @@ | ||
| import { createHash } from 'crypto'; | ||
| import * as fs from 'fs'; | ||
|
|
||
| import { logger } from '@bitgo/logger'; | ||
| import { BitGoJsError } from '@bitgo/sdk-core'; | ||
|
|
||
| import assert from 'assert'; | ||
|
|
||
| const CONSUMED_SHARES_MAX_AGE_MS = 30 * 24 * 3600 * 1000; | ||
|
|
||
| /** | ||
| * Mirrors @bitgo/sdk-api decryptV1.ts `ccmNonceLength`: v1 (SJCL/CCM) decryption truncates | ||
| * the iv to the nonce length derived from the plaintext size, so trailing iv bytes do not | ||
| * affect decryption. Keep in sync with decryptV1.ts. | ||
| */ | ||
| function v1CcmNonceLength(plaintextLen: number): number { | ||
| let L = 2; | ||
| while (L < 4 && plaintextLen >= 2 ** (8 * L)) L++; | ||
| return 15 - L; | ||
| } | ||
|
|
||
| /** | ||
| * Key excludes attacker-controlled fields (e.g. walletId) so a relabeled replay is still | ||
| * rejected. The key covers the canonical decoded bytes that decryption actually consumes: | ||
| * for v1 (CCM) the iv is truncated to the effective nonce length (trailing iv bytes are | ||
| * ignored by decryption, so any iv longer than the nonce fails closed), and for v2 (GCM) the | ||
| * full iv is authenticated. Re-serializing the envelope (key order, whitespace, base64 | ||
| * padding) still collides with the original key and is rejected. | ||
| */ | ||
| export function computeConsumedShareKey(shareType: string, encryptedShare: string): string { | ||
| let envelope: Record<string, unknown>; | ||
| try { | ||
| envelope = JSON.parse(encryptedShare) as Record<string, unknown>; | ||
| } catch { | ||
| return createHash('sha256').update(`${shareType}:${encryptedShare}`).digest('hex'); | ||
| } | ||
| assert( | ||
| typeof envelope.iv === 'string' && typeof envelope.ct === 'string', | ||
| 'encrypted share envelope is missing iv/ct' | ||
| ); | ||
| const iv = Buffer.from(envelope.iv, 'base64'); | ||
| const ct = Buffer.from(envelope.ct, 'base64'); | ||
| // v2 (GCM) authenticates the full iv. v1 (CCM) truncates the iv to the nonce length | ||
| // (real SJCL envelopes carry a 16-byte iv for a 13-byte nonce), so hash only the | ||
| // effective nonce bytes — trailing iv bytes can neither change the key nor affect | ||
| // decryption. | ||
| let effectiveIv = iv; | ||
| if (envelope.v !== 2) { | ||
| const tagBytes = typeof envelope.ts === 'number' ? envelope.ts / 8 : 0; | ||
| const nonceLen = v1CcmNonceLength(ct.length - tagBytes); | ||
| if (iv.length < nonceLen) { | ||
| // decryption would reject this envelope anyway; hash the raw string for uniqueness | ||
| return createHash('sha256').update(`${shareType}:${encryptedShare}`).digest('hex'); | ||
| } | ||
| effectiveIv = iv.subarray(0, nonceLen); | ||
| } | ||
| const hash = createHash('sha256'); | ||
| hash.update(`${shareType}:`); | ||
| hash.update(effectiveIv); | ||
| hash.update(ct); | ||
| return hash.digest('hex'); | ||
| } | ||
|
|
||
| /** | ||
| * One-time consumption registry: marks wShare/oShare ciphertexts as used before the share runs; | ||
| * a second attempt with the same ciphertext throws (fail-closed against replay). | ||
| */ | ||
| export class ConsumedShareRegistry { | ||
| private readonly consumed = new Map<string, number>(); | ||
| private loaded = false; | ||
|
|
||
| constructor(private readonly filePath: string) {} | ||
|
|
||
| private load(): void { | ||
| if (this.loaded) { | ||
| return; | ||
| } | ||
| let raw: string; | ||
| try { | ||
| raw = fs.readFileSync(this.filePath, 'utf8'); | ||
| } catch (e) { | ||
| if (typeof e === 'object' && e !== null && 'code' in e && e.code === 'ENOENT') { | ||
| // No registry file yet (first run): nothing to load. | ||
| this.loaded = true; | ||
| return; | ||
| } | ||
| // An unreadable registry must not silently forget consumed shares: fail closed and | ||
| // retry the load on the next call. | ||
| const detail = e instanceof Error ? e.message : String(e); | ||
| throw new BitGoJsError(`failed to read consumed share registry from ${this.filePath}: ${detail}`); | ||
| } | ||
| try { | ||
| const parsed = JSON.parse(raw) as Record<string, number>; | ||
| for (const [key, consumedAt] of Object.entries(parsed)) { | ||
| if (typeof consumedAt === 'number') { | ||
| this.consumed.set(key, consumedAt); | ||
| } | ||
| } | ||
| this.loaded = true; | ||
| } catch { | ||
| // A corrupt registry must not silently forget consumed shares: fail closed and retry | ||
| // the load on the next call. | ||
| throw new BitGoJsError(`failed to parse consumed share registry from ${this.filePath}`); | ||
| } | ||
| } | ||
|
|
||
| markConsumed(key: string): void { | ||
| // Check-then-set is race-free only because the whole path is synchronous; do not add an | ||
| // await between the check and the set. | ||
| this.load(); | ||
| if (this.consumed.has(key)) { | ||
| throw new BitGoJsError('encrypted share has already been consumed; restart the signing session'); | ||
| } | ||
| this.consumed.set(key, Date.now()); | ||
| this.persist(); | ||
| } | ||
|
|
||
| /** Drops expired entries so the in-memory map and the persisted file stay bounded. */ | ||
| private prune(): void { | ||
| const now = Date.now(); | ||
| for (const [key, consumedAt] of this.consumed) { | ||
| if (now - consumedAt >= CONSUMED_SHARES_MAX_AGE_MS) { | ||
| this.consumed.delete(key); | ||
|
vibhavgo marked this conversation as resolved.
Resolved
|
||
| } | ||
| } | ||
| } | ||
|
|
||
| private persist(): void { | ||
| let tempPath = ''; | ||
| // Atomic write: a crash mid-write cannot truncate the live registry (a truncated file | ||
| // would fail closed on load and lock out all signing). | ||
| try { | ||
| this.prune(); | ||
| tempPath = `${this.filePath}.${process.pid}.${Math.random().toString(36).slice(2)}.tmp`; | ||
| fs.writeFileSync(tempPath, JSON.stringify(Object.fromEntries(this.consumed))); | ||
| fs.renameSync(tempPath, this.filePath); | ||
| } catch (e) { | ||
| try { | ||
| fs.unlinkSync(tempPath); | ||
| } catch { | ||
| // nothing to clean up | ||
| } | ||
| // The signer process never assumes file write access (only the separate | ||
| // fetchEncryptedPrivKeys provisioning step writes there). If the registry location | ||
| // is not writable, degrade to in-memory-only protection: the share stays consumed | ||
| // for this process, but persistence (and restart protection) is lost until the | ||
| // operator relocates the registry via --signerRegistryFilePath. | ||
| const detail = e instanceof Error ? e.message : String(e); | ||
| logger.warn( | ||
| `failed to persist consumed share registry (${detail}); ` + | ||
| `replay protection is in-memory only until ${this.filePath} is writable ` + | ||
| '(relocate via --signerRegistryFilePath if needed)' | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
vibhavgo marked this conversation as resolved.
Resolved
|
||
| const registries = new Map<string, ConsumedShareRegistry>(); | ||
|
|
||
| export function getConsumedShareRegistry(registryFilePath: string): ConsumedShareRegistry { | ||
| let registry = registries.get(registryFilePath); | ||
| if (!registry) { | ||
| registry = new ConsumedShareRegistry(registryFilePath); | ||
| registries.set(registryFilePath, registry); | ||
| } | ||
| return registry; | ||
| } | ||
|
|
||
| export function resetConsumedShareRegistriesForTesting(): void { | ||
| registries.clear(); | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.