Fix SQL Injection Vulnerabilities (#134)

* avoid sql injections

* linter

* fix test using random sid

* add some test cases

* remove tests that don't use the new validation

* add test

* linter

* fix tests

* add test

---------

Co-authored-by: Guilherme Rauen <g.rauen@cognigy.com>

lib/models/model.js

@@ -107,7 +107,7 @@ class Model extends Emitter {
       if (pk.name in obj) throw new DbErrorBadRequest(`primary key ${pk.name} is immutable`);
       getMysqlConnection((err, conn) => {
         if (err) return reject(err);
-        conn.query(`UPDATE ${this.table} SET ? WHERE ${pk.name} = '${sid}'`, obj, (err, results, fields) => {
+        conn.query(`UPDATE ${this.table} SET ? WHERE ${pk.name} = ?`, [obj, sid], (err, results, fields) => {
           conn.release();
           if (err) return reject(err);
           resolve(results.affectedRows);

lib/routes/api/accounts.js

@@ -46,17 +46,34 @@ const stripPort = (hostport) => {
 };
 
 const validateUpdateForCarrier = async(req) => {
-  const account_sid = parseAccountSid(req);
-  if (req.user.hasScope('admin')) return ;
-  if (req.user.hasScope('account')) {
-    if (account_sid === req.user.account_sid) return ;
-    throw new DbErrorForbidden('insufficient permissions to update account');
-  }
-  if (req.user.hasScope('service_provider')) {
-    const [r] = await promisePool.execute(
-      'SELECT service_provider_sid from accounts WHERE account_sid = ?', [account_sid]);
-    if (r.length === 1 && r[0].service_provider_sid === req.user.service_provider_sid) return;
-    throw new DbErrorForbidden('insufficient permissions to update account');
+  try {
+    const account_sid = parseAccountSid(req);
+
+    if (req.user.hasScope('admin')) {
+      return;
+    }
+
+    if (req.user.hasScope('account')) {
+      if (account_sid === req.user.account_sid) {
+        return;
+      }
+
+      throw new DbErrorForbidden('insufficient permissions to update account');
+    }
+
+    if (req.user.hasScope('service_provider')) {
+      const [r] = await promisePool.execute(
+        'SELECT service_provider_sid from accounts WHERE account_sid = ?', [account_sid]
+      );
+
+      if (r.length === 1 && r[0].service_provider_sid === req.user.service_provider_sid) {
+        return;
+      }
+
+      throw new DbErrorForbidden('insufficient permissions to update account');
+    }
+  } catch (error) {
+    throw error;
   }
 };
 
@@ -785,11 +802,11 @@ router.put('/:sid/Calls/:callSid', async(req, res) => {
  * create a new Message
  */
 router.post('/:sid/Messages', async(req, res) => {
-  const account_sid = parseAccountSid(req);
-  const setName = `${(process.env.JAMBONES_CLUSTER_ID || 'default')}:active-fs`;
   const {retrieveSet, logger} = req.app.locals;
 
   try {
+    const account_sid = parseAccountSid(req);
+    const setName = `${(process.env.JAMBONES_CLUSTER_ID || 'default')}:active-fs`;
     const serviceUrl = await getFsUrl(logger, retrieveSet, setName);
     if (!serviceUrl) res.json({msg: 'no available feature servers at this time'}).status(480);
     await validateCreateMessage(logger, account_sid, req);

lib/routes/api/add-from-predefined-carrier.js

@@ -24,13 +24,13 @@ router.post('/:sid', async(req, res) => {
   let service_provider_sid;
   const {account_sid} = req.user;
 
-  if (!account_sid) {
-    service_provider_sid = parseServiceProviderSid(req);
-  } else {
-    service_provider_sid = req.user.service_provider_sid;
-  }
-
   try {
+    if (!account_sid) {
+      service_provider_sid = parseServiceProviderSid(req);
+    } else {
+      service_provider_sid = req.user.service_provider_sid;
+    }
+
     const [template] = await PredefinedCarrier.retrieve(sid);
     logger.debug({template}, `Retrieved template carrier for sid ${sid}`);
     if (!template) return res.sendStatus(404);

lib/routes/api/alerts.js

@@ -1,17 +1,13 @@
 const router = require('express').Router();
 const sysError = require('../error');
 const {DbErrorBadRequest} = require('../../utils/errors');
+const { parseServiceProviderSid } = require('./utils');
 
 const parseAccountSid = (url) => {
   const arr = /Accounts\/([^\/]*)/.exec(url);
   if (arr) return arr[1];
 };
 
-const parseServiceProviderSid = (url) => {
-  const arr = /ServiceProviders\/([^\/]*)/.exec(url);
-  if (arr) return arr[1];
-};
-
 router.get('/', async(req, res) => {
   const {logger, queryAlerts, queryAlertsSP} = req.app.locals;
   try {

lib/routes/api/error.js

@@ -1,6 +1,10 @@
-const {DbErrorBadRequest, DbErrorUnprocessableRequest} = require('../../utils/errors');
+const { BadRequestError, DbErrorBadRequest, DbErrorUnprocessableRequest } = require('../../utils/errors');
 
 function sysError(logger, res, err) {
+  if (err instanceof BadRequestError) {
+    logger.info(err, err.message);
+    return res.status(400).json({msg: 'Bad request'});
+  }
   if (err instanceof DbErrorBadRequest) {
     logger.info(err, 'invalid client request');
     return res.status(400).json({msg: err.message});

lib/routes/api/limits.js

@@ -28,16 +28,18 @@ router.post('/', async(req, res) => {
     category,
     quantity
   } = req.body;
-  const account_sid = parseAccountSid(req);
-  let service_provider_sid;
-  if (!account_sid) {
-    if (!req.user.hasServiceProviderAuth && !req.user.hasAdminAuth) {
-      logger.error('POST /SpeechCredentials invalid credentials');
-      return res.sendStatus(403);
-    }
-    service_provider_sid = parseServiceProviderSid(req);
-  }
+
   try {
+    let service_provider_sid;
+    const account_sid = parseAccountSid(req);
+    if (!account_sid) {
+      if (!req.user.hasServiceProviderAuth && !req.user.hasAdminAuth) {
+        logger.error('POST /SpeechCredentials invalid credentials');
+        return res.sendStatus(403);
+      }
+      service_provider_sid = parseServiceProviderSid(req);
+    }
+
     let uuid;
     if (account_sid) {
       const existing = (await AccountLimits.retrieve(account_sid) || [])
@@ -80,10 +82,11 @@ router.post('/', async(req, res) => {
  */
 router.get('/', async(req, res) => {
   let service_provider_sid;
-  const account_sid = parseAccountSid(req);
-  if (!account_sid) service_provider_sid = parseServiceProviderSid(req);
   const logger = req.app.locals.logger;
+
   try {
+    const account_sid = parseAccountSid(req);
+    if (!account_sid) service_provider_sid = parseServiceProviderSid(req);
     const limits = account_sid ?
       await AccountLimits.retrieve(account_sid) :
       await ServiceProviderLimits.retrieve(service_provider_sid);
@@ -99,10 +102,11 @@ router.get('/', async(req, res) => {
 
 router.delete('/', async(req, res) => {
   const logger = req.app.locals.logger;
-  const account_sid = parseAccountSid(req);
-  const {category} = req.query;
-  const service_provider_sid = parseServiceProviderSid(req);
+
   try {
+    const account_sid = parseAccountSid(req);
+    const {category} = req.query;
+    const service_provider_sid = parseServiceProviderSid(req);
     if (account_sid) {
       if (category) {
         await promisePool.execute(sqlDeleteAccountLimitsByCategory, [account_sid, category]);

lib/routes/api/service-providers.js

@@ -35,27 +35,47 @@ function validateAdd(req) {
 }
 
 async function validateRetrieve(req) {
-  const service_provider_sid = parseServiceProviderSid(req);
-  if (req.user.hasScope('admin')) return ;
-  if (req.user.hasScope('service_provider')) {
-    if (service_provider_sid === req.user.service_provider_sid) return ;
+  try {
+    const service_provider_sid = parseServiceProviderSid(req);
+
+    if (req.user.hasScope('admin')) {
+      return;
+    }
+
+    if (req.user.hasScope('service_provider')) {
+      if (service_provider_sid === req.user.service_provider_sid) return ;
+    }
+
+    if (req.user.hasScope('account')) {
+      /* allow account users to retrieve service provider data from parent SP */
+      const sid = req.user.account_sid;
+      const [r] = await promisePool.execute('SELECT service_provider_sid from accounts WHERE account_sid = ?', [sid]);
+      if (r.length === 1 && r[0].service_provider_sid === req.user.service_provider_sid) return;
+    }
+
+    throw new DbErrorForbidden('insufficient permissions to update service provider');
+  } catch (error) {
+    throw error;
   }
-  if (req.user.hasScope('account')) {
-    /* allow account users to retrieve service provider data from parent SP */
-    const sid = req.user.account_sid;
-    const [r] = await promisePool.execute('SELECT service_provider_sid from accounts WHERE account_sid = ?', [sid]);
-    if (r.length === 1 && r[0].service_provider_sid === req.user.service_provider_sid) return;
-  }
-  throw new DbErrorForbidden('insufficient permissions to update service provider');
 }
 
 function validateUpdate(req) {
-  if (req.user.hasScope('admin')) return ;
-  if (req.user.hasScope('service_provider')) {
+  try {
     const service_provider_sid = parseServiceProviderSid(req);
-    if (service_provider_sid === req.user.service_provider_sid) return ;
+
+    if (req.user.hasScope('admin')) {
+      return;
+    }
+
+    if (req.user.hasScope('service_provider')) {
+      if (service_provider_sid === req.user.service_provider_sid) return;
+    }
+
+    throw new DbErrorForbidden('insufficient permissions to update service provider');
+  } catch (error) {
+    console.log('Passing forward the error received');
+    throw error;
   }
-  throw new DbErrorForbidden('insufficient permissions to update service provider');
 }
 
 /* can not delete a service provider if it has any active accounts */
@@ -191,7 +211,6 @@ router.post('/', async(req, res) => {
   const logger = req.app.locals.logger;
   try {
     validateAdd(req);
-
     // create webhooks if provided
     const obj = Object.assign({}, req.body);
     for (const prop of ['registration_hook']) {
@@ -212,7 +231,6 @@ router.post('/', async(req, res) => {
 /* list */
 router.get('/', async(req, res) => {
   const logger = req.app.locals.logger;
-
   try {
     const results = await ServiceProvider.retrieveAll();
     logger.debug({results, user: req.user}, 'ServiceProvider.retrieveAll');
@@ -242,10 +260,10 @@ router.get('/:sid', async(req, res) => {
 
 /* update */
 router.put('/:sid', async(req, res) => {
-  const sid = req.params.sid;
   const logger = req.app.locals.logger;
   try {
     validateUpdate(req);
+    const sid = req.params.sid;
 
     // create webhooks if provided
     const obj = Object.assign({}, req.body);
@@ -255,15 +273,14 @@ router.put('/:sid', async(req, res) => {
           const sid = obj[prop]['webhook_sid'];
           delete obj[prop]['webhook_sid'];
           await Webhook.update(sid, obj[prop]);
-        }
-        else {
+        } else {
           const sid = await Webhook.make(obj[prop]);
           obj[`${prop}_sid`] = sid;
         }
-      }
-      else {
+      } else {
         obj[`${prop}_sid`] = null;
       }
+
       delete obj[prop];
     }
 
@@ -271,6 +288,7 @@ router.put('/:sid', async(req, res) => {
     if (rowsAffected === 0) {
       return res.status(404).end();
     }
+
     res.status(204).end();
   } catch (err) {
     sysError(logger, res, err);

lib/routes/api/speech-credentials.js

@@ -135,21 +135,24 @@ const encryptCredential = (obj) => {
 
 router.post('/', async(req, res) => {
   const logger = req.app.locals.logger;
-  const {
-    use_for_stt,
-    use_for_tts,
-    vendor,
-  } = req.body;
-  const account_sid = req.user.account_sid || req.body.account_sid;
-  const service_provider_sid = req.user.service_provider_sid ||
-  req.body.service_provider_sid || parseServiceProviderSid(req);
-  if (!account_sid) {
-    if (!req.user.hasServiceProviderAuth && !req.user.hasAdminAuth) {
-      logger.error('POST /SpeechCredentials invalid credentials');
-      return res.sendStatus(403);
-    }
-  }
+
   try {
+    const {
+      use_for_stt,
+      use_for_tts,
+      vendor,
+    } = req.body;
+    const account_sid = req.user.account_sid || req.body.account_sid;
+    const service_provider_sid = req.user.service_provider_sid ||
+    req.body.service_provider_sid || parseServiceProviderSid(req);
+
+    if (!account_sid) {
+      if (!req.user.hasServiceProviderAuth && !req.user.hasAdminAuth) {
+        logger.error('POST /SpeechCredentials invalid credentials');
+        return res.sendStatus(403);
+      }
+    }
+
     const encrypted_credential = encryptCredential(req.body);
     const uuid = await SpeechCredential.make({
       account_sid,
@@ -169,10 +172,11 @@ router.post('/', async(req, res) => {
  * retrieve all speech credentials for an account
  */
 router.get('/', async(req, res) => {
-  const account_sid = parseAccountSid(req) || req.user.account_sid;
-  const service_provider_sid = parseServiceProviderSid(req) || req.user.service_provider_sid;
   const logger = req.app.locals.logger;
+
   try {
+    const account_sid = parseAccountSid(req) || req.user.account_sid;
+    const service_provider_sid = parseServiceProviderSid(req) || req.user.service_provider_sid;
     const credsAccount = account_sid ? await SpeechCredential.retrieveAll(account_sid) : [];
     const credsSP = service_provider_sid ?
       await SpeechCredential.retrieveAllForSP(service_provider_sid) :

lib/routes/api/utils.js

@@ -1,10 +1,10 @@
-const { v4: uuid } = require('uuid');
+const { v4: uuid, validate } = require('uuid');
 const bent = require('bent');
 const Account = require('../../models/account');
 const {promisePool} = require('../../db');
 const {cancelSubscription, detachPaymentMethod} = require('../../utils/stripe-utils');
 const freePlans = require('../../utils/free_plans');
-const { DbErrorBadRequest} = require('../../utils/errors');
+const { BadRequestError, DbErrorBadRequest } = require('../../utils/errors');
 const insertAccountSubscriptionSql = `INSERT INTO account_subscriptions 
 (account_subscription_sid, account_sid) 
 values (?, ?)`;
@@ -140,37 +140,76 @@ const createTestAlerts = async(writeAlerts, AlertType, account_sid) => {
 
 const parseServiceProviderSid = (req) => {
   const arr = /ServiceProviders\/([^\/]*)/.exec(req.originalUrl);
-  if (arr) return arr[1];
+  if (arr) {
+    const sid = arr[1];
+    const sid_validation = validate(sid);
+    if (!sid_validation) {
+      throw new BadRequestError('invalid service_provider_sid format');
+    }
+
+    return arr[1];
+  }
 };
 
 const parseAccountSid = (req) => {
   const arr = /Accounts\/([^\/]*)/.exec(req.originalUrl);
-  if (arr) return arr[1];
+  if (arr) {
+    const sid = arr[1];
+    const sid_validation = validate(sid);
+    if (!sid_validation) {
+      throw new BadRequestError('invalid account_sid format');
+    }
+
+    return arr[1];
+  }
 };
 
 const hasAccountPermissions = (req, res, next) => {
-  if (req.user.hasScope('admin')) return next();
-  if (req.user.hasScope('service_provider')) return next();
-  if (req.user.hasScope('account')) {
-    const account_sid = parseAccountSid(req);
-    if (account_sid === req.user.account_sid) return next();
+  try {
+    if (req.user.hasScope('admin')) {
+      return next();
+    }
+
+    if (req.user.hasScope('service_provider')) {
+      return next();
+    }
+
+    if (req.user.hasScope('account')) {
+      const account_sid = parseAccountSid(req);
+      if (account_sid === req.user.account_sid) {
+        return next();
+      }
+    }
+
+    res.status(403).json({
+      status: 'fail',
+      message: 'insufficient privileges'
+    });
+  } catch (error) {
+    throw error;
   }
-  res.status(403).json({
-    status: 'fail',
-    message: 'insufficient privileges'
-  });
 };
 
 const hasServiceProviderPermissions = (req, res, next) => {
-  if (req.user.hasScope('admin')) return next();
-  if (req.user.hasScope('service_provider')) {
-    const service_provider_sid = parseServiceProviderSid(req);
-    if (service_provider_sid === req.user.service_provider_sid) return next();
+  try {
+    if (req.user.hasScope('admin')) {
+      return next();
+    }
+
+    if (req.user.hasScope('service_provider')) {
+      const service_provider_sid = parseServiceProviderSid(req);
+      if (service_provider_sid === req.user.service_provider_sid) {
+        return next();
+      }
+    }
+
+    res.status(403).json({
+      status: 'fail',
+      message: 'insufficient privileges'
+    });
+  } catch (error) {
+    throw error;
   }
-  res.status(403).json({
-    status: 'fail',
-    message: 'insufficient privileges'
-  });
 };
 
 const checkLimits = async(req, res, next) => {

lib/routes/error.js

@@ -1,6 +1,15 @@
-const {DbErrorBadRequest, DbErrorUnprocessableRequest, DbErrorForbidden} = require('../utils/errors');
+const {
+  BadRequestError,
+  DbErrorBadRequest,
+  DbErrorUnprocessableRequest,
+  DbErrorForbidden
+} = require('../utils/errors');
 
 function sysError(logger, res, err) {
+  if (err instanceof BadRequestError) {
+    logger.info(err, err.message);
+    return res.status(400).json({msg: 'Bad request'});
+  }
   if (err instanceof DbErrorBadRequest) {
     logger.info(err, 'invalid client request');
     return res.status(400).json({msg: err.message});

lib/utils/errors.js

@@ -1,3 +1,9 @@
+class BadRequestError extends Error {
+  constructor(msg) {
+    super(msg);
+  }
+}
+
 class DbError extends Error {
   constructor(msg) {
     super(msg);
@@ -23,6 +29,7 @@ class DbErrorForbidden extends DbError {
 }
 
 module.exports = {
+  BadRequestError,
   DbError,
   DbErrorBadRequest,
   DbErrorUnprocessableRequest,

test/accounts.js

@@ -219,6 +219,21 @@ test('account tests', async(t) => {
     });
     t.ok(result.statusCode === 201, 'successfully updated a call session limit to an account');
 
+    /* try to update an existing limit for an account giving a invalid sid */
+    try {
+      result = await request.post(`/Accounts/invalid-sid/Limits`, {
+        auth: authAdmin,
+        json: true,
+        resolveWithFullResponse: true,
+        body: {
+          category: 'voice_call_session',
+          quantity: 205
+        }
+      });
+    } catch (err) {
+      t.ok(err.statusCode === 400, 'returns 400 bad request if sid param is not a valid uuid');
+    }
+
     /* query all limits for an account */
     result = await request.get(`/Accounts/${sid}/Limits`, {
       auth: authAdmin,

test/service-providers.js

@@ -107,6 +107,20 @@ test('service provider tests', async(t) => {
     });
     t.ok(result.statusCode === 204, 'successfully updated service provider');
 
+    /* try to update service providers with invalid sid format*/
+    try {
+      result = await request.put(`/ServiceProviders/123`, {
+        auth: authAdmin,
+        json: true,
+        resolveWithFullResponse: true,
+        body: {
+          name: 'robb'
+        }
+      });
+    } catch (err) {
+      t.ok(err.statusCode === 400, 'returns 400 bad request if sid param is not a valid uuid');
+    }
+
     /* add an api key for a service provider */
     result = await request.post(`/ApiKeys`, {
       auth: authAdmin,

test/speech-credentials.js

@@ -22,6 +22,24 @@ test('speech credentials tests', async(t) => {
     const service_provider_sid = await createServiceProvider(request);
     const account_sid = await createAccount(request, service_provider_sid);
 
+    /* return 400 if invalid sid param is used */
+    try {
+      result = await request.post(`/ServiceProviders/foobarbaz/SpeechCredentials`, {
+        resolveWithFullResponse: true,
+        simple: false,
+        auth: authAdmin,
+        json: true,
+        body: {
+          vendor: 'google',
+          service_key: jsonKey,
+          use_for_tts: true,
+          use_for_stt: true
+        }
+      });
+    } catch (err) {
+      t.ok(err.statusCode === 400, 'returns 400 bad request if sid param is not a valid uuid');
+    }
+
     /* add a speech credential to a service provider */
     result = await request.post(`/ServiceProviders/${service_provider_sid}/SpeechCredentials`, {
       resolveWithFullResponse: true,
@@ -73,8 +91,8 @@ test('speech credentials tests', async(t) => {
     t.ok(result.statusCode === 201, 'successfully added speech credential');
     const sid1 = result.body.sid;
 
-    /* return 403 if invalid account is used  */
-    result = await request.post(`/Accounts/foobarbaz/SpeechCredentials`, {
+    /* return 403 if invalid account is used - randomSid: bed7ae17-f8b4-4b74-9e5b-4f6318aae9c9 */
+    result = await request.post(`/Accounts/bed7ae17-f8b4-4b74-9e5b-4f6318aae9c9/SpeechCredentials`, {
       resolveWithFullResponse: true,
       simple: false,
       auth: authUser,