Skip to content

Fix SQL injection #18

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 9, 2015
Merged

Fix SQL injection #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 51 additions & 7 deletions lib/mssql.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,50 @@ function dateToMsSql(val) {
}
}

function escape(val) {
if (val === undefined || val === null) {
return 'NULL';
}

switch (typeof val) {
case 'boolean':
return (val) ? 1 : 0;
case 'number':
return val + '';
}

if (typeof val === 'object') {
val = (typeof val.toISOString === 'function')
? val.toISOString()
: val.toString();
}

val = val.replace(/[\0\n\r\b\t\\\'\"\x1a]/g, function (s) {
switch (s) {
case "\0":
return "\\0";
case "\n":
return "\\n";
case "\r":
return "\\r";
case "\b":
return "\\b";
case "\t":
return "\\t";
case "\x1a":
return "\\Z";
case "\'":
return "''"; // For sql server, double quote
case "\"":
return s; // For oracle
default:
return "\\" + s;
}
});
// return "q'#"+val+"#'";
return "'" + val + "'";
}
Copy link
Member

@bajtos bajtos Jan 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until we ditch out string concatenation in favour of parameterized commands/queries: it would be nice to move this escape function to loopback-connector/lib/sql.js so that there is only one instance of this code shared by all SQL connectors.


//toDatabase is used for formatting data when inserting/updating records
// it is also used when building a where clause for filtering selects
// in the case of update/insert we want the data to be returned raw
Expand All @@ -437,7 +481,7 @@ MsSQL.prototype.toDatabase = function (prop, val, wrap) {
return null;
}
if (prop.type && prop.type.modelName) {
return "'" + JSON.stringify(val) + "'";
return escape(JSON.stringify(val));
}
if (val.constructor && val.constructor.name === 'Object') {
var operator = Object.keys(val)[0]
Expand All @@ -456,12 +500,12 @@ MsSQL.prototype.toDatabase = function (prop, val, wrap) {
//check if it is an array of string, because in that cause we need to
// wrap them in single quotes
if (typeof val[0] === 'string') {
return "'" + val.join("','") + "'";
return escape(val.join("','"));
}
return val.join(',');
} else {
if (typeof val === 'string') {
val = "'" + val + "'";
val = escape(val);
}
return val;
}
Expand All @@ -471,12 +515,12 @@ MsSQL.prototype.toDatabase = function (prop, val, wrap) {
}
if (!prop) {
if (typeof val === 'string' && wrap) {
val = "'" + val + "'";
val = escape(val);
}
return val;
}
if (prop.type.name === 'Number') {
return val;
return escape(val);
}
if (prop.type.name === 'Date') {
if (!val) {
Expand All @@ -488,7 +532,7 @@ MsSQL.prototype.toDatabase = function (prop, val, wrap) {
}
val = dateToMsSql(val);
if (wrap) {
val = "'" + val + "'";
val = escape(val);
}
return val;
}
Expand All @@ -501,7 +545,7 @@ MsSQL.prototype.toDatabase = function (prop, val, wrap) {
}

if (wrap) {
return "'" + val.toString().replace(/'/g, "''") + "'";
return escape(val.toString());
}
return val.toString();
};
Expand Down
137 changes: 137 additions & 0 deletions test/mssql.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
require('./init');

var should = require('should');

var Post, db;

describe('mssql connector', function () {

before(function () {
db = getDataSource();

Post = db.define('PostWithBoolean', {
title: { type: String, length: 255, index: true },
content: { type: String },
approved: Boolean
});
});

it('should run migration', function (done) {
db.automigrate('PostWithBoolean', function () {
done();
});
});

var post;
it('should support boolean types with true value', function(done) {
Post.create({title: 'T1', content: 'C1', approved: true}, function(err, p) {
should.not.exists(err);
post = p;
Post.findById(p.id, function(err, p) {
should.not.exists(err);
p.should.have.property('approved', true);
done();
});
});
});

it('should support updating boolean types with false value', function(done) {
Post.update({id: post.id}, {approved: false}, function(err) {
should.not.exists(err);
Post.findById(post.id, function(err, p) {
should.not.exists(err);
p.should.have.property('approved', false);
done();
});
});
});


it('should support boolean types with false value', function(done) {
Post.create({title: 'T2', content: 'C2', approved: false}, function(err, p) {
should.not.exists(err);
post = p;
Post.findById(p.id, function(err, p) {
should.not.exists(err);
p.should.have.property('approved', false);
done();
});
});
});

it('should single quote escape', function(done) {
Post.create({title: "T2", content: "C,D", approved: false}, function(err, p) {
should.not.exists(err);
post = p;
Post.findById(p.id, function(err, p) {
should.not.exists(err);
p.should.have.property('content', "C,D");
done();
});
});
});

it('should return the model instance for upsert', function(done) {
Post.upsert({id: post.id, title: 'T2_new', content: 'C2_new',
approved: true}, function(err, p) {
p.should.have.property('id', post.id);
p.should.have.property('title', 'T2_new');
p.should.have.property('content', 'C2_new');
p.should.have.property('approved', true);
done();
});
});

it('should return the model instance for upsert when id is not present',
function(done) {
Post.upsert({title: 'T2_new', content: 'C2_new', approved: true},
function(err, p) {
p.should.have.property('id');
p.should.have.property('title', 'T2_new');
p.should.have.property('content', 'C2_new');
p.should.have.property('approved', true);
done();
});
});

it('should escape number values to defect SQL injection in findById',
function(done) {
Post.findById('(SELECT 1+1)', function(err, p) {
should.exists(err);
done();
});
});

it('should escape number values to defect SQL injection in find',
function(done) {
Post.find({where: {id: '(SELECT 1+1)'}}, function(err, p) {
should.exists(err);
done();
});
});

it('should escape number values to defect SQL injection in find with gt',
function(done) {
Post.find({where: {id: {gt: '(SELECT 1+1)'}}}, function(err, p) {
should.exists(err);
done();
});
});

it('should escape number values to defect SQL injection in find',
function(done) {
Post.find({limit: '(SELECT 1+1)'}, function(err, p) {
should.exists(err);
done();
});
});

it('should escape number values to defect SQL injection in find with inq',
function(done) {
Post.find({where: {id: {inq: ['(SELECT 1+1)']}}}, function(err, p) {
should.exists(err);
done();
});
});

});