Semgrep で SQL Injection と戦う #1
Semgrep で SQL Injection と戦う #1
Semgrep はソースコードの静的解析ツールです。どんなものか知りたい方は .NET でも .NET でなくても静的セキュリティコード検査 (Semgrep 編) を参考にしてください。
タイトルの通りです。Semgrep の公式ルールは .NET の O/R マッパーの Dapper に対応していないので、Dapper 用のルールを書いてみます。
途中経過とかどうでもいい! 使えればいいんじゃ! って人は、読み飛ばしてページの下までスクロールしてください。
実際のところ、この手の SAST ツールはちょっとコードの書き方を変えただけで検出できなくなることが多いです。現場によっては脆弱性がたくさんあっても、検出できないことも多いです。自分たちの現場に合わせたルールを整備し続けるのが大事だと思います。
ルールを作る
少しずつルールを強化していきます。
もっとも単純なケース
よくある SQL Injection なコードです。// unsafe
の記述があるところが安全でないコードです。
public void Test(string id)
{
using var connection = new SqliteConnection(":memory:");
connection.Execute("select * from customers where id = 1"); // safe
connection.Execute("select * from customers where id = '" + id + "'"); // unsafe
connection.Execute($"select * from customers where id = '{id}'"); // unsafe
connection.Execute(@$"
select * from customers where id = '{id}';
"); // unsafe
}
まずはこれを検出できるようにします。
---
rules:
- id: csharp/sql-injection
message: SQL Injection
patterns:
- pattern: $CONNECTION.Execute($ARG, ...);
languages: [csharp]
severity: ERROR
pattern
はこんな感じです。hit
と記述があるところがヒットするようになります。
Execute(sql); // not hit
a.Execute(); // not hit
a.Execute(sql); // hit
a.Execute(sql, "..."); // hit
実行してみます。
$ semgrep --config=semgrep.yml Vulnerability/Pattern1.cs
Scanning 1 file.
Findings:
Vulnerability/Pattern1.cs
csharp/sql-injection
SQL Injection
11┆ connection.Execute("select * from customers where id = '1'"); // safe
⋮┆----------------------------------------
12┆ connection.Execute("select * from customers where id = `" + id + "`"); // unsafe
⋮┆----------------------------------------
13┆ connection.Execute($"select * from customers where id = '{id}'"); // unsafe
⋮┆----------------------------------------
14┆ connection.Execute(@$"
15┆ select * from customers where id = '{id}';
16┆ "); // unsafe
Ran 1 rule on 1 file: 4 findings.
残念ながら、安全な connection.Execute("select * from customers where id = '1'"); // safe
もマッチしました。pattern-not
を利用して、これを除外するようにします。
---
rules:
- id: csharp/sql-injection
message: SQL Injection
patterns:
- pattern: $CONNECTION.Execute($ARG, ...);
- pattern-not: $CONNECTION.Execute("...", ...);
languages: [csharp]
severity: ERROR
再度検査してみると、今度はうまくいきました!
semgrep --config=semgrep.yml Vulnerability/Pattern1.cs
Scanning 1 file.
Findings:
Vulnerability/Pattern1.cs
csharp/sql-injection
SQL Injection
12┆ connection.Execute("select * from customers where id = `" + id + "`"); // unsafe
⋮┆----------------------------------------
13┆ connection.Execute($"select * from customers where id = '{id}'"); // unsafe
⋮┆----------------------------------------
14┆ connection.Execute(@$"
15┆ select * from customers where id = '{id}';
16┆ "); // unsafe
Ran 1 rule on 1 file: 3 findings.
Execute() 以外への対応
Dappter は Execute()
以外に、ExecuteAsync()
などのメソッドが用意されています。それらを検出できるようにします。
public void Test(string id)
{
using var connection = new SqliteConnection(":memory:");
connection.Execute($"select * from customers where id = '{id}'"); // unsafe
connection.ExecuteReader($"select * from customers where id = '{id}'"); // unsafe
connection.ExecuteScalar($"select * from customers where id = '{id}'"); // unsafe
connection.Query($"select * from customers where id = '{id}'"); // unsafe
connection.QueryFirst($"select * from customers where id = '{id}'"); // unsafe
connection.QueryFirstOrDefault($"select * from customers where id = '{id}'"); // unsafe
connection.QuerySingle($"select * from customers where id = '{id}'"); // unsafe
connection.QuerySingleOrDefault($"select * from customers where id = '{id}'"); // unsafe
connection.QueryMultiple($"select * from customers where id = '{id}'"); // unsafe
connection.ExecuteAsync($"select * from customers where id = '{id}'"); // unsafe
connection.ExecuteScalar<int>($"select * from customers where id = '{id}'"); // unsafe
}
あたりまえですけど、Execute()
しか検出できません。
Vulnerability/Pattern2.cs
csharp/sql-injection
SQL Injection
12┆ connection.Execute($"select * from customers where id = '{id}'"); // unsafe
ルールを修正します。すべてのメソッド名を記述するのは面倒なので、metavariable-regex
を利用して、メソッド名を正規表現にマッチさせます。
---
rules:
- id: csharp/sql-injection
message: SQL Injection
patterns:
- pattern: $CONNECTION.$EXECUTE($ARG, ...);
- pattern-not: $CONNECTION.$EXECUTE("...", ...);
- metavariable-regex:
metavariable: $EXECUTE
regex: ^(Execute|Query)
languages: [csharp]
severity: ERROR
これで Execute
か Query
で始まるメソッドだけが検出できます。厳密にマッチさせるには、regex: ^(Execute|Query(|First|FirstOrDefault|Single|SingleOrDefault|Multiple)(|Async)$
のように記述した方がいいのでしょうけど、Dapper が将来メソッドを追加したときのためにこれくらい緩いルールがいいでしょう。
12┆ connection.Execute($"select * from customers where id = '{id}'"); // unsafe
⋮┆----------------------------------------
13┆ connection.ExecuteReader($"select * from customers where id = '{id}'"); // unsafe
⋮┆----------------------------------------
14┆ connection.ExecuteScalar($"select * from customers where id = '{id}'"); // unsafe
⋮┆----------------------------------------
16┆ connection.Query($"select * from customers where id = '{id}'"); // unsafe
⋮┆----------------------------------------
17┆ connection.QueryFirst($"select * from customers where id = '{id}'"); // unsafe
⋮┆----------------------------------------
18┆ connection.QueryFirstOrDefault($"select * from customers where id = '{id}'"); // unsafe
⋮┆----------------------------------------
19┆ connection.QuerySingle($"select * from customers where id = '{id}'"); // unsafe
⋮┆----------------------------------------
20┆ connection.QuerySingleOrDefault($"select * from customers where id = {id}"); // unsafe
⋮┆----------------------------------------
21┆ connection.QueryMultiple($"select * from customers where id = '{id}'"); // unsafe
⋮┆----------------------------------------
23┆ connection.ExecuteAsync($"select * from customers where id = '{id}'"); // unsafe
⋮┆----------------------------------------
25┆ connection.ExecuteScalar<int>($"select * from customers where id = '{id}'"); // unsafe
foo.Execute() が検出されないように
今のままでは、SqliteConnection
オブジェクト以外でも検出されます。Execute()
なんてありふれた名前ですので、誤検出だらけになります。もちろん、// nosemgrep
で無視することはできるんですけど、無視するのが一般的になると... ねぇ?
検出されてほしくないコードです。
public void Test(string type)
{
var foo = new Foo();
foo.Execute(type); // safe
}
検査すると、残念ながら検出されます。
8┆ foo.Execute(type); // safe
検出されないようルールを書き換えます。オブジェクトの型を SqliteConnection
に限定するようにしました。
---
rules:
- id: csharp/sql-injection
message: SQL Injection
patterns:
- pattern: ($TYPE $CONNECTION).$EXECUTE($ARG, ...);
- pattern-not: ($TYPE $CONNECTION).$EXECUTE("...", ...);
- metavariable-regex:
metavariable: $EXECUTE
regex: ^(Execute|Query)
- metavariable-regex:
metavariable: $TYPE
regex: ^SqliteConnection$
languages: [csharp]
severity: ERROR
検査すると、無事検出されなくなりました。
Ran 1 rule on 1 file: 0 findings.
一時変数代入に対応する
Semgrep は基本 AST による解析ですのですが、ある程度コードを追いかけます。とてもありがたいです。でも、どこまで追いかけるのかわからないので、一時変数を利用したコードで対応できるか確かめてみます。
public void Test(string id)
{
var connection = new SqliteConnection(":memory:");
var s1 = "select * from customers where id = '1'";
connection.Execute(s1); // safe
const string s2 = "select * from customers where id = '1'";
connection.Execute(s2); // safe
var s3 = $"select * from customers where id = '{id}'";
connection.Execute(s3); // unsafe
var conn = connection;
conn.Execute($"select * from customers where id = '{id}'"); // unsafe
}
これは一発で通りました!
18┆ connection.Execute(s3); // unsafe
⋮┆----------------------------------------
21┆ conn.Execute($"select * from customers where id = '{id}'"); // unsafe
s1
と s2
は connection.Execute("...")
と解釈されるようです。便利!
DbProviderFactory 対応
new SqliteConnection("...")
とコネクションクラスのインスタンス化を開発者が書く現場ってあまりないのではないでしょうか? Sqlite3 から他のデータベースに切り替える時に大変です。.NET には 'DbProviderFactory' という便利なものがあるので、これを使っている現場も多いでしょう。
public void Test1(string id)
{
using IDbConnection connection = DbProviderFactories.GetFactory("Default").CreateConnection();
connection.Execute($"select * from customers where id = '{id}'"); // unsafe (Test1)
}
public void Test2(string id)
{
using var connection = DbProviderFactories.GetFactory("Default").CreateConnection();
connection.Execute($"select * from customers where id = '{id}'"); // unsafe (Test2)
}
検査しても検出できません...
Ran 1 rule on 1 file: 0 findings.
今は SqliteConnection
だけに反応しているので、これを DbProviderFactory.CreateConnection()
の戻り値の型、IDbConnection
をルールに追加してみます。
patterns:
- pattern: ($TYPE $CONNECTION).$EXECUTE($ARG, ...);
- pattern-not: ($TYPE $CONNECTION).$EXECUTE("...", ...);
- metavariable-regex:
metavariable: $EXECUTE
regex: ^(Execute|Query)
- metavariable-regex:
metavariable: $TYPE
regex: ^(Sqlite|IDb)Connection$
検査してみます。
12┆ connection.Execute($"select * from customers where id = {id}"); // unsafe (Test1)
残念ながら、Semgrep は DbProviderFactory.CreateConnection()
の戻り値の型がわからず、うまく検出できないようです。
さすがに一つのルールで対応するのは難しいので、pattern-either
(OR) を利用して DbProviderFactory
ありとなしのルールを別々で記述します。
---
rules:
- id: csharp/sql-injection
message: SQL Injection
pattern-either:
- patterns:
- pattern: ($TYPE $CONNECTION).$EXECUTE($ARG, ...);
- pattern-not: ($TYPE $CONNECTION).$EXECUTE("...", ...);
- metavariable-regex:
metavariable: $EXECUTE
regex: ^(Execute|Query)
- metavariable-regex:
metavariable: $TYPE
regex: ^(Sqlite|IDb)Connection$
- patterns:
- pattern: $CONNECTION.$EXECUTE($ARG, ...);
- pattern-not: ($TYPE $CONNECTION).$EXECUTE("...", ...);
- metavariable-regex:
metavariable: $EXECUTE
regex: ^(Execute|Query)
- pattern-inside: |
var $CONNECTION = DbProviderFactories.GetFactory(...).CreateConnection();
...
languages: [csharp]
severity: ERROR
pattern-inside
を利用すると、DbProviderFactories.GetFactory(...).CreateConnection()
の戻り値を使った部分から選ぶといったことを指定できます。
pattern-inside: |
var $CONNECTION = DbProviderFactories.GetFactory(...).CreateConnection(); ...
ちゃんと検出できるようになりました!
⋮┆----------------------------------------
19┆ connection.Execute($"select * from customers where id = '{id}'"); // unsafe (Test2)
独自の IDbConnection ファクトリ
IDbConnection
インスタンスのファクトリを独自に用意している現場も多いかと思います。
using var connection = Database.Connection();
connection.Execute("....");
こういうパターンは Semgrep の公式ルールなどの汎用ルールではほぼ検出できません。DbProviderFactory
に対応させたように、公式ルールを使うにしても独自のカスタマイズが必要になります。
DbProviderFactory もうちょっと
DbProviderFactory
で誤検出や未検出しそうなパターンを確かめてみます。
-
DbProviderFactory
インスタンスを一時変数に代入する -
DbProviderFactory.CreateConnection()
の戻り値とは別のオブジェクトのobj.Execute()
を呼び出す。
public void Test1(string id)
{
var factory = DbProviderFactories.GetFactory("Default");
using var connection = factory.CreateConnection();
connection.Execute($"select * from customers id = '{id}'"); // unsafe (Test1)
}
public void Test2(string id)
{
using var _ = DbProviderFactories.GetFactory("Default").CreateConnection();
var connection = new Foo();
connection.Execute(id); // safe (Test2)
}
残念ながら Test1
は検出してくれませんでした。
Ran 1 rule on 1 file: 0 findings.
symbolic_propagation
オプションを有効にして、変数の追跡を強化してみます。
---
rules:
- id: csharp/sql-injection
message: SQL Injection
options:
symbolic_propagation: true
pattern-either:
# ...
無事検出できました!
12┆ connection.Execute($"select * from customers id = '{id}'"); // unsafe (Test1)
Microsoft.Extensions.DependencyInjection に対応させる
今どきは Microsoft のサンプルもそうなってますし、Microsoft.Extensions.DependencyInjection
を利用して IDbConnection
インスタンスをインジェクトしていることが多いのではないでしょうか?
public class Pattern7 {
private readonly IDbConnection _connection;
public Pattern7(IDbConnection connection)
{
_connection = connection;
}
public void Test(string id)
{
_connection.Execute($"select * from customers where id = '{id}'"); // unsafe
}
}
残念ながら、検出できませんでした。
Ran 1 rule on 1 file: 0 findings.
フィールド変数から利用している場合に対応します。pattern-inside
を工夫して、フィールドメンバーにも対応させています。
---
rules:
- id: csharp/sql-injection
message: SQL Injection
options:
symbolic_propagation: true
pattern-either:
- patterns:
- pattern: ($TYPE $CONNECTION).$EXECUTE($ARG, ...);
- pattern-not: ($TYPE $CONNECTION).$EXECUTE("...", ...);
- metavariable-regex:
metavariable: $EXECUTE
regex: ^(Execute|Query)
- metavariable-regex:
metavariable: $TYPE
regex: ^(Sqlite|IDb)Connection$
- patterns:
- pattern: $CONNECTION.$EXECUTE($ARG, ...);
- pattern-not: $CONNECTION.$EXECUTE("...", ...);
- metavariable-regex:
metavariable: $EXECUTE
regex: ^(Execute|Query)
- pattern-either:
- pattern-inside: |
var $CONNECTION = DbProviderFactories.GetFactory(...).CreateConnection();
...
- patterns:
- metavariable-regex:
metavariable: $TYPE
regex: ^(Sqlite|IDb)Connection$
- patterns:
- pattern-inside: |
class $CLASS
{
$TYPE $CONNECTION;
...
}
- metavariable-regex:
metavariable: $TYPE
regex: ^(Sqlite|IDb)Connection$
languages: [csharp]
severity: ERROR
検出できるようになりました!
16┆ _connection.Execute($"select * from customers where id = '{id}'"); // unsafe
もうちょっと
Dapper の Execute()
メソッドかどうかを調べるのに、変数の型を調べずに、次のようにする方法もあります。
pattern-inside: using Dapper;
...
変数の型を調べる必要がないので、今回のようにここまでルールが複雑化しません。
ですが、これがいいかどうかは微妙です。
-
foo.Execute()
呼び出しが誤検出しやすくなる。 - Global using を使われると検出できない。
なので、今回は面倒なのを承知で頑張ってみました。
あと、Semgrep 自体の能力の限界にぶちあたります。
次の例は using
で SqliteConnection
を MyConnection
に別名を与えていますが、これをやられるとマジでお手上げです。
using MyConnection = System.Data.Common.SqliteConnection;
using Dapper;
// ...
var connection = new MyConnection(":memory:");
connection.Execute("...");
まあ、これは極端な例ですけどね。
所感
この手の SAST ツールはちょっと書き方が違うだけですぐに検出から漏れてしまいます。Semgrep 公式ルールなどの配布されているルールも、現場によってはほとんど役に立ちません。やはり自分たちンの現場に合わせたルールが必要そうです。
とはいえ、ゼロから書き起こすのはすごくしんどいので、Semgrep 公式のルールなどの配布されているルールをベースに、アプリケーションコードをレビューしながら調整していくのがいいと思います。
検出率を上げようとすると、誤検出も増えていくので... すごく大変ですけど。いたちごっこですけど、安全なアプリケーションのためには必要なことだと思います。
ルール
このルールを使いたいときは次のようにしてください。
$ semgrep --config=https://gitlab.com/masakura/semgrep-training/-/raw/master/semgrep.yml
最終的に出来上がったルールはこれです。長いですね...
---
rules:
- id: csharp/sql-injection
message: SQL Injection
options:
symbolic_propagation: true
pattern-either:
- patterns:
- pattern: ($TYPE $CONNECTION).$EXECUTE($ARG, ...);
- pattern-not: ($TYPE $CONNECTION).$EXECUTE("...", ...);
- metavariable-regex:
metavariable: $EXECUTE
regex: ^(Execute|Query)
- metavariable-regex:
metavariable: $TYPE
regex: ^(Sqlite|IDb)Connection$
- patterns:
- pattern: $CONNECTION.$EXECUTE($ARG, ...);
- pattern-not: $CONNECTION.$EXECUTE("...", ...);
- metavariable-regex:
metavariable: $EXECUTE
regex: ^(Execute|Query)
- pattern-either:
- pattern-inside: |
var $CONNECTION = DbProviderFactories.GetFactory(...).CreateConnection();
...
- patterns:
- metavariable-regex:
metavariable: $TYPE
regex: ^(Sqlite|IDb)Connection$
- patterns:
- pattern-inside: |
class $CLASS
{
$TYPE $CONNECTION;
...
}
- metavariable-regex:
metavariable: $TYPE
regex: ^(Sqlite|IDb)Connection$
languages: [csharp]
severity: ERROR
Discussion