Skip to content
This repository has been archived by the owner on Nov 9, 2021. It is now read-only.

string escape is incorrect, especially when string contains \n or ', which may cause SQL injection #366

Open
yibn2008 opened this issue Sep 20, 2018 · 1 comment

Comments

@yibn2008
Copy link

yibn2008 commented Sep 20, 2018

IMPORTANT: This issue may cause SQL injection:

const unsafeString = "' AND '1' = '1"
console.log(squel.select().from('some_table').where('id = ?', unsafeString).toString())
// output: SELECT * FROM some_table WHERE (id = '' AND '1' = '1')

Problem

for example:

const squel = require('squel')
const lines = 'ABC\nDEF\'IJK'
const json = JSON.stringify({lines: lines})
console.log(squel.select().from('some_table').where('lines = ? OR json = ?', lines, json).toString())

expect:

SELECT * FROM some_table WHERE (lines = 'ABC\nDEF\'IJK' OR json = '{\"lines\":\"ABC\\nDEF\'IJK\"}')

actual:

SELECT * FROM some_table WHERE (lines = 'ABC
DEF'IJK' OR json = '{"lines":"ABC\nDEF'IJK"}')

Workaround

Use sqlstring will escape string value correctly:

const sqlstring = require('sqlstring')
const squel = require('squel')
squel.registerValueHandler('string', (v) => {
  return {
    value: sqlstring.escape(v),
    rawNesting: true
  }
});
@yibn2008 yibn2008 changed the title MySQL string escape is incorrect MySQL string escape is incorrect, especially when string contains \n Sep 20, 2018
@yibn2008 yibn2008 changed the title MySQL string escape is incorrect, especially when string contains \n string escape is incorrect, especially when string contains \n or ' Sep 20, 2018
@yibn2008 yibn2008 changed the title string escape is incorrect, especially when string contains \n or ' string escape is incorrect, especially when string contains \n or ', which may cause SQL injection Sep 20, 2018
@demurgos
Copy link
Contributor

demurgos commented Apr 1, 2019

I just tracked down a bug in my code base that was caused by the similar issue: by default, STRINGS ARE NOT ESCAPED!!!

console.log(squel.update()
        .table("t")
        .set("name", "foo'bar")
        .toString())
// Prints:
// UPDATE t SET name = 'foo'bar'

Note that the string is not escaped.

This is an extremely surprising and dangerous default!

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

No branches or pull requests

2 participants