WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#34562 closed defect (bug) (fixed)

`wp_tempnam()` can return a path that already exists

Reported by: dd32 Owned by: dd32
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Filesystem API Keywords: has-patch
Focuses: Cc:

Description

At present it's possible for the wp_tempnam() to generate a filename which already exists on the server, it requires two processes to attempt to create the same filename within the same short period of time. The period of time varies on the IO operation time, so is more of an issue where the filesystem is remote (say a NFS share).

There's two things we can do to improve this
a) Suffix a random ID onto the filename to make it far more unique, and unlikely to conflict with an existing file
b) verify the result from touch() to see if the file exists

a uses wp_generate_password( 6, $special_char=false ) which is the only real random string function we have - uniq() is possible but is mearly the same as microtime() converted to a-Z.

b is actually a little complex, touch() isn't what we want here, what we want is more akin to fopen( $file, 'x' ) - create the file if it doesn't exist, else bail.

Attached is a patch that implements this with a few extra checks, it currently allows for an unwritable directory/filename to be returned (I mean, it's best-effort, the failure isn't likely to occur) to prevent infinite looping.

Attachments (1)

34562.diff (1.5 KB) - added by dd32 3 years ago.

Download all attachments as: .zip

Change History (5)

@dd32
3 years ago

#1 @wonderboymusic
3 years ago

  • Owner set to dd32
  • Status changed from new to assigned

#2 @dd32
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 35644:

Decrease the chances that wp_tempnam() will conflict with an existing file by suffixing a random ID to the generated filename.
This also switches from using touch() to using fopen( $file, 'x') to ensure that we're the process creating the file.

Fixes #34562

#3 @dd32
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This unfortunately didn't get applied to the WP_Upgrader class too, which doesn't rely upon wp_tempnam() as it's after a directory name, not a filename.

We should apply something similar to Upgrades to reduce the chances of issues caused by any upgrade-related race conditions. 34562.2.diff should do this.

#4 @dd32
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Scratch that. Turns out wp_tempnam() IS used during the zip download, so $package at this point is something like wordpress-latest-6CHK9M already.

Note: See TracTickets for help on using tickets.