News:

You may pay someone to create your store, or you visit our seminar and become a professional yourself with the silver certification

Main Menu

XSS bug in views/cart/view.json.php

Started by mb000000, December 17, 2013, 16:46:30 PM

Previous topic - Next topic

mb000000

Hi all,

The astute amongst you will see I'm having the "joys" of PCI compliance testing.... (except, now I've pointed it out, everyone can be astute ;-)  )

Anyway, one thing that has come out of it is a cross site scripting (XSS) problem with Itemid.

In VM2.0.22c (and I've just checked 2.0.26), views/cart/view.json.php contains at around line 50 (in both quoted versions):
private function prepareContinueLink() {
// Get a continue link
$menuid = JRequest::getVar('Itemid','');
if(!empty($menuid)){


This should be:
private function prepareContinueLink() {
// Get a continue link
$menuid = JRequest::getInt('Itemid','');
if(!empty($menuid)){


Itemid should only ever be numeric, so forcing the get to return a "validated" (by Joomla) int does seem reasonable.

Mark

AH

Regards
A

Joomla 3.10.11
php 8.0

mb000000

I see I was a little too early in my proposed fix.

Expanding the original (unchanged) code:
private function prepareContinueLink() {
// Get a continue link
$menuid = JRequest::getVar('Itemid','');
if(!empty($menuid)){
$menuid = '&Itemid='.$menuid;
echo '$menuid';
}


I suggest the change actually needs to be:
private function prepareContinueLink() {
// Get a continue link
$menuid = JRequest::getInt('Itemid','');
if(!empty($menuid)){
$menuid = '&Itemid='.$menuid;
echo '$menuid';
} else $menuid = '';


Otherwise what happens is that later in the function $menuid is used effectively as a string.  When getVar is used, if it is "empty" then '' (an empty string) is returned, whereas with getInt, it is 0 and so the later string uses append a zero where it then breaks some URIs (oops).

@Hutson - BTW - just about everywhere else in Joomla uses getInt for Itemid, rather than getVar

Sorry about the missed side-effect

Mark

Milbo

I wonder why we have the itemId there anyway. Should be done by joomla, no?
Should I fix your bug, please support the VirtueMart project and become a member
______________________________________
Extensions approved by the core team: http://extensions.virtuemart.net/

mb000000

I think Itemid is in there to set the "correct" menu "selector" once the shopper goes back to the category view.  Without it, the menu "selector" will be unknown or potentially random.

Mark

AH

Regards
A

Joomla 3.10.11
php 8.0