How safe is this generated query from SQL injection?

Question

I am trying to make a search bar which works with multiple words, but I am worried about SQL injection.

I am using node express with the npm mssql package.

Here's the code which gets the criteria, generates the SQL and runs it:

router
.get('/search/:criteria', function (req, res) {
    var criteria = req.params.criteria;
    var words = criteria.split(" ");

    var x = ""
    words.map(word => x += `name like '%${word}%' and `);
    x = x.substring(0, x.length - 5); // Remove trailing 'and'

    var query = `SELECT * FROM table WHERE ${x}`

    new sql.ConnectionPool(db).connect().then(pool => {
        return pool.request().query(query)
    }).then(result => {

    })
});

A search for something to search would result in this query:

SELECT * FROM table 
WHERE 
    name like '%something%'
    and name like '%to%'
    and name like '%search%'

I tried some SQL injections myself, but none of them seem to work.

Note: I am aware that we should always use inputs for this. It works fine for one word, but I don't know how to use inputs for many words. Ex:

new sql.ConnectionPool(db).connect().then(pool => {
        return pool.request()
        .input('input', '%'+criteria+'%')
        .query(query)
    })

What exactly is your question? If all you're asking for is a code review I would like to refer you to https://codereview.stackexchange.com/ — InzeNL, May 27 '18 at 13:44
@InzeNL I'd like to know how an SQL injection would be made, or if it's even possible. I don't think I'm looking for a code review. — Ivan, May 27 '18 at 14:03
Anything where you're blindly going to append user input to a string and execute it is vulnerable to SQL injection (e.g. user passes in `%search'' OR 1=1--`). I don't know if your framework supports complex data structures like DataTable / table-valued parameters but if you're concerned about SQL injection that may be a change you'll want to make. — Aaron Bertrand, May 27 '18 at 14:23

Tomalak · Accepted Answer · 2018-05-27T14:53:27.897

The answer is: It's not safe. Your code does exactly nothing to make it safe, either. Don't build SQL by concatenating/interpolating user-supplied data into the statement.

In addition, you don't do any escaping for LIKE itself, either, so that is just as unclean.

If you need dynamic SQL, build a prepared SQL statement with the expected number of placeholders and then bind user-supplied values to those placeholders.

router.get('/search/:criteria', (req, res) => {
    const ps = new sql.PreparedStatement();
    const sqlConditions = [];
    const escapedValues = {};

    // set up escaped values, safe SQL bits, PS parameters
    req.params.criteria.split(" ").forEach((v, i) => {
        const paramName = 'val' + i;
        escapedValues[paramName] = v.replace(/[\\%_]/g, '\\$&');
        sqlConditions.push(`name LIKE '%' + @${paramName} + '%' ESCAPE '\'`);
        ps.input(paramName, sql.VarChar);
    });

    // build safe SQL string, prepare statement
    const sql = 'SELECT * FROM table WHERE ' + sqlConditions.join(' AND '); 
    ps.prepare(sql);

    // connect, execute, return
    ps.execute(escapedValues).then(result => {
        res(result)
    });
});

(Disclaimer: code is untested, as I have no SQL Server available right now, but you get the idea.)

Thank you for the amazing explanation! Genius! – Ivan May 27 '18 at 16:43 — Ivan, May 27 '18 at 16:43

How safe is this generated query from SQL injection?

1 Answers1