fix(security): resolve a potential SQL injection bypass through objects

[wellwelwel]

Important

Normally, this would be handled with much more care, but considering that it is a vulnerability that has been exploited since 2022 in a completely public manner, I also decided to resolve it publicly.

Fixes #1914, Fixes #4051, Fixes #2528, Fixes #3481.

---

Vulnerability Exploitation

• https://flattsecurity.medium.com/finding-an-unseen-sql-injection-by-bypassing-escape-functions-in-mysqljs-mysql-90b27f6542b4
• https://blog.mantrainfosec.com/blog/18/prepared-statements-prepared-to-be-vulnerable

---

Changes

This fix doesn't change MySQL2's behavior, nor does it introduce any kind of breaking change.

Unlike what's recommended in the two exploits (forcing the use of true in stringifyObjects), this doesn't fix the reported vulnerability, it just "hides" it.

I researched several scenarios where it's actually necessary to convert values derived from objects, and there are the three safe/expected situations:

✅ UPDATE

const query = format('UPDATE users SET ?', [
  { name: 'foo', email: 'bar@test.com' },
]);

✅ INSERT + SET

const query = format('INSERT INTO users SET ?', [
  { name: 'foo', email: 'bar@test.com' },
]);

✅ INSERT ... KEY UPDATE

const query = format(
  'INSERT INTO users (name, email) VALUES (?, ?) ON DUPLICATE KEY UPDATE ?',
  ['foo', 'bar@test.com', { name: 'foo', email: 'bar@test.com' }]
);

❗️ For SELECT, conventional INSERT, DELETE, etc., they will always cause a potential SQL injection. For example:

const query = format('SELECT * FROM users WHERE email = ?', [{ email: 1 }]);

// ❌ SELECT * FROM users WHERE email = `email` = 1

---

The replacement of sqlstring (and why)

Mainly motivated by this comment: mysqljs/sqlstring#49 (comment), sqlstring must be compatible with Node.js v0.6. This limits the natural improvement of new language features, such as the addition of BigInt (v10.4) and Uint8Array (v0.10).

---

Security + Breaking Change Conflicts

I read the discussion around #1914 and evaluated the ideas:

• Remove stringifyObjects entirely, create an alternative feature such as sqlstring.SET, etc.

I thought carefully about the issue and the time that has passed without resolution (~3 years), probably due to the challenge of negotiating a breaking change that affects so many users and packages simultaneously.

---

The solution

Instead of introducing a breaking change, I just handled the parameters properly, for example:

• Performing an AST-based approach, this way it captures both spaces, line breaks and tables, doesn't depend on RegExp, supports comments and understands when key values are used inside a string 🔥

Then, in short:

// Lazy: compute SET position only when we first encounter an object
if (setIndex === -2) setIndex = findSetKeyword(sql);

if (
  setIndex !== -1 &&
  setIndex < placeholderPosition &&
  !hasSqlString(currentValue) &&
  !Array.isArray(currentValue) &&
  !Buffer.isBuffer(currentValue) &&
  !isDate(currentValue) &&
  isRecord(currentValue)
) {
  escapedValue = objectToValues(currentValue, timezone);
  setIndex = -1;
}

This way, SET or KEY UPDATE provide the expected functionality, while SELECT, common INSERT, etc., return the same behavior when stringifyObjects is enabled, for example [Object ...]. Unlike a breaking change, these queries should no longer process the object values in any case, after all, that is where the vulnerability lies.

---

Long term support and pros

It will enable us to move forward with fixing issues #2528, #3481, and #1176, including support for BigInt (mysqljs/sqlstring#49).

---

⚡️ Performance

Not only the language resource point, but also performance limitations, where SQL Escaper outperforms sqlstring up to 40%:

Each benchmark formats 10,000 queries using format with 100 mixed values (numbers, strings, null, and dates), comparing SQL Escaper against the original sqlstring through hyperfine:

Benchmark	sqlstring	SQL Escaper	Difference
Select 100 values	248.8 ms	178.7 ms	1.39x faster
Insert 100 values	247.5 ms	196.2 ms	1.26x faster
SET with 100 values	257.5 ms	205.2 ms	1.26x faster
SET with 100 objects	348.3 ms	250.5 ms	1.39x faster
ON DUPLICATE KEY UPDATE with 100 values	466.2 ms	394.6 ms	1.18x faster
ON DUPLICATE KEY UPDATE with 100 objects	558.2 ms	433.9 ms	1.29x faster

---

Considerations

I still consider that this problem should indeed be solved on the sqlstring side, but unfortunately I was unable to contribute due to the limitations of the environment (Node.js v0.6): String.prototype.search(), for example, requires version 0.10 😅

I believe that MySQL2 has evolved differently from mysqljs/mysql. Even if we ignore the security issue, I still believe that MySQL2 has a lot to gain from the level of performance that new approaches can offer.

SQL Escaper is also compatible with mysqljs/mysql as usual, but requires at least Node.js v12.

Tip

With this idea, for example, we can discuss different approaches, such as removing it entirely, etc., with the project already safely and available in the meantime.

[wellwelwel]

cc @sidorares

If you like the solution, I would be happy to assign you as a maintainer, as was done with AWS SSL Profiles. I'm totally open to migrating the repository to mysqljs organization or what you think is best.

---

cc @dougwilson

It will be a pleasure to contribute to sqlstring, regardless of the direction this PR takes. Also, from docs:

Acknowledgements

• SQL Escaper is adapted from sqlstring (MIT), modernizing it with high performance, TypeScript support and multi-runtime compatibility.
• Special thanks to Douglas Wilson for the original sqlstring project and its contributors.

---

@mantrainfosec and all the community

The proposed repository is: https://github.com/mysqljs/sql-escaper.

It has the same strictness as lru.min, with 100% coverage of real tests, passing in all sqlstring tests (and including new ones too, specially considered for MySQL2), 100% typed, and fully documented too.

Please, feel free to review 💙

[dougwilson]

I have not really been in Node.js land for some time now and am open to delegating the modules to someone who is / the mysqljs org too.

[wellwelwel]

I have not really been in Node.js land for some time now and am open to delegating the modules to someone who is / the mysqljs org too.

@dougwilson, although I'm not part of mysqljs organization, I'm open to helping in any way I can 🙋🏻‍♂️

In this specific case, I didn't simply think about creating a different project, I recognized that there is a really strong philosophy behind sqlstring and mysqljs/mysql that must be respected.

[wellwelwel]

It's finally ready 🎉

Now it uses an AST-based approach to:

• Support multi lines, spaces and tables.
• Support SQL comments, including multi line comments.
• Distinguish when a keyword is used as value.
• Distinguish between multiple clauses/keywords in the same query.
• Distinguish when a column has a keyword name.

New features:

• Supports Uint8Array and BigInt ✨
• Preserves JSON path expressions

As proof of concept, I fixed #2528 and #3481 too 🙋🏻‍♂️

---

For those concerned about this issue, you can use an overrides in your package.json:

"dependencies": {
  "mysql2": "^3.16.3"
},
"overrides": {
  "sqlstring": "npm:sql-escaper"
}

• Then, clean the node_modules and run npm i.

Now, it will work normally for UPDATE, INSERT...SET and INSERT...KEY UPDATE, but it will not compromise queries as reported in #4051 (comment).

Tip

The same works for mysqljs/mysql, but it requires at least Node.js v12.

[sidorares]

@wellwelwel sorry just found time to get myself up to date with recent discussions

I'm pretty sure node versions can be listed a bit for sqlstring. We can stay reasonable conservative but v12 is probably quite reasonable

If you believe modernized code is 100% compatible with sqlstring surface api mo vote is to just replace it with your version
I invited you to mysqljs org - let me know if you need anything else

[wellwelwel]

If you believe modernized code is 100% compatible with sqlstring surface api mo vote is to just replace it with your version
I invited you to mysqljs org - let me know if you need anything else

Thanks, @sidorares! All sqlstring tests pass, but if there is any unnoticed usage, I mentioned in the documentation that this must be treated as a bug and reported/fixed (except for potentially vulnerable behavior, naturally).

I'm polishing things up after the migration, before continuing here. Thanks again!

[wellwelwel]

I'm polishing things up after the migration, before continuing here. Thanks again!

Well (not me), I went over every project detail, created even more tests for new use cases, and so far, I really don't see anything different from sqlstring for final users.

If any conflicts arise, I believe users will report them. From now on, it's ready to merge 🤝