Download & Extend
Webform Report » Issues

Fix security vulnerabilities in webform report

Posted by decibel.places on August 16, 2009 at 2:25pm
Project:Webform Report
Version:5.x-2.0
Component:Code
Category:task
Priority:normal
Assigned:Unassigned
Status:closed (fixed)

Issue Summary

following the XSS report http://drupal.org/node/540980 and removal of module, is it safe for admin use only?

or is the security problem in data submitted in webform?

Login or register to post comments

Comments

#1

Posted by decibel.places on August 16, 2009 at 2:37pm

The security report states that data is not being excaped before display by Webform Report

Is this not actually a flaw in the Webform module that collects the data? If escaped safely in the db, then Webform Report would not have an issue

Wouldn't we want to safely escape the data on collection with Webform and not only when viewing with Webform Report?

#2

Posted by jbrauer on October 6, 2009 at 2:10pm
Status:active» closed (won't fix)

The page at http://drupal.org/node/28984 further explains secure text handling in Drupal.

It explains how Drupal handles user inputed data.

When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database (be sure to read the db_query() documentation on how to use the database API securely).

The issue in http://drupal.org/node/540980 suggests that a user can input data that would compromise your admin account when an admin views the page. In short as the advisory says this module is not secure and should be disabled to prevent your site from being compromised.

#3

Posted by decibel.places on October 6, 2009 at 3:22pm

I understand how input text could compromise the site.

Is there another module that presents data collected with webform?

I am using webform/webform report as a key function on a couple of web sites

My other option is to "fix" webform report...

#4

Posted by Vako on October 6, 2009 at 4:11pm

Exactly.
Abandoning an important part of Webform is not acceptable.
Other modules also have the same security issue and they have been fixed. Why not Webform Report?
It should also be integrated in the main Webform module. Reporting is an integral part of any webform. The current results pages in Webform are not really customizable and usable.
I just hope that someone will come-up with a solution.

#5

Posted by greggles on October 9, 2009 at 4:10pm
Title:is module secure for admin only use?» Fix security vulnerabilities in webform report
Category:support request» task
Status:closed (won't fix)» active

The reason that this has not been fixed is because the maintainer did not place a priority on it.

As part of our security service we are also offering security upgrades to modules that have been abandoned.

We feel that the best way to do this is:

  1. Look for other security problems in the module
  2. Fix the reported vulnerability and any others we find
  3. Create simpletests that help ensure the problem never comes back
  4. Update the code in general to follow Drupal best practices
  5. Create a small number of additional simpletests for basic functionality of the module
  6. Create a new official release of the module

As you can see, the goal is not to just fix the vulnerability, but also make sure the module is in a generally solid state. We don't want to fix something and have our name associated with a low quality module.

It's also always possible that someone else could fix the vulnerability and become the maintainer. By paying us to fix it people can be certain that a new release will be created on a specific timeline, otherwise you must wait and hope for someone else in the community to take care of it.

#6

Posted by jimbullington on January 29, 2010 at 1:53pm
Status:active» needs review

I have created a patch for the 5.x-2.2 version (Not on the list for some reason - webform_report-5.x-2.2.tar.gz ) of the webform report module that (hopefully) corrects the XSS issues.

  1. All dbquery requests have been parameterized
  2. All $_POST usage has been removed - hook_load and hook_form have been reworked to use more generally accepted Drupal coding standards
  3. Some other minor fixes have been incorporated

I would be glad to take over responsibility for this module, if needed - I would like to incorporate it into my site as see it move forward. I would also be willing to work on a 6.x version of this module as time allows.

AttachmentSize
webform_report-5.x-2.2-XSS.patch 23.91 KB

#7

Posted by jimbullington on January 29, 2010 at 3:19pm

Sorry - here is another patch - found a few other things mentioned here.

AttachmentSize
webform_report-5.x-2.2-XSS_Version_2.patch 24.7 KB

#8

Posted by decibel.places on January 29, 2010 at 6:17pm

great Jim

since you've already patched v 5, can you take a shot at v 6?

I'd be willing to pay a (small) bounty for a v 6 patch that passes testing.

#9

Posted by jimbullington on January 29, 2010 at 6:45pm

One more time - after reviewing this, I added a filter_xss() call to filter user submission data before displaying on reports.

AttachmentSize
webform_report-5.x-2.2-XSS_Version_3.patch 25.16 KB

#10

Posted by jimbullington on January 29, 2010 at 6:30pm

Would be glad to - there doesn't appear to be a v 6 for this module - is that correct?

I would just like to see the module move forward since I plan on using it myself.

#11

Posted by decibel.places on January 29, 2010 at 11:40pm

attaching webform_report-6.x-1.8 I am using on a couple of D6 sites (with low vulnerability)

AttachmentSize
webform_report-6.x-1.8.tar_.gz 15.27 KB

#12

Posted by decibel.places on January 29, 2010 at 11:44pm

Jim,

Would you be able to become the new maintainer for the abandoned webform report module?

I don't have the time...

You will achieve great glory and no dinero

#13

Posted by jimbullington on January 30, 2010 at 4:40pm

I would be happy to maintain the project - if it gets "resurrected". However, it looks like we have lost contact with the current maintainer, so I don't know how to go about requesting it...

#14

Posted by jimbullington on January 30, 2010 at 5:21pm

I'm applying for a CVS account - we'll see how it goes...

#15

Posted by jimbullington on January 31, 2010 at 8:46pm

As requested by Drupal Security, an XSS only patch is attached. Only the security issues are addressed in this patch.

  1. All db_query() requests have been parameterized
  2. The drupal_set_title() parameter has been processed through check_plain()
  3. All watchdog() text parameters have been processed through t()
  4. User entered data from webreport has been processed through filter_xss() before being output
  5. $_GET[] usage for the custom pager has been removed - the standard pager.inc theming functions are being used
AttachmentSize
webform_report-5.x-2.2-XSS-Only.patch 8.17 KB

#16

Posted by grendzy on February 1, 2010 at 7:29am
Status:needs review» needs work

Thanks for your continued effort Jim.

+++ webform_report.module (working copy)
@@ -227,7 +226,7 @@
-  $output = "<p>" . $node->description;
+  $output = "<p>" . t($node->description);

User-generated text isn't translatable.

+++ webform_report.module (working copy)
@@ -265,11 +264,11 @@
-    watchdog('webform_report', 'Webform report "'.$node->title.'" added', WATCHDOG_NOTICE);
+    watchdog('webform_report', t('Webform report @title added', array('@title' => $node->title)), WATCHDOG_NOTICE);

Watchdog calls should use placeholders, but not the t() function. Watchdog messages are translated when they are displayed

+++ webform_report.module (working copy)
@@ -280,7 +279,7 @@
-    watchdog('webform_report', 'Webform report "'.$node->title.'" updated', WATCHDOG_NOTICE);
+    watchdog('webform_report', t('Webform report @title updated', array('@title' => $node->title)), WATCHDOG_NOTICE);
   }

remove t().

+++ webform_report.module (working copy)
@@ -291,7 +290,7 @@
-    watchdog('webform_report', 'Webform report "'.$node->title.'" deleted', WATCHDOG_NOTICE);
+    watchdog('webform_report', t('Webform report @title deleted', array('@title' => $node->title)), WATCHDOG_NOTICE);

remove t()

Powered by Dreditor.

#17

Posted by jimbullington on February 1, 2010 at 1:45pm

Thank you for your review - I appreciate the feedback!

A corrected patch is attached.

t($node->description) has been changed to check_markup($node->description)

However, I think t() needs to stay in the watchdog() calls - the module I am patching is for Drupal V5 which still requires t() for substitution according to the API and this article.

AttachmentSize
webform_report-5.x-2.2-XSS-Only-Version-2.patch 8.18 KB

#18

Posted by jimbullington on February 1, 2010 at 3:47pm
Status:needs work» needs review

Here is an XSS patch for the 6.x-1.8 version that was downloaded from http://ftp.drupal.org/files/projects/webform_report-6.x-1.8.tar.gz. This is the latest 6.x version of this module that I know of.

AttachmentSize
webform_report-6.x-1.8_XSS-Patch.patch 8.53 KB

#19

Posted by Vako on February 3, 2010 at 9:10pm

The dates on the 6.x-1.8.tar.gz files are 2008...so I am assuming that we need the 6.x-1.8.tar.gz file plus the patch. Right?

#20

Posted by jimbullington on February 3, 2010 at 9:20pm

Yes. That is what I used when I tested it. It's the latest 6.x version I could find...

#21

Posted by grendzy on February 5, 2010 at 5:28pm
Status:needs review» needs work

Hi, I found a few more potential issues:

webform_report.inc:11 missing db_rewrite_sql()

webform_report:373 missing db_rewrite_sql()

webform_report.module:419 XSS vulnerability in checkbox #options

BTW I applied your patch to the 6.x-1.x branch in CVS. You can check out the branch with this command:
cvs -z6 -d:pserver:anonymous:anonymous@cvs.drupal.org:/cvs/drupal-contrib checkout -d webform_report -r DRUPAL-6--1 contributions/modules/webform_report

#22

Posted by jimbullington on February 6, 2010 at 7:35pm
Status:needs work» needs review

Thanks for the review!

I checked out the 6--1 CVS version and addressed your comments in the attached patch.

  1. Added db_rewrite_sql() as required
  2. Added array_map('filter_xss',... to the checkbox options

Thanks again for your time and patience.

AttachmentSize
webform_report-6--1-CVS_XSS-Patch.patch 10.24 KB

#23

Posted by grendzy on February 9, 2010 at 6:19pm
Status:needs review» needs work

Almost there! I found a couple more vulnerabilities:

webform_report.inc:224 - Needs filter_xss_admin() around $node->description

webform_report.inc:417 - $fields array needs check_plain()

webform_report.inc:100,281,285,298 - Use url() instead of href

The first two are confirmed XSS vulnerabilities; I'm not sure about the use of url() but it would safer to convert those links.

thanks!

#24

Posted by jimbullington on February 10, 2010 at 2:37am
Status:needs work» needs review

Thanks for the review!

I've addressed your concerns in the attached patch:

  1. Wrapped $node->description with filter_xss_admin()
  2. Wrapped $row->name with check_plain() at 83 - it looks like this is where the $fields array is built that is used in 417
  3. Changed the href cat url() expressions to l() calls - I thought l() was a better fit in this situation

Thanks for the feedback - I'm learning a lot about Drupal security by going through this exercise

AttachmentSize
webform_report-6--1-CVS_XSS-Version2.patch 12.19 KB

#25

Posted by grendzy on February 11, 2010 at 9:29pm
Status:needs review» reviewed & tested by the community

well done! You are of course correct about l().

#26

Posted by jimbullington on February 11, 2010 at 11:21pm

Thanks grendzy!

#27

Posted by jimbullington on February 15, 2010 at 11:35pm
Status:reviewed & tested by the community» fixed

#28

Posted by jimbullington on February 15, 2010 at 11:38pm
Status:fixed» closed (fixed)

#29

Posted by Vako on February 16, 2010 at 4:04pm

Thanks everyone who helped resolve the security issue in this module and get it back online. Especially thank you jimbullington for taking over as maintainer of this module.

#30

Posted by decibel.places on February 19, 2010 at 4:00am

thanks for your work jimbullington!

I have updated the webform report module with your new version on my production sites.