Posts tagged with c - page 15

Avoiding SQL injection

Back in ’98 I was developing an extranet site for a local company when I realized that it would be open for exploit if somebody put single quotes in text fields. It was early in the development cycle so I fixed it and moved on, unable to find out how other people were avoiding the problem.

It turned out many were not and it became a well-known exploit called SQL injection. Unfortunately there are many developers who don’t know or appreciate the problem, and it is this:

If you build SQL by appending strings and data without correct encoding your application can be exploited. These exploits can range from exposing sensitive information, through to modification and deletion of data.

This problem is very real and applies to:

  • All SQL statements, not just SELECT
  • All database systems, not just MS SQL or MySQL
  • All programming environments, not just C#, PHP or ASP
  • All data, most essentially that obtained from end-users, regardless of client-side checking

Let’s walk through an example and see how it works and what can be done to avoid it.

Example: User login

We have a user-name and password from a web form and want to get the users ID from the database, or nothing if it wasn’t valid. We want to send something like this SQL statement to our database.

SELECT UserID FROM Users WHERE UserName='Bob' AND Password='test'

And so a developer might do something like this (in C# using .NET);

var dr = connection.Execute("SELECT UserID FROM Users WHERE UserName='" + Request("UserName") + "' AND Password='" + Request("Password") + "'");
if (dr.Read()) userId = dr.GetInt32(dr.GetOrdinal("UserID"));

The problem here is that if there is a ‘ in the form fields it effectively breaks out of the selection criteria and allows the end user to add extra criteria or even commands to what we are sending to the database server. Should they enters the following into the password form field…

    ' OR ''='

Then our code above will send the following SQL to the database:

SELECT UserID FROM Users WHERE UserName='aaa' AND Password='' OR ''=''

Which will return every record in the database and our code will let him log in as the first user it finds – normally a developer or administrator account. Ouch!

Bad solution: Encode it yourself

One solution often adopted is to always ensure all string input has a single-quote replaced by two single-quotes, which is what SQL server expects if you really want to send it a single quote.

This solution fails in that it doesn’t handle numbers or dates and falls apart in that both numbers and dates are often regionally formatted.

Good solution: Let the DB client encode it

A much better solution is to use your environment to perform all the proper encoding for you. As well as protecting you from such exploits you’ll also avoid localization problems where the string representation of something on your client is interpreted differently in your database. This can be a real problem in the UK where dates formatted by a UK web-server are sent to a misconfigured SQL server expecting US formatting and the days and months become transposed without error.

var cmd = new SqlCommand("SELECT UserID FROM Users WHERE UserName=@UserName AND Password=@Password");
cmd.Parameters.Add(new SqlParameter("@UserName", System.Data.SqlDbType.NVarChar, 255, Request("UserName")))
cmd.Parameters.Add(new SqlParameter("@Password", System.Data.SqlDbType.NVarChar, 255, Request("Password")))
dr = cmd.ExecuteReader();
if (dr.Read()) userId = dr.GetInt32(dr.GetOrdinal("UserID"));

Okay there is no doubt this is a little longer but it takes care of all our encoding and localization worries, and if for example you want to insert a lot of data into the database, creating the command and parameters once, then just setting the parameters for each insert (and executing it) will run faster than lots of string building inserts…

Moral of the story

Always encode data properly. If you can use the provided methods and functions to do so. If none are provided grab the specification and find out all the special characters used. Learn what encoding and escape sequences are used and apply them properly.

A few places where data should be encoded properly:

  • HTML – Obviously < and > have special meanings and need to be escaped. ASP.NET controls will take care of this if you set the .Text or .InnerText properties but set .InnerHTML at your own peril. Old ASP has the Server.HTMLEncode() function.
  • URL – A whole host of rules but the query string is often modified in code. Use URLEncode() or something similar especially if you want XHTML compliance too.
  • XML – Again a whole host of rules for what is valid data. Either use an XML object to write out your data (MSXML, Xerces etc) or maybe even store it in [[CDATA sections.
  • CSV – Even comma-separated value files have encoding rules. What do you need to do if text is going to have a ” in a field. What happens if a number contains a comma! Find out or use a well regarded library to do it for you.

Notes about the example

A better login system would not allow the web server direct access to sensitive data such as the user table. All access to sensitive information should be through stored procs that enforce those restrictions.

Such a login system would therefore call a stored procedure that logged the attempted, decided if it was valid, and locked out the user if too many incorrect attempts. I’ll blog that if anyone is interested.

Even if you don’t want to do that, returning a single field is better achieved by using ExecuteScalar() and forgetting a data reader.

Microsoft have a developer how-to on injection.

[)amien

VB.NET to C# conversion

I recently converted some components on a project from VB.NET to C#, mainly for overloading and better tool support (such as ReSharper). Some of the existing code was generated from my own CodeSmith templates, so a small rewrite to generate C# handled most of that.

VB.NET to C# Converter 1.31

The remaining code, while not extensive in size, is a rather complex affair and the prospect of debugging this code when hand-converted was a little daunting so I decided to give the demo version of VBConversion’s VB.NET to C# Converter 1.31 a spin.

I was rather disappointed it made some rather obvious and stupid mistakes especially when it had converted everything else so well – so much for the 99% accuracy claim. I thought it might just be our project be we adhere to many of Microsoft’s guidelines

Problem 1: Ignores all instance/class variables default assignments and constructors.

Public Class Mine
  Public myObj As MyClass = New MyClass()
  Private myVar As MyEnumeration = MyEnumeration.MyDefault
End Class

Suddenly becomes;

public class Mine {
  public MyClass myObj;
  private MyEnumeration myVar;
}

While the object not being created will soon throw an exception, the defaults for value types is a little harder to track down.

Problem 2: Gives up on Select Case statements after the first case – commenting out the others.

In at least one case it got so confused it commented out substantially more code after the case statement too.

Problem 3: Declares additional unnecessary name-spaces including the one for Microsoft.VisualBasic, despite not needing it.

Either we hadn’t used the CDate/CInt/CLng functions or it had converted them)…

Problem 4: All with statements are converted to be variable with1, even when the old with clause was a simple case.

For example:

With myObj
  .doThis()
End With

becomes;

MyClass with1 = myObj;
with1.doThis();

Using both VB.NET and C# together

If you are going to use both in a project you can bring the syntax a little closer in format, here’s a few tips.

  • Don’t use the Visual Basic CInt/CDate/IsNumber etc functions. Use the .NET Framework equivalents such as Int.Parse, Date.Parse etc. which will work in both languages and are normally faster than these legacy functions.
  • Bring the source closer together by;
    • Putting brackets round if conditions in VB.NET
    • Putting quotes round region declarations in C#
    • Putting attributes on separate lines in VB.NET with a _ suffix
    • Dropping the implementation and interface declarations onto a new line in C#
    • Dropping the base(whatever) onto a new line in C# constructors (good idea anyway)
  • Check out the “Differences Between Visual Basic .NET and Visual C# .NET” white paper on MSDN
  • Use VBCommenter to get C# style XML comment documentation.
  • Use Monitor.Enter() instead of VB.NET’s synclock and C#’s lock.
  • Be aware of the differences regarding math! VB.NET defaults to floating point precision and rounding when converting with CInt/CLng etc.. C# normally uses integer division and casting truncates.

[)amien

Lapsed-listeners – Memory leaks in subscriber-publisher scenarios

I’ve been promising something .NET related for a while… here’s something!

The lapsed-listener scenario

There exists a ‘gotcha’ in .NET (and other programming environment) whereby an object has subscribed to another objects published event will not be garbage collected when you expect because the environment itself holds a reference to the subscriber inside the event notification system.

Let’s illustrate with an example:

MyBusiness Class

Encapsulates some aspect of business functionality and publishes a Changed event.

MyTreeNode Business Adapter Class

Inherits from TreeNode and takes an instance of MyBusiness in the constructor. The object signs up to the Changed event of the MyBusiness object so that it can always accurately show the correct Icon (via ImageIndex), Text and update any associated child nodes etc.

Now you happily add instances of MyTreeNodeBusinessAdapter class to your TreeView, each with the associated MyBusiness instance it should be reflecting.

The problem now arises in that if the TreeNode is removed from the TreeView with either Nodes.Remove or Nodes.Clear then the garbage collector will not be able dispose of the MyTreeNodeBusinessAdapter objects because the MyBusiness objects hold a reference to them in their event notification queues. Even worse, an inactive TreeView hangs around because the not-yet-disposed TreeNode’s are still pointing to it.

Possible solutions…

.NET Framework 3.0 has a WeakEvent pattern to make things simple!

1. MyTreeNodeBusinessAdapter listens in to TreeView events and un-subscribes when it is removed (Rejected)

TreeNode’s maintain their own internal array of child nodes which they publish under the “Nodes” property which uses the TreeNodeCollection class to just point straight back at itself calling internal methods. None are over-ridable and while they do send the message TVM_DELETEITEM to their own TreeView control it does nothing with them nor does it have methods you could implement in a subclass. (.NET Reflector is great for finding out what’s going on inside the WinForms assembly)

This would also mean every UI control you adapt would need sub-classing to override even if you could hook into the remove events.

2. Weak Events

Xavier Musy’s web log has an interesting C# class called WeakMulticastDelegate that works in a similar way to normal delegates but uses the WeakReference class in .NET to allow referencing an object that can be garbage collected.

This solution also prevents subscribers from subscribing more than once and also from subscribers throwing an exception that stops other subscribers receiving this event (in .NET an exception will break the chain).

Alas it requires that you publish an event by writing Add and Remove methods for your event – not possible directly under VB.NET but possible with the following workaround:

  1. Create a C# Class Library project inside your solution and add the WeakMulticastDelegate code to it (remember to put using System; and using System.Reflection; at the top of the file)
  2. Create a new MyPublisherWorkaround class in the C Sharp project with the private WeakMulticastDelegate and public event EventHandler code blocks described in Xavier’s article.
  3. In MyPublisherWorkaround create a method to fire your event (because .NET does not let you raise events defined in a parent class) e.g: protected internal void Changed(EventArgs e) { if (this.weakEventHandler != null) weakEventHandler.Invoke(new object[] { this, e } ); }
  4. Add the new C# project to your VB.NET project reference and appropriate Imports line at the top of your VB.NET project (obviously).
  5. Change your MyPublisher class to inherit from MyPublisherWorkaround.
  6. Change your MyPublisher class to call the Changed method (or whatever you named it) instead of executing RaiseEvent directly (because of aforementioned restrictions in the .NET framework).

3. Weak Delegates

Greg Schechter on the Microsoft Avalon team has written an article about this very problem and his solution of Weak Delegates.

Our scenario would become:

  1. MyPublisher instance receives request to add instance of MySubscriber to event.
  2. MyPublisher creates new instance of own internal class derived from WeakReference.
  3. This WeakReference instance actually contains the reference to the target object.
  4. The WeakReference instance’s event handler (with the same signature) is added to the event listener.

While this works in VB.NET it has a problem in that the class derived from WeakReference requires all subscribers to either be the same class, derived from a common parent or support a specific interface that defines this event handler.

This defeats much of the point of delegates although it might be possible to rework his code so that WeakReference stores a weak reference to the given event handler and not the object implementing it.

Stepping back

WinForms is focused around the Win32 API itself and not what modern applications require from a UI when developing along Model-View-Controller patterns.

Compound controls such as TreeView and ListView should deal with interfaces such as ITreeNode and IListViewItem and then provide a concrete implementation for backwards compatibility/general use (TreeNode and ListViewItem).

This would solve some issues – check out Skybound’s VisualStyles for a free and easy to use fix for the poor theme support.

Check out the WeakEventManager in .NET 3.5 for an event mechanism that is immune to this problem.

[)amien