Spotted this in passing:

/**
 * Return an md5 checksummed path.
 *
 * This allows us to create paths that's resistant to tampering. By appending
 * an md5 sum of the path and drupal_private_key, and checking the sum in
 * requests, it's not possible to change a path element. This means that we
 * can put data in the URL, and be sure it's not changed.
 */
 // TODO: use core token system instead?
function _quickpay_md5_path($path) {
  // The rand is to ensure failure if drupal_private_key haven't been set.
  return $path . '/' . md5($path . variable_get('drupal_private_key', rand()));
}

Could drupal_get_token() not be used here instead? This looks like the sort of thing it's used for, and it would simplify the code.

Comments

joachim created an issue. See original summary.

xen’s picture

xen commented

No, because it's used for the callback URL (which is called by Quickpays server), and drupal_get_token() hashes the session_id in.

Wouldn't mind if this is refactored to use drupal_hmac_base64() instead of md5 though. Certainly it should use drupal_get_private_key(). And probably drupal_get_hash_salt() now we're at it.

joachim’s picture

joachim commented

> drupal_get_token() hashes the session_id in.

Oh, of course -- it's the Quickpay server that's calling that, so not the user's session. Wasn't thinking, sorry!

joachim’s picture

joachim commented

There was one other thing:

> // The rand is to ensure failure if drupal_private_key haven't been set.

I don't think it's possible for Drupal to not have this set -- it's created on site install.

joachim’s picture

joachim commented

Actually, do we still need this, now that in Quickpay's v10 API, the callback POST gets a checksum based on the API key?

xen’s picture

xen commented

The rand() is not needed. If we use drupal_get_private_key() it will ensure that the site has a key, generating it if necessary.

The old API is already MD5ing the payload. But it's not that we're checking, we're ensuring that someone haven't tapered with the path. It's an extra defence as the path contains the module we call back. In theory, two modules could be using the same order id for payments, and without this Quickpay could be fooled into calling the wrong one. Very theoretical, but with this safety we don't have to consider the possibilities.

joachim’s picture

joachim commented

Ok. I'll leave this as postponed, and copy the code you suggested from the other issue to here:

  return $path . '/' . drupal_hmac_base64($path, drupal_get_private_key() . drupal_get_hash_salt());

  • Xen committed c9a9537 on 7.x-2.x 
    Issue #2684865 by Xen: Update path hashing
xen’s picture

xen commented
Version: 7.x-1.x-dev » 7.x-2.x-dev
Status: Postponed » Fixed

Fixed in -dev.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.