diff --git a/modules/express/src/clientRoutes.ts b/modules/express/src/clientRoutes.ts index 8f8c6b8791..70710c5c70 100755 --- a/modules/express/src/clientRoutes.ts +++ b/modules/express/src/clientRoutes.ts @@ -1858,26 +1858,43 @@ function isTraversableObject(value: unknown): value is Record | return value !== null && typeof value === 'object'; } +/** + * Maximum number of distinct sensitive values per request whose redaction the + * error path can guarantee. Requests carrying more than this many cannot be + * verified exhaustively, so their error responses fail closed. + */ +const MAX_SENSITIVE_VALUES = 64; + +interface CollectedSensitiveValues { + values: Set; + /** True when the request contained more distinct sensitive values than can be guaranteed redactable. */ + exceeded: boolean; +} + function collectSensitiveRequestValues( value: unknown, - values = new Set(), + collected: CollectedSensitiveValues = { values: new Set(), exceeded: false }, seen = new WeakSet | unknown[]>() -): Set { +): CollectedSensitiveValues { if (!isTraversableObject(value) || seen.has(value)) { - return values; + return collected; } seen.add(value); if (Array.isArray(value)) { - value.forEach((item) => collectSensitiveRequestValues(item, values, seen)); + value.forEach((item) => collectSensitiveRequestValues(item, collected, seen)); } else { Object.entries(value).forEach(([key, item]) => { if (SENSITIVE_REQUEST_KEYS.has(key.toLowerCase()) && typeof item === 'string' && item.length > 0) { - values.add(item); + if (collected.values.size < MAX_SENSITIVE_VALUES) { + collected.values.add(item); + } else { + collected.exceeded = true; + } } - collectSensitiveRequestValues(item, values, seen); + collectSensitiveRequestValues(item, collected, seen); }); } - return values; + return collected; } function redactSensitiveValues( @@ -1886,7 +1903,63 @@ function redactSensitiveValues( ancestors = new WeakSet | unknown[]>() ): unknown { if (typeof value === 'string') { - return [...sensitiveValues].reduce((redacted, secret) => redacted.split(secret).join('[REDACTED]'), value); + if (sensitiveValues.size === 0) { + return value; + } + // Single scan over the input: + // - every secret keeps a cached next-match position that only moves + // forward, so each secret scans the string about once and the total + // cost is linear in the input length and the number of secrets, even + // when one secret matches thousands of times while another never does; + // - when matches overlap, the union of their ranges is replaced, so no + // part of any matched secret can remain in the output; + // - replaced text is never rescanned, so output length stays bounded at + // roughly 11x the input regardless of the number or content of the + // collected secrets. (The previous chained split/join passes re-matched + // the inserted [REDACTED] tokens and grew the output exponentially for + // attacker-chosen values.) + const secrets = [...sensitiveValues]; + const notSearched = -2; + const nextMatch = new Array(secrets.length).fill(notSearched); + let redacted = ''; + let fromIndex = 0; + while (fromIndex < value.length) { + let matchIndex = -1; + let matchEnd = -1; + for (let i = 0; i < secrets.length; i++) { + const cached = nextMatch[i]; + if (cached !== -1 && (cached === notSearched || cached < fromIndex)) { + nextMatch[i] = value.indexOf(secrets[i], fromIndex); + } + const index = nextMatch[i]; + if (index !== -1 && (matchIndex === -1 || index < matchIndex)) { + matchIndex = index; + matchEnd = index + secrets[i].length; + } + } + if (matchIndex === -1) { + break; + } + // extend the replacement over every secret that starts inside the matched + // span, so a secret overlapping the chosen match cannot leak its tail + let extended = true; + while (extended) { + extended = false; + for (let i = 0; i < secrets.length; i++) { + const index = nextMatch[i]; + if (index !== -1 && index >= matchIndex && index < matchEnd) { + const end = index + secrets[i].length; + if (end > matchEnd) { + matchEnd = end; + extended = true; + } + } + } + } + redacted += `${value.slice(fromIndex, matchIndex)}[REDACTED]`; + fromIndex = matchEnd; + } + return redacted + value.slice(fromIndex); } if (!isTraversableObject(value)) { return value; @@ -1910,36 +1983,58 @@ function redactSensitiveValues( } function handleRequestHandlerError(res: express.Response, error: unknown, requestBody?: unknown) { - let err; - if (error instanceof Error) { - err = error; - } else if (typeof error === 'string') { - err = new BitGoExpressError('(string_error) ' + error); - } else { - err = new BitGoExpressError('(object_error) ' + JSON.stringify(error)); - } - - const sensitiveValues = collectSensitiveRequestValues(requestBody); - const message = (redactSensitiveValues(err.message, sensitiveValues) as string) || 'local error'; - let result = redactSensitiveValues(err.result || { error: message }, sensitiveValues); - result = _.extend({}, result, { - message, - name: err.name || 'BitGoExpressError', - bitgoJsVersion: version, - bitgoExpressVersion: pjson.version, - ...(err.code ? { code: err.code } : {}), - }); - const status = err.status || 500; - if (!(status >= 200 && status < 300)) { - console.log('error %s: %s', status, message); - } - if (status >= 500 && status <= 599) { - if (err.response && err.response.request) { - console.log(`failed to make ${err.response.request.method} request to ${err.response.request.url}`); + try { + let err; + if (error instanceof Error) { + err = error; + } else if (typeof error === 'string') { + err = new BitGoExpressError('(string_error) ' + error); + } else { + err = new BitGoExpressError('(object_error) ' + JSON.stringify(error)); + } + + const { values: sensitiveValues, exceeded: exceededSensitiveValues } = collectSensitiveRequestValues(requestBody); + // Values of any length (including 1-3 characters) are redacted, so + // nothing sensitive is echoed. Only a request carrying more distinct + // sensitive values than can be guaranteed redactable fails closed - with + // a message naming that cause - so callers keep their real, redacted + // error details in every other case. + const message = exceededSensitiveValues + ? 'error details withheld: too many sensitive values in request' + : (redactSensitiveValues(err.message, sensitiveValues) as string) || 'local error'; + let result = exceededSensitiveValues + ? { error: message } + : redactSensitiveValues(err.result || { error: message }, sensitiveValues); + result = _.extend({}, result, { + message, + name: err.name || 'BitGoExpressError', + bitgoJsVersion: version, + bitgoExpressVersion: pjson.version, + ...(err.code ? { code: err.code } : {}), + }); + const status = err.status || 500; + if (!(status >= 200 && status < 300)) { + console.log('error %s: %s', status, message); + } + if (status >= 500 && status <= 599 && !exceededSensitiveValues) { + if (err.response && err.response.request) { + console.log(`failed to make ${err.response.request.method} request to ${err.response.request.url}`); + } + console.log(redactSensitiveValues(err.stack, sensitiveValues)); + } + res.status(status).send(result); + } catch (e) { + // Building the error response itself failed (for example redaction or + // serialization of a pathological thrown value). Never let this failure + // escape: the wrapper's catch block has already run, so an escaped throw + // becomes an unobserved rejection and the request hangs until the connect + // timeout. Respond generically instead, and log the error name only - + // its message may contain unredacted secret content. + if (!res.headersSent) { + res.status(500).send({ message: 'internal error' }); } - console.log(redactSensitiveValues(err.stack, sensitiveValues)); + console.log('error-response construction failed:', e instanceof Error ? e.name : typeof e); } - res.status(status).send(result); } /** diff --git a/modules/express/test/unit/clientRoutes/index.ts b/modules/express/test/unit/clientRoutes/index.ts index 8b6603e8b9..75b1582805 100644 --- a/modules/express/test/unit/clientRoutes/index.ts +++ b/modules/express/test/unit/clientRoutes/index.ts @@ -236,5 +236,224 @@ describe('common methods', () => { send.firstCall.args[0].secret.should.equal('[REDACTED]'); send.firstCall.args[0].self.should.equal('[Circular]'); }); + + it('should redact short sensitive values in a single pass', async () => { + // attack-shaped body: many 1-3 character values under sensitive keys, + // chosen as substrings of the [REDACTED] replacement token + const handler = sandbox.stub().rejects(new Error('failed with a sensitive message')); + const req = { + body: Object.fromEntries( + ['e', '[', 'RE', 'ED', ']', 'D]', 'A', 'CT'].map((v, i) => [`k${i}`, { password: v }]) + ), + } as unknown as express.Request; + const status = sandbox.stub().returnsThis(); + const send = sandbox.stub().returnsThis(); + const res = { status, send } as unknown as express.Response; + const next = sandbox.stub(); + const consoleLog = sandbox.stub(console, 'log'); + + await promiseWrapper(handler)(req, res, next); + + // short values are redacted too, and the single-pass scan cannot amplify: + // the message is mangled but stays bounded + status.calledWith(500).should.be.true(); + const response = send.firstCall.args[0]; + response.message.should.equal('fail[REDACTED]d with a s[REDACTED]nsitiv[REDACTED] m[REDACTED]ssag[REDACTED]'); + JSON.stringify(response).should.not.containEql('sensitive message'); + consoleLog.args.flat().join(' ').should.not.containEql('sensitive message'); + }); + + it('should redact the passphrase normally when short junk values are present', async () => { + const passphrase = 'real-wallet-passphrase'; + const handler = sandbox.stub().rejects(new Error(`failed with ${passphrase}`)); + const req = { + body: { + walletPassphrase: passphrase, + ...Object.fromEntries(['e', '[', 'RE', 'ED]'].map((v, i) => [`k${i}`, { password: v }])), + }, + } as unknown as express.Request; + const status = sandbox.stub().returnsThis(); + const send = sandbox.stub().returnsThis(); + const res = { status, send } as unknown as express.Response; + const next = sandbox.stub(); + + await promiseWrapper(handler)(req, res, next); + + // short junk values no longer degrade the response: the real message is + // returned with the passphrase redacted (the junk 'e' is redacted as well) + send.firstCall.args[0].message.should.equal('fail[REDACTED]d with [REDACTED]'); + }); + + it('should redact secrets containing special characters literally', async () => { + const secret = 'p@$$w0rd.*+?(x)'; + const handler = sandbox.stub().rejects(new Error(`failed with ${secret}`)); + const req = { body: { walletPassphrase: secret } } as unknown as express.Request; + const status = sandbox.stub().returnsThis(); + const send = sandbox.stub().returnsThis(); + const res = { status, send } as unknown as express.Response; + const next = sandbox.stub(); + + await promiseWrapper(handler)(req, res, next); + + send.firstCall.args[0].message.should.equal('failed with [REDACTED]'); + }); + + it('should redact overlapping secrets as a single token', async () => { + const handler = sandbox.stub().rejects(new Error('failed with REDACTED] token')); + const req = { + body: Object.fromEntries([ + ['password', 'REDA'], + ['prv', 'REDACTED]'], + ]), + } as unknown as express.Request; + const status = sandbox.stub().returnsThis(); + const send = sandbox.stub().returnsThis(); + const res = { status, send } as unknown as express.Response; + const next = sandbox.stub(); + + await promiseWrapper(handler)(req, res, next); + + // the union of the overlapping matches is replaced as one token + send.firstCall.args[0].message.should.equal('failed with [REDACTED] token'); + }); + + it('should not leak the tail of a secret overlapped by a shorter one', async () => { + const handler = sandbox.stub().rejects(new Error('failed with abcdef')); + const req = { + body: { + prv: 'cdef', + password: 'abc', + }, + } as unknown as express.Request; + const status = sandbox.stub().returnsThis(); + const send = sandbox.stub().returnsThis(); + const res = { status, send } as unknown as express.Response; + const next = sandbox.stub(); + + await promiseWrapper(handler)(req, res, next); + + // 'abc' matches first, but the replacement extends over 'cdef' which + // starts inside its span, so no part of either secret leaks + const response = send.firstCall.args[0]; + response.message.should.equal('failed with [REDACTED]'); + JSON.stringify(response).should.not.containEql('cdef'); + JSON.stringify(response).should.not.containEql('abcdef'); + }); + + it('should stay linear when one secret matches many times and another never', async function () { + // regression for the quadratic rescan: an absent secret must not be + // re-searched once per match of the present secret + const hugeMessage = 'a'.repeat(200000); + const handler = sandbox.stub().rejects(new Error(hugeMessage)); + const req = { + body: { + password: 'aaaa', + nested: { secret: 'zzzz-not-present-anywhere' }, + }, + } as unknown as express.Request; + const status = sandbox.stub().returnsThis(); + const send = sandbox.stub().returnsThis(); + const res = { status, send } as unknown as express.Response; + const next = sandbox.stub(); + sandbox.stub(console, 'log'); + + const startedAt = Date.now(); + await promiseWrapper(handler)(req, res, next); + const elapsedMs = Date.now() - startedAt; + + status.calledWith(500).should.be.true(); + send.firstCall.args[0].message.should.equal('[REDACTED]'.repeat(50000)); + elapsedMs.should.be.below(10000); + }); + + it('should fail closed with a specific message when the collected secret cap is exceeded', async () => { + const passphrase = 'real-wallet-passphrase'; + const handler = sandbox.stub().rejects(new Error(`failed with ${passphrase}`)); + const junk: Record = {}; + for (let i = 0; i < 80; i++) { + junk[`junk${i}`] = { password: `junk-passphrase-value-${i}` }; + } + const req = { body: { walletPassphrase: passphrase, ...junk } } as unknown as express.Request; + const status = sandbox.stub().returnsThis(); + const send = sandbox.stub().returnsThis(); + const res = { status, send } as unknown as express.Response; + const next = sandbox.stub(); + + await promiseWrapper(handler)(req, res, next); + + // too many distinct sensitive values to guarantee redaction: the + // response names that cause and echoes nothing + const response = send.firstCall.args[0]; + response.message.should.equal('error details withheld: too many sensitive values in request'); + response.error.should.equal('error details withheld: too many sensitive values in request'); + JSON.stringify(response).should.not.containEql(passphrase); + }); + + it('should not treat empty sensitive values as secrets', async () => { + const handler = sandbox.stub().rejects(new Error('plain failure message')); + const req = { body: { walletPassphrase: '' } } as unknown as express.Request; + const status = sandbox.stub().returnsThis(); + const send = sandbox.stub().returnsThis(); + const res = { status, send } as unknown as express.Response; + const next = sandbox.stub(); + + await promiseWrapper(handler)(req, res, next); + + // an empty sensitive value carries no secret and is not collected + send.firstCall.args[0].message.should.equal('plain failure message'); + }); + + it('should still redact full-length encrypted key material', async () => { + const encryptedPrv = `{"iv":"${'a'.repeat( + 32 + )}","v":1,"iter":10000,"ks":256,"ts":64,"mode":"ccm","adata":"","cipher":"aes","ct":"${'b'.repeat(2048)}"}`; + const handler = sandbox.stub().rejects(new Error(`failed to decrypt ${encryptedPrv}`)); + const req = { body: { encryptedprv: encryptedPrv } } as unknown as express.Request; + const status = sandbox.stub().returnsThis(); + const send = sandbox.stub().returnsThis(); + const res = { status, send } as unknown as express.Response; + const next = sandbox.stub(); + + await promiseWrapper(handler)(req, res, next); + + send.firstCall.args[0].message.should.equal('failed to decrypt [REDACTED]'); + JSON.stringify(send.firstCall.args[0]).should.not.containEql(encryptedPrv); + }); + + it('should return a generic 500 when building the error response itself fails', async () => { + // a thrown object containing a BigInt makes JSON.stringify throw inside + // handleRequestHandlerError + const handler = sandbox.stub().rejects({ message: 'weird', value: 10n }); + const req = { body: {} } as unknown as express.Request; + const status = sandbox.stub().returnsThis(); + const send = sandbox.stub().returnsThis(); + const res = { headersSent: false, status, send } as unknown as express.Response; + const next = sandbox.stub(); + const consoleLog = sandbox.stub(console, 'log'); + + // resolves instead of escaping as an unhandled rejection, which would + // leave the request hanging until the connect timeout + await promiseWrapper(handler)(req, res, next); + + status.calledWith(500).should.be.true(); + send.calledWith({ message: 'internal error' }).should.be.true(); + consoleLog.args.flat().join(' ').should.not.containEql('weird'); + }); + + it('should not attempt a second response when headers were already sent', async () => { + const handler = sandbox.stub().rejects({ message: 'weird', value: 10n }); + const req = { body: {} } as unknown as express.Request; + const status = sandbox.stub().returnsThis(); + const send = sandbox.stub().returnsThis(); + const res = { headersSent: true, status, send } as unknown as express.Response; + const next = sandbox.stub(); + const consoleLog = sandbox.stub(console, 'log'); + + await promiseWrapper(handler)(req, res, next); + + status.called.should.be.false(); + send.called.should.be.false(); + consoleLog.args.flat().join(' ').should.containEql('TypeError'); + }); }); });