0
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

2つ目のRichEditorを追加したとき、サニタイズ処理だけコピーし忘れていた話

0
Posted at

「便利だから」と、FilamentのRichEditor*1 コンポーネントを2つ目・3つ目のモデルにも追加したことはありませんか。私はあります。そして、最初のモデルにはちゃんと書いていた「保存前にHTMLを無害化する処理」を、2つ目以降にコピーし忘れて、保存型XSS*2 の穴を空けたままリリースしてしまったことがあります。「1つ目はちゃんとやったのに、2つ目を追加したときにだけ忘れる」というのは誰にでも起こりうるので、同じ轍を踏まないように共有します。

何が起きていたか

最初に作ったブログ記事モデル(Post)では、本文を保存する前にサニタイズ(無害な形に変換すること)していました。

class Post extends Model
{
    public function setContentAttribute($value): void
    {
        $this->attributes['content'] = MyHtmlPurifier::purify($value);
    }
}

setContentAttributeは、Laravelのミューテタ*3(属性を保存する直前に横から処理を挟める仕組み)です。ここでHTMLを無害化してから保存していたので、Postモデルは安全でした。

その後、会社概要・利用規約・サービス紹介など、同じくRichEditorで書ける管理画面をいくつか追加しました。しかし、これらのモデル(CompanyProfilePolicyService)には同じサニタイズ処理を書き忘れていました。1つ目のモデルを作ったときの自分は「ちゃんとサニタイズしなきゃ」と意識していたのに、2つ目以降を作ったときの自分は、もうその意識が抜け落ちていた、という状態です。

FilamentのRichEditorは見た目上はWYSIWYG(見たまま編集できるエディタ)*4 なので、普段の管理者操作では何も問題が表面化しません。ただし、貼り付け操作や、細工されたリクエストによっては、ツールバーに無いような生のHTML・スクリプトが保存されうる経路でもあります。サニタイズしていないモデルに保存された内容を、そのままフロント側で{!! $content !!}のように出力すれば、そのスクリプトはブラウザ上で実行されてしまいます。

「社内の管理画面から入力されるものだから、変な内容が入るはずがない」と思ってしまいがちですが、管理画面という「信頼できるはずの入力元」に対しても、保存前のサニタイズは省略できない、というのが今回の一番の教訓です。

なぜ横展開し忘れるのか

  • 最初のモデルのサニタイズ処理が、そのモデル専用のprivateメソッドとして書かれていて、他のモデルから再利用する導線が用意されていなかった。コピペするにも「どこにその処理があったか」を覚えていないと再現できません。
  • 「RichEditorを追加する」というタスクをやるとき、意識が向くのは表示側(Blade*5 側でちゃんとエスケープ*6 されているか)で、保存側のサニタイズは見落とされやすい
  • レビューでも「見た目が正しく表示されるか」は確認されるが、「悪意のあるHTMLを入れたらどうなるか」は普段のQA観点に入りにくい。

直し方:サニタイズを再利用可能なtraitに切り出す

モデル固有のメソッドではなく、共通のtrait*7(複数のクラスに同じ機能を持たせるための、Laravel/PHPの仕組み)として切り出し、RichEditorを使うモデルには必ずこのtraitを使わせるようにしました。

namespace App\Models\Concerns;

use HTMLPurifier;
use HTMLPurifier_Config;

trait SanitizesHtml
{
    protected static function sanitizeHtml(?string $value): string
    {
        return static::htmlPurifier()->purify((string) $value);
    }

    private static function htmlPurifier(): HTMLPurifier
    {
        static $purifier = null;

        if ($purifier === null) {
            $config = HTMLPurifier_Config::createDefault();
            // 許可タグはホワイトリスト方式*8 で明示。scriptやonclick等の属性は入る余地がない。
            $config->set('HTML.Allowed', 'p,br,h2,h3,h4,strong,em,b,i,u,s,a[href|title],ul,ol,li,blockquote,code,pre,img[src|alt|width|height],table,thead,tbody,tr,th,td');
            $config->set('AutoFormat.RemoveEmpty', true);
            $purifier = new HTMLPurifier($config);
        }

        return $purifier;
    }
}

各モデル側は、このtraitを使ってsetXxxAttribute()から呼ぶだけです。同じ手間を毎回繰り返す必要がなくなるので、「今度こそ書き忘れる」ということが起きにくくなります。

class Policy extends Model
{
    use SanitizesHtml;

    public function setBodyAttribute($value): void
    {
        $this->attributes['body'] = self::sanitizeHtml($value);
    }
}

CompanyProfileServiceも同様に追加しました。多言語対応でsetTranslation()経由の保存経路がある場合は、そちらも同じサニタイズを通すのを忘れないようにします(今回はここも一度漏れていて、あわせて直しました)。

テストは「許可していないタグ(<script>onerror属性など)を入力したら、保存後にそれらが除去されていること」を素直に確認する形で書けます(下記はPest*9 での例)。

it('RichEditorのscriptタグは保存時に除去される', function () {
    $policy = Policy::factory()->create([
        'body' => '<p>本文</p><script>alert(1)</script>',
    ]);

    expect($policy->fresh()->body)
        ->not->toContain('<script>')
        ->toContain('<p>本文</p>');
});

教訓:次に自分がまた忘れないために

  • サニタイズ処理はモデル専用のprivateメソッドにせず、最初から再利用可能なtrait/共通クラスにしておく。1箇所目を書いた時点で「これは他のモデルにも要る」と想定しておくのが一番安い予防策です。
  • 「RichEditorコンポーネントを追加する」タスクのチェックリストに、保存側のサニタイズ確認を必須項目として入れておく。表示側のエスケープだけでは足りません。
  • 同じ理由で、既存レコードにXSSペイロード(攻撃用に埋め込まれたスクリプトなどの文字列)が残っていないかは別途確認が必要です。ミューテタは「保存する瞬間」にしか効かないため、修正前に保存された既存データはサニタイズされないまま残ります。バックフィル(既存データへの後追い適用)するかどうかは、実データを見て判断してください。

もし「うちも似たようなRichEditor、複数モデルに入れてるかも」と思ったら、ぜひ一度、それぞれのモデルで保存前のサニタイズがちゃんと効いているか確認してみてください。1つ目だけ安全で、2つ目以降が素通しになっている、というのは本当によくあるパターンです。


このバグは、フリーランス・少人数チーム向けの案件管理ツール AYUMI(歩) を作っている過程で、他の自社プロジェクトのセキュリティ点検をしていて見つけたものです。よかったら覗いてみてください。

用語メモ

  • *1 RichEditor:Filamentが提供する、太字・見出し・リンクなどを見た目で編集できる入力コンポーネント。裏側では入力内容がHTMLとして保存される。
  • *2 保存型XSS(ストアドXSS):悪意のあるスクリプトがデータベースに保存されてしまい、そのデータを表示するたびに他のユーザーのブラウザ上で実行されてしまう種類の脆弱性。一度きりでなく「表示するたびに」実行される点が厄介。
  • *3 ミューテタ(mutator):Laravelで、モデルの属性に値を保存する直前・取得する直前に、独自の処理を差し込める仕組み。setXxxAttribute()は保存直前に呼ばれる。
  • *4 WYSIWYG(ウィジウィグ):「What You See Is What You Get」の略。ボタン操作で太字や見出しを付けると、裏側で対応するHTMLタグが自動生成される種類のエディタ。
  • *5 Blade:Laravel標準のテンプレートエンジン。画面(View)を組み立てるためのファイル形式。
  • *6 エスケープ:文字列の中の特殊な記号(<>など)を、HTMLとして解釈されない安全な形に変換すること。表示側の対策の基本。
  • *7 trait:PHPで、複数のクラスに同じメソッド群をまとめて持たせるための仕組み。継承とは別に、機能を「横に」共有できる。
  • *8 ホワイトリスト方式:「許可するものだけを明示的に指定し、それ以外は全部弾く」という考え方。逆に「危険なものだけを列挙して弾く」方式(ブラックリスト方式)は、列挙漏れが起きやすく安全性が低いとされる。
  • *9 Pest:Laravelでよく使われるテスティングフレームワーク。it('〜する', function () { ... })のように、テスト内容を文章に近い形で書けるのが特徴。
0
0
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
0
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?