diff --git a/packages/code-analyzer-pmd-engine/src/config.ts b/packages/code-analyzer-pmd-engine/src/config.ts index d7a79210..49955a4e 100644 --- a/packages/code-analyzer-pmd-engine/src/config.ts +++ b/packages/code-analyzer-pmd-engine/src/config.ts @@ -42,7 +42,7 @@ export type PmdEngineConfig = { // List of xml ruleset files containing custom PMD rules to be made available for rule selection. // Each ruleset must be an xml file that is either:: // - on disk (provided as an absolute path or a relative path to 'config_root') - // - or a relative resource found on the Java classpath. + // - or a relative resource found on the Java classpath (or, for a ruleset that references another ruleset file by a path relative to the folder you run Code Analyzer from, that referenced file). // Not all custom rules can be fully defined within an xml ruleset file. For example, Java based rules may be defined in jar files. // In these cases, you will need to also add your additional files to the Java classpath using the 'java_classpath_entries' field. // See https://pmd.github.io/pmd/pmd_userdocs_making_rulesets.html diff --git a/packages/code-analyzer-pmd-engine/src/messages.ts b/packages/code-analyzer-pmd-engine/src/messages.ts index 6884c9dd..9339e3bb 100644 --- a/packages/code-analyzer-pmd-engine/src/messages.ts +++ b/packages/code-analyzer-pmd-engine/src/messages.ts @@ -27,7 +27,7 @@ const MESSAGE_CATALOG : { [key: string]: string } = { `List of xml ruleset files containing custom PMD rules to be made available for rule selection.\n` + `Each ruleset must be an xml file that is either:\n` + ` - on disk (provided as an absolute path or a relative path to 'config_root')\n` + - ` - or a relative resource found on the Java classpath.\n` + + ` - or a relative resource found on the Java classpath (or, for a ruleset that references another ruleset file by a path relative to the folder you run Code Analyzer from, that referenced file).\n` + `Not all custom rules can be fully defined within an xml ruleset file. For example, Java based rules may be defined in jar files.\n` + `In these cases, you will need to also add your additional files to the Java classpath using the 'java_classpath_entries' field.\n` + `See https://pmd.github.io/pmd/pmd_userdocs_making_rulesets.html to learn more about PMD rulesets.`, diff --git a/packages/code-analyzer-pmd-engine/src/pmd-wrapper.ts b/packages/code-analyzer-pmd-engine/src/pmd-wrapper.ts index ed26a98f..8c4efae3 100644 --- a/packages/code-analyzer-pmd-engine/src/pmd-wrapper.ts +++ b/packages/code-analyzer-pmd-engine/src/pmd-wrapper.ts @@ -1,6 +1,7 @@ import {JavaCommandExecutor} from "@salesforce/code-analyzer-engine-api/utils"; import path from "node:path"; import fs from "node:fs"; +import {stageReferencedRulesets} from "./ruleset-staging"; import {getMessageFromCatalog, LogLevel, SHARED_MESSAGE_CATALOG} from "@salesforce/code-analyzer-engine-api"; const PMD_WRAPPER_JAVA_CLASS: string = "com.salesforce.sfca.pmdwrapper.PmdWrapper"; @@ -78,6 +79,26 @@ export class PmdWrapperInvoker { this.emitLogEvent = emitLogEvent; } + /** + * Creates the Java classpath. Note that the Java process is pinned to the engine's install folder (instead of the + * inherited, possibly untrusted, folder being scanned) to avoid CWE-427. But PMD resolves refs inside of custom + * rulesets (like ) relative to the cwd or the classpath. To keep those refs + * resolving without exposing the entire repository to the JVM as classes/resources, we stage copies of only the + * referenced xml ruleset files into a dedicated folder that we place last on the classpath. + */ + private async createJavaClassPaths(rulesets: string[], workingFolder: string): Promise { + const classPaths: string[] = [ + path.join(PMD_WRAPPER_LIB_FOLDER, '*'), + ...this.userProvidedJavaClasspathEntries.map(toJavaClasspathEntry) + ]; + const stagingFolder: string | undefined = await stageReferencedRulesets( + rulesets, process.cwd(), path.join(workingFolder, 'stagedRulesets')); + if (stagingFolder) { + classPaths.push(stagingFolder + path.sep); // Last so that it has the lowest precedence + } + return classPaths; + } + async invokeDescribeCommand(customRulesets: string[], pmdRuleLanguageIds: string[], workingFolder: string, emitProgress: (percComplete: number) => void): Promise { @@ -87,10 +108,7 @@ export class PmdWrapperInvoker { emitProgress(10); const javaCmdArgs: string[] = [PMD_WRAPPER_JAVA_CLASS, 'describe', pmdRulesOutputFile, customRulesetsListFile, pmdRuleLanguageIds.join(',')]; - const javaClassPaths: string[] = [ - path.join(PMD_WRAPPER_LIB_FOLDER, '*'), - ... this.userProvidedJavaClasspathEntries.map(toJavaClasspathEntry) - ]; + const javaClassPaths: string[] = await this.createJavaClassPaths(customRulesets, workingFolder); await this.javaCommandExecutor.exec(javaCmdArgs, javaClassPaths, (stdOutMsg) => { if (stdOutMsg.startsWith(STDOUT_ERROR_MARKER)) { const errorMessage: string = stdOutMsg.slice(STDOUT_ERROR_MARKER.length).replaceAll('{NEWLINE}','\n'); @@ -138,10 +156,8 @@ export class PmdWrapperInvoker { const resultsOutputFile: string = path.join(workingFolder, 'resultsFile.json'); const javaCmdArgs: string[] = [PMD_WRAPPER_JAVA_CLASS, 'run', inputFile, resultsOutputFile]; - const javaClassPaths: string[] = [ - path.join(PMD_WRAPPER_LIB_FOLDER, '*'), - ... this.userProvidedJavaClasspathEntries.map(toJavaClasspathEntry) - ]; + const javaClassPaths: string[] = await this.createJavaClassPaths( + [...new Set(pmdRuleInfoList.map(ruleInfo => ruleInfo.ruleSetFile))], workingFolder); this.emitLogEvent(LogLevel.Fine, `Calling JAVA command with class path containing ${JSON.stringify(javaClassPaths)} and arguments: ${JSON.stringify(javaCmdArgs)}`); await this.javaCommandExecutor.exec(javaCmdArgs, javaClassPaths, (stdOutMsg: string) => { if (stdOutMsg.startsWith(STDOUT_PROGRESS_MARKER)) { @@ -189,10 +205,7 @@ export class PmdWrapperInvoker { const resultsOutputFile: string = path.join(workingFolder, 'astDumpResults.json'); const javaCmdArgs: string[] = [PMD_WRAPPER_JAVA_CLASS, 'ast-dump', inputFile, resultsOutputFile]; - const javaClassPaths: string[] = [ - path.join(PMD_WRAPPER_LIB_FOLDER, '*'), - ...this.userProvidedJavaClasspathEntries.map(toJavaClasspathEntry) - ]; + const javaClassPaths: string[] = await this.createJavaClassPaths([], workingFolder); this.emitLogEvent(LogLevel.Fine, `Calling AST dump for file: ${fileToDump}`); diff --git a/packages/code-analyzer-pmd-engine/src/ruleset-staging.ts b/packages/code-analyzer-pmd-engine/src/ruleset-staging.ts new file mode 100644 index 00000000..14572b00 --- /dev/null +++ b/packages/code-analyzer-pmd-engine/src/ruleset-staging.ts @@ -0,0 +1,112 @@ +import fs from "node:fs"; +import path from "node:path"; + +// Matches the "ref" attribute of and style elements. +const RULE_REF_REGEX: RegExp = /]*?\bref\s*=\s*["']([^"']+)["']/gi; + +/** + * PMD resolves refs inside of a ruleset (like ) relative to the folder it was run + * from or from the Java classpath. Since we pin the Java process away from the (untrusted) folder being scanned, those + * refs would no longer resolve. Instead of putting the whole folder on the Java classpath (which would let a scanned + * repository supply arbitrary classes and resources to the JVM), we copy only the xml ruleset files that the custom + * rulesets actually reference (transitively) into a dedicated staging folder, and put just that folder on the classpath. + * + * Only regular ".xml" files that live underneath the base folder are ever staged. + * + * @param rulesetFiles the rulesets to inspect (absolute paths on disk, or paths relative to the base folder). Entries that are not xml files on disk are ignored. + * @param baseFolder the folder that relative refs are resolved against + * @param stagingFolder the folder to copy the referenced rulesets into + * @returns the staging folder if at least one file was staged, otherwise undefined + */ +export async function stageReferencedRulesets(rulesetFiles: string[], baseFolder: string, stagingFolder: string): Promise { + const realBase: string = await realpathOrUndefined(baseFolder) ?? baseFolder; + const visited: Set = new Set(); + const queue: string[] = []; + let stagedSomething: boolean = false; + + // Stages the file given by a (relative) ref and queues it up so that its own refs get processed too + const stageRef = async (ref: string): Promise => { + const relativeRulesetPath: string | undefined = extractRelativeXmlPath(ref); + if (relativeRulesetPath === undefined) { + return; + } + const source: string | undefined = await resolveStageableFile(relativeRulesetPath, realBase); + if (source === undefined) { + return; + } + const destination: string = path.join(stagingFolder, relativeRulesetPath); + await fs.promises.mkdir(path.dirname(destination), {recursive: true}); + await fs.promises.copyFile(source, destination); + stagedSomething = true; + queue.push(source); + }; + + for (const rulesetFile of rulesetFiles) { + if (path.isAbsolute(rulesetFile)) { + queue.push(rulesetFile); + } else { + await stageRef(rulesetFile); + } + } + + while (queue.length > 0) { + const currentFile: string = queue.shift()!; + if (visited.has(currentFile)) { + continue; + } + visited.add(currentFile); + + let contents: string; + try { + contents = await fs.promises.readFile(currentFile, 'utf-8'); + } catch (_err) { + continue; // Not a readable file on disk (for example, a ruleset inside of a jar file) + } + + for (const match of contents.matchAll(RULE_REF_REGEX)) { + await stageRef(match[1]); + } + } + return stagedSomething ? stagingFolder : undefined; +} + +/** + * Returns the relative path to the xml file portion of a rule ref (stripping off any trailing "/RuleName"), or + * undefined if the ref is not a relative path to an xml file (for example, "category/apex/design.xml" is fine to stage + * if it exists in the base folder, but absolute paths and non-xml refs are ignored). + */ +function extractRelativeXmlPath(ref: string): string | undefined { + const xmlMatch: RegExpMatchArray | null = ref.match(/^(.*?\.xml)(\/.*)?$/i); + if (!xmlMatch) { + return undefined; + } + const candidate: string = xmlMatch[1]; + if (path.isAbsolute(candidate) || path.win32.isAbsolute(candidate)) { + return undefined; + } + return path.normalize(candidate); +} + +async function resolveStageableFile(relativePath: string, realBase: string): Promise { + const real: string | undefined = await realpathOrUndefined(path.join(realBase, relativePath)); + if (real === undefined) { + return undefined; + } + const relativeToBase: string = path.relative(realBase, real); + if (relativeToBase === '' || relativeToBase.startsWith('..') || path.isAbsolute(relativeToBase)) { + return undefined; // Never stage anything that resolves (even through symlinks) outside of the base folder + } + try { + return (await fs.promises.stat(real)).isFile() ? real : undefined; + } catch (_err) /* istanbul ignore next */ { + return undefined; + } +} + +async function realpathOrUndefined(p: string): Promise { + try { + return await fs.promises.realpath(p); + } catch (_err) { + return undefined; + } +} diff --git a/packages/code-analyzer-pmd-engine/test/pmd-engine.test.ts b/packages/code-analyzer-pmd-engine/test/pmd-engine.test.ts index 198de645..bdcf9ed6 100644 --- a/packages/code-analyzer-pmd-engine/test/pmd-engine.test.ts +++ b/packages/code-analyzer-pmd-engine/test/pmd-engine.test.ts @@ -16,6 +16,7 @@ import { import {PmdEngine} from "../src/pmd-engine"; import fs from "node:fs"; import * as fsPromises from "node:fs/promises"; +import os from "node:os"; import path from "node:path"; import {Language, PMD_VERSION} from "../src/constants"; import {DEFAULT_PMD_ENGINE_CONFIG, PMD_AVAILABLE_LANGUAGES, PmdEngineConfig} from "../src/config"; @@ -321,6 +322,31 @@ describe('Tests for the describeRules method of PmdEngine', () => { } }); + it('When a custom ruleset references a rule using a path relative to the folder Code Analyzer is run from, then the rule is resolved', async () => { + const repoFolder: string = fs.mkdtempSync(path.join(os.tmpdir(), 'pmd-cwd-ref-test-')); + const originalCwd: string = process.cwd(); + try { + await fsPromises.mkdir(path.join(repoFolder, 'scanner')); + await fsPromises.copyFile(path.join(TEST_DATA_FOLDER, 'custom rules', 'somecat3.xml'), path.join(repoFolder, 'scanner', 'full.xml')); + const wrapperRuleset: string = path.join(repoFolder, 'wrapper.xml'); + await fsPromises.writeFile(wrapperRuleset, `\n` + + `\n` + + ` Wrapper\n` + + ` \n` + + `\n`, 'utf-8'); + process.chdir(repoFolder); + const engine: PmdEngine = new PmdEngine({ + ...DEFAULT_PMD_ENGINE_CONFIG, + custom_rulesets: [wrapperRuleset] + }); + const ruleDescriptions: RuleDescription[] = await engine.describeRules(createDescribeOptions()); + expectContainsRuleWithName(ruleDescriptions, 'fakerule7'); + } finally { + process.chdir(originalCwd); + fs.rmSync(repoFolder, {recursive: true, force: true}); + } + }); + it('When specifying a custom_ruleset that cannot be found, then error with a good error message', async () => { const engine: PmdEngine = new PmdEngine({ ...DEFAULT_PMD_ENGINE_CONFIG, diff --git a/packages/code-analyzer-pmd-engine/test/ruleset-staging.test.ts b/packages/code-analyzer-pmd-engine/test/ruleset-staging.test.ts new file mode 100644 index 00000000..21a3c54b --- /dev/null +++ b/packages/code-analyzer-pmd-engine/test/ruleset-staging.test.ts @@ -0,0 +1,92 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import {stageReferencedRulesets} from "../src/ruleset-staging"; + +describe('Tests for stageReferencedRulesets', () => { + let baseFolder: string; + let stagingFolder: string; + + beforeEach(() => { + const tempRoot: string = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'staging-test-'))); + baseFolder = path.join(tempRoot, 'repo'); + stagingFolder = path.join(tempRoot, 'staged'); + fs.mkdirSync(path.join(baseFolder, 'scanner'), {recursive: true}); + }); + + afterEach(() => { + fs.rmSync(path.dirname(baseFolder), {recursive: true, force: true}); + }); + + const rulesetWithRefs = (...refs: string[]): string => + `${refs.map(r => ``).join('')}`; + + it('When a ruleset references a sibling xml file, then only that xml file is staged', async () => { + fs.writeFileSync(path.join(baseFolder, 'scanner', 'full.xml'), rulesetWithRefs()); + fs.writeFileSync(path.join(baseFolder, 'scanner', 'unrelated.xml'), rulesetWithRefs()); + fs.writeFileSync(path.join(baseFolder, 'Evil.class'), 'not really a class'); + const wrapper: string = path.join(baseFolder, 'wrapper.xml'); + fs.writeFileSync(wrapper, rulesetWithRefs('scanner/full.xml/MyRule')); + + const result: string | undefined = await stageReferencedRulesets([wrapper], baseFolder, stagingFolder); + + expect(result).toEqual(stagingFolder); + expect(fs.readdirSync(stagingFolder, {recursive: true}).sort()).toEqual(['scanner', path.join('scanner', 'full.xml')]); + }); + + it('When referenced rulesets reference other rulesets, then they are staged transitively (and cycles are tolerated)', async () => { + fs.writeFileSync(path.join(baseFolder, 'a.xml'), rulesetWithRefs('b.xml/R')); + fs.writeFileSync(path.join(baseFolder, 'b.xml'), rulesetWithRefs('a.xml/R', 'scanner/c.xml')); + fs.writeFileSync(path.join(baseFolder, 'scanner', 'c.xml'), rulesetWithRefs()); + const wrapper: string = path.join(baseFolder, 'wrapper.xml'); + fs.writeFileSync(wrapper, rulesetWithRefs('a.xml/R')); + + await stageReferencedRulesets([wrapper], baseFolder, stagingFolder); + + expect(fs.existsSync(path.join(stagingFolder, 'a.xml'))).toBe(true); + expect(fs.existsSync(path.join(stagingFolder, 'b.xml'))).toBe(true); + expect(fs.existsSync(path.join(stagingFolder, 'scanner', 'c.xml'))).toBe(true); + }); + + it('When a relative ruleset is given directly, then it is resolved against the base folder and staged', async () => { + fs.writeFileSync(path.join(baseFolder, 'scanner', 'full.xml'), rulesetWithRefs()); + + const result: string | undefined = await stageReferencedRulesets(['scanner/full.xml'], baseFolder, stagingFolder); + + expect(result).toEqual(stagingFolder); + expect(fs.existsSync(path.join(stagingFolder, 'scanner', 'full.xml'))).toBe(true); + }); + + it('When refs try to escape the base folder, are absolute, or are not xml, then nothing is staged', async () => { + const outsideFile: string = path.join(path.dirname(baseFolder), 'outside.xml'); + fs.writeFileSync(outsideFile, rulesetWithRefs()); + fs.writeFileSync(path.join(baseFolder, 'Evil.class'), 'x'); + const wrapper: string = path.join(baseFolder, 'wrapper.xml'); + fs.writeFileSync(wrapper, rulesetWithRefs('../outside.xml/R', outsideFile + '/R', 'Evil.class', 'does/not/exist.xml/R')); + + const result: string | undefined = await stageReferencedRulesets([wrapper], baseFolder, stagingFolder); + + expect(result).toBeUndefined(); + expect(fs.existsSync(stagingFolder)).toBe(false); + }); + + it('When a ref is a symlink pointing outside of the base folder, then it is not staged', async () => { + const outsideFile: string = path.join(path.dirname(baseFolder), 'secret.xml'); + fs.writeFileSync(outsideFile, rulesetWithRefs()); + try { + fs.symlinkSync(outsideFile, path.join(baseFolder, 'link.xml')); + } catch (_err) { + return; // Symlink creation isn't permitted in this environment (e.g. Windows without privileges) + } + const wrapper: string = path.join(baseFolder, 'wrapper.xml'); + fs.writeFileSync(wrapper, rulesetWithRefs('link.xml/R')); + + expect(await stageReferencedRulesets([wrapper], baseFolder, stagingFolder)).toBeUndefined(); + }); + + it('When rulesets are not readable files on disk (like jar based rulesets), then they are ignored', async () => { + const result: string | undefined = await stageReferencedRulesets( + [path.join(baseFolder, 'missing.xml'), 'category/apex/design.xml'], baseFolder, stagingFolder); + expect(result).toBeUndefined(); + }); +});