From 1a49c32c38ecf87427faa4429eb6e2c1692edf98 Mon Sep 17 00:00:00 2001 From: John-David Dalton Date: Sat, 28 Mar 2026 16:29:13 -0400 Subject: [PATCH] fix: harden HTTP request security against downgrade and timing attacks --- package.json | 2 +- pnpm-lock.yaml | 30 +++++++++++++++--------------- scripts/test/main.mjs | 2 +- src/dlx/binary.ts | 22 +++++++++++++++------- src/http-request.ts | 40 ++++++++++++++++++++++++++++++++++++---- 5 files changed, 68 insertions(+), 28 deletions(-) diff --git a/package.json b/package.json index 29fea8a..5484ba1 100644 --- a/package.json +++ b/package.json @@ -815,7 +815,7 @@ "hosted-git-info": "8.1.0", "isexe": "3.1.1", "lru-cache": "11.2.2", - "minimatch": "9.0.5", + "minimatch": "9.0.6", "minipass": "7.1.3", "minipass-fetch": "4.0.1", "minipass-sized": "1.0.3", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4981b13..e5fa61b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -22,7 +22,7 @@ overrides: hosted-git-info: 8.1.0 isexe: 3.1.1 lru-cache: 11.2.2 - minimatch: 9.0.5 + minimatch: 9.0.6 minipass: 7.1.3 minipass-fetch: 4.0.1 minipass-sized: 1.0.3 @@ -2463,8 +2463,8 @@ packages: resolution: {integrity: sha512-z0yWI+4FDrrweS8Zmt4Ej5HdJmky15+L2e6Wgn3+iK5fWzb6T3fhNFq2+MeTRb064c6Wr4N/wv0DzQTjNzHNGQ==} engines: {node: '>=10'} - minimatch@9.0.5: - resolution: {integrity: sha512-G6T0ZX48xgozx7587koeX9Ys2NYy6Gmv//P89sEte9V9whIapMNF4idKxnW2QtCcLiTWlb/wfCabAtAFWhhBow==} + minimatch@9.0.6: + resolution: {integrity: sha512-kQAVowdR33euIqeA0+VZTDqU+qo1IeVY+hrKYtZMio3Pg0P0vuh/kwRylLUddJhB6pf3q/botcOvRtx4IN1wqQ==} engines: {node: '>=16 || 14 >=14.17'} minimist@1.2.8: @@ -3615,7 +3615,7 @@ snapshots: dependencies: '@eslint/object-schema': 2.1.7 debug: 4.4.3(supports-color@10.2.2) - minimatch: 9.0.5 + minimatch: 9.0.6 transitivePeerDependencies: - supports-color @@ -3634,7 +3634,7 @@ snapshots: ignore: 5.3.2 import-fresh: 3.3.1 js-yaml: 4.1.1 - minimatch: 9.0.5 + minimatch: 9.0.6 strip-json-comments: 3.1.1 transitivePeerDependencies: - supports-color @@ -3839,7 +3839,7 @@ snapshots: hosted-git-info: 8.1.0 json-stringify-nice: 1.1.4 lru-cache: 11.2.2 - minimatch: 9.0.5 + minimatch: 9.0.6 nopt: 8.1.0 npm-install-checks: 7.1.2 npm-package-arg: 12.0.2 @@ -3888,7 +3888,7 @@ snapshots: '@npmcli/name-from-folder': 4.0.0 '@npmcli/package-json': 7.0.0 glob: 13.0.6 - minimatch: 9.0.5 + minimatch: 9.0.6 '@npmcli/metavuln-calculator@9.0.3(supports-color@10.2.2)': dependencies: @@ -4219,7 +4219,7 @@ snapshots: '@tufjs/models@4.1.0': dependencies: '@tufjs/canonical-json': 2.0.0 - minimatch: 9.0.5 + minimatch: 9.0.6 '@types/cacheable-request@6.0.3': dependencies: @@ -4820,7 +4820,7 @@ snapshots: is-glob: 4.0.3 json-stable-stringify-without-jsonify: 1.0.1 lodash.merge: 4.6.2 - minimatch: 9.0.5 + minimatch: 9.0.6 natural-compare: 1.4.0 optionator: 0.9.4 optionalDependencies: @@ -4964,7 +4964,7 @@ snapshots: dependencies: foreground-child: 3.3.1 jackspeak: 3.4.3 - minimatch: 9.0.5 + minimatch: 9.0.6 minipass: 7.1.3 package-json-from-dist: 1.0.1 path-scurry: 1.11.1 @@ -4973,14 +4973,14 @@ snapshots: dependencies: foreground-child: 3.3.1 jackspeak: 4.2.3 - minimatch: 9.0.5 + minimatch: 9.0.6 minipass: 7.1.3 package-json-from-dist: 1.0.1 path-scurry: 2.0.2 glob@13.0.6: dependencies: - minimatch: 9.0.5 + minimatch: 9.0.6 minipass: 7.1.3 path-scurry: 2.0.2 @@ -5059,7 +5059,7 @@ snapshots: ignore-walk@8.0.0: dependencies: - minimatch: 9.0.5 + minimatch: 9.0.6 ignore@5.3.2: {} @@ -5336,7 +5336,7 @@ snapshots: mimic-response@3.1.0: {} - minimatch@9.0.5: + minimatch@9.0.6: dependencies: brace-expansion: 5.0.5 @@ -6038,7 +6038,7 @@ snapshots: type-coverage-core@2.29.7(typescript@5.9.2): dependencies: fast-glob: 3.3.3 - minimatch: 9.0.5 + minimatch: 9.0.6 normalize-path: 3.0.0 tslib: 2.8.1 tsutils: 3.21.0(typescript@5.9.2) diff --git a/scripts/test/main.mjs b/scripts/test/main.mjs index 13eabce..1dedd42 100644 --- a/scripts/test/main.mjs +++ b/scripts/test/main.mjs @@ -198,7 +198,7 @@ async function runTests( const { mode, reason, tests: testsToRun } = testInfo // No tests needed - if (testsToRun === null) { + if (testsToRun == null) { logger.substep('No relevant changes detected, skipping tests') return 0 } diff --git a/src/dlx/binary.ts b/src/dlx/binary.ts index 2ebf51d..58a99b8 100644 --- a/src/dlx/binary.ts +++ b/src/dlx/binary.ts @@ -566,13 +566,21 @@ export async function downloadBinaryFile( .digest('base64') const actualIntegrity = `sha512-${hash}` - // Verify integrity if provided. - if (integrity && actualIntegrity !== integrity) { - // Clean up invalid file. - await safeDelete(destPath) - throw new Error( - `Integrity mismatch: expected ${integrity}, got ${actualIntegrity}`, - ) + // Verify integrity if provided (constant-time comparison). + if (integrity) { + const integrityMatch = + actualIntegrity.length === integrity.length && + crypto.timingSafeEqual( + Buffer.from(actualIntegrity), + Buffer.from(integrity), + ) + if (!integrityMatch) { + // Clean up invalid file. + await safeDelete(destPath) + throw new Error( + `Integrity mismatch: expected ${integrity}, got ${actualIntegrity}`, + ) + } } // Make executable on POSIX systems. diff --git a/src/http-request.ts b/src/http-request.ts index 3fc24e1..a04f72b 100644 --- a/src/http-request.ts +++ b/src/http-request.ts @@ -776,6 +776,17 @@ async function httpDownloadAttempt( ? res.headers.location : new URL(res.headers.location, url).toString() + // Reject HTTPS-to-HTTP downgrade redirects. + const redirectParsed = new URL(redirectUrl) + if (isHttps && redirectParsed.protocol !== 'https:') { + reject( + new Error( + `Redirect from HTTPS to HTTP is not allowed: ${redirectUrl}`, + ), + ) + return + } + resolve( httpDownloadAttempt(redirectUrl, destPath, { ca, @@ -946,6 +957,17 @@ async function httpRequestAttempt( ? res.headers.location : new URL(res.headers.location, url).toString() + // Reject HTTPS-to-HTTP downgrade redirects. + const redirectParsed = new URL(redirectUrl) + if (isHttps && redirectParsed.protocol !== 'https:') { + reject( + new Error( + `Redirect from HTTPS to HTTP is not allowed: ${redirectUrl}`, + ), + ) + return + } + resolve( httpRequestAttempt(redirectUrl, { body, @@ -1141,8 +1163,10 @@ export async function httpDownload( // Download to a temp file first, then atomically rename to destination. // This prevents partial/corrupted files at the destination path if download fails, // and preserves the original file (if any) until download succeeds. + const crypto = getCrypto() const fs = getFs() - const tempPath = `${destPath}.download` + const tempSuffix = crypto.randomBytes(6).toString('hex') + const tempPath = `${destPath}.${tempSuffix}.download` // Clean up any stale temp file from a previous failed download. if (fs.existsSync(tempPath)) { @@ -1165,7 +1189,6 @@ export async function httpDownload( // Verify checksum if sha256 hash is provided. if (sha256) { - const crypto = getCrypto() // eslint-disable-next-line no-await-in-loop const fileContent = await fs.promises.readFile(tempPath) const computedHash = crypto @@ -1173,12 +1196,21 @@ export async function httpDownload( .update(fileContent) .digest('hex') - if (computedHash !== sha256.toLowerCase()) { + const expectedHash = sha256.toLowerCase() + + // Use constant-time comparison to prevent timing attacks. + if ( + computedHash.length !== expectedHash.length || + !crypto.timingSafeEqual( + Buffer.from(computedHash), + Buffer.from(expectedHash), + ) + ) { // eslint-disable-next-line no-await-in-loop await safeDelete(tempPath) throw new Error( `Checksum verification failed for ${url}\n` + - `Expected: ${sha256.toLowerCase()}\n` + + `Expected: ${expectedHash}\n` + `Computed: ${computedHash}`, ) }