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?
Comments
#1
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
The page at http://drupal.org/node/28984 further explains secure text handling in Drupal.
It explains how Drupal handles user inputed data.
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
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
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
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:
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
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.
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.
#7
Sorry - here is another patch - found a few other things mentioned here.
#8
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
One more time - after reviewing this, I added a filter_xss() call to filter user submission data before displaying on reports.
#10
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
attaching webform_report-6.x-1.8 I am using on a couple of D6 sites (with low vulnerability)
#12
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
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
I'm applying for a CVS account - we'll see how it goes...
#15
As requested by Drupal Security, an XSS only patch is attached. Only the security issues are addressed in this patch.
#16
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
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.
#18
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.
#19
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
Yes. That is what I used when I tested it. It's the latest 6.x version I could find...
#21
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
Thanks for the review!
I checked out the 6--1 CVS version and addressed your comments in the attached patch.
Thanks again for your time and patience.
#23
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
Thanks for the review!
I've addressed your concerns in the attached patch:
Thanks for the feedback - I'm learning a lot about Drupal security by going through this exercise
#25
well done! You are of course correct about l().
#26
Thanks grendzy!
#27
#28
#29
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
thanks for your work jimbullington!
I have updated the webform report module with your new version on my production sites.