Provide useful API functions in Drupal core to sanitize user-provided file names, extensions and paths.

Reasons

Drupal core makes it easy to use file_save_data() (as well as system_retrieve_file(), file_unmanaged_save_data(), file_copy(), file_unmanaged_copy(), file_move() and file_unmanaged_move()) insecurely because it does not provide any functions or advice about sanitizing user-provided file names, paths and extensions, which is a non-trivial task. The recent security release for Services uses this solution.

Changes

  1. New PHP constant FILE_SAFE_EXTENSIONS: 'jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp'
  2. Supporting function to make the list configurable (via $conf['file_safe_extensions']) and help other functions that optionally use the list via an optional parameter; file_safe_extensions()
  3. Functions modified to use the list instead of hard coded duplicate string (No change to logic):
    1. system_update_7060() (uses constant)
    2. file_save_upload()
    3. Documentation of various functions
  4. file_munge_filename()'s second parameter $extensions becomes optional and defaults to the configurable list.
  5. New API functions:
    1. file_check_name_extension($name, $extensions = FILE_SAFE_EXTENSIONS)
      Sanitizes a user-input file name and extension.
    2. file_check_name($name)
      Sanitizes a user-input file or directory name.
    3. file_check_destination($destination)
      Sanitizes a user-input file path, name and extension.
    4. file_check_destination_uri($uri)
      Sanitizes a user-input file URI.
    5. file_check_destination_scheme()
      Sanitizes a user-input file scheme.
    6. file_safe_extensions()
      Gets the list of safe file extensions.
  6. For various functions that accept a file path as a parameter to use as the destination for a new file or file to be copied;
    1. Document that the destination must be sanitized
    2. Reference new API functions for sanitizing
  7. Tests for everything

Notes

I chose the keyword "check" instead of "sanitize" for the function names to be more consistent with other Drupal core functions like check_plain(), even though "sanitize" is a more accurate description than "check", including for those existing core functions.

I chose to allow only alphanumeric characters, underscore (_), hyphen (-), space ( ) and period (.), for file and directory names. Those four punctuation characters are removed if they are first or last characters. I made this decision because of a report of PHP not handling filenames with special characters very well, and considering that these characters can be from RTL alphabets. I am still looking/waiting for sound evidence/advice about non-alphanumeric characters in file names. Until then I have erred on the side of caution.

Various schemes are considered unsafe to allow untrusted users to write files to; temporary, file, http, https, ftp. I am not sure if this list is comprehensive or excessive. I also wonder if it should be configurable.

This patch was used to fix the security vulnerability for Services. As such it is for Drupal 7. However Drupal 6|8 share these shortcomings and might|should also be fixed.

When/if this is fixed in Drupal core, Services might be refactored to use the new functions and remove any duplicate code. Issues should be opened in their queues to that effect.

CommentFileSizeAuthor
file_check_destination.patch63.98 KBBevan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Bevan’s picture

scor’s picture

Issue tags: +Security improvements