(3)で完成したと思っていたDBLayer Trapping anti-SQL-Injectionのロジックですが、これを実際にXOOPS 2.0.16a-JPでテスト運用してみたら、一般設定の更新でSQL Injectionの誤検出が発生しました。(具体的には、picoの共通ヘッダとしてダブルクオーテーションを含む文字列をポストする時)
このSQL対策ロジックは原理上、誤検出も当然あり得るのですが、どう見てもそういうパターンでもないので、疑問に思いながらXOOPSのソースを追ってみたら、かなりビックリするコードを見つけました。
class/database/mysqldatabase.php
function quoteString($str)
{
$str = "'".str_replace('\\"', '"', addslashes($str))."'";
return $str;
}
function quoteString($str)
{
$str = "'".str_replace('\\"', '"', addslashes($str))."'";
$str = "'".mysql_real_escape_string($str)."'";
return $str;
}
DBレイヤーのオーバーライドの準備ができたとしても、リクエストとすべてのSQLを比較するのは、それなりにコストが高い手法であることを自覚する必要があります。
そこで、まずはDBレイヤーをオーバーライドする頻度を下げる努力をします。
ほとんどのリクエストには「攻撃となりえる」文字列は最初から含まれていないのですから、その時には元のDBレイヤーをそのまま使います。
あくまで、特定の文字列が含まれている時だけ、DBレイヤーをオーバーライドすることで、速度と安全性を両立できるでしょう。
もちろんその「特定の文字列」の判断こそが難しいのですが、こんなルールにしてみました。
/(information_schema|select|'|")/i
'OR 1#
あっという間に1週間経ってしまいましたが、「リクエスト層とDB層の両方を比較して、SQL Injectionを防ぐ方法」の続きです。
PHP汎用の話題であれば、何らかのフレームワークにこれを組み込む、という話になるのでしょうが、今どきのフレームワークはORマッパーを使うものが多く、クエリを直接発行することもないでしょうから、例によってXOOPS用モジュールであるProtectorにどう組み込むか、という話になります。
まず、DBレイヤーをフックする必要がありますが、XOOPSの場合、ここが非常に硬直化していて、後からDBインスタンスを乗っ取ることはほぼ不可能です。つまり、いずれのXOOPSコアについても、データベースファクトリクラスを書き換える必要があります。
フック方法もいろいろ考えたのですが、絶対にリクエスト依存にならない、という理由から、定数を利用することにしました。最初に、XOOPS_DB_ALTERNATIVEという定数が宣言されていれば、そのクラスがDBインスタンスとして用意されます。
class/database/databasefactory.php
require_once $file;
/* patch from */
if ( defined('XOOPS_DB_ALTERNATIVE') && class_exists( XOOPS_DB_ALTERNATIVE ) ) {
$class = XOOPS_DB_ALTERNATIVE ;
} else /* patch to */if (!defined('XOOPS_DB_PROXY')) {
$class = 'Xoops'.ucfirst(XOOPS_DB_TYPE).'DatabaseSafe';
} else {
$class = 'Xoops'.ucfirst(XOOPS_DB_TYPE).'DatabaseProxy';
}
$instance =& new $class();
世界的にXOOPSの本家とは xoops.org ってことになり、本流は2.3系列って事になっているようです。
そこの最新版である xoops-2.3.2b というアーカイブをダウンロードしてみてあきれました。
htdocs/ の中に、xoops_lib というフォルダがあって、その中に、XOOPS_TRUST_PATHに置かれるべきファイルが詰まっているのですから。
しかも.htaccessすらなし。
思わず目が点になりました。
mamba氏が、XOOPS_TRUST_PATH内のファイルにLFIがある、という不思議なレポートをしてきて、その意味が理解できなかったのですが、アーカイブ構造を見て納得しました。
つまり、彼らはXOOPS_TRUST_PATHの意味を一つも理解していないのです。
XOOPS_TRUST_PATHを、xoops_lib なんて名前に勝手に変更しているのも、彼らがXOOPS_TRUST_PATHの意味を理解していないことの証拠の一つでしょう。
mamba氏は「Protectorにセキュリティホール(LFI)があるから独自にfixをした」、とか言ってましたが、DocumentRootの外にあるのが前提となっているファイル群に場当たり的なパッチを当ててもまったくの無意味です。
案の定、今度はRFIとしてレポートが上がったそうです。
http://www.milw0rm.com/exploits/7705
これはProtectorの穴ではなく、XOOPS-2.3.2の穴であると正確に理解してください。
とにかく、phppp はじめ xoops.org の開発者に言いたいことは以下の3つです。
XOOPS_TRUST_PATHをアーカイブのhtdocs/の外に出してください。
DocumentRootの外と内の違いについて勉強してください。
Protector V3のインストール方法を繰り返し読んでください。
それが出来ないのなら、同梱なんてしないでください。
誤ったインストール方法をデフォルトで用意しておいて、Protectorにセキュリティホールがあるなんて言ってくるxoops.orgの神経が信じられませんし、それが原因で第三者にProtectorの存在意義を問われてしまうのもこれまた心外です。
XOOPSに限らず、いろいろなPHPアプリケーションで、今でもちょくちょくSQL Injection脆弱性が出てきます。それもケアレスミスです。
もちろん、ケアレスミスは人力では根絶不可能であり、SQLを文字列として生成すること自体をやめて、パラメータ方式で呼び出すべきだ、という方が本質的な議論でしょう。
ただ、オープンソースの世界では、自分の考えを他者に押しつけることはできませんし、SQL Injectionの可能性ある「誰か」の書いたコードを一切利用しない、というのも現実的ではありません。
事実として、SQL InjectionはXSSとは比べものにならないくらい恐い攻撃です。もし私がオープンソースアプリケーションで構築された特定のサイトを攻撃するとしたら、最初に狙うのはSQL Injectionでしょう。なぜならXSSやCSRF、セッション関連の攻撃と違い、即座に直接攻撃可能だからです。(さすがにコマンドインジェクションとかRFIはない、という前提で)
そういうわけで、Protectorでは、SQL Injection対策っぽいものも入っているのですが、私自身、あまり役に立っている気がしません。攻撃者がProtectorの対策コードを読めば、明らかに抜け道だらけです。(本当に役に立ったとすれば「最後にidのつくリクエスト変数名について強制的にintval()をかける」くらいでしょうが、これも副作用が大きすぎます)
強力なSQL Injection対策ができない理由は簡単で、本当に必要なリクエストかどうか区別がつかないからです。「UNIONをUNI-ONにする」なんてのも、本当はあってはならないことです。UNIONという単語を使ったまともな投稿が勝手に書き換わってしまいます。
ただ、入口であるリクエスト層だけで判断している以上は、ここが限界となりますが、大きな傘anti-XSSシステムと同様、入口と出口の両方を押さえれば、かなり有力な手段となることにふと気がつきました。これはいけそうです。
anti-XSSでは、出口はobフィルターでした。XSSになる可能性があるリクエストがある時のみob_start()をかけて、出力にリクエストがそのまま含まれるようならXSSとして停止する。
これをanti-SQL-Injectionにあてはめると、出口は当然クエリ関数(メソッド)となります。SQL Injectionになる可能性があるリクエストがある時のみ、DBレイヤーを乗っ取って、クエリ本文(クオーテーション外)にリクエストが含まれるようならSQL Injectionとして停止する…
…とまあ、文章として書くのは簡単なのですが、SQL Injection用の出口処理は結構面倒です。SQLをちゃんとパースする必要がありますから。
また、この仕掛けをXOOPS用Protectorとして採用する場合、もう一つ問題があります。それは、現状のDBレイヤーは、途中で乗っ取ることが不可能、ということです。この点は、コア側に対応してもらう必要があるため、別エントリにしたいと思います。
ちなみに、今回私が考え出した(と思っていた)方法ですが、元ネタがあったのを思い出しました。
「本質的なSQL Injection対策のためにはDBレイヤーを乗っ取るしかない」
と、もう4年も前にJM2さんが指摘していました。流石としか言いようがありません