Make WordPress Core

Opened 3 weeks ago

Last modified 3 hours ago

#40085 assigned defect (bug)

Audio/video uploads are broken in 4.2.13 and 4.3.9

Reported by: uzegonemad Owned by: joemcgill
Milestone: 4.7.4 Priority: normal
Severity: blocker Version: 3.7.19
Component: Media Keywords: has-patch
Focuses: Cc:

Description (last modified by SergeyBiryukov)

As the title states, audio/video uploads are broken in the 4.2.13 and 4.3.9 security releases.

These releases backport a function for validating metadata, wp_kses_post_deep, but this function wasn't introduced until 4.4.

Uploading an audio/video file will respond with an undefined function error.

Relevant 4.2 commit: [40148]

Relevant 4.3 commit: [40153]

Trac doesn't let me choose 4.2.13, so I've filed this against trunk.

Attachments (2)

40085.diff (1.0 KB) - added by joemcgill 2 weeks ago.
Media Library mp3 upload.png (100.1 KB) - added by lukecavanagh 4 days ago.
mp3 Media Library patch fix test

Download all attachments as: .zip

Change History (21)

#1 @SergeyBiryukov
3 weeks ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 4.7.4

Hi @uzegonemad, welcome to Trac! Thanks for the report.

I can confirm the issue. [40148] was backported to all branches from 3.7 to 4.3, so they all appear to be affected.

#2 @uzegonemad
3 weeks ago

I just noticed that as well. It looks like this affects the following releases:

  • 3.7.19
  • 3.8.19
  • 3.9.17
  • 4.0.16
  • 4.1.16
  • 4.2.13
  • 4.3.9

This ticket was mentioned in Slack in #forums by sergey. View the logs.

3 weeks ago

#5 @johnbillion
3 weeks ago

  • Keywords needs-patch added
  • Severity changed from normal to blocker
  • Version changed from trunk to 3.7.19

#6 @joemcgill
3 weeks ago

  • Owner set to joemcgill
  • Status changed from new to assigned

Initially thought we could simply backport both wp_kses_post_deep() and map_deep() to these branches, but am thinking we should revert and use a more targeted approach than wp_kses_post_deep() (see: @ocean90 's comment on 40075).

Also need to investigate why we aren't seeing failing tests on these branches and make sure we have coverage for these code paths.

2 weeks ago

#7 @joemcgill
2 weeks ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

40085.diff moves the call to wp_kses_post() inside wp_add_id3_tag_data() which I think sanitizes all the metadata that might be used in a display context. This should also avoid #40075. Testing and feedback welcome.

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

13 days ago

#9 @dlh
13 days ago

I tested 40085.diff with some audio files on the 4.3, 4.2, and 3.9 branches.

Although I can't speak intelligently about the use of wp_kses_post() itself in this context, I can at least report that the uploads succeeded, and the files and metadata seemed OK.

I would also note there was a PHP 7-specific fatal error in 3.9: Fatal error: 'break' not in the 'loop' or 'switch' context in /wp-includes/ID3/getid3.lib.php on line 285. It looks like that break was removed in [29734].

#10 @joemcgill
12 days ago

Thanks for testing, @dlh. I'm not sure we need to patch a PHP 7 specific bug as far back as 3.9, but it's good to have documented.

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

12 days ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.

12 days ago

#13 @mikeschroder
8 days ago

Took a look at the patch.

This makes sense, but it'd be good to have someone familiar with ID3 like @wonderboymusic to take a look and make sure there isn't anything that wp_kses_post() sanitizes that commonly occurs in ID3 comments and would need to be preserved.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.

5 days ago

#15 @lukecavanagh
4 days ago


I tested 40058.diff and it does fix the issues in #40075. But a JPEG thumbnail image is still generated besides having the mp3 display with the cover artwork correctly in the media library.

4 days ago

mp3 Media Library patch fix test

#16 @joemcgill
4 days ago

  • Keywords 2nd-opinion removed

Thanks @lukecavanagh. I think that may have been existing behavior so the image could be used as a post_thumbnail for the attachment page, but will have to investigate further.

#17 @lukecavanagh
4 days ago


Okay in that case, then it makes sense as to why the image would be generated.

This ticket was mentioned in Slack in #forums by sergey. View the logs.

4 hours ago

#19 @SergeyBiryukov
4 hours ago

Uploaded a workaround plugin for both this ticket and #40075, so that people on support forums could use it until the next minor release is available: https://wordpress.org/plugins/correct-audio-video-uploads/

The plugin is based on 40085.diff.

Last edited 3 hours ago by SergeyBiryukov (previous) (diff)
Note: See TracTickets for help on using tickets.