PHP の extract 関数は危険という話

ギークなお姉さんは好きですか - ＜title＞をクールにしてみた！で、こんなコードが紹介されている。

<?php
extract($_GET);

mysql_connect('localhost','ユーザ名','パスワード');
mysql_select_db('データベース名');
mysql_query("set names utf8");

$sql="select * from geekDB where name_id = "$name_id"";   # ←「"」が重複しているのでコピペミスかな？
$result = mysql_query($sql); 

これに対して、コメント欄で以下のようなツッコミが入っている。

extract() をユーザー入力 ($_GET, ...) のような信頼できないデータについて使用しないでください。 http://jp.php.net/extract より。

たしかに、PHPのマニュアルには以下のように書かれている。

警告

extract() をユーザー入力 ($_GET, ...) のような信頼できないデータについて使用しないでください。 もし行う場合、例えば register_globals を信頼しているような古いコードを一時的に実行したい場合、 EXTR_SKIP のような extract_type の値が上書きされていないことを確認してください。そして php.ini の variables_order で定義されたものと同じ順で展開すべきであることに留意してください。

でもこれって、PHPのマニュアルもあんまりよくないと思う。

• セキュリティに関する大切な警告なのに、マニュアルの後ろの方に書かれている。サンプルコードより前じゃないと気がつかない人が多そう
• 「なぜ使用してはいけないか」が書かれていない。なので、対策を読んでも OK の場合と NG の場合の区別がつかない

PHP のマニュアルは充実していて好きなんだけど、こういうところがちょっと苦手。

なぜ入力値に extract を使用すると危険なのか

今回のべにぢょさんのコードだと最初に extract を実行しているから、そんなに危険はなさそう。 でももし、以下のようなコードだったら…。

<?php
$sql = "select * from geekDB where name_id = '";
extract($_GET);
$result = mysql_query($sql + "'" + $name_id + "'"); 

extract を実行した時点で変数が展開されるので、以下のURLにアクセスしたらSQL文を上書きできてしまう。

geek.php?sql=delete%20from%20GeekDB%20where%20name_id%3d&name_id=amachang

ちょっとわかりにくいけど、以下のようになる。

$sql = "delete from GeekDB where name_id=";
$name_id = "amachang";
$result = mysql_query($sql + "'" + $name_id + "'");  // delete from GeekDB where name_id='amachang'

（わかりにくいけど）マニュアルに書かれているように EXTR_SKIP を使えばすでに存在する変数を上書きしないので、まだ幾分マシ。

extract($_GET, EXTR_SKIP);

ただ、任意の変数を設定できることに変わりないので、やっぱり危険なのもかわんない。 なので、普通にこう書きましょうとなる。

$name_id = $_GET["name_id"];
$sql = "select * from geekDB where name_id = ?";
$result = $db->query($sql, array($name_id));

• name_id 以外の入力値（クエリ）は無視される
• name_id の値も信頼しない（prepared statementを使う）

でもセキュリティ過敏症にならないために

賛否両論あるセキュリティ過敏症の話だけど、完璧にセキュリティ対策しないとダメってのは（個人サイト）では息苦しく感じるのも確か。 あんまり考えなくていいように、こういうのは個人的にはフレームワーク側（PHPの場合は言語レベル）で対応して欲しいところ。 たとえば…

• extract のデフォルトを EXTR_OVERWRITE ではなく EXTR_SKIP にする
• そもそも入力値の展開以外での extract の必要性があんまり分からない。非推奨にしてもいいのでは？

とかね。 次の PHP6 とかで古い互換性をバッサリ捨てて、シンプルで安全なテンプレートエンジンになってくれるといいなぁ。 とりあえず、以下のところだけを整備してくれれば他はいいから！

• echo や printf などの出力値はデフォルトで HTML エスケープされる
• DBへのSQL文は prepared statement を標準にする