WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 8 months ago

#12756 reopened defect (bug)

WPMU does not handle files with two or more dots in the filename

Reported by: Namely Owned by: wpmuguru
Milestone: Future Release Priority: normal
Severity: minor Version: 2.9.2
Component: Upload Keywords: has-patch
Focuses: multisite Cc:

Description

  • WPMU does download images that have two or more dots in the file name

    E.g., One..jpg One...jpg One....jpg

rewrites do work (checked)

  • this is clearly a WP issue:

    /wp-content/blogs.php

...
$file = BLOGUPLOADDIR . str_replace( '..', , $_GET[ 'file' ] );
if ( !is_file( $file ) ) {

status_header( 404 );
die('404 — File not found.');

}
...

WPMU removes two dots!!!

workaround:

$file = BLOGUPLOADDIR . $_GET[ 'file' ]; name.ly: workaround for files with two or more dots

tested and works fine

Attachments (1)

12756.diff (531 bytes) - added by wpmuguru 5 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 @Namely5 years ago

Errata, yes WPMU does download the files into the blogs.dir... but /wp-content/blogs.php does not handle them properly, hence, the files are not seen by the user.

Needs to be corrected.

comment:2 @nacin5 years ago

  • Keywords multisite added; WPMU uploads files removed

comment:3 @nacin5 years ago

  • Milestone changed from MU 2.9.x to Future Release

comment:5 @wpmuguru5 years ago

The purpose of the str_replace was to prevent a request like /files/../../../../wp-config.php from being processed.

Probably $file = BLOGUPLOADDIR . str_replace( '../', , $_GET[ 'file' ] ); would work, but I haven't tested it.

comment:6 @wpmuguru5 years ago

I seem to be missing two quotes there

$file = BLOGUPLOADDIR . str_replace( '../', '', $_GET[ 'file' ] );

comment:7 @larysa5 years ago

  • Cc larysa added
  • Owner set to wpmuguru
  • Status changed from new to assigned

Anything
$file = BLOGUPLOADDIR . str_replace( '../', , $_GET[ 'file' ] );
or
$file = BLOGUPLOADDIR . str_replace( '/..',
, $_GET[ 'file' ] );
or
$file = BLOGUPLOADDIR . str_replace( '/../', , $_GET[ 'file' ] );
would solve the issue.

The last one is enough. Any chance to see this change in the official release soon? Shame to miss it in 3.0.2.

@wpmuguru5 years ago

comment:8 @wpmuguru5 years ago

  • Keywords has-patch added

I think we should 404 the request if it has a '../' in the request uri.

comment:9 @Namely4 years ago

  • Keywords needs-patch added
  • Version changed from 2.9.2 to 3.2.1

We are using:

$file = BLOGUPLOADDIR . str_replace( '../', '', $_GET[ 'file' ] );

If would be nice to have it in the next release, not to hack the code each time we run an upgrade.

Thank you.

comment:10 @SergeyBiryukov4 years ago

  • Keywords needs-patch removed
  • Version changed from 3.2.1 to 2.9.2

Please do not change the version number. It's there to track when the bug was introduced/reported.

Also, the patch still looks valid and applies correctly.

comment:11 @SergeyBiryukov4 years ago

  • Milestone changed from Future Release to 3.3

comment:12 follow-up: @nacin4 years ago

  • Milestone changed from 3.3 to Future Release

Using validate_file() here would yield the same bug anyway. Curious if ".." rather than "../" could therefore prevent path traversal with "..\". Since this isn't a regression, I'm inclined to continue to punt it.

In IRC, we're wondering whether a backslash could cause some issues, either via traversal or by escaping characters.

The simple fix is to just not use .. in a URL. Punt.

comment:13 in reply to: ↑ 12 @wpmuguru4 years ago

Replying to nacin:

In IRC, we're wondering whether a backslash could cause some issues, either via traversal or by escaping characters.

The simple fix is to just not use .. in a URL. Punt.

For this one, I think the better solution is to sanitize the filename on upload and replace the .. with --.

comment:14 @wpmuguru3 years ago

  • Keywords close added

This was more or less addressed by #19235. Recommending close.

comment:15 @SergeyBiryukov3 years ago

  • Keywords close removed
  • Milestone Future Release deleted
  • Resolution set to invalid
  • Status changed from assigned to closed

comment:16 @nacin3 years ago

  • Milestone set to Future Release
  • Resolution invalid deleted
  • Status changed from closed to reopened

Unfortunately, people will be running ms-files.php for the foreseeable future. Unlike global terms, moving away from ms-files.php is not easy. Some fixes to ms-files like this can be easy wins.

comment:17 @eligijus8 months ago

WordPress 4.1 multisite still allow to upload files with two dots but don't allow to download these files:

http://example.lt/multisite/userpage/files/2015/01/test..pdf
404 — File not found.
Note: See TracTickets for help on using tickets.