As notified by @tatarbj and @mlhess, this module has a cross site scripting (XSS) vulnerability.
Here are the steps how it can be reproduced (courtesy of @tatarbj):
1. Enabling the module
2. As a user with the module defined 'administer dynamic_banner' permission create a new banner item under admin/structure/banners/add/0 path and set the text textfield to this: <script>alert('xss')</script>
3. When user saves the item, it immediately drops up the alert window as under admin/structure/banners page the output is not sanitized.
Even bigger issue the tpl file also prints out without any sanitization the variable plainly.
Attached is the patch to rectify the XSS issue. I'm hitting 2 birds with one stone by doing all the XSS checking (specifically the text and link field) in dynamic_banner.module. It would be too late to do the checking in banner_output.tpl.php (external users might remove the checks by accident during customization).
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | dynamic_banner-xss_issues_in_the_codebase-2968515-10.patch | 3.22 KB | tatarbj |
Comments
Comment #2
tatarbji've just fixed the summary, adding code tags around the example.
Comment #3
tatarbjHi @abrlam,
unfortunately the patch itself can't be applied on either 7.x-1.0 tag, nor 7.x-1.x branch.
As I see there has been a lot of changes since the first tag got released, I would go to release 1.1 version which is on the top of the current 7.x-1.x branch, then if you take this patch that I'm attaching now, it works flawlessly.
I show you another patch that is the diff between the last release and the one that is planned to resolve the issue. Maybe because there are many changes that has never been released in a stable version, it could be good to make a release that only fixes the issues on the top of 1.0, then releasing the rest of the changes in another version - but based on the usage of the module, possibly it's not needed, just rolling out a new version, having everything inside and getting back the coverage sounds a proper way.
Cheers,
Balazs.
Comment #4
coolestdude1 commentedHere is a similar patch based on 7.x-1.x that I adapted from #3
Comment #5
abrlam commentedI probably cloned the wrong branch. Right now the default is pointing at 7.x-1.x but I should really clone 7.x-1.0. I will try again.
Based on usage, a security patch for 7.x-1.0 is desperately needed. For simplicity sake and preventing breaking the build, can we provide a minor security patch for 7.x-1.0 first? Afterwards and once the unsupported status is removed, merge 7.x-1.0 with 7.x-1.x (dev)?
Comment #6
tatarbjI think that seems a good scenario, just post here (or even in s.d.o) the related patch first to deeply review it then the release, and later merging all the changes from dev to another release. Sounds a good plan?
Comment #7
abrlam commentedSounds good @tatarbj.
Attached is the security patch for 7.x-1.0. Will include the patch on s.d.o for reference.
Comment #8
abrlam commentedComment #9
tatarbjPlease use this to create the tag (it's cleaned up).
Tested and it seems fine :)
Comment #10
tatarbjNop, sorry, still there are tabs instead of spaces. Use this one!
Also please take a good look on this: https://www.drupal.org/docs/develop/standards
Comment #11
abrlam commentedThank you @tatarbj. Just made a new release (7.x-1.1).
Comment #14
tatarbjThanks! I think this issue can be closed as patch is already committed and new release is rolled out.
Comment #15
abrlam commented