1

I've been developing an "Employee leave management" web app project for our internal use using node.js with express and ejs template. Now, my employer wants me to make the app accessible through internet and I'm worried about SQL injection.

Let's say I have a button like this in html:

<a href="/edit/<%= ReqID %>">Edit</a>

This will GET from index.js file:

const { edit } = require("./request");
app.get("/edit/:ReqID", edit);

This will then go to module edit in request.js file:

module.exports = {
        edit: (req, res) => {
        let ReqID= req.params.ReqID;

        let squery = `SELECT * FROM table1 WHERE ReqID="${ReqID}";

                      SELECT * FROM table2 WHERE ReqID="${ReqID}";`;

        db.query(squery, function (err, result) {
            if (err) {
                return res.status(500).send(err);
            }
            res.render("edit.ejs", {
                srecords1: result[0],
                srecords2: result[1]
            })
        })
    }
}

There might be two or more queries in there and I'm using mysql driver for node.js with multipleStatements: true and I'm aware of warning "Support for multiple statements is disabled for security reasons (it allows for SQL injection attacks if values are not properly escaped)." This will return something like http://localhost:port/edit/reqid on the browser address box. I saw a video from youtube that says SQL Injection can be done through the browser's address box like http://localhost:port/edit/reqid;";SELECT * FROM users; so I did that and for sure I can see that syntax being send to the server. So I follow the suggestion in the video to do a placeholder like this:

module.exports = {
        edit: (req, res) => {
        let ReqID= req.params.ReqID;

        let squery = `SELECT * FROM table1 WHERE ReqID= ?;

                      SELECT * FROM table2 WHERE ReqID= ?;`;

        db.query(squery, [ReqID, ReqID], function (err, result) {
            if (err) {
                return res.status(500).send(err);
            }
            res.render("edit.ejs", {
                srecords1: result[0],
                srecords2: result[1]
            })
        })
    }
}

Then I try the extreme http://localhost:port/edit/reqid;";DELETE FROM users; and http://localhost:port/edit/reqid;";DROP TABLE users; separately and it works! First it deletes data from users tble and for sure the second drop table command also worked. After the first attempt, I refresh the browser with the same sql injection syntax and I've got this message:

{"code":"ER_BAD_TABLE_ERROR","errno":1051,"sqlMessage":"Unknown table 'users'","sqlState":"42S02","index":1,"sql":"SELECT * FROM table1 WHERE ReqID= "ReqID;";drop table users;";SELECT * FROM table1 WHERE ReqID= "ReqID;";drop table users;";"}

So, the table users clearly have been dropped from the database.

Update:

I did further testing based on the information I gained from this answer and I did something like this:

module.exports = {
        edit: (req, res) => {
        let ReqID= req.params.ReqID;

        db.query(`SELECT * FROM table1 WHERE ReqID= ?; SELECT * FROM table2 WHERE ReqID= ?;` , [ReqID, ReqID], function (err, result) {
            if (err) {
                return res.status(500).send(err);
            }
            res.render("edit.ejs", {
                srecords1: result[0],
                srecords2: result[1]
            })
        })
    }
}

Then I re-test with multiple variation of http://localhost:port/edit/reqid;";DROP TABLE users; (double quote in between) http://localhost:port/edit/reqid;';DROP TABLE users; (single quote in between) etc. and it doesn't seem to be dropping the table anymore. However, I still see the statement being sent to the server so I'm still wary of the DROP syntax being effective somehow.

Update 2:

Note: Fortunately, the deployment has been delayed and I have more time to sort out the issue.

After researching for a while, taking the comments into consideration and testing multiple method, I came up with this structure:

function(req, res) { 
        let dcode = [req.body.dcode];
        let query1 =`SELECT col1, col2 FROM table1 WHERE DCode=?`;
     
        db.query(query1, dcode, function(err, result_1) {
        if (err) {
            return res.status(500).send(err);
        }

        let query2 =`SELECT col1, col2 FROM table2 WHERE DCode=?`;

        db.query(query2, dcode, function(err, result_2) {
          if (err) {
            return res.status(500).send(err);
        }
          res.render("login.ejs", {
            result1: result_1,
            result2: result_2
          });
        });
      });
    }

Which is simple enough and no major change to my current codes. Would this be sufficient to prevent SQL injection in node.js?

FanoFN
  • 6,815
  • 2
  • 13
  • 33
  • 1
    You could parse the `ReqID` to a numeric value (assuming it's numeric). I also don't see why you wouldn't make 2 requests, or use an outer join to get the data? It's really hard to say without knowing what data you are requesting – Icepickle Jan 29 '21 at 01:56
  • I don't see what's the difference between your second example and the third example. Aren't they the same? – Dharman Jan 29 '21 at 01:57
  • 1
    As for multiple statements, I really do not understand when would that ever be useful. Why not execute each statement on its own? What is the benefit of enabling multiple statements? – Dharman Jan 29 '21 at 01:58
  • 1
    I would also recommend using https://github.com/sidorares/node-mysql2 as it has support for native prepared statements. – Dharman Jan 29 '21 at 02:04
  • @Icepickle , `ReqID` is varchar. I guess I haven't though about it. It does make sense just to join the query instead of running multiple statement. – FanoFN Jan 29 '21 at 03:11
  • @Dharman , I also think it's the same but as I was doing testing, with the 2nd example I still able to drop a table by appending `DROP TABLE` statement on the browser but with the third structure, I've tried multiple variant of appending `DROP TABLE` and I haven't once manage to drop any table. – FanoFN Jan 29 '21 at 03:14
  • I'm sorry but my knowledge in these things are very elementary, I just picked up node.js a year ago and I just "crash course" it. I'll follow both of you advice by doing execute each statement on it's own and switch to `mysql2`. At this point, I don't think there's anything else I can do until the app goes live by the next two weeks. Appreciate your insights. – FanoFN Jan 29 '21 at 03:23
  • 1
    If you want to use `multipleStatements: true` then make sure to sanitize your input. using `joi` module to sanitize your input – aRvi Apr 12 '21 at 16:11
  • I'm actually opt not to use `multipleStatements: true` any longer.. Nonetheless, I'll take a look at `Joi`. Maybe I can use it for any future projects. Thanks for the suggestion @aRvi – FanoFN Apr 13 '21 at 00:30

2 Answers2

2

Allowing multi-statement strings, itself, invites SQL injection. So, avoid it.

Plan A:

Consider ending an array (perhaps in JSON) to the server; let it then execute each statement, and return an array of resultsets.

But it would be simpler to simply issue the statements one at a time.

(If the client and server are far apart, the one-statement-at-a-time method may cause a noticeable latency.)

Plan B:

Build suitable Stored procedures for any multi-statement needs. This, where practical, avoids multi-statement calls. And avoids latency issues (usually).

Rick James
  • 135,179
  • 13
  • 127
  • 222
2

Here are a few suggestions that might help:

  1. Never use template strings like this: Select * from table where id = ${value}. SQL injections will happen - 100%!. Instead you should use build in driver defense mechanism. Like this: query('Select * from table where id = ?', [value]). This should prevent SQL injection.
  2. Use single statements per query. If you need to do multiple operations in one request to database - consider creating stored procedure. Stored procedures also have build in security mechanism.
  3. Consider using query builder or ORM. They also have additional layer of security on top of build in driver one.
  4. You could also explicitly escape SQL string with help of 3rd party library.
Serhiy Mamedov
  • 1,080
  • 5
  • 11
  • Yeah, I've tested myself with (1). Now I'm doing (2) just like my last **Update 2**. I think the app is too large for me to use query builder or ORM at this point; it's going to be like starting from scratch. But it's something I'll keep in mind future projects (hopefully). Thanks for the advices! – FanoFN Apr 14 '21 at 08:32
  • 1
    If it is a large, live, existing project with many potential issues you can consider adding filtering of requests at the server, proxy, or firewall level. Note that this is not a "proper" solution, but a temporary workaround to patch things up while you manually fix issues in the code (as described in other answers). Some ideas: https://serverfault.com/questions/989076/how-to-protect-apache-server-from-sql-injection – Aron Apr 14 '21 at 13:40
  • That would be something to consider. If I calculate my remaining time before its going live, I think I still have ample time to sort out the codes but I'll keep your advise as my first option if somehow I can't keep up with the deadline @Aron – FanoFN Apr 15 '21 at 00:27