File field shouldn't allow any file extension to be uploaded when the list of allowed extensions is left blank

txt is the default which gets overridden and the extension validator removed completely if you remove "txt" and leave the text box for extensions blank on the file field configuration page.

modules/file/file.field.inc

 function file_field_widget_upload_validators($field, $instance)
 // Add the extension check if necessary.
  if (!empty($instance['settings']['file_extensions'])) {
    $validators['file_validate_extensions'] = array($instance['settings']['file_extensions']);
  }

  return $validators;

Here is what I propose. This is the most sensible solution I can think of atm.

a) Provide a configurable default list of extensions in admin/config/media/file-system.

b) File field's configuration form will reflect this default list of extensions.

I see two ways of doing this for b):

1) Prepopulate the form's extensions text box with the list of default extensions.
-or-
2) Leave the text box blank and provide a message below it that says something like "If left blank then the following extensions will be used [list_here]"

I prefer option 2). Option 1) probably still needs to show the user the list of default extensions and so 2) will be less redundant.

c) Either make file_validate_extensions be a required key in the validators array for file uploads and return an error when it doesn't exist (should never happen for behaving code) -or- Don't return an error. Add file_validate_extensions to the validators array if it is missing and populate it with the default extension list in a).

Essentially then we're only allowing white listed extensions, so we won't need to bother checking for dangerous ones anymore.

How about having two configurable lists @ admin/config/media/file-system. A white and a black extension list.

The file_validate_extensions() validator can ensure the whitelisted extension is not also in the blacklist. If it is in the blacklist, then it'll return an error. If the site admin really wants to allow the blacklisted extension then they'll have to first remove it from the blacklist. A message on the form @ admin/config/media/file-system can tell them how dangerous those extensions are and even force them to confirm if they remove anything from that list. It also has the added benefit of giving site admins the power to control how secure their extensions are by letting them add to the blacklist.

Well for starters here is what is used in Drupal 6 for its default list:
"jpg jpeg gif png txt html?? doc xls pdf ppt pps odt ods odp"

IMO it doesn't have to be a long list if it is to be configurable. My point of view is that this list is merely a convenience -- so that users won't have to type these in manually. Prepopulating the form with a long list might be irritating to some because if they wish to specify only one or two they'll first have to delete most if not all. And if they want them all, it could just be left blank and let Drupal do it for them. I think I've already said this, just like to reiterate that.

We could use one of the usability experts to chime in. We already have two? security team members participating. I'd like to get CHX and SUN'S opinion too. The faster we come up with a consensus the faster we can begin work on a patch for this ugly. If I'm going to be the one to write it, I'm fine with attempting it, but only after given the go-ahead.

If anyone thinks my proposal in #5 will work please do speak up and let us know your opinion of the options I provided there. Much of #5 is based on comments I've read from the other issue this stemmed from. We need a clear path to a solution, and soon :)

re: 1)

Blacklist is just a label I gave it as another way of saying "Bad file". To me, how these files are handled is separate. You want to munge it, deny it, rename to .txt, put a dress on it, whatever it is done to it, it's still a bad file. There are two file extension lists that Drupal currently checks: one is a list of safe files to allow, and one is a short list of Drupal 'exploitables' (php, cgi, pl, py) that gets renamed to filename.txt. It's that list that I refer to as blacklist. I understand your need for clarification on that due to the new label I slapped on it.

But if a site admin wants to allow a potentially hazardous extension such as .php for uploading, then Drupal shouldn't come along and ruin their day by always renaming it to .txt or munging it up. What dww and tstoeckler are saying is that if a site admin reallly wants to allow it, then ensure Drupal doesn't make it too easy for them. Give them a warning, ask them for a confirmation ("Do you reallly want to do this? this could be very bad.")

There are reasons one might want to allow such extensions. For example, If I had a Drupal site where I uploaded some of my own personal PHP scripts, I should be able to upload them to Drupal without Drupal hijacking it. Because *I* trust my own files.

Is this fine with everyone?

re: 2)

File extension validation is handled by the $validators array and its key 'file_validate_extensions'. The key 'file_validate_extensions' is itself an array composed of extensions to allow. File field uses that for #upload_validators, which populates the file_validate_extensions array with the extensions the user wants. That's how extensions are being validated right now, just so everyone's clear on that. Anything *not* in that list will be rejected. If a contrib module wants to alter that list, then they should be able to.

re: 3)

That's simple enough and makes perfect sense to me. If you want to allow an extension then you must be explicit.

The change to the preg_match was necessary because if php and foo extensions are being allowed and someone uploads foo.php.foo then neither the rename to .txt or munge will happen and their Drupal site can now be exploited since .foo is an unknown file type.

So now it matches .php. or .php(end of line), I think that's a correct regexp I used in the patch but if not please let me know :)

Instead of having warnings if someone wants to allow php, it'll just do what it does currently and rename to .txt and let the user know what it did. If someone really wants to allow it, then they need to set the allow_insecure_uploads variable.

Also the reason I put the check for allow_insecure_uploads instead of on the same if condition as the preg_match is so that check will only need to be done if the preg_match finds a match.

Anyway, I think I covered all the bases, if not please tell. Thanks for all the input so far peoples.

I don't know. I'm not surprised though. In order to make Drupal secure we made file extension validation a requirement to file_save_upload. Drupal as of right now, supports allowing any extensions to be uploaded and its tests and code expect that I guess. This changes the API.

Took out the requirement and made it fallback to a list of safe extensions. Let's see if this passes.

The list is really to be determined. But the thing is, any code written with security in mind needs to tell file_save_upload the extensions it will accept, so Drupal can properly do its validation check. This is very easy to do. Any code/modules that don't do that should be considered insecure.

@ #24

sun, there are lots of drupal_set_messages in file_save_upload including the functions (file_munge_filename) it calls.

"Should wrap at 80 chars, not before. This comment needs to be clarified, since no one else will understand what is meant with "it's required, *should* not be empty", since required stuff always is non-empty, because that is the sole purpose of #required in the first place."

I'll take it out.

No details in the latest failed test? wtf?

Stupid regexp was causing atleast most of those tests to fail. I left the drupal_set_message because there's a lot of others in that function and I don't know where else to put it.

mradcliffe, personally I don't mind, but I have a feeling others won't like that change. The field is the whitelist and is already marked required so they have to put something there anyway. Nothing stopping you from re-patching and getting it reviewed. I'll add it to the patch tho if someone else agrees with you that it should be added. Let's wait till it passes the testbot first.

This one should work.

"File exists after uploading a file with no extension checking" will never pass because it tests with a .gif which is in the fallback whitelist for when there's no extensions passed in. It will likely pass if gif is taken out. Which could make other tests fail since part of Drupal and its tests expect extension validation to be required and the other parts expect extension validation TO NOT BE REQUIRED. This absolutely does not make any sense. Wtf.

For example, aggregator test does not pass in any extensions so xml must be added to the fallback list in order for it to pass.

Nor will the translation stuff in locale.test ever pass because

$name = tempnam(file_directory_path('temporary'), "po_");

generates a random file with no extension. Looks like we have another issue to open and another to postpone yay!

Every file uploaded to Drupal reaches file_save_upload() eventually, so this hits on every file save operation, which is why we're getting failed tests -- there are tests that expect file validation, and tests that don't expect it. However you can't validate jack without a file extension and the locale tests use no extensions for its files when it does a file upload. These are supposed to be .po files (for the locale tests), but they don't include the .po extension.

file_save_upload() is what calls the various validator functions. Like I said in one of my other comments, file_save_upload() is the last stop before the file gets saved, so any validation/munging is a really good place for it to happen. Since there's no common "inbetween" functions, so no other commonly called function in the flow to say -- hey let's pass this to a validator before heading to file_save_upload() _as far as I know of_, I could be wrong there.

The issue dww referenced in #2 which this stems from requires changes to file_save_upload() like the ones I made for that one to be fixed. And then all that's needed in that issue is to make the update.manager pass in the list of extensions to file_save_upload() for validation, which is the same thing #upload_validators eventually does.

Yea, this isn't just about file field, this is really a global problem. You can't munge filenames without first having a list of extensions to allow. That's why it's important for them to be required, and if they don't exist, fallback to some default safe list or simply return an error. In #693084 the module installer's files were getting munged because a) it wasn't passing in a list of extensions and b) the default safe list contained an unconfigurable variable didn't include tar and gz.

BUT the list of extensions passed in to file_save_upload isn't even checked until later on - just before it gets saved. Whereas the munging happens well before that, so the only list being provided to the munger is that default safe list I mention in b). So files are getting munged even if their extensions pass validation. This affects all file saving globally. Not just the update.manager or file field.

If I understand things correctly then this should pass tests. There is no default whitelist fallback, because that won't pass the tests indicated in #36 and #37. According to the API doc for file_save_upload validators is optional, so if there's no extensions provided, then any will be accepted (the tests expect this behavior apparently). This patch also mostly fixes #693084 (update.manager's module installer still needs a patch to pass in a list of archive types so it won't try to install non-archives).

re again #38:

In file_save_upload() munging needs to happen before the validators because it changes the filename. The file object isn't completely built until $file->destination gets set. $file->destination depends on a 'finished' filename, so if it's going to get changed for any reason, it needs to happen before that.

Also the validators get called after the file object is completely built. You can't really put them before $file->destination is set because the caller may pass in a validator for the file's destination.

The change to #required for file field is causing that test to fail. So this is really postponed until that test is updated. I don't know whether I can upload a patch which changes a test. I wish I could test myself, but I keep getting irritating database is locked exceptions from PDO.

Ok I made a mistake when I made these patches. The issue with file_save_upload() and its munging is a different issue as sun was trying to tell me. So this is a very simple patch to make the file extensions form setting required by default in core when adding/managing a file field. Sun, catch, and dww all agree that this should be required for security reasons.

The test in #41 was failing because it was passing in an empty list of extensions to the file extension validator resulting in a failed upload. File field should have turned off the validation check if the list was empty ( this is what Drupal does currently, and something I had changed in the previous patch ).

And I'm not saying extension(s) anymore.

haha sorry. I did muddy this up one pretty badly. The problem with the munging in file_save_upload() can also affect file field uploads (files getting munged even if they're on the allowed list) which is why I kept trying to upload a patch that fixes that too, but it doesn't belong here I guess.

sun, yea, the test that failed in #41 would be the same thing you're talking about, and it passes now.

The form field for extensions is marked required. The description then tells them to enter their list of extensions. If they attempt to enter nothing, they get an error stating that the field is required and to enter an extension to allow. I don't see how this is inadequate.