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.  

 

 

Comments (8)

  1. John Rudy says:

    Is there some less expensive way to do this, something that doesn’t involve copying potentially massive arrays? I mean, copying an array of strings — like you said — isn’t bad, but if you get into a deep copy situation, and you’re writing a server-class library, this is going to really screw with performance.

    Perhaps a locking mechanism? Synchronized access? Something?

    John Rudy

    MCSD

  2. David Levine says:

    This is one of those situations where the cure may be worse then the disease. It is extremely difficult to always recognize when an argument/field/object is mutable and subject to this problem, let alone ensure that it is properly dealt with. Making copies of all potentially mutable objects on the off chance that you might get hosed is going to wreak havoc on memory usage and performance and will probably introduce more bugs then it will prevent. Race conditions are notoriously difficult to detect and debug, let alone code against, especially when the resource is really a system resource outside the direct control of the application, such as a file on disk.

    If this extra coding effort is really necessary, then here’s a random thought: introduce a keyword, "mutable" that modifies an argument’s signature so that it indicates that the method takes extra precautions to detect changes to that argument during the lifetime of the method call, possibly including copying the object graph. This is similar in intent to documenting whether a method is thread-safe. If an argument is not marked as mutable it puts the onus on the caller to ensure that mutable objects do not get modified thoughout the duration of a method call. If the argument is marked as mutable then the extra overhead of is clearly indicated.

    To be honest, I think this coding guideline needs a lot more thought.

  3. anon says:

    …maybe you should put more effort into making sure hackers don’t have access to your source code?

  4. Thanks for the feedback.. Notice the guideline says “If the operation is security sensitive”… That is I don’t expect you to do this for EVERY method, just those that are at a trust boundary where there is untrusted callers calling your API… implementing a website that may never happen, but writing a component that can be used by semi-trusted code it may happen.

    Does that help?

  5. Sean Chase says:

    Interesting post Brad, thanks!

  6. anon says:

    This isn’t just a security concern, IMHO… this needs to be considered any time you are writing code that could be accessed by a caller that may be multi-threaded and your method requires that the reference object, once validated, does not change for the duration of the method.

    So, is Microsoft going to be publishing a set of security design guidelines at some point resulting from the security push? Or are these published somewhere already?

  7. Daniel Moth says:

    Blog link of the week 49

  8. Hey Anon… This is the best source right now:

    http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dnnetsec/html/seccodeguide.asp

    We will update this site with more information before we ship Whidbey…