Is this javascript vulnerable to SQL Injection?

Question

I am using node-postgres library.

  const sql = `
  SELECT * 
  FROM "Employees"
  where employee_id = '${employee_id}' ; 
  `;
  console.log(`Query formatted: ${sql}`);
  const result = await this.db.run(sql)

// DB Run method looks like this

async run(sql) {
    let retVal = "";
    let client;
    try {
      await this.init();
      console.log(`Connecting to ${this.connection.host}`);
      client = new pg.Client(this.connection);
      await client.connect();
      console.log(`inner sql:  ${sql}`);
      const res = await client.query(sql);
      
      retVal = res.rows;
 
      client.end();
    } catch (e) {
      console.log(`ERROR: ${e}`);
      retVal = e;
      client.end();
    }
    return retVal;
  }

employee_id is passed via user input as POST call.

SQLMAP tells me this is vulnerable but I tried different inputs like

employee_id = "123'; SELECT * from employees;'"

But it seems to always execute the queries together telling me No results found.

I will be parametrizing the queries but wondering what the current vulnerability level is?
And for parametrizing how would I go about it if I have the same 3 statements above in a lot of different places in my code? Each query is shaped differently so not quite sure I can move them to a common method. Instead I will have to refactor all places?

It is highly likely vulnerable, but your example uses a different variable name than your template string does; I wouldn’t expect it to affect the query’s execution if that’s what you truly tested with. If that’s *not* what you tested with, I would recommend updating your example to show *exactly* what you did. It’s also not clear if your `run()` abstraction passes this query to anything that would allow for multiple queries for a single query text, which would preclude your test string from demonstrating if it was indeed exploitable. — esqew, Aug 29 '23 at 22:45
Sorry. updated. That was not a defined variable but I passed that variable in through an API JSON call. — Ace McCloud, Aug 29 '23 at 22:47
Yes, this is vulnerable to SQL injection. As you described, `employee_id` comes from user input. That user input is then used as (part of) SQL code, which is passed to a function that blindly executes whatever code it's given. *"But it seems to always execute the queries together"* - If it's executing both queries then that is evidence of SQL injection. Fortunately a simple `SELECT` query is less likely to damage anything. But I wouldn't rely on malicious users only sending non-malicious code. — David, Aug 29 '23 at 22:55
Does this answer your question? [Prevent SQL Injection with Nodejs and Postgres](https://stackoverflow.com/questions/58174695/prevent-sql-injection-with-nodejs-and-postgres) — David, Aug 29 '23 at 22:58
String concatenation is never safe, it can't be: It just generates a string. And then you execute this string as a piece of SQL, a collection of database commands, in your database. Whatever that command may be... — Frank Heikens, Aug 29 '23 at 23:10
"*telling me No results found*" - that does not mean it's safe. Just pass something like `employee_id = "0'; UPDATE employee SET role = 'admin' WHERE employee_id = '123"` to see the danger. — Bergi, Aug 30 '23 at 00:43
"*how would I go about it if I have the same 3 statements above in a lot of different places in my code?*" - that does not seem related to your question about how to fix the sqli vulnerability via parameterised queries. Parameterise the queries in all places where you use them. If you have the *same* query in multiple places, and don't know how to refactor this to avoid duplicated code, you can [ask a separate question](/questions/ask) about that. Please post the code of all the three places there. — Bergi, Aug 30 '23 at 00:46

synnøve · Accepted Answer · 2023-08-30T22:48:37.927

-1

JavaScript doesn't sanitise inputs automatically and neither does Node, so this is absolutely vulnerable.

See https://github.com/mysqljs/mysql#escaping-query-values (first answer from Preventing SQL injection in Node.js for more)

edited Aug 30 '23 at 22:48

answered Aug 29 '23 at 23:33

synnøve

47
5

https://stackoverflow.com/questions/22310812/how-do-prepared-statements-prevent-sql-injection-better-than-statements – Jared Smith Aug 29 '23 at 23:42

Is this javascript vulnerable to SQL Injection?

1 Answers1