Skip to content

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

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

[Security]Add a file extension validation whitelist mapped to an allowed mimetype#262
BboyKeen wants to merge 1 commit into1up-lab:masterfrom
BboyKeen:master

Conversation

@BboyKeen
Copy link
Copy Markdown
Contributor

@BboyKeen BboyKeen commented Jul 27, 2016

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
Copy link
Copy Markdown
Member

bytehead commented May 3, 2017

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

@Lctrs
Copy link
Copy Markdown
Contributor

Lctrs commented Sep 11, 2017

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