0

Currently my code is susceptible to SQL Injection as I just test it.

The way I used to retrieve a record from dbo.Account

var condition = String.Format("[Username] = '{0}' AND [Password] = '{1}' AND Active = 1", username, password);

var account = new Data.Account().Select(condition, string.Empty, 0, 0);

public List<Model.Account> Select(string condition, string orderBy, int limit = 0, int offset = 0)
{
    var list = new List<Model.Account>();
    var query = "SELECT " + TABLE_COLUMN + " FROM [DBO].[ACCOUNT]";

    if (condition != string.Empty) 
        query += " WHERE " + condition;

    if (orderBy != string.Empty || limit > 0) 
        query += " ORDER BY " (orderBy == string.Empty ? "ID DESC" : orderBy);

    if (limit > 0) 
        query += " OFFSET " + offset + " ROWS FETCH NEXT " + limit + " ROWS ONLY";

    using (var db = new SqlManager())
    {
        using (var reader = db.ExecuteReader(query))
        {
            while (reader.Read())
            {
                var item = BindData(reader);
                list.Add(item);
            }
        }
    }
}

And right now I am trying to create a string using SqlParameter like this:

SqlCommand condition = new SqlCommand("[Username] = @Username AND [Password] = @Password");
condition.Parameters.AddWithValue("@Username", username);
condition.Parameters.AddWithValue("@Password", password);

var account = GetAccountByCondition(condition.CommandText);

How can I achieve to get the SqlCommand string along with the username and password value included?

I am doing this way because my data access layer is generated via program. And the String.Format for condition is really wrong.

The SqlManager class I am using, for your reference:

public class SqlManager : IDisposable
{
    private SqlConnection _connection;

    public SqlConnection Connection
    {
        get { return _connection; }
    }

    private SqlCommand _command;

    public SqlCommand Command
    {
        get { return _command; }
    }

    private SqlTransaction _transaction;

    public SqlTransaction Transaction
    {
        get { return _transaction; }
    }

    private List<SqlParameter> _parameters = new List<SqlParameter>();

    public List<SqlParameter> Parameters
    {
        get { return _parameters; }
        set { _parameters = value; }
    }

    public SqlManager()
    {
        var constr = ConfigurationManager.ConnectionStrings["constr"].ConnectionString;
        _connection = new SqlConnection(constr);
        _command = new SqlCommand();
        _connection.Open();
    }

    public int ExecuteNonQuery(string commandText)
    {
        return ExecuteNonQuery(commandText, CommandType.Text);
    }

    public int ExecuteNonQuery(string commandText, CommandType commandType)
    {
        _command.Connection = _connection;
        _command.Transaction = _transaction;
        _command.CommandType = commandType;
        _command.CommandText = commandText;
        _command.Parameters.Clear();
        _command.Parameters.AddRange(_parameters.ToArray());

        return _command.ExecuteNonQuery();
    }

    public object ExecuteScalar(string commandText)
    {
        return ExecuteScalar(commandText, CommandType.Text);
    }

    public object ExecuteScalar(string commandText, CommandType commandType)
    {
        _command.Connection = _connection;
        _command.Transaction = _transaction;
        _command.CommandType = commandType;
        _command.CommandText = commandText;
        _command.Parameters.Clear();
        _command.Parameters.AddRange(_parameters.ToArray());

        return _command.ExecuteScalar();
    }

    public IDataReader ExecuteReader(string commandText)
    {
        return ExecuteReader(commandText, CommandType.Text);
    }

    public IDataReader ExecuteReader(string commandText, CommandType commandType)
    {
        _command.Connection = _connection;
        _command.CommandType = commandType;
        _command.CommandText = commandText;
        _command.Parameters.Clear();
        _command.Parameters.AddRange(_parameters.ToArray());

        return _command.ExecuteReader();
    }

    public XmlReader ExecuteXml(string commandText)
    {
        return ExecuteXml(commandText, CommandType.Text);
    }

    public XmlReader ExecuteXml(string commandText, CommandType commandType)
    {
        _command.Connection = _connection;
        _command.CommandType = commandType;
        _command.CommandText = commandText;
        _command.Parameters.Clear();
        _command.Parameters.AddRange(_parameters.ToArray());

        return _command.ExecuteXmlReader();
    }

    public DataTable ExecuteDataTable(string commandText)
    {
        return ExecuteDataTable(commandText, CommandType.Text);
    }

    public DataTable ExecuteDataTable(string commandText, CommandType commandType)
    {
        _command.Connection = _connection;
        _command.CommandType = commandType;
        _command.CommandText = commandText;
        _command.Parameters.Clear();
        _command.Parameters.AddRange(_parameters.ToArray());

        var dt = new DataTable();
        dt.Load(_command.ExecuteReader());

        return dt;
    }

    public void AddParameter(string paraName, object objectValue)
    {
        AddParameter(paraName, objectValue, ParameterDirection.Input);
    }

    public void AddParameter(string paraName, object objectValue, ParameterDirection direction)
    {
        var para = new SqlParameter();
        para.ParameterName = paraName;
        para.Direction = direction;
        para.Value = objectValue;
        _parameters.Add(para);
    }

    public void BeginTransaction()
    {
        _transaction = _connection.BeginTransaction();
    }

    public void Commit()
    {
        if (_transaction != null)
        {
            _transaction.Commit();
        }
    }

    public void Rollback()
    {
        if (_transaction != null)
        {
            _transaction.Rollback();
        }
    }

    public void Dispose()
    {
        GC.SuppressFinalize(this);

        if (_connection.State == ConnectionState.Open)
        {
            _connection.Close();
        }

        _connection = null;
        _command = null;
        _transaction = null;
    }
}
13
  • If you need to get the full query as text, that defeats the purpose of using parameters; you'll have to do all injection prevention manually. Doesn't the SqlManager class you use have some inbuilt system that allows executing queries with parameters? Commented Jan 30, 2018 at 11:08
  • @Nyerguds hi, I have been referring to my senior's code. He left the company. I just edited the content and included the SqlManager class. I am not really sure how to use it. Sorry for all the troubles. Commented Jan 30, 2018 at 11:14
  • There's a Parameters property on your SqlManager class that sets the internal _parameters used in ExecuteReader. Use that. Commented Jan 30, 2018 at 11:15
  • @PanagiotisKanavos There's a manager class involved though. I just wasn't aware OP had full access to its internals. OP should just have read the code behind ExecuteReader to see where the parameters were taken from. Commented Jan 30, 2018 at 11:17
  • 1
    I honestly don't understand your issue then, though. Just feed the parameters into the Parameters property of db before calling ExecuteReader with the actual query text. Commented Jan 30, 2018 at 11:41

1 Answer 1

1

You could build the SELECT query and assign it to SqlCommand object

Dim cmd As SqlCommand

cmd = New SqlCommand("SELECT * FROM [DBO].[ACCOUNT] WHERE [Username] = @username AND [Password] = @pwd AND Active = 1")
cmd.Parameters.Add(New SqlParameter("@username", txtName.Text))
cmd.Parameters.Add(New SqlParameter("@pwd", txtId.Text))

Then pass parameter names and values using Parameters.Add method to prevent SQL injection as I also tried to explain at referred document.

Since your SQL query is more complex than the code above, you can build your SQL command dynamically by using a string query variables with parameter placeholders in it, then finally assign it to the SqlCommand object, too

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

1 Comment

He's using a manager class though, and its execute function clears and re-fills those from its own internal list.

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.