Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

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

bytehead commented May 3, 2017

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

@Lctrs
Copy link
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
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