[#946 state:resolved] New config directives, `DeniedAssetFileExtensio…

…ns` and `AssetFileExtensions`, as well as a new MT::App method: `validate_upload()` which utilizes them.

In the next release, all code which deals with uploaded files needs to be refactored to use this method (or whatever it becomes) as a gatekeeper for sanity checking potential uploads.

lib/MT/App.pm

@@ -2441,6 +2441,59 @@ sub upload_info {
     return ( $fh, $info );
 } ## end sub upload_info
 
+sub validate_upload {
+    my $app        = shift;
+    my $args       = shift;
+
+    ###
+    # Check filename for validity and allowed upload file extensions
+    # The file need not exist for these checks.
+    ###
+    if ( defined $args->{filename} ) {
+        require File::Basename;
+        my ($file) = File::Basename::fileparse( $args->{filename} );
+        my $cfg    = $app->config();
+
+        return $app->errtrans( 'Invalid upload file' )
+            if
+            ! defined $file 
+                or
+            $file =~ m{
+                /       | # Filename shouldn't have slash; indicates directory
+                \.\.    | # No upward traversal allowed
+                \0      | # No NULL bytes allowed
+                \|      | # No pipes allowed
+                ^$        # Empty filename
+            }x;
+
+        if ( my $deny_exts  = $cfg->DeniedAssetFileExtensions ) {
+            # Return error IF file extension matches
+            MT::Util::match_file_extension( $file, $deny_exts )
+                and return $app->errtrans( 'Invalid upload file' );
+        }
+
+        if ( my $allow_exts = $cfg->AssetFileExtensions ) {
+            # Return error UNLESS file extension matches
+            MT::Util::match_file_extension( $file, $allow_exts )
+                or return $app->errtrans( 'Invalid upload file' );
+        }
+    }
+
+    ###
+    # Evaluation of a binary data to see if it contains HTML or
+    # JavaScript content in the first 1K of the body.  Image
+    # files (in particular) that contain embedded HTML or JavaScript are
+    # a known vector for an IE 6 and 7 content-sniffing vulnerability.
+    ###
+    if ( my $data = $args->{data} ) {
+        require MT::Image;
+        MT::Image->has_html_signature( data => $data )
+            and return $app->errtrans( 'Invalid upload file' );
+    }
+
+    1;
+}
+
 sub cookie_val {
     my $app     = shift;
     my $cookies = $app->cookies;
@@ -4111,6 +4164,33 @@ a file handle and a hashref of header details for a given field name.
 If there is a problem with the upload, undef will be returned instead
 and a warning may be logged with more detail.
 
+=head2 $app->validate_upload( \%param )
+
+This method performs basic upload file validation on either the filename or
+the data contained in the file.  MT::App subclasses should override this
+method as needed to perform validation specific to the app.  See
+C<MT::AtomServer> for an example.
+
+The C<%param> has can contain one or more of the following keys:
+
+=over 4
+
+=item * filename
+
+The value of the C<filename> key is expected to be a filename (with or without
+a path) and will be checked for a variety of disallowed characters and strings
+as well as having an approved file extension as specified by the
+DeniedAssetFileExtensions and AssetFileExtensions configuration directives. 
+The file does not have to exist on the filesystem to validate the filename.
+
+=item * data
+
+The value of the C<data> key is expected to be a scalar variable containing 
+binary data (presumably an image).  This data is checked (via
+C<MT::Image::has_html_signature>) for HTML-ish content within the first 1K of 
+the data. Image files (in particular) that contain embedded HTML or JavaScript 
+are a known vector for an IE 6 and 7 content-sniffing vulnerability.
+
 =head2 $app->uri_params(%param)
 
 A utility method that assembles the query portion of a URI, taking

lib/MT/Core.pm

@@ -552,6 +552,20 @@ BEGIN {
             'DefaultTemplateSet'   => { default => 'DePoClean_theme' },
 
             'AssetFileTypes' => { type => 'HASH' },
+            'AssetFileExtensions' => {
+                                   type    => 'ARRAY',
+                                   default => undef,
+            },
+            'DeniedAssetFileExtensions' => {
+                type    => 'ARRAY',
+                default => [ qw(
+                      ascx asis asp aspx bat cfc cfm cgi cmd com cpl dll
+                      exe htaccess html? inc jhtml js jsb jsp mht(ml)?
+                      msi php[s\d]? phtml? pif pl pwml py reg scr
+                      sh shtml? vbs vxd
+                      )
+                ],
+            },
 
             'FastCGIMaxTime'     => { default => 60 * 60 },    # 1 hour
             'FastCGIMaxRequests' => { default => 1000 },       # 1000 requests

t/04-config.t

@@ -12,7 +12,7 @@ use MT::Test;
 use Cwd;
 use File::Spec;
 use File::Temp qw( tempfile );
-use Test::More tests => 16;
+use Test::More tests => 17;
 
 use MT;
 use MT::ConfigMgr;
@@ -29,6 +29,8 @@ Database $db_dir/mt.db
 ObjectDriver DBI::SQLite 
 AltTemplate foo bar
 AltTemplate baz quux
+DeniedAssetFileExtensions DEFAULT
+DeniedAssetFileExtensions doc
 CFG
 close $fh;
 
@@ -57,6 +59,31 @@ is( @paths,                    2,       'paths=2' );
 is( ( $cfg->AltTemplate )[0], 'foo bar',  'foo bar' );
 is( ( $cfg->AltTemplate )[1], 'baz quux', 'baz quux' );
 
+## Test that multiple settings (DeniedAssetFileExtensions) work plus the
+## DEFAULT value for adding default elements
+subtest "DeniedAssetFileExtensions ARRAY and DEFAULT value" => sub {
+
+    plan tests => 2,
+
+    my $default = [qw(
+        ascx asis asp aspx bat cfc cfm cgi cmd com cpl dll
+        exe htaccess html? inc jhtml js jsb jsp mht(ml)?
+        msi php[s\d]? phtml? pif pl pwml py reg scr
+        sh shtml? vbs vxd
+    )];
+
+    is( $cfg->type('DeniedAssetFileExtensions'), 'ARRAY',
+        'DeniedAssetFileExtensions=ARRAY' );
+
+    my $exts         = $cfg->DeniedAssetFileExtensions;
+    my $default_plus = [ @$default, 'doc' ];
+
+    is_deeply( $exts, $default_plus, 
+                'DeniedAssetFileExtensions with DEFAULT' )
+        || diag explain $exts;
+};
+
+
 ## Test bug in early version of ConfigMgr where space was not
 ## stripped from the ends of values
 is( $cfg->ObjectDriver, 'DBI::SQLite', 'ObjectDriver=SQLite' );