MTDDC開催中。セキュリティ強化月間らしいので。
公開日 : 2011-06-25 13:50:41
えー、まぁ、何というかセキュリティアップデート続きのおかげもあって今日も仕事してます。ええ。最初は行く予定だったんですけどね。
さて、ちょっと切り替えて真面目に行きます。
仮にテスト・ドリブンの開発を行っていたとしても、バグやセキュリティ脆弱性が見つかることはある程度以上のソフトウェアにとっては現状のところ避けられない問題ではないかと思います。ソフトウェアのコードを読み進むことで開発者が想定していない問題が見つかったりすることは実際にある訳で、最終的にはコードレビューこそが最終的な品質管理だと言えるのではないでしょうか(結局のところ開発者の予期しなかった問題を指摘するのはアナログな手法でコードを読めるテスターの存在頼りだったりします)。
現在、MT開発チームはセキュリティ強化月間らしいです。コードレビューをしましょうよってのは僕も思って提案してたから、良い傾向だと思います。
単にコードレビューといっても、やはり何らかの目的と視点を持ってみていかなければ脆弱性やバグは発見できないことがあります。レビューやテストに実は一番大切なのは「想像力」ではないかと思う今日この頃です。
簡単な例題でコードレビューの視点を考える
例題として「ウェブページに対する操作を行う以下のメソッドのコードレビューでは何を想像すれば良いか」というお題を考えてみましょう。
リクエストの形:
__mode=some_action&blog_id=1&id=100
問題のコード
sub some_action{
my $app= shift;
require MT::Page;
my $page = MT::Page->load( $app->param( 'id' ) );
if (! $page ) {
return $app->translate( 'Invalid Page ID:[_1].', $app->param( 'id' ) );
}
my $perm = $app->user->is_superuser;
if (! $perm ) {
$perm = $app->user->permissions( $app->blog->id )->can_administer_website;
$perm = $app->user->permissions( $app->blog->id )->can_administer_blog unless $perm;
$perm = $app->user->permissions( $app->blog->id )->can_manage_pages unless $perm;
}
if (! $perm ) {
return $app->translate( 'Permission denied.' );
}
# この後、$pageへの処理
}
例外として想像すべきケースは例えば以下のようなものです。
- パラメタidに数字以外のものが渡された場合
- パラメタidにpageオブジェクトではなくentryオブジェクトのidが渡された場合
- パラメタidが空の場合
- パラメタidに現在のblogに属さないentry/pageのidが渡された場合
1.のケースですが、数字以外のものが渡された場合、pageはロードされませんから、 return $app->translate( 'Invalid Page ID:[_1].', $app->param( 'id' ) ); が呼ばれます。return している際にパラメタ id を付けてメッセージを返していますが、idにJavaScriptが渡されたらどうなるでしょうか。このコードにはXSS脆弱性があります。
2.のケースでは、$pageに entryオブジェクトがそのまま格納されます(IDのみを指定した場合、classに関わらずロードされます)。ところが、この後の権限チェックではcan_manage_pagesで権限をチェックしています。このコードには権限のないオブジェクトに対する操作が可能な脆弱性があります。
3.のケースですが、my $page = MT::Page->load();となりますが、$pageには条件指定なしで呼び出されたオブジェクトの先頭1件が格納されてしまいます。操作の内容によりますが、これはバグである可能性があります。
4.のケースですが、entry_idが100のページがblog_idが2のブログに属していた場合どうなるでしょうか。 blog_idが1のブログに対してウェブページの管理権限を持っているかどうかのチェックを行っていますが、blog_idが2のブログの権限チェックはしていません。blog_idが2のブログにこのユーザーが権限を持っていない場合、このコードはそのまま実行されてしまいます。このコードには権限のないオブジェクトに対する操作が可能な脆弱性があります。
その他にも、オブジェクトに対する操作を伴うメソッドには magic_token によるチェック、及びcms_save_permission_filterやcms_delete_permission_filter等操作に応じたコールバックをコールして権限チェックを行わなければなりません。
例題のコードの修正すべき点は以下の通りです。
- idがなければその時点でエラーを返す(エラーメッセージに受け取ったパラメタを含める場合はencode_html)を忘れない(もしくはテンプレート側でエスケープする)。
- idが数字かどうかをチェックする(数字でなければエラー)、またはエラーメッセージにユーザーが指定可能なパラメタを含める場合、MT::Util::encode_htmlを通して返す(ettor.tmplの方でエラー出力のタグにescape="html"が含まれている場合は$app->errorメソッドを使うことでも良いですが、現状のMTではコード側でエスケープした方が安全)。
- 読み込んだ$pageの$page->classをチェックして、page以外であればエラーを返す。もしくはロードメソッドでclass=>'page'を指定して、オブジェクトがpageであることを保証する。
- $app->user->permissions()に渡すBlogIDを $app->blog->id ではなく、 $page->blog_id とする(もしくは$app->blog->id と $page->blog_id が異なる場合にエラーを返す*システムスコープで実行される可能性があるケースでは前者)。
- この後オブジェクトに何らかの操作を行うメソッドの場合は、magic_tokenのチェックを事前に入れる
- 保存、削除を伴うメソッドの場合、cms_save_permission_filter.pageコールバックをコールして、戻り値がない場合にエラーを返す(必要に応じてcms_save_filter.page/cms_pre_save.page/cms_post_save.pageをコールする)
修正適用後のコード(例)
sub some_action {
my $app = shift;
$app->validate_magic or
return $app->trans_error( 'Permission denied.' );
require MT::Page;
my $page;
if (! $app->param( 'id' ) ||
(! $page = MT::Page->load( $app->param( 'id' ) ) ) {
return $app->trans_error( 'Invalid Page ID:\'[_1]\'', MT::Util::encode_html( $app->param( 'id' ) ) );
}
if ( $page->class ne 'page' ) {
return $app->trans_error( 'Invalid Class:\'[_1]\'', $page->class );
}
# permission check case 1
if ( $app->blog->id != $page->blog_id ) {
return $app->trans_error( 'Invalid BlogID:\'[_1]\'', MT::Util::encode_html( $app->blog->id ) );
}
if (! $app->can_do( 'manage_pages' ) ) {
return $app->trans_error( 'Permission denied.' );
}
# or case 2
my $perm = $app->user->is_superuser;
if (! $perm ) {
$perm = $app->user->permissions( $page->blog_id )->can_administer_website;
$perm = $app->user->permissions( $page->blog_id )->can_administer_blog unless $perm;
$perm = $app->user->permissions( $page->blog_id )->can_manage_pages unless $perm;
}
if (! $perm ) {
return $app->trans_error( 'Permission denied.' );
}
if (! $app->run_callbacks( 'cms_save_permission_filter.page' $app, $page ) {
return $app->trans_error( 'Permission denied.' );
}
# Do Something.
}
こんなことが本当にあるの? ってか、でも結構あるんですよね。だいぶ潰れてるとは思いますが、セキュリティ強化月間で徹底的にやっていただきたいものです。もちろん、弊社もセキュリティ強化月間です。
さて、実はもう一つ紹介したい例題があるのですが、さすがにちょっとここには書けないです。世の中には危ないコードがいっぱいありますからね。またいずれ気が変わったら書くかもしれません。