Hi, I've an issue with mapping to file fields.

When importing emails through mailhandler, including attachments, the allowed file extensions of my file field are not respected.
This is actually a security issue for me, as all file types are now saved from email attachments, including php files, or whatever (malicious code) people want to send me.

My setup is mailhandler, with the Mailhandler IMAP stream parser, that maps the email 'attachments' to a field of type 'file'. It works great, I just need to filter out unwanted file types.

Can anyone help me with this? Is it actually Feeds, or perhaps an issue that should be solved through Mailhandler? Any help is really appreciated!

Thanks,
Roeneman

CommentFileSizeAuthor
#20 core-adapted-security.patch11.36 KBBevan
#20 SDO120198-standalone-feeds.patch11.36 KBBevan
#17 feeds-1848498-17.patch10.81 KBtwistor
#17 interdiff.txt501 bytestwistor
#15 feeds-1848498-15.patch10.81 KBtwistor
#14 feeds-1848498-14.patch11.93 KBtwistor
#11 feeds-1848498-11.patch10.29 KBtwistor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kerasai’s picture

kerasai CreditAttribution: kerasai commented

Hello, the file type restrictions are imposed by the core Fields module and only affect content that is manipulated through the interface (node add/edit form). Unfortunately I don't know of a drop-in solution to your problem, but to do something like this you'd probably need to invoke hook_node_presave() to analyze the node's field data and unset fields with unacceptable file types.

roeneman’s picture

roeneman CreditAttribution: roeneman commented

@kerasai, thanks, this small pointer helped.

For others, for my use case the following solution worked.

Making use of hook_feeds_presave, I constructed the following (not an expert solution I'm sure, but it works for me):

hook_feeds_presave{
    //FILE EXTENSION CHECK
    If (isset($entity->field_attachments['und'])){
    //Make list of extensions seperated by a space.
    $extensions = 'rtf doc txt pdf jpg gif png bmp doc docx rtf ppt pptx xls xlsx';
    //Regex adopted from : http://api.drupal.org/api/drupal/includes!file.inc/function/file_validate_extensions/7
    $regex = '/\.(' . preg_replace('/ +/', '|', preg_quote($extensions)) . ')$/i';
    //Loop through array of file field, if match unset from array and delete file
    foreach ($entity->field_attachments['und'] as $k => $v){
        if (!preg_match($regex, $v['filename'])) {
            $file = file_load($v['fid']);
            unset($entity->field_attachments['und'][$k]);
            file_delete($file, $force = FALSE);
         }
     }
    }

Hope this helps for someone.
Rgds, Roeneman

kerasai’s picture

kerasai CreditAttribution: kerasai commented

Glad you got this going.

To make it simpler, I think you could pass each field value directly to file_validate_extensions and analyze it's return code to determine whether or not to unset it. Might be slightly more complicated than that, but no need to maintain the same code in 2 places.

roeneman’s picture

roeneman CreditAttribution: roeneman commented
Project: Feeds » Mailhandler
Version: 7.x-2.0-alpha7 » 7.x-2.x-dev
Component: Code » Mailhandler

Moving this request to Mailhandler. In my opinion we should catch the files even before they are (temporary) saved.

So request is for a 'file exentions field' in mailhandler, with implementation of 'file_validate_extensions' function, removal of unwanted files from the import, and a (configurable) text in the mailbody (or creation of a .txt file) stating that the file has been removed.

My implementation in hook_feeds_presave for adding text to the mailbody:

$entity->body["und"][0]["value"] = $entity->body["und"][0]["value"] ."\n" .'Attachment ' .$v['filename'] .' was removed, only the following extensions are allowed: ' .$extensions;

I know that proposing a patch is the best way to do this, but that is (still) outside of my programming capabilities. Sorry for that. Please consider this request, as importing files with certain extensions is a safety risk!

Rgds, Roeneman

roeneman’s picture

roeneman CreditAttribution: roeneman commented

@kerasai, indeed you're right. And additionally, when going for this feed_presave implementation, we should first fetch allowed extensions from the $entity->type file-field. This just as a note for someone wanting to go this direction, I believe this is something best implemented in Mailhandler.

kerasai’s picture

kerasai CreditAttribution: kerasai commented

@roeneman good thinking.

I can't promise when, but I'll try to take a look at this. I'm not familiar with the Mailhandler module but I do have a practical use for it so I hope to pick it up shortly.

Dane Powell’s picture

Dane Powell CreditAttribution: Dane Powell commented
Project: Mailhandler » Feeds
Component: Mailhandler » Code

There is precedent for closing security issues such as this one on the Processor side, rather than the Parser / caller side: https://drupal.org/node/1808832

Dane Powell’s picture

Dane Powell CreditAttribution: Dane Powell commented

Also if this is truly a 'security issue' then it needs to go through the proper process for that, rather than being discussed publicly.

twistor’s picture

twistor CreditAttribution: twistor commented
Title: Allowed file extensions » Respect allowed file extensions in file mapper.
Assigned: Unassigned » twistor
Category: Feature request » Bug report
Priority: Normal » Major
Issue summary: View changes

Well, Feeds doesn't have a proper release, so the security process doesn't technically apply.

That said, this should be handled in Feeds file mapper.

twistor’s picture

twistor CreditAttribution: twistor commented
Related issues: +#706908: Enhance filefield mapper to perform more validation on remote URLs
twistor’s picture

twistor CreditAttribution: twistor commented
Version: 7.x-2.x-dev » 7.x-2.0-alpha8
Priority: Major » Critical
FileSize
feeds-1848498-11.patch10.29 KB
twistor’s picture

twistor CreditAttribution: twistor commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: feeds-1848498-11.patch, failed testing.

twistor’s picture

twistor CreditAttribution: twistor commented
Status: Needs work » Needs review
FileSize
feeds-1848498-14.patch11.93 KB
twistor’s picture

twistor CreditAttribution: twistor commented
Version: 7.x-2.0-alpha8 » 7.x-2.x-dev
FileSize
feeds-1848498-15.patch10.81 KB

Committed to 7.x-2.0-alpha9 branch.

Status: Needs review » Needs work

The last submitted patch, 15: feeds-1848498-15.patch, failed testing.

twistor’s picture

twistor CreditAttribution: twistor commented
Status: Needs work » Needs review
FileSize
interdiff.txt501 bytes
feeds-1848498-17.patch10.81 KB

  • twistor committed 9a197f1 on 7.x-2.x 
    Issue #1848498 by twistor: Respect allowed file extensions in file...
twistor’s picture

twistor CreditAttribution: twistor commented
Status: Needs review » Fixed
Bevan’s picture

Bevan CreditAttribution: Bevan commented
FileSize
SDO120198-standalone-feeds.patch11.36 KB
core-adapted-security.patch11.36 KB

This patch is from the security.drupal.org issue node that led to SA-CONTRIB-2015-096, which covered multiple vulnerabilities in the Services module, one of which is the same as the vulnerability identified here.

This patch solves the problem using a solution crafted initially for Drupal 7 core. See #2472895: Provide file name sanitization functions. The new core functions have simply been copied and namespaced.

This patch is now redundant because @twistor has already solved this issue using a simpler patch. I am posting it solely for the purpose of making it public, now that there is no longer a reason to keep it private.

Bevan’s picture

Bevan CreditAttribution: Bevan commented
Version: 7.x-2.x-dev » 8.x-3.x-dev
Status: Fixed » Needs work

This issue likely applies to the Drupal 8 version of Feeds.

It might also apply to the Drupal 6 version if it is enhanced to respect the file field's configured path.

Feeds for Drupal 6 is not vulnerable, but only because it does not use the file field's path when building the destination directory. Since this is a feature that might be added, we should at least open an issue in the public issue queue after the SA is released.—Security.drupal.org issue node

I am not sure if it is better to create new issue nodes for the two other versions that need investigation, or leave both on this issue node.

twistor’s picture

twistor CreditAttribution: twistor commented
Version: 8.x-3.x-dev » 6.x-1.x-dev

It doesn't exist in 8.x. http://cgit.drupalcode.org/feeds/tree/src/Feeds/Target/File.php?h=8.x-3....

As to 6.x, I highly doubt anyone is going to fix the bug that alleviates the security issue. But, we can leave this here to make sure.

twistor’s picture

twistor CreditAttribution: twistor commented
Assigned: twistor » Unassigned
Status: Needs work » Closed (outdated)