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

SQL Injection/Syntax error inserting GeoJSON documents with single quotes #6194

Closed
yocontra opened this issue Jun 30, 2016 · 16 comments · Fixed by #6303
Closed

SQL Injection/Syntax error inserting GeoJSON documents with single quotes #6194

yocontra opened this issue Jun 30, 2016 · 16 comments · Fixed by #6303
Labels
dialect: postgres For issues and PRs. Things that involve PostgreSQL (and do not involve all dialects). P1: important For issues and PRs. type: bug

Comments

@yocontra
Copy link
Contributor

yocontra commented Jun 30, 2016

What you are doing?

Inserting inserting GeoJSON documents with single quotes in an attribute.

What do you expect to happen?

No errors.

What is actually happening?

Errors.

Executing (default): INSERT INTO "boundaries" ("id","name","type","geo","createdAt","updatedAt") VALUES ('0138248','Jacksons'' Gap','place',ST_GeomFromGeoJSON('{"type":"MultiPolygon","properties":{"NAME":"Jacksons' Gap","NAMELSAD":"Jacksons' Gap town","coordinates":[]}'),'2016-06-30 20:45:23.143 +00:00','2016-06-30 20:45:23.143 +00:00') RETURNING *;

SequelizeDatabaseError: syntax error at or near "Gap"

Seems like the GeoJSON string is not being escaped at all, so having a single quote in any property in the GeoJSON is causing a syntax error

Dialect: postgres
Database version: 9.5.3
Sequelize version: 3.23.4

@janmeier janmeier added type: bug dialect: postgres For issues and PRs. Things that involve PostgreSQL (and do not involve all dialects). labels Jul 3, 2016
@yocontra
Copy link
Contributor Author

Just to be more clear: This is a pretty major security issue. Anyone who accepts user data with geojson types is vulnerable to a SQL injection on the insert.

@yocontra
Copy link
Contributor Author

yocontra commented Jul 18, 2016

Proof of concept:

{
  "type": "MultiPolygon",
  "properties": {
    "exploit": "'); drop schema public cascade; -- "
  },
  "coordinates": []
}

Send this object to any Sequelize-backed service as the value of a GeoJSON type and it will drop the entire database - not good!

Query generated by Sequelize:

INSERT INTO "boundaries" ("geo") VALUES (ST_GeomFromGeoJSON('{"type":"MultiPolygon","properties":{"exploit": "'); drop schema public cascade; -- "},"coordinates":[]}')) RETURNING *;

@yocontra yocontra changed the title Syntax error inserting GeoJSON documents with single quotes SQL Injection/Syntax error inserting GeoJSON documents with single quotes Jul 18, 2016
@PizzaBrandon
Copy link

This is now listed in NSP as a critical vulnerability and breaks all builds that use nsp during CI. https://nodesecurity.io/advisories/122

@yocontra
Copy link
Contributor Author

yocontra commented Jul 18, 2016

@PizzaBrandon

  • Vulnerability isn't in PostGIS or Postgres via ST_GeomFromGeoJSON, its solely on sequelize to do the escaping before passing the value to the DB.
  • Validation won't help since this works in valid GeoJSON

@pdehaan
Copy link

pdehaan commented Jul 18, 2016

FWIW, you can set up an nsp exception for this by adding an .nsprc file with something like the following:

{
  "exceptions": [
    "https://nodesecurity.io/advisories/122" // SQL Injection via GeoJSON
  ]
}

@mickhansen
Copy link
Contributor

Fix released in v3.23.5

@sushantdhiman
Copy link
Contributor

Now both MySQL and Postgres SQL Injection cases has been fixed. Everyone is advised to upgrade to v3.23.6

@contra You need to edit the NSP advisory with patch version v3.23.6

More details #6302 and #6305

@yocontra
Copy link
Contributor Author

yocontra commented Jul 19, 2016

@sushantdhiman I didn't make the NSP advisory, I think that was @PizzaBrandon

@sushantdhiman
Copy link
Contributor

@contra It says issue reported by /user/contra, nevertheless advisory has been updated with patch version.

Thanks for reporting this, Also please report the security issues to maintainers privately so we can patch it before any attacker knows about it :)

@yocontra
Copy link
Contributor Author

yocontra commented Jul 19, 2016

@sushantdhiman Who is the contact point? There's a lot of contributors on this repository.

Might be a good idea to have a security.md with info

@sushantdhiman
Copy link
Contributor

You can report to @mickhansen, @janmeier or me. You can contact by

  • Email (listed on our profiles / blogs / package.json)
  • Slack to mhansen, janmeier or sushantdhiman

@nstarke
Copy link

nstarke commented Jul 19, 2016

^ it might be a good idea to add this contact information to the documentation somewhere specifically for security issues.

@sushantdhiman
Copy link
Contributor

For future references, now we have responsible-disclosure section in readme. Security issues must be reported accordingly

@yocontra
Copy link
Contributor Author

Still not available in the npm version. Maybe this should be backported if the 4.x release isn't ready.

@felixfbecker
Copy link
Contributor

@contra #6194 (comment)

@yocontra
Copy link
Contributor Author

@felixfbecker Sorry, checked the changelog and saw it under the 4.0.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialect: postgres For issues and PRs. Things that involve PostgreSQL (and do not involve all dialects). P1: important For issues and PRs. type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants