Skip to content

Fix: Code injection vulnerability (CodeQL #5) #30

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.

Sign up for GitHub

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

Open
jmaddington wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix: Code injection vulnerability (CodeQL #5) #30

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 140 additions & 18 deletions lib/charts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,112 @@
const { logger } = require('../logging');
const { uniqueSvg } = require('./svg');

/**
* Validates a string to ensure it only contains a safe chart configuration
* @param {string} input The chart configuration string to validate
* @returns {boolean} True if the input is a safe chart configuration
*/
function isValidChartConfig(input) {
// List of dangerous patterns that could indicate code injection
const dangerousPatterns = [
/\beval\s*\(/i, // eval()
/\bFunction\s*\(/i, // Function constructor
/\bsetTimeout\s*\(/i, // setTimeout
/\bsetInterval\s*\(/i, // setInterval
/\brequire\s*\(/i, // require
/\bimport\s*\(/i, // import()
/\bprocess\b/i, // process object
/\bglobal\b/i, // global object
/\b__dirname\b/i, // __dirname
/\b__filename\b/i, // __filename
/\bconstructor\b.*\bprototype\b/i, // prototype pollution
/\bObject\s*\.\s*([gs]et)?[pP]rototype[oO]f\b/i, // prototype manipulation
/\bdocument\b/i, // DOM access
/\bwindow\b/i, // window object
/\blocation\b/i, // location object
/\bnavigator\b/i, // navigator object
/\bfetch\b/i, // fetch API
/\bXMLHttpRequest\b/i, // XHR
/\bWebSocket\b/i, // WebSocket
/\balert\b/i, // alert
/\bconfirm\b/i, // confirm
/\bprompt\b/i, // prompt
/\blocalStorage\b/i, // localStorage
/\bsessionStorage\b/i, // sessionStorage
/\bindexedDB\b/i, // indexedDB
/\bfs\b/i, // filesystem
/\bhttps?\b/i, // http/https modules
/\bnet\b/i, // net module
/\bchild_process\b/i, // child_process module
/\bcrypto\b/i, // crypto module
/\bzlib\b/i, // zlib module
/\bdgram\b/i, // dgram module
/<(\w+)>/i, // HTML tags
/\(\s*\)\s*=>/i, // arrow functions
/\bfunction\s*\(/i, // function declarations
/\bnew\s+/i, // new keyword
/\bdelete\b/i, // delete operator
];

// Check if the input contains any dangerous patterns
for (const pattern of dangerousPatterns) {
if (pattern.test(input)) {
return false;
}
}

// Basic structure validation - ensure the input starts with a chart-like object
const validStartPatterns = [
/^\s*{/, // Object literal
/^\s*\(\s*{/, // Parenthesized object literal
/^\s*\{\s*type\s*:/i, // Object with type property
];

let isValidStart = false;
for (const pattern of validStartPatterns) {
if (pattern.test(input)) {
isValidStart = true;
break;
}
}

if (!isValidStart) {
return false;
}

// Check for balanced curly braces as a basic syntax check
let braceCount = 0;
let inString = false;
let stringChar = '';
let escaped = false;

for (let i = 0; i < input.length; i++) {
const char = input[i];

if (inString) {
if (escaped) {
escaped = false;
} else if (char === '\\') {
escaped = true;
} else if (char === stringChar) {
inString = false;
}
} else if (char === '"' || char === "'") {
inString = true;
stringChar = char;
} else if (char === '{') {
braceCount++;
} else if (char === '}') {
braceCount--;
if (braceCount < 0) {
return false; // Unbalanced braces
}
}
}

return braceCount === 0 && !inString;
}

// Polyfills
require('canvas-5-polyfill');
global.CanvasGradient = canvas.CanvasGradient;
Expand Down Expand Up @@ -130,25 +236,41 @@
) {
let chart;
if (typeof untrustedChart === 'string') {
// The chart could contain Javascript - run it in a VM.
// First, try to parse as JSON
try {
const { getGradientFill, getGradientFillHelper } = getGradientFunctions(width, height);
const chartFunction = new Function(
'getGradientFill',
'getGradientFillHelper',
'pattern',
'Chart',
`return ${untrustedChart}`,
);
chart = chartFunction(
getGradientFill,
getGradientFillHelper,
{ draw: patternDraw },
getChartJsForVersion(version),
);
} catch (err) {
logger.error('Input Error', err, untrustedChart);
return Promise.reject(new Error(`Invalid input\n${err}`));
chart = JSON.parse(untrustedChart);
} catch (jsonErr) {
// If it's not valid JSON, it might be a JavaScript object literal
// Try to validate and sanitize it before evaluation
if (isValidChartConfig(untrustedChart)) {
try {
const { getGradientFill, getGradientFillHelper } = getGradientFunctions(width, height);
// Use indirect eval in a controlled context
const chartFunction = new Function(
'getGradientFill',
'getGradientFillHelper',
'pattern',
'Chart',
`"use strict"; return ${untrustedChart}`,

Check failure

Code scanning / CodeQL

Code injection Critical

This code execution depends on a
user-provided value
Loading
.
This code execution depends on a
user-provided value
Loading
.
This code execution depends on a
user-provided value
Loading
.
This code execution depends on a
user-provided value
Loading
.
);
chart = chartFunction(
getGradientFill,
getGradientFillHelper,
{ draw: patternDraw },
getChartJsForVersion(version),
);
} catch (evalErr) {
logger.error('Input Error', evalErr, untrustedChart);
return Promise.reject(new Error(`Invalid input\n${evalErr}`));
}
} else {
logger.error('Invalid chart configuration format', jsonErr, untrustedChart);
return Promise.reject(
new Error(
'Invalid chart configuration. Must be valid JSON or a safe chart object literal.',
),
);
}
}
} else {
// The chart is just a simple JSON object.
Expand Down
Loading