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

SQL injection with PostgreSQL #3545

Closed
huguesdk opened this Issue Apr 16, 2015 · 12 comments

Comments

4 participants
@huguesdk

huguesdk commented Apr 16, 2015

Backslashes are not escaped properly in strings when using PostgreSQL, leading to possible SQL injections.

The following code drops the students table.

var Sequelize, Student, sequelize;

Sequelize = require('sequelize');

sequelize = new Sequelize('database', 'user', 'password', {
  dialect: 'postgres',
  host: 'school.example',
  logging: false,
  define: {
    timestamps: false
  }
});

Student = sequelize.define('student', {
  name: Sequelize.STRING
});

Student.create({
  name: 'Robert\\\'); DROP TABLE "students"; --'
}).then(function(result) {
  console.log('Successfully created one student:', result.get());
  return Student.findAll();
}).then(function(result) {
  return console.log('Number of students:', result.length);
})["catch"](function(error) {
  return console.error('Oh no! Here strikes the mom again:', error.message);
});

The console output is:

Successfully created one student: { id: null, name: 'Robert\\\'); DROP TABLE "students"; --' }
Oh no! Here strikes the mom again: relation "students" does not exist

Tested with PostgreSQL 8.2.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 16, 2015

Hmm that's highly strange.
Will figure out a fix immediately

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 16, 2015

What version of sequelize?

mickhansen added a commit that referenced this issue Apr 16, 2015

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 16, 2015

Unable to prove attack vector in: 99d8ced

Could you tell me where i'm wrong in the unit test or ammend it so it fails in a PR?

@huguesdk

This comment has been minimized.

huguesdk commented Apr 16, 2015

Wow, that’s a quick reaction! Thanks!

I tested with Sequelize 2.0.5. I think the test should check that the generated query contains the value as (in SQL):

'Robert\\''); DROP TABLE "students"; --'

that is, escaped in JavaScript:

\'Robert\\\\\'\'); DROP TABLE "students"; --\'

instead of currently (in SQL):

'Robert\''); DROP TABLE "students"; --'

that is, escaped in JavaScript:

\'Robert\\\'\'); DROP TABLE "students"; --\'
@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 16, 2015

@huguesdk You are welcome to ammend the test to show what you mean. Although i would think that simply testing that the query passes (and doesn't throw a table does not exist error) should be sufficient.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 16, 2015

Even with your modifications the attack doesn't work as far as i can tell.

@huguesdk

This comment has been minimized.

huguesdk commented Apr 16, 2015

I just saw in http://www.postgresql.org/docs/current/static/sql-syntax-lexical.html#SQL-SYNTAX-STRINGS-ESCAPE that the default setting for string escaping with backslashes in normal strings changed in PostgreSQL 9.1. Beginning with that version, they are disabled by default for normal strings, and work only if the string is prefixed with E. I tested this with PostgreSQL 8.2 so I think this explains it (and also why this wasn’t discovered before). I wonder how other ORMs work around this. Could the standard_conforming_strings configuration parameter (which controls this behavior) be set to true for each session?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 16, 2015

Yes we could probably set that on connect.
Although do keep in mind that not all features in Sequelize work for Postgresql 8.2.
Our findOrCreate needs featureso nly in 9.2 and up (IIRC)

@huguesdk

This comment has been minimized.

huguesdk commented Apr 17, 2015

In SQLAlchemy they do this:

self._backslash_escapes = self.server_version_info < (8, 2) or \
    connection.scalar(
    "show standard_conforming_strings"
) == 'off'

They escape all backslashes or not depending on this value. The version check is because standard_conforming_strings doesn’t exist for PostgreSQL earlier than 8.2.

Even with PostgreSQL 9.2, Sequelize would be vulnerable to this if standard_conforming_strings is set to false on the server. So we should either do the same as SQLAlchemy or force standard_conforming_strings to true on connect.

@janmeier janmeier closed this in 76727a6 Apr 20, 2015

@janmeier

This comment has been minimized.

Member

janmeier commented Apr 20, 2015

Fixed by settting standard_conforming_strings=on on connection. pg 8.2 may have reached its EOL in december 2011, put since this was not introduced before 9.1 it's worth it to fix this on the sequelize side.

Thanks for bringing this to our attention

@andythecoderman

This comment has been minimized.

andythecoderman commented May 15, 2017

I know this is an old issue, but I'm trying to figure out if sequelize with postgres uses paramaterized queries as supported by pg. If not by default is there a config setting to enable them?

@janmeier

This comment has been minimized.

Member

janmeier commented May 18, 2017

@andythecoderman raw queries support bind parameters, but regular sequelize calls (findAll) etc. does now utilize it yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment