diff --git a/.changeset/wise-eyes-warn.md b/.changeset/wise-eyes-warn.md new file mode 100644 index 0000000000..ef3b3439cc --- /dev/null +++ b/.changeset/wise-eyes-warn.md @@ -0,0 +1,21 @@ +--- +"@comet/cms-api": patch +--- + +Improve SVG validation + +Following tags are banned in SVGs: + +- script +- \[new\] foreignObject +- \[new\] use +- \[new\] image +- \[new\] animate +- \[new\] animateMotion +- \[new\] animateTransform +- \[new\] set + +Following attributes are banned: + +- Event handlers (`onload`, `onclick`, ...) +- \[new\] `href` and `xlink:href` (if the value starts with `http://`, `https://` or `javascript:`) diff --git a/packages/admin/cms-admin/src/dam/DataGrid/fileUpload/fileUploadErrorMessages.tsx b/packages/admin/cms-admin/src/dam/DataGrid/fileUpload/fileUploadErrorMessages.tsx index 793f514a3a..39569f9506 100644 --- a/packages/admin/cms-admin/src/dam/DataGrid/fileUpload/fileUploadErrorMessages.tsx +++ b/packages/admin/cms-admin/src/dam/DataGrid/fileUpload/fileUploadErrorMessages.tsx @@ -77,8 +77,8 @@ export const FileExtensionTypeMismatchError = ({ extension, mimetype }: FileExte export const SvgContainsJavaScriptError = () => ( ); } - } else if (typedErr.response?.data.message === "Rejected File Upload: SVG must not contain JavaScript") { + } else if (typedErr.response?.data.message.includes("SVG contains forbidden content")) { addValidationError(file, ); } else if (typedErr.response === undefined && typedErr.request) { addValidationError(file, ); diff --git a/packages/api/cms-api/src/dam/files/file-validation.service.ts b/packages/api/cms-api/src/dam/files/file-validation.service.ts index 70e7ce18b7..d5ef57d016 100644 --- a/packages/api/cms-api/src/dam/files/file-validation.service.ts +++ b/packages/api/cms-api/src/dam/files/file-validation.service.ts @@ -1,7 +1,7 @@ import { readFile } from "fs/promises"; import { FileUploadInput } from "./dto/file-upload.input"; -import { getValidExtensionsForMimetype, svgContainsJavaScript } from "./files.utils"; +import { getValidExtensionsForMimetype, isValidSvg } from "./files.utils"; export class FileValidationService { constructor(public config: { maxFileSize: number; acceptedMimeTypes: string[] }) {} @@ -45,8 +45,8 @@ export class FileValidationService { if (file.mimetype === "image/svg+xml") { const fileContent = await readFile(file.path, { encoding: "utf-8" }); - if (svgContainsJavaScript(fileContent)) { - return "SVG must not contain JavaScript"; + if (!isValidSvg(fileContent)) { + return "SVG contains forbidden content (e.g., JavaScript, security-relevant tags or attributes)"; } } diff --git a/packages/api/cms-api/src/dam/files/files.utils.spec.ts b/packages/api/cms-api/src/dam/files/files.utils.spec.ts index 8a4034e312..cca1e03219 100644 --- a/packages/api/cms-api/src/dam/files/files.utils.spec.ts +++ b/packages/api/cms-api/src/dam/files/files.utils.spec.ts @@ -1,16 +1,16 @@ -import { svgContainsJavaScript } from "./files.utils"; +import { isValidSvg } from "./files.utils"; describe("Files Utils", () => { - describe("svgContainsJavaScript", () => { - it("should return false if the svg doesn't contain a script tag or event handler", async () => { + describe("isValidSvg", () => { + it("should return true if the svg doesn't contain any forbidden content", async () => { const cleanSvg = ` `; - expect(svgContainsJavaScript(cleanSvg)).toBe(false); + expect(isValidSvg(cleanSvg)).toBe(true); }); - it("should return true if the svg contains a script tag", async () => { + it("should return false if the svg contains a script tag", async () => { const svgWithScriptTag = ` `; - expect(svgContainsJavaScript(svgWithScriptTag)).toBe(true); + expect(isValidSvg(svgWithScriptTag)).toBe(false); }); - it("should return true if the svg contains an event handler", async () => { + it("should return false if the svg contains an event handler", async () => { const svgWithOnloadHandler = ` `; - expect(svgContainsJavaScript(svgWithOnloadHandler)).toBe(true); + expect(isValidSvg(svgWithOnloadHandler)).toBe(false); + }); + + it("should return false if the svg contains a href attribute containing javascript", async () => { + const svgWithOnloadHandler = ` + + + + `; + + expect(isValidSvg(svgWithOnloadHandler)).toBe(false); }); }); }); diff --git a/packages/api/cms-api/src/dam/files/files.utils.ts b/packages/api/cms-api/src/dam/files/files.utils.ts index 05574bcaff..ff237835bf 100644 --- a/packages/api/cms-api/src/dam/files/files.utils.ts +++ b/packages/api/cms-api/src/dam/files/files.utils.ts @@ -49,34 +49,54 @@ type SvgNode = [key: string]: SvgNode; }; -const recursivelyFindJSInSvg = (node: SvgNode): boolean => { +const disallowedSvgTags = [ + "script", // can lead to XSS + "foreignObject", // can embed non-SVG content + "use", // can load external resources + "image", // can load external resources + "animate", // can modify attributes; resource exhaustion + "animateMotion", // can modify attributes; resource exhaustion + "animateTransform", // can modify attributes; resource exhaustion + "set", // can modify attributes +]; + +const recursiveIsValidSvgNode = (node: SvgNode): boolean => { if (typeof node === "string") { // is plain text -> can't contain JS - return false; + return true; } - for (const tagOrAttr of Object.keys(node)) { - if (tagOrAttr.toLowerCase() === "script" || tagOrAttr.toLowerCase().startsWith("on")) { - // is script tag or event handler - return true; + for (const [tagOrAttributeName, value] of Object.entries(node)) { + const containsDisallowedTags = disallowedSvgTags.map((tag) => tag.toLowerCase()).includes(tagOrAttributeName.toLowerCase()); + + const containsEventHandler = tagOrAttributeName.toLowerCase().startsWith("on"); // can execute JavaScript + + const containsHref = // can execute JavaScript or link to malicious targets + ["href", "xlink:href"].includes(tagOrAttributeName) && + typeof value === "string" && + (value.startsWith("http://") || value.startsWith("https://") || value.startsWith("javascript:")); + + if (containsDisallowedTags || containsEventHandler || containsHref) { + return false; } // is node -> children can contain JS - const children = node[tagOrAttr]; - const childrenContainJS = recursivelyFindJSInSvg(children); - if (childrenContainJS) { - return true; + const children = node[tagOrAttributeName]; + const childrenAreValid = recursiveIsValidSvgNode(children); + + if (!childrenAreValid) { + return false; } } - return false; + return true; }; -export const svgContainsJavaScript = (svg: string) => { +export const isValidSvg = (svg: string) => { const parser = new XMLParser({ ignoreAttributes: false, attributeNamePrefix: "" }); const jsonObj = parser.parse(svg) as SvgNode; - return recursivelyFindJSInSvg(jsonObj); + return recursiveIsValidSvgNode(jsonObj); }; export const removeMulterTempFile = async (file: FileUploadInput) => {