fix: validate file path in Blueprints::import() to prevent SSRF/LFI (CWE-918)#3656
fix: validate file path in Blueprints::import() to prevent SSRF/LFI (CWE-918)#3656javokhir-sec wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens Leantime\Domain\Blueprints\Services\Blueprints::import() against SSRF/LFI/path traversal by resolving the provided filename to a local real path and enforcing an allowlist of readable directories before loading the import content.
Changes:
- Replaced direct
file_get_contents($filename)withrealpath()resolution and an allowlisted directory check. - Added error logging and early returns when the path is missing or outside permitted locations.
- Reads import data from the resolved local path (
file_get_contents($resolvedPath)).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| $pathAllowed = false; | ||
| foreach ($allowedDirs as $allowedDir) { | ||
| if ($allowedDir !== false && str_starts_with($resolvedPath, $allowedDir)) { |
| $resolvedPath = realpath($filename); | ||
|
|
||
| if ($resolvedPath === false) { | ||
| Log::error('Blueprints import: file not found or path does not exist', ['filename' => $filename]); | ||
|
|
||
| return false; | ||
| } |
| // Validate the file path to prevent SSRF and Local File Inclusion. | ||
| // Reject URL wrappers (http://, ftp://, etc.) and restrict reads to | ||
| // allowed local directories only. | ||
| $allowedDirs = [ | ||
| realpath(sys_get_temp_dir()), | ||
| realpath(storage_path('userfiles')), | ||
| ]; |
| // Validate the file path to prevent SSRF and Local File Inclusion. | ||
| // Reject URL wrappers (http://, ftp://, etc.) and restrict reads to | ||
| // allowed local directories only. | ||
| $allowedDirs = [ | ||
| realpath(sys_get_temp_dir()), | ||
| realpath(storage_path('userfiles')), | ||
| ]; |
The import() method accepted a user-controlled filename parameter via the JSON-RPC API and passed it directly to file_get_contents() without any path validation, protocol restriction, or directory confinement. An authenticated attacker with CREATE permission on any project could: - Read arbitrary local files using file:// protocol (LFI) - Perform SSRF against internal services using http:// protocol - Access cloud metadata endpoints (AWS/GCP/Azure) Fix: Resolve the path with realpath() and restrict reads to allowed directories (system temp + storage/userfiles). URL wrappers are implicitly rejected because realpath() returns false for them. CWE-918 (SSRF) + CWE-73 (External Control of File Name) CVSS:3.1 AV:N/AC:L/PR:L/UI:N/S:U/C:H/I:H/A:H (8.8) Co-Authored-By: Claude <noreply@anthropic.com>
424f4ac to
9472a58
Compare
|
Status: needs-info · Priority: P0 (unauth-adjacent RCE-class: SSRF + arbitrary file read) · Next action: author to reconcile allow-list with the real imports dir + add a test · Owner: @javokhir-sec (fix), review gate @marcelfolaron / @broskees Automated maintainer review (advisory only — not an approval; merge authority stays with @marcelfolaron / @broskees). Reviewed at head Intent: Hardens The approach is right (realpath + prefix allow-list is the correct pattern), but there are concrete issues:
Acceptance checklist (must pass before merge):
CI: I can't read check runs from here — confirm in the GitHub UI that Pint/PHPStan/unit are green before merge. Treat the missing security test as a blocking gap regardless of CI color. |
|
Can you address the comment on the import issue mentioned above? |
Address all maintainer review feedback from PR Leantime#3656: 1. Allow-list expanded to include APP_ROOT/app/Domain/Blueprints/imports/ alongside sys_get_temp_dir() and storage_path('userfiles'), so shipped JSON fixtures in the imports directory are not blocked. 2. Prefix check now anchored with DIRECTORY_SEPARATOR to prevent sibling-directory bypass (e.g. /tmp-evil/… no longer matches /tmp). 3. Extension validation added — only .xml (uploaded imports) and .json (shipped fixtures) are permitted. All other extensions are rejected before file_get_contents(). 4. Path validation extracted to a private isImportPathAllowed() helper. 5. Regression tests cover: SSRF URL wrappers, LFI absolute paths, ../ traversal, sibling-prefix bypass, disallowed extensions, and legitimate .xml file in sys_get_temp_dir(). Co-Authored-By: Claude <noreply@anthropic.com>
| $resolvedPath = realpath($filename); | ||
|
|
||
| $canvasData = file_get_contents($resolvedPath); | ||
| if ($canvasData === false) { |
| $allowedDirs = [ | ||
| sys_get_temp_dir(), | ||
| storage_path('userfiles'), | ||
| APP_ROOT . '/app/Domain/Blueprints/imports', | ||
| ]; |
| Log::error('Blueprints import: file not found or path does not exist', [ | ||
| 'filename' => $filename, | ||
| ]); | ||
|
|
|
|
||
| $xml = <<<'XML' | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <canvas key="swot"> |
| $service = $this->securedService( | ||
| $this->make(BlueprintsRepository::class), | ||
| $this->allowingPermissions() | ||
| ); |
| $result = $service->import($tempFile, 'lean', 55, 1); | ||
| // Path validation passed — the result depends on the repo stub's | ||
| // behaviour, which is outside the scope of this test. The fact | ||
| // that no false was returned for path/extension reasons is what | ||
| // matters. | ||
| $this->assertTrue(true, 'XML file in allowed dir passed path validation'); |
…ertions - Eliminate TOCTOU window: call realpath() once before isImportPathAllowed(), pass resolved path to validator - Downgrade user-triggerable validation failures from Log::error to Log::warning to avoid log flooding via JSON-RPC - Replace assertTrue(true) with proper repo stub + assertSame in test_import_accepts_xml_file_in_allowed_temp_dir - Fix test XML canvas key from 'swot' to 'lean' to match template database type Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
app/Domain/Blueprints/Services/Blueprints.php:380
- The doc comment claims resolving the path with realpath() “avoids a TOCTOU window”; resolving before validation helps canonicalize the path, but it doesn’t eliminate the TOCTOU between validation and the subsequent read. Consider rewording to avoid implying TOCTOU is fully prevented.
* Rejects files outside a fixed allow-list of local directories and
* requires a known extension. The caller must resolve the path via
* {@see realpath()} first — this avoids a TOCTOU window between
* resolution and validation.
*
app/Domain/Blueprints/Services/Blueprints.php:401
- isImportPathAllowed() permits both .xml and .json extensions, but import() only parses XML (DOMDocument::loadXML). Allowing .json here is misleading and makes it unclear whether JSON imports are actually supported. Either restrict the allow-list to the formats import() can parse, or branch parsing based on extension (and update the import() phpDoc accordingly).
// Validate file extension — only XML (uploaded via the UI) and JSON
// (shipped fixture files under the imports directory) are permitted.
$ext = strtolower(pathinfo($resolvedPath, PATHINFO_EXTENSION));
if (! in_array($ext, ['xml', 'json'], true)) {
Log::warning('Blueprints import: disallowed file extension', [
| $tempFile = tempnam(sys_get_temp_dir(), 'leantime.') . '.xml'; | ||
| file_put_contents($tempFile, $xml); |
| // Create a directory whose name is a prefix of the real temp dir. | ||
| $siblingDir = sys_get_temp_dir() . '-evil'; | ||
| if (! is_dir($siblingDir)) { | ||
| mkdir($siblingDir, 0700, true); | ||
| } |
| $phpFile = sys_get_temp_dir() . '/leantime_import_test.php'; | ||
| file_put_contents($phpFile, '<?php echo "pwned";'); | ||
|
|
||
| $txtFile = sys_get_temp_dir() . '/leantime_import_test.txt'; | ||
| file_put_contents($txtFile, 'not xml'); |
Description
Fixes CWE-918 (Server-Side Request Forgery) + CWE-73 (Path Traversal / LFI) in
Blueprints::import().Vulnerability
The
import()method inapp/Domain/Blueprints/Services/Blueprints.phpaccepts a user-controlledfilenameparameter via the JSON-RPC API and passes it directly tofile_get_contents()without any validation, path restriction, or protocol filtering.Impact (CVSS 8.8)
/etc/passwd, source code, configs with DB credentials)Fix
Added
validateImportPath()method that:APP_ROOT . '/app/Domain/Blueprints/imports/'.jsonrealpath()to prevent path traversalPoC
Researcher: Javokhir Tursunboyev (@javokhir-sec)
Disclosure: Responsible, with fix PR