dadada Posted July 25, 2012 Share Posted July 25, 2012 CSRF Token - Admin Side http://addons.oscommerce.com/info/8506 Link to comment Share on other sites More sharing options...
burt Posted July 25, 2012 Share Posted July 25, 2012 May you like to explain why this is good, and for what reason shopowners should add it... $form .= tep_draw_hidden_field('token', $token) . PHP_EOL; . Link to comment Share on other sites More sharing options...
dadada Posted July 25, 2012 Author Share Posted July 25, 2012 @@burt: As you know programmers can do same work with different codes ;) thanks for the trick, I'll include that in next release. Link to comment Share on other sites More sharing options...
burt Posted July 25, 2012 Share Posted July 25, 2012 I mean maybe you should explain what the problem is that your code cures. Forget that line of code I just added, I added that after, by the edit. Most shopowners have no clue what your code does or what potential problem it does cure. . Link to comment Share on other sites More sharing options...
Harald Ponce de Leon Posted July 25, 2012 Share Posted July 25, 2012 Similar to what was done for the Catalog side in v2.3, forms will also be protected with a session token on the Administration Tool to protect store owners clicking on external malicious links that could perform administrative actions if they have an active Administration Tool session in their browser. Such malicious links would also bypass any kind of htaccess protection layer due to the active session in the browser (browser tab or window). There have been no reports of this happening - it is just a security blanket that is being added to v2.3.3. @, the solution you came up with is a quick solution that adds the token to all internal links and is not desirable. This is what was done for the Catalog side to protect forms individually: https://github.com/osCommerce/oscommerce2/commit/95b005d1312b636695ee278471496372e8d56fd3 This is what should be done for the Administration Tool as well. , osCommerce Link to comment Share on other sites More sharing options...
dadada Posted July 25, 2012 Author Share Posted July 25, 2012 @@harald: Thanks for your comments :) But, your codes will work only at POST form submits, and never gonna prevent GET CSRF attacks. And keep in mind that there are hundreds of addons may use get parameters to change the values. #UPDATE 2: Actually, I don't see any problem to make "token= ... " in all internal links. If you know bank sites, other ecommerce sites , phpMyadmin, and other populer CMSs are using tokens as the same way. Link to comment Share on other sites More sharing options...
dadada Posted July 25, 2012 Author Share Posted July 25, 2012 @@burt: Actually, I didn't understand what you're talking about ! Pls, explain better. #update: Alright, the reason for update to fix get forms. I already added the description into update log. This bug has reported before few hours. "Hi, I added your addon the probleme is when i do a search in my admon i go back to login looks like the token is not registered any idea???" Link to comment Share on other sites More sharing options...
Harald Ponce de Leon Posted July 25, 2012 Share Posted July 25, 2012 Here is a good summary why not to disclose the token in the URL: https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#Disclosure_of_Token_in_URL , osCommerce Link to comment Share on other sites More sharing options...
dadada Posted July 25, 2012 Author Share Posted July 25, 2012 @@harald: The information that you point is TRUE and let check it out the texts. "CSRF tokens in GET requests are potentially leaked at several locations: browser history, HTTP log files, network appliances that make a point to log the first line of an HTTP request, and Referrer headers if the protected site links to an external site." -My solution uses session based tokens, so after the log-out nobody cannot use again (ex: server log files, browser history, etc.). -" My solution uses oscommerce framework's functions and cannot referrer to any external link. " You can check it out too. -And the admin is already free to limit session.gc_maxlifetime using php.ini file in the admin folder for better practice. If sensitive server-side actions are guaranteed to only ever respond to POST requests, then there is no need to include the token in GET requests. -My solution will work well with 3d party add-ons here. Well :) , Unfortunately I don't have a time to comment more detailed. Actually I spent last 8-9 months for web application security. I've developed the security extension for X shopping cart. As I'm no more working with/on oscommerce I've just shared my work with the community. As you pointed out: Note: Contributions are used at own risk. And I can talk much more about oscommerce security issues. example : Imagine a bot , which: -go to create_account.php page finds name="formid" and submit forms using curl. So the result : just in minutes database can crush. Good Improvements: -Use random value with random sizes like name="ndiu93y49nf9438gf9gb34" (+regenerate on server side on every success or/and fail) -Use captcha, which cannot read by bots like reCaptcha." (+regenerate on server side on every success or/and fail) -Use account activations, and use cron job to remove all unactivated accounts periodically to prevent spamming. -Use limit for reviews etc... -Use Capthca at logins (to prevent brute force attack and help preventing to DDOS) + form submits (which affect db) etc... -Also another security issue at image uploading in admin side. that doesn't check mime tyoe of uploading file. finfo_file can be a solution there but that will need phpv5.3. And so on... Sorry, I really don't have a time to tell everything about security. So sorry, may I cannot answer in to the next post. Thanks to community. Link to comment Share on other sites More sharing options...
dadada Posted July 25, 2012 Author Share Posted July 25, 2012 + :) action recorder at user logins based on user emails. ie: if (email_exists(email) && password !== true) $attempt++; Link to comment Share on other sites More sharing options...
Harald Ponce de Leon Posted July 25, 2012 Share Posted July 25, 2012 This is a good quote from the OWASP article: Ultimately, the acceptance of this risk as opposed to the cost of significant architecture design is up to the business. Having the token automatically added to all internal links poses minimum security risk of exposing it in such a public manner. It is more work to add tokens manually - especially for Add-Ons - however this provides more control over the use of the token. We may disagree on the actual implementation, but agree that any kind of implementation should be added. :thumbsup: , osCommerce Link to comment Share on other sites More sharing options...
Harald Ponce de Leon Posted July 25, 2012 Share Posted July 25, 2012 + :) action recorder at user logins based on user emails. ie: if (email_exists(email) && password !== true) $attempt++; Want to work on it? :) , osCommerce Link to comment Share on other sites More sharing options...
♥FWR Media Posted July 25, 2012 Share Posted July 25, 2012 @@Harald Ponce de Leon Interesting conversation this and it's great to see CSRF protection added to osCommerce. Instead of lines like: - if (isset($HTTP_POST_VARS['action']) && ($HTTP_POST_VARS['action'] == 'process') && isset($HTTP_POST_VARS['formid']) && ($HTTP_POST_VARS['formid'] == $sessiontoken)) Would it make sense to have a tep function that validates forms, CSRF and typecasts? Example: - $sessiontoken = 'dw3f53523f523534'; $_POST = array( 'action' => 'process', 'form_id' => 'dw3f53523f523534', 'input' => '1', 'hack' => '<script>alert(\'Nasty!\');</script>', 'escape' => '&testing tep_ &' ); // tep_form_validate( array, [,$csrf_protection = true], [,$csrf_only_protection = false]) $result = tep_form_validate( array( 'action' => 'process', 'input' => 'int', 'hack' => 'no_tags', 'escape' => 'tep_output_string_protected' ), $csrf_protection = true, $csrf_only_protection = false ); if ( false === $result ) { echo 'Form validation failed'; } else echo '<pre>' . var_dump( $result ) . '</pre>'; Results in: - array(4) { ["action"]=> string(7) "process" ["input"]=> int(1) ["hack"]=> string(16) "alert('Nasty!');" ["escape"]=> string(23) "&testing tep_ &" } Reason I ask is I just wrote one, one minute I was thinking about it, the next .... Simple implementations: - tep_form_validate( array(), true, true ); Checks only the CSRF and returns bool true/false tep_form_validate( array( 'action' => 'process' ), true ); Checks CSRF and that _POST[action] = process and returns typecast array( 'action' => (string)process ) tep_form_validate( array( 'action' => 'process', 'some_string_number' => 'int' ), true ); 1) Checks CSRF 2) That _POST[action] = (string)process 3) Type casts 'some_string_number' to int 4) returns typecast array( 'action' => (string)process, 'some_string_number' => (int)123 ) Currently typecasts are int, numeric, float, string, no_tags, array and tep_ ( use a tep_function on the string ) I thought it might be useful to have a relatively short function that checks CSRF, validates the _POST against what is expected then returns an array of type cast results. Ultimate SEO Urls 5 PRO - Multi Language Modern, Powerful SEO Urls KissMT Dynamic SEO Meta & Canonical Header Tags KissER Error Handling and Debugging KissIT Image Thumbnailer Security Pro - Querystring protection against hackers ( a KISS contribution ) If you found my post useful please click the "Like This" button to the right. Please only PM me for paid work. Link to comment Share on other sites More sharing options...
Harald Ponce de Leon Posted July 25, 2012 Share Posted July 25, 2012 @@FWR Media Hi Robert.. 1. Fork on Github 2. Get the implementation working (as simple and cleanly as possible) 3. Propose the feature to be added 4. We'll review it :thumbsup: , osCommerce Link to comment Share on other sites More sharing options...
♥FWR Media Posted July 25, 2012 Share Posted July 25, 2012 @@Harald Ponce de Leon Well .. to be fair .. it is conceptual so I wanted your opinion before taking the time to go through those processes. Ultimate SEO Urls 5 PRO - Multi Language Modern, Powerful SEO Urls KissMT Dynamic SEO Meta & Canonical Header Tags KissER Error Handling and Debugging KissIT Image Thumbnailer Security Pro - Querystring protection against hackers ( a KISS contribution ) If you found my post useful please click the "Like This" button to the right. Please only PM me for paid work. Link to comment Share on other sites More sharing options...
Harald Ponce de Leon Posted July 25, 2012 Share Posted July 25, 2012 Robert, it looks interesting! That's why I requested to see a working example :) If it's a lot of changes, it won't go in v2.3.3, but could in v2.4 where legacy code will be removed/replaced. , osCommerce Link to comment Share on other sites More sharing options...
♥FWR Media Posted July 25, 2012 Share Posted July 25, 2012 Robert, it looks interesting! That's why I requested to see a working example :) Right oh! I'll have a bash .. I've only ever used SVN before and no public online services so it should be hilarious. If I destroy github I'll claim diminished responsibility .. a touch of insanity .. then point them in your general direction :D If it's a lot of changes, it won't go in v2.3.3, but could in v2.4 where legacy code will be removed/replaced. Well I'll write it with a load of functionality which, if you happen to like it, you can obviously strip out as you see fit. Obviously there are a lot of forms in 2.3.2 so equally obviously it would mean a lot of code changes if used for every form capture. Ultimate SEO Urls 5 PRO - Multi Language Modern, Powerful SEO Urls KissMT Dynamic SEO Meta & Canonical Header Tags KissER Error Handling and Debugging KissIT Image Thumbnailer Security Pro - Querystring protection against hackers ( a KISS contribution ) If you found my post useful please click the "Like This" button to the right. Please only PM me for paid work. Link to comment Share on other sites More sharing options...
dadada Posted July 25, 2012 Author Share Posted July 25, 2012 Want to work on it? :) Ooops :) That was bad idea. Why? Because the attacker will be able to block victim's login. The similar thing happened before years at HOTMAIL. So, captcha still is the better solution. Link to comment Share on other sites More sharing options...
Harald Ponce de Leon Posted July 26, 2012 Share Posted July 26, 2012 I'll have a bash .. I've only ever used SVN before and no public online services so it should be hilarious. If I destroy github I'll claim diminished responsibility .. a touch of insanity .. then point them in your general direction :D Be sure to check out this classic presentation to help you get started: http://blip.tv/oscommerce/working-with-the-development-repository-on-github-1933713 , osCommerce Link to comment Share on other sites More sharing options...
Recommended Posts
Archived
This topic is now archived and is closed to further replies.