From 7c9d3ea9140225c42a674bb5328bd5de73055288 Mon Sep 17 00:00:00 2001 From: "Andres G. Aragoneses" Date: Fri, 17 May 2013 13:20:51 +0100 Subject: [PATCH 1/3] [Gendarme] New Security rule: SqlCommandShouldUseNamedParameters This rule will report failures on source code prone to SQL injection attacks. It includes basic tests that cover SqlCommand usage via constructor and CommandText property. Contributed under the MIT/X11 licence. --- .../Gendarme.Rules.Security.csproj | 1 + .../rules/Gendarme.Rules.Security/Makefile.am | 2 + .../SqlCommandShouldUseNamedParameters.cs | 123 ++++++++++++++ .../SqlCommandShouldUseNamedParametersTest.cs | 151 ++++++++++++++++++ .../Test/Tests.Rules.Security.csproj | 1 + 5 files changed, 278 insertions(+) create mode 100644 gendarme/rules/Gendarme.Rules.Security/SqlCommandShouldUseNamedParameters.cs create mode 100644 gendarme/rules/Gendarme.Rules.Security/Test/SqlCommandShouldUseNamedParametersTest.cs 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..cbfdef5e2 --- /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 (); + } + } + + [TestCase ("CallsSqlCommandCtorWithVariableString")] + [TestCase ("CallsSqlCommandCtorWithVariableStringAndAConn")] + [TestCase ("CallsSqlCommandPropertyWithVariableString")] + public void CheckFailures (string method) + { + AssertRuleFailure (method); + } + + [TestCase ("CallsSqlCommandCtorWithConstantString")] + [TestCase ("CallsSqlCommandEmptyCtor")] + [TestCase ("CallsSqlCommandPropertyWithConstantString")] + public void CheckSuccesses (string method) + { + AssertRuleSuccess (method); + } + } +} 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 @@ + From 7ff3a461df64842a1f60f23d360c68b8861fa236 Mon Sep 17 00:00:00 2001 From: "Andres G. Aragoneses" Date: Fri, 17 May 2013 13:27:52 +0100 Subject: [PATCH 2/3] [Gendarme] Stop using [TestCase] as it's NUnit 2.5 specific --- .../SqlCommandShouldUseNamedParametersTest.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/gendarme/rules/Gendarme.Rules.Security/Test/SqlCommandShouldUseNamedParametersTest.cs b/gendarme/rules/Gendarme.Rules.Security/Test/SqlCommandShouldUseNamedParametersTest.cs index cbfdef5e2..6bcfbeecd 100644 --- a/gendarme/rules/Gendarme.Rules.Security/Test/SqlCommandShouldUseNamedParametersTest.cs +++ b/gendarme/rules/Gendarme.Rules.Security/Test/SqlCommandShouldUseNamedParametersTest.cs @@ -132,20 +132,20 @@ public static void CallsSqlCommandPropertyWithConstantString () } } - [TestCase ("CallsSqlCommandCtorWithVariableString")] - [TestCase ("CallsSqlCommandCtorWithVariableStringAndAConn")] - [TestCase ("CallsSqlCommandPropertyWithVariableString")] + [Test] public void CheckFailures (string method) { - AssertRuleFailure (method); + AssertRuleFailure ("CallsSqlCommandCtorWithVariableString"); + AssertRuleFailure ("CallsSqlCommandCtorWithVariableStringAndAConn"); + AssertRuleFailure ("CallsSqlCommandPropertyWithVariableString"); } - [TestCase ("CallsSqlCommandCtorWithConstantString")] - [TestCase ("CallsSqlCommandEmptyCtor")] - [TestCase ("CallsSqlCommandPropertyWithConstantString")] + [Test] public void CheckSuccesses (string method) { - AssertRuleSuccess (method); + AssertRuleSuccess ("CallsSqlCommandCtorWithConstantString"); + AssertRuleSuccess ("CallsSqlCommandEmptyCtor"); + AssertRuleSuccess ("CallsSqlCommandPropertyWithConstantString"); } } } From d8839d56609fe2300469daaeccde17b140b4a7c2 Mon Sep 17 00:00:00 2001 From: "Andres G. Aragoneses" Date: Fri, 17 May 2013 13:28:15 +0100 Subject: [PATCH 3/3] [Gendarme] Fix my email in AUTHORS I don't work for Novell anymore --- gendarme/AUTHORS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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