Skip to content

Add additional path validations to UploadsPipeline

Nick Malcolm requested to merge nmalcolm-import-upload-checks into master

What does this MR do and why?

This refactor adds some defense-in-depth. There should be no difference to end users.

I was looking at this code earlier in the week, and noticed that it called save_avatar before performing a check to validate the file wasn't a symlink. I couldn't exploit it though - BulkImports::FileDownloadService makes sure whatever we download is safe before UploadsPipeline gets around to using it. The AVATAR_PATTERN regex was pretty loose, and the checks for not-avatars also didn't seem to validate that file_path was in a tmpdir (just that the path matches DYNAMIC_PATH_PATTERN regex).

It seemed like an area where things might potentially go wrong in the future, so I reshuffled the existing checks and added one more (ensure the path is in a tmpdir and not attempting to traverse out). Now even a sneaky avatar or other file shouldn't be able to get past the checks.

FWIW we addressed a severity1 in this area of code at the start of 2022: Arbitrary file read via the bulk imports Uploa... (#349524 - closed)

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Nick Malcolm

Merge request reports