From 2bc6d249de42f0137c13d6fc7e3d377b161f619a Mon Sep 17 00:00:00 2001 From: Reinier Criel Date: Mon, 8 Dec 2025 13:38:38 -0800 Subject: [PATCH] Some fixes --- .../src/registryProxy/certBundle.js | 45 ++++++++-- .../src/registryProxy/certBundle.spec.js | 84 +++++++++++++++++++ 2 files changed, 121 insertions(+), 8 deletions(-) diff --git a/packages/safe-chain/src/registryProxy/certBundle.js b/packages/safe-chain/src/registryProxy/certBundle.js index 518d1d1..98810d6 100644 --- a/packages/safe-chain/src/registryProxy/certBundle.js +++ b/packages/safe-chain/src/registryProxy/certBundle.js @@ -15,6 +15,8 @@ import { ui } from "../environment/userInteraction.js"; */ function isParsable(pem) { if (!pem || typeof pem !== "string") return false; + // Normalize Windows CRLF to LF to ensure consistent parsing + pem = pem.replace(/\r\n/g, "\n").replace(/\r/g, "\n"); const begin = "-----BEGIN CERTIFICATE-----"; const end = "-----END CERTIFICATE-----"; const blocks = []; @@ -95,6 +97,15 @@ export function getCombinedCaBundlePath() { return cachedPath; } +/** + * Normalize path + * @param {string} p - Path to normalize + * @returns {string} + */ +function normalizePathF(p) { + return p.replace(/\\/g, "/"); +} + /** * Read and validate user certificate file * @param {string} certPath - Path to certificate file @@ -102,32 +113,50 @@ export function getCombinedCaBundlePath() { */ function readUserCertificateFile(certPath) { try { - // Perform security checks before reading + // 1) Basic validation if (typeof certPath !== "string" || certPath.trim().length === 0) { return null; } - if (certPath.includes("..") || certPath.includes("//") || certPath.includes("\\\\")) { + // 2) Reject path traversal attempts (normalize backslashes first for Windows paths) + const normalizedPath = normalizePathF(certPath); + if (normalizedPath.includes("..")) { return null; } - if (!fs.existsSync(certPath)) { + // 3) Check if file exists and is not a directory or symlink + let stats; + try { + stats = fs.lstatSync(certPath); + } catch { + // File doesn't exist or can't be accessed return null; } - const stats = fs.lstatSync(certPath); - if (!stats.isFile() || stats.isSymbolicLink()) { + if (!stats.isFile()) { + // Reject directories and symlinks + return null; + } + + // 4) Read file content + let content; + try { + content = fs.readFileSync(certPath, "utf8"); + } catch { return null; } - const content = fs.readFileSync(certPath, "utf8"); if (!content || typeof content !== "string") { return null; } - // 6) Validate PEM format + // 5) Validate PEM format if (!isParsable(content)) { - return null; + // Fallback: accept if it at least contains PEM delimiters + // (covers edge cases with unusual formatting that X509Certificate might reject) + if (!content.includes("-----BEGIN CERTIFICATE-----") || !content.includes("-----END CERTIFICATE-----")) { + return null; + } } return content; diff --git a/packages/safe-chain/src/registryProxy/certBundle.spec.js b/packages/safe-chain/src/registryProxy/certBundle.spec.js index 38b313d..dd718af 100644 --- a/packages/safe-chain/src/registryProxy/certBundle.spec.js +++ b/packages/safe-chain/src/registryProxy/certBundle.spec.js @@ -272,4 +272,88 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { const contents = fs.readFileSync(bundlePath, "utf8"); assert.match(contents, /-----BEGIN CERTIFICATE-----/, "Should contain Safe Chain CA"); }); + + it("accepts files with CRLF line endings (Windows-style)", async () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "certtest-")); + const safeChainPath = path.join(tmpDir, "safechain.pem"); + fs.writeFileSync(safeChainPath, getValidCert(), "utf8"); + + // Create a real file with CRLF content to test Windows line ending support + const userCertPath = path.join(tmpDir, "user-cert-crlf.pem"); + const crlfCert = getValidCert().replace(/\n/g, "\r\n"); + fs.writeFileSync(userCertPath, crlfCert, "utf8"); + + mock.module("./certUtils.js", { + namedExports: { + getCaCertPath: () => safeChainPath, + }, + }); + + const { getCombinedCaBundlePathWithUserCerts } = await import("./certBundle.js"); + const bundlePath = getCombinedCaBundlePathWithUserCerts(userCertPath); + assert.ok(fs.existsSync(bundlePath), "Bundle path should exist"); + const contents = fs.readFileSync(bundlePath, "utf8"); + const certCount = (contents.match(/-----BEGIN CERTIFICATE-----/g) || []).length; + assert.ok(certCount >= 2, "Bundle should contain Safe Chain and user certificates with CRLF"); + }); + + it("detects and handles Windows-style path syntax (drive letters and UNC)", async () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "certtest-")); + const safeChainPath = path.join(tmpDir, "safechain.pem"); + fs.writeFileSync(safeChainPath, getValidCert(), "utf8"); + + mock.module("./certUtils.js", { + namedExports: { + getCaCertPath: () => safeChainPath, + }, + }); + + const { getCombinedCaBundlePathWithUserCerts } = await import("./certBundle.js"); + + // Test that Windows path syntax is recognized (even if files don't exist on macOS/Linux) + // These should gracefully fail (return Safe Chain CA only) rather than crash + const winPaths = [ + "C:\\temp\\cert.pem", + "D:\\Users\\name\\certs\\ca.pem", + "\\\\server\\share\\cert.pem" + ]; + + for (const winPath of winPaths) { + const bundlePath = getCombinedCaBundlePathWithUserCerts(winPath); + assert.ok(fs.existsSync(bundlePath), `Bundle should exist for ${winPath}`); + const contents = fs.readFileSync(bundlePath, "utf8"); + assert.match(contents, /-----BEGIN CERTIFICATE-----/, "Should contain Safe Chain CA"); + } + }); + + it("rejects path traversal with Windows-style paths (C:\\temp\\..\\etc\\passwd)", async () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "certtest-")); + const safeChainPath = path.join(tmpDir, "safechain.pem"); + fs.writeFileSync(safeChainPath, getValidCert(), "utf8"); + + mock.module("./certUtils.js", { + namedExports: { + getCaCertPath: () => safeChainPath, + }, + }); + + const { getCombinedCaBundlePathWithUserCerts } = await import("./certBundle.js"); + + // Test various Windows-style traversal attempts + const traversalPaths = [ + "C:\\temp\\..\\etc\\passwd", + "D:\\Users\\..\\..\\Windows\\System32", + "\\\\server\\share\\..\\admin", + "../../../etc/passwd", // Unix-style for comparison + ]; + + for (const badPath of traversalPaths) { + const bundlePath = getCombinedCaBundlePathWithUserCerts(badPath); + assert.ok(fs.existsSync(bundlePath), "Bundle path should exist"); + const contents = fs.readFileSync(bundlePath, "utf8"); + // Only Safe Chain CA should be present (user cert rejected due to traversal) + const certCount = (contents.match(/-----BEGIN CERTIFICATE-----/g) || []).length; + assert.strictEqual(certCount, 1, `Traversal path ${badPath} should be rejected; only Safe Chain CA included`); + } + }); });