CodeQL documentation
CodeQL resources

Type confusion through parameter tampering

ID: js/type-confusion-through-parameter-tampering
Kind: path-problem
Security severity: 9.8
Severity: error
Precision: high
Tags:
   - security
   - external/cwe/cwe-843
Query suites:
   - javascript-code-scanning.qls
   - javascript-security-extended.qls
   - javascript-security-and-quality.qls

Click to see the query in the CodeQL repository

Sanitizing untrusted HTTP request parameters is a common technique for preventing injection attacks such as SQL injection or path traversal. This is sometimes done by checking if the request parameters contain blacklisted substrings.

However, sanitizing request parameters assuming they have type String and using the builtin string methods such as String.prototype.indexOf is susceptible to type confusion attacks. In a type confusion attack, an attacker tampers with an HTTP request parameter such that it has a value of type Array instead of the expected type String. Furthermore, the content of the array has been crafted to bypass sanitizers by exploiting that some identically named methods of strings and arrays behave differently.

Recommendation

Check the runtime type of sanitizer inputs if the input type is user-controlled.

An even safer alternative is to design the application so that sanitization is not needed, for instance by using prepared statements for SQL queries.

Example

For example, Node.js server frameworks usually present request parameters as strings. But if an attacker sends multiple request parameters with the same name, then the request parameter is represented as an array instead.

In the following example, a sanitizer checks that a path does not contain the ".." string, which would allow an attacker to access content outside a user-accessible directory.

var app = require("express")(),
  path = require("path");

app.get("/user-files", function(req, res) {
  var file = req.param("file");
  if (file.indexOf("..") !== -1) {
    // BAD
    // we forbid relative paths that contain ..
    // as these could leave the public directory
    res.status(400).send("Bad request");
  } else {
    var absolute = path.resolve("/public/" + file);
    console.log("Sending file: %s", absolute);
    res.sendFile(absolute);
  }
});

As written, this sanitizer is ineffective: an array like ["../", "/../secret.txt"] will bypass the sanitizer. The array does not contain ".." as an element, so the call to indexOf returns -1 . This is problematic since the value of the absolute variable then ends up being "/secret.txt". This happens since the concatenation of "/public/" and the array results in "/public/../,/../secret.txt", which the resolve-call converts to "/secret.txt".

To fix the sanitizer, check that the request parameter is a string, and not an array:

var app = require("express")(),
  path = require("path");

app.get("/user-files", function(req, res) {
  var file = req.param("file");
  if (typeof file !== 'string' || file.indexOf("..") !== -1) {
    // GOOD
    // we forbid relative paths that contain ..
    // as these could leave the public directory
    res.status(400).send("Bad request");
  } else {
    var absolute = path.resolve("/public/" + file);
    console.log("Sending file: %s", absolute);
    res.sendFile(absolute);
  }
});

References

  • © GitHub, Inc.
  • Terms
  • Privacy