chillu commented 5 years ago

Overview

Our Upload_Validator limits the file extensions you're allowed to upload via File.allowed_extensions. It does not perform any mime type sniffing to verify the contents of that file actually match its declared extension. This isn't a security issue as such (it was previously discussed in https://github.com/silverstripe-security/security-issues/issues/41), but can become dangerous if it is combined with a mismatch between the file extensions which are allowed to upload, the ability of a web server environment to detect the mime type on those file extensions (through direct mapping, or "magic" detection), and a lack of nosniff headers in your response.

The underlying issue here is that we've documented file uploads in the context of "allowed extensions", while we should really ask devs to set "allowed mime types".

Status Quo

We do optionally support mime type upload validation via https://github.com/silverstripe/silverstripe-mimevalidator, which is implemented in CWP but can be loaded into any SilverStripe project. It uses PHP's fileinfo module, which could lead to denied uploads if the sniffing fails. It works in addition to the Upload_Validator in core, so validates both extensions and mimetypes. Note that fileinfo is a required module by core according to our server requirements (as well as the flysystem and intervention third party packages).

Mime Type Sources

HTTP::get_mime_type() has a fallback for a manual list of HTTP.MimeTypes if fileinfo isn't availble. That's unnecessary given we require it in core, but part of our documented API surface. This has the structural flaw of not allowing multiple mime types per extension, so is basically useless - it's a "many many" relationship.
Mimevalidator has an additional MimeUploadValidator.MimeTypes to work around that issue - see getExpectedMimes()

Options

Option 1: Point devs to optional module in our secure coding practices, and rely on mime detection during download rather than upload Option 2a: Add module to 4.x framework as-is (only adjust namespace?) Option 2b: Fold module code into 4.x frameworkd via Upload_Validator in addition to extension-based checks Option 2c: Fold module into 4.x framework via Upload_Validator, only perform mime-based checks, but continue allowing extension-based checks (via the existing getExpectedMimes() workaround). In case somebody has allowed an extension that isn't in those 900+ mime types, it'd be a breaking change. Option 2d: Fold module code into 5.x framework via Upload_Validator, only perform mime-based checks, and remove support for extension based checks (throw exception) Option 3: Add module to a "better recipe", going beyond the basics.

So the question is if mime validation on upload is the baseline, or an optional add-on. We already maintain mimevalidator as a supported module. I think we need to foster the behaviour shift in devs from ambiguous file extensions to safer mime types, it's just a matter in what timeframe that happens.

Notes

Symfony recommends guessing the extension and mime type validation constraints by default. They notably don't provide a "file extension" constraint, presumably because that's considered inferior to mime types.
There's actually a (IANA maintained) official list of mime-types, and a map to file extensions: https://en.wikipedia.org/wiki/Media_type#mime.types.

/cc @silverstripe/core-team

Pull Requests

ScopeyNZ commented 5 years ago

Keen on some derivative of option 2 that targets 4.x. I think 2c sounds best. The potential breakage is low enough.

sminnee commented 5 years ago

I'm supportive of this being added to the core recipe; I also think elemental and userforms should be part of the core recipe. 2c seems fine although I think in my head I'm thinking more of "3" given my previous comment — I would just see this "better recipe" render the current "arbitrarily restricted cores that is only useful to experienced SS developers who probably have their own skeletons anyway" as deprecated.

wernerkrauss commented 5 years ago

@sminnee I strongly disagree with userforms and elemental being part of the cms core recipe. Both are some extra requirements that are not used in all cms projects.

sminnee commented 5 years ago

I’m not suggesting that they’re silverstripe/cms dependencies, but that they’re part of the default download that new SilverStripe users are most likely to first use.

For SilverStripe experts, making use of the packages most relevant to their project would totally be an option.

chillu commented 4 years ago

Marking as impact/high since this comes up in audits every now and then (where the audited codebase isn't CWP, and doesn't have this included through the recipe)

robbieaverill commented 4 years ago

So far 5 core committers (@robbieaverill @chillu @sminnee @ScopeyNZ @maxime-rainville) in favour and none against

maxime-rainville commented 4 years ago

Tried upgrading a CWP-installer project and realised that the mimevalidator.yml file doesn't get copied to the project because cwp/cwp is not a recipe.

I've got a PR to revert the change to CWP/CWP. Might need to think what I do with the recipe-core file that gets copied. It might need an after statement.