クマのブログ

つまづいたところ、学びを書いていきます

「DBへの保存時にエスケープしちゃいけない」からわかったこと

背景

  • バグ改修をせっせとやっていた時の話
  • その一つに「XSS発生の対応」があった
  • その時に見事にやらかした話
  • 言語はLaravel

バグ状況

状況はこんな感じ

  1. 管理画面でユーザーの注文一覧を見れる
  2. この各注文に管理者が「メモ」を書けるようにしていた
  3. この「メモ」がtextareaで書かれていた
  4. この「メモ」に以下のようなコードを書くとJSが実行されてしまうバグが発生していた
<script>alert('危険！')</script>

典型的なXSSですね

当初の実装方針

  • 注文の一覧表示までの処理の流れはこう
  • ユーザーが注文＝注文テーブルへの保存処理

  • 管理者が注文一覧を閲覧＝注文テーブルからデータを取得

  • そのうえで、私の実装方針は

「じゃあ、1. の保存時にエスケープ処理して対応して終わりじゃん」

と思い、「メモ」のsave時にe()（※）を実装しました。イメージは以下。

public function update(Request $request)
{
    ...
    DB::beginTransaction();
  try {
      $user->order->fill([
        // e()でエスケープ処理で対応
        'memo' => e($validated['memo']),
      ]);
      $authenticatedUser->cart->save();
      DB::commit();
    ...
    }

※e()はLaravelのエスケープ処理メソッド

結論

もうすでに「エスケープされたデータがDBに保存される」という問題が発生しており、「注文一覧の出力時に、エスケープ処理された文字（特殊文字）がそのまま出力してしまう」という脅威を招いている。

ということで即修正。

本当の問題点

ただ、テストを全くしてないわけではなかった。

実際に「管理画面の注文一覧ではXSSは発生せず出力内容に問題はなかった」ことは確認済みだった。

  • 実際にタスクが完了したのちに、再度確認してみると本当の問題点が発覚。

起こっていた問題点

実装箇所を間違えてテストをしていました。。。

原因①：なぜエスケープ処理したのか（なぜ）

  • 「ソースコードを読み違えによる見落とし」が原因
    • 本来改修すべきは注文新規登録（＝storeメソッド）
    • ただ、自分が今回実装した箇所は注文編集（＝updateメソッド）
    • テストしたのは改修をしたと思い込んでいる新規登録箇所
      • ココでは何も追加実装をしていないので、当たり前のように正常に表示される

原因②：なぜ実装箇所とテスト範囲が違うのか

  • これはデバッグ不足
    • ちゃんと本来改修すべきstoreメソッドでXSSが起きていることをPHP Debugで確認せずに実装した
  • 自分の中では無意識に

「新規登録処理の中でupdateメソッドを通っている」

と思い込んで実装していた

原因③：なぜデバッグを怠ったのか

  • 主に2点
    • バグ改修の期限が迫っており、精神的に焦っていたため
    • 最近デバッグせずとも実装できることが多かったので、驕った

再発防止

レビュー前のチェック項目の追加

  • 「想定している処理をしているか、デバッグを使って確認したか」
    • レビュー前には自作のチェックシートでテスト漏れがないか確認してます。
    • これの追加し、必ず想定したソースコードを通って処理されていることをテストする

所見

  • なかなかしょぼいことしたな、と実感したので自戒

  • 当初は「XSSの理解不足」でまとめようとしたら、問題はそこじゃなかったので記事の方向性を軌道修正しました。