New Design Guideline: Parameter validation
More guideline updates from the Security push (as Soma said earlier this year, the push is now upon us!)
Do be aware that mutable values may have changed after they were validated. If the operation is security sensitive you are encouraged to make a copy, then validate and process the argument.
The following sample code demonstrates the issue. The DeleteFiles() method does not create a copy of the filenames array before validation as such it is subject to a race condition.
/// <summary>
/// Deletes all the files passed in if they all pass validation, throws otherwise.
/// </summary>
public void DeleteFiles(string[] filenames)
{
if (!ValidateFiles(filenames)) throw new ArgumentException("Files must be local");
Thread.Sleep(1);//force a race to demonstrate the problem
foreach (string s in filenames)
{
Console.WriteLine("Thread1: Deleting file: {0}", s);
/*TODO: File.Delete(s); */
}
}
private bool ValidateFiles(string[] filenames)
{
foreach (string s in filenames)
{
if (s.Contains("..")/*TODO: real test */)
{
return false;
}
else
{
Console.WriteLine("Thread1: '{0}' passes validation", s);
}
} //end for
return true;
}
Notice we force the race condition with the Thread.Sleep() call, without the call the problem may still happen depending on other factors on the machine.
This is exploited by the calling code forking off a thread to change the values in the array after validation occurs.
static void Main(string[] args)
{
Program p = new Program();
string[] names = { "one.txt", "two.txt" }; //init to a set of valid values
Thread hackerThread = new Thread(delegate(object obj) //set up hacker thread
{
names[1] = @"..\..\boot.ini";
Console.WriteLine("Thread2: changing 1 to '{0}'", names[1]);
});
hackerThread.Start();
p.DeleteFiles(names); //call the API being hacked
}
The output from this code is:
Thread1: 'one.txt' passes validation
Thread1: 'two.txt' passes validation
Thread2: changing 1 to '..\..\boot.ini'
Thread1: Deleting file: one.txt
Thread1: Deleting file: ..\..\boot.ini
Notice that Thread1 validates ‘one.txt’ and ‘two.txt’, but ends up deleting ‘one.txt’ and ‘..\..\boot.ini’ because thread2 is able to change one of the values after validation happens.
The fix for this problem is relatively simple, if expensive. Simply use Array.Copy() to create a local copy of the array *before* validating and performing operations on it. Notice Array.Copy() only does a shallow copy, this works for arrays of strings as strings are immutable, but if you are dealing with an array of some mutable type (such as StringBuilder, boxed value type, etc) you’d have to do a deep copy for the array.
/// <summary>
/// Deletes all the files passed in if they pass validation, throws otherwise.
/// </summary>
public void DeleteFiles(string[] filenames)
{
string [] filenamesCopy = new string[filenames.Length];
Array.Copy(filenames, filenamesCopy,filenames.Length);
if (!ValidateFiles(filenamesCopy)) throw new ArgumentException("Files must be local");
Thread.Sleep(1);//force a race
foreach (string s in filenamesCopy)
{
Console.WriteLine("Thread1: Deleting file: {0}", s);
/*TODO: File.Delete(s); */
}
}
With this change, we throw an exception as expected or delete only the safe values depending on a race.