Signing URLs for improved security

[bradyvercher]

Pippin asked me to go ahead and open an issue here for discussion on a new implementation for improving the security of URLs that I built out as an add-on at the moment. Considering the implications of something like this, it'd be ideal to roll it into the core plugin, but may require breaking some currently valid URLs.

The basic premise is that URLs can be signed using a unique string, in this case wp_salt(), and when requested, the token can be checked to ensure none of the variables in the query string have been tampered with. This allows for passing non-sensitive data through the URL in the clear and being certain that it hasn't been changed. It essentially allows for more secure and concise URLs.

Code

My implementation is available here for anyone wanting to try it out and offer feedback on how it could be improved or integrated: https://github.com/bradyvercher/EDD-Signed-URLs

The first three functions simply hook into EDD to modify and check the URLs, while the last three functions provide the API for signing URLs.

The concept will work with any URL structure, so my method of condensing everything into a colon delimited eddfile query arg isn't required (and that arg could even be renamed with a simple filter if desired).

Breakage

As far as potentially breaking changes, existing URLs that haven't expired would quit working with this solution. It might be possible to run things side by side for a short while, but the current method would need to be deprecated at some point.

This would also decrease the reliance on using the download_key to look up a payment, instead using actual payment IDs, which might make things a little easier to work with. In that case, the current API utilizing the download_key might need to be augmented, or a new solution could be run side by side. I'm not sure if anyone is using the existing functions directly or not.

Improvements

In any case, we're opening this up to see if anyone has ideas on implementing a solution like this without breaking anything. Feedback about improvements, suggestions on the default URL structure (is it worth trying to simplify?), and increasing security is welcome.

Feel free to correct me if I got anything wrong, Pippin.

[pippinsplugins]

Great, thanks Brady. Once I finish getting some of the remaining 1.4 issues off the table, I'll start looking into this in more depth.

[pippinsplugins]

V1.4.4 is too close to put this in, so pushing back to 1.5.

[pippinsplugins]

I'm going to push this one back a little more again (sorry). Definitely want to do it, but need to focus on taxes for the rest of the 1.5 dev cycle.

[sunnyratilal]

@pippinsplugins Is the idea to integrate the entire plugin into Core?

[pippinsplugins]

Yes, more or less.

[sunnyratilal]

Punt to 1.7?

[pippinsplugins]

Not yet. I'm going to play with it today and tomorrow.

On Thursday, May 30, 2013, Sunny Ratilal wrote:

Punt to 1.7?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/603#issuecomment-18674719
.

[chriscct7]

Punt.

[pippinsplugins]

This will be a priority for 1.7 but it has to be really thoroughly tested with extensions (especially S3), and there simply isn't time for this release.

[pippinsplugins]

Punting to 1.8.

[pippinsplugins]

Punting again to 2.1 due to lack of time.

[cklosowski]

Getting started on this, but we have one slight issue we'll have to overcome, and that's the usage of base64_encode part of the reason for updating to use this is to avoid this method as explained in issue #1930, some hosts delete our files for it.

Going to start looking at some alternatives, if anyone has any suggestions, I'm open.

$secret = apply_filters( 'eddsurl_get_token_secret', rawurlencode( base64_encode( wp_salt() ) ) );

[cklosowski]

Anyone opposed to just using a php hash() and making the algorithm a filter? By default I'm thinking a sha256 hash should be fine for the token secret.

@pippinsplugins @ghost1227

Thinking:

$hash = apply_filters( 'edd_url_token_secret_hash', 'sha256' );
$secret = apply_filters( 'edd_url_get_token_secret', rawurlencode( hash( $hash, wp_salt() ) ) );

[bradyvercher]

Hi @cklosowski, it's been awhile since I looked into this, but there are a few things that can be changed about that original implementation. The secret isn't ever exposed, so there's really no need to run it through base64_encode() or rawurlencode().

I'm not sure if there's anything on the EDD side that's changed, but I tweaked those signing methods some time ago, so this might provide a better base to work from: https://gist.github.com/bradyvercher/8327a2bf74dc2d88cbfc

[pippinsplugins]

Definitely no need for base64 anything

On Monday, December 22, 2014, Brady Vercher notifications@github.com
wrote:

Hi @cklosowski https://github.com/cklosowski, it's been awhile since I
looked into this, but there are a few things that can be changed about that
original implementation. The secret isn't ever exposed, so there's really
no need to run it through base64_encode() or rawurlencode().

I'm not sure if there's anything on the EDD side that's changed, but I
tweaked those signing methods some time ago, so this might provide a better
base to work from:
https://gist.github.com/bradyvercher/8327a2bf74dc2d88cbfc

—
Reply to this email directly or view it on GitHub
#603 (comment)
.

[cklosowski]

@ghost1227 @pippinsplugins @bradyvercher You guys mind testing this out a bit with branch issue/603? I think I covered most cases (as mentioned in the commit log details).

I'm not counting on this being the only commit. This is a super complex thing to make sure we're backwards compatible with.

[pippinsplugins]

I can test tomorrow.

[pippinsplugins]

New download links validate properly 👍

[pippinsplugins]

And old-style links work properly as well 👍

[pippinsplugins]

edd_validate_url_token() will need to have a filter on it in order to allow additional options for validating tokens. For example (using the inline one), if someone wanted to restrict file download to the IP used to make the purchase, they would need to filter the response from edd_validate_url_token().

[pippinsplugins]

@ghost1227 PDF Stamper will need to be updated to not call edd_verify_download_link() anymore.

[pippinsplugins]

The biggest issue I'm running into so far is plugins that called edd_verify_download_link() themselves during process-download.php. Those plugins (such as PDF Stamper) are failing due to some issues with populating the $_GET super globals.

[pippinsplugins]

A change needs to be made to PDF Stamper @ghost1227: https://trello.com/c/dsmMHdfl/291-pdf-stamper

[pippinsplugins]

PDF Vouchers breaks. When you try to download a voucher, it errors: https://cloudup.com/cSrVWkRS6gs

[pippinsplugins]

PDF Vouchers breaks because our new signed URLs are not adding the custom variables that PDF Vouchers appends to download URLs.

New URL:

http://edd/index.php?eddfile=2231%3A514%3A0&ttl=1424167604&token=4d0a86fb8f8be40cf9927222076a5ced

Old URL:

http://edd/index.php?download_key=7211cce2df06b572890934752a4c0896&email=pippin%40pippinsplugins.com&file=edd_vou_pdf&price_id=1&download_id=514&expire=MTQyNDE2NzcyOQ%3D%3D&edd_voucher=1

[pippinsplugins]

PDF Vouchers adds the extra args with the edd_download_file_url_args filter:

add_filter( 'edd_download_file_url_args', array( $this, 'edd_vou_order_download_file_url_args' ) );
...
    /**
     * Display Download Voucher Link
     * 
     * Handles to display download voucher link for user
     * 
     * @package Easy Digital Downloads - Voucher Extension
     * @since 1.0.0
     */
    public function edd_vou_order_download_file_url_args( $params ) {

        global $edd_receipt_args, $edd_payment_id;

        $payment    = get_post( $edd_receipt_args['id'] );
        $order_id   = isset( $payment->ID ) ? $payment->ID : '';        

        if( empty( $order_id ) ) {

            $order_id = $edd_payment_id;
        }
        $downloadid     = isset( $params['download_id'] ) ? $params['download_id'] : '';

        // If product is variable then take variavle option id
        $price_id = !empty($params['price_id']) ? $params['price_id'] : '';

        // If product is variable then making product key
        if( !empty($price_id) ) {
            $product_data_id = $downloadid . '_' . $price_id;
        } else {
            $product_data_id = $downloadid;
        }

        if( $this->model->edd_vou_check_enable_voucher( $downloadid ) ) { //check enable voucher

            $vou_codes_key  = array();
            $vou_codes_key  = $this->model->edd_vou_get_multi_voucher_key( $order_id, $params['download_id'], $price_id );

            if( !is_admin() && isset( $params['download_key'] ) && isset( $params['file'] ) && !empty( $params['file'] )
                && ( $params['file'] == 'edd_vou_pdf' || in_array( $params['file'], $vou_codes_key ) ) ) { // Check front side & edd voucher pdf

                $voucher_key = $product_data_id;
                $meta_query = array(
                                    array( 
                                                'key'   => '_edd_payment_purchase_key',
                                                'value' => $params['download_key']
                                            )
                                );
                $payment_data   = edd_get_payments( array( 'meta_query' => $meta_query ) );
                $payment        = isset( $payment_data['0'] ) ? $payment_data['0'] : array();

                //get voucher order details
                $ordervoudata = $this->model->edd_vou_get_post_meta_ordered( $payment->ID );
                $downloadvoudata = isset( $ordervoudata[$voucher_key] ) ? $ordervoudata[$voucher_key] : array();

                if( !empty( $downloadvoudata ) ) { // Check voucher download data

                    $params['edd_voucher'] = '1';
                }
            }
        } // end enaable check

        return $params;
    }

[pippinsplugins]

We've broken how the edd_download_file_url_args filter works. It used to filter the final URL that is returned, now it just filters the parameters sent to edd_get_download_token().

[pippinsplugins]

We need to make sure that any custom arguments registered through edd_download_file_url_args still get appended to URLs, while also making sure we don't append all of our default arguments, since that would largely defeat the entire purpose of signed URLs.

I've just about got this working by looking at the difference between the starting parameters and the final parameters (after it goes through the filter) and then just appending any custom ones:

// Ensure all custom args registered with extensions through edd_download_file_url_args get added to the URL, but without adding all the old args
$args = array_merge( $args, array_diff_key( $params, $old_args ) );

[pippinsplugins]

EDD Partial Downloader is going to fail completely due to how they're overwriting our download process: https://github.com/EhsaanF/edd-partial-downloader/blob/master/edd-partial-downloader.php#L38

I'll get in touch with them and ask them to update it.

[pippinsplugins]

Extensions that have to be updated to be compatible (ones that are checked off have been updated):

• PDF Stamper
• PDF Vouchers
• Partial downloads

[pippinsplugins]

Aside from the three extensions above, every extension on the known-to-be-affected list works properly.

[pippinsplugins]

Good news: this has been live on pippinsplugins.com for 3 days and not a single ticket about customers being unable to download files.

[pippinsplugins]

This is also live on affiliatewp.com and has not presented any issues yet.

[pippinsplugins]

I'm putting in on the EDD site now.

[pippinsplugins]

So issues so far! Closing for now.