根拠レスな「このコードはセキュリティがダメだね」発言は居酒屋での「大したことねえなマツザカ（笑」みたいな会話に近い
概要と結論：
- PHP を使って Twitter 風のシステムをサイトに構築する (IBM Developer Works 2009/3) という記事で紹介されているコードが、
- IBMもピンキリである (よくきたはてダ2009/3)というブログで「これ（セキュリティ的に）どうしようもないね(笑」と表面だけの斜め読みで根拠レスに切り捨てられていたので、
- どれどれと思って筆者がコードを見てみたら、XSS脆弱性につながりそうな箇所がひとつだけ見つかったのだが、サンプル用のソースコードという前提にあってこれが「問題が盛りだくさんです．」って言うほどかあ？と。アイビーエムって言いたかっただけちゃうんかと。
- そもそも、「マツザカも大したことねえな、ちょっと打たれすぎだろ（笑」みたいな居酒屋での一億総監督的な野球談義ならともかく、根拠も示さずに訳知り顔で「セキュリティがなってないぜ（笑！」と切り捨てるだけではHelloWorldプラスアルファな人々をムダに萎縮させハードルを上げるだけであり誰も幸せになれない。っていうか元ネタのサイトに失礼。
- まあ世の中の情報というものはマスコミでもブログでもこんな感じなことはよくあるのだから、どんなジャンルの情報であれ、いろんな前提条件をあえて無視あるいは認識しそこねたりすると、かなり大きく話が違ってくるということに注意しながら情報にあたろうね。
というお話。
さて、対象のコードを見る前に、まずは、同時にそこに書いてある種々の前提に目を通すべきだろう。動作環境とかいろいろ書いてあるが全部引用してもしょうがないのでとにかく読むとして、ここでは特にこの↓前提に留意したい。
最後の前提として、この記事の説明は、何らかの形でユーザーが関係するアプリケーションがすでにあり、実行中のそのアプリケーションにマイグロブロギングまたはつぶやきの機能を追加するという設定で進めます。そのため、アプリケーションの機能のなかでもユーザー自身の設定に関する部分 (ログイン、プロフィールの管理など) は省略し、投稿の機能に焦点を絞って説明します。
セキュリティ観点でコードを読む場合はそのベースとなっているモノなり技術なりの前提が重要だ。ソースがセキュアかどうか以前にまず「サーバールームのドアにはちゃんと鍵をかけてあること」といった前提があるはずだ。大雑把な例えだけど。今回は既存のサイトに追加するためのソースコードのレビューなので、既存のサイトは一応はセキュアであるという前提。でないと話が進まない。
順番が前後するが、まず最初にリスト7に示されたテキストの投稿フォーム画面。
（ところどころ省略） $userid = $_SESSION['userid']; $body = substr($_POST['body'],0,140); add_post($userid,$body);
- ポイント：$_SESSION['userid']は本当に意図された数値か？他の値で汚染されたりしていないか？
- ↑その少し上にこういう説明がある。「リスト 6に、index.php ファイルのマークアップを記載します。注意する点として、ここでは PHP セッションを使用して、ユーザー ID の値をデータベース内での jane のユーザー ID である 1 にハードコーディングしています。今のところはこの方法でまったく問題ありませんが、後で変更が必要になることは明らかです。」
つまり既存サイトのコード上において $_SESSION['userid']は必ず意図された数字が入っていることを前提としている。 リスト1やリスト2のテーブル定義においても、ユーザーIDはint（整数型）でNOT NULLとされている。数値以外の値を入れようも取り出しようも無い。 - ポイント：$bodyを、文字列長を140バイトで切リ捨てる処理だけやって、add_post()メソッドに渡している。そんな渡し方で大丈夫か？
- ↑ここで$bodyの中身を吟味しないのであればadd_post()のほうでで何かやるしかない。add_postのソースを見てみよう。
リスト 5. posts テーブルにコンテンツを追加する関数（の抜粋）
function add_post($userid,$body){
$sql = "insert into posts (user_id, body, stamp)
values ($userid, '". mysql_real_escape_string($body). "',now())";
$result = mysql_query($sql);
}
$resultを評価してちゃんとINSERTが成功したか確認しないの？というツッコミはセキュリティ的な話とはちょっと違うのでスルー。（そもそもこれはサンプルコードである）
$useridは先ほどのとおり必ず数値が入っている前提であり、かつ、
$bodyについてはmysql_real_escape_stringを使っている。したがってSQLインジェクションを防ぐことができている。
次。リスト 8. show_posts() 関数（の一部を抜粋）
$sql = "select body, stamp from posts where user_id = '$userid' order by stamp desc"; $result = mysql_query($sql); （以下省略）SQLに入れ込む変数は$useridのみ。前述のとおり数値が必ず入っている前提なので安全にSQLを実行できる。そのほかの部分も特に問題なし。
次に、上のshow_posts()関数を呼び出して、投稿内容を表示するためのページ： 「リスト 9. index.php ページでの投稿の表示」（の一部引用）
$posts = show_posts($_SESSION['userid']); （省略） echo "<tr valign='top'>\n"; echo "<td>".$list['userid'] ."</td>\n"; echo "<td>".$list['body'] ."<br/>\n"; echo "<small>".$list['stamp'] .">/small></td>\n";show_posts()に渡す$_SESSION['userid']は先述のとおり必ず数値が入っている前提なので関数の実行に問題はない。リスト 2のposts テーブルのテーブル定義から言って、 $list['userid']は数値、$list['stamp']は日時（2009-01-23 12:04:45みたいな）しか入らない。したがってそのまま表示しても問題なし。
ところが、真ん中へんの echo $list['body']; がまずい。 postsテーブルに情報をINSERTするadd_post()メソッド内部では「SQLインジェクションを防ぐためにmysql_real_escape_string($body)してる」と先ほど書いたが、逆に言うとそれ以上のことはしていない。
ということは、もしユーザーが <script>alert('JavaScript実行できちゃうぜ！')</script> という内容をbodyとして投稿したら、そのまんまDBに格納されてしまうはずである。そして他のユーザーがそれをSELECTしたものを画面表示するときにブラウザ上でこのJavascriptが実行されてしまう。つまり残念ながらここはXSS脆弱性につながる。
このようなケースでは、PHPの場合は例えば
echo htmlspecialchars($list['body']);
と書くことで防ぐのが基本である。
さらに言うと、実際の現場ではどの変数が安全かなんていうのをいちいち吟味していられない＆あとでどういう仕様変更が発生するかもわからないので、たとえば表示系では上の例で言うと
echo "<tr valign='top'>\n"; echo "<td>".htmlspecialchars($list['userid']) ."</td>\n"; echo "<td>".htmlspecialchars($list['body']) ."<br/>\n"; echo "<small>".htmlspecialchars($list['stamp']) .">/small></td>\n";のように、変数出力のすべてにおいて特殊文字の変換を行ってしまうのがセオリーではある（ほっといてもそうしてくれるフレームワークすらある）。同様に、SQL文の組み立てと実行の際にも全ての変数に対してmysql_real_escape_string()しろ、ということも言える。（そもそもプリペアドステートメント使えという話は後述）
以降、リスト10からリスト19までまだまだ続くのだが、全部書いてくとキリがないので結論だけ言うと、リスト10以降のスクリプトにおいてはDBや画面における入出力で使われているほとんどの変数が予定された値（数値など）しかありえないようになっていた。つまり、サンプル用のコードであるという前提において、リスト10以降のスクリプトはSQLインジェクションやXSS脆弱性につながりにくい最低限度のセキュア性があると言える。
さて、話は変わるが、ソフトウェア（ここでは特にWebアプリ）のソースコードというものは、
- 実際に公開して動かすためのコード（ユーザーが機能として使うコード）
- 実現方法を模索し検討するためのサンプル（試作品）のコード
- テストするためのコード
といったものがある。 実際の現場ではいきなり1を書くしかないくらいの納期だったりすることは日常茶飯事だが（苦笑）、それでも、高品質な1のコードの存在の裏側には2,3のようなコードがあることを忘れてはならない。
逆に言うと、2や3のジャンルに属するコードに向かって「セキュリティがなってねえよ」というツッコミは、開発中の試作車に向かって「ボディついてねえよ」とつっこむのにも似ている。
さらに話が飛ぶが、実際に動くコードを書く際には、セキュリティを意識したアリモノのフレームワークを使うことによって、セキュリティ性を高めるという方法がよく取られる。（っていうか取るべきである）
しかし、フレームワークそのものの使い方に関するサンプルならともかく、機能の実現性を検討するためのサンプルコードにおいてもフレームワークを使った場合には、そのフレームワークのユーザーにしか理解できないサンプルコードとなり、それって何のためのサンプルなのよ、意味ないじゃん、という諸刃の刃。 実はこの件の元ネタの記事を書いた人も同様に思っていて、その文中には次のようにちゃんと書いてある。
私は普段、CodeIgniter などのモデル・ビュー・コントローラー (MVC) アプリケーション・フレームワークのコンテキストで作業を進めています。MVC アプリケーション・フレームワークには、この種のアプリケーションを作成するための本格的なツール一式が揃っているからです。例えば通常なら、私はまず users、posts、および following テーブルを操作するための 2 つのモデル (1 つはユーザー用、もう 1 つは投稿用) を作成し、それから作業を進めます。
読者はこれとは異なるフレームワークですでに作業を進めている可能性があるので、今回はこの手法を適用しないことにしました。代わりに選んだのは、フレームワークに依存しない、もっと単純な手法です。
しかしそれでも、元記事には、ソースコードのこことかあそことかいうよりもさらに低いレイヤーでの根本的な問題がいくつか残る。
- 単純なSQLを発行するだけならプリペアドステートメントを使うべき。ソースの可読性向上の観点から見ても安全性の観点で見ても、現代では、っていうか少なくとも4,5年前からこれはデファクトと言っていいだろう。
- そもそも、プリペアドステートメントの実装すら無いとても古くからあるMySQL関数群をいつまで使ってんだ、今やるんだったらたとえばPDOあたりを使った例を示すべきなのでは？ と。
そんな話は実は去年書いた↓。長くなったので以降はそっち参照。
HelloWorldプラスアルファからさらに上を目指すために (PHP編)(2008/9)
セキュアなコードを書くのは、めんどくさい。 「セキュリティ大丈夫ですか？」ってツッコまれたところで「もっと具体的に言えよセキュリティって言いたいだけちゃうんかハゲ」と思うのは俺だけじゃないだろう。 そういう意味でも、セキュリティ過敏症というやつは大嫌いだ。自分の案件の納期が迫ってるからという話でもないのにフレームワーク使えの一言で済ませることしか考えてない人もね。
see also:
- pg_pconnectに気をつけろという話に気をつけるべき理由(2009/3)
- mixiのデザインのリニューアルに対するallaboutの「専門家」陣のツッコミがなんだかなあな件 (2007/10)
- 世間の認識と脅威レベルのギャップ――XSSは本当に危ないか？ － ＠IT (2009/3)
2009年4月 2日
| Permalink
|
Comments [0]
|
Trackbacks [0]
