Fix Path Traversal: 管理画面ファイル操作のパストラバーサル脆弱性を修正#1342
Conversation
- CSS editor: strengthen checkPath() to block ".." and "/" in filenames - Page editor: add realpath validation in createPHPFile() to ensure files stay within USER_REALDIR - Template upload: validate archive contents before extraction (block path traversal, absolute paths, and symlinks) - FileManager: fix convertToAbsolutePath() to use str_starts_with() instead of str_contains() for secure path boundary check Refs #1337 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthrough複数のファイル管理・テンプレート操作でパストラバーサル防止の検証を追加・強化しました。パスの正規化とUSER_REALDIRプレフィックス確認、CSS名の検証強化、アップロードアーカイブ内の相対/絶対パスやシンボリックリンク検出を実装し、不正なパスを早期にブロックします。 Changes
Estimated code review effort🎯 3 (中程度) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1342 +/- ##
=======================================
Coverage 54.39% 54.40%
=======================================
Files 84 84
Lines 10815 10816 +1
=======================================
+ Hits 5883 5884 +1
Misses 4932 4932
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
data/class/pages/admin/design/LC_Page_Admin_Design_CSS.php (1)
298-310:⚠️ Potential issue | 🔴 Critical
checkPathの検証ロジックは適切ですが、doRegister/doDeleteの前に呼ばれていません
checkPathは Line 133 でファイル読み込み前にのみ呼ばれていますが、confirmモード（Line 97-112）とdeleteモード（Line 113-124）ではcheckPathによる検証なしに$css_path（$css_dir.$this->css_name.'.css'）を使ってファイル書き込み・削除が行われています。攻撃者が
css_nameに../../etc/configのような値を送信した場合、doRegisterやdoDeleteでパストラバーサルが可能になります。🔒 修正案: confirm/delete の前に checkPath を追加
switch ($this->getMode()) { // データ更新処理 case 'confirm': - if (!$is_error) { + if (!$is_error && $this->checkPath($this->css_name)) { $this->arrErr = array_merge($this->arrErr, $this->lfCheckError($objFormParam, $this->arrErr));case 'delete': - if (!$is_error) { + if (!$is_error && $this->checkPath($this->css_name)) { if ($this->doDelete($css_path)) {Based on learnings: "Check file operations for パストラバーサル and upload validation vulnerabilities"
🤖 Fix all issues with AI agents
In `@data/class/helper/SC_Helper_FileManager.php`:
- Around line 525-527: The prefix check using str_starts_with allows
directory-boundary bypass because realpath(USER_REALDIR) lacks a trailing slash;
in SC_Helper_FileManager.php adjust the check around $path and USER_REALDIR so
you compare with the base directory plus a directory separator (e.g., ensure
$base = rtrim(realpath(USER_REALDIR), DIRECTORY_SEPARATOR) .
DIRECTORY_SEPARATOR) and use that $base in str_starts_with or equivalent — also
normalize $path via realpath() before checking and fall back to $base when the
check fails to prevent paths like /var/www/user_data_evil from passing.
In `@data/class/pages/admin/design/LC_Page_Admin_Design_MainEdit.php`:
- Around line 375-380: The check using realpath(USER_REALDIR) + str_starts_with
can false-positive on similarly named dirs and fails when dirname($path) does
not exist; fix by ensuring USER_REALDIR has a trailing directory separator
before prefix checks (e.g. $userRealDir = rtrim(realpath(USER_REALDIR),
DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR) and use strict prefix check against
that value; if realpath(dirname($path)) returns false, construct a logical
absolute parent path by joining the canonical USER_REALDIR with
dirname($filename) (or use the existing
SC_Helper_FileManager::convertToAbsolutePath logic) and normalize path segments
(resolving "." and ".." without requiring the filesystem) before performing the
same trailing-slash prefix check; update both
LC_Page_Admin_Design_MainEdit::(the method containing $path/$filename checks)
and SC_Helper_FileManager::convertToAbsolutePath accordingly.
- str_starts_withのディレクトリ境界バイパス対策（末尾/付加） - CSS設定のconfirm/deleteモードにcheckPath検証を追加 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nanasess
left a comment
nanasess
left a comment
There was a problem hiding this comment.
Code review
Found 1 issue:
confirmモードでold_css_nameに対するcheckPath()が適用されていません。css_nameのみ検証されていますが、doRegister()内でold_css_nameが直接unlink()に使用されるため、攻撃者がcss_nameに正常な値、old_css_nameにパストラバーサルを含む値を送信することで、任意の.cssファイルを削除可能です。
ec-cube2/data/class/pages/admin/design/LC_Page_Admin_Design_CSS.php
Lines 97 to 99 in 44d9cde
doRegister() 内の該当箇所:
ec-cube2/data/class/pages/admin/design/LC_Page_Admin_Design_CSS.php
Lines 179 to 187 in 44d9cde
suggested fix:
- if (!$is_error && $this->checkPath($this->css_name)) {
+ if (!$is_error && $this->checkPath($this->css_name) && $this->checkPath($this->old_css_name)) {🤖 Generated with Claude Code
- If this code review was useful, please react with 👍. Otherwise, react with 👎.
|
上記の レビュー観点以下の3つの観点でレビューを実施しました。
各ファイルの評価
🤖 Generated with Claude Code |
nanasess
left a comment
nanasess
left a comment
There was a problem hiding this comment.
@nobuhiko
以下のコメントを確認お願いします🙇♂️
#1342 (review)
doRegister() で old_css_name を使ったファイル削除が行われるため、 パストラバーサル防止のために checkPath() による検証を追加。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nanasess
left a comment
nanasess
left a comment
There was a problem hiding this comment.
LGTM
多層防御の観点から有効ですのでマージします
2 participants
Summary
checkPath()を強化し、..や/を含むファイル名をブロックcreatePHPFile()にrealpath()+str_starts_with()でディレクトリ境界チェックを追加convertToAbsolutePath()でstr_contains()→str_starts_with()に修正し、パス境界チェックを厳格化Test plan
Refs #1337
🤖 Generated with Claude Code
Summary by CodeRabbit