WordPress.org

Make WordPress Core

Opened 9 years ago

Last modified 7 months ago

#14148 new defect (bug)

wp_get_attachment_url() is not url encoding

Reported by: danorton Owned by:
Milestone: Future Release Priority: normal
Severity: major Version: 3.0
Component: Security Keywords: has-patch needs-testing dev-feedback needs-unit-tests needs-refresh
Focuses: Cc:

Description

A fairly fundamental flaw, the function wp_get_attachment_url() doesn't return a valid URL if the filename contains unescaped URL characters.

I'm not sure, but this might be a security issue, as the current version can generate URLs that don't match the filename, but instead passes query parameters back to the server.

The attached patch for Version 3.0 file fixes this in wp-includes/post.php

Attachments (4)

post.php.patch (1.6 KB) - added by danorton 9 years ago.
14148.diff (2.8 KB) - added by kawauso 8 years ago.
Compact patch based on danorton's but adding corrections for thumbnails and intermediate sizes. Tidy up comments a bit too.
14148.2.diff (1.4 KB) - added by Mte90 2 years ago.
refresh patch
14148.3.diff (1.1 KB) - added by andizer 10 months ago.
Fix codestyle issue

Download all attachments as: .zip

Change History (26)

@danorton
9 years ago

#1 follow-up: @nacin
9 years ago

Can you post an example URL and what would get returned with and without the patch?

#2 in reply to: ↑ 1 @danorton
9 years ago

Replying to nacin:

Can you post an example URL and what would get returned with and without the patch?

For a file named "X%X.txt"

Currently the URL returned is:
.../wp-content/uploads/2010/06/X%X.txt

This patch corrects it to return:
.../wp-content/uploads/2010/06/X%25X.txt

#3 @nacin
9 years ago

  • Component changed from General to Security
  • Milestone changed from Awaiting Review to 3.1

@kawauso
8 years ago

Compact patch based on danorton's but adding corrections for thumbnails and intermediate sizes. Tidy up comments a bit too.

#4 @kawauso
8 years ago

  • Cc otterish@… added
  • Keywords has-patch needs-testing added; url query removed

Had to force image_downsize() to use encoded filenames when replacing which may break things. Any other ideas?

#5 @ryan
8 years ago

  • Milestone changed from 3.1 to Future Release

This is too risky for 3.1 and will require thorough testing.

#6 @chriscct7
5 years ago

  • Keywords dev-feedback added

Tagging for re-review

#7 @chriscct7
3 years ago

  • Keywords needs-refresh added

@Mte90
2 years ago

refresh patch

#8 follow-up: @Mte90
2 years ago

  • Keywords needs-refresh removed

The code in this years it's changed so the patch for post.php I changed approach with an encoding after the generation of the url itself.

#9 in reply to: ↑ 8 @nevis2us
2 years ago

Replying to Mte90:

The code in this years it's changed so the patch for post.php I changed approach with an encoding after the generation of the url itself.

IMHO this is the right approach but php urlencode can't be used to encode the whole url.
An equivalent of javascript encodeURI is needed here.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURI.

eg

function encodeURI ($uri)	{

	$revert = array (

		// reserved characters
		"%3B" => ";", "%2C" => ",", "%2F" => "/", "%3F" => "?", "%3A" => ":",
		"%40" => "@", "%26" => "&", "%3D" => "=", "%2B" => "+", "%24" => "$",

		// unescaped characters
		"%2D" => "-", "%5F" => "_", "%2E" => ".", "%21" => "!", "%7E" => "~",
		"%2A" => "*", "%27" => "'", "%28" => "(", "%29" => ")",

		// number sign
		"%23" => "#"
	);

	return strtr (rawurlencode ($uri), $revert);
}

This ticket was mentioned in Slack in #core by mte90. View the logs.


21 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


15 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


15 months ago

#13 @SergeyBiryukov
15 months ago

  • Keywords needs-unit-tests added

#14 @afercia
15 months ago

  • Keywords needs-unit-tests removed

14148.2.diff needs some coding standards.

#15 @afercia
15 months ago

  • Keywords needs-unit-tests needs-refresh added

@andizer
10 months ago

Fix codestyle issue

#16 follow-up: @mukkoo
8 months ago

I've checked this issue and related patches but I can't reproduce a bugged context.
Uploading a media that needs encoding WordPress remove all special chars making this scenario not reproducible.
I've used tests/phpunit/tests/media.php as test base using

     $filename = DIR_TESTDATA . '/images/test%%%%image-large.png';

uploading this media WordPress remove all special chars leaving testimage-large.png

Can someone suggest any possible way to replicate bug?

#17 in reply to: ↑ 16 @nevis2us
8 months ago

Replying to mukkoo:

Can someone suggest any possible way to replicate bug?

Upload a file with a valid non-ASCII name on a system which supports UTF-8.

#18 @nevis2us
8 months ago

This patch assumes only base names may contain characters which need to be encoded. What about directory names? This may not be the case with wordpress attachments but why reinvent the wheel? There's a (client-side) standard function which can be safely applied to the whole URI and spare the trouble of handling URI components separately.

BTW there's a number of similar issues in functions like image_get_intermediate_size and image_downsize.

#19 @nevis2us
8 months ago

One more note. Patching the core on this level may break existing code. I'm currently handling this problem in a custom plugin which applies the above encodeURI function to the whole URL in wp_get_attachment_url filter. The filter can be easily turned off in the UI in case the patches make it to the core.

#20 follow-ups: @Mte90
8 months ago

Cna you share the code of your custom plugin?
So we can understand better how to handle it.
Cna you share also an image already existing with the issues on the name?

#21 in reply to: ↑ 20 @nevis2us
8 months ago

Replying to Mte90:

Cna you share the code of your custom plugin?
So we can understand better how to handle it.

Sure, I can share the relevant parts of the code:

add_filter("wp_get_attachment_url", "<namespace>_wp_get_attachment_url", 90, 2);

function <namespace>_wp_get_attachment_url($url, $post_id)	{
	return encodeURI($url);
}

The code for encodeURI() can be found in comment:9 above.

This is what a plugin dev can use if he needs this bug fixed ASAP before the relevant patches make it to the core. But fixing this and similar issues in the core or in a filter may break existing 3rd party code. I don't use 3rd party plugins on my sites so it works for me.

Cna you share also an image already existing with the issues on the name?

If I got you right:

The function wp_get_attachment_url() doesn't return a valid URL if the filename contains UTF-8 characters.

Example:

attachment filename.../wp-content/uploads/2015/10/Оля-в-Инстаграме.BW_.jpg
wp_get_attachment_url() returns.../wp-content/uploads/2015/10/Оля-в-Инстаграме.BW_.jpg
valid URL.../wp-content/uploads/2015/10/%D0%9E%D0%BB%D1%8F-%D0%B2-%D0%98%D0%BD%D1%81%D1%82%D0%B0%D0%B3%D1%80%D0%B0%D0%BC%D0%B5.BW_.jpg

#22 in reply to: ↑ 20 @nevis2us
7 months ago

Replying to Mte90:

Cna you share the code of your custom plugin?
So we can understand better how to handle it.

Complete plugin with the fix mentioned above can be downloaded from http://ipros24.ru/wordpress/#ipros24-fixpack. It also includes fixes for similar encoding problems in image_get_intermediate_size() and image_downsize() core functions. After install individual fixes must be enabled in Settings -> Fixpack.

This plugin may break 3rd party code e.g. Jetpack, which also uses image_downsize filter to override the core function.

Note: See TracTickets for help on using tickets.