0

I am encrypting password using md5 security and storing it into the database in asp.net but it gives me the exception of Incorrect Syntax near ' '. I used binary(16) data type for storing encrypted password. 

query = "insert into accounts(email, password, user_type) output inserted.id values('" + clientBLL.email + "', " + clientBLL.password + ", 0);";
string accountId = DBManager.ExecuteScaler(query);

When I put single quotes around password " ' " + signIn.password + " ' " it gives me Implicit conversion from data type varchar to binary is not allowed. Use the CONVERT function to run this query error. What should I do for storing md5 encrypted password into database.

Encrypted password code.

public static string encryptMd5(string textToEncrypt)
    {
        using (MD5CryptoServiceProvider md5 = new MD5CryptoServiceProvider())
        {
            md5.ComputeHash(Encoding.ASCII.GetBytes(textToEncrypt));
            byte[] result = md5.Hash;
            StringBuilder str = new StringBuilder();

            for (int i = 1; i < result.Length; i++)
            {
                str.Append(result[i].ToString("x2"));
            }

            return str.ToString();
        }
    }
  • 2
    *Stop* building SQL queries via string concatenation. It's a bad idea, you run into data type conversion issues (as here) because everything is forced to become a string (and then gets implicitly converted back to the right data type) and its vulnerable to SQL Injection. Use *parameters* and use the *appropriate* data types in both your C# code and your SQL code and your issues will evaporate. – Damien_The_Unbeliever Jan 25 '18 at 10:38
  • And MD5 is the most insecure choice you can make. (a part from plaintext of course). See [Best way to store passwords in a database](https://stackoverflow.com/questions/1054022/best-way-to-store-password-in-database) – Steve Jan 25 '18 at 10:44
  • And why in the world would your encryption function return the string version of a binary value (represented in hex notation)? It is a binary value - why complicate things by converting (and then storing it) as string? Change the datatype of the column in your table as well as follow the suggestion from Matthew. – SMor Jan 25 '18 at 14:07
  • I changed it to byte[] now its working –  Jan 25 '18 at 16:12

1 Answers1

2

Please note comments re the extremely bad practice of generating SQL this way. Your current implementation is wide open to a SQL injection attack. Rather follow the pattern in the code below.

That being said, the exception tells you to use convert

i.e.

    using (SqlCommand dbCommand = new SqlCommand(@"insert into accounts(email,  
                    password, user_type) output inserted.id 
                    values(@username, convert(varbinary, @password),
                    @userType)", dbConn))
    {

        dbCommand.Parameters.Add("username", SqlType.VarChar).Value = clientBLL.email;
        dbCommand.Parameters.Add("password", SqlType.VarBinary).Value = clientBLL.password;
        dbCommand.Parameters.Add("userType", SqlType.Int).Value = 0;
        dbCommand.ExecuteNonQuery();
    }
Matt Evans
  • 7,113
  • 7
  • 32
  • 64
  • 1
    Please read the comment of @Damien_The_Unbeliever above. Better post an answer that uses parameters. – VDWWD Jan 25 '18 at 10:41
  • It gives me the exception "String or binary data would be truncated." –  Jan 25 '18 at 10:43
  • Then i guess varbinary 16 is too small to store your hash. Try varbinary(max) – Matt Evans Jan 25 '18 at 10:48
  • When I use parameterize method it give me the following exception "Implicit conversion from data type nvarchar to binary is not allowed. Use the CONVERT function to run this query". What exactly is going on here man! –  Jan 25 '18 at 11:07
  • Are you using this syntax: convert(varbinary, @password)? Also there was a typo in my original code. The SqlType is obviously VarBinary. I have corrected this – Matt Evans Jan 25 '18 at 11:14
  • 1
    Once you've corrected the parameter type (as you have already done), the `CONVERT` is no longer needed (and is potentially dangerous since you've not specified a length, so could truncate the data) – Damien_The_Unbeliever Jan 25 '18 at 15:49