Zenn
🐈

Semgrep で SQL Injection と戦う #1

2022/12/29に公開
.NET
Security
#
semgrep
tech

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
}

まずはこれを検出できるようにします。

semgrep.yml
---
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 を利用して、これを除外するようにします。

semgrep.yml
---
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 を利用して、メソッド名を正規表現にマッチさせます。

semgrep.yml
---
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 に限定するようにしました。

semgrep.yml
---
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 をルールに追加してみます。

semgrep.yml
    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 ありとなしのルールを別々で記述します。

semgrep.yml
---
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 オプションを有効にして、変数の追跡を強化してみます。

semgrep.yml
---
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 を工夫して、フィールドメンバーにも対応させています。

semgrep
---
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() メソッドかどうかを調べるのに、変数の型を調べずに、次のようにする方法もあります。

semgrep.yml
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

最終的に出来上がったルールはこれです。長いですね...

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