Needs work
Project:
Drupal core
Version:
11.4.x-dev
Component:
media system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
9 Jul 2021 at 14:51 UTC
Updated:
17 Oct 2026 at 17:23 UTC
Jump to comment: Most recent
Problem/Motivation
In #3080666-74: oEmbed system doesn't work if thumbnail url does not have a file extension, @b_sharpe raised the point that the oEmbed system, when retrieving the thumbnail for a resource, does not do any kind of control of what the extension is. A provider could theoretically return a file with a .php extension, containing actual PHP code, and it would be saved into the filesystem.
Now, as far as I know, it never actually be executed by the PHP interpreter; instead, it would be fed into the image toolkit, which would probably choke on it. So I don't think this is an actual exploitable security issue, but it's certainly a thing we could harden.
Steps to reproduce
N/A; this is a theoretical case.
Proposed resolution
The oEmbed source plugin should try to validate that the downloaded thumbnail is, in fact, an image, by both limiting the thumbnail filename to valid image extension and also using the image toolkit to confirm that the downloaded image data is truly a valid image.
Remaining tasks
None
User interface changes
None.
API changes
TBD, but likely none.
Data model changes
None.
Release notes snippet
TBD
Issue fork drupal-3223012
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
phenaproximaComment #3
cilefen commentedWould the MimeTypeGuesser be more lightweight and simpler?
Comment #4
phenaproximaWell, we're already leveraging the MimeTypeGuesser in #3080666: oEmbed system doesn't work if thumbnail url does not have a file extension. But I'm not sure if that does the kind of deeper validation (i.e., does this really work with the image toolkit, or does it merely say it does?) that I'm talking about in this issue.
Comment #5
cilefen commented^ that issue seems to use the extension list, which is different than the guesser but I understand your point.
Comment #6
b_sharpe commentedI'd be curious to know if even if the image toolkit can't read the file if it is indeed saved to the likely public file system, IF (and I'm assuming with oEmbed this is a pretty big IF) said provider returned a malicious file that it might somehow be able to execute the file, I'm guessing not given the file is hashed as well, but this was where the original concern came from for context.
Comment #14
longwaveComment #15
smustgrave commentedLeft comments and a question on the MR.
Comment #16
smustgrave commentedSorry this fell off my radar. Thanks for answering the question.
Comment #17
longwaveUpdated the IS with the current solution.
Comment #22
catchCommitted/pushed main, 12.0.x, 11.x and 11.5.x, thanks!
I made a suggestion in the MR to bump the deprecation to 11.5, but then forgot to apply it before committing.
But then I wondered whether we should backport this to 11.4.x after all since it's security hardening, so moving to 'to be ported' to discuss. We've mostly stopped 11.4 backports at this point but this is in the category of things we still might.
Comment #23
longwaveAlso the deprecation notice links to a non-existent nid, so we should write the change record and update that!
Comment #24
catchIt's only a constructor deprecation, so I wouldn't write a change record, but yes we should link back to this issue instead of 9999999, was too busy looking at the version.