JS: Add experimental variants of common security queries with more sources #11582
Conversation
938983d to
a727142
Compare
|
QHelp previews: javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-078/CommandInjection.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-079/Xss.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-089/SqlInjection.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-094/CodeInjection.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-117/LogInjection.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-134/TaintedFormatString.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-346/CorsMisconfigurationForCredentials.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-400/RemotePropertyInjection.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-502/UnsafeDeserialization.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-611/Xxe.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-643/XpathInjection.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-730/RegExpInjection.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-770/ResourceExhaustion.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-776/XmlBomb.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-807/ConditionalBypass.qhelperrors/warnings:javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-915/PrototypePollutingAssignment.qhelperrors/warnings: |
|
QHelp previews: javascript/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.qhelpUntrusted data passed to external APIUsing unsanitized untrusted data in an external API can cause a variety of security issues. This query reports external APIs that use untrusted data. The results are not filtered so that you can audit all examples. The query provides data for security reviews of the application and you can also use it to identify external APIs that should be modeled as either taint steps, or sinks for specific problems. An external API is defined as a method call to a method that is not defined in the source code, not overridden in the source code, and is not modeled as a taint step in the default taint library. External APIs may be from the third-party dependencies or from internal dependencies. The query reports uses of untrusted data one of the arguments of external API call or in the return value from a callback passed to an external API. RecommendationFor each result:
ExampleIn this first example, a query parameter is read from the express().get('/news', (req, res) => {
let topic = req.query.topic;
res.send(`<h1>${topic}</h1>`);
});This is a reflected XSS sink. The XSS query should therefore be reviewed to confirm that this sink is appropriately modeled, and if it is, to confirm that the query reports this particular result, or that the result is a false positive due to some existing sanitization. In this second example, again a query parameter is read from let path = require('path');
express().get('/data', (req, res) => {
let file = path.join(HOME_DIR, 'public', req.query.file);
res.sendFile(file);
});If the query reported the call to Note that both examples are correctly handled by the standard taint tracking library and security queries. References
javascript/ql/src/Security/CWE-078/CommandInjection.qhelpUncontrolled command lineCode that passes user input directly to RecommendationIf possible, use hard-coded string literals to specify the command to run or library to load. Instead of passing the user input directly to the process or library function, examine the user input and then choose among hard-coded string literals. If the applicable libraries or commands cannot be determined at compile time, then add code to verify that the user input string is safe before using it. ExampleThe following example shows code that takes a shell script that can be changed maliciously by a user, and passes it straight to var cp = require("child_process"),
http = require('http'),
url = require('url');
var server = http.createServer(function(req, res) {
let cmd = url.parse(req.url, true).query.path;
cp.exec(cmd); // BAD
});References
javascript/ql/src/Security/CWE-079/Xss.qhelpClient-side cross-site scriptingDirectly writing user input (for example, a URL query parameter) to a webpage without properly sanitizing the input first, allows for a cross-site scripting vulnerability. This kind of vulnerability is also called DOM-based cross-site scripting, to distinguish it from other types of cross-site scripting. RecommendationTo guard against cross-site scripting, consider using contextual output encoding/escaping before writing user input to the page, or one of the other solutions that are mentioned in the references. ExampleThe following example shows part of the page URL being written directly to the document, leaving the website vulnerable to cross-site scripting. function setLanguageOptions() {
var href = document.location.href,
deflt = href.substring(href.indexOf("default=")+8);
document.write("<OPTION value=1>"+deflt+"</OPTION>");
document.write("<OPTION value=2>English</OPTION>");
}References
javascript/ql/src/Security/CWE-089/SqlInjection.qhelpDatabase query built from user-controlled sourcesIf a database query (such as a SQL or NoSQL query) is built from user-provided data without sufficient sanitization, a malicious user may be able to run malicious database queries. RecommendationMost database connector libraries offer a way of safely embedding untrusted data into a query by means of query parameters or prepared statements. For NoSQL queries, make use of an operator like MongoDB's ExampleIn the following example, assume the function The handler constructs two copies of the same SQL query involving user input taken from the request object, once unsafely using string concatenation, and once safely using query parameters. In the first case, the query string In the second case, the parameter is embedded into the query string const app = require("express")(),
pg = require("pg"),
pool = new pg.Pool(config);
app.get("search", function handler(req, res) {
// BAD: the category might have SQL special characters in it
var query1 =
"SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" +
req.params.category +
"' ORDER BY PRICE";
pool.query(query1, [], function(err, results) {
// process results
});
// GOOD: use parameters
var query2 =
"SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY=$1" + " ORDER BY PRICE";
pool.query(query2, [req.params.category], function(err, results) {
// process results
});
});References
javascript/ql/src/Security/CWE-094/CodeInjection.qhelpCode injectionDirectly evaluating user input (for example, an HTTP request parameter) as code without properly sanitizing the input first allows an attacker arbitrary code execution. This can occur when user input is treated as JavaScript, or passed to a framework which interprets it as an expression to be evaluated. Examples include AngularJS expressions or JQuery selectors. RecommendationAvoid including user input in any expression which may be dynamically evaluated. If user input must be included, use context-specific escaping before including it. It is important that the correct escaping is used for the type of evaluation that will occur. ExampleThe following example shows part of the page URL being evaluated as JavaScript code. This allows an attacker to provide JavaScript within the URL. If an attacker can persuade a user to click on a link to such a URL, the attacker can evaluate arbitrary JavaScript in the browser of the user to, for example, steal cookies containing session information. eval(document.location.href.substring(document.location.href.indexOf("default=")+8))The following example shows a Pug template being constructed from user input, allowing attackers to run arbitrary code via a payload such as const express = require('express')
var pug = require('pug');
const app = express()
app.post('/', (req, res) => {
var input = req.query.username;
var template = `
doctype
html
head
title= 'Hello world'
body
form(action='/' method='post')
input#name.form-control(type='text)
button.btn.btn-primary(type='submit') Submit
p Hello `+ input
var fn = pug.compile(template);
var html = fn();
res.send(html);
})Below is an example of how to use a template engine without any risk of template injection. The user input is included via an interpolation expression const express = require('express')
var pug = require('pug');
const app = express()
app.post('/', (req, res) => {
var input = req.query.username;
var template = `
doctype
html
head
title= 'Hello world'
body
form(action='/' method='post')
input#name.form-control(type='text)
button.btn.btn-primary(type='submit') Submit
p Hello #{username}`
var fn = pug.compile(template);
var html = fn({username: input});
res.send(html);
})References
javascript/ql/src/Security/CWE-117/LogInjection.qhelpLog injectionIf unsanitized user input is written to a log entry, a malicious user may be able to forge new log entries. Forgery can occur if a user provides some input with characters that are interpreted when the log output is displayed. If the log is displayed as a plain text file, then new line characters can be used by a malicious user. If the log is displayed as HTML, then arbitrary HTML may be included to spoof log entries. RecommendationUser input should be suitably sanitized before it is logged. If the log entries are in plain text then line breaks should be removed from user input, using For log entries that will be displayed in HTML, user input should be HTML-encoded before being logged, to prevent forgery and other forms of HTML injection. ExampleIn the first example, a username, provided by the user, is logged using `console.info`. In the first case, it is logged without any sanitization. In the second case, the username is used to build an error that is logged using `console.error`. If a malicious user provides `username=Guest%0a[INFO]+User:+Admin%0a` as a username parameter, the log entry will be splitted in two different lines, where the second line will be `[INFO]+User:+Admin`. const http = require('http');
const url = require('url');
const server = http.createServer((req, res) => {
let q = url.parse(req.url, true);
console.info(`[INFO] User: ${q.query.username}`); // BAD: User input logged as-is
})
server.listen(3000, '127.0.0.1', () => {});In the second example, const http = require('http');
const url = require('url');
const server = http.createServer((req, res) => {
let q = url.parse(req.url, true);
// GOOD: remove newlines from user controlled input before logging
let username = q.query.username.replace(/\n|\r/g, "");
console.info(`[INFO] User: ${username}`);
});
server.listen(3000, '127.0.0.1', () => {});References
javascript/ql/src/Security/CWE-134/TaintedFormatString.qhelpUse of externally-controlled format stringFunctions like the Node.js standard library function RecommendationEither sanitize the input before including it in the format string, or use a ExampleThe following program snippet logs information about an unauthorized access attempt. The log message includes the user name, and the user's IP address is passed as an additional argument to const app = require("express")();
app.get("unauthorized", function handler(req, res) {
let user = req.query.user;
let ip = req.connection.remoteAddress;
console.log("Unauthorized access attempt by " + user, ip);
});However, if a malicious user provides Instead, the user name should be included using the const app = require("express")();
app.get("unauthorized", function handler(req, res) {
let user = req.query.user;
let ip = req.connection.remoteAddress;
console.log("Unauthorized access attempt by %s", user, ip);
});References
javascript/ql/src/Security/CWE-346/CorsMisconfigurationForCredentials.qhelpCORS misconfiguration for credentials transferA server can send the When the RecommendationWhen the Since the ExampleIn the example below, the server allows the browser to send user credentials in a Cross-Origin request. The request header var https = require('https'),
url = require('url');
var server = https.createServer(function(){});
server.on('request', function(req, <
8000
span class="pl-s1">res) {
let origin = url.parse(req.url, true).query.origin;
// BAD: attacker can choose the value of origin
res.setHeader("Access-Control-Allow-Origin", origin);
res.setHeader("Access-Control-Allow-Credentials", true);
// ...
});This is not secure, since an attacker can choose the value of the var https = require('https'),
url = require('url');
var server = https.createServer(function(){});
server.on('request', function(req, res) {
let origin = url.parse(req.url, true).query.origin,
whitelist = {
"https://example.com": true,
"https://subdomain.example.com": true,
"https://example.com:1337": true
};
if (origin in whitelist) {
// GOOD: the origin is in the whitelist
res.setHeader("Access-Control-Allow-Origin", origin);
res.setHeader("Access-Control-Allow-Credentials", true);
}
// ...
});References
javascript/ql/src/Security/CWE-400/RemotePropertyInjection.qhelpRemote property injectionDynamically computing object property names from untrusted input may have multiple undesired consequences. For example, if the property access is used as part of a write, an attacker may overwrite vital properties of objects, such as Moreover, if the name of an HTTP header is user-controlled, an attacker may exploit this to overwrite security-critical headers such as RecommendationThe most common case in which prototype pollution vulnerabilities arise is when JavaScript objects are used for implementing map data structures. This case should be avoided whenever possible by using the ECMAScript 2015 When using user input as part of a header name, a sanitization step should be performed on the input to ensure that the name does not clash with existing header names such as ExampleIn the example below, the dynamically computed property var express = require('express');
var app = express();
var myObj = {}
app.get('/user/:id', function(req, res) {
var prop = req.query.userControlled; // BAD
myObj[prop] = function() {};
console.log("Request object " + myObj);
});This is not secure since an attacker may exploit this code to overwrite the property var express = require('express');
var app = express();
var myObj = {}
app.get('/user/:id', function(req, res) {
var prop = "$" + req.query.userControlled; // GOOD
myObj[prop] = function() {};
console.log("Request object " + myObj);
});References
javascript/ql/src/Security/CWE-502/UnsafeDeserialization.qhelpDeserialization of user-controlled dataDeserializing untrusted data using any deserialization framework that allows the construction of arbitrary functions is easily exploitable and, in many cases, allows an attacker to execute arbitrary code. RecommendationAvoid deserialization of untrusted data if at all possible. If the architecture permits it, then use formats like JSON or XML that cannot represent functions. When using YAML or other formats that support the serialization and deserialization of functions, ensure that the parser is configured to disable deserialization of arbitrary functions. ExampleThe following example calls the const app = require("express")(),
jsyaml = require("js-yaml");
app.get("load", function(req, res) {
let data = jsyaml.load(req.params.data);
// ...
});Using the const app = require("express")(),
jsyaml = require("js-yaml");
app.get("load", function(req, res) {
let data = jsyaml.safeLoad(req.params.data);
// ...
});References
javascript/ql/src/Security/CWE-611/Xxe.qhelpXML external entity expansionParsing untrusted XML files with a weakly configured XML parser may lead to an XML External Entity (XXE) attack. This type of attack uses external entity references to access arbitrary files on a system, carry out denial-of-service (DoS) attacks, or server-side request forgery. Even when the result of parsing is not returned to the user, DoS attacks are still possible and out-of-band data retrieval techniques may allow attackers to steal sensitive data. RecommendationThe easiest way to prevent XXE attacks is to disable external entity handling when parsing untrusted data. How this is done depends on the library being used. Note that some libraries, such as recent versions of ExampleThe following example uses the const app = require("express")(),
libxml = require("libxmljs");
app.post("upload", (req, res) => {
let xmlSrc = req.body,
doc = libxml.parseXml(xmlSrc, { noent: true });
});To guard against XXE attacks, the const app = require("express")(),
libxml = require("libxmljs");
app.post("upload", (req, res) => {
let xmlSrc = req.body,
doc = libxml.parseXml(xmlSrc);
});References
javascript/ql/src/Security/CWE-643/XpathInjection.qhelpXPath injectionIf an XPath expression is built using string concatenation, and the components of the concatenation include user input, it makes it very easy for a user to create a malicious XPath expression. RecommendationIf user input must be included in an XPath expression, either sanitize the data or use variable references to safely embed it without altering the structure of the expression. ExampleIn this example, the code accepts a user name specified by the user, and uses this unvalidated and unsanitized value in an XPath expression constructed using the const express = require('express');
const xpath = require('xpath');
const app = express();
app.get('/some/route', function(req, res) {
let userName = req.param("userName");
// BAD: Use user-provided data directly in an XPath expression
let badXPathExpr = xpath.parse("//users/user[login/text()='" + userName + "']/home_dir/text()");
badXPathExpr.select({
node: root
});
});Instead, embed the user input using the variable replacement mechanism offered by const express = require('express');
const xpath = require('xpath');
const app = express();
app.get('/some/route', function(req, res) {
let userName = req.param("userName");
// GOOD: Embed user-provided data using variables
let goodXPathExpr = xpath.parse("//users/user[login/text()=$userName]/home_dir/text()");
goodXPathExpr.select({
node: root,
variables: { userName: userName }
});
});References
javascript/ql/src/Security/CWE-730/RegExpInjection.qhelpRegular expression injectionConstructing a regular expression with unsanitized user input is dangerous as a malicious user may be able to modify the meaning of the expression. In particular, such a user may be able to provide a regular expression fragment that takes exponential time in the worst case, and use that to perform a Denial of Service attack. RecommendationBefore embedding user input into a regular expression, use a sanitization function such as lodash's ExampleThe following example shows a HTTP request parameter that is used to construct a regular expression without sanitizing it first: var express = require('express');
var app = express();
app.get('/findKey', function(req, res) {
var key = req.param("key"), input = req.param("input");
// BAD: Unsanitized user input is used to construct a regular expression
var re = new RegExp("\\b" + key + "=(.*)\n");
});Instead, the request parameter should be sanitized first, for example using the function var express = require('express');
var _ = require('lodash');
var app = express();
app.get('/findKey', function(req, res) {
var key = req.param("key"), input = req.param("input");
// GOOD: User input is sanitized before constructing the regex
var safeKey = _.escapeRegExp(key);
var re = new RegExp("\\b" + safeKey + "=(.*)\n");
});References
javascript/ql/src/Security/CWE-770/ResourceExhaustion.qhelpResource exhaustionApplications are constrained by how many resources they can make use of. Failing to respect these constraints may cause the application to be unresponsive or crash. It is therefore problematic if attackers can control the sizes or lifetimes of allocated objects. RecommendationEnsure that attackers can not control object sizes and their lifetimes. If object sizes and lifetimes must be controlled by external parties, ensure you restrict the object sizes and lifetimes so that they are within acceptable ranges. ExampleThe following example allocates a buffer with a user-controlled size. var http = require("http"),
url = require("url");
var server = http.createServer(function(req, res) {
var size = parseInt(url.parse(req.url, true).query.size);
let buffer = Buffer.alloc(size); // BAD
// ... use the buffer
});This is problematic since an attacker can choose a size that makes the application run out of memory. Even worse, in older versions of Node.js, this could leak confidential memory. To prevent such attacks, limit the buffer size: var http = require("http"),
url = require("url");
var server = http.createServer(function(req, res) {
var size = parseInt(url.parse(req.url, true).query.size);
if (size > 1024) {
res.statusCode = 400;
res.end("Bad request.");
return;
}
let buffer = Buffer.alloc(size); // GOOD
// ... use the buffer
});ExampleAs another example, consider an application that allocates an array with a user-controlled size, and then fills it with values: var http = require("http"),
url = require("url");
var server = http.createServer(function(req, res) {
var size = parseInt(url.parse(req.url, true).query.size);
let dogs = new Array(size).fill("dog"); // BAD
// ... use the dog
});The allocation of the array itself is not problematic since arrays are allocated sparsely, but the subsequent filling of the array will take a long time, causing the application to be unresponsive, or even run out of memory. Again, a limit on the size will prevent the attack: var http = require("http"),
url = require("url");
var server = http.createServer(function(req, res) {
var size = parseInt(url.parse(req.url, true).query.size);
if (size > 1024) {
res.statusCode = 400;
res.end("Bad request.");
return;
}
let dogs = new Array(size).fill("dog"); // GOOD
// ... use the dogs
});ExampleFinally, the following example lets a user choose a delay after which a function is executed: var http = require("http"),
url = require("url");
var server = http.createServer(function(req, res) {
var delay = parseInt(url.parse(req.url, true).query.delay);
setTimeout(f, delay); // BAD
});This is problematic because a large delay essentially makes the application wait indefinitely before executing the function. Repeated registrations of such delays will therefore use up all of the memory in the application. A limit on the delay will prevent the attack: var http = require("http"),
url = require("url");
var server = http.createServer(function(req, res) {
var delay = parseInt(url.parse(req.url, true).query.delay);
if (delay > 1000) {
res.statusCode = 400;
res.end("Bad request.");
return;
}
setTimeout(f, delay); // GOOD
});References
javascript/ql/src/Security/CWE-776/XmlBomb.qhelpXML internal entity expansionParsing untrusted XML files with a weakly configured XML parser may be vulnerable to denial-of-service (DoS) attacks exploiting uncontrolled internal entity expansion. In XML, so-called internal entities are a mechanism for introducing an abbreviation for a piece of text or part of a document. When a parser that has been configured to expand entities encounters a reference to an internal entity, it replaces the entity by the data it represents. The replacement text may itself contain other entity references, which are expanded recursively. This means that entity expansion can increase document size dramatically. If untrusted XML is parsed with entity expansion enabled, a malicious attacker could submit a document that contains very deeply nested entity definitions, causing the parser to take a very long time or use large amounts of memory. This is sometimes called an XML bomb attack. RecommendationThe safest way to prevent XML bomb attacks is to disable entity expansion when parsing untrusted data. How this is done depends on the library being used. Note that some libraries, such as recent versions of ExampleThe following example uses the XML parser provided by the const app = require("express")(),
expat = require("node-expat");
app.post("upload", (req, res) => {
let xmlSrc = req.body,
parser = new expat.Parser();
parser.on("startElement", handleStart);
parser.on("text", handleText);
parser.write(xmlSrc);
});At the time of writing, const app = require("express")(),
sax = require("sax");
app.post("upload", (req, res) => {
let xmlSrc = req.body,
parser = sax.parser(true);
parser.onopentag = handleStart;
parser.ontext = handleText;
parser.write(xmlSrc);
});References
javascript/ql/src/Security/CWE-807/ConditionalBypass.qhelpUser-controlled bypass of security checkUsing user-controlled data in a permissions check may allow a user to gain unauthorized access to protected functionality or data. RecommendationWhen checking whether a user is authorized for a particular activity, do not use data that is entirely controlled by that user in the permissions check. If necessary, always validate the input, ideally against a fixed list of expected values. Similarly, do not decide which permission to check for, based on user data. In particular, avoid using computation to decide which permissions to check for. Use fixed permissions for particular actions, rather than generating the permission to check for. ExampleIn this example, we have a server that shows private information for a user, based on the request parameter var express = require('express');
var app = express();
// ...
app.get('/full-profile/:userId', function(req, res) {
if (req.cookies.loggedInUserId !== req.params.userId) {
// BAD: login decision made based on user controlled data
requireLogin();
} else {
// ... show private information
}
});This security check is, however, insufficient since an attacker can craft his cookie values to match those of any user. To prevent this, the server can cryptographically sign the security critical cookie values: var express = require('express');
var app = express();
// ...
app.get('/full-profile/:userId', function(req, res) {
if (req.signedCookies.loggedInUserId !== req.params.userId) {
// GOOD: login decision made based on server controlled data
requireLogin();
} else {
// ... show private information
}
});Referencesjavascript/ql/src/Security/CWE-915/PrototypePollutingAssignment.qhelpPrototype-polluting assignmentMost JavaScript objects inherit the properties of the built-in One way to cause prototype pollution is by modifying an object obtained via a user-controlled property name. Most objects have a special RecommendationUse an associative data structure that is resilient to untrusted key values, such as a Map. In some cases, a prototype-less object created with Object.create(null) may be preferable. Alternatively, restrict the computed property name so it can't clash with a built-in property, either by prefixing it with a constant string, or by rejecting inputs that don't conform to the expected format. ExampleIn the example below, the untrusted value let express = require('express');
let app = express()
app.put('/todos/:id', (req, res) => {
let id = req.params.id;
let items = req.session.todos[id];
if (!items) {
items = req.session.todos[id] = {};
}
items[req.query.name] = req.query.text;
res.end(200);
});One way to fix this is to use Map objects to associate key/value pairs instead of regular objects, as shown below: let express = require('express');
let app = express()
app.put('/todos/:id', (req, res) => {
let id = req.params.id;
let items = req.session.todos.get(id);
if (!items) {
items = new Map();
req.sessions.todos.set(id, items);
}
items.set(req.query.name, req.query.text);
res.end(200);
});Referencesjavascript/ql/src/experimental/heuristics/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.qhelpUntrusted data passed to external API with additional heuristic sourcesUsing unsanitized untrusted data in an external API can cause a variety of security issues. This query reports external APIs that use untrusted data. The results are not filtered so that you can audit all examples. The query provides data for security reviews of the application and you can also use it to identify external APIs that should be modeled as either taint steps, or sinks for specific problems. An external API is defined as a method call to a method that is not defined in the source code, not overridden in the source code, and is not modeled as a taint step in the default taint library. External APIs may be from the third-party dependencies or from internal dependencies. The query reports uses of untrusted data one of the arguments of external API call or in the return value from a callback passed to an external API. RecommendationFor each result:
ExampleIn this first example, a query parameter is read from the express().get('/news', (req, res) => {
let topic = req.query.topic;
res.send(`<h1>${topic}</h1>`);
});This is a reflected XSS sink. The XSS query should therefore be reviewed to confirm that this sink is appropriately modeled, and if it is, to confirm that the query reports this particular result, or that the result is a false positive due to some existing sanitization. In this second example, again a query parameter is read from let path = require('path');
express().get('/data', (req, res) => {
let file = path.join(HOME_DIR, 'public', req.query.file);
res.sendFile(file);
});If the query reported the call to Note that both examples are correctly handled by the standard taint tracking library and security queries. References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-078/CommandInjection.qhelpUncontrolled command line with additional heuristic sourcesCode that passes user input directly to RecommendationIf possible, use hard-coded string literals to specify the command to run or library to load. Instead of passing the user input directly to the process or library function, examine the user input and then choose among hard-coded string literals. If the applicable libraries or commands cannot be determined at compile time, then add code to verify that the user input string is safe before using it. ExampleThe following example shows code that takes a shell script that can be changed maliciously by a user, and passes it straight to var cp = require("child_process"),
http = require('http'),
url = require('url');
var server = http.createServer(function(req<
8000
/span>, res) {
let cmd = url.parse(req.url, true).query.path;
cp.exec(cmd); // BAD
});References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-079/Xss.qhelpClient-side cross-site scripting with additional heuristic sourcesDirectly writing user input (for example, a URL query parameter) to a webpage without properly sanitizing the input first, allows for a cross-site scripting vulnerability. This kind of vulnerability is also called DOM-based cross-site scripting, to distinguish it from other types of cross-site scripting. RecommendationTo guard against cross-site scripting, consider using contextual output encoding/escaping before writing user input to the page, or one of the other solutions that are mentioned in the references. ExampleThe following example shows part of the page URL being written directly to the document, leaving the website vulnerable to cross-site scripting. function setLanguageOptions() {
var href = document.location.href,
deflt = href.substring(href.indexOf("default=")+8);
document.write("<OPTION value=1>"+deflt+"</OPTION>");
document.write("<OPTION value=2>English</OPTION>");
}References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-089/SqlInjection.qhelpDatabase query built from user-controlled sources with additional heuristic sourcesIf a database query (such as a SQL or NoSQL query) is built from user-provided data without sufficient sanitization, a malicious user may be able to run malicious database queries. RecommendationMost database connector libraries offer a way of safely embedding untrusted data into a query by means of query parameters or prepared statements. For NoSQL queries, make use of an operator like MongoDB's ExampleIn the following example, assume the function The handler constructs two copies of the same SQL query involving user input taken from the request object, once unsafely using string concatenation, and once safely using query parameters. In the first case, the query string In the second case, the parameter is embedded into the query string const app = require("express")(),
pg = require("pg"),
pool = new pg.Pool(config);
app.get("search", function handler(req, res) {
// BAD: the category might have SQL special characters in it
var query1 =
"SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" +
req.params.category +
"' ORDER BY PRICE";
pool.query(query1, [], function(err, results) {
// process results
});
// GOOD: use parameters
var query2 =
"SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY=$1" + " ORDER BY PRICE";
pool.query(query2, [req.params.category], function(err, results) {
// process results
});
});References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-094/CodeInjection.qhelpCode injection with additional heuristic sourcesDirectly evaluating user input (for example, an HTTP request parameter) as code without properly sanitizing the input first allows an attacker arbitrary code execution. This can occur when user input is treated as JavaScript, or passed to a framework which interprets it as an expression to be evaluated. Examples include AngularJS expressions or JQuery selectors. RecommendationAvoid including user input in any expression which may be dynamically evaluated. If user input must be included, use context-specific escaping before including it. It is important that the correct escaping is used for the type of evaluation that will occur. ExampleThe following example shows part of the page URL being evaluated as JavaScript code. This allows an attacker to provide JavaScript within the URL. If an attacker can persuade a user to click on a link to such a URL, the attacker can evaluate arbitrary JavaScript in the browser of the user to, for example, steal cookies containing session information. eval(document.location.href.substring(document.location.href.indexOf("default=")+8))The following example shows a Pug template being constructed from user input, allowing attackers to run arbitrary code via a payload such as const express = require('express')
var pug = require('pug');
const app = express()
app.post('/', (req, res) => {
var input = req.query.username;
var template = `
doctype
html
head
title= 'Hello world'
body
form(action='/' method='post')
input#name.form-control(type='text)
button.btn.btn-primary(type='submit') Submit
p Hello `+ input
var fn = pug.compile(template);
var html = fn();
res.send(html);
})Below is an example of how to use a template engine without any risk of template injection. The user input is included via an interpolation expression const express = require('express')
var pug = require('pug');
const app = express()
app.post('/', (req, res) => {
var input = req.query.username;
var template = `
doctype
html
head
title= 'Hello world'
body
form(action='/' method='post')
input#name.form-control(type='text)
button.btn.btn-primary(type='submit') Submit
p Hello #{username}`
var fn = pug.compile(template);
var html = fn({username: input});
res.send(html);
})References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-117/LogInjection.qhelpLog injection with additional heuristic sourcesIf unsanitized user input is written to a log entry, a malicious user may be able to forge new log entries. Forgery can occur if a user provides some input with characters that are interpreted when the log output is displayed. If the log is displayed as a plain text file, then new line characters can be used by a malicious user. If the log is displayed as HTML, then arbitrary HTML may be included to spoof log entries. RecommendationUser input should be suitably sanitized before it is logged. If the log entries are in plain text then line breaks should be removed from user input, using For log entries that will be displayed in HTML, user input should be HTML-encoded before being logged, to prevent forgery and other forms of HTML injection. ExampleIn the first example, a username, provided by the user, is logged using `console.info`. In the first case, it is logged without any sanitization. In the second case, the username is used to build an error that is logged using `console.error`. If a malicious user provides `username=Guest%0a[INFO]+User:+Admin%0a` as a username parameter, the log entry will be splitted in two different lines, where the second line will be `[INFO]+User:+Admin`. const http = require('http');
const url = require('url');
const server = http.createServer((req, res) => {
let q = url.parse(req.url, true);
console.info(`[INFO] User: ${q.query.username}`); // BAD: User input logged as-is
})
server.listen(3000, '127.0.0.1', () => {});In the second example, const http = require('http');
const url = require('url');
const server = http.createServer((req, res) => {
let q = url.parse(req.url, true);
// GOOD: remove newlines from user controlled input before logging
let username = q.query.username.replace(/\n|\r/g, "");
console.info(`[INFO] User: ${username}`);
});
server.listen(3000, '127.0.0.1', () => {});References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-134/TaintedFormatString.qhelpUse of externally-controlled format string with additional heuristic sourcesFunctions like the Node.js standard library function RecommendationEither sanitize the input before including it in the format string, or use a ExampleThe following program snippet logs informa
6D40
tion about an unauthorized access attempt. The log message includes the user name, and the user's IP address is passed as an additional argument to const app = require("express")();
app.get("unauthorized", function handler(req, res) {
let user = req.query.user;
let ip = req.connection.remoteAddress;
console.log("Unauthorized access attempt by " + user, ip);
});However, if a malicious user provides Instead, the user name should be included using the const app = require("express")();
app.get("unauthorized", function handler(req, res) {
let user = req.query.user;
let ip = req.connection.remoteAddress;
console.log("Unauthorized access attempt by %s", user, ip);
});References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-346/CorsMisconfigurationForCredentials.qhelpCORS misconfiguration for credentials transfer with additional heuristic sourcesA server can send the When the RecommendationWhen the Since the ExampleIn the example below, the server allows the browser to send user credentials in a Cross-Origin request. The request header var https = require('https'),
url = require('url');
var server = https.createServer(function(){});
server.on('request', function(req, res) {
let origin = url.parse(req.url, true).query.origin;
// BAD: attacker can choose the value of origin
res.setHeader("Access-Control-Allow-Origin", origin);
res.setHeader("Access-Control-Allow-Credentials", true);
// ...
});This is not secure, since an attacker can choose the value of the var https = require('https'),
url = require('url');
var server = https.createServer(function(){});
server.on('request', function(req, res) {
let origin = url.parse(req.url, true).query.origin,
whitelist = {
"https://example.com": true,
"https://subdomain.example.com": true,
"https://example.com:1337": true
};
if (origin in whitelist) {
// GOOD: the origin is in the whitelist
res.setHeader("Access-Control-Allow-Origin", origin);
res.setHeader("Access-Control-Allow-Credentials", true);
}
// ...
});References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-400/RemotePropertyInjection.qhelpRemote property injection with additional heuristic sourcesDynamically computing object property names from untrusted input may have multiple undesired consequences. For example, if the property access is used as part of a write, an attacker may overwrite vital properties of objects, such as Moreover, if the name of an HTTP header is user-controlled, an attacker may exploit this to overwrite security-critical headers such as RecommendationThe most common case in which prototype pollution vulnerabilities arise is when JavaScript objects are used for implementing map data structures. This case should be avoided whenever possible by using the ECMAScript 2015 When using user input as part of a header name, a sanitization step should be performed on the input to ensure that the name does not clash with existing header names such as ExampleIn the example below, the dynamically computed property var express = require('express');
var app = express();
var myObj = {}
app.get('/user/:id', function(req, res) {
var prop = req.query.userControlled; // BAD
myObj[prop] = function() {};
console.log("Request object " + myObj);
});This is not secure since an attacker may exploit this code to overwrite the property var express = require('express');
var app = express();
var myObj = {}
app.get('/user/:id', function(req, res) {
var prop = "$" + req.query.userControlled; // GOOD
myObj[prop] = function() {};
console.log("Request object " + myObj);
});References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-502/UnsafeDeserialization.qhelpDeserialization of user-controlled data with additional heuristic sourcesDeserializing untrusted data using any deserialization framework that allows the construction of arbitrary functions is easily exploitable and, in many cases, allows an attacker to execute arbitrary code. RecommendationAvoid deserialization of untrusted data if at all possible. If the architecture permits it, then use formats like JSON or XML that cannot represent functions. When using YAML or other formats that support the serialization and deserialization of functions, ensure that the parser is configured to disable deserialization of arbitrary functions. ExampleThe following example calls the const app = require("express")(),
jsyaml = require("js-yaml");
app.get("load", function(req, res) {
let data = jsyaml.load(req.params.data);
// ...
});Using the const app = require("express")(),
jsyaml = require("js-yaml");
app.get("load", function(req, res) {
let data = jsyaml.safeLoad(req.params.data);
// ...
});References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-611/Xxe.qhelpXML external entity expansion with additional heuristic sourcesParsing untrusted XML files with a weakly configured XML parser may lead to an XML External Entity (XXE) attack. This type of attack uses external entity references to access arbitrary files on a system, carry out denial-of-service (DoS) attacks, or server-side request forgery. Even when the result of parsing is not returned to the user, DoS attacks are still possible and out-of-band data retrieval techniques may allow attackers to steal sensitive data. RecommendationThe easiest way to prevent XXE attacks is to disable external entity handling when parsing untrusted data. How this is done depends on the library being used. Note that some libraries, such as recent versions of ExampleThe following example uses the const app = require("express")(),
libxml = require("libxmljs");
app.post("upload", (req, res) => {
let xmlSrc = req.body,
doc = libxml.parseXml(xmlSrc, { noent: true });
});To guard against XXE attacks, the const app = require("express")(),
libxml = require("libxmljs");
app.post("upload", (req, res) => {
let xmlSrc = req.body,
doc = libxml.parseXml(xmlSrc);
});References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-643/XpathInjection.qhelpXPath injection with additional heuristic sourcesIf an XPath expression is built using string concatenation, and the components of the concatenation include user input, it makes it very easy for a user to create a malicious XPath expression. RecommendationIf user input must be included in an XPath expression, either sanitize the data or use variable references to safely embed it without altering the structure of the expression. ExampleIn this example, the code accepts a user name specified by the user, and uses this unvalidated and unsanitized value in an XPath expression constructed using the const express = require('express');
const xpath = require('xpath');
const app = express();
app.get('/some/route', function(req, res) {
let userName = req.param("userName");
// BAD: Use user-provided data directly in an XPath expression
let badXPathExpr = xpath.parse("//users/user[login/text()='" + userName + "']/home_dir/text()");
badXPathExpr.select({
node: root
});
});Instead, embed the user input using the variable replacement mechanism offered by const express = require('express');
const xpath = require('xpath');
const app = express();
app.get('/some/route', function(req, res) {
let userName = req.param("userName");
// GOOD: Embed user-provided data using variables
let goodXPathExpr = xpath.parse("//users/user[login/text()=$userName]/home_dir/text()");
goodXPathExpr.select({
node: root,
variables: { userName: userName }
});
});References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-730/RegExpInjection.qhelpRegular expression injection with additional heuristic sourcesConstructing a regular expression with unsanitized user input is dangerous as a malicious user may be able to modify the meaning of the expression. In particular, such a user may be able to provide a regular expression fragment that takes exponential time in the worst case, and use that to perform a Denial of Service attack. RecommendationBefore embedding user input into a regular expression, use a sanitization function such as lodash's ExampleThe following example shows a HTTP request parameter that is used to construct a regular expression without sanitizing it first: var express = require('express');
var app = express();
app.get('/findKey', function(req, res) {
var key = req.param("key"), input = req.param("input");
// BAD: Unsanitized user input is used to construct a regular expression
var re = new RegExp("\\b" + key + "=(.*)\n");
});Instead, the request parameter should be sanitized first, for example using the function var express = require('express');
var _ = require('lodash');
var app = express();
app.get('/findKey', function(req, res) {
var key = req.param("key"), input = req.param("input");
// GOOD: User input is sanitized before constructing the regex
var safeKey = _.escapeRegExp(key);
var re = new RegExp("\\b" + safeKey + "=(.*)\n");
});References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-770/ResourceExhaustion.qhelpResource exhaustion with additional heuristic sourcesApplications are constrained by how many resources they can make use of. Failing to respect these constraints may cause the application to be unresponsive or crash. It is therefore problematic if attackers can control the sizes or lifetimes of allocated objects. RecommendationEnsure that attackers can not control object sizes and their lifetimes. If object sizes and lifetimes must be controlled by external parties, ensure you restrict the object sizes and lifetimes so that they are within acceptable ranges. ExampleThe following example allocates a buffer with a user-controlled size. var http = require("http"),
url = require("url");
var server = http.createServer(function(req, res) {
var size = parseInt(url.parse(req.url, true).query.size);
let buffer = Buffer.alloc(size); // BAD
// ... use the buffer
});This is problematic since an attacker can choose a size that makes the application run out of memory. Even worse, in older versions of Node.js, this could leak confidential memory. To prevent such attacks, limit the buffer size: var http = require("http"),
url = require("url");
var server = http.createServer(function(req, res) {
var size = parseInt(url.parse(req.url, true).query.size);
if (size > 1024) {
res.statusCode = 400;
res.end("Bad request.");
return;
}
let buffer = Buffer.alloc(size); // GOOD
// ... use the buffer
});ExampleAs another example, consider an application that allocates an array with a user-controlled size, and then fills it with values: var http = require("http"),
url = require("url");
var server = http.createServer(function(req, res) {
var size = parseInt(url.parse(req.url, true).query.size);
let dogs = new Array(size).fill("dog"); // BAD
// ... use the dog
});The allocation of the array itself is not problematic since arrays are allocated sparsely, but the subsequent filling of the array will take a long time, causing the application to be unresponsive, or even run out of memory. Again, a limit on the size will prevent the attack: var http = require("http"),
url = require("url");
var server = http.createServer(function(req, res) {
var size = parseInt(url.parse(req.url, true).query.size);
if (size > 1024) {
res.statusCode = 400;
res.end("Bad request.");
return;
}
let dogs = new Array(size).fill("dog"); // GOOD
// ... use the dogs
});ExampleFinally, the following example lets a user choose a delay after which a function is executed: var http = require("http"),
url = require("url");
var server = http.createServer(function(req, res) {
var delay = parseInt(url.parse(req.url, true).query.delay);
setTimeout(f, delay); // BAD
});This is problematic because a large delay essentially makes the application wait indefinitely before executing the function. Repeated registrations of such delays will therefore use up all of the memory in the application. A limit on the delay will prevent the attack: var http = require("http"),
url = require("url");
var server = http.createServer(function(req, res) {
var delay = parseInt(url.parse(req.url, true).query.delay);
if (delay > 1000) {
res.statusCode = 400;
res.end("Bad request.");
return;
}
setTimeout(f, delay); // GOOD
});References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-776/XmlBomb.qhelpXML internal entity expansion with additional heuristic sourcesParsing untrusted XML files with a weakly configured XML parser may be vulnerable to denial-of-service (DoS) attacks exploiting uncontrolled internal entity expansion. In XML, so-called internal entities are a mechanism for introducing an abbreviation for a piece of text or part of a document. When a parser that has been configured to expand entities encounters a reference to an internal entity, it replaces the entity by the data it represents. The replacement text may itself contain other entity references, which are expanded recursively. This means that entity expansion can increase document size dramatically. If untrusted XML is parsed with entity expansion enabled, a malicious attacker could submit a document that contains very deeply nested entity definitions, causing the parser to take a very long time or use large amounts of memory. This is sometimes called an XML bomb attack. RecommendationThe safest way to prevent XML bomb attacks is to disable entity expansion when parsing untrusted data. How this is done depends on the library being used. Note that some libraries, such as recent versions of ExampleThe following example uses the XML parser provided by the const app = require("express")(),
expat = require("node-expat");
app.post("upload", (req, res) => {
let xmlSrc = req.body,
parser = new expat.Parser();
parser.on("startElement", handleStart);
parser.on("text", handleText);
parser.write(xmlSrc);
});At the time of writing, const app = require("express")(),
sax = require("sax");
app.post("upload", (req, res) => {
let xmlSrc = req.body,
parser = sax.parser(true);
parser.onopentag = handleStart;
parser.ontext = handleText;
parser.write(xmlSrc);
});References
javascript/ql/src/experimental/heuristics/ql/src/Security/CWE-807/ConditionalBypass.qhelpUser-controlled bypass of security check with additional heuristic sourcesUsing user-controlled data in a permissions check may allow a user to gain unauthorized access to protected functionality or data. RecommendationWhen checking whether a user is authorized for a particular activity, do not use data that is entirely controlled by that user in the permissions check. If necessary, always validate the input, ideally against a fixed list of expected values. Similarly, do not decide which permission to check for, based on user data. In particular, avoid using computation to decide which permissions to check for. Use fixed permissions for particular actions, rather than generating the permission to check for. ExampleIn this example, we have a server that shows private information for a user, based on the request parameter var express = require('express');
var app = express();
// ...
app.get('/full-profile/:userId', function(req, res) {
if (req.cookies.loggedInUserId !== req.params.userId) {
// BAD: login decision made based on user controlled data
requireLogin();
} else {
// ... show private information
}
});This security check is, however, insufficient since an attacker can craft his cookie values to match those of any user. To prevent this, the server can cryptographically sign the security critical cookie values: var express = require('express');
var app = express();
// ...
app.get('/full-profile/:userId', function(req, res) {
if (req.signedCookies.loggedInUserId !== req.params.userId) {
// GOOD: login decision made based on server controlled data
requireLogin();
} else {
// ... show private information
}
});Referencesjavascript/ql/src/experimental/heuristics/ql/src/Security/CWE-915/PrototypePollutingAssignment.qhelpPrototype-polluting assignment with additional heuristic sourcesMost JavaScript objects inherit the properties of the built-in One way to cause prototype pollution is by modifying an object obtained via a user-controlled property name. Most objects have a special RecommendationUse an associative data structure that is resilient to untrusted key values, such as a Map. In some cases, a prototype-less object created with Object.create(null) may be preferable. Alternatively, restrict the computed property name so it can't clash with a built-in property, either by prefixing it with a constant string, or by rejecting inputs that don't conform to the expected format. ExampleIn the example below, the untrusted value let express = require('express');
let app = express()
app.put('/todos/:id', (req, res) => {
let id = req.params.id;
let items = req.session.todos[id];
if (!items) {
items = req.session.todos[id] = {};
}
items[req.query.name] = req.query.text;
res.end(200);
});One way to fix this is to use Map objects to associate key/value pairs instead of regular objects, as shown below: let express = require('express');
let app = express()
app.put('/todos/:id', (req, res) => {
let id = req.params.id;
let items = req.session.todos.get(id);
if (!items) {
items = new Map();
req.sessions.todos.set(id, items);
}
items.set(req.query.name, req.query.text);
res.end(200);
});References |
a727142 to
442749b
Compare
| import DataFlow::PathGraph | ||
|
|
||
| /** | ||
| * Holds if the value of `nd` flows into `guard`. |
There was a problem hiding this comment.
This is the only query in the heuristics folder with non-trivial contents. Would it be possible to move the predicates into a shared file?
There was a problem hiding this comment.
Yes, and there already is a ConditionalBypassQuery.qll file where these predicates should have been.
| import DataFlow::PathGraph | ||
| import semmle.javascript.heuristics.AdditionalSources | ||
|
|
||
| from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink |
There was a problem hiding this comment.
I was wondering whether you could move the entire select statement to shared query predicate problems() . If so, you could write:
import javascript
import semmle.javascript.security.dataflow.ExternalAPIUsedWithUntrustedDataQuery as Query
import DataFlow::PathGraph
import semmle.javascript.heuristics.AdditionalSources
query predicate problems(DataFlow::PathNode a, DataFlow::PathNode b, DataFlow::PathNode c, string d, DataFlow::PathNode e, string f) {
Query::problems(a,b,c,d,e,f) and b.getNode() instanceof HeuristicSource
}Not sure if it is an improvement though.
There was a problem hiding this comment.
I don't think that's an improvement, so I prefer not to do that.
I tried to keep the modifications to the ordinary queries as small as possible, so that they wouldn't be harder to read.
ConditionalBypass.ql could use a Query.qll file though, so I'll get on that.
The approach I took was to implement a semantic patch (see backref) that converts the queries into heuristic variants.
And some scripting that converts the existing
.qhelpfiles to.inc.qhelpfiles and adds.qhelpfiles with<include ../>in the right locations.Nothing in the
JS: add heuristic variants of queries that use RemoteFlowSourcecommit was written manually.I did an evaluation (on this commit) to confirm that the new queries dont blow up.
The QL-for-QL alert is a TP (and a nice find).
That CI check will stay red, but we can merge anyway.
We will get re-evaluation of the cached
RemoteFlowSourcelayer when evaluating these new experimental queries.I find this acceptable, given that these queries need to be explicitly enabled, and there should only be one re-evaluation.
The QL-for-QL change is needed for the patch, so I've included it in this PR.
This PR nicely complements the new query-suite added by
turbo: #11702