search

EC-CUBE / ec-cube2

EC-CUBE official repository version 2
https://www.ec-cube.net
Other
86 stars 97 forks source link

SC_Response::sendRedirect() transactionid= を画一的に追加せず、一定条件での継承のみとする #922

Closed seasoftjapan closed 1 month ago

seasoftjapan commented 3 months ago

不要ならば削除したい。少なくとも全てのリダイレクトに適用する必要は無いと思うので、必要な箇所のみに適用したい。

背景・参考

nanasess commented 3 months ago

導入当初はガラケーなど、cookie を使用できない環境への配慮もあったのですが、現代では不要ですね。

以下の脆弱性対応のため、GET でも CSRF トークンのチェックをしているページがあります。 https://www.ec-cube.net/info/weakness/weakness.php?id=64

seasoftjapan commented 3 months ago

以下の脆弱性対応のため、GET でも CSRF トークンのチェックをしているページがあります。 https://www.ec-cube.net/info/weakness/weakness.php?id=64

謎な適用もありますね。本来は必要な処理を POST に変更してチェックするのが良さそうですが、応急対応として本質的な要否に関わらず mode で割り切って判定している感じですかね。

いずれにしても、本メソッドが絡むリダイレクトに関しましては、transactionid= を追加する必要は無いと想定していますが、良さそうでしょうか?

seasoftjapan commented 3 months ago

transactionid を受け取ったリクエストに関して、値を継承してリダイレクトするなどが妥当なような。

とりあえず値を継承するパターンを実装してみました。 https://github.com/EC-CUBE/ec-cube2/compare/master...seasoftjapan:eccube-2_13:seasoft-922

需要があるか不明なので、さくっと削除してしまう方が良いのか、意見をいただけますと幸いです。

nanasess commented 3 months ago

@seasoftjapan 本質的には SC_Response::sendRedirect() で transactionid を発行する必要は無さそうです。 一部、GET で CSRF チェックをしている箇所のみ利用できれば問題ないと思います

seasoftjapan commented 3 months ago

@nanasess https://github.com/EC-CUBE/ec-cube2/compare/master...seasoftjapan:eccube-2_13:seasoft-922 にて、transactionid の新たな付与は行わず、transactionid を含む URL を別の URL へリダイレクトするときのみ transactionid を継承する (リダイレクト前に transactionid が無ければ transactionid は付与しない) 仕様としました。

/aaa → /bbb?x=1
/aaa?transactionid=foo → /bbb?x=1&transactionid=foo

その対応も不要で、リダイレクト前にあった transactionid も切り捨てて良い感じでしょうか?

/aaa?transactionid=foo → /bbb?x=1
nanasess commented 3 months ago

@seasoftjapan クエリストリングに mode が含まれる場合は、GET でも CSRF のチェックをしています。 https://github.com/EC-CUBE/ec-cube2/blob/a7236b6c23a7e0c8f17481e8f543078b30ec9bd7/data/class/pages/LC_Page.php#L649-L655

リダイレクトの際に mode が付与される場合は transactionid が必要ですが、それ以外は切り捨ててしまって問題ないと思っています

seasoftjapan commented 3 months ago

そもそもリダイレクトを通らないケースが大半だと思いますし、アクセスログの観点ではリダイレクト前も記録されますから、「管理画面」「mode なし」「transactionid あり」というリクエストを発生させる箇所があったら、そっちを改められないかの観点も確認した方が良さそうですね。

ちょっと嫌な予感がするのが、リダイレクト前後で mode なし → あり に変わるパターンがあると、厄介ですね…。 無いよなフツー・・・と、無いことを祈りつつ、確認しようと思います。

もう一つ気づきました。先日の https://github.com/EC-CUBE/ec-cube2/compare/master...seasoftjapan:eccube-2_13:seasoft-922 は、PRG パターンを考慮できていませんでした。 管理画面はリダイレクト先に mode がある場合は transactionid を維持 (新たな付与はしない)。フロントはサクッと削除。これで再検討しようと思います。

nanasess commented 3 months ago

@seasoftjapan

そもそもリダイレクトを通らないケースが大半だと思いますし、アクセスログの観点ではリダイレクト前も記録されますから、「管理画面」「mode なし」「transactionid あり」というリクエストを発生させる箇所があったら、そっちを改められないかの観点も確認した方が良さそうですね。

おっしゃるとおりですね

ちょっと嫌な予感がするのが、リダイレクト前後で mode なし → あり に変わるパターンがあると、厄介ですね…。 無いよなフツー・・・と、無いことを祈りつつ、確認しようと思います。

sendRedirect で grep して調べた感じは無さそうでした。

管理画面はリダイレクト先に mode がある場合は transactionid を維持 (新たな付与はしない)。フロントはサクッと削除。これで再検討しようと思います。

よろしくお願いいたします🙇‍♂️

seasoftjapan commented 1 month ago

ちょっと嫌な予感がするのが、リダイレクト前後で mode なし → あり に変わるパターンがあると、厄介ですね…。 無いよなフツー・・・と、無いことを祈りつつ、確認しようと思います。

sendRedirect で grep して調べた感じは無さそうでした。

情報ありがとうございます。 SC_Response::sendRedirectFromUrlPath() SC_Response::reload() SC_Display::reload() についても確認しましたが大丈夫そうでした。

seasoftjapan commented 1 month ago

備忘録を兼ねてまとめます。

https://github.com/EC-CUBE/ec-cube2/issues/922#issuecomment-2143859746 から抜粋:

@seasoftjapan クエリストリングに mode が含まれる場合は、GET でも CSRF のチェックをしています。 リダイレクトの際に mode が付与される場合は transactionid が必要ですが、それ以外は切り捨ててしまって問題ないと思っています

PRG パターン

「メルマガ管理＞テンプレート設定」画面 (/admin/mail/template_input.php) で登録する際、以下の流れが発生する。

POST /admin/mail/template_input.php? (302 Found)
GET /admin/mail/template_input.php?mode=complete

フロント同様に transactionid を破棄してしまうと、完了画面を表示できない。

完了画面を PRG で表示するために transactionid を維持するのはバカバカしいけど、応急対応だから仕方がない。考慮して実装する。

もう一つ気づきました。先日の https://github.com/EC-CUBE/ec-cube2/compare/master...seasoftjapan:eccube-2_13:seasoft-922 は、PRG パターンを考慮できていませんでした。

$_REQUEST で実装していたので、結果 PRG でも動く。(というか、上のコメント時、PRP パターンが念頭にあったと思うが、本件メソッドが扱うのは (307 ではなく) 302 リダイレクトなので、考慮不要だった。)

よって、これをベースに、フロント画面や、管理画面で mode が無い場合、transactionid を維持しないよう分岐すれば良さそう (不要に transactionid が含まれるのを避けられる)。

GRG パターン (って用語があるか知らんけど)

以下のような mode ありの GET 同士のリダイレクトでも考慮が必要なはず。

GET /admin/hogehoge.php?mode=foo (302 Found)
GET /admin/hogehoge.php?mode=bar

しかし、こういった使い方が実在するか不明。

https://github.com/EC-CUBE/ec-cube2/compare/master...seasoftjapan:eccube-2_13:seasoft-922 で動作するので、新たに難が見つからなければ維持する予定。