Storefront sanitization: Review/update handling for various 'standard' variables

[torvista]

As reported here: https://www.zen-cart.com/showthread.php?228293-popup-image-additional-sql-injection-attack&p=1383167

I've had this in place ever since with no recurrence of the logs. Should this be included?

[lat9]

I'll say "Yes". When I plug that URL into my zc158 test site (running PHP 8.1), I receive the following logs

[28-Jun-2022 08:48:13 America/New_York] Request URI: /zc158/index.php?main_page=popup_image_additional&pID=50+%27-6863+union+all+select+CONCAT(0x3a6f79753a,0x4244764877697569706b,0x3a70687a3a)%23&pic=0&products_image_large_additional=bmz_cache/madstad/md-info_01jpg.image.500x500.jpg, IP address: ::1
#0 C:\xampp\htdocs\zc158\includes\modules\pages\popup_image_additional\header_php.php(39): zen_debug_error_handler()
#1 C:\xampp\htdocs\zc158\index.php(35): require('C:\\xampp\\htdocs...')
--> PHP Warning: Undefined array key "products_image_large_additional" in C:\xampp\htdocs\zc158\includes\modules\pages\popup_image_additional\header_php.php on line 39.

[28-Jun-2022 08:48:13 America/New_York] Request URI: /zc158/index.php?main_page=popup_image_additional&pID=50+%27-6863+union+all+select+CONCAT(0x3a6f79753a,0x4244764877697569706b,0x3a70687a3a)%23&pic=0&products_image_large_additional=bmz_cache/madstad/md-info_01jpg.image.500x500.jpg, IP address: ::1
#0 [internal function]: zen_debug_error_handler()
#1 C:\xampp\htdocs\zc158\includes\modules\pages\popup_image_additional\header_php.php(39): stripslashes()
#2 C:\xampp\htdocs\zc158\index.php(35): require('C:\\xampp\\htdocs...')
--> PHP Deprecated: stripslashes(): Passing null to parameter #1 ($string) of type string is deprecated in C:\xampp\htdocs\zc158\includes\modules\pages\popup_image_additional\header_php.php on line 39.

For completeness, 'that URL' is

/index.php?main_page=popup_image_additional&pID=50+%27-6863+union+all+select+CONCAT(0x3a6f79753a,0x4244764877697569706b,0x3a70687a3a)%23&pic=0&products_image_large_additional=bmz_cache/madstad/md-info_01jpg.image.500x500.jpg

[torvista]

I'm surprised there is not something already in place to filter a dodgy pID?

[lat9]

There is, in /includes/init_includes/init_sanitize.php, but it's allowing non-numeric characters. That (obviously) needs some review.

[lat9]

Having reviewed `init_sanitize', here are the current sanitizations that (IMO) require some redo as they're all currently sanitized as

preg_replace('/[^\/0-9a-zA-Z_:@.-]/', '', $_GET[$key])

$saniGroup1 = [
    'action',                           //- Various
    'addr',                             //- Used by the "unsubscribe" page, an email address
    'alpha_filter_id',                  //- Set by /includes/modules/product_listing_alpha_sorter.php, [A-Z0-9]
    'alpha_filter',                     //- Not present
    'authcapt',                         //- Used in the admin inly
    'chapter',                          //- EZ-Pages, a 'toc_chapter', integer
    'cID',                              //- A "coupon_id", integer
    'currency',                         //- A currency definition
    'debug',
    'delete',                           //- address_book_process, the id to delete (integer)
    'dfrom',                            //- Various, a date-formatted value
    'disp_order',                       //- /includes/modules/listing_display_order (integer)
    'dto',                              //- Various, a date-formatted value
    'edit',                             //- address_book_process, the id to edit (integer)
    'faq_item',                         //- gv_faq (integer)
    'filter_id',                        //- Various index_filters (integer)
    'goback',                           //- gv_redeem, gv_faq, if set is 'true'
    'goto',                             //- redirect, a banners_id (integer)
    'gv_no',                            //- A gift-voucher number [0-9a-z]
    'id',                               //- An EZ-page or download ID
    'inc_subcat',                       //- Searches (integer)
    'language',                         //- A language string
    'markflow',                         //- Paypal processing (integer)
    'music_genre_id',                   //- Music products (integer)
    'nocache',                          //- init_db_config_read and square (mixed)
    'notify',                           //- Various (mixed)
    'number_of_uploads',                //- Various (integer)
    'order_id',                         //- Various, an order_id (integer)
    'order',                            //- download page, an order_id (integer)
    'override',                         //- ot_total.php, remove method, admin only
    'page',                             //- Various, page number (integer)
    'pfrom',                            //- Searches, price-from (float)
    'pid',                              //- A products_id or uprid
    'pID',                              //- images/additional_images, a products_id (integer)
    'pos',                              //- EZ-pages (page) page, either 'h' or 'v'
    'product_id',                       //- shopping-cart removal, either a products_id (integer) or uprid
    'products_image_large_additional',  //- A filename
    'products_tax_class_id',            //- Not used as a $_GET or $_POST variable
    'pto',                              //- Searches, price-to (float)
    'record_company_id',                //- Music products (integer)
    'referer',                          //- PayPal payment method, specifically 'paypal'
    'reviews_id',                       //- Various, a reviews_id (integer)
    'search_in_description',            //- Searches indicator (integer)
    'set_session_login',                //- init_customer_auth (not set or 'true')
    'token',                            //- paypalwpp/paypaldp, [0-9A-Z.-]
    'tx',                               //- paypal/paypay_functions
    'type',                             //- Paypal
    'zenid',                            //- [a-z0-9]
    $zenSessionId                       //- [a-z0-9]
];

[lat9]

I'm pretty sure that this has been asked before, but given that there are no $_GET arrays in a standard ZC install and the $_GET arrays are one of the normal 'script-kiddy' things, is it time to disallow all $_GET arrays during the storefront sanitization?

[proseLA]

I'm pretty sure that this has been asked before, but given that there are no $_GET arrays in a standard ZC install and the $_GET arrays are one of the normal 'script-kiddy' things, is it time to disallow all $_GET arrays during the storefront sanitization?

no.

i think we should have an extra script for devs/store owners to do their own sanitizing. or some sort of auto-loading similar to auto-loading of observers.

i think it's a valid question, but lets not inflict more pain on those of us maintaining some of these stores.

[proseLA]

there are no $_GET arrays in a standard ZC install....

4934

[lat9]

there are no $_GET arrays in a standard ZC install....

4934

I don't understand; that's simply changing how an invalid manufacturers_id is handled.

[lat9]

I'm pretty sure that this has been asked before, but given that there are no $_GET arrays in a standard ZC install and the $_GET arrays are one of the normal 'script-kiddy' things, is it time to disallow all $_GET arrays during the storefront sanitization?

no.

i think we should have an extra script for devs/store owners to do their own sanitizing. or some sort of auto-loading similar to auto-loading of observers.

i think it's a valid question, but lets not inflict more pain on those of us maintaining some of these stores.

OK. I'll note that there's already a notification from the storefront init_sanitize.php to aid in that ... as well as answering my previous question:

foreach ($_GET as $getvar) {
    if (is_array($getvar)) {
        $site_array_override = false;
        $zco_notifier->notify('NOTIFY_INIT_SANITIZE_GET_VAR_CHECK', ['getvarname' => $getvar], $site_array_override);
        if ($site_array_override === false) {
            zen_redirect(zen_href_link(FILENAME_DEFAULT));
        }
    }
}

Essentially, without store-specific observer-customization, no $_GET arrays are allowed.

[scottcwilson]

Possible confusion over wording? I think was meant was, "There are no $_GET variables which are themselves arrays." Correct?

[proseLA]

scott is correct.

my apologies.

[torvista]

In the case of a custom storefront GET variable, I assume it needs to be added to one of the $saniGroup arrays manually or an override file created, there's no other method?

[drbyte]

In the case of a custom storefront GET variable, I assume it needs to be added to one of the $saniGroup arrays manually or an override file created, there's no other method?

For bespoke code, just sanitize it in your code wherever you use it. Even for plugins, since your plugin code is what uses that variable, and your plugin knows what to do with it, just sanitize it where you use it.

[torvista]

Ok, I had the idea that the sanitation was based on a whitelist, only allowing what was specifically listed.

[scottcwilson]

If some nice person would like to update the documentation, it kind of trails off at the end ...

https://docs.zen-cart.com/dev/code/admin_sanitization/