Only allow PNGs and JPGs in image scaling requests
What does this MR do?
We recently merged an experimental feature: rescaling avatars on-the-fly when served through Workhorse. The feature is behind a feature toggle.
This feature comes with a number of security concerns that we are currently considering. One was that some file types like SVGs represent attack vectors that can hijack the scaler proc (graphicsmagick
), an incident that was publicly documented as the ImageTragick attack.
We already reject SVGs by default when uploading avatars, so that part is covered, but we haven't done enough testing with some other less popular file formats such as TIFF, ICO etc. They are also wildly unpopular on GitLab compared to PNGs and JPGs, which make up 99% of the avatar data set. Hence we would like to, for now, only allow PNGs and JPGs to make it through to workhorse (where the scaler will run.)
As with similar restrictions we put in place before (allowed sizes), the app will silently ignore the scaling request in this case and simply serve the original file. This makes for a smooth migration path with no disruption to the user.
Issue: #237848 (closed)
NOTE: For this to fully work, a change in Workhorse is required to actually interpret the format
string. That MR is here: gitlab-workhorse!564 (merged); but we don't have to wait for it, it's safe to send it now, WH/master will simply ignore it.
NOTE: This does not affect the functionality, availability, or other constraints around avatars in general. It merely further constrains which of the existing avatar files are eligible to be processed by the experimental scaler. Any avatars that do not qualify will simply continue to be served as-is.
Does this MR meet the acceptance criteria?
Conformity
- [-] Changelog entry This is an experimental feature and should not be announced yet.
- [-] Documentation (if required)
-
Code review guidelines -
Merge request performance guidelines -
Style guides - [-] Database guides
- [-] Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. - [-] Tested in all supported browsers
- [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
The security team is actively involved in this epic and is copied on the underlying issue: #237848 (closed)