Adding entries to an MS Access file using OleDB

Question

In the following piece of code I am adding entries to an MS Access file using OleDB. The purpose of this post is to point out my bad programming practices and if I have created any security flaws here. I was reading somewhat about SQL injection attacks, and I wonder if this code might have any potential bugs.

First, it's the AddEntry button in which I am sending a data depending on which type was chosen, and each type has its own SQL query.

private void btnAddEntry_Click(object sender, EventArgs e)
{
    // Multiple level field validations.
    if (cmbType.SelectedIndex != -1)
    {
        if (cmbType.SelectedIndex == 0 &&
            (!string.IsNullOrEmpty(txtUserName.Text.Trim()) &&
            !string.IsNullOrEmpty(txtPassword.Text.Trim())))
        {
            string SQL =
                "INSERT INTO PersonalData([Type], [UserName], [Password]) " +
                "VALUES(@Type, @UserName, @Password)";

            InsertData(SQL);
        }
        else if (cmbType.SelectedIndex == 1 &&
            (!string.IsNullOrEmpty(txtURL.Text.Trim()) &&
            !string.IsNullOrEmpty(txtUserName.Text.Trim()) &&
            !string.IsNullOrEmpty(txtPassword.Text.Trim())))
        {
            // Creating SQL string. Using [] will prevent any erros
            // that might occur if any other names will be reserved words.
            string SQL =
                "INSERT INTO PersonalData([Type], [URL], [UserName], [Password]) " +
                "VALUES(@Type, @URL, @UserName, @Password)";

            InsertData(SQL);
        }
        else if (cmbType.SelectedIndex == 2 &&
            (!string.IsNullOrEmpty(txtSoftwareName.Text.Trim()) &&
            !string.IsNullOrEmpty(txtSerialCode.Text.Trim())))
        {
            // Creating SQL string. Using [] will prevent any erros
            // that might occur if any other names will be reserved words.
            string SQL =
                "INSERT INTO PersonalData([Type], [SoftwareName], [SerialCode]) " +
                "VALUES(@Type, @SoftwareName, @SerialCode)";

            InsertData(SQL);
        }
        else
        {
            lblMessage.Text = "Please fill out all required fields!";
        }
    }
    else
    {
        lblMessage.Text = "Please select a type first!";
    }
}

Secondly, this is actual code which inserts data into the file:

private void InsertData(string sql)
{
    // Initialising encrypting/decrypting file.
    Security security = new Security();

    using (OleDbConnection connection = new OleDbConnection())
    {
        // Creating command object.
        connection.ConnectionString =
            "Provider=Microsoft.ACE.OLEDB.12.0;" +
            "Data Source=" + filePath + ";" +
            "Persist Security Info=False;" +
            "Jet OLEDB:Database Password=" + hashPhrase.ShortHash(pass) + ";";

        using (OleDbCommand command = new OleDbCommand(sql, connection))
        {
            // I need to make sure that I am using correct parameters
            // when using specified combo type. If I wouldn't
            // differenciate between types, then I would end up
            // with empty fields in the database file.
            if (cmbType.SelectedIndex == 0)
            {
                OleDbParameter prmType = new OleDbParameter
                    ("@Type", security.EncryptAES(cmbType.Text, pass, user));
                OleDbParameter prmUserName = new OleDbParameter
                    ("@UserName", security.EncryptAES(txtUserName.Text, pass, user));
                OleDbParameter prmPassword = new OleDbParameter
                    ("@Password", security.EncryptAES(txtPassword.Text, pass, user));

                command.Parameters.Add(prmType);
                command.Parameters.Add(prmUserName);
                command.Parameters.Add(prmPassword);
            }
            else if (cmbType.SelectedIndex == 1)
            {
                OleDbParameter prmType = new OleDbParameter
                    ("@Type", security.EncryptAES(cmbType.Text, pass, user));
                OleDbParameter prmURL = new OleDbParameter
                    ("@URL", security.EncryptAES(txtURL.Text, pass, user));
                OleDbParameter prmUserName = new OleDbParameter
                    ("@UserName", security.EncryptAES(txtUserName.Text, pass, user));
                OleDbParameter prmPassword = new OleDbParameter
                    ("@Password", security.EncryptAES(txtPassword.Text, pass, user));

                command.Parameters.Add(prmType);
                command.Parameters.Add(prmURL);
                command.Parameters.Add(prmUserName);
                command.Parameters.Add(prmPassword);
            }
            else if (cmbType.SelectedIndex == 2)
            {
                OleDbParameter prmType = new OleDbParameter
                    ("@Type", security.EncryptAES(cmbType.Text, pass, user));
                OleDbParameter prmSoftwareName = new OleDbParameter
                    ("@SoftwareName", security.EncryptAES(txtSoftwareName.Text, pass, user));
                OleDbParameter prmSerialCode = new OleDbParameter
                    ("@SerialCode", security.EncryptAES(txtSerialCode.Text, pass, user));

                command.Parameters.Add(prmType);
                command.Parameters.Add(prmSoftwareName);
                command.Parameters.Add(prmSerialCode);
            }

            try
            {
                connection.Open();
                command.ExecuteNonQuery();
            }
            catch (Exception ex)
            {
                MessageBox.Show("Error: " + ex.Message);
            }
        }
    }

    // Refreshing state of main window.
    mainWindow.DisplayFileContent(filePath);

    lblMessage.Text = "Data was successfully added.";

    // Clearing all fields.
    ClearFields();
}

@asveikau: Do you mind telling me what might be the advantages? — HelpNeeder, Dec 4 '11 at 4:01
Basically it allows you to declare table records as C# classes, and deal with them as objects. You don't have to write any SQL by hand. You can write queries straight from C# in terms of C# objects. — asveikau, Dec 4 '11 at 4:53

MPelletier · Accepted Answer · 2011-12-04 19:47:46Z

Completely unrelated to encryption, I would like to provide feedback on your btnAddEntry_Click and InsertData functions.

The biggest problem I see here is that your database intelligence is partially in this button's code, and the insertion function has several checks on the UI. What's a button? It's a control put in an interface that a user can activate to perform an action. What's an insert query? It's a command sent to a database to add information. What's the link between the two? There really isn't one.

The button is not your program. The form is not your program. And your program is not your form. The logic you wrote is.

First, let's simplify your selection choices. What would happen if you had to add a new insertion type? You'd need to edit two functions. Lots of work, very easy to make mistakes. Instead, you can bind those choices to a list:

public enum EntryType
{
    [StringValue("")]
    None,
    [StringValue("User")]
    User,
    [StringValue("User and URL")]
    UserAndURL,
    [StringValue("Software")]
    Software
}

The StringValue attribute is taken from here:

public class StringValue : System.Attribute
{
    public string Value { get; set; }

    public StringValue(string value)
    {
        Value = value;
    }
}

Plus this function to get the attribute:

using System;
using System.Reflection;

public static class StringEnum
{
    public static string GetStringValue(Enum value)
    {
        string output = String.Empty;
        Type type = value.GetType();

        FieldInfo fi = type.GetField(value.ToString());
        StringValue[] attrs =
           fi.GetCustomAttributes(typeof(StringValue),
                                   false) as StringValue[];
        if (attrs.Length > 0)
            output = attrs[0].Value;

        return output;
    }
}

And this extension method to bind a ComboBox to an enum with string values

    public static void BindToEnum<TEnum>(this ComboBox combo)
    {
        Dictionary<string, TEnum> enumList =
                     new Dictionary<string, TEnum>();

        foreach (var enumValue in Enum.GetValues(typeof(TEnum)))
            enumList.Add(StringEnum.GetStringValue((Enum)enumValue), (TEnum)enumValue);

        // Bind the custom type combo box
        combo.DisplayMember = "Key";
        combo.ValueMember = "Value";
        combo.DataSource = new BindingSource(enumList, null);
    }

Finally, in your form's constructor:

public frmPersonalDataEntry()
{
    InitializeComponent();
    cmbType.BindToEnum<EntryType>();
}

Now we have a solid Enum to work with and that displays well. The basic idea with the btnAddEntry_Click function can just be a big switch:

switch ((EntryType)cmbType.SelectedValue)
{
    case EntryType.None:
    //do stuff
        break;
    case EntryType.User:
    //do stuff
        break;
    case EntryType.UserAndURL:
    //do stuff
        break;
    case EntryType.Software:
    //do stuff
        break;
}

Your database logic could be in a separate class, which I called PersonalDataEntry. I use a PersonalDataEntry object, with a constructor like:

public PersonalDataEntry(string user, string pass, string filePath)

This will handle your recording. Your form should have one object of this class, it doesn't need to be remade every entry.

In InsertData, do not repeat your switch. InsertData should not be aware of a form. I put InsertData in the PersonalDataEntry class:

private bool InsertData(string insertQuery, string[] paramNames, object[] paramValues, out string message)
{
    message = "";
    // Initialising encrypting/decrypting file.
    Security security = new Security();

    using (OleDbConnection connection = new OleDbConnection())
    {
        // Creating command object.
        connection.ConnectionString = String.Format(connectionString, filePath, hashPhrase.ShortHash(pass));
        using (OleDbCommand command = new OleDbCommand(insertQuery, connection))
        {
            for (int i = 0; i < paramNames.Length; i++)
                command.Parameters.Add(new OleDbParameter(paramNames[i],security.EncryptAES(paramValues[i].ToString(),pass,user)));

            try
            {
                connection.Open();
                command.ExecuteNonQuery();
                return true;
            }
            catch (Exception ex)
            {
                message = "Error: " + ex.Message;
                return false;
            }
        }
    }
}

The trick is to just reassemble the rest. Each "do stuff" is to be replaced with a function in the PersonalDataEntry object that will handle the logic (query text, variables, checking if text is valid, etc). I have uploaded a full demo project.

You have pointed out a lot of useful things here, can't wait to change some things around. — HelpNeeder, Dec 4 '11 at 22:29
PS. Your project is kind of buggy, but I have fixed it a little. — HelpNeeder, Dec 4 '11 at 22:32
Well, I had to create empty classes for Security and HashPhrase. Sorry. I was in a bit of a rush. I didn't mean for you to run it, just to see the structure. — MPelletier, Dec 4 '11 at 22:37
No problem at all! I see everything in syntax & logic. Personally I like reading code. You are a great help. Thanks! — HelpNeeder, Dec 4 '11 at 22:42

Peter Taylor · Answer 2 · 2011-12-03 10:18:37Z

I can't see any SQL vulnerabilities, but I see a few crypto mistakes.

You're using the same password for the database login and the contents. You shouldn't reuse passwords, because it increases the risk of leakage.
AES has two parameters: the key and the initialisation vector (IV). Even when you use the same key, you should really be using a different IV to avoid correlation. (E.g. the same @Type value encrypted under the same key with the same IV will be the same; if you use a different IV, you can avoid an attacker being able to match them up). It would be better to get a random IV (using a secure RNG) and store that in another column.
You're using AES to conceal the password. With very few exceptions, you should hash passwords rather than encrypt them. The standard recommendation is to use bcrypt.

1.Yes, I am using the same password because I could not find any other way I could do this. This is because I am hashing database password. 2. I wasn't sure if I should use the same IV or not. I will generate them randomly I suppose. 3. Since this is a password manager program it stores the data which I must retrieve later. I am not encrypting passwords here, but the data. +1 for help as of right now. I will accept soon if no one else post. :) — HelpNeeder, Dec 3 '11 at 17:24

Arjan Einbu · Answer 3 · 2011-12-04 15:33:01Z

You are building your queries in two places, which makes it hard to read/understand your code. I need to jump back and forth between the two places where you assess the value of cmbType.SelectedIndex to see the query and whatever parameters it gets in. You're repeating the if/else if statements accross two different methods.

You should instead try to come up with a way to set the parameters that go with each query close to where you're creating the query. (If thats the only way, you could perhaps create the SqlCommand object(s) in one place, and send that into the InsertData method instead of the SQL string.

Arjan Einbu · Answer 4 · 2011-12-04 15:41:38Z

up vote 2 down vote

I would find the code easier to read if you tested cmbType to see if you can exit early, instead of saving it till the end. (with the else block)

At this moment your code is a little like this:

if (cmbType.SelectedIndex != -1)
{
    // ... very many lines of code
}
else
{
    lblMessage.Text = "Please select a type first!";
}

... and by changing it to this:

if (cmbType.SelectedIndex == -1)
{
    lblMessage.Text = "Please select a type first!";
    return;
}

// ... very many lines of code

It would be easier to follow the code (reading), since you're not jumping so many lines up and down in code to follow the flow.

Another nice reward is offcourse that you're decreasing the nesting levels, which also makes it easier to follow the code.

answered Dec 4 '11 at 15:41

Arjan Einbu
30125

Ah! I have just added it to the long series of IF statements. – HelpNeeder Dec 4 '11 at 21:54

You should consider creating only one answer. You do help, but I must scroll down to see your both answers. – HelpNeeder Dec 4 '11 at 22:53

@HelpNeeder: I created seperate answers for different topics, so that its easier to upvote/downvote on the parts people agree/disagree on... – Arjan Einbu Dec 5 '11 at 8:10

add a comment |

asked	3 years ago
viewed	296 times
active	8 months ago

current community

your communities

more stack exchange communities

Adding entries to an MS Access file using OleDB

4 Answers 4

Your Answer

Not the answer you're looking for? Browse other questions tagged c# security sql-injection ms-access or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

Adding entries to an MS Access file using OleDB

4 Answers 4

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c# security sql-injection ms-access or ask your own question.

Related

Hot Network Questions