diff --git a/gendarme/AUTHORS b/gendarme/AUTHORS index 4acb9738e..f5e0bc58a 100644 --- a/gendarme/AUTHORS +++ b/gendarme/AUTHORS @@ -12,7 +12,7 @@ Adrian Tsai Seo Sanghyeon Scott Peterson Cedric Vivier -Andres G. Aragoneses +Andres G. Aragoneses Peter Johanson Jesse Jones Rolf Bjarne Kvinge diff --git a/gendarme/rules/Gendarme.Rules.Security/Gendarme.Rules.Security.csproj b/gendarme/rules/Gendarme.Rules.Security/Gendarme.Rules.Security.csproj index 62fc565fb..d1a68d69f 100755 --- a/gendarme/rules/Gendarme.Rules.Security/Gendarme.Rules.Security.csproj +++ b/gendarme/rules/Gendarme.Rules.Security/Gendarme.Rules.Security.csproj @@ -71,6 +71,7 @@ + diff --git a/gendarme/rules/Gendarme.Rules.Security/Makefile.am b/gendarme/rules/Gendarme.Rules.Security/Makefile.am index 02b17f6d0..34fe40c2c 100644 --- a/gendarme/rules/Gendarme.Rules.Security/Makefile.am +++ b/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 diff --git a/gendarme/rules/Gendarme.Rules.Security/SqlCommandShouldUseNamedParameters.cs b/gendarme/rules/Gendarme.Rules.Security/SqlCommandShouldUseNamedParameters.cs new file mode 100644 index 000000000..f94f46827 --- /dev/null +++ b/gendarme/rules/Gendarme.Rules.Security/SqlCommandShouldUseNamedParameters.cs @@ -0,0 +1,123 @@ +// +// Gendarme.Rules.Concurrency.SqlCommandShouldUseNamedParameters +// +// Authors: +// Andres G. Aragoneses +// +// 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 { + + /// + /// This rule will fire if a method creates or uses an instance of + /// System.Data.SqlClient.SqlCommand 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. + /// + /// + /// Bad example: + /// + /// 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 (); + /// } + /// } + /// } + /// + /// + /// + /// Good example: + /// + /// 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 (); + /// } + /// } + /// } + /// + /// + + [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; + } + } +} diff --git a/gendarme/rules/Gendarme.Rules.Security/Test/SqlCommandShouldUseNamedParametersTest.cs b/gendarme/rules/Gendarme.Rules.Security/Test/SqlCommandShouldUseNamedParametersTest.cs new file mode 100644 index 000000000..6bcfbeecd --- /dev/null +++ b/gendarme/rules/Gendarme.Rules.Security/Test/SqlCommandShouldUseNamedParametersTest.cs @@ -0,0 +1,151 @@ +// +// Unit tests for SqlCommandShouldUseNamedParameters +// +// Authors: +// Andres G. Aragoneses +// +// 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 { + + [Test] + public void DoesNotApply () + { + // no IL for p/invokes + AssertRuleDoesNotApply (SimpleMethods.ExternalMethod); + // no calls[virt] + AssertRuleDoesNotApply (SimpleMethods.EmptyMethod); + + AssertRuleDoesNotApply ("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 ("CallsSqlCommandCtorWithVariableString"); + AssertRuleFailure ("CallsSqlCommandCtorWithVariableStringAndAConn"); + AssertRuleFailure ("CallsSqlCommandPropertyWithVariableString"); + } + + [Test] + public void CheckSuccesses (string method) + { + AssertRuleSuccess ("CallsSqlCommandCtorWithConstantString"); + AssertRuleSuccess ("CallsSqlCommandEmptyCtor"); + AssertRuleSuccess ("CallsSqlCommandPropertyWithConstantString"); + } + } +} diff --git a/gendarme/rules/Gendarme.Rules.Security/Test/Tests.Rules.Security.csproj b/gendarme/rules/Gendarme.Rules.Security/Test/Tests.Rules.Security.csproj index 2568557b9..a76a873a4 100755 --- a/gendarme/rules/Gendarme.Rules.Security/Test/Tests.Rules.Security.csproj +++ b/gendarme/rules/Gendarme.Rules.Security/Test/Tests.Rules.Security.csproj @@ -68,6 +68,7 @@ +