Skip to content

[Security]Add a file extension validation whitelist mapped to an allowed mimetype - #262

Closed
BboyKeen wants to merge 1 commit into
1up-lab:masterfrom
BboyKeen:master
Closed

BboyKeen wants to merge 1 commit into
1up-lab:masterfrom
BboyKeen:master

Conversation

@BboyKeen

@BboyKeen BboyKeen commented Jul 27, 2016 •

Copy link
Copy Markdown
Contributor

Hi,

There is no extension check during file upload.
This lack of validation can lead to security problems.

Let's say the configuration contains "image/png" as allowed mimetype
An evil-minded user can upload an HTML page by forging a file beginning with an image header followed by HTML code. This way the uploaded file will pass the mime-type check but will be finally uploaded as HTML file. Then we can use this file as XSS vector to execute javascript.

As recommended in this document https://www.owasp.org/index.php/Unrestricted_File_Upload, I've added an extension whitelist linked to each allowed mimetype.

@bytehead

bytehead commented May 3, 2017

Copy link
Copy Markdown
Member

Thank you! 👍
We should also update the documentation for this feature.

@Lctrs

Lctrs commented Sep 11, 2017 •

Copy link
Copy Markdown
Contributor

There is a bug in the current implementation.

With the current defined configuration, Symfony's DI component will normalize dashes to underscores in the keys of allowed_mimetypes.

Example :

oneup_uploader:
# ...
    allowed_mimetypes:
        video/x-msvideo: ['avi']

will be translated to :

// ...
'allowed_mimetypes' => array(
    'video/x_msvideo' => array(
        0 => 'avi',
    ),
),
// ...

You need to tell Symfony to not normalize keys by calling normalizeKeys(false) on the allowed_mimetypes node.

Like this :

//...
->arrayNode('allowed_mimetypes')
    ->normalizeKeys(false)
//...

@bytehead

Copy link
Copy Markdown
Member

Closed in favor of #289.

@bytehead bytehead closed this Nov 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants