Skip to content

Commit 4bc15ac

Browse files
c-wc-w
committed
Replace escapeSql with pg built-in escaping
In CatalystCode/project-fortis-pipeline#222 we discovered another set of SQL-injection attack vectors for the feature service. This commit fixes potential injection vulnerabilities once and for all by switching all queries to using the postgres built-in parameter interpolation mechanism instead of using string queries. The changes were validated by calling the following routes which should be roughly representative of the entire functionality space of the featureService: ```sh curl 'localhost:8080/features/name/paris' curl 'localhost:8080/features/name/bogota,paris' curl 'localhost:8080/features/id/wof-85971971' curl 'localhost:8080/features/id/wof-404477281,wof-85971971' curl 'localhost:8080/features/point/40.71/74.0' curl 'localhost:8080/features/bbox/47/5/35/10' curl 'localhost:8080/features/bbox/47/5/35/10?filter_name=Saint&filter_layer=campus' ```
1 parent 17e6044 commit 4bc15ac

File tree

3 files changed

+93
-58
lines changed

3 files changed

+93
-58
lines changed

‎services/features.js‎

Lines changed: 63 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -6,17 +6,16 @@ const async = require('async'),
66
GeoPoint = require('geopoint'),
77
HttpStatus = require('http-status-codes'),
88
postgres = require('./postgres'),
9-
escapeSql = postgres.escapeSql,
109
ServiceError = common.utils.ServiceError,
1110
turf = require('turf');
1211

1312
let featureDatabasePool;
1413

15-
function executeQuery(query, callback) {
14+
function executeQuery(query, params, callback) {
1615
featureDatabasePool.connect((err, client, done) => {
1716
if (err) return callback(err);
1817

19-
client.query(query, (err, results) => {
18+
client.query(query, params, (err, results) => {
2019
done();
2120

2221
if (err)
@@ -29,8 +28,15 @@ function executeQuery(query, callback) {
2928

3029
function getById(query, callback) {
3130
const ids = query.id.constructor === Array ? query.id : [query.id];
32-
const getQuery = `SELECT ${buildQueryColumns(query)} FROM features WHERE id IN (${ids.map(escapeSql).join(',')})`;
33-
executeQuery(getQuery, (err, rows) => {
31+
32+
const getParams = [];
33+
34+
const getQuery = `SELECT ${buildQueryColumns(query)} FROM features WHERE id IN (${ids.map(id => {
35+
getParams.push(id);
36+
return `$${getParams.length}`;
37+
}).join(',')})`;
38+
39+
executeQuery(getQuery, getParams, (err, rows) => {
3440
if (err) return callback(err);
3541
if (!rows || rows.length === 0) return callback(null, null);
3642

@@ -119,58 +125,72 @@ function buildQueryColumns(query) {
119125
return queryColumns;
120126
}
121127

122-
function addQueryPredicates(sql, query) {
128+
function addQueryPredicates(sql, query, params) {
123129
if (query.layer) {
124-
sql += ` AND layer = ${escapeSql(query.layer)}`;
130+
params.push(query.layer);
131+
sql += ` AND layer = $${params.length}`;
125132
}
126133

127134
if (query.filter_name) {
128-
sql += ` AND strpos(lower(name), lower(${escapeSql(query.filter_name)})) > 0`;
135+
params.push(query.filter_name);
136+
sql += ` AND strpos(lower(name), lower($${params.length})) > 0`;
129137
}
130138

131139
if (query.filter_namespace) {
132-
sql += ` AND lower(split_part(id, '-', 1)) = lower(${escapeSql(query.filter_namespace)})`;
140+
params.push(query.filter_namespace);
141+
sql += ` AND lower(split_part(id, '-', 1)) = lower($${params.length})`;
133142
}
134143

135144
if (query.filter_layer) {
136-
sql += ` AND lower(layer) IN (${query.filter_layer.split(',').map(layer => `lower(${escapeSql(layer)})`).join(',')})`;
145+
sql += ` AND lower(layer) IN (${query.filter_layer.split(',').map(layer => {
146+
params.push(layer);
147+
return `lower($${params.length})`
148+
}).join(',')})`;
137149
}
138150

139151
return sql;
140152
}
141153

142154
function getByBoundingBox(query, callback) {
143155
let boundingBoxQuery = `SELECT ${buildQueryColumns(query)} FROM features WHERE ST_Intersects(hull, ST_MakeEnvelope(
144-
${query.west}, ${query.south},
145-
${query.east}, ${query.north}, 4326
156+
$1, $2,
157+
$3, $4, 4326
146158
))`;
147159

148-
boundingBoxQuery = addQueryPredicates(boundingBoxQuery, query);
160+
const boundingBoxParams = [query.west, query.south, query.east, query.north];
149161

150-
return executeQuery(boundingBoxQuery, callback);
162+
boundingBoxQuery = addQueryPredicates(boundingBoxQuery, query, boundingBoxParams);
163+
164+
return executeQuery(boundingBoxQuery, boundingBoxParams, callback);
151165
}
152166

153167
function getByPoint(query, callback) {
154-
let pointQuery = `SELECT ${buildQueryColumns(query)} FROM features WHERE ST_Contains(hull, ST_GeomFromText(
155-
'POINT(${query.longitude} ${query.latitude})', 4326)
168+
let pointQuery = `SELECT ${buildQueryColumns(query)} FROM features WHERE ST_Contains(hull,
169+
ST_SetSRID(ST_MakePoint($1, $2), 4326)
156170
)`;
157171

158-
pointQuery = addQueryPredicates(pointQuery, query);
172+
const pointParams = [query.longitude, query.latitude];
173+
174+
pointQuery = addQueryPredicates(pointQuery, query, pointParams);
159175

160-
return executeQuery(pointQuery, callback);
176+
return executeQuery(pointQuery, pointParams, callback);
161177
}
162178

163179
function getByName(query, callback) {
164180
const names = query.name.constructor === Array ? query.name : [query.name];
165181

166-
let namesDisjunction = `(${names.map(function(name) {
167-
return `lower(name) = lower(${escapeSql(name)})`;
182+
const nameParams = [];
183+
184+
const namesDisjunction = `(${names.map(name => {
185+
nameParams.push(name);
186+
return `lower(name) = lower($${nameParams.length})`;
168187
}).join(" OR ")})`;
188+
169189
let nameQuery = `SELECT ${buildQueryColumns(query)} FROM features WHERE ${namesDisjunction}`;
170190

171-
nameQuery = addQueryPredicates(nameQuery, query);
191+
nameQuery = addQueryPredicates(nameQuery, query, nameParams);
172192

173-
executeQuery(nameQuery, callback);
193+
executeQuery(nameQuery, nameParams, callback);
174194
}
175195

176196
function init(callback) {
@@ -228,29 +248,39 @@ function upsert(feature, callback) {
228248
feature.elevation = feature.elevation || 'null';
229249
feature.hierarchy = feature.hierarchy || '{}';
230250

231-
let upsertQuery = `INSERT INTO features (
251+
const upsertQuery = `
252+
INSERT INTO features (
232253
id, name, layer, properties, hull, created_at, updated_at
233254
) VALUES (
234-
${escapeSql(feature.id)},
235-
${escapeSql(feature.name)},
236-
${escapeSql(feature.layer)},
237-
${escapeSql(JSON.stringify(feature.properties))},
255+
$1,
256+
$2,
257+
$3,
258+
$4,
238259
239-
ST_SetSRID(ST_GeomFromGeoJSON(${escapeSql(JSON.stringify(feature.hull))}), 4326),
260+
ST_SetSRID(ST_GeomFromGeoJSON($5), 4326),
240261
241262
current_timestamp,
242263
current_timestamp
243264
) ON CONFLICT (id) DO UPDATE SET
244-
name = ${escapeSql(feature.name)},
245-
layer = ${escapeSql(feature.layer)},
246-
properties = ${escapeSql(JSON.stringify(feature.properties))},
265+
name = $6,
266+
layer = $7,
267+
properties = $8,
247268
248-
hull = ST_SetSRID(ST_GeomFromGeoJSON(${escapeSql(JSON.stringify(feature.hull))}), 4326),
269+
hull = ST_SetSRID(ST_GeomFromGeoJSON($9), 4326),
249270
250271
updated_at = current_timestamp
251272
;`;
252273

253-
executeQuery(upsertQuery, callback);
274+
const hullJson = JSON.stringify(feature.hull);
275+
const propertiesJson = JSON.stringify(feature.properties);
276+
277+
const upsertParams = [
278+
feature.id, feature.name, feature.layer, propertiesJson,
279+
hullJson, feature.name, feature.layer, propertiesJson,
280+
hullJson,
281+
];
282+
283+
executeQuery(upsertQuery, upsertParams, callback);
254284
}
255285

256286
module.exports = {

‎services/postgres.js‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,6 @@ const pg = require('pg'),
33
querystring = require('querystring'),
44
url = require('url');
55

6-
function escapeSql(value) {
7-
return `'${value.replace(/'/g,"''")}'`;
8-
}
9-
106
function init(callback) {
117
const connectionString = process.env.FEATURES_CONNECTION_STRING;
128

@@ -34,6 +30,5 @@ function init(callback) {
3430
}
3531

3632
module.exports = {
37-
escapeSql: escapeSql,
3833
init: init
3934
};

‎services/visits.js‎

Lines changed: 30 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ const async = require('async'),
66
HttpStatus = require('http-status-codes'),
77
log = common.services.log("featureService/services/visits"),
88
postgres = require('./postgres'),
9-
escapeSql = postgres.escapeSql,
109
ServiceError = common.utils.ServiceError,
1110
uuid = require('uuid/v4');
1211

@@ -41,14 +40,17 @@ function resultsToVisits(results) {
4140
}
4241

4342
function deleteByUserId(userId, callback) {
44-
executeQuery(`DELETE FROM visits WHERE user_id=${escapeSql(userId)}`, callback);
43+
const query = `DELETE FROM visits WHERE user_id = $1`;
44+
const params = [userId];
45+
46+
executeQuery(query, params, callback);
4547
}
4648

47-
function executeQuery(query, callback) {
49+
function executeQuery(query, params, callback) {
4850
featureTablePool.connect((err, client, done) => {
4951
if (err) return callback(err);
5052

51-
client.query(query, (err, results) => {
53+
client.query(query, params, (err, results) => {
5254
done();
5355

5456
if (err)
@@ -70,12 +72,17 @@ function fromRequest(visitsJson, callback) {
7072
}
7173

7274
function getByTimestamp(userId, timestamp, callback) {
73-
executeQuery(`SELECT * FROM visits WHERE user_id=${escapeSql(userId)} AND start >= ${timestamp} AND finish <= ${timestamp}`, callback);
75+
const query = `SELECT * FROM visits WHERE user_id = $1 AND start >= $2 AND finish <= $3`;
76+
const params = [userId, timestamp, timestamp];
77+
78+
executeQuery(query, params, callback);
7479
}
7580

7681
function getByUserId(userId, callback) {
77-
let query = `SELECT * FROM visits WHERE user_id=${escapeSql(userId)}`;
78-
executeQuery(query, callback);
82+
const query = `SELECT * FROM visits WHERE user_id = $1`;
83+
const params = [userId];
84+
85+
executeQuery(query, params, callback);
7986
}
8087

8188
function init(callback) {
@@ -106,8 +113,6 @@ function toResponse(visits) {
106113
}
107114

108115
function upsert(visits, callback) {
109-
let prefix = "";
110-
111116
visits.forEach(visit => {
112117
if (!visit.id) return callback(new ServiceError(HttpStatus.BAD_REQUEST, "'id' not provided for visit: " + JSON.stringify(visit, null ,2)));
113118
if (!visit.userId) return callback(new ServiceError(HttpStatus.BAD_REQUEST, "'userId' not provided for visit: " + JSON.stringify(visit, null ,2)));
@@ -117,25 +122,30 @@ function upsert(visits, callback) {
117122
});
118123

119124
async.each(visits, (visit, visitCallback) => {
120-
let upsertQuery = `INSERT INTO visits (
125+
const upsertQuery = `INSERT INTO visits (
121126
id, user_id, feature_id, start, finish, created_at, updated_at
122127
) VALUES (
123-
${escapeSql(visit.id)},
124-
${escapeSql(visit.userId)},
125-
${escapeSql(visit.featureId)},
126-
${visit.start},
127-
${visit.finish},
128+
$1,
129+
$2,
130+
$3,
131+
$4,
132+
$5,
128133
current_timestamp,
129134
current_timestamp
130135
) ON CONFLICT (id) DO UPDATE SET
131-
user_id = ${escapeSql(visit.userId)},
132-
feature_id = ${escapeSql(visit.featureId)},
133-
start = ${visit.start},
134-
finish = ${visit.finish},
136+
user_id = $6,
137+
feature_id = $7,
138+
start = $8,
139+
finish = $9,
135140
updated_at = current_timestamp
136141
;`;
137142

138-
executeQuery(upsertQuery, visitCallback);
143+
const upsertParams = [
144+
visit.id, visit.userId, visit.featureId, visit.start, visit.finish,
145+
visit.userId, visit.featureId, visit.start, visit.finish,
146+
];
147+
148+
executeQuery(upsertQuery, upsertParams, visitCallback);
139149
}, callback);
140150
}
141151

0 commit comments

Comments
 (0)