Skip to content

Commit 490957a

Browse files
raulgarciamsftraulgarciamsft
authored
Merge pull request #117 from microsoft/SqlConnFP_fix
Fixing a false positive in cs/insecure-sql-connection
2 parents 7ad49cf + 97bfc5d commit 490957a

11 files changed

+187
-2
lines changed

‎csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.ql

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
import csharp
1414
import InsecureSqlConnection::PathGraph
15+
import InsecureSQLConnection
1516

1617
class Source extends DataFlow::Node {
1718
string sourcestring;
@@ -48,6 +49,11 @@ class Sink extends DataFlow::Node {
4849
predicate isEncryptTrue(Source source, Sink sink) {
4950
sink.isEncryptedByDefault() and
5051
not source.setsEncryptFalse()
52+
or
53+
exists(ObjectCreation oc, Expr e | oc.getRuntimeArgument(0) = sink.asExpr() |
54+
getInfoForInitializedConnEncryption(oc, e) and
55+
e.getValue().toLowerCase() = "true"
56+
)
5157
}
5258

5359
/**

‎csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.qll

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import csharp
2+
3+
/**
4+
* Holds if `ObjectCreation` has an initializer for a member named `Encrypt`, set to `e`
5+
*/
6+
predicate getInfoForInitializedConnEncryption(ObjectCreation oc, Expr e) {
7+
exists(MemberInitializer mi | mi.getInitializedMember().hasName("Encrypt") |
8+
e = mi.getRValue() and
9+
oc.getInitializer().(ObjectInitializer).getAMemberInitializer() = mi
10+
)
11+
}

‎csharp/ql/src/Security Features/CWE-327/InsecureSQLConnectionInitializer.qhelp

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
7+
<p>
8+
SQL Server connections where the client is not enforcing the encryption in transit are susceptible to multiple attacks, including a man-in-the-middle, that would potentially compromise the user credentials and/or the TDS session.
9+
</p>
10+
11+
</overview>
12+
<recommendation>
13+
14+
<p>Ensure that the client code enforces the <code>Encrypt</code> option by setting it to <code>true</code> in the connection string or as a property.</p>
15+
<p>Explicitly setting the property <code>Encrypt</code> to <code>false</code> will result in unprotected connections.</p>
16+
17+
</recommendation>
18+
<example>
19+
20+
<p>The following example shows a SQL connection string that is explicitly disabling the <code>Encrypt</code> setting.</p>
21+
22+
<sample src="InsecureSQLConnectionInitializerBad.cs" />
23+
24+
<p>
25+
The following example shows a SQL connection string that is explicitly enabling the <code>Encrypt</code> setting to force encryption in transit.
26+
</p>
27+
28+
<sample src="InsecureSQLConnectionInitializerGood.cs" />
29+
30+
</example>
31+
<references>
32+
<li>Microsoft, SQL Protocols blog:
33+
<a href="https://blogs.msdn.microsoft.com/sql_protocols/2009/10/19/selectively-using-secure-connection-to-sql-server/">Selectively using secure connection to SQL Server</a>.
34+
</li>
35+
<li>Microsoft:
36+
<a href="https://msdn.microsoft.com/en-us/library/system.data.sqlclient.sqlconnection.connectionstring(v=vs.110).aspx">SqlConnection.ConnectionString Property</a>.
37+
</li>
38+
<li>Microsoft:
39+
<a href="https://msdn.microsoft.com/en-us/library/ms130822.aspx">Using Connection String Keywords with SQL Server Native Client</a>.
40+
</li>
41+
<li>Microsoft:
42+
<a href="https://msdn.microsoft.com/en-us/library/ms378988(v=sql.110).aspx">Setting the connection properties</a>.
43+
</li>
44+
</references>
45+
</qhelp>

‎csharp/ql/src/Security Features/CWE-327/InsecureSQLConnectionInitializer.ql

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
/**
2+
* @name Insecure SQL connection in Initializer
3+
* @description Using an SQL Server connection without enforcing encryption is a security vulnerability.
4+
* This rule variant will flag when the `encrypt` property is explicitly set to `false` during the object initializer
5+
* @kind path-problem
6+
* @id cs/insecure-sql-connection-initializer
7+
* @problem.severity error
8+
* @security-severity 7.5
9+
* @precision medium
10+
* @tags security
11+
* external/cwe/cwe-327
12+
*/
13+
14+
import csharp
15+
import InsecureSqlConnectionInitialize::PathGraph
16+
import InsecureSQLConnection
17+
18+
class Source extends DataFlow::Node {
19+
Source() { this.asExpr().(BoolLiteral).getBoolValue() = false }
20+
}
21+
22+
class Sink extends DataFlow::Node {
23+
Sink() { getInfoForInitializedConnEncryption(_, this.asExpr()) }
24+
}
25+
26+
/**
27+
* A data flow configuration for tracking strings passed to `SqlConnection[StringBuilder]` instances.
28+
*/
29+
module InsecureSqlConnectionInitializeConfig implements DataFlow::ConfigSig {
30+
predicate isSource(DataFlow::Node source) { source instanceof Source }
31+
32+
predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
33+
}
34+
35+
/**
36+
* A data flow configuration for tracking strings passed to `SqlConnection[StringBuilder]` instances.
37+
*/
38+
module InsecureSqlConnectionInitialize = DataFlow::Global<InsecureSqlConnectionInitializeConfig>;
39+
40+
from
41+
ObjectCreation oc, InsecureSqlConnectionInitialize::PathNode source,
42+
InsecureSqlConnectionInitialize::PathNode sink
43+
where
44+
InsecureSqlConnectionInitialize::flowPath(source, sink) and
45+
getInfoForInitializedConnEncryption(oc, sink.getNode().asExpr())
46+
select sink.getNode(), source, sink,
47+
"A value evaluating to $@ flows to $@ and sets the `encrypt` property.", source.getNode(),
48+
"`false`", oc, "this SQL connection initializer"

‎csharp/ql/src/Security Features/CWE-327/InsecureSQLConnectionInitializerBad.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
using System.Data.SqlClient;
2+
3+
// BAD, Encrypt not specified
4+
string connectString =
5+
"Server=1.2.3.4;Database=Anything;Integrated Security=true;";
6+
var builder = new SqlConnectionStringBuilder(connectString) { Encrypt = false }
7+
var conn = new SqlConnection(builder.ConnectionString);

‎csharp/ql/src/Security Features/CWE-327/InsecureSQLConnectionInitializerGood.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
using System.Data.SqlClient;
2+
3+
// BAD, Encrypt not specified
4+
string connectString =
5+
"Server=1.2.3.4;Database=Anything;Integrated Security=true;";
6+
var builder = new SqlConnectionStringBuilder(connectString) { Encrypt = true }
7+
var conn = new SqlConnection(builder.ConnectionString);

‎csharp/ql/test/query-tests/Security Features/CWE-327/InsecureSQLConnection/InsecureSQLConnection.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,9 @@ public void StringInBuilderProperty()
3434
public void StringInInitializer()
3535
{
3636
string connectString = "Server=1.2.3.4;Database=Anything;UID=ab;Pwd=cd;Encrypt=false";
37-
SqlConnectionStringBuilder conBuilder = new SqlConnectionStringBuilder(connectString) { Encrypt = true};
37+
SqlConnectionStringBuilder conBuilder = new SqlConnectionStringBuilder(connectString) { Encrypt = true };
3838
}
39-
39+
4040

4141
public void TriggerThis()
4242
{

‎csharp/ql/test/query-tests/Security Features/CWE-327/InsecureSQLConnection/InsecureSQLConnection.expected

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,14 @@
11
edges
2+
| InsecureSQLConnection.cs:36:20:36:32 | access to local variable connectString : String | InsecureSQLConnection.cs:37:84:37:96 | access to local variable connectString | provenance | |
3+
| InsecureSQLConnection.cs:36:36:36:97 | "Server=1.2.3.4;Database=Anything;UID=ab;Pwd=cd;Encrypt=false" : String | InsecureSQLConnection.cs:36:20:36:32 | access to local variable connectString : String | provenance | |
24
| InsecureSQLConnection.cs:49:20:49:32 | access to local variable connectString : String | InsecureSQLConnection.cs:52:81:52:93 | access to local variable connectString | provenance | |
35
| InsecureSQLConnection.cs:50:17:50:64 | "Server=1.2.3.4;Database=Anything;UID=ab;Pwd=cd" : String | InsecureSQLConnection.cs:49:20:49:32 | access to local variable connectString : String | provenance | |
46
| InsecureSQLConnection.cs:58:20:58:32 | access to local variable connectString : String | InsecureSQLConnection.cs:61:81:61:93 | access to local variable connectString | provenance | |
57
| InsecureSQLConnection.cs:59:17:59:78 | "Server=1.2.3.4;Database=Anything;UID=ab;Pwd=cd;Encrypt=false" : String | InsecureSQLConnection.cs:58:20:58:32 | access to local variable connectString : String | provenance | |
68
nodes
9+
| InsecureSQLConnection.cs:36:20:36:32 | access to local variable connectString : String | semmle.label | access to local variable connectString : String |
10+
| InsecureSQLConnection.cs:36:36:36:97 | "Server=1.2.3.4;Database=Anything;UID=ab;Pwd=cd;Encrypt=false" : String | semmle.label | "Server=1.2.3.4;Database=Anything;UID=ab;Pwd=cd;Encrypt=false" : String |
11+
| InsecureSQLConnection.cs:37:84:37:96 | access to local variable connectString | semmle.label | access to local variable connectString |
712
| InsecureSQLConnection.cs:44:52:44:128 | "Server=myServerName\\myInstanceName;Database=myDataBase;User Id=myUsername;" | semmle.label | "Server=myServerName\\myInstanceName;Database=myDataBase;User Id=myUsername;" |
813
| InsecureSQLConnection.cs:49:20:49:32 | access to local variable connectString : String | semmle.label | access to local variable connectString : String |
914
| InsecureSQLConnection.cs:50:17:50:64 | "Server=1.2.3.4;Database=Anything;UID=ab;Pwd=cd" : String | semmle.label | "Server=1.2.3.4;Database=Anything;UID=ab;Pwd=cd" : String |

‎csharp/ql/test/query-tests/Security Features/CWE-327/InsecureSQLConnectionInitializer/InsecureSQLConnectionInitializer.cs

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
namespace System.Data.SqlClient
2+
{
3+
public sealed class SqlConnectionStringBuilder
4+
{
5+
public bool Encrypt { get; set; }
6+
public SqlConnectionStringBuilder(string connectionString) { }
7+
}
8+
9+
}
10+
11+
namespace InsecureSQLConnection
12+
{
13+
public class Class1
14+
{
15+
void Test6()
16+
{
17+
string connectString = "Server=1.2.3.4;Database=Anything;UID=ab;Pwd=cd;Encrypt=false";
18+
var conn = new System.Data.SqlClient.SqlConnectionStringBuilder(connectString) { Encrypt = false }; // Bug - cs/insecure-sql-connection-initializer
19+
}
20+
21+
void Test72ndPhase(bool encrypt)
22+
{
23+
string connectString = "Server=1.2.3.4;Database=Anything;UID=ab;Pwd=cd;Encrypt=false";
24+
var conn = new System.Data.SqlClient.SqlConnectionStringBuilder(connectString) { Encrypt = encrypt }; // Bug - cs/insecure-sql-connection-initializer (sink)
25+
}
26+
27+
void Test7()
28+
{
29+
Test72ndPhase(false); // Bug - cs/insecure-sql-connection-initializer (source)
30+
}
31+
32+
void Test7FP()
33+
{
34+
Test72ndPhase(true); // Not a bug source
35+
}
36+
37+
void Test8FP()
38+
{
39+
string connectString = "Server=1.2.3.4;Database=Anything;UID=ab;Pwd=cd;Encrypt=false";
40+
var conn = new System.Data.SqlClient.SqlConnectionStringBuilder(connectString) { Encrypt = true };
41+
}
42+
}
43+
}

‎csharp/ql/test/query-tests/Security Features/CWE-327/InsecureSQLConnectionInitializer/InsecureSQLConnectionInitializer.expected

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
edges
2+
| InsecureSQLConnectionInitializer.cs:21:33:21:39 | encrypt : Boolean | InsecureSQLConnectionInitializer.cs:24:104:24:110 | access to parameter encrypt | provenance | |
3+
| InsecureSQLConnectionInitializer.cs:29:27:29:31 | false : Boolean | InsecureSQLConnectionInitializer.cs:21:33:21:39 | encrypt : Boolean | provenance | |
4+
nodes
5+
| InsecureSQLConnectionInitializer.cs:18:104:18:108 | false | semmle.label | false |
6+
| InsecureSQLConnectionInitializer.cs:21:33:21:39 | encrypt : Boolean | semmle.label | encrypt : Boolean |
7+
| InsecureSQLConnectionInitializer.cs:24:104:24:110 | access to parameter encrypt | semmle.label | access to parameter encrypt |
8+
| InsecureSQLConnectionInitializer.cs:29:27:29:31 | false : Boolean | semmle.label | false : Boolean |
9+
subpaths
10+
#select
11+
| InsecureSQLConnectionInitializer.cs:18:104:18:108 | false | InsecureSQLConnectionInitializer.cs:18:104:18:108 | false | InsecureSQLConnectionInitializer.cs:18:104:18:108 | false | A value evaluating to $@ flows to $@ and sets the `encrypt` property. | InsecureSQLConnectionInitializer.cs:18:104:18:108 | false | `false` | InsecureSQLConnectionInitializer.cs:18:24:18:110 | object creation of type SqlConnectionStringBuilder | this SQL connection initializer |
12+
| InsecureSQLConnectionInitializer.cs:24:104:24:110 | access to parameter encrypt | InsecureSQLConnectionInitializer.cs:29:27:29:31 | false : Boolean | InsecureSQLConnectionInitializer.cs:24:104:24:110 | access to parameter encrypt | A value evaluating to $@ flows to $@ and sets the `encrypt` property. | InsecureSQLConnectionInitializer.cs:29:27:29:31 | false | `false` | InsecureSQLConnectionInitializer.cs:24:24:24:112 | object creation of type SqlConnectionStringBuilder | this SQL connection initializer |

0 commit comments

Comments
 (0)