I did a quick security review of the uc_realex module.

    $output .= '<h3>'. t('Reason provided: ') . $message .'</h3>';

$message is coming straight from $_POST and should be sanitized before being displayed to prevent XSS.

  $timestamp  = $_POST['TIMESTAMP'];
  $result     = $_POST['RESULT'];
  $orderid    = $_POST['ORDER_ID'];
  $message    = $_POST['MESSAGE'];
  $authcode   = $_POST['AUTHCODE'];
  $pasref     = $_POST['PASREF'];
  $realexsha1 = $_POST['SHA1HASH'];
  $batch      = $_POST['BATCHID'];

looks suspicious. Besides $message (see above), these don't seem to be displayed anywhere else in the module but it might be a good measure to check_plain them unless you're expecting some HTML in them... just in case ubercart displays these without sanitizing them (which it should not in theory).

This is not a bug, but just a remark:

  watchdog('uc_realex', t('Receiving new order notification for order !order_id.', array('!order_id' => check_plain($_POST['ORDER_ID']))));

the @ placeholder takes care of the check_plain, I would rewrite this as

  watchdog('uc_realex', t('Receiving new order notification for order @order_id.', array('@order_id' => $_POST['ORDER_ID'])));

(there are more than one occurence in uc_realex.module)

See http://drupal.org/writing-secure-code for more tips.

CommentFileSizeAuthor
#1 667464.patch1.99 KBstella

Comments

stella’s picture

Status: Active » Needs review
StatusFileSize
new1.99 KB
alanburke’s picture

Status: Needs review » Fixed

Fixed in HEAD.
Thanks guys!

Status: Fixed » Closed (fixed)

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