Strip onload attribute from SVG input

In order to prevent a remote code injection, the `onload` attribute needs to be removed from the SVG input as Puppeteer/Chromium will execute any code within it which could potentially be malicious. Fixes #81
neocotic · May 29, 2022 · 7e6031a · 7e6031a
1 parent 5e39f45
commit 7e6031a
Show file tree

Hide file tree

Showing 11 changed files with 21 additions and 15 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/packages/convert-svg-core/package.json b/packages/convert-svg-core/package.json
@@ -36,7 +36,7 @@
   },
   "dependencies": {
     "chalk": "^4.1.2",
-    "cheerio": "^1.0.0-rc.10",
+    "cheerio": "^1.0.0-rc.11",
     "commander": "^9.2.0",
     "file-url": "^3.0.0",
     "get-stdin": "^8.0.0",

diff --git a/packages/convert-svg-core/src/Converter.js b/packages/convert-svg-core/src/Converter.js
@@ -47,6 +47,7 @@ const _parseOptions = Symbol('parseOptions');
 const _provider = Symbol('provider');
 const _roundDimension = Symbol('roundDimension');
 const _roundDimensions = Symbol('roundDimensions');
+const _sanitize = Symbol('sanitize');
 const _setDimensions = Symbol('setDimensions');
 const _tempFile = Symbol('tempFile');
 const _validate = Symbol('validate');
@@ -114,9 +115,7 @@ class Converter {
 
     options = this[_parseOptions](options);
 
-    const output = await this[_convert](input, options);
-
-    return output;
+    return await this[_convert](input, options);
   }
 
   /**
@@ -191,11 +190,13 @@ class Converter {
     input = Buffer.isBuffer(input) ? input.toString('utf8') : input;
 
     const { provider } = this;
-    const svg = cheerio.default.html(cheerio.load(input, null, false)('svg'));
+    const svg = cheerio.default.html(this[_sanitize](cheerio.load(input, null, false)('svg')));
+
+    if (!svg) {
+      throw new Error('SVG element not found in input. Check the SVG input');
+    }
 
-    let html = '';
-    if (svg) {
-      html += `<!DOCTYPE html>
+    const html = `<!DOCTYPE html>
 <html>
 <head>
 <base href="${options.baseUrl}">
@@ -207,9 +208,6 @@ html { background-color: ${provider.getBackgroundColor(options)}; }
 </head>
 <body>${svg}</body>
 </html>`;
-    } else {
-      throw new Error('SVG element not found in input. Check the SVG input');
-    }
 
     const page = await this[_getPage](html);
 
@@ -226,12 +224,10 @@ html { background-color: ${provider.getBackgroundColor(options)}; }
 
     await page.setViewport(dimensions);
 
-    const output = await page.screenshot(Object.assign({
+    return await page.screenshot(Object.assign({
       type: provider.getType(),
       clip: Object.assign({ x: 0, y: 0 }, dimensions)
     }, provider.getScreenshotOptions(options)));
-
-    return output;
   }
 
   async [_getDimensions](page, options) {
@@ -389,6 +385,10 @@ html { background-color: ${provider.getBackgroundColor(options)}; }
     };
   }
 
+  [_sanitize](svg) {
+    return svg.removeAttr('onload');
+  }
+
   async [_setDimensions](page, dimensions) {
     if (typeof dimensions.width !== 'number' && typeof dimensions.height !== 'number') {
       return;

diff --git a/packages/convert-svg-test-helper/src/fixtures/input/issue-81.svg b/packages/convert-svg-test-helper/src/fixtures/input/issue-81.svg
diff --git a/packages/convert-svg-test-helper/src/tests.json b/packages/convert-svg-test-helper/src/tests.json
@@ -163,6 +163,11 @@
     "file": "cve-2021-23631.svg",
     "message": "should only read SVG element"
   },
+  {
+    "name": "when SVG has onload attribute",
+    "file": "issue-81.svg",
+    "message": "should strip onload attribute"
+  },
   {
     "name": "when setting both baseFile and baseUrl options",
     "file": "external-file.svg",

diff --git a/packages/convert-svg-to-jpeg/test/fixtures/expected/28.jpeg b/packages/convert-svg-to-jpeg/test/fixtures/expected/28.jpeg
diff --git a/packages/convert-svg-to-jpeg/test/fixtures/expected/35.jpeg b/packages/convert-svg-to-jpeg/test/fixtures/expected/35.jpeg
diff --git a/packages/convert-svg-to-jpeg/test/fixtures/expected/36.jpeg b/packages/convert-svg-to-jpeg/test/fixtures/expected/36.jpeg
diff --git a/packages/convert-svg-to-jpeg/test/fixtures/expected/37.jpeg b/packages/convert-svg-to-jpeg/test/fixtures/expected/37.jpeg
diff --git a/packages/convert-svg-to-jpeg/test/fixtures/expected/38.jpeg b/packages/convert-svg-to-jpeg/test/fixtures/expected/38.jpeg
diff --git a/packages/convert-svg-to-png/test/fixtures/expected/28.png b/packages/convert-svg-to-png/test/fixtures/expected/28.png