同一画像を参照している商品を削除した際の挙動

[chihiro-adachi]

概要(Overview)

同一の画像を参照している商品を削除した際、以下の挙動となる。

dtb_product	dtb_product_image	save_image配下の画像ファイル
商品A	画像A	画像A
商品B	画像A	画像A

dtb_product	dtb_product_image	save_image配下の画像ファイル
商品A	画像A	(削除)
商品B(削除)	画像A(削除)	(削除)

この際、#4575 の修正により、画像パスのチェックが厳密になったため、商品登録画面から商品Aの編集ができない

商品Aを修正する手順としては、以下の対応が必要になる

• 商品画像をFTP等でアップロード
• 商品CSV登録で、上記が画像ファイルに更新する

特に2系からの移行を行ったサイトで発生しており、改善したい

期待する内容(Expect) or 要望 (Requirement)

• 商品画像のバリデーション処理の見直し
  • 商品登録画面から編集ができるようにする
• 商品を削除する際に、商品画像の削除処理を見直し
  • 他の商品で画像が利用されている場合は画像削除処理を行わないなど

再現手順(Procedure)

概要に記載のとおり

環境 (environment)

• EC-CUBE: 4.0.x
• PHP: 7.x.x
• DB:
  • PostgreSQL x.x.x
  • MySQL x.x.x

関連情報 (Ref)

[izayoi256]

@okazy
#4575 はパストラバーサル攻撃に対する修正ですか？
もしそうなら、以下のどちらかへの変更を提案します。

• バリデーションを廃して、ファイル(移動|削除)時にbasenameする。
  (カスタマイズ等によって、パスにディレクトリを含むケースで問題が発生する)

• パストラバーサル攻撃につながる階層移動(./や../)を弾くバリデーションにする。
  (hoge../fuga.jpgはvalidであるため正規表現を考えるのが大変そう)

後者は別の穴を作る可能性もあるので、カスタマイズのことを考慮する必要がなければ前者がいいと思いますが…。

[okazy]

@izayoi256

#4575 はパストラバーサル攻撃に対する修正ですか？
はい、そうです。

ご提案ありがとうございます。

バリデーションを廃して、ファイル(移動|削除)時に basename する。

こちらの対策が一番確実かと思いました。
確かにカスタマイズされている場合に影響がある可能性がありますが、セキュリティ上クリティカルな機能なのと、パスにディレクトリを含めるのそもそもカスタマイズとして好ましいものではないと思いますので、私個人の意見としては対策を入れてもいいのかなと思っています。
こちらは他の方のご意見も伺いたいところです。

[okazy]

• 商品を削除する際に、商品画像の削除処理を見直し
  • 他の商品で画像が利用されている場合は画像削除処理を行わないなど

こちらは #4767 で修正いただきました。

[chihiro-adachi]

@izayoi256 @okazy

バリデーションを廃して、ファイル(移動|削除)時にbasenameする。
(カスタマイズ等によって、パスにディレクトリを含むケースで問題が発生する)

2系から移行される場合に、save_image配下にディレクトリを切って画像を登録しているケースがよくあります。
移行の際に画像の配置も見直してもらうことができればよいのですが、なかなかそうもいかずというところです...