In class on Wed. Mar. 2, during code inspection of the Blinky webservice PostBlog() method, Christophe noticed that the webservice code was susceptible to a SQL injection attack. 

We would like for class members to avoid this type of vulnerability in any Project 7 database queries.  Here is the newly modified BLinky webservice code.

Christophe:

A vulnerability jumped to my eyes when you gave us to code source of the webservice. The vulnerability is called SQL injection and is present in several of the public methods. For example, in PasswordOK, the SQL command is built by concatenating

   "Select UserPassword from Users where UserID='" + userid + "'"

The problem is that the content of userid is never checked (only trailing white spaces are removed) and can be chosen maliciously. For example, it is likely that calling this webservice method with userid set to

   "whatever'; DELETE FROM Users WHERE true; SELECT * FROM Users WHERE userid='whatever"

would cause some trouble. If this particular one doesn't work, another would. The user input should never be used unchecked in a SQL command. Checking that userid is only composed of valid characters could be a start (let's say alphanumeric and .-).

My understanding is that right now everyone has right access to the database using SQLinjection (with or without the code source).

Pat:

Thanks for the heads up.  I will forward this to Gayle to see if she wants to fix it, or if I should.  A couple of possible fixes come to mind:

1) checking the added part (UserId or whatever) to see if any ";" is in it; I believe that would be required to inject an additional command
2) checking to see if the UserId exists in the Users table

Christophe:

The ";" character does not matter, as it will be between the outer '.  Moreover, it is possible to do SQL injection without it using subqueries with parenthesis.

Gayle:

Thanks for bringing this to my attention. I'll fix it, since it's my code.  Christophe, what are your thoughts on the best way to fix this? Checking for alphanumeric characters would work only for the userid table, and this is a more general problem. If I just wanted to fix that problem, I could actually just check the length of what's passed in. What is the elegant solution? I believe that the best way may be to:

   1) create stored procedures in the database
   2) create a new sql user with only access to these stored procedures
   3) generate the stored procedures with the following code:

   SqlCommand salesCMD = new SqlCommand("SalesByCategory", nwindConn);
   salesCMD.CommandType = CommandType.StoredProcedure;
   SqlParameter myParm = salesCMD.Parameters.Add("@CategoryName",
   SqlDbType.NVarChar, 15);
   myParm.Value = "Beverages";
   salesCMD.ExecuteNonQuery();

How does this look to you?

Christophe:

I am not sure if using stored procedures would be enough. I would validate all inputs before calling the database. I don't think it's very difficult in the case of Blinky, except for existing users with too much creativity (when picking up their logins/passwords).  For all inputs except login/password/displayname, you know the form of the input so you can validate it easily. For example for blogID I would simply do a conversion to int before using it to build my sql command. For login/password/displayname, I would use regular expressions as explained below.

I would use one regular expression per type of input. Then the same regular expressions should be used everywhere, so that the validation in RegisterUser rejects the same bad inputs as PasswordOK.  Since the database is already used, the regular expression should take into account already existing data.  For example if someone picked "!@# $$" for its login (someone did!), the regular expression should allow this login or at least reject it consciously.  For the login, the regular exception could allow only alphanumerical characters and restrict the number of characters (something like [a-Z0-9]{5-16}).

So I would do something like that (NOTE 1: This regular expression will prevent some user currently registered from login since they use funky login/password/...) (NOTE 2: I have not tested the code below; in particular I am not sure if C# regexp are like unix regexp):

   // Declare in one place all my regular expressions
   Regex login_regex = new Regex("[a-Z0-9]{5-16}");
   Regex password_regex = new Regex("[a-Z0-9]{5-16}");
   Regex displayname_regex = new Regex("[a-Z0-9 _-.]{1-16}");

   // And use them wherever necessary
   Match m = login_regex.Match(login);
   if (!m.Success) return false;

The webservice RegisterUser method descriptions should be extended to clarify the restriction imposed to each input.  UserID, Password and DisplayName are not the only input that need to be validated but they are the more problematic since they are the one not supposed to be generate by the webservice.

Another security issue with the login, but less critical, is that the webservice does not restrict the use of the PasswordOK method in time (I have no idea if it is difficult to implement this kind of security feature). So a brute force attack to find a user password is easy.

Gayle:

So, with the PasswordOK thing, yeah, I could have restricted the number of attempts. There's just no reason to... it's a course project, after all :-). If I wanted good security, there's a number of things that you should be done, such as not accepting the password in plain text.

I'm not sure how regular expressions really help here. Couldn't someone apply the same tricks to the PostBlog method, which you can't create a regular expression for?

And with validation for userid, password, etc... all the lengths are restricted to 16 characters so as long you do that check, wouldn't that be enough? Could you really write a malicious sql statement in less than 16 characters?

Pat:

"delete * from users" is longer than 16 chars, and it's the shortest, meanest thing I could think of.

Christophe:

Just for the challenge: "drop table Users" would be 16. But It would not be too long to inject with a string at most 16 characters long. Anyway I think that if the quote characters are correctly escaped, the ";" character does not matter as it will be between the outer '. Moreover it is possible to do sql injection without it using subqueries with parenthesis.

Re: regular expressions not working in all cases--yes. As pat proposed, I think it is necessary to escape some characters.
I don't know how it is usually done in .Net. There is certainly a .NET way to sanitize an input. In PHP, I would use something like:

http://us4.php.net/manual/en/function.dbx-escape-string.php

I would not be surprised if something similar is available in .NET. And I agree that checking the length (of the userid/password) would be a start.

Gayle:

Ok, so I made the following changes:

- length validation immediately on all string parameters on all methods
- all strings have their apostrophe's escaped. (a simple replacing of ' with '')

Not sure if this makes it full proof... but it at least makes it tougher.  I think the way to go eventually is stored procedures. But for now... this is probably good enough.  I'd rather not make too many changes to the code while people are still working on their projects.  Thanks for bringing this to my attention.

Christophe:

I agree (with this solution). With security it's always difficult to be sure, but I think Gayle's fix
is good enough for Blinky.