Cover Image & More Cannot be Uploaded on Windows Servers with 2.9.3 Update

[andrewteg]

In the 2.9.3 update bp-core/bp-core-attachments.php line 1317 changes from

if ( ! is_dir( $cover_dir ) ) {
to
if ( 0 !== validate_file( $cover_dir ) || ! is_dir( $cover_dir ) ) {

Using validate_file on a directory will always return 2 in a Windows/WAMP environment because a directory is always something like

C:\Apache24\htdocs\buddypress/wp-content/uploads/buddypress/groups/4/cover-image

This produces a general error with the default message on a WAMP install that upgrades to 2.9.3.

I believe it also affects line 450 as that uses validate_file on $type_dir, but I have only confirmed it on line 1317 with $cover_dir.

If I roll just that line back to 2.9.2 version cover update works great.

Thanks,
Andrew

[andrewteg]

Confirmed that line 450 does indeed to be updated to get the cover image to save. It will update the display but not save properly without removing validate_file from the check on line 450.

[DJPaul]

Thanks for the report, and I'm sorry we broke this. None of the people involved in the 2.9.3 release had Windows environments to test on.

[andrewteg]

No worries here. It's a great product and we appreciate all your hard work on every release. We just found it on a test setup and wanted to let everyone know to help out in whatever little way we can!

[DJPaul]

Discussion on Slack ​https://wordpress.slack.com/archives/C02RQBYUG/p1517258207000486 has suggested that we could check for a return value of 1 here instead.

We've found some team members with access to Windows servers, so next step is to test validate_file() on those machines (maybe via WP-CLI) and verify the behaviour.

[wordplus]

Replying to DJPaul:

Discussion on Slack ​https://wordpress.slack.com/archives/C02RQBYUG/p1517258207000486 has suggested that we could check for a return value of 1 here instead.

We've found some team members with access to Windows servers, so next step is to test validate_file() on those machines (maybe via WP-CLI) and verify the behaviour.

I think the problem is that, validate_file performs abit another function.
This function should check only relative paths, ​here its trying to check absolute path, thats why its not working properly.

We should remove it from that conditional or change what we checking.

​https://developer.wordpress.org/reference/functions/validate_file/#comment-1494

[DJPaul]

No, we can't remove the check from the attachments class. The point of adding those was to fix and harden security issues. Taking that out will not help.

[wordplus]

Replying to DJPaul:

No, we can't remove the check from the attachments class. The point of adding those was to fix and harden security issues. Taking that out will not help.

hi there, check patch file, possible move validate function to preupload filter and this will work well.

[DJPaul]

@espellcaste and @r-a-y, you said on Slack you've got Windows development machines. Is it possible one of you could pick this up?

First I think we need some testing of validate_file() on Windows with absolute paths to confirm the behaviour. Then, think about how to fix this (checking for a return type of 1 or 2 is probably the way forward, not sure which is best) and create a patch. Once we know it works on Windows, you/we/I can then test that patch on a Linux server and check it still works there.

Let me know if either of you are able to take this on in the next two weeks (I can volunteer to do a release on 21st February if this is fixed by then).

[r-a-y]

01.patch fixes this issue by seeing if validate_file() returns 1 meaning a directory traversal attempt, which is really all we are concerned about.

Tested on a WAMP install and it works.

[mercime]

Installed WAMP, saw the issue reported using Chrome, FF, and Edge.

Tested @r-a-y's patch and confirmed patch fixed the issue using 3 browsers above.

Edit - P.S. Patch tested and works on IE 9-11 as well.

[DJPaul]

If this has been tested on non-windows and it’s fine, let’s get committed — you don’t need me to approve a fix!

[r-a-y]

Just tested on a nginx install and uploading still works.

Going to commit.

[r-a-y]

In 11873:

Attachments: Fix uploading for those on Windows/WAMP installs.

Props andrewteg, r-a-y.

Fixes #7674 (2.9-branch).

[r-a-y]

In 11874:

Attachments: Fix uploading for those on Windows/WAMP installs.

Props andrewteg, r-a-y.

Fixes #7674 (trunk).

[DJPaul]

Milestone renamed