Some fixes

This commit is contained in:
Reinier Criel 2025-12-08 13:38:38 -08:00 committed by Sander Declerck
parent f3b7847697
commit 3de53e1f8a
No known key found for this signature in database
2 changed files with 121 additions and 8 deletions

View file

@ -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;

View file

@ -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`);
}
});
});