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?

9
  • 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 Commented Jan 29, 2021 at 1:56
  • I don't see what's the difference between your second example and the third example. Aren't they the same? Commented Jan 29, 2021 at 1: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? Commented Jan 29, 2021 at 1:58
  • 1
    I would also recommend using github.com/sidorares/node-mysql2 as it has support for native prepared statements. Commented Jan 29, 2021 at 2:04
  • 1
    If you want to use multipleStatements: true then make sure to sanitize your input. using joi module to sanitize your input Commented Apr 12, 2021 at 16:11

2 Answers 2

2
+100

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).

Sign up to request clarification or add additional context in comments.

3 Comments

I'm having difficulty in understanding the method to "issue the statement one at a time". Does my last code (in "Update 2") example still not doing it?
@FaNo_FN - Update 2 seems safe.
got it Rick!.. thanks. I'm changing all my codes to follow the structure now.
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.

3 Comments

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!
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: serverfault.com/questions/989076/…
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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.