Security review

[johnz0r]

Invalid payment validation (critical)

standardreturn.php

/mercadopago/controllers/front/standardreturn.php:68 validateOrder()

An user may validate an abritary order without paying it.

Step to reproduce exploit :

1/ Create an order, enter delivery address, validate until asked for payment method, do not chose a payment method.

2/ fetch the following page http://prestashop1609.local/module/mercadopago/standardreturn?external_reference=11&collection_status=approved

(external_reference is cart_id and incremental so it can be easily guessed).

We advise you to create a shared secret key between the shop and the API, so the API can sign the parameters with the key, and the shop can verify the request using the same key. This way, it won't be possible to forge a request to this page without knowing the secret.

Morever, validateOrder use $cart->getOrderTotal(true, 3) ($total) as $amount_paid to validate the order. It would be better the API pass the real paid price via a signed parameters in the callback.

custompayment.php

/mercadopago/controllers/front/custompayment.php:71 validateOrder() $total (may not be exploitable)

In this case too, validateOrder use $cart->getOrderTotal(true, 3) ($total) as $amount_paid to validate the order. It would be better, if possible, to get the real paid price from the return value of $mercadopago->execPayment($_POST) ($response)

However, I don't think we have a security issue here because the price sent to API via $mercadopago->execPayment(); is not editable by the user (it sends the quantity and the price of each items of $cart->getProducts() ), but I prefer to warn you about this.

XSS

/mercadopago/views/templates/admin/settings.tpl:244 to 257 replace |escape:'htmlall' with |escape:'javascript'

/mercadopago/views/templates/hook/checkout.tpl :454 $iframe_width add |escape:'javascript' :455 $iframe_height add |escape:'javascript'

/mercadopago/views/templates/hook/payment_top.tpl:31 replace |escape:'htmlall' with |escape:'javascript'

[ricardobrito]

Hi,

All issues were fixed. Please review them. Any doubts feel free to ask.

One curiosity: How can an attacker make a XSS when there is no input for it? The only input is via admin page.

[Quetzacoalt91]

@johnz0r, can you check if the issue in standardreturn.php is now fixed ? Thanks

[johnz0r]

Yes it's fixed, standardreturn.php does not use user-provided values anymore but ask for confirmation via the API server-side.

Also, the module is now using paid amount provided by the API for validateOrder which improve security too.

@ricardobrito We assume we have a potential XSS each time a variable is incorrectly escaped in smarty template, we don't check the origin of each variable deeply (if it's user controlled, coming from the database, or admin input only etc.) because it would takes too much time to follow the flow in code, database etc. for each variable. So yes, potential XSS may not lead to a vulnerability each time.

[Quetzacoalt91]

Thanks @johnz0r