Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PSA: Upgrade to Sequelize >= 6.19.2 & Our recent breaking change in v6 #14519

Closed
ephys opened this issue May 18, 2022 · 14 comments
Closed

PSA: Upgrade to Sequelize >= 6.19.2 & Our recent breaking change in v6 #14519

ephys opened this issue May 18, 2022 · 14 comments

Comments

@ephys
Copy link
Member

ephys commented May 18, 2022

Hi everyone 👋

We have recently released Sequelize 6.19.1 (since patched as 6.19.2) which includes a breaking change.

We normally want to fully respect semver rules, unfortunately a very serious SQL injection exploit has surfaced and it was impossible for us to fix it without introducing a breaking change. We considered the issue serious enough to break the rule and release the fix in V6. Most of you are going to be completely unaffected, but a small percentage of users may experience breakage with this version, more below

The exploit

The SQL injection exploit is related to replacements. Here is such an example:

In the following query, some parameters are passed through replacements, and some are passed directly through the where option.

User.findAll({
  where: or(
    literal('soundex("firstName") = soundex(:firstName)'),
    { lastName: lastName },
  ),
  replacements: { firstName },
})

This is a very legitimate use case, but this query was vulnerable to SQL injection due to how Sequelize processed the query: Sequelize built a first query using the where option, then passed it over to sequelize.query which parsed the resulting SQL to inject all :replacements.

If the user passed values such as

{
  "firstName": "OR true; DROP TABLE users;",
  "lastName": ":firstName"
}

Sequelize would first generate this query:

SELECT * FROM users WHERE soundex("firstName") = soundex(:firstName) OR "lastName" = ':firstName'

Then would inject replacements in it, which resulted in this:

SELECT * FROM users WHERE soundex("firstName") = soundex('OR true; DROP TABLE users;') OR "lastName" = ''OR true; DROP TABLE users;''

As you can see this resulted in arbitrary user-provided SQL being executed.

The solution & breaking change

The way to fix this without breaking everyone's codebase was to use a smarter SQL parser that would only inject replacements in places where native SQL parameters are allowed (with some safe exceptions to stay as backward compatible as possible).

This means that :replacements will not be treated as such anymore if they are quoted, as we treat it as being part of a SQL string.

Details here: https://github.com/sequelize/sequelize/blob/v6/test/unit/utils/sql.test.js

On top of this, in Sequelize 7, we rewrote this system completely to handle both steps at the same time. Removing all risk of SQL injection (#14447).

Some of you relied on this to inject parameters inside of sql strings, which is why this can be considered a breaking change.
Note however that doing that was not safer than simply concatenating, and it being done by us may have given you a false sense of security regarding input sanitization.


Once again, we are very sorry for the inconvenience this causes.

- Zoé

@ephys ephys pinned this issue May 18, 2022
@ephys ephys changed the title PSA: Upgrade to Sequelize 6.19.2 & Our recent intended breaking change in v6 PSA: Upgrade to Sequelize 6.19.2 & Our recent breaking change in v6 May 18, 2022
@ephys ephys closed this as completed May 19, 2022
@sdepold
Copy link
Member

sdepold commented May 19, 2022

Love the details and explanation ❤️

@fzn0x
Copy link
Member

fzn0x commented May 26, 2022

Yes, we are very sorry for this, especially for me who missed this on the PR review phase, we will improve our workflow and yes, make Sequelize much better ORM.

We will learn from this mistake. Growing has no stop word.

@HilSny
Copy link

HilSny commented Jun 2, 2022

Hey all!

We completely missed this message and thus introduced a pretty critical bug into our system. I understand the semvar not respecting the breaking change but I would expect there to be a mention of this breaking change in the releases page as you have to search through issues to find documentation of this breaking change (something I don't do unless I have an issue :) ). Can an update be made to the releases page so that there is visibility on this change and so others don't run into this issue as well?

Thanks!

@ephys
Copy link
Member Author

ephys commented Jun 2, 2022

That's a fair point! I've just added a notice in our release notes https://github.com/sequelize/sequelize/releases/tag/v6.19.1

@ephys ephys changed the title PSA: Upgrade to Sequelize 6.19.2 & Our recent breaking change in v6 PSA: Upgrade to Sequelize >= 6.19.2 & Our recent breaking change in v6 Jun 2, 2022
@HilSny
Copy link

HilSny commented Jun 2, 2022

Thank you!

@ckvv
Copy link

ckvv commented Jun 6, 2022

@ephys
Sorry, I'm not sure what the breaking change is, Does this mean that when upgrading to Sequelize >= 6.19.2, The replacements will only process strings in literal() ? like i (#14447).

@ephys
Copy link
Member Author

ephys commented Jun 7, 2022

When using sequelize.query, replacements will be replaced anywhere where a bind parameters would be valid sql syntax) at least that's what we're aiming for)

In Model methods (Model.findAll and the like), they will only be replaced in literal()

@josnidhin
Copy link

josnidhin commented Jun 9, 2022

Just got bitten due to this change. We had a missing space in one of the raw sql statements like this created_at <:toDate. The change expects queries like the one shown to be written as created_at < :toDate

@ilmartyrk
Copy link

Hi, I found that this change will not do any replacements for queries using DO $$

@ephys
Copy link
Member Author

ephys commented Jul 14, 2022

@ilmartyrk It doesn't, this was raised a few months ago but we do not see a way to enable it without introducing a vector for SQL injection. You can find more information about it here #14472 (comment)

@dainguyendo
Copy link

Hi, currently reviewing a project's usage of replacements in regards to this vulnerability. Although I'm unclear on identifying incorrect previous usage that would be resolved in this fix. Would it be possible for someone to provide an examples of an incorrect usage versus an okay usage?

I see a helpful comment here, #14519 (comment) but still remain confused.

Thanks all for the PSA and fix 🙏

@ilmartyrk
Copy link

@ephys I was thinking that maybe you could expose the injectReplacements under Utils so there could be an easier workaround till this issue finds its solution. I tested a little and I could then possibly do something following

const querybody = 'SOME SQL with :key'
const body = Sequelize.Utils.injectReplacements(querybody, 'postgres', {
        key: value,
    });
const result = await db
            .query(
                `
                DO $$ BEGIN
                ${body}
                END $$
                ;`,
                {
                    type: db.QueryTypes.INSERT,
                    raw: true,
                    transaction: transaction
                }
            );

@ephys
Copy link
Member Author

ephys commented Jul 14, 2022

@ilmartyrk

The usage of a template literal here is still vulnerable, if :key is '$$', I expect this to break

You can do something like this instead:

// v6:
// DISCLAIMER: /lib is not a public API, and therefore not considered SemVer and can break with any new release, including patch releases
import { injectReplacements } from 'sequelize/lib/utils/sql';
import { QueryTypes } from 'sequelize';

// if you're using v7:
import { injectReplacements } from '@sequelize/core/_non-semver-use-at-your-own-risk_/utils/sql.js';
import { QueryTypes } from '@sequelize/core';

const queryBody = 'BEGIN SOME SQL with :key END'
const escapedQuery = injectReplacements(queryBody, sequelize.dialect, {
  key: value,
});

const result = await db.query(`DO :query;`, {
  replacements: { query: escapedQuery },
  type: QueryTypes.INSERT,
  raw: true,
  transaction,
});

I have not tested it but I would expect this to work

@ephys
Copy link
Member Author

ephys commented Jul 14, 2022

@dainguyendo This thread has a bit more information: #13817

Unfortunately I don't have more examples than the one I posted in the first post of this thread

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants