Add some casts in Custom_Image_Header

[xknown]

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.

[xknown]

Cast to the appropiate types

[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 don't see a memory consumption issue, unless an image is being referenced, and thus GD goes through a resizing.

[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). 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.

[nacin]

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.

[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?

Rest of the patch looks good.

[xknown]

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.

[nacin]

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

[nacin]

In [18298]:

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