Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better checking of the file type when determining which files are SVGs #28

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Feb 10, 2022

Description of the Change

We currently have a function that is hooked into the wp_handle_upload_prefilter filter. In there we check if the file type is image/svg+xml and if so, we sanitize the file. If that sanitization process fails, we don't allow the file to be uploaded to WordPress.

There's a potential issue here though because the file type we rely on comes from the Content-Type of the original form-data request. Typically this will be automatically set to whatever the real file type is but in theory, someone could spoof this request and set Content-Type to be anything, like image/png. In this case, they could send an actual SVG in the form-data but set the Content-Type to be image/png, which would then bypass our sanitization process (but would still be uploaded to WordPress).

This PR fixes this by utilizing the core WordPress function wp_check_filetype_and_ext to get the file type, instead of relying on the value that comes from the request. This should still work in all legitimate cases and if someone were to try sending a proper SVG but they change the Content-Type in the request, it will also catch that.

I tested the following scenarios:

  • Valid SVG uploaded without a custom Content-Type set. Our sanitization kicks in
  • Valid SVG uploaded with a custom Content-Type set (set to image/png). Our sanitization kicks in
  • Valid PNG uploaded without a custom Content-Type set. No sanitization needed from us
  • SVG file with a .png extension uploaded without a custom Content-Type set. File rejected
  • SVG file with a .png extension uploaded with a custom Content-Type set. File rejected
  • SVG file with no extension uploaded without a custom Content-Type set. File rejected
  • SVG file with no extension uploaded with a custom Content-Type set. File rejected

Alternate Designs

None

Possible Drawbacks

Slight impact on performance but should be minimal

Verification Process

Ensure that you can still properly upload SVGs in WordPress.

If desired, can also manually make a curl request to send an SVG with a manipulated Content-Type field, to verify that captures things correctly. I can provide an example curl command if needed.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog Entry

Fixed - add better file type checking when looking for SVG files

…se a core WordPress function to determine this, to avoid any issues around spoofing the content-type
@dkotter dkotter self-assigned this Feb 10, 2022
@dkotter dkotter marked this pull request as draft February 10, 2022 21:51
@dkotter dkotter marked this pull request as ready for review February 10, 2022 21:52
Copy link
Collaborator

@darylldoyle darylldoyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I tested it this morning and can confirm it's returning the correct mime type using this method!

@jeffpaul jeffpaul added this to the 1.10.0 milestone Feb 14, 2022
@jeffpaul jeffpaul merged commit 00cb9a8 into develop Feb 14, 2022
@jeffpaul jeffpaul deleted the fix/proper-type-checking branch February 14, 2022 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants