How to call a stored procedure without being open to SQL Injection

Question

I have a .net Core API that I need to call a stored procedure from. I have the following code but I am told that there is a risk of SQL injection with it. I am not experienced enough to be able to identify where that threat is. How could I call this procedure in a more secure way?

[HttpPut]
[Route("move")]
public IActionResult Move(string id, string dir)
{
    if (id == null || dir == null)
    {
        return BadRequest();
    }

    try
    {
        // todo: sql injection threat
        _db.Database.ExecuteSqlCommand($"Exec ProcMove @ParaId = {id}, @Direction = {dir}");
        _db.SaveChanges();

        return Ok();
    }
    catch (Exception)
    {
        return BadRequest();
    }
}

I am using Angular 5 for my front end and am calling the endpoint with

http.put("http://localhost:5000/api/move?id={id}&dir={dir}");

Answer 1

There are a couple of issues here. The first, lower hanging fruit is that the input is not validated before it is sent to the DB. As strings, both id and dir could be anything the user wants to send. For example, if the attacker sent a malicious formulation like:

.../move?id={id}&dir=SomeValidDir;DROP%20TABLE%20someTable;

you might be out a table. The Q&D (quick and dirty) solution in this case would be to ensure some constraints on both id and dir before passing them off to the DB. Is id really an integer? Does a valid dir in your context match any criteria like "always a single word, consisting of only alphanumeric characters"? For example, a quick fix for id might be to change the function signature:

public IActionResult Move(int id, string dir)

Then the worst an attacker could do is pick an oddball number; your framework (the language, really) would take care of the rest of ensuring it is an integer. If there are further constraints, like "the id must be between 10 and 42,000," then include those further checks before sending to the DB.

The second, more elegant and "more generally correct" solution would be to use prepared statements. The concept of a prepared statement is that you first tell the DB the format of the query, and what/where the parameters will be, and then you pass the parameters in a separate call. I will omit that description here (but point to an MSDN doc on prepared statements) because a stored procedure is inherently a prepared statement.

Thus, your (constructive) criticizer is likely balking at the fact that you have not used prepared statements and they are correct to do so. To address their critique, then, you need to call your stored procedure directly, and pass the parameters separately:

using ( SqlCommand cmd = new SqlCommand("ProcMove", con) ) {
    // ...
    cmd.CommandType = CommandType.StoredProcedure;
    cmd.Parameters.Add("@ParaId", SqlDbType.VarChar).Value = id;
    cmd.Parameters.Add("@Direction", SqlDbType.VarChar).Value = dir;
    cmd.ExecuteNonQuery();
    // ...
}