[Gendarme] New Security rule: SqlCommandShouldUseNamedParameters

gendarme/AUTHORS

@@ -12,7 +12,7 @@ Adrian Tsai <[email protected]>
 Seo Sanghyeon  <[email protected]>
 Scott Peterson <[email protected]>
 Cedric Vivier <[email protected]>
-Andres G. Aragoneses <aaragoneses@novell.com>
+Andres G. Aragoneses  <andres@7digital.com>
 Peter Johanson  <[email protected]>
 Jesse Jones  <[email protected]>
 Rolf Bjarne Kvinge <[email protected]>

gendarme/rules/Gendarme.Rules.Security/Gendarme.Rules.Security.csproj

@@ -71,6 +71,7 @@
     <Compile Include="ArrayFieldsShouldNotBeReadOnlyRule.cs" />
     <Compile Include="DoNotShortCircuitCertificateCheckRule.cs" />
     <Compile Include="NativeFieldsShouldNotBeVisibleRule.cs" />
+    <Compile Include="SqlCommandShouldUseNamedParameters.cs" />
     <Compile Include="StaticConstructorsShouldBePrivateRule.cs" />
   </ItemGroup>
   <ItemGroup>

gendarme/rules/Gendarme.Rules.Security/Makefile.am

@@ -4,10 +4,12 @@ rules_sources =  \
 	ArrayFieldsShouldNotBeReadOnlyRule.cs \
 	DoNotShortCircuitCertificateCheckRule.cs \
 	NativeFieldsShouldNotBeVisibleRule.cs \
+	SqlCommandShouldUseNamedParameters.cs \
 	StaticConstructorsShouldBePrivateRule.cs
 
 tests_sources = \
 	ArrayFieldsShouldNotBeReadOnlyTest.cs \
 	DoNotShortCircuitCertificateCheckTest.cs \
 	NativeFieldsShouldNotBeVisibleTest.cs \
+	SqlCommandShouldUseNamedParametersTest.cs \
 	StaticConstructorsShouldBePrivateTest.cs 

gendarme/rules/Gendarme.Rules.Security/SqlCommandShouldUseNamedParameters.cs

@@ -0,0 +1,123 @@
+//
+// Gendarme.Rules.Concurrency.SqlCommandShouldUseNamedParameters
+//
+// Authors:
+//	Andres G. Aragoneses <[email protected]>
+//
+// Copyright (C) 2013 7digital Media, Ltd (http://www.7digital.com)
+//
+// Permission is hereby granted, free of charge, to any person obtaining
+// a copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to
+// permit persons to whom the Software is furnished to do so, subject to
+// the following conditions:
+//
+// The above copyright notice and this permission notice shall be
+// included in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+// LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+// OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+// WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+//
+
+using Gendarme.Framework;
+using Gendarme.Framework.Engines;
+using Gendarme.Framework.Helpers;
+using Gendarme.Framework.Rocks;
+using Mono.Cecil;
+using Mono.Cecil.Cil;
+
+namespace Gendarme.Rules.Security {
+
+	/// <summary>
+	/// This rule will fire if a method creates or uses an instance of
+	/// <c>System.Data.SqlClient.SqlCommand</c> with a command text that comes from 
+	/// a string that has been concatenated with another one. This is a very bad
+	/// idea because it is the most frequent cause of security holes regarding SQL
+	/// injection.
+	///
+	/// The preferred way to execute SQL queries that contain variable input
+	/// is using Named SQL Parameters.
+	/// </summary>
+	/// <example>
+	/// Bad example:
+	/// <code>
+	/// class BadExample {
+	/// 	public void EnableUser (string userName)
+	/// 	{
+	/// 		var sqlString = "UPDATE Users SET enabled = 1 WHERE UserName = " + userName;
+	/// 		using (var conn = new SqlConnection (connString)) {
+	/// 			var command = new SqlCommand (sqlString, conn);
+	/// 			command.Connection.Open ();
+	/// 			command.ExecuteNonQuery ();
+	/// 		}
+	/// 	}
+	/// }
+	/// </code>
+	/// </example>
+	/// <example>
+	/// Good example:
+	/// <code>
+	/// class GoodExample {
+	///
+	/// 	static readonly string sqlString = "UPDATE Users SET enabled = 1 WHERE UserName = @theUserName";
+	///
+	/// 	public void EnableUser (string userName)
+	/// 	{
+	/// 		using (var conn = new SqlConnection (connString)) {
+	/// 			var command = new SqlCommand (sqlString, conn);
+	/// 			command.Parameters.Add ("@theUserName", SqlDbType.VarChar).Value = userName;
+	/// 			command.Connection.Open ();
+	/// 			command.ExecuteNonQuery ();
+	/// 		}
+	/// 	}
+	/// }
+	/// </code>
+	/// </example>
+
+	[Problem ("This method uses a SqlCommand with a not constant command text, which is risky for SQL injection attacks")]
+	[Solution ("Use a const/readonly string for the command text; and use Named SQL parameters for the variable parts of it")]
+	[EngineDependency (typeof(OpCodeEngine))]
+	public class SqlCommandShouldUseNamedParameters : Rule, IMethodRule {
+
+		public RuleResult CheckMethod (MethodDefinition method)
+		{
+			// rule doesn't apply if the method has no IL
+			if (!method.HasBody)
+				return RuleResult.DoesNotApply;
+
+			// avoid looping if we're sure there's no call in the method
+			if (!OpCodeBitmask.Calls.Intersect (OpCodeEngine.GetBitmask (method)))
+				return RuleResult.DoesNotApply;
+
+			foreach (Instruction ins in method.Body.Instructions) {
+				if (ins.OpCode.FlowControl != FlowControl.Call)
+					continue;
+
+				var call = (ins.Operand as MethodReference);
+				if (call == null)
+					continue;
+
+				if (call.IsNamed ("System.Data.SqlClient", "SqlCommand", ".ctor") ||
+					call.IsNamed ("System.Data.SqlClient", "SqlCommand", "set_CommandText")) {
+					if (call.Parameters.Count > 0) {
+						var offset = call.IsProperty () ? 1 : 0;
+						var theString = ins.TraceBack (call, 0 - offset);
+						if (theString.OpCode.FlowControl == FlowControl.Call) {
+							return RuleResult.Failure;
+						}
+					}
+				}
+			}
+
+
+			return RuleResult.Success;
+		}
+	}
+}

gendarme/rules/Gendarme.Rules.Security/Test/SqlCommandShouldUseNamedParametersTest.cs

@@ -0,0 +1,151 @@
+//
+// Unit tests for SqlCommandShouldUseNamedParameters
+//
+// Authors:
+//	Andres G. Aragoneses <[email protected]>
+//
+// Copyright (C) 2013 7digital Media, Ltd (http://www.7digital.com)
+//
+// Permission is hereby granted, free of charge, to any person obtaining
+// a copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to
+// permit persons to whom the Software is furnished to do so, subject to
+// the following conditions:
+//
+// The above copyright notice and this permission notice shall be
+// included in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+// LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+// OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+// WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+//
+
+using System.Data.SqlClient;
+using Gendarme.Rules.Security;
+using NUnit.Framework;
+using Test.Rules.Definitions;
+using Test.Rules.Fixtures;
+
+//FAKE API so as not to reference System.Data
+namespace System.Data.SqlClient {
+
+	public class SqlCommand {
+		public SqlCommand (string commandText)
+		{
+		}
+
+		public SqlCommand ()
+		{
+		}
+
+		public SqlCommand (string commandText, SqlConnection conn)
+		{
+		}
+
+		public SqlCommand (string commandText, SqlConnection conn, SqlTransaction trans)
+		{
+		}
+
+		public string CommandText {
+			get { return null; }
+			set { }
+		}
+
+		public void ExecuteNonQuery ()
+		{
+		}
+	}
+
+	public class SqlConnection
+	{
+	}
+
+	public class SqlTransaction
+	{
+	}
+}
+
+namespace Test.Rules.Security {
+
+	[TestFixture]
+	public class SqlCommandShouldUseNamedParametersTest : MethodRuleTestFixture<SqlCommandShouldUseNamedParameters> {
+
+		[Test]
+		public void DoesNotApply ()
+		{
+			// no IL for p/invokes
+			AssertRuleDoesNotApply (SimpleMethods.ExternalMethod);
+			// no calls[virt]
+			AssertRuleDoesNotApply (SimpleMethods.EmptyMethod);
+
+			AssertRuleDoesNotApply<SqlExecutor> ("CallWithoutSqlCommandUsage");
+		}
+
+		public class SqlExecutor {
+			public static void CallWithoutSqlCommandUsage ()
+			{
+				var c = new SqlConnection ();
+			}
+
+			public static void CallsSqlCommandCtorWithVariableString (string param)
+			{
+				new SqlCommand ("some SQL query " + param).ExecuteNonQuery ();
+			}
+
+			private static SqlConnection conn;
+			public static void CallsSqlCommandCtorWithVariableStringAndAConn (string param)
+			{
+				new SqlCommand ("some SQL query " + param, conn).ExecuteNonQuery ();
+			}
+
+			private const string aField = "some SQL query from a field";
+			private static SqlCommand commandField;
+			public static void CallsSqlCommandPropertyWithVariableString (string param)
+			{
+				commandField.CommandText = aField + param;
+				commandField.ExecuteNonQuery ();
+			}
+
+			public static void CallsSqlCommandEmptyCtor ()
+			{
+				new SqlCommand ().ExecuteNonQuery ();
+			}
+
+			private const string someField = "some SQL query from a field";
+			public static void CallsSqlCommandCtorWithConstantString ()
+			{
+				new SqlCommand (someField).ExecuteNonQuery ();
+			}
+
+			private const string someOtherField = "some SQL query from a field";
+			public static void CallsSqlCommandPropertyWithConstantString ()
+			{
+				var c = new SqlCommand ();
+				c.CommandText = someOtherField;
+				c.ExecuteNonQuery ();
+			}
+		}
+
+		[Test]
+		public void CheckFailures (string method)
+		{
+			AssertRuleFailure<SqlExecutor> ("CallsSqlCommandCtorWithVariableString");
+			AssertRuleFailure<SqlExecutor> ("CallsSqlCommandCtorWithVariableStringAndAConn");
+			AssertRuleFailure<SqlExecutor> ("CallsSqlCommandPropertyWithVariableString");
+		}
+
+		[Test]
+		public void CheckSuccesses (string method)
+		{
+			AssertRuleSuccess<SqlExecutor> ("CallsSqlCommandCtorWithConstantString");
+			AssertRuleSuccess<SqlExecutor> ("CallsSqlCommandEmptyCtor");
+			AssertRuleSuccess<SqlExecutor> ("CallsSqlCommandPropertyWithConstantString");
+		}
+	}
+}

gendarme/rules/Gendarme.Rules.Security/Test/Tests.Rules.Security.csproj

@@ -68,6 +68,7 @@
     <Compile Include="ArrayFieldsShouldNotBeReadOnlyTest.cs" />
     <Compile Include="DoNotShortCircuitCertificateCheckTest.cs" />
     <Compile Include="NativeFieldsShouldNotBeVisibleTest.cs" />
+    <Compile Include="SqlCommandShouldUseNamedParametersTest.cs" />
     <Compile Include="StaticConstructorsShouldBePrivateTest.cs" />
   </ItemGroup>
   <ItemGroup>