本当は怖いFuelPHP 1.6までのRestコントローラ

FuelPHP Advent Calendar 2013の24日目です。昨日は、@mycb750さんの「Heroku(PaaS)でFuelPHP環境(PHP5.3 + MySQL + Apatch)を構築する」でした。

FuelPHP 1.6までには、Restコントローラを使うとXSS脆弱性を作り込みやすい隠れ機能(アンドキュメントな機能)がありました。

具体的には、公式ドキュメントのサンプルコードをそのまま実行すれば、XSSが可能でした。

ただし、FuelPHP 1.7ではバグのためエラーが発生しXSSは成立せず、1.7.1で修正されました。

なお、この問題を重視しているのは世界中で私1人かも知れませんので、実際に影響があるユーザは少ないのかも知れません。本家はセキュリティ勧告は出しませんでした(Changelogに仕様変更の記述はあります)し、fuelphp.jp Googleグループで告知しましたが、特に反応はありませんでした。

ただし、まだ知らないユーザの方もいるかも知れませんし、今後の参考のためにも、どのような問題があったのかまとめておきます。

Restコントローラとは?

Restコントローラとは、RESTfuelなAPIを簡単に作成するためのコントローラです。以下は公式ドキュメントからのサンプルコードです。

class Controller_Test extends Controller_Rest
{
    public function get_list()
    {
        return $this->response(array(
            'foo' => Input::get('foo'),
            'baz' => array(
                1, 50, 219
            ),
            'empty' => null
        ));
    }
}

http://localhost/fuel/以下がFuelPHPだとして、

ブラウザから、

http://localhost/fuel/test/list.json

にアクセスすれば、以下のようにJSONでの結果が返ります。

{"foo":null,"baz":[1,50,219],"empty":null}

http://localhost/fuel/test/list.xml

にアクセスすれば、以下のようにXMLでの結果が返ります。

<?xml version="1.0" encoding="utf-8"?>
<xml><foo/><baz><item>1</item><item>50</item><item>219</item></baz><empty/></xml>

このようにアクセスされるURLなどにより動的に出力を変更する機能を持っています。

XSS脆弱性の解説

それでは、

http://localhost/fuel/test/list.html

にアクセスしたらどうでしょうか?結果は以下のようになりました。

array(3) {
  ["foo"]=>
  string(0) ""
  ["baz"]=>
  array(3) {
    [0]=>
    int(1)
    [1]=>
    int(50)
    [2]=>
    int(219)
  }
  ["empty"]=>
  string(0) ""
}

何故か、var_dump()した結果が表示されました。

ということで、Firefoxから以下のURLにアクセスすれば、めでたく警告ダイアログが表示されます。

http://localhost/fuel/test/list.html?foo=%3Cscript%3Ealert%28document.cookie%29%3C/script%3E

それなら、エスケープすればいいのでは?と普通は考えます。

            'foo' => Input::get('foo'),

上記のInput::get('foo')は$_GET['foo']を返すFuelPHPのメソッドです。これを以下のようにエスケープして渡せばいいと考えるでしょう。e()は、FuelPHPでのhtmlentities()関数みたいなものです。

            'foo' => e(Input::get('foo')),

でもダメです。実際に試してみると、結果は変わりません。

どこに問題があったのか?

Fuel\Core\Responseクラスの__toString()メソッドに問題がありました。

    /**
     * Returns the body as a string.
     *
     * @return  string
     */
    public function __toString()
    {
        // special treatment for array's
        if (is_array($this->body))
        {
            // this var_dump() is here intentionally !
            ob_start();
            var_dump($this->body);
            $this->body = html_entity_decode(ob_get_clean());
        }

        return (string) $this->body;
    }

配列の場合は、var_dump()して、しかもhtml_entity_decode()して返されています。ここが問題の所在です。Restコントローラから配列を返す場合は、ここでvar_dump()されるというわけです。

さきほどのサンプルコードでは、Restコントローラで配列を作成しているため、配列であることがすぐにわかりますが、ORMなどからも結果が配列で返ることもありますので注意してください。

今となっては何故こんなコードがあったのか正確にはわかりませんが、デバッグ用のためのもののように思われます。デバッグ用コードがデフォルトで有効になっており、それが脆弱性につながるという典型的なパターンのように思われます。

対策

FuelPHP 1.7.1(=現在の1.7/masterブランチ)にアップデートすれば問題は修正されています。

http://localhost/fuel/test/list.html?foo=%3Cscript%3Ealert%28document.cookie%29%3C/script%3E にアクセスしても本番環境以外では、以下のようにメッセージとJSONでの結果が表示され、

The requested REST method returned an array:

{ "foo": "\u003Cscript\u003Ealert(document.cookie)\u003C\/script\u003E", "baz": [ 1, 50, 219 ], "empty": null }

本番環境では406 Not Acceptableが返ります。

アップデートが無理な場合は、Restコントローラで出力フォーマットをjsonやxml(html以外)に固定すればいいでしょう。

    protected $format = 'json';

こう指定することで、URLの拡張子から出力フォーマットを動的に決定することはなくなり、出力フォーマットが固定されます。

というか、FuelPHPのドキュメントには

結果のフォーマットをRESTコントローラ内でハードコードすることはバッドプラクティスであることに注意してください。

と書いてあるんですが、私自身はなんでバッドプラクティスなのかよくわかりません。なので、必ず、フォーマットは固定してます。バッドプラクティスである理由がわかる方がいましたら、是非、お教え願いたいです。

ああ、それから、JSONを返すWeb APIなどは、そもそもブラウザからの直接のアクセスは禁止した方がいいでしょうね。JSONを返すWeb APIの作り方は、『PHP逆引きレシピ 第2版』で解説されていますので、興味のある方はご覧ください(宣伝)。あの徳丸先生も

この第2版は本当に素晴らしい。PHPセキュリティの最新動向をよく把握して、具体的なレシピに落とし込んでいます。すべてのPHP開発者にお勧めします。(…略…)冒頭に書いたように、この第2版は素晴らしいです。「もうペチパーは緩いなんて言わせない」と叫びたくなるほどのインパクトがあります。
http://blog.tokumaru.org/2013/12/php12sql.html

と絶賛されてますので、一家に一冊あって損はないと思います(宣伝)。

あと、FuelPHP 1.7.1からRestコントローラからのJSON出力もそうですが、FormatクラスのJSON出力でのエスケープ(json_encode()関数の第2引数のオプション指定)がより安全なものに変更されています。1.7以前はオプション指定はありませんでした。

最後に

どんなフレームワークも完璧ではないと考えた方が現実的です。今まで、脆弱性が修正されたことがないメジャーなフレームワークというのはないと思います。そのため、フレームワークのソースを読み、実装が本当に安全か確認することが有効です。

FuelPHPは規模的にもまだソースが読めるっぽい分量だと思いますし、読みやすいと思いますので、みなさん、がんがんソースを読んで、バグなどがあれば修正するPull Requestを本家に送るなり、とりあえず、Googleグループで相談してみるとよいと思います。

ただし、セキュリティ上の問題を発見した場合は、いきなりPull Requestを送ったり、Googleグループに投稿するなど、いきなり内容を公開してはいけません。これは、脆弱性情報がいきなり公開されても、即座には対応できないため、ユーザがより危険な状態に置かれてしまうからです。脆弱性情報はその対策ととも公開されるべきものです。

本家へセキュリティ上の問題を報告する場合は、http://fuelphp.com/contactのコンタクトフォームからしてください。

明日は、@samui_さんの「Authのユーザーモデルとほかモデルへのリレーションを作る」です。お楽しみに!

Tags: fuelphp, security

俺の脳内選択肢が、SQLインジェクション対策を全力で邪魔している

PHP Advent Calendar 2013 in Adventarの19日目です。昨日も私の「PDOでの数値列の扱いにはワナがいっぱい(2)」でした。

うっかりtogetterなんか見てしまい、無駄に時間を使ってしまったと後悔した上に混乱してしまい余計にわからなくなってしまった人もいるかも知れません。

そこで、せっかくの機会なので、SQLインジェクション対策について、現在の私の考えをまとめておこうと思います。

選べ
①SQLインジェクション対策にプリペアドステートメントを使う
②SQLインジェクション対策にエスケープを使う

もし、上記のような選択にはまってしまったら、あなたのSQLインジェクション対策は、現実的には、ほぼ100%間違っていると言えるのではないでしょうか。プリペアドステートメントとエスケープは、このような対立構造にはありませんから。

なお、この記事は、SQLインジェクション対策をどう教えるかという教育的な視点ではなく、どう実際にコードを書くかという実務的な視点からの、現在の個人的なまとめです。

SQLインジェクションとは?

ユーザからの文字列をそのまま使用してSQL文を作成することにより、プログラマが意図しないSQL文が実行される脆弱性です。

データベースにあるデータが漏洩、改竄、破壊される可能性がある大変危険な脆弱性です。

典型的なSQLインジェクション可能なサンプルは以下のようなコードです。

$gid  = $_GET['gid'];
$sort = $_GET['sort'];
$sql = "SELECT * FROM users WHERE gid=$gid ORDER BY $sort";
$db->query($sql);

上記では、$gid$sortはユーザが自由に値を操作できるので、どんな値でも入力できます。その値をそのまま使ってSQL文を作成するため、例えば、$gidに「1」を$sortに「; delete from users」を入力すれば、実行されるSQL文は以下のようになります。

SELECT * FROM users WHERE gid=1 ORDER BY name; delete from users

複文が実行できる環境(PostgreSQLやPDO+MySQLで動的プレースホルダの場合)では、これでusersテーブルの全データを削除することが可能です。

なお、複文が実行できない環境の場合は、複文が実行できる環境よりは攻撃の自由度が下がりますが、SQLインジェクションは依然として可能です。

SQLインジェクション対策

実際のSQLインジェクション対策のルールは主に次の2つです。

  • (1) リテラル部分ではプレースホルダを使用する
  • (2) リテラル以外の部分はホワイトリストで検証する

順に説明します。

SQL文の構造

SELECT id,name FROM users WHERE gid >= 10 AND city = '名古屋' ORDER BY name

上記のSQLの場合、SQL文の構造は以下のようになります。

名称
キーワード(予約語) SELECT FROM WHERE AND ORDER BY
演算子など , >= =
識別子 id name users gid city
リテラル 10 '名古屋'

(1) リテラル部分ではプレースホルダを使用する

リテラル部分は、プリペアドステートメントでパラメータ化できるため、プリペアドステートメントを使います。

以下のサンプルでは:gidがプレースホルダです。

$gid  = $_GET['gid'];
$sort = $_GET['sort'];

// SQL文を準備
$sql = "SELECT * FROM users WHERE gid=:gid ORDER BY $sort";
$prepare = $db->prepare($sql);

// 準備されたSQL文の「:gid」の部分に$gidの値をバインド
$prepare->bindValue(':gid', (int) $gid, PDO::PARAM_INT);

// SQL文を実行
$prepare->execute();

ただし、上記ではまだ識別子であるカラム名の部分($sort)でSQLインジェクションが可能です。

(2) リテラル以外の部分はホワイトリストで検証する

リテラル以外の部分は、プリペアドステートメントでパラメータ化できないので、動的にSQL文を作成するか、あらかじめ全パターンのSQL文を用意しておくしかありません。

ここで、あらかじめ全パターンのSQL文を用意しておくというのは、動的に検索条件が増えたりIN句を使う場合などパターン数があまりにも多くなってしまう場合は、実用的ではありません。

通常のアプリでは、テーブル名、カラム名などの識別子はあらかじめ決まっています。

ですから、SQL文を作成する際、プログラマがあらかじめ定義した値のみが使われるように(ユーザからの入力がそのまま使われることがないように)、ホワイトリストで検証します。

$gid  = $_GET['gid'];
$sort = $_GET['sort'];

// ホワイトリスト
$sort_whitelist = array(1 => 'id', 2 => 'name');
// ホワイトリストによる検証
$sort_safe = isset($sort_whitelist[$sort]) ? $sort_whitelist[$sort] : $sort_whitelist[1];

$sql = "SELECT * FROM users WHERE gid=:gid ORDER BY $sort_safe";

キーワードや演算子もSQLで決まっていますので同様です。また、キーワードを動的に変化させる必要があるケースはORDER BYのASC、DESCくらいではないかと思います。

なお、上記のようなコーディングでは、識別子をエスケープ(というかクォート?)していないため、キーワードと同じ識別子(例えば、カラム名group)がSQLエラーになり使えません。

例外的なケース

通常は、上記の

  • (1) リテラル部分ではプレースホルダを使用する
  • (2) リテラル以外の部分はホワイトリストで検証する

の2つでSQLインジェクションを完全に防ぐことができますが、例外的なケースではそれらの対策が使えないことがあります。

プリペアドステートメントが使えない場合

プリペアドステートメントが使えない環境では、リテラルを正しくエスケープする必要があります。

リテラルのエスケープ方法については、http://www.ipa.go.jp/security/vuln/websecurity.htmlの「安全なSQLの呼び出し方」を参照してください。

識別子をユーザが指定する場合

テーブルを作成したり、変更したりするphpMyAdminのようなのデータベース管理ツールを作成する場合は、ホワイトリストが使えません。

ホワイトリストが使えない場合は、識別子を正しくエスケープする必要があります。

識別子のエスケープ方法については、

を参照してください。

この記事で扱っていないこと

  • 文字エンコーディングの正しい指定方法
    • 文字エンコーディングが正しく指定されていないとSQLインジェクションが可能な場合がある
  • 動的プレースホルダと静的プレースホルダの違い
    • 実装にバグがなければどちらもSQLインジェクション対策としては有効
    • 原理的には静的プレースホルダがより安全
  • LIKE句でのワイルドカードのエスケープ処理
    • SQLインジェクションの原因にはならないがDoSになる可能性はある
  • 個別の実装に関するワナとしか思えないような仕様など
    • PDOでのワナなど
    • たぶんSQLインジェクションの原因になるものはない

まとめ

SQLインジェクションの原因は、SQL文の作成時にユーザに由来する入力値をそのまま使ってしまうことです。

通常は、以下の対策でほぼ100%対応可能だと思います。

  • (1) リテラル部分ではプレースホルダを使用する
  • (2) リテラル以外の部分はホワイトリストで検証する

参考

明日は、ngyukiさんの「Visual Studio Express 2013 で PHP をステップ実行する」です。

Tags: security, pdo, sql, database

PDOでの数値列の扱いにはワナがいっぱい(2)

PHP Advent Calendar 2013 in Adventarの18日目です。昨日は、takc923さんの「PHPのissetの罠」でした。

PDOでの数値列の扱いにはワナがいっぱい」を書いたところ、以下のように結構反響がありました。

ということで、今日はPDOで数値列を扱う場合、どうコーディングしたらいいのかについて書いてみようと思います。

PDOでの数値列の扱いにはワナがいっぱい」をまだ読まれていない方は先にそちらをお読みください。

どうすればいいのか?

数値が文字列型として扱われてしまうことが問題なので、解決策は型を明示することです。この場合、PDOには、以下の2つのメソッドがあります。

bindParam()は変数(参照渡し)を、bindValue()は値を、プリペアドステートメントのパラメータにバインドしますが、いずれも第3引数がdata_typeになっており、ここでPDOの定数でデータ型を指定できます。指定しなかった場合は、PDO::PARAM_STRになりますので、やっぱり文字列型とされてしまいます。

また、bindParam()は変数を参照としてバインドするので、実際の値はPDOStatement::execute()がコールされたときに決まります。素直なPHPerの皆さんは無駄に参照を使い、混乱し、さらに「PHPの参照は糞参照」などど世界の中心で叫ぶ必要はまったくありませんので、通常はbindValue()を使いましょう。

具体的なコード

それでは具体的なコードで検証してみましょう(動作確認環境:PHP 5.3.27 & MySQL 5.5.16、PHP 5.5.6 & MySQL 5.6.14)。

テーブルなどは「PDOでの数値列の扱いにはワナがいっぱい」のものを流用します。

try {
  $db = new PDO(
    'mysql:host=127.0.0.1;dbname=sample;charset=utf8'
    , 'root'
    , ''
  );

  echo 'MySQL: エミュレーション On';
  $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, true);
  $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

  $int = '3';
  $str = 'Ruby';

  $prepare = $db->prepare('SELECT * FROM example WHERE id = :id and language = :lang');
  $prepare->bindValue(':id', $int, PDO::PARAM_INT);
  $prepare->bindValue(':lang', $str, PDO::PARAM_STR);
  $prepare->execute();
  //var_dump($int, $str);

  $db = null;
} catch (Exception $e) {
  var_dump($e->getMessage());
}


try {
  $db = new PDO(
    'mysql:host=127.0.0.1;dbname=sample;charset=utf8'
    , 'root'
    , ''
  );

  echo 'MySQL: エミュレーション Off';
  $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
  $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

  $int = '3';
  $str = 'Ruby';

  $prepare = $db->prepare('SELECT * FROM example WHERE id = :id and language = :lang');
  $prepare->bindValue(':id', $int, PDO::PARAM_INT);
  $prepare->bindValue(':lang', $str, PDO::PARAM_STR);
  $prepare->execute();
  //var_dump($int, $str);

  $db = null;
} catch (Exception $e) {
  var_dump($e->getMessage());
}

念のため、PDOでのプリペアドステートメントのエミュレーションをOnとOffの場合の2パターン記述してます。エミュレーションOnとは「安全なSQLの呼び出し方」での「動的プレースホルダ」のこと(PDO_MySQLでのデフォルト)、エミュレーションOffとは「静的プレースホルダ」のことです。

上記のコードを実行すると、MySQLのクエリログは以下のようになりました。

153 Connect root@localhost on sample
153 Query   SELECT * FROM example WHERE id = '3' and language = 'Ruby'
154 Connect root@localhost on sample
154 Prepare SELECT * FROM example WHERE id = ? and language = ?
153 Quit    
154 Execute SELECT * FROM example WHERE id = '3' and language = 'Ruby'
154 Close stmt  
154 Quit    

153が最初の接続(動的プレースホルダ)、154が次の接続(静的プレースホルダ)ですが、どちらも結果として、以下のSQLが実行されていることがわかります。

SELECT * FROM example WHERE id = '3' and language = 'Ruby'

あれ、おかしいですね。PDO::PARAM_INTを明示したにも関わらず、id = '3'とシングルクォートで囲まれており、文字列となってしまっています。これでは、PDOStatement::execute()の引数で値を渡す場合と変わりません。

何が問題だったのか?

勘の鋭い方はすでにお気付きかと思いますが、問題は数値列の値が、PHPで文字列型だったという点です。

  $int = '3';

これを、

  $int = 3; // 整数

にすれば、期待したように数値列が数値列として扱われる以下のSQL文が実行されます。

SELECT * FROM example WHERE id = 3 and language = 'Ruby'

$_GET、$_POSTなどの値は原則すべて文字列です(配列を渡すこともできますが)ので、キャストせずに使えば結局、数値を文字列として扱っていることになり、SQLでの暗黙の型変換が発生してしまいます。

PDO::PARAM_INTとは一体何だったのか?

PDOでの型を指定する定数について調べると、現在の実装は以下のようになっているようです。

  • PDO::PARAM_STRは値を文字列に変換
  • PDO::PARAM_INTはboolをintに変換(他は何もしない)
  • PDO::PARAM_BOOLはintをboolに変換

まとめ

ということで、PDOで数値列を扱う場合の私が考える正しい解決策をサンプルコードで示すと以下のようになります。

  $prepare = $db->prepare('SELECT * FROM example WHERE id = :id and language = :lang');
  $prepare->bindValue(':id', (int) $int, PDO::PARAM_INT);
  $prepare->bindValue(':lang', $str, PDO::PARAM_STR);
  $prepare->execute();

(2015-04-10 追記) ただし、文字列を数値にキャストするとオーバーフロー/アンダーフローの可能性があります。PHPで扱えない範囲の数値をデータベースで使っている場合は注意が必要です。

今日解説したPDOのワナ

  • バインド時にPDO::PARAM_INTを指定しても、文字列型の値を渡せばSQLで文字列として扱われてしまう

整数型を指定しているにも関わらず、値がシングルクォートで囲まれるというのはバグじゃないんでしょうかね?PDOの偉い人がいましたら、ご意見を伺いたいです。

参考

Tags: php, pdo, database