Page 1 of 2 12 LastLast
Results 1 to 10 of 13
  1. #1
    Join Date
    Sep 2005
    Location
    Waikato, New Zealand
    Posts
    1,539
    Plugin Contributions
    3

    Default Question about replacing [Get]s in addon code for 1.5

    Ok in this post regarding image handler 3 in the
    "Confirmed working mods in Zen Cart 1.5" thread

    Kuroi says

    Quote Originally Posted by kuroi View Post
    Clyde has addressed the page registration requirement, but our analysis of it has shown that it also has destructive GET actions - one of the potential security vulnerabilities addressed extensively in 1.5, and which are a tad more complex to deal with.

    Indeed that's going to be one of the challenges going forward in a PCI-certified world: understanding "it works" v "it's secure".
    What exactly does that mean, as when i look at the code the vast majority of the gets are in the php code and not available to the frontend user to manipulate, i am not doubting or dissing Kuroi in anyway, he knows way more than me, I just don't understand what is wrong with the GETs, i have attempted to read the PCI specs but well I can't make sense of them they seem vague in parts and obtuse in others.

    What should i be looking at replacing "Get"s with that would be PCI complant, If i was working on a mod, is it specific usages of the GET action?or is it the whole GET action that is the problem?
    Webzings Design
    Semi retired from Web Design

  2. #2
    Join Date
    Apr 2006
    Location
    London, UK
    Posts
    10,569
    Plugin Contributions
    25

    Default Re: Question about replacing [Get]s in addon code for 1.5

    The issue isn't GETs in general. It's limited to GETs that lead directly to a change to the database. These could theoretically be picked up and used out of context, in particular to bypass the protection added to Zen Cart to defend against Cross Site Request Forgery (CSRF) attacks.

    Examples might be a delete confirm box that relied on a button wrapped in a link that passed details of the thing to be deleted as a string of GET parameters only. Normally that link would only be generated when a user hits a delete button, but a similar one could be constructed and used during a CSRF attack.

    That's now prevented by checking a security token passed as a POST var against the one held in the session array. So those "destructive" GET actions have been converted into forms with the security token passed as a hidden variable (the zen_draw_form function automatically adds it). Usually the id of the thing being deleted is passed as a POST var too. Though that's not essential as long as the code forces at least one other piece of information to be present in the POST var before the database change can be executed.
    Kuroi Web Design and Development | Twitter

    (Questions answered in the forum only - so that any forum member can benefit - not by personal message)

  3. #3
    Join Date
    Apr 2006
    Location
    London, UK
    Posts
    10,569
    Plugin Contributions
    25

    Default Re: Question about replacing [Get]s in addon code for 1.5

    It's also worth noting that there are some forms in mods that only pass data via the GET array. These need to be encouraged to use the POST array too.
    Kuroi Web Design and Development | Twitter

    (Questions answered in the forum only - so that any forum member can benefit - not by personal message)

  4. #4
    Join Date
    Apr 2006
    Location
    London, UK
    Posts
    10,569
    Plugin Contributions
    25

    Default Re: Question about replacing [Get]s in addon code for 1.5

    Shortly before he so sadly passed away at the weekend, Clyde Jones and I were looking at the changes needed for his Links Manager. Clyde was a huge supporter of this community, and I'm sure that he would have been happy for me to use examples from our discussion for general benefit.

    The following examples are taken from the links.php file:

    Link Deletion

    Line 609 uses a form for the delete confirm, but does so with a form action containing all the data needed for the database deletion:
    PHP Code:
          $contents = array('form' => zen_draw_form('links'FILENAME_LINKSzen_get_all_get_params(array('lID''action')) . 'lID=' $lInfo->links_id '&action=deleteconfirm')); 
    This isn't good enough as that form action could be lifted out of the form and used as a simple URL plus parameters, thus bypassing the security token. Although there is some fallback code to try to prevent this, good practise dictates making at least one of the parameters passed (or in this case the only one), a POST variable:
    PHP Code:
          $contents = array('form' => zen_draw_form('links'FILENAME_LINKSzen_get_all_get_params(array('lID''action')) . '&action=deleteconfirm') . zen_draw_hidden_field('lID'$lInfo->links_id)); 
    Line 177 reads the now posted ID, so change from:
    PHP Code:
            $links_id zen_db_prepare_input($_GET['lID']); 
    to:
    PHP Code:
            $links_id zen_db_prepare_input($_POST['lID']); 
    Line 183 reconstructs the URL
    PHP Code:
            zen_redirect(zen_href_link(FILENAME_LINKSzen_get_all_get_params(array('lID''action'))); 
    and so needs to be changed to add the ID from POST for use when constructing the URL
    PHP Code:
            zen_redirect(zen_href_link(FILENAME_LINKSzen_get_all_get_params(array('action')).'&lID=' $_POST['lID']))); 
    Status Changes

    Changing approved/pending flag can also be done on a GET alone. So ...

    Lines 564-569, build the links that lead directly to database changes so:
    PHP Code:
          if ($links->fields['links_status'] == '2') {
            echo 
    zen_image(DIR_WS_IMAGES 'icon_status_green.gif''Approved'1010) . '&nbsp;&nbsp;<a href="' zen_href_link(FILENAME_LINKS'page=' $_GET['page'] . '&amp;lID=' $links->fields['links_id'] . '&amp;action=setflag&amp;flag=1') . '">' zen_image(DIR_WS_IMAGES 'icon_status_yellow_light.gif''Set Pending'1010) . '</a>';
          } else {
            echo 
    '<a href="' zen_href_link(FILENAME_LINKS'page=' $_GET['page'] . '&amp;lID=' $links->fields['links_id'] . '&amp;action=setflag&amp;flag=2') . '">' zen_image(DIR_WS_IMAGES 'icon_status_green_light.gif''Set Approved'1010) . '</a>&nbsp;&nbsp;' zen_image(DIR_WS_IMAGES 'icon_status_yellow.gif''Pending'1010);
          }
          
    ?> 
    needs to be converted to forms as follows:
    PHP Code:
          if ($links->fields['links_status'] == '2') {
            echo 
    zen_image(DIR_WS_IMAGES 'icon_status_green.gif''Approved'1010) . '&nbsp;&nbsp;';
            echo 
    zen_draw_form('links_status'FILENAME_LINKS'page=' $_GET['page'] . '&amp;action=setflag');
            echo 
    zen_draw_hidden_field('lID'$links->fields['links_id']);
            echo 
    zen_draw_hidden_field('flag',1);
            echo 
    '<input type="image" src="' DIR_WS_IMAGES 'icon_status_yellow_light.gif" title="Set Pending" />';
            echo 
    '</form>';
          } else {
            echo 
    zen_draw_form('links_status'FILENAME_LINKS'page=' $_GET['page'] . '&amp;action=setflag');
            echo 
    zen_draw_hidden_field('lID'$links->fields['links_id']);
            echo 
    zen_draw_hidden_field('flag',2);
            echo 
    '<input type="image" src="' DIR_WS_IMAGES 'icon_status_green_light.gif" title="Set Approved" />';
            echo 
    '</form>';
            echo 
    '&nbsp;&nbsp;' zen_image(DIR_WS_IMAGES 'icon_status_yellow.gif''Pending'1010);
          }
          
    ?> 
    Of particular note is the way that we have converted the linked image into an image type input field to both display the status and act as a submit button for the form.
    The monolingual image titles (naughty, naughty) have been not been fixed.

    Lines 28-29, receive the now posted request:
    PHP Code:
            if ( ($_GET['flag'] == '1') || ($_GET['flag'] == '2') ) {
              
    zen_set_links_status($_GET['lID'], $_GET['flag']); 
    and so change to:
    PHP Code:
            if ( ($_POST['flag'] == '1') || ($_POST['flag'] == '2') ) {
              
    zen_set_links_status($_POST['lID'], $_POST['flag']); 
    Line 34 reconstructs the URL:
    PHP Code:
            zen_redirect(zen_href_link(FILENAME_LINKS'page=' $_GET['page'] . '&amp;lID=' $_GET['lID'])); 
    and so needs to change to:
    PHP Code:
            zen_redirect(zen_href_link(FILENAME_LINKS'page=' $_GET['page'] . '&amp;lID=' $_POST['lID'])); 
    Kuroi Web Design and Development | Twitter

    (Questions answered in the forum only - so that any forum member can benefit - not by personal message)

  5. #5
    Join Date
    Sep 2005
    Location
    Waikato, New Zealand
    Posts
    1,539
    Plugin Contributions
    3

    Default Re: Question about replacing [Get]s in addon code for 1.5

    Cheers Kuroi, i am slowly assimilating this, thanks for taking the time out to explain it clearly
    Webzings Design
    Semi retired from Web Design

  6. #6
    Join Date
    Jan 2008
    Posts
    43
    Plugin Contributions
    0

    Default Re: Question about replacing [Get]s in addon code for 1.5

    Hi,

    Thanks for the explanation.

    I'm not fully grasping the idea behind this yet though.

    Can't someone just use cURL to use the POST variable using another server or even a server installed on their own computer, like Xampp?

    If so, is this just to make it more difficult, making it not possible to just lift a link from a GET form, instead of impossible?

  7. #7
    Join Date
    Apr 2006
    Location
    London, UK
    Posts
    10,569
    Plugin Contributions
    25

    Default Re: Question about replacing [Get]s in addon code for 1.5

    Quote Originally Posted by Diavire View Post
    Can't someone just use cURL to use the POST variable using another server or even a server installed on their own computer, like Xampp?
    The context here is not obtaining information from a different server, but submitting data from an admin page in a browser back to the site's own server.

    Quote Originally Posted by Diavire View Post
    is this just to make it more difficult, making it not possible to just lift a link from a GET form, instead of impossible?
    Not sure I understand the subtle differences between "not possible" and "impossible", so the best answer I can give is to re-iterate that this is to protect against Cross Site Request Forgery (CSRF) attacks that seek to exploit the trust the site has in an authenticated user's browser.
    Kuroi Web Design and Development | Twitter

    (Questions answered in the forum only - so that any forum member can benefit - not by personal message)

  8. #8
    Join Date
    Jan 2008
    Posts
    43
    Plugin Contributions
    0

    Default Re: Question about replacing [Get]s in addon code for 1.5

    I see, misunderstood the context then.

    In my reasoning the "not possible" referred to disabling the possibility to simply create an url with gets (by adding or changing entirely to a post), making the overall exploit more difficult, but still possibly (using a server to server approach like cURL).

    Since I misunderstood the context, the "not possible" and "impossible" thing don't make any sense whatsoever

    Sometimes you don't learn until you've asked some stupid questions, so thanks for explaining. :)

  9. #9
    Join Date
    Jan 2004
    Location
    N of San Antonio TX
    Posts
    9,103
    Plugin Contributions
    11

    Default Re: Question about replacing [Get]s in addon code for 1.5

    SO.... If my foggy brain is absorbing this, the following code only needs the $_GET on several lines changed to $_POST to meet standards. Yes or no?
    PHP Code:
    <?php
    /**
     * Module Template
     *
     * @package templateSystem
     * @copyright Copyright 2007 FUAL
     * @copyright Portions Copyright 2003-2005 Zen Cart Development Team
     * @copyright Portions Copyright 2003 osCommerce
     * @license http://www.zen-cart.com/license/2_0.txt GNU Public License V2.0
     * @version $Id: tpl_modules_main_product_image.php 2007-12-04 btyler $
     */
    ?>
    <?php 
    require(DIR_WS_MODULES zen_get_module_directory(FILENAME_MAIN_PRODUCT_IMAGE)); ?> 
    <div id="productMainImage" class="centeredContent back">
    <!-- bof Zen Slimbox v0.1 btyler 2007-12-04 -->
    <?php
    if( FUAL_SLIMBOX == 'true' || ZEN_LIGHTBOX_STATUS == 'true' ) {
        
    // Set the title
        
    if ( $current_page_base == 'product_reviews' ) {
            
    $fual_slimbox_title htmlentities($review->fields['products_name'],ENT_QUOTES);
        } else {
            
    $fual_slimbox_title htmlentities($products_name,ENT_QUOTES);
        }
        
    // Get the href for the large image
        
    $fual_slimbox_href zen_lightbox($products_image_large$fual_slimbox_titleLARGE_IMAGE_WIDTHLARGE_IMAGE_HEIGHT);
        
    $fual_slimbox_a '<a href="' $fual_slimbox_href '" rel="lightbox[gallery]" title="' $fual_slimbox_title '">';
        
    // Get the img element for this product.
        
    $fual_slimbox_image zen_image($products_image_medium$fual_slimbox_titleMEDIUM_IMAGE_WIDTHMEDIUM_IMAGE_HEIGHT); 

        
    // Test remote images to simulate slow loading for local development
        //$fual_slimbox_image = '<img src="http://demos.mootools.net/demos/DomReadyVS.Load/moo.png" width="150px" height="150px;" alt="test" />'; 
        
        // Note if you want to test a slow DOM load, then in /index.php add sleep(5); (in php brackets) just before the final </html>
        // This will make the DOM take an extra 5 seconds to load, which simulates dialup (what a cool feature)
        
        
    $fualSlimboxContent "";
        if( 
    ZEN_LIGHTBOX_STATUS == 'true' ) {
            
    $fualNervousSwitch 0;
        } else {
            
    $fualNervousSwitch FUAL_SLIMBOX_NERVOUS;
        }
        switch( 
    $fualNervousSwitch ) {
            case 
    2:
                
    $fualSlimboxContent .= '<div id="slimboxWrapper">';
                break;
            case 
    1:
                
    $fualSlimboxContent .=  '<div id="slimboxWrapper" style="display:block;">';
                break;
            case 
    0:
            default:
                
    $fualSlimboxContent .= '<div id="slimboxWrapper" style="display:block; visibility:visible;">';
        }
        
    $fualSlimboxContent .=  $fual_slimbox_a $fual_slimbox_image '</a>'
        
    // Putting the text link together with the image is nasty!
        
    $fualSlimboxContent .=  '<br class="clearBoth" />';
        
    $fualSlimboxContent .=  $fual_slimbox_a '<span class="imgLink">' TEXT_CLICK_TO_ENLARGE '</span></a>';
        
    $fualSlimboxContent .=  '</div>';
    ?>
        <script language="javascript" type="text/javascript"><!--
        document.write('<?php echo $fualSlimboxContent?>' );
        //--></script>
        <noscript>
        <?php
        
    // If they can't be bothered to get a decent browser or turn js on then they only deserve the default behaviour.
        
    echo '<a href="' zen_href_link(FILENAME_POPUP_IMAGE'pID=' $_GET['products_id']) . '" target="_blank">' zen_image($products_image_medium$products_nameMEDIUM_IMAGE_WIDTHMEDIUM_IMAGE_HEIGHT) . '<br /><span class="imgLink">' TEXT_CLICK_TO_ENLARGE '</span></a>';
    ?>
        </noscript>
    <?php         
    } else {
    ?>
    <!-- bof Zen Slimbox v0.1 btyler 2007-12-04 -->
        <script language="javascript" type="text/javascript"><!--
        document.write('<?php echo '<a href="javascript:popupWindow(\\\'' zen_href_link(FILENAME_POPUP_IMAGE'pID=' $_GET['products_id']) . '\\\')">' zen_image($products_image_mediumaddslashes($products_name), MEDIUM_IMAGE_WIDTHMEDIUM_IMAGE_HEIGHT) . '<br /><span class="imgLink">' TEXT_CLICK_TO_ENLARGE '</span></a>'?>');
        //--></script>
        <noscript>
        <?php
        
    echo '<a href="' zen_href_link(FILENAME_POPUP_IMAGE'pID=' $_GET['products_id']) . '" target="_blank">' zen_image($products_image_medium$products_nameMEDIUM_IMAGE_WIDTHMEDIUM_IMAGE_HEIGHT) . '<br /><span class="imgLink">' TEXT_CLICK_TO_ENLARGE '</span></a>';
        
    ?>
        </noscript>
    <?php ?>
    </div>
    Last edited by dbltoe; 18 Jan 2012 at 06:35 AM.

  10. #10
    Join Date
    Apr 2006
    Location
    London, UK
    Posts
    10,569
    Plugin Contributions
    25

    Default Re: Question about replacing [Get]s in addon code for 1.5

    Quote Originally Posted by dbltoe View Post
    SO.... If my foggy brain is absorbing this, the following code only needs the $_GET on several lines changed to $_POST to meet standards. Yes or no?
    No.

    I doubt that any changes are needed. They rarely (maybe never) will be on the store side of a site unless they relate to the checkout or the account area, as these are the only places where database changes are likely to occur.

    I recommend looking back specifically at post #2 earlier in this thread for an explanation of the limited circumstances when $_GET variables could create security vulnerabilities.
    Kuroi Web Design and Development | Twitter

    (Questions answered in the forum only - so that any forum member can benefit - not by personal message)

 

 
Page 1 of 2 12 LastLast

Similar Threads

  1. v151 question about installing Admin Keepalive Timer Addon
    By SilverHD in forum All Other Contributions/Addons
    Replies: 11
    Last Post: 30 Nov 2014, 11:08 PM
  2. configure.php question about addon-domains
    By Webskipper in forum Upgrading to 1.5.x
    Replies: 16
    Last Post: 4 Jun 2013, 08:24 PM
  3. quick question about overrides vs Image Handler2 addon
    By buckit in forum Basic Configuration
    Replies: 2
    Last Post: 25 Aug 2010, 09:11 PM
  4. Question about debug error (broken by google checkout addon)
    By Kenichi in forum General Questions
    Replies: 3
    Last Post: 12 Aug 2010, 07:53 PM
  5. Question about JAM/Jrox code
    By linnx in forum All Other Contributions/Addons
    Replies: 1
    Last Post: 19 May 2010, 03:42 AM

Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  
disjunctive-egg
Zen-Cart, Internet Selling Services, Klamath Falls, OR