Security Fixes: Please Check My Methods

[pippinsplugins]

Hey everyone, I've spent the last couple of hours making some significant security improvements to the way that EDD protects download files.

In the latest stable version, EDD stored all files in /wp-content/uploads/edd/{year}/{month}/, and in each of these folders was a .htaccess file that had a single option to prevent directory browsing. It did not prevent direct access to the file, meaning that any user who knew the direct file URL could get any file they wanted for free.

I've now improved this system in a couple of ways:

Previously I noted that sometimes folders would fail to get their .htaccess file created. There is now a function in includes/upload-functions.php called edd_create_protection_files() that loops through every directory in /wp-content/uploads/edd/ and creates a .htaccess file and a blank index.php file in each. This edd_create_protection_files() function runs on admin_init, but only if the transient (set to an expiration of 31 days) doesn't exist. This method ensures that all .htaccess files and index.php files are generated the first time after updating.

The function to create the files looks like this:

<?php
function edd_create_protection_files() {

    if( false === get_transient( 'edd_check_protection_files' ) ) {
        $wp_upload_dir = wp_upload_dir();
        $upload_path = $wp_upload_dir['basedir'] . '/edd';
        $folders = edd_scan_folders( $upload_path );
        foreach( $folders as $folder ) {
            // create or replace .htaccess file
            $contents = @file_get_contents( $folder . '.htaccess' );
            if( strpos( $contents, 'Deny from all' ) === false || ! $contents ) {
                $rules = 'Order Deny,Allow' . PHP_EOL . 'Deny from all';
                file_put_contents( $folder . '.htaccess', $rules );
            }

            if( !file_exists( $folder . 'index.php' ) ) {
                file_put_contents( $folder . 'index.php', '<?php' . PHP_EOL . '// silence is golden' );
            }
        }
        // only have this run the first time. This is just to create .htaccess files in existing folders
        set_transient( 'edd_check_protection_files', true, 2678400 );
    }
}
add_action('admin_init', 'edd_create_protection_files');

Previously, there was a problem with some servers not allowing files to download due to some server configuration that I couldn't figure out. My hacky (and insecure) way for getting around that was to add a constant called EDD_READ_FILE_MODE that basically caused the plugin to simply send the download file via header('location: urltofile.ext');. One of the many problems with this method is that the only way it would work is if files were either stored remotely, or files had no .htaccess protection on them at all.

Because of the new .htaccess files that I've added via the function above, the EDD_READ_FILE_MODE constant will no longer work. I've updated includes/process-download.php to convert all local file URLs to absolute paths, which should render the need for EDD_READ_FILE_MODE obsolete.

The main question I have here (excuse all the rambling) is this: are there further improvements that can be made to this system to ensure a couple of things:

• large file downloads work
• files cannot be browsed or accessed directly
• all normal server configs are supported.

It will probably help for you to see the code that processes the download file, so here it is:

<?php
function edd_process_download() {
    if(isset($_GET['download']) && isset($_GET['email']) && isset($_GET['file'])) {
        $download = urldecode($_GET['download']);
        $key = urldecode($_GET['download_key']);
        $email = rawurldecode($_GET['email']);
        $file_key = urldecode($_GET['file']);
        $expire = urldecode(base64_decode($_GET['expire']));

        $payment = edd_verify_download_link($download, $key, $email, $expire, $file_key);

         // defaulting this to true for now because the method below doesn't work well
        $has_access = true;
        //$has_access = ( edd_logged_in_only() && is_user_logged_in() ) || !edd_logged_in_only() ? true : false;
        if($payment && $has_access) {

            do_action('edd_process_verified_download', $download, $email);;

            // payment has been verified, setup the download
            $download_files = get_post_meta($download, 'edd_download_files', true);

            $requested_file = apply_filters('edd_requested_file', $download_files[$file_key]['file'] );

            $user_info = array();
            $user_info['email'] = $email;
            if(is_user_logged_in()) {
                global $user_ID;
                $user_data = get_userdata($user_ID);
                $user_info['id'] = $user_ID;
                $user_info['name'] = $user_data->display_name;
            }

            edd_record_download_in_log($download, $file_key, $user_info, edd_get_ip(), date('Y-m-d H:i:s'));

            $file_extension = edd_get_file_extension($requested_file);

            switch ($file_extension) :
                case "pdf": $ctype  = "application/pdf"; break;
                case "exe": $ctype  = "application/octet-stream"; break;
                case "zip": $ctype  = "application/zip"; break;
                case "doc": $ctype  = "application/msword"; break;
                case "xls": $ctype  = "application/vnd.ms-excel"; break;
                case "ppt": $ctype  = "application/vnd.ms-powerpoint"; break;
                case "gif": $ctype  = "image/gif"; break;
                case "png": $ctype  = "image/png"; break;
                case "jpe": $ctype  = "image/jpg"; break;
                case "jpeg": $ctype = "image/jpg"; break;
                case "jpg": $ctype  = "image/jpg"; break;
                case 'mp3': $ctype  = "audio/mpeg"; break;
                case 'wav': $ctype  = "audio/x-wav"; break;
                case 'mpeg': $ctype = "video/mpeg"; break;
                case 'mpg': $ctype  = "video/mpeg"; break;
                case 'mpe': $ctype  = "video/mpeg"; break;
                case 'mov': $ctype  = "video/quicktime"; break;
                case 'avi': $ctype  = "video/x-msvideo"; break;
                default: $ctype     = "application/force-download";
            endswitch;

            if( !ini_get('safe_mode') ){ 
                set_time_limit(0);
            }
            if( function_exists('get_magic_quotes_runtime') && get_magic_quotes_runtime() ) {
                set_magic_quotes_runtime(0);
            }
            header("Pragma: no-cache");
            header("Expires: 0");
            header("Cache-Control: must-revalidate, post-check=0, pre-check=0");
            header("Robots: none");
            header("Content-Type: " . $ctype . "");
            header("Content-Description: File Transfer");   
            header("Content-Disposition: attachment; filename=\"" . apply_filters('edd_requested_file_name', basename($requested_file) ) . "\";");
            header("Content-Transfer-Encoding: binary");

            if( strpos( $requested_file, home_url() ) !== false) {
                // local file
                $upload_dir = wp_upload_dir();

                $requested_file = str_replace( $upload_dir['baseurl'], $upload_dir['basedir'], $requested_file);    

                $requested_file = realpath( $requested_file );

                header("Content-Length: " . @requested_filesize( $requested_file ) );

                $requested_file = @fopen( $requested_file, "rb" );
                while( !feof( $requested_file ) ) {
                    print( @fread( $requested_file, 1024*8 ) );
                    ob_flush();
                    flush();
                }

            } else {
                // this is a remote file 
                header("Location: " . $requested_file);
            }

            exit;

        } else {
            wp_die(__('You do not have permission to download this file', 'edd'), __('Purchase Verification Failed', 'edd'));
        }
        exit;
    }
}
add_action('init', 'edd_process_download', 100);

@MadeByMike @sksmatt @elliott-stocks @sunnyratilal @wpsmith

[pippinsplugins]

All related commits:

• https://github.com/pippinsplugins/Easy-Digital-Downloads/commit/5997657d2563dc0247881afb24f1f93103c1dc84
• https://github.com/pippinsplugins/Easy-Digital-Downloads/commit/c90033f81fe22992b51ef2351c32172edb0f55c6

[johnkary]

You can link to commits within your repository by just typing the first few characters in the SHA :)

5997657 c90033f

[pippinsplugins]

Damn! I've been trying to figure out how to do that forever!

[sunnyratilal]

This is all looking really good instead of going through each folder and creating a .htaccess file, would a top-level rule in the edd folder be better. I'm not too good with .htaccess but maybe adding a RewriteBase directive which can be dynamically generated using PHP and then the deny to all completely?

[Larceniii]

What about those of us on Nginx? Just no protection?

+Jason

On Thu, Aug 16, 2012 at 9:38 PM, Sunny Ratilal notifications@github.comwrote:

This is all looking really good instead of going through each folder and creating a .htaccess file, would a top-level rule in the edd folder be better. I'm not too good with .htaccess but maybe adding a RewriteBase directive which can be dynamically generated using PHP and then the deny to all completely?

— Reply to this email directly or view it on GitHubhttps://github.com/pippinsplugins/Easy-Digital-Downloads/issues/260#issuecomment-7805573.

[pippinsplugins]

I actually had completely forgotten about nginx not supporting htaccess . . . Do you know of a better solution that would support both?

[sunnyratilal]

Maybe running a little PHP snippet ($_SERVER['SERVER_SOFTWARE']) to determine whether server is Apache or nginx and then creating a nginx.conf file if it's nginx which contains this code # nginx configuration deny all;

This is the equivalent of Apache's Order Deny,Allow Deny from All

[Larceniii]

Sunny, Nginx won't simply read nginx.conf files all over the place, they have to be located or linked to in the main conf file.

Also, that's not a recommend way to setup nginx anyway. Nginx specificly ignores files with rewrite rules in public folders because it's a security risk.

Ask anyone who's had an exploit on their server used to spew .htaccess files everywhere they send all their google referrer traffic to russian forums!

It's beyond me why anyone woulh configure nginx to make it more insecure. About half or more of the example configs on blogs that show how to setup nginx for wordpress do it in a way that leaves your server wide open for attack. It's imperative that you follow the guidelines in the wordpress codex for nginx configuration.'

In other words, creating nginx.conf files all over the place won't do either.

Here is a better option for both Apache AND Nginx Apache http://codeutopia.net/blog/2009/03/06/sending-files-better-apache-mod_xsendfile-and-php/ Nginx http://wiki.nginx.org/XSendfile

You will just need to do a test, or option to ask which server the plugin is using, and pick one of the methods of sending the protected file. This sends the files with the server, and not php, but is completely out of the document root.

Yea, it's more work, but I think it's worth looking into to be more secure and done correctly.

Great work so far Pippin!

+Jason

On Fri, Aug 17, 2012 at 10:09 AM, Sunny Ratilal notifications@github.comwrote:

Maybe running a little PHP snippet ($_SERVER['SERVER_SOFTWARE']) to determine whether server is Apache or nginx and then creating a nginx.conffile if it's nginx which contains this code

nginx configuration

deny all;

This is the equivalent of Apache's Order Deny,Allow Deny from All

— Reply to this email directly or view it on GitHubhttps://github.com/pippinsplugins/Easy-Digital-Downloads/issues/260#issuecomment-7825498.

[MadeByMike]

I've never installed WordPress on Nginx, so please excuse me if this is wrong. I wondered, "What does WordPress do when it detects Nginx?" As best I can tell (and I haven't looked too thoroughly) it doesn't detect Nginx at all.

Am I right from my research that Nginx users must manually configure the nginx.conf. In which case is what's good enough for WordPress good enough for EDD?

And let's be honest Nginx users will configure it manually anyway :)- just cause they can. So do we need to bother about this at all?

Also sunnyratilal's comment about a top level rule sounds sensible.

And why the index.php files? to stop directory browsing? Why not Options -Indexes in the htaccess. It they have a setup that overrides this leave it to the user to work out.

[sunnyratilal]

@MadeByMike I'm the same too - never used nginx before but after reading this http://codex.wordpress.org/Nginx I'm getting the idea that nginx needs to be configured manually. You're right in that WP doesn't detect nginx.

I'm starting to agree with you in that we need to stop bothering with this. Maybe we could add a message on the plugin activation hook that tells nginx users to configure their servers properly?

[pippinsplugins]

@Larceniii - xsendfile is great and I'd love to use it, but I cant, as it's not a standard apache module. Because this is an open source plugin that is widely used (and will continue to grow), it must support the standard installations. We can't require that users install / configure a non-standard apache module.

@MadeByMike - The blank index.php files are to help prevent directory browsing. I know they are not really needed when using the .htacces files, but I also don't feel that a little redundancy is a bad thing. I will also freely admit that I do NOT know htaccess rules well at all, so it's possible that there really is no reason whatsoever for the redundancy.

Everyone above: as @MadeByMike said, nginx is not natively supported by WordPress, so until it is, I do not personally feel that it needs to be worried about too much at this time. Once it is more widely adopted and is natively supported in WordPress (even if only minimally), then I will be concerned with supporting it. Users who are willing to use nginx don't mind doing a little extra anyhow, so we don't need to worry about normal every day users getting irritated because it's not supported.

I'd be happy with adding notes to the documentation about nginx support.

[Larceniii]

Most users, lets be honest, don't know how to configure nginx. It's like some kind of black magic voodoo, but they somehow think apache is easier, or more stable. I've been happily using it for three years now, and in reality, it's far easier than apache.

That aside, @pippinsplugins is right about the blank index.php files, on Nginx if directory indexing is turned on then it's great to have a fall back. It's good practice and wordpress does this as well, so I would "keep the status quo" in that regards.

I think it's on by default in Nginx, but I'm not certian. It would be a next extra option to turn on or off down the road, esp if that would be the standard way on nginx over .htaccess files.

And unless you have a plugin that checks for apache, wordpress never checks. It just comes with a drop in .htaccess in the root of the download.

@ https://github.com/MadeByMikeSunny I've NEVER had to configure wordpress to use nginx. I've never had to configure it for apache either. That statement just doesn't make sense to me. In all servers, you gotta drop the wordpress files in a publicly shared folder, that was configured for or by apache or nginx.

The nittygritty of my point of view is simply this. Nginx is so easy to configure, 99% of people who set it up, have no idea what they are doing, and they're not going to mess with the config. It's not a tweakers playground! If a simple switch to add support for nginx is available, (and present on all nginx configs) then I thought it would be worth mentioning.

My final thoughts: If Easy means only works correctly on apache, then it should just be disclosed. I've installed plugins before to later find .htaccess files everywhere, and the plugin not work correctly. Random .htaccess files are just another security hole, not a fix on your server anyway.

+Jason

On Fri, Aug 17, 2012 at 11:29 PM, Pippin Williamson < notifications@github.com> wrote:

@Larceniii https://github.com/Larceniii - xsendfile is great and I'd love to use it, but I cant, as it's not a standard apache module. Because this is an open source plugin that is widely used (and will continue to grow), it must support the standard installations. We can't require that users install / configure a non-standard apache module.

@MadeByMike https://github.com/MadeByMike - The blank index.php files are to help prevent directory browsing. I know they are not really needed when using the .htacces files, but I also don't feel that a little redundancy is a bad thing. I will also freely admit that I do NOT know htaccess rules well at all, so it's possible that there really is no reason whatsoever for the redundancy.

Everyone above: as @MadeByMike https://github.com/MadeByMike said, nginx is not natively supported by WordPress, so until it is, I do not personally feel that it needs to be worried about too much at this time. Once it is more widely adopted and is natively supported in WordPress (even if only minimally), then I will be concerned with supporting it. Users who are willing to use nginx don't mind doing a little extra anyhow, so we don't need to worry about normal every day users getting irritated because it's not supported.

I'd be happy with adding notes to the documentation about nginx support.

— Reply to this email directly or view it on GitHubhttps://github.com/pippinsplugins/Easy-Digital-Downloads/issues/260#issuecomment-7841670.

[sksmatt]

+1 to @sunnyratilal a single .htaccess file on the top edd/ folder should be enough ( as it will work for all sub-directories )

Rest looks superb.

[pippinsplugins]

Thanks everyone.

After hearing what you guys said, ad doing some searching on my own, I definitely confirmed that only one .htaccess file is neede for apache. I never knew this, but am very glad I ask now.

In regards to nginx, here's what I'd like to do: push this update out to fix the immediate issue at hand for the users that reported it and then do a follow up patch that adds support for nginx.

I spent some time talking to @JohnPBloch while at WordCamp Grand Rapids and he had some really good ideas for providing backwards compatibility for both systems.

[pippinsplugins]

The final code / methods that are going to be used can be seen 7e5cbe6f2eda491d87c05ba4d8bed0c34ef06610.

I'll work on adding in nginx support soon.