302 replies to this topic

[mharrisr]

eww, on Mar 2 2008, 08:45 AM, said:

a lot of people have complained about this.  it isn't a hack.  it's simply the fact of somebody getting to checkout_confirmation & then manually typing checkout_process.php and getting the success page.

if i offered downloads, i would offer an alternative method of supplying the download.  say, after the payment has been manually verified.

Will the contribution "PayPal_Shopping_Cart_IPN" help resolve this issue? Please reply.

[chris23]

Nice work!

Another one for the excludes list - redirect.php.

Product urls to external sites break with SecurityPro on but the passed url is checked by redirect.php against the legitimate url for the product so redirect.php can be safely excluded.

HTH

Chris

[FWR Media]

chris23, on Mar 3 2008, 11:58 AM, said:

Nice work!

Another one for the excludes list - redirect.php.

Product urls to external sites break with SecurityPro on but the passed url is checked by redirect.php against the legitimate url for the product so redirect.php can be safely excluded.

HTH

Chris

Thanks for responding Chris

This was actually known but imo that function should not exist. I just don't like the idea of any script that allows a redirect to offsite. Redirect.php has been exploited in the past.

[chris23]

FWR Media, on Mar 3 2008, 12:12 PM, said:

This was actually known but imo that function should not exist. I just don't like the idea of any script that allows a redirect to offsite. Redirect.php has been exploited in the past.

Couldn't agree more, though I have several clients reliant on the url redirecting. I think Harald's default query check should be sufficient to stop an exploit here (pls correct me if I'm wrong)

I agree with you about getting rid of % from the allowed characters. As you were asking for ideas, rather than get rid of it completely, would a new regexp of allowed encoded characters be any better, say allowing %20 for space but explicitly excluding %3C and %3E (<> definite no-nos)?

I appreciate this would be difficult in view of the different payment modules but just getting rid of the worst suspects would be an improvement IMHO.

Again, thanks for your contrib

Cheers

Chris

[FWR Media]

chris23, on Mar 3 2008, 01:13 PM, said:

I agree with you about getting rid of % from the allowed characters. As you were asking for ideas, rather than get rid of it completely, would a new regexp of allowed encoded characters be any better, say allowing %20 for space but explicitly excluding %3C and %3E (<> definite no-nos)?

This is very much in mind, however %3C isn't enough you need %253E as well because of double encoding.

str_replace(array('<', '%3C', '%253C', '>', '%3E', '%253E'), array('', '', '', '', '', ''), $get_var);

and then there's octals and then there's hexadecimal .. I'm still looking at these.

Quote

I appreciate this would be difficult in view of the different payment modules but just getting rid of the worst suspects would be an improvement IMHO.

This wont really be worthy of its name until it DOES start breaking more scripts. The idea should be that it sanitizes as much as possible and only "trusted" scripts get to be allowed through as an exception.

The % should definately be removed from the preg_replace otherwise all you need do (well nearly) is urlencode a querystring to bypass cleansing. That or urldecode before cleansing.

[FWR Media]

Tougher security.php

A new catalog/includes/functions/security.php has been added to contributions.

The changes are minimal but it makes a big difference.

Changes:
% character removed
$get_var is urldecoded before the preg_replace strips bad characters.

No changes are required of the contribution just directly replace catalog/includes/functions/security.php

Please note: Unlike other contributions, this one will break more things the better it gets.

Odd sounding I know but it is the case. Now that this is urldecoding and is missing the % character a lot more scripts, payment modules etc will fail .. this is a GOOD thing. By all means exclude your broken payment module from cleansing by security Pro .. however, I wouldn't advise doing the same for a less important contribution .. why not see where it's stopped by this script and change it so that it doesn't use bad characters in the querystring.

Most important: Test fully your important systems after adding this . .especially payment/shipping etc.

As usual I need feedback.

Thanks to perfectpassion for continuing to help me test this alongside his PROTX payment module (which I use myself by the way).


Please try to think along the following lines:-

If Security Pro breaks a feature/function

1) Try to remove the need for the feature to use bad characters (Stay here as long as you can)

2) Exclude the file from cleansing only if you really have to. (Should be critical operations only like payment where you have no control over the incoming querystring)

3) NEVER alter the preg_replace or other functions in security.php unless improving it (making it tougher).

Note: With this contribution the usual "Yaaay I got it to work by removing XXX from security.php" = you broke your security!

[Dennisra]

Thanks for the update.

How, exactly do I exclude a file from cleansing?
Thanks!

FWR Media, on Mar 4 2008, 02:07 AM, said:

Tougher security.php.
By all means exclude your broken payment module from cleansing by security Pro
2) Exclude the file from cleansing only if you really have to.

[FWR Media]

Dennisra, on Mar 4 2008, 06:14 PM, said:

Thanks for the update.

How, exactly do I exclude a file from cleansing?
Thanks!

If you have the latest version there is an admin setting for this.

[Dennisra]

FWR Media, on Mar 4 2008, 12:31 PM, said:

If you have the latest version there is an admin setting for this.

Best I can tell I have the latest version. The contribution has two files available available for download:
SecurityPro1.0.zip which I have installed and security.zip and I updated the
catalog/includes/functions/security.php file today.

In admin under configuration I have FWR Security Pro.The only option there is to enable true or false. I am sure I am missing something. Was there another database update that isn't in the last secuirty.zip file? Any assistance would be appreciated.

[FWR Media]

Dennisra, on Mar 5 2008, 04:03 AM, said:

Best I can tell I have the latest version. The contribution has two files available available for download:
SecurityPro1.0.zip which I have installed and security.zip and I updated the
catalog/includes/functions/security.php file today.

In admin under configuration I have FWR Security Pro.The only option there is to enable true or false. I am sure I am missing something. Was there another database update that isn't in the last secuirty.zip file? Any assistance would be appreciated.

That is not the latest version .. see contributions.

[Dennisra]

FWR Media, on Mar 5 2008, 06:34 AM, said:

That is not the latest version .. see contributions.

I give up. May I ask for a link to the newsest version? This is all I can find http://addons.oscommerce.com/info/5752 and as you said that's not the latest version.
Thanks for your help.

[FWR Media]

Dennisra, on Mar 5 2008, 01:41 PM, said:

I give up. May I ask for a link to the newsest version? This is all I can find http://addons.oscommerce.com/info/5752 and as you said that's not the latest version.
Thanks for your help.

My apologies it is not up as a contribution yet see post 6

http://forums.oscommerce.com/index.php?s=&...t&p=1210572

[Dennisra]

FWR Media, on Mar 5 2008, 10:07 AM, said:

My apologies it is not up as a contribution yet see post 6

http://forums.oscommerce.com/index.php?s=&...t&p=1210572

I totally missed that post. Thank you!

[FWR Media]

The 1.0.2 version is now uploaded to contributions.

[valerif]

thanks for the contributions
this is what i was looking for

i installed it and see this problem
Fatal error: Call to undefined function: fwr_clean_global() in /.../includes/application_top.php on line 88

if i disable the line from the application top code it works fine

perhaps you could have a suggestion what this could be

thanks
valeri

[FWR Media]

valerif, on Mar 6 2008, 05:24 PM, said:

thanks for the contributions
this is what i was looking for

i installed it and see this problem
Fatal error: Call to undefined function: fwr_clean_global() in /.../includes/application_top.php on line 88

if i disable the line from the application top code it works fine

perhaps you could have a suggestion what this could be

thanks
valeri

Sounds like you have an old catalog/includes/functions/security.php

The latest version has a function fwr_clean_global()

[valerif]

FWR Media, on Mar 6 2008, 05:45 PM, said:

Sounds like you have an old catalog/includes/functions/security.php

The latest version has a function fwr_clean_global()

yes you were right
the stramge thing is that i think i did download the new package
thanks for the help
valerif

[eww]

FWR Media,

I just noticed this contribution affected my newsletter contributions, where I allow people to remove themselves from lists.

It strips the @ from the:
unsubscribe.php?email=myemail@example.com

to fix this I edited security.php:

return preg_replace("/[^ {}a-zA-Z0-9_.-]/i", "", urldecode($get_var));

with:

return preg_replace("/[^ {}a-zA-Z0-9@_.-]/i", "", urldecode($get_var));

wondering if it's possible to incorporate this with a future version?

[eww]

also added this to my admin panel.

top if admin/includes/application_top.php:

  require('../includes/functions/security.php');

never know when you get an angsty employee that wants to deface your site before he quits

[FWR Media]

eww, on Mar 7 2008, 02:07 AM, said:

FWR Media,

I just noticed this contribution affected my newsletter contributions, where I allow people to remove themselves from lists.

It strips the @ from the:
unsubscribe.php?email=myemail@example.com

to fix this I edited security.php:

return preg_replace("/[^ {}a-zA-Z0-9_.-]/i", "", urldecode($get_var));

with:

return preg_replace("/[^ {}a-zA-Z0-9@_.-]/i", "", urldecode($get_var));

wondering if it's possible to incorporate this with a future version?

Changing the preg_replace is really the WORST possible thing to do, although having said that @ I can't see being any danger. I still think there is no reason for it to exist in the querystring. I would rather see the value passed as a replaceable character.

[url=http://mysite.com/index.php?querystring=rob-emailand-mysite.com]http://mysite.com/index.php?querystring=ro...land-mysite.com[/url]

then at the receiving end ..

str_replace('-emailand-', '@', $HTTP_GET_VARS['querystring']);

Edited by FWR Media, 07 March 2008 - 11:02 AM.