WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 2 weeks 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
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 (3)

post.php.patch (1.6 KB) - added by danorton 7 years ago.
14148.diff (2.8 KB) - added by kawauso 7 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 8 months ago.
refresh patch

Download all attachments as: .zip

Change History (13)

@danorton
7 years ago

#1 follow-up: @nacin
7 years ago

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

#2 in reply to: ↑ 1 @danorton
7 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
7 years ago

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

@kawauso
7 years ago

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

#4 @kawauso
7 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
7 years ago

  • Milestone changed from 3.1 to Future Release

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

#6 @chriscct7
3 years ago

  • Keywords dev-feedback added

Tagging for re-review

#7 @chriscct7
22 months ago

  • Keywords needs-refresh added

@Mte90
8 months ago

refresh patch

#8 follow-up: @Mte90
8 months 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
6 months 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.


2 weeks ago

Note: See TracTickets for help on using tickets.