Repository navigation
Fix - Unauthenticated arbitrary file deletion via upload field cleanup - #1672
Conversation
The disabled-entries cleanup rebuilt the delete path from an attacker
supplied file URL with a greedy preg_replace('/.*wp-content/', ...) and
no realpath or uploads containment, so a crafted old_files URL (a second
wp-content segment) resolved outside wp-content/uploads and was unlinked.
Route the delete sink through resolve_uploads_file_from_url(), which the
accept path and the other callers in this class already use, and delete
via wp_delete_file() so the file that is removed is the same validated,
uploads-contained path.
QA suite — refused, no regression specThis PR changes product source but adds no spec, so the suite was Source files changed with no matching spec |
There was a problem hiding this comment.
🟡 Changes recommended
File ownership is not verified, allowing unauthenticated deletion of other uploads.
1 open finding
What changed in this PR
This PR hardens uploaded-file cleanup against unsafe path derivation.
Changes:
- Validates upload paths before deletion.
- Replaces direct
unlink()usage withwp_delete_file(). - Documents the security fix.
A critical issue remains: attacker-controlled old_files can still delete another file within the uploads directory.
| File | Description |
|---|---|
includes/abstracts/class-evf-form-fields-upload.php |
Validates upload paths before cleanup deletion. |
changelog.txt |
Documents the security fix. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| $file_url = isset( $file_data['value'] ) && is_string( $file_data['value'] ) ? $file_data['value'] : ''; | ||
| $resolved_path = $this->resolve_uploads_file_from_url( $file_url ); | ||
|
|
||
| $uploaded_file = ABSPATH . preg_replace( '/.*wp-content/', 'wp-content', wp_parse_url( $file_url, PHP_URL_PATH ) ); | ||
| if ( file_exists( $uploaded_file ) ) { | ||
| unlink( $uploaded_file ); | ||
| } | ||
| if ( false !== $resolved_path ) { | ||
| wp_delete_file( $resolved_path ); |
Containment to the uploads root still let an unauthenticated submitter delete any existing file under wp-content/uploads via a crafted old_files URL, because old_files is taken from the request and only checked for containment, not ownership. Restrict deletion to the form's own hash-protected everest_forms_uploads/<id>-<wp_hash> folder, which a request cannot forge, so a resolved path outside that folder is skipped.
| @@ -1672,13 +1674,12 @@ public function remove_csv_file_after_email_send( $attachment, $entry, $form_dat | |||
| if ( isset( $meta_value['type'] ) && ( 'file-upload' === $meta_value['type'] && ! empty( $meta_value['value_raw'] ) || 'image-upload' === $meta_value['type'] && isset( $meta_value['value_raw'] ) ) ) { | |||
There was a problem hiding this comment.
Correctness/security: this new per-form confinement ($allowed_base + resolve_uploads_file_from_url) is only applied to the file-upload/image-upload branch below. The sibling branch just above it — if ( preg_match( '/signature_/', $meta_key ) && file_exists( $meta_value ) ) { unlink( $meta_value ); } (lines 1670-1672) — still calls unlink() on the raw $meta_value from $entry with none of this validation (no uploads-dir check, no form-folder confinement, no realpath canonicalization). $entry here is $fields, explicitly documented elsewhere in this codebase as "derived from validation routine, not the DB", i.e. attacker-influenced submission data, so if any signature-handling code path ever lets this value reach here without being regenerated server-side (e.g. an entry-update/REST flow), it is the exact same unauthenticated arbitrary-file-deletion class this PR is fixing, just unpatched. Worth confirming every path that sets a signature_* entry value always overwrites it server-side before this runs, or applying the same resolve_uploads_file_from_url/$allowed_base guard here too.
| } | ||
|
|
||
| $uploads = wp_get_upload_dir(); | ||
| $folder = absint( $form_data['id'] ) . '-' . wp_hash( $form_data['id'] . $form_data['created'] ); |
There was a problem hiding this comment.
Reuse: this hash-folder computation (absint( $form_data['id'] ) . '-' . wp_hash( $form_data['id'] . $form_data['created'] ), joined with /everest_forms_uploads/) duplicates get_form_files_dir() at line 1518-1524 of this same class, which computes the identical folder name from $this->form_data. Right now a change to the hashing/foldering scheme has to be made in two places and will silently drift if one is missed. Since get_form_files_dir() relies on $this->form_data (which is not guaranteed to be the form in scope when this hook fires with its own $form_data argument), consider changing get_form_files_dir() to accept an optional $form_data = null parameter (defaulting to $this->form_data) and have get_form_uploads_base() call that single helper instead of re-deriving the folder name.
| unlink( $uploaded_file ); | ||
| } | ||
| // Only delete files that live in this form's own hash-protected uploads folder, so a crafted old_files URL cannot remove another form's or user's file. | ||
| if ( false !== $resolved_path && '' !== $allowed_base && 0 === strpos( $resolved_path, $allowed_base ) ) { |
There was a problem hiding this comment.
Altitude: this fix layers a local allowlist check ($allowed_base + strpos) on top of resolve_uploads_file_from_url(), which already exists as the shared sanitizer but only confines results to the whole uploads basedir, not a specific form's folder. Every future caller that needs to delete a form-scoped file will have to remember to re-implement this same confinement, the way this one spot now does, instead of getting it "for free". Consider generalizing resolve_uploads_file_from_url() (or adding a sibling method) to accept an optional base-directory parameter it validates against internally, so the form-scoping guarantee lives in one place rather than being a special case bolted onto this call site.
| @@ -1672,13 +1674,12 @@ public function remove_csv_file_after_email_send( $attachment, $entry, $form_dat | |||
| if ( isset( $meta_value['type'] ) && ( 'file-upload' === $meta_value['type'] && ! empty( $meta_value['value_raw'] ) || 'image-upload' === $meta_value['type'] && isset( $meta_value['value_raw'] ) ) ) { | |||
There was a problem hiding this comment.
Pre-existing but in the function this PR touches: at line 1670, file_exists( $meta_value ) is called whenever $meta_key matches /signature_/ — but $meta_value is only guaranteed to be a string for genuine signature entries. If a file-upload/image-upload field's meta-key slug happens to contain the substring signature_ (a plugin/site author is free to name a field's meta-key anything), $meta_value here is an array (['type' => ..., 'value_raw' => [...]]), and on PHP 8.0+ file_exists() throws an uncaught TypeError for a non-string argument, crashing the post-send cleanup (and anything after it in the request) instead of silently skipping like it did under PHP 7.4. Not introduced by this diff, but worth a quick is_string( $meta_value ) guard while this function is already being hardened.
|
@deepench — did a deep pass on this fix for the unauthenticated arbitrary file deletion issue (CVSS 8.2, Wordfence). Posted 4 inline comments on the specific lines, summarizing here so they don't get lost:
Nothing here blocks the main fix for the reported CWE-73/CVE — just want eyes on #1 before merge since it's the same vulnerability class in the neighboring branch. Let me know if you want a hand patching any of these. |
The cleanup still trusted file URLs from the entry, which old_files fills from the request, so an unauthenticated submission could delete any other existing file in the same form's uploads folder. Track the files this request moves into place and delete only those. This also drops the signature branch, which never matched here and threw a TypeError when a field ID contained "signature_", and adds the same is_string() guard to the attachment builder. Verified live (logged out, Save and Continue active, entries disabled): crafted old_files URLs for files in the same form, another form, outside the form folder and the wp-content/.. escape all survive; real file and image uploads are still attached to both notifications and then removed.
|
@lihsaa591 thanks, checked all of these on a live site.
While testing I also found that a crafted |
|
@deepench Checked 04daf9f — good fix, deletion no longer depends on request data at all ( One non-blocking note: LGTM otherwise. |
|
@deepench Also please resolve the conflict and merge this |
All Submissions:
Changes proposed in this Pull Request:
Unauthenticated arbitrary file deletion (CWE-73) via the upload field.
Root cause
everest_forms_ajax_form_submissionis public (wp_ajax_nopriv_*). With the Save-and-Continue addon active,class-evf-form-task.phpre-reads$_POST['everest_forms_{form_id}_old_{field_id}']into the field'sold_filesafter the 3.6.1 hardeningunset(), so the file list is attacker-controlled again. On a form with "Disable storing entry information" plus a notification email, the cleanup hookremove_csv_file_after_email_send()inincludes/abstracts/class-evf-form-fields-upload.phpthen unlinked each file URL.That sink derived the path with a greedy
ABSPATH . preg_replace( '/.*wp-content/', 'wp-content', <url-path> )and norealpath/containment check. The accept path (resolve_uploads_file_from_url()) validates a realpath-collapsed path but stores the raw URL, so the two derivations disagree: a URL carrying a secondwp-contentsegment (e.g..../uploads/x/wp-content/../wp-config.php) passes acceptance (realpath keeps it inside uploads against a decoy) while the greedy regex cuts at the secondwp-content, escaping uploads — deleting a file anywhere under the webroot (e.g.wp-config.php). Even without the escape, any existing uploads file could be deleted unauthenticated.Fix
Route the delete sink through the existing
resolve_uploads_file_from_url()helper (same validation already used by the accept path and the other callers in this class) and delete viawp_delete_file(). The path that is removed is now the same validated, realpath-resolved, uploads-contained path — the greedy rebuild is gone, so the containment escape and out-of-uploads deletion are closed.Scope note: the companion read/accept hardening (
old_filesvalidation andresolve_uploads_file_from_url()) already shipped topre-develop; this PR closes the one remaining unfixed derivation — the deletion sink.Closes # .
How to test the changes in this Pull Request:
Setup: Everest Forms + Save-and-Continue active. A published form with a File Upload field, "Disable storing entry information" enabled, and a notification email. Note the form id and the file-upload field id from the page markup.
_wpnonce{form_id}from the form page, then POSTeverest_forms_ajax_form_submissionwitheverest_forms_{form_id}_old_{field_id}[0]={"value":"<URL of an existing file under wp-content/uploads>"}. Before the fix the file is deleted (HTTP 200 on the file becomes 404); after the fix the file remains.uploads/evf-escape/wp-content/../<name>) plus a real webroot file<name>, and submit{"value":".../uploads/evf-escape/wp-content/../<name>"}. Before the fix the webroot file is deleted; after the fix it remains.wp-content/uploadsare still removed after the email is sent.Types of changes:
Other information:
Note: PHPCS was not run — the free plugin's dev toolchain (
wpeverest/wpeverest-sniffs) could not be installed in this environment (locked PHPUnit 7.x vs PHP 8.5). The change mirrors theresolve_uploads_file_from_url()usage already present in this class.Changelog entry
Fix - Improved security of file deletion when processing uploaded file submissions.