PrestaShopCorp / bpostshm

3 stars 5 forks source link

security review #17

Closed johnz0r closed 9 years ago

johnz0r commented 9 years ago

Unsafe unserialize call

/AdminOrdersBpost.php:1079 $value = unserialize($value); Replace serialize/unserialize call with json_encode/json_decode

Unsafe include

/controllers/admin/AdminOrdersBpost.php:308 /AdminOrdersBpost.php:254

check $iso_code ^[a-z]{2}$

Unsafe SQL

/classes/Service.php:1272 $iso_list array_map + pSQL /classes/lib/Ps/OrderBpost.php:153 $reference add pSQL /classes/lib/Ps/OrderBpost.php:221, 252, 306 intval()

/AdminOrdersBpost.php:98 $id_bpost_carriers array_map + intval /controllers/admin/AdminOrdersBpost.php:105 $id_bpost_carriers array_map + intval

Potential XSS

Escape using Tools::safeOutput()

/AdminOrdersBpost.php :1086 $value[0] (isCleanHtml may not protect a variable without html displayed in an attribute) :1088 $value[1] :934,1026,1029,1184,1197,1203 $current_index :937,995,1027,1030,1185,1199,1205 $token :571 $option (please recheck because it may break something if $option contains html)

/controllers/admin/AdminOrdersBpost.php :668 $option (please recheck because it may break something if $option contains html)

Escape with smarty |escape

/views/templates/front/lightbox-point-list.tpl :13 postcode :15 city

/views/templates/front/lightbox-at-247.tpl :47 remove 'javascript' argument of escape call :52 gender->id gender->name :59 firstname :64 lastname :69 street :74 number :79 postal_code :84 locality :89 birthday :94 email :99 mobile_phone :106 $_language['lang'] $_language['name']

/views/templates/admin/settings.tpl :24 error :87 account_id_account :99 account_passphrase :111 account_api_url :208 opt['title']

/views/templates/admin/orders_bpost/helpers/list/list_action_option.tpl :8 href|urldecode :9 disabled

/views/templates/admin/orders_bpost/helpers/list/list_content.tpl most of the variables aren't escaped, please escape them all

example line 3 : id="tr_{$idcategory}{$tr.$identifier}_{if isset($tr.position['position'])}{$tr.position['position']}{else}0{/if} escape id_category, $tr.$identifier and $tr.position['position']

/views/templates/admin/orders_bpost/helpers/list/list_footer.tpl :14 :61 :83 $list_id $value :88 :156 :157 :190 $reload_href in comment : "All smarty escape attempts FAIL!!" did you try escape:'javascript' ? :230 replace escape with escape:'javascript' :231 replace escape with escape:'javascript' :460 escape:'javascript'

/views/templates/admin/orders_bpost/helpers/list/list_footer14.tpl remove if deprecated file, otherwise, escape all call to <?php echo with Tools::safeOutput

/views/templates/admin/orders_bpost/helpers/list/list_header.tpl 13 escape:'javascript' 14 escape:'javascript' 23 escape:'javascript' 24 escape:'javascript' 32 escape:'javascript' 33 escape:'javascript' 66 135 $currentIndex $identifier 137 $currentIndex $identifier 173 $key 193 params.filter_key key params.width 243 escape:'javascript' 250 to 262 replace all escape with escape:'javascript' 290 293 301 307 309 330 to 381 replace all escape with escape:'javascript' 385 389 396 420 identifier list_id 423 identifier list_id 468 475 480 to 502 replace all escape with escape:'javascript' 506 510 516

Stigmi commented 9 years ago

Comments below for any issues discovered


/AdminOrdersBpost.php

lines :916 to :1124 function displayListHeader is an override of AdminTab parent controller and is 96% Prestashop 1.4.11 code, minus Globals, exceeding lines + validated variable names.

If 1.4 AdminTab controller is unsafe or open to XSS attacks, changing this function won't make any of the other back office tabs (Ex. Orders) any safer so the whole of version 1.4 must be withdrawn, and likewise we can drop support.

When a safe version 1.4.12 is available, we will be happy to alter the rest of this function to match.


Unsafe unserialize call

see above /AdminOrdersBpost.php:1079 $value = unserialize($value); cannot json_decode what may have been serialized elsewhere in Prestashop 1.4 !! Replace serialize/unserialize call with json_encode/json_decode

Unsafe include

/controllers/admin/AdminOrdersBpost.php:308 done /AdminOrdersBpost.php:254 done

check $iso_code ^[a-z]{2}$ done

Unsafe SQL

/classes/Service.php:1272 $iso_list array_map + pSQL done /classes/lib/Ps/OrderBpost.php:153 $reference add pSQL done /classes/lib/Ps/OrderBpost.php:221, 252, 306 intval() done as casts

/AdminOrdersBpost.php:98 $id_bpost_carriers array_map + intval done /controllers/admin/AdminOrdersBpost.php:105 $id_bpost_carriers array_map + intval done

Potential XSS

Escape using Tools::safeOutput()

/AdminOrdersBpost.php :1086 $value[0](isCleanHtml may not protect a variable without html displayed in an attribute) :1088 $value[1] :934,1026,1029,1184,1197,1203 $currentindex Tools::safeOutput results in FAIL :937,995,1027,1030,1185,1199,1205 $token is now only $this->token $this being AdminTab_ :571 $option (please recheck because it may break something if $option contains html) done

/controllers/admin/AdminOrdersBpost.php :668 $option (please recheck because it may break something if $option contains html) done

Escape with smarty |escape

/views/templates/front/lightbox-point-list.tpl :13 postcode done :15 city done

/views/templates/front/lightbox-at-247.tpl :47 remove 'javascript' argument of escape call done :52 gender->id gender->name done :59 firstname done :64 lastname done :69 street done :74 number done :79 postal_code done :84 locality done :89 birthday done :94 email done :99 mobile_phone done :106 $_language['lang'] $_language['name'] done

/views/templates/admin/settings.tpl :24 error done :87 account_id_account done :99 account_passphrase done :111 account_api_url done :208 opt['title'] done

/views/templates/admin/orders_bpost/helpers/list/list_action_option.tpl :8 href|urldecode _FAIL_ :9 disabled done

/views/templates/admin/orders_bpost/helpers/list/list_content.tpl most of the variables aren't escaped, please escape them all

example line 3 : id="tr_{$idcategory}{$tr.$identifier}_{if isset($tr.position['position'])}{$tr.position['position']}{else}0{/if} escape id_category, $tr.$identifier and $tr.position['position']

done the example and some others: _there are no text inputs here, so if there are any left please be specific_

/views/templates/admin/orders_bpost/helpers/list/list_footer.tpl :14 done :61 done :83 $list_id $value done :88 done :156 done :157 done :190 $reload_href in comment : "All smarty escape attempts FAIL!!" did you try escape:'javascript' ? done :230 replace escape with escape:'javascript' done :231 replace escape with escape:'javascript' done :460 escape:'javascript' done

/views/templates/admin/orders_bpost/helpers/list/list_footer14.tpl remove if deprecated file, otherwise, escape all call to <?php echo with Tools::safeOutput 1.4 template: Very much an active file Tools::safeOutput results in FAIL same as :190 $reload_href above

/views/templates/admin/orders_bpost/helpers/list/list_header.tpl 13 escape:'javascript' done 14 escape:'javascript' done 23 escape:'javascript' done 24 escape:'javascript' done 32 escape:'javascript' done 33 escape:'javascript' done 66 done 135 $currentIndex $identifier done 137 $currentIndex $identifier done 173 $key' done 193 params.filter_key key params.width' done 243 escape:'javascript'' done 250 to 262 replace all escape with escape:'javascript'' done 290 done 293 done 301 done 307 done 309 done 330 to 381 replace all escape with escape:'javascript' done 385 done 389 done 396 done 420 identifier list_id done 423 identifier list_id done 468 done 475 done 480 to 502 replace all escape with escape:'javascript' done 506 done 510 done 516 done

Quetzacoalt91 commented 9 years ago

/views/templates/admin/orders_bpost/helpers/list/list_action_option.tpl :8 href|urldecode FAIL

So {$href|urldecode|escape} does not work ?

/views/templates/admin/orders_bpost/helpers/list/list_content.tpl most of the variables aren't escaped, please escape them all

done the example and some others: there are no text inputs here, so if there are any left please be specific

You escaped only the example. Even if there are no test inputs, the content displayed should be escaped

views/templates/admin/orders_bpost/helpers/list/list_header.tpl 135 $currentIndex $identifier done 137 $currentIndex $identifier done

Not done for $currentIndex

193 params.filter_key key params.width

Not done for params.width

386

{$title|end} --> {$title|end|escape} should also escape your value when we have an array

420 identifier list_id 423 identifier list_id

$identifier not done

469

{$params.value.0} should be escaped too

476

{$params.value.1} should be escaped too

511

{$option_display} is not escaped

Escape using Tools::safeOutput()

/AdminOrdersBpost.php :1086 $value[0](isCleanHtml may not protect a variable without html displayed in an attribute) :1088 $value[1]

I fixed its comment, which was hidden

:934,1026,1029,1184,1197,1203 $current_index Tools::safeOutput results in FAIL

Can you tell us more about how it failed ?

Stigmi commented 9 years ago

/views/templates/admin/orders_bpost/helpers/list/list_action_option.tpl :8 href|urldecode FAIL

stuck-a stuck-b

So {$href|urldecode|escape} does not work ? I'm afraid, Not


/views/templates/admin/orders_bpost/helpers/list/list_content.tpl most of the variables aren't escaped, please escape them all

done the example and some others: there are no text inputs here, so if there are any left please be specific

You escaped only the example. Even if there are no test inputs, the content displayed should be escaped

Should be ok now


views/templates/admin/orders_bpost/helpers/list/list_header.tpl 135 $currentIndex $identifier done 137 $currentIndex $identifier done

screen shot 2015-03-12 at 21 52 41 screen shot 2015-03-12 at 21 53 23 screen shot 2015-03-12 at 21 52 18

Not done for $currentIndex

Can't be done unless a "Working" escape is provided for all ($currentIndex / $current_index) etc.

There should be none of the below left now

193 params.filter_key key params.width

Not done for params.width

386

{$title|end} --> {$title|end|escape} should also escape your value when we have an array

420 identifier list_id 423 identifier list_id

$identifier not done

469

{$params.value.0} should be escaped too

476

{$params.value.1} should be escaped too

511

{$option_display} is not escaped


Escape using Tools::safeOutput()

/AdminOrdersBpost.php :1086 $value[0](isCleanHtml may not protect a variable without html displayed in an attribute) :1088 $value[1]

I fixed its comment, which was hidden

:934,1026,1029,1184,1197,1203 $current_index Tools::safeOutput results in FAIL

Can you tell us more about how it failed ?

Sure, Any header search or up/down sort results in navigating away from the admin page:

screen shot 2015-03-13 at 02 25 37

to:

screen shot 2015-03-13 at 02 26 07

Quetzacoalt91 commented 9 years ago

Okay, Thanks. We can now publish the module.