Regression: file_munge_filename() extension handling broken by move to File Field

some analysis and then a patch:

update.manager.inc uses the function file_save_upload (defined in file.inc) from within function update_manager_install_form_submit() to upload the archive file.

The function file_save_upload sets the variable $extensions and tries to read the variable "upload_extensions_$rid" to incorporate extensions defined there (which should be 'tar zip' etc.) into $extensions.

  // Build the list of non-munged extensions.
  // @todo: this should not be here. we need to figure out the right place.
  $extensions = '';
  foreach ($user->roles as $rid => $name) {
    $extensions .= ' ' . variable_get("upload_extensions_$rid",
    variable_get('upload_extensions_default', 'jpg jpeg gif png txt html doc xls pdf ppt pps odt ods odp'));

  // Begin building file object.
  $file = new stdClass();
  $file->uid      = $user->uid;
  $file->status   = 0;
  $file->filename = file_munge_filename(trim(basename($_FILES['files']['name'][$source]), '.'), $extensions);
  $file->uri      = $_FILES['files']['tmp_name'][$source];
  $file->filemime = file_get_mimetype($file->filename);
  $file->filesize = $_FILES['files']['size'][$source];

  }

The variable "upload_extensions_$rid" doesn't exist however, so extensions like tar.gz, tar and tar.bz2 don't get read into $extensions. As a result the filename gets "munged" by file_munge_filename() in file.inc.

function file_munge_filename($filename, $extensions, $alerts = TRUE) {
  $original = $filename;

  // Allow potentially insecure uploads for very savvy users and admin
  if (!variable_get('allow_insecure_uploads', 0)) {
    $whitelist = array_unique(explode(' ', trim($extensions)));

    // Split the filename up by periods. The first part becomes the basename
    // the last part the final extension.
    $filename_parts = explode('.', $filename);
    $new_filename = array_shift($filename_parts); // Remove file basename.
    $final_extension = array_pop($filename_parts); // Remove final extension.

    // Loop through the middle parts of the name and add an underscore to the
    // end of each section that could be a file extension but isn't in the list
    // of allowed extensions.
    foreach ($filename_parts as $filename_part) {
      $new_filename .= '.' . $filename_part;
      if (!in_array($filename_part, $whitelist) && preg_match("/^[a-zA-Z]{2,5}\d?$/", $filename_part)) {
        $new_filename .= '_';
      }
    }
    $filename = $new_filename . '.' . $final_extension;

    if ($alerts && $original != $filename) {
      drupal_set_message(t('For security reasons, your upload has been renamed to %filename.', array('%filename' => $filename)));
    }
  }

  return $filename;
}

In the end we end up with file.tar_.gz in the temp directory and this can't be untarred and thus results in the error.

The attached patch removes the checking for the variable "upload_extensions_$rid", which doesn't exist anyway, and instead adds the necessary extensions for users with the permission of "administer software updates".

Note however that I don't know if a mechanism for setting the missing variables was meant to be built in later on. Also in the existing code it says of the modified function: "@todo: this should not be here. we need to figure out the right place." I don't know who put this there and why.

Better patch assuming that the deleted lines are cruft.

I left "zip" in there as a supported upload extension, although I couldn't find any code in Drupal that would allow the unzipping of zip-archives. Only .tar, .tar.gz, and .tar.bz2 seem to be supported by /modules/system/system.tar.inc.

I wrote a test for this patch. It tests uploading a tarball and installs the module uploaded. The only problem is that in order to test the upload of a module the module actually is installed in the drupal install - not in the simpletest sandbox as that would not work.

There is a tearDown function that deletes the module that the test installs, but if an error happens before that there is a useless module floating around in the install.

So, it *can* be tested, but I am not too happy with a test that leaves garbage in the system if it fails. Not sure where to go from here.

Thanks a lot for the test. I ran it locally on a fresh install of Drupal head and it ran through without any errors (screenshot attached). It also cleaned the modules directory after running.

It's OK to leave garbage if tests fail, they should never be run on a live site. File could also be a real one placed into modules/simpletest/tests/files or similar.

Setting to CNR for the bot.

I'm very confused (and worried) by this patch. WTF happened to file_munge_filename() and the upload_extensions_* variables in D7? ;) That used to be critical security functionality in upload.module. Upload module appears to have been replaced in D7 by File field, but I don't see any equivalent replacement for this functionality. That appears to be a major oversight + regression during the move to file field, and completely unrelated to the update manager. ;)

So, for starters, at the *very* least, we need:

A) the extensions in file_save_upload() to be initialized by a variable_get() so they can be modified.

Better yet, we need:

B) to restore the per-role stuff we used to have.

Better still, we need:

C) to restore the admin UI where the extension handling can be defined appropriately per role.

Finally, we should have:

D) a D6 => D7 upgrade path to rename these variables (assuming they're not longer going to be prefixed with "upload_extensions" -- although maybe that's actually still the most self-documenting name for these, I'm not sure).

Once all that's done, we can contemplate the right way to handle .tar, .zip, .tgz, etc for users with 'administer software updates'. I'm not convinced we should always append those to the allowed extension list, but maybe that's the best we can do. Regardless, that's small potatoes compared to A-D above.

Cross-linking to #563000: Provide upgrade path to file - a proper upgrade path from upload should have at least incorporated the variable renaming, if such an upgrade path existed at all. This might even be a duplicate.

Also adding security tag.

Thanks for the link, catch. I assumed there must have already been an issue where this was discussed. I read through there and didn't see any explicit mention of the extension whitelist handling, so I added #563000-64: Provide upgrade path to file.

Cheers,
-Derek

Patch needs some more work but it fulfills #693084-8: Regression: file_munge_filename() extension handling broken by move to File Field list I hope.

The tests from #5 need to reapplied though.

Troubles are:

1. What should the menu path be?
- Is admin/settings/file_role_upload a good path?

2. It is unclear what 'user 1', 'default' versus the current roles is.
- Is a three table layout better?

update.manager works with the patch. That is I managed to pass the security test for upload extensions.

@clemens.tolboom: Thanks for working on this, but that's an empty patch file. ;) Can you upload your code? Thanks!

The process with images is weird.

- I configured my image field to allow xmp image files.
- I set the default to a XMP file which gets _uploaded_ (bug?!)
- As user-1 when add/content the allowed extensions are _without_ xmp.

The intersection between a roles allowed extensions and a image/file field intersection is unclear.

So I am unable to upload an xmp as user 1. If found out the image module has hard coded extensions too.

Do we need to make image extensions role based configurable?

For me the string with file extensions should be defined once. I did some grepping

- file.inc : ok to me
- modules/image/image.field.inc : in several flavours. Should only be one
- profiles/standard/standard.install : guess it's ok
- modules/system/system.api.php: misaligned with system module: 'extensions' => array('tar', 'tar.gz', 'tar.bz2'),
- modules/system/system.module: 'extensions' => array('tar', 'tgz', 'tar.gz', 'tar.bz2'),

grep -r "[^\.]doc" * | grep -v "css" | grep -v "CVS" | grep -v "document" | grep -v "\.js" | grep -v "ajax\.inc"

--cuts--
includes/file.inc:    variable_get('upload_extensions_default', 'jpg jpeg gif png txt html doc xls pdf ppt pps odt ods odp'));

grep -r "[^\.]jpg" * | grep -v "css" | grep -v "CVS" | grep -v "document" | grep -v "\.js"

--cuts--
includes/file.inc:    variable_get('upload_extensions_default', 'jpg jpeg gif png txt html doc xls pdf ppt pps odt ods odp'));
modules/system/image.gd.inc: *   A string containing one of the following extensions: gif, jpg, jpeg, png.
modules/image/image.field.inc:        'file_extensions' => 'png gif jpg jpeg',
modules/image/image.field.inc:    $supported_extensions = array('png', 'gif', 'jpg', 'jpeg');
profiles/standard/standard.install:      'file_extensions' => 'png gif jpg jpeg',

grep -r "[^\.]tar" * | grep -v "css" | grep -v "CVS" | grep -v "document" | grep -v "\.js" | grep -v "ajax\.inc" | grep "gz"

--cuts--
modules/system/system.api.php:      'extensions' => array('tar', 'tar.gz', 'tar.bz2'),
modules/system/system.module:    'extensions' => array('tar', 'tgz', 'tar.gz', 'tar.bz2'),

html should be removed from any default list of safe file extensions because uploaded html files can contain XSS attacks.

The inclusion of "html" here is a remnant from when upload module allowed html files to be uploaded by default, which was fixed in Drupal 5.3, see http://drupalcode.org/viewvc/drupal/drupal/modules/upload/upload.module?...

Due to an oversight, in Drupal 6, html was removed from the upload_extensions_default in upload.admin.inc but not file.inc

would be nice to find a generic solution, considering all the paths to file uploads already in core, let alone when all the contrib modules begin adding their cruft. i agree that having a basic default fallback is the best; we should make sure there's a simple way to send any uploaded files through these checks.

tagging

subscribe

Perhaps obviously this problem also affects the ability to upload contrib modules via the Web UI.

Renaming the file's extension to .tgz permits the upload (i.e. no rename).

As best I can tell file.module should have .tar.gz allowed as an extension (since .tgz is already allowed). I did try adding:
application/x-tar-gz
however, this seemed to have no effect.

@emmajane: Yes, that was exactly the bug from the original post. I repurposed the issue to clarify the real problem, of which the failure to upload .tar.gz files from the Update manager is but one symptom...

Fix patch in #14 so that it applies.

If I take the test from #5 and run it with the patch from #25, the test fails. So I don't think #25 fixes the bug. Here's the patch from #25 (with some code-style changes) with the test from #5. Note that there is a TODO in the code.

Forgot --no-prefix in last patch. Having read all of the patch now, I don't think having functions like file_upload_extensions_user_1() and file_upload_extensions_default() is the cleanest way to go. Instead of re-inventing the wheel, the following patch inspires itself from D6's upload.admin.inc.

#693084: Regression: file_munge_filename() extension handling broken by move to File Field got rid of the 'upload files' permission, so I added it back.

I added a file_permitted_extensions() function to address the @todo in file.inc. I'm not sure if we would want this function to take $account as a parameter. Someone with "administer software updates" can additionally upload tar, tgz, tar.gz, tar.bz2 files. This list is hardcoded; I can easily make this list configurable, though. There is no attempt to eliminate duplicates in the list of extensions generated by this function.

#9: A) and B) are already is the case (without any patches). The current patch attempts to address C). With respect to D), the patch doesn't rename the variables, so the upgrade path should not need extra handing AFAIK.

#16: I fixed the discrepancy in settings.api.php.

#17: I removed "html" as a permitted extension.

Added an "upload theme" test, which will fail until #764418: Move query conditional helper functions to database.inc is fixed.

Failed as expected. If this is committed before #764418: Move query conditional helper functions to database.inc, #28 is the patch to commit.

If I add a File field to a content type and specify that e.g. .exe files may be uploaded, this allows any user to upload .exe files, even though exe is not in the list specified on admin/config/media/uploads. That's not intended, is it?

This patch does not integrate with fields. So, if you create a content type with a file field to which you may upload .exe files, then anyone with permission to create content of that content type will have permission to upload .exe files when doing so (that is my understanding of content type permissions, anyway).

The language of the new admin configuration page in this patch should probably be adjusted to be more specific about what uploads it specifically controls. Personally, I just want to be able to upload tar.gz files when I install themes/modules.

File extension configuration per field (instance) is absolutely totally required.

I'm not sure whether a system-wide and per-role configuration still makes sense. #type file_managed should/needs to allow to specify allowed file extensions for managed file uploads in custom forms (which affects update manager). What else and what other functionality do we need to support?

Updated To me, this issue, as in the OP before the title rename in #9, was just to allow the uploading of tar.gz files in the update manager. But here's a summary as I understand it:

Whoever was responsible for killing upload module (see #563000: Provide upgrade path to file) wasn't diligent about getting rid of its last dying remnants in file_save_upload in file.inc, leaving the upload_extension_* variables. In #9 above, dww explains how these variables served an important purpose. In #10, catch notes that this issue ties into upgrade path issues.

The easy solution explored so far was to re-insert out some of the still-needed functionality of upload module into file module. A number of places in Drupal call file_save_upload, e.g. for uploading the logo and favicon in system module and for uploading a picture in user module, to name two. See #28 for the patch that got furthest along this path.

As the admin UI I contributed in #28 is not without its own issues, I'm scaling back my patch, so that we at least fix the bug. Adding an admin UI and adding per-role stuff can be a "feature request".

Minimalist patch that only accomplishes A in #9 above. (For a patch that accomplishes A, B, C from #9, see #28).

@jpmckinney: Allowed file upload extensions are a critical aspect of Drupal security. If people configure this wrong, hackers can own the site. It's not enough to just get this working for the update manager again, we need to fix a critical regression in D7 before we can release.

@sun re:

File extension configuration per field (instance) is absolutely totally required.

Can you elaborate on this?

I think the simpler and more general we make this, and the more like the rest of the Drupal permission system, the better. (See also #110242: make access to filters first class permissions). Therefore, perhaps *all* we need is the per-role configuration. At least as far as security is concerned.

In addition to that, for UX, we might want to validate and reject various file extension types for certain file fields (e.g. only image types for an image field, whatever). But, that seems out of scope for this issue (at least until I hear sun's answer).

However, without per-role whitelists, how can site admins safely allow file uploads on their site?

Sorry, my statement was based on misinformation. File field and #type file_managed already allows to configure allowed file extensions per field instance.

Further, the form element #type file_managed defined in http://api.drupal.org/api/function/file_element_info/7 defines #upload_validators, which seems to invoke http://api.drupal.org/api/function/file_validate_extensions/7 by default.

Following #type file_managed's flow, http://api.drupal.org/api/function/file_managed_file_save_upload/7 invokes http://api.drupal.org/api/function/file_save_upload/7, which in turn calls http://api.drupal.org/api/function/file_munge_filename/7 with a list of extensions to not munge, using per-role variables.

However, that code in http://api.drupal.org/api/function/file_save_upload/7 looks weird and wrong, and even contains a @todo:

  // Build the list of non-munged extensions.
  // @todo: this should not be here. we need to figure out the right place.
  $extensions = '';
  foreach ($user->roles as $rid => $name) {
    $extensions .= ' ' . variable_get("upload_extensions_$rid",
    variable_get('upload_extensions_default', 'jpg jpeg gif png txt html doc xls pdf ppt pps odt ods odp'));
  }

Those variables cannot be configured anywhere. We have an existing configuration page for "File system" on admin/config/media/file-system though.

--

Technically, those $extensions in file_save_upload() are superfluous, since they are duplicating existing #upload_validators previously defined and applied in the process. If we still need this munging at all, then at least the list of extensions of those system variables has to be shortened to actually harmful extensions, i.e. "php exe htm html js css".

However, I'm really not sure why we still need that munging. I can simply configure my file fields correctly and don't allow such extensions in the first place. And if I explicitly want to allow such extensions, then Drupal shouldn't hi-jack my uploads.

--

Update manager, however, does not use #type file_managed. And it seems like #type file does not implement any of the processing and validation properties and callbacks of #type file_managed. A proper fix is to make Update manager use a #type file_managed for its file upload, so it's able to specify the allowed extensions, and we don't need the proposed hack of the latest patch.

--

However, from a technical and API standpoint, I do not understand why the #upload_validators property and processing is part of #type file_managed instead of #type file. This smells like a really major misconception to me.

For per-role whitelists, see #28. The only real issue with #28, as I see it, is the language of the admin UI.

If we want to switch to using the Field API for file_save_upload, I would recommend that the people proposing it write a patch, as I am not familiar enough with Field API to do that.

If people configure this wrong...

There are myriad ways to misconfigure Drupal that create vulnerabilities. If we cared only about security, we would recommend people not modify Drupal at all, in case they write some custom theme/module that creates a vulnerability.

@jpmckinney:

There are myriad ways to misconfigure Drupal that create vulnerabilities

Indeed. That's why the job of those of us on the security team (and all Drupal developers, really) should be to make Drupal as secure as possible by default, and as intuitive and simple to configure securely. Lots of complicated knobs for permission-related functionality spread out through multiple systems and admin interfaces is a recipe for disaster.

Not only does the security team investigate and fix actual vulnerabilities, we're active in the development process to try to avoid things that people can easily shoot themselves in the foot with.

If we cared only about security, we would recommend people not modify Drupal at all, in case they write some custom theme/module that creates a vulnerability

We don't care *only* about security, but we *do* care about it. ;) And we try to educate developers and designers on how to properly use the Drupal APIs to ensure that they do *not* introduce a vulnerability. Hence, we're concerned about making those APIs as well designed and clearly documented as possible. We also want to make a default Drupal install as secure as possible so that if people aren't particularly savvy about web security issues, their site isn't full of problems that are easily exploited. You can't just adopt the position "it's hard to make Drupal secure, so who cares? We can't do anything if all we care about is security". Providing actual security for Drupal as a whole is about finding the appropriate balance (which is always shifting) between convenience, flexibility, and security.

Point taken. Still, I'm going to wait for someone with better Drupal chops to write the next patch.

Whoever was responsible for killing upload module (see #563000: Remove upload module and provide upgrade path to file) wasn't diligent about getting rid of its last dying remnants in file_save_upload in file.inc, leaving the upload_extension_* variables.

Yikes, apologies. ;-)

Putting in the tracker, I'll review and see if there are ends I can tie up.

I've been out of the loop on a lot of the recent changes to core so take my comments with a grain of salt. I think I'm the one who put that initial @todo in there in the run up to D6 #115267: Simplify File Uploads, Centralize File Validation and Quotas, Fix File Previews.... when we were trying to move the file saving code out of upload.module and into file.inc. My goal had been to either enhance the 'file' element to act like 'file_managed' and pass the valid extensions in via something like #upload_validators. We clearly didn't get that done.

I agree with sun's comments in #39, we should remove/minimize the list and instead rely on validators. I don't remember what the rationale was for not adding enhanced validation to the 'file' element. I think quicksketch might be able to provide more detail on that.

#801330: Munging archive extensions breaks module installer was duplicate.

Ok here's what I did.

Per-role permissions can already be accomplished via giving roles access to create specific content types (e.g: a content type that allows binary extensions can be given create access to a trusted role), unless I'm misunderstanding something. It may not be the most ideal way, but it is a way.

There are to two lists that deal with extensions in file_save_upload. One is a whitelist that's already been discussed and one is a blacklist (preg_match('/\.(php|pl|py|cgi|asp|js)$/i', $file->filename)). Sun's short dangerous extension list @ #39 contains some of what's already in the blacklist. This is the same line that should include sun's list and then we shouldn't need to munge filenames anymore.

Content types allow you specify what extensions you wish to allow. There is a problem however, if you enter nothing, it will allow all extensions. So we can't simply remove the dangerous extension checks because unsuspecting users with little knowledge of security will unintentionally shoot themselves in the foot. Now file_save_upload() is where the validation functions get called, and is the last stop before the file gets let loose, so it is a good place to have these checks.

If you choose to allow a potentially dangerous extension file, then you must explicitly list that extension in the content type's file field configuration and *Drupal won't mess with it*. -OR- you can allow insecure uploads. All of the code I've looked at so far uses "file_validate_extensions" to whitelist extensions. So if file_validate_extensions is in the validators array, then only those extensions will be allowed. If however it isn't in the validators array then we need to check for dangerous extensions or if the allow insecure uploads variable is set ( if it is, then any extension will be allowed, hence insecure).

As far as this 'file_managed' type stuff goes. The code used in update.manager.inc looks very similar to what is used in aggregator and the users module for their file upload handling parts. The code in update.manager.inc looks to be perfectly capable of performing said task as is (Especially with it being this late in the game, I think it would be a wasted effort to change it now, if it can handle it as is), the main issue with it is that there were no validators passed off to file_save_upload, infact, there was a @todo in there indicating that it needed validators. A list of archive extensions given to the file_validate_extensions validator will allow the update manager to pass the validation check for allowed extensions.

The patch provided attempts to do what I outlined above.

doh

What about the filename munging functionality? Isn't this critical for security?

Right now, with or without the above patch, I can add a file field to a content type, set allowed file extensions to "foo" and allow non-admins to create nodes of this type, then as a non-admin user create a file named test.html.foo with contents <script>alert('XSS!');</script> and upload it. The filename will not be munged.

When you download the file, the javascript alert dialog pops up. This is because Apache scans the file name from the right side for a known extension, skips over the unknown "foo" and serves the file as text/html, allowing for XSS attacks.

Oh ok, you're right. Thanks for the input. I was just about to re-up the patch.

Thanks again mbr. See if this helps. Getting a little tired...

hm I think it needs another munge for when it's allowing all extensions.

Yes, there is no filename munging when $validators['file_validate_extensions'] doesn't exist; it will only detect .html at the end of the filename.

Also, re: the blacklist of "potentially executable files" in this section, there are probably others. For example .swf is just as dangerous as .html in terms of executability, since most people have flash installed and flash can contain malicious scripts that e.g. steal session cookies. Also, according to http://heideri.ch/jso/#svg .svg files can execute javascript in many browsers.

Ok this is a security improvement I think. Where all extensions are being allowed this now munges anything that isn't the same extension of the file being uploaded.

http://www.onebillionbytes.com/dangerous_filetype.pdf

How big of a list is needed? :) A big one probably needs to be configurable thru the admin UI and preferably maintained by a security team. I don't see it hurting performance for the majority as long as it's only looked at when allowing all extensions and most people shouldn't be doing that, I think there should be a warning urging them not to on file field's configuration page.

It's nice to see some energy being put in here, but I'd like to suggest we refocus the effort...

A whitelist is always safer than a blacklist. Just list the extensions you want to allow, don't try to list all the extensions you think are unsafe.

Also, at this late stage in the D7 release cycle, I think it's the wrong time to try to radically redesign how all this works. We just need our happy filename extension whitelists back that we lost in the move to filefield so that D7 isn't a gaping security hole. I'm sure there are lots of ways we could make them more slick, but all of that should happen in D8. The critical bug here is that there is *no way* to secure file uploads in D7, and that's a release blocking critical bug. So, let's just (more or less) restore the functionality we've had for ages since it's already fairly well understood.

Once 7.0 is out and D8 opens for development, we can open a new issue to revisit how all this works and try to make it even better.

Thanks,
-Derek

dww, that is the whole point of the patch. There are white lists and they are configurable. But file field makes it extremely easy for a newbie to _allow everything_ without them even realizing the implications and without giving them any form of warning which is why there needs to be a limited blacklist of the most dangerous file types that are likely to affect their site and its visitors. _However_ IF you want to allow any of the blacklisted extensions then you can white list them! I thought I made all this clear in #46.

I should probably say munge & rename to .txt list instead

@dhthwy: Sorry, I haven't had time to read the patches, just skimmed some of the recent discussion. My apologies.

Re: "Content types allow you specify what extensions you wish to allow. There is a problem however, if you enter nothing, it will allow all extensions."

Then that's a critical bug in filefield, IMHO. It can't fail unsafe. It needs to fail safe. If you don't specify anything, it shouldn't let you upload anything. If the D7UX people come along and complain about this, they can find a nice way in the UI to detect this configuration, warn the user when they've configured a file field that doesn't allow any uploads, and make it easy for people to do this right. The target audience of D7UX are *exactly* the people we should be going out of our way to help configure this securely, since they're generally the people who have the least security knowledge to draw on.

For the (rare) cases where you actually need to give a trusted user the ability to upload absolutely anything, the UI could have some kind of "grant full access (WARNING: this is extremely dangerous, don't do this unless you can trust all users who can create %type content)" checkbox or something. I'm too busy with other things right now to design (much less implement) the whole UI, but I'm sure we can figure out a reasonable solution if we have a separate issue to discuss this aspect.

Cheers,
-Derek

Np, I actually agree with you that having a 'blacklist' sucks. The code was actually there already, I just added some file extensions to it, figured it was there for a good reason. I don't know what's going to happen with the allow all extensions issue, it might be decided that it should be kept. Keep in mind though, the 'blacklist' it is only checked when there are no white listed extensions specified.. so as long as you list your extensions, that code will never trigger.

IMO it is essential for #803926: File field shouldn't allow any file extension to be uploaded when the list of allowed extensions is left blank to be discussed before work on this issue can continue.

dww, I took note of #700558: Add test to ensure filename extensions are properly merged in the Update manager UI

And lastly, my patch needs tests and maybe some adjustments. I'm not going to bother writing any tests for it tho until my patch gets an "ok that approach will work."

Yeah, I'm fine with saying this should be on hold until #803926 is decided. I replied there. Thanks for opening that.

I can add back that @todo if anyone has a problem with me removing it.

This should pass testbot and I've verified this fixes stuff like the update manager's module installer.

so in this patch, calling file_munge_filename(), which is critical for security, only happens if $validators['file_validate_extensions'] is not empty. In this case the function docs need to make this clear for module developers: $validators may be an optional associative array, but to prevent file upload exploits, callers must specify a list of allowed file extensions in $validators['file_validate_extensions']

mfb, yes that is a correct assessment. This patch puts the API function file_save_upload() more in line with what it is supposed to do (according to the docs, tests, and other core code, probably contrib modules as well).

Requiring validation will be an API change and break many tests, core code, and possibly contrib modules. And I'm certain we can't do that at this late stage.

I agree wholeheartedly that developers should be strongly encouraged to validate files, and dangers of not doing so be effectively communicated throughout documentation and other channels.

And so file_save_upload should say in the doxygen:

$validators An optional, associative array of callback functions used to validate the file. See file_validate() for a full discussion of the array format. Beware, if left empty then there is no validation and that means any file extension can be uploaded which should only be allowed to trusted users if at all.

or something like that with a lot better grammar :P

I have just rerolled the patch, jhodgdon wrote a better comment based on #66. As I would have RTBC'd without this, I am RTBC'ing it now.

webchick, I tried that, doing so will break many tests, and other parts of Drupal, it's an API change. Drupal was designed for validators to be optional.

This patch here attempts to add a list of defaults
http://drupal.org/node/803926#comment-2997576

Warning if you read thru the comments, I made quite a mess out of that issue.

The default list is long because it has to cover all the cases where no extensions were passed in, yet it still fails many tests.
$validators['file_validate_extensions'] = array('jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp po xml');
And it actually needs to also include all the archive extensions for the module installer, unless the module installer is changed to pass in a validator with the list of archive extensions.

There is atleast one test that passes in a filename with no extension to file_save_upload(), that will always fail with the validator turned on.

I think if we want to do that, we should open a separate issue for the API change.

+    // Munge the filename to protect against possible malicious extension hiding
+    // within an unknown file type (ie: filename.html.foo).

I'm not sure I ever understood the purpose of file_munge_filename() (I have requested some better documentation in #710640: Improve documentation for file_munge_filename()), but this comment seems to describe a different scenario than that in the Doxygen comment of file_munge_filename().

This comment mentions an unknown extension (.foo), while the Doxygen comment for file_munge_filename() mentions a well-known extension (.pps = a PowerPoint file). Is that intentional?

"ie" should be written with periods, i.e. (i.e. filename.html.foo).

If Apache doesn't recognize the file type it'll skip the extension and keep moving on until it finds one it recognizes. If filename is filename.php.pps and Apache doesn't recognize pps then it'll execute the file as a PHP file resulting in a possible hacked server. My understanding is that file munging is done to protect such a scenario.

There is a limited blacklist of extensions that match php, js, etc. It'll append .txt to to those files. This will help protect Drupal against exploits where all extensions are being allowed. Best effort only, as having a blacklist is essentially a futile effort as dww pointed out.

I put the test from #5 in with the patch from #67. The test was written before the issue started dealing with file_munge_filename(), so it just tests that a module can bu uploaded from the browser and enabled.

Please disregard this patch if the test is not really testing what is the issue anymore.

We're all set here. The patch provides the fix and includes a reasonable test. An issue has been created to further document the file_munge_filename(): #710640: Improve documentation for file_munge_filename() where I have included dww's comment above (#73). Also, davereid keeps saying "Do it! Do it!" whenever I look at this issue and want to mark it RTBC.

Adds a fall back list of safe extensions so file munging can still happen when no extensions are provided via $validator (to please webchick). The same list is used for D6 minus .html.

We need a) safe defaults b) allowance of any file type to be upped. So why not make it so that you need to explicitly pass in '$validators['file_validate_extensions'] => array() to get anything uploaded? This way , we still can upload anything if you explicitly say so. If you just leave out the file_validate_extensions then you get a safe default list.

#77. Let's see how many tests this fails!

Fixing.

In order for the tests to pass both Aggregator and Locale module needed to pass in an extension validator. The alternative was to add opml and po to the default list of extensions. For locale, the situation is a little different because it tests with files which contain no extension at all, to fix this .po was appended to the filenames it tests with.

For Aggregator and the Locale module the exact list of extensions to allow should be another issue. Also the patch in #74 is separate. Another issue needs to be opened up for the module installer. It needs a) an extension validator and b) a test (perhaps the one in #74).

Fixed a few minor issues found by Dreditor.

This patch should do what chx suggested in #77. Tests are included. The function file_munge_filename() is tested elsewhere however there were no tests to ensure file_save_upload() is calling file_munge_filename() when appropriate, so tests were added for that too.

1) "where do i collect my winnings?"

That's the success message indicating the file was uploaded. 'You WIN!' is used elsewhere in file.test, not just the tests I added. I didn't plan on refactoring the entire file.test code.

2) "can we kill the php4-isms, and change 'var' to 'protected'? also, i think coding style is to leave a blank line between class variable declarations."

I noticed this and used var for consistency because the test already used var for $image. However that can be changed.
It is also used in the other following tests:
find modules/simpletest/tests/ | xargs grep 'var \$'
modules/simpletest/tests/menu.test:447: var $links = array(
modules/simpletest/tests/common.test:478: var $validTags = array(
modules/simpletest/tests/file.test:540: var $image;
modules/simpletest/tests/file.test:544: var $phpfile;
modules/simpletest/tests/file.test:549: var $maxFidBefore;
modules/simpletest/tests/filetransfer.test:157: var $commandsRun = array();
modules/simpletest/tests/filetransfer.test:158: var $connectionString;

So really, a separate issue should be opened up to address this.

3) "i don't think is your patch, but why do we only test this in testNormal()? setUp is run for each test, so we should test it there."

I don't know ATM, as said, it wasn't my intention to refactor the entire test.

4) "not so nitpick, do we want to leave this list "php|pl|py|cgi|asp|js" hard-coded? i can think of others, but more importantly, while that's a good default, we should allow admins to adjust that based on their server setup. i'm ok if that's another patch, but it struck me as a bit inflexible."

That's another issue too I believe.

Sounds like every concern was addressed so back to CNR. I do not think I can RTBC this.

Changed var to protected.

Back to RTBC. Crosspost.

Makes sense and fixes a critical bug. Committed to CVS HEAD.

Don't know if I set status correctly, but I still get this error running 7.X-dev dated 7/20, clean install. This is similar to other isues marked duplicate to this one. #93 indicates patch is in already. I am trying to install this module from my local system. Is this a new problem?

ps. Process was - toolbar ->Appearance -> modules -> install new module -> Upload a module or theme archive to install

Can't recreate on clean install of head.

#95 was fixed in #747252: Cannot extract themes and modules