VirtueMart Forum

Virtuemart 3.4.2 ~ 3.6.2.10159

The file /administrator/components/com_virtuemart/models/ratings.php contains protection for onclick, onload and more...

$value = (string)preg_replace('#on[a-z](.+?)\)#si','',$value);//replace start of script onclick() onload()...

This protection removes all words that contain "on" and has brackets afterwards. Specifically everything after "on" to the brackets inclusive.
The problem is that this protection creates many problems.

Here are 2 examples:
User writes: "I was the only one to write a review (with rating and comment). So I have a question."
And the following is published: "I was the . So I have a question."

User writes: "A good configuration (in my opinion) is desirable."
And the following is published: "A good c is desirable."

I have now found a foolish method to avoid this and at the same time not to lose the protection of "on...". But the decision is temporary and I will wait for the team to give an opinion.

If You make this in two steps.

1. First remove white space.
2. check for "on("

If On( is not found then OK else some restoring before deleting the on( construct or rejecting this text totally because it is injecting bad code !

Jörgen @ Kreativ Fotografi

Yes, Jörgen, but this file is in the core and it is good to be permanently changed by the team.

By the way, I don't think that just "on(" will be enough.
Because it can be onclick or onload and then something in brackets. That is why such protection has been made. But maybe it should be better to avoid the problems described above.

This is of course academical right now, but search for the pattern according to how js interpretets the code a you have your answer. Your pattern search is to wide and takes in a lot of other valid constructs. I am not good at using preg and do not have the time to dig deeper into this, check js syntax and you will have your answer.

Jörgen @ Kreativ Fotografi

Thank you Jörgen for your reply.

Unfortunately, this search for a model is not mine but Max's. So I'm signaling what's not ok for the team to keep in mind.

As I wrote in the first post, I found a temporary and stupid solution that works for now. Now I'll just wait for the team to find the right solution if they want.

yes .. a bit of misunderstanding going on here  .. the code that Ventsi is referring to is core VM ..
But it is catching too much..

what was your temp solution V?

Quote from: GJC Web Design on October 08, 2019, 15:12:36 PM
what was your temp solution V?

I warned the method was stupid. So don't laugh. :D

I replace the line:
$value = (string)preg_replace('#on[a-z](.+?)\)#si','',$value);//replace start of script onclick() onload()...

With:
if (preg_match('#on[a-z](.+?)\)#si', $value)) {
$banned = array('/on/', '/On/');
$replace = array('оn', 'Оn'); // Cyrillic 'o' and 'O'
$value = preg_replace($banned, $replace, $value);
}

In practice, under the same condition, I do not delete everything, but replace the letters "o" and "O" with the same in Cyrillic.
This way, the script cannot be executed, while at the same time the information remains.
True, not good for search engine indexing, but I prefer to have a meaningful review rather than a cropped one. Because when the review gets cut, customers are not happy. At the same time, indexing also makes no sense.

Unfortunately, this is the only solution I could think of right now.

See my last answer here http://forum.virtuemart.net/index.php?topic=143614

Quote from: Studio 42 on October 08, 2019, 17:36:03 PM
As i know HTML is not tolerated by default. So using core Joomla filter or PHP should remove any javascript and dont need this stupid line.
Eg.

Code Select Expand

$value = filter_var($value , FILTER_SANITIZE_STRING);
See https://www.w3schools.com/php/phptryit.asp?filename=tryphp_func_sanitize_string (https://www.w3schools.com/php/phptryit.asp?filename=tryphp_func_sanitize_string)

Yes, Patrick, this is probably the right way.
Then maybe this line should also be dropped:

$value = preg_replace('@<[\/\!]*?[^<>]*?>@si','',$data['comment']);//remove all html tags

Interestingly, this code (FILTER_SANITIZE_STRING) is used in several places in Virtuemart. I don't know why it's not used in reviews. There may be some reason.

However, Max has to make a decision, because as I wrote earlier, this file is in the core.

We changed the filters long time ago, and let the old stuff in there. So a simple vRequest::getWord should be correct then.

So just try

// no HTML TAGS but permit all alphabet
/*$value = preg_replace('@<[\/\!]*?[^<>]*?>@si','',$data['comment']);//remove all html tags
$value = (string)preg_replace('#on[a-z](.+?)\)#si','',$value);//replace start of script onclick() onload()...
$value = trim(str_replace('"', ' ', $value),"'") ;
$data['comment'] = (string)preg_replace('#^\'#si','',$value);//replace ' at start
$data['comment'] = nl2br($data['comment']);  // keep returns*/

$data['comment'] = trim(vRequest::getWord('data'));
[code]

How behave new lines? I mean, I wonder, getWord uses also FILTER_FLAG_STRIP_LOW

Okey I tested it, we need this
[code]
$data['comment'] = vRequest::filter($data['comment'],FILTER_SANITIZE_STRING, array());
$data['comment'] = ShopfunctionsF::vmSubstr($data['comment'], 0, VmConfig::get('vm_reviews_maximum_comment_length', 2000)) ;
$data['comment'] = nl2br($data['comment']);


Please test

Quote from: Milbo on October 08, 2019, 21:03:04 PM
Okey I tested it, we need this

Code Select Expand


$data['comment'] = vRequest::filter($data['comment'],FILTER_SANITIZE_STRING, array());
$data['comment'] = ShopfunctionsF::vmSubstr($data['comment'], 0, VmConfig::get('vm_reviews_maximum_comment_length', 2000)) ;
$data['comment'] = nl2br($data['comment']);


Please test

No, Max. This code does not work. Anything can be published.

If the following is written:
Welcome <script> alert(\"Hi virtuemart\")</script>
publish everything.

But if I put the top of your code:
$data['comment'] = 'Welcome <script> alert(\"Hi virtuemart\")</script>';
publish: Welcome alert(\"Hi virtuemart\")


And please change vm_reviews_maximum_comment_length with reviews_maximum_comment_length.

Why using a virtuemart function if native PHP function is easier to use ?
Try
         $data['comment'] = filter_var($data['comment'] , FILTER_SANITIZE_STRING);
         $data['comment'] = ShopfunctionsF::vmSubstr($data['comment'], 0, VmConfig::get('reviews_maximum_comment_length', 2000)) ;
         $data['comment'] = nl2br($data['comment']);

Patrick, no change. I also tried this before writing. The same effect I described above.

Please note in the post above where I described that if the text is inserted into the php file then it works.
For example, if you put this above: $data['comment'] = 'Welcome <script> alert(\"Hi virtuemart\")</script>';

Which means the FILTER_SANITIZE_STRING works, but obviously there is a problem with the text that is assigned to the $data['comment'] when writing a review.

I tested the changes to the file (ratings.php) - revision 10172, which are actually the same ones Max wrote about.

In my opinion this does not work. Does not remove any codes from the review.

Also, vm_reviews_maximum_comment_length has not yet been changed to reviews_maximum_comment_length. Until that changes, the maximum number of characters selected by the administration will not work.

Have you try to dump the $data to check what is inside the $data['comment'] or with vmdebug ?

Yes of course.

There is no change before and after the FILTER_SANITIZE_STRING. Absolutely no difference:

Array
(
    [vote] => 5
    [comment] => Welcome <script> alert(\"Hi virtuemart\")</script>
    ......
)


But if I add the following:
$data['comment'] = 'Welcome <script> alert(\"Hi virtuemart\")</script>';

before:
$data['comment'] = vRequest::filter($data['comment'],FILTER_SANITIZE_STRING, array());

The result after the FILTER_SANITIZE_STRING is:

Array
(
    [vote] => 5
    [comment] => Welcome  alert(\"Hi virtuemart\")
    ......
)


i think that you have do a mistake.
I tried this and filter is working :


// no HTML TAGS but permit all alphabet
$value = filter_var($data['comment'] , FILTER_SANITIZE_STRING);
/* $value = preg_replace('@<[\/\!]*?[^<>]*?>@si','',$data['comment']);//remove all html tags
$value = (string)preg_replace('#on[a-z](.+?)\)#si','',$value);//replace start of script onclick() onload()... */
$value = trim(str_replace('"', ' ', $value),"'") ;
$data['comment'] = (string)preg_replace('#^\'#si','',$value);//replace ' at start
$data['comment'] = nl2br($data['comment']);  // keep returns

Patrick, the last file change was made by Max in revision 10172. The purpose is to fix the old code.
http://dev.virtuemart.net/projects/virtuemart/repository/revisions/10172
I test with it and it doesn't work.

Your code doesn't work either.
Just put this in a review:
Welcome <script> alert(\"Hi virtuemart\")</script>
and post it. There is no code cleanup.

Then put the same text here and test:
https://www.w3schools.com/php/phptryit.asp?filename=tryphp_func_sanitize_string

Can you share what text you are testing?

I have done the test with input :
"test<script>my script</script>"
Result was "test my script"

Very strange. It doesn't work for me.
In all variants (yours or Max's) it doesn't change the text.

To see that it got changed, you must open the source in a new tab. Then you will see that for example the < is replaced against &#60

I understand now. I was expecting it to disappear from the text.  :)

Would you also correct the vm_reviews_maximum_comment_length, please.
Must be reviews_maximum_comment_length.

Thank you.

Please test http://dev.virtuemart.net/attachments/1204/com_virtuemart.3.6.3.10179_package_or_extract.zip