WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#17779 closed defect (bug) (fixed)

Add some casts in Custom_Image_Header

Reported by: xknown Owned by: nacin
Milestone: 3.2 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch
Focuses: Cc:

Description

The step_3 method of the Custom_Image_Header class does not sanitize the input data. One can pass for example any value in $_POST['attachment_id'] (even an URL), which can cause memory consumption problems in multisite environments.

Attachments (1)

custom-header.17779.diff (2.1 KB) - added by xknown 3 years ago.
Cast to the appropiate types

Download all attachments as: .zip

Change History (8)

xknown3 years ago

Cast to the appropiate types

comment:1 follow-up: nacin3 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.2

In the future, please report things that sound like vulnerabilities to security@…. After walking through the code, this comes pretty close to being one.

I don't see a memory consumption issue, unless an image is being referenced, and thus GD goes through a resizing.

comment:2 in reply to: ↑ 1 ; follow-up: xknown3 years ago

Replying to nacin:

In the future, please report things that sound like vulnerabilities to security@…. After walking through the code, this comes pretty close to being one.

I reported it before the latest minor release (on May 5). I quote myself

It may be also good if you add some casts to the user specified values in the step_3 method.


I don't see a memory consumption issue, unless an image is being referenced, and thus GD goes through a resizing.

Well, I'm not sure about that. The imagecreatefromstring function still needs some amount of memory to load the image. Thus, if someone forces to load multiple large images you may have a problem there.

Last edited 3 years ago by xknown (previous) (diff)

comment:3 in reply to: ↑ 2 nacin3 years ago

Replying to xknown:

Replying to nacin:

In the future, please report things that sound like vulnerabilities to security@…. After walking through the code, this comes pretty close to being one.

I reported it before the latest minor release (on May 5) :)

Ah, sorry Alex. You're right. I didn't recognize the username. :-) Thanks for going through the right channels. We should have included it in the omnibus, but at least there's no vuln here.

I don't see a memory consumption issue, unless an image is being referenced, and thus GD goes through a resizing.

Well, I'm not sure about that. The imagecreatefromstring function still needs some amount of memory to load the image. Thus, if someone forces to load multiple large images you may have a problem there.

Indeed. Still something we should fix.

comment:4 follow-up: nacin3 years ago

Looking at the patch:

wp_crop_image() can take a ID, so I think we should pass an int and let that function call get_attached_file(). Thoughts?

Rest of the patch looks good.

comment:5 in reply to: ↑ 4 xknown3 years ago

Replying to nacin:

Looking at the patch:

wp_crop_image() can take a ID, so I think we should pass an int and let that function call get_attached_file(). Thoughts?

It seems okay to me, the proposed patch passes indeed an int to wp_crop_image.

comment:6 nacin3 years ago

Sorry, I didn't see that it was $original = get_attached_file....

comment:7 nacin3 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [18298]:

Sanity int casts in custom header step_3. props xknown, fixes #17779.

Note: See TracTickets for help on using tickets.