search

vektor-inc / vk-all-in-one-expansion-unit

wordpress plugin of powerful support
https://ja.wordpress.org/plugins/vk-all-in-one-expansion-unit/
GNU General Public License v2.0
7 stars 1 forks source link

XSS 対応 #1101

Closed drill-lancer closed 3 months ago

drill-lancer commented 3 months ago

チケットへのリンク / 変更の理由（元のissueがあればリンクを貼り付ければOK）

https://github.com/vektor-inc/vk-all-in-one-expansion-unit/issues/1100

どういう変更をしたか？

上記に潜んでいた XSS を潰しました

P.S. メイン設定については潰れているようでした。

実装者はレビュワーに回す前に以下の事を確認してチェックをつけてください。

ソースコードについて

デザイン・UI

プログラムの変更の場合

テストを書かないのは普通ではありません。書けるテストは極力書くようにしてください。 書いていない場合は書かない理由を記載してください。

その他

変更内容について何を確認したか、どういう方法で確認をしたかなど

上記の input[type="text"] に "><script>alert(0)</script> を入れてアラートが出ないのを確認しました。

確認URL

ローカル環境

レビュワーの確認方法・確認する内容など

上記の確認をお願いします。

レビュワーに回す前の確認事項

レビュワー向け

確認して変更が反映されていない場合の確認事項

drill-lancer commented 3 months ago

@mthaichi stripslashes はエスケープには関係がないと思ったので削除しました。 そういう意味があったというのを知りませんでした。勉強になりました。 というわけで、stripslashes が必要そうなところにはそれを施してみました。

drill-lancer commented 3 months ago

@kurudrive ちょっと上にそれっぽい許可条件があったのでそれを元にエスケープしてみました。

mthaichi commented 3 months ago

@mthaichi stripslashes はエスケープには関係がないと思ったので削除しました。 そういう意味があったというのを知りませんでした。勉強になりました。 というわけで、stripslashes が必要そうなところにはそれを施してみました。

@drill-lancer ありがとうございます！ 適切に修正されていると思います。

mthaichi commented 3 months ago

@drill-lancer ありがとうございます！ 適切に修正されていると思います。

@kurudrive @drill-lancer あと、XSSとまではいかないので must ではないのですが、 たとえば、連絡先の「電話アイコン」は、HTMLは必要ないので、wp_kses_post ではなく、wp_strip_all_tagsを使うといいかなと思います。

「電話番号」については、太字にしたいとか色をつけたいというニーズがあるので、wp_kses_postで良いかもしれません。

余計なHTMLを許可しているのはあまり印象が良くないので、この機会に修正頂くと良いと思います。

面倒な修正、ありがとうございます！

mthaichi commented 3 months ago

@kurudrive @drill-lancer "echo $" でプロジェクト全体に全文検索かけて、ひっかかった変数が、入力由来のものであればエスケープをかけるという感じで残りを潰していってよいと思います。

drill-lancer commented 3 months ago

@mthaichi

連絡先の「電話アイコン」は、HTMLは必要ない

i タグで書くものと思って wp_kses_post にしています。

mthaichi commented 3 months ago

@mthaichi

連絡先の「電話アイコン」は、HTMLは必要ない

i タグで書くものと思って wp_kses_post にしています。

@drill-lancer なるほど、でもここは fas fa-phone-square とクラス名を書く感じではないでしょうか。 font awesomeで iタグをそのままコピペしても通るようにしてあるのであれば、最低限の wp_kses_postでよいと思いますが、としても、理想を言えば wp_ksesで iタグのみ通るようにすべきところだと思います。

長い目でみても、バリデーションやサニタイズを厳しめにしたほうがいいので、HTMLいれる必要がないフィールドは wp_strip_all_tags でHTML全削除したほうが良いと私は思います。ご面倒をおかけしますが（・人・）

kurudrive commented 3 months ago

@mthaichi @drill-lancer アイコンの部分はもともと クラス名入力だけだったものを後から i タグで入力してもらうように仕様変更しているケースもあるので、少なくとも i タグの許可は必要です（・ｗ・

mthaichi commented 3 months ago

@mthaichi @drill-lancer アイコンの部分はもともと クラス名入力だけだったものを後から i タグで入力してもらうように仕様変更しているケースもあるので、少なくとも i タグの許可は必要です（・ｗ・

@kurudrive @drill-lancer なるほど、tel_iconだけですが直してみましたー。

mthaichi commented 3 months ago

すみません。私このあと用事がございまして、@kurudriveさんが見てOK なら良いと思います。🙏

kurudrive commented 3 months ago

@drill-lancer @mthaichi ありがとうございました！