Video module getting ready for 4.7 release: need help debugging
Hi everybody, I just uploaded to cvs a new version of the video module which adds long time needed and requested features to the module. The new code is still not mature and I'd like you guys to give a try to the new video module and reports your bug at http://drupal.org/node/add/project_issue/video If you have time please install the module on a test site and give it a try... Instead please point your browser to: http://www.varesano.net/video-demo/ There is a drupal install with the new video module. I ask you to play the video there and reports problems. It's quite difficult to try the video embed stuff with all kind of browsers / players plug-in / platforms ... this is why I need that lot of people with different setups try to play videos. Ok.. hope to hear something from you soon. Fabio Following there is a list of last added features: Pluginization: Video.module file was too big and complex. I isolated each different feature which was not excencial and removed from the video.module file. Those features are now added by helper modules called plugins under the directory "plugins". The download has been separed from multidownload. There is now a separated plugin called video_multidownload which add multidownload feature. There are also some hooks being defined. See file hooks.php for details. XHTML Compliace: I worked hard to remove unvalid code from video module. Now the code generated by the module validates on W3C validator. This will probæbly generate some problems with uncommon browsers we will try to solve them as soon as reported. Thanks a lot to Karl Rudd who point me to the right direction towards compliace. Thumbnailing: There is plugin called video_image.module which add thumbnails support for the video module. Thumbnails are uploaded throught the video creation form and a image node is created with it. Video file Uploads: There is a plugin called video_upload.module which add a file upload field to the node creation form. The uploaded file is automatically set as path of the video. Then usable for plays/downloads.
On 18 Jun 2006, at 18:30, Fabio Varesano wrote:
I just uploaded to cvs a new version of the video module which adds long time needed and requested features to the module.
The new code is still not mature and I'd like you guys to give a try to the new video module and reports your bug at http://drupal.org/node/add/project_issue/video
1. Your code has various XSS problems. For example: t('play %link', array('%link' => $node->title)) should be: t('play %link', array('%link' => theme('placeholder', $node->title))) You also need to escape data before outputting it: <object type="video/quicktime" width="'. $node->videox .'" height="'. $height .'" data="'. $node->vidfile .'"> It's insecure, and unfortunately, it needs quite a bit of work. 2. For consistency, don't capitalize each word in a sentence. For example: Video Size Height should be: Video size height 3. In MySQL queries you don't need quotes around %d; that will break compatibility with PostgreSQL. Hope that helps, -- Dries Buytaert :: http://www.buytaert.net/
On 18 Jun 2006, at 23:44, Dries Buytaert wrote:
1. Your code has various XSS problems. For example:
t('play %link', array('%link' => $node->title))
should be:
t('play %link', array('%link' => theme('placeholder', $node-
title)))
The following modules or files make the exact same security mistake (XSS): acidfree/acidfree.module acidfree/class_album.inc acidfree/class_photo.inc acidfree/class_video.inc aggregator2/aggregator2.module amazontools/amazon.module bugs/bugs.module citizenspeak/citizenspeak.theme.php commentmail/commentmail.module cvbuilder/cvbuilder.module discography/discography.module eatlocal/resource/resource.module ecommerce/contrib/auction/auction.module ecommerce/subproducts/subproducts.inc eventrepeat/eventrepeat.module export_docbook/export_docbook.module faq/faq.module gojoingo/modules/gjg_event/gjg_event.module groups/groups.module img_assist/img_assist.module interview/interview.module listhandler/listhandler.module macrotags/macros.inc mail/mail.module moviereview/moviereview.module naggregator/naggregator.module naggregator/naggregator_convert.php news_page/news_page.module node_aggregator/naggregator.convert.php node_image/node_image.module playlist/playlist.module pr/pr.module print/print.node.tpl.php project/update-project.php publication/publication.module recipe/recipe.module send/send.inc shortcuts/shortcuts.module spam/spam.module staffbio/staffbio.module tagnode/tagnode.module tec/tec.module term_access/patches/book.patch topic/topic.module trackback/trackback.module upcomingorg/upcomingorg.module userreview/userreview.module wallpaper/wallpaper.module webcomic/webcomic_theme.inc webform/webform.module whatsrelated/whatsrelated.module wishlist/wishlist.module (There might be some false positives.) -- Dries Buytaert :: http://www.buytaert.net/
@ adrian anonymous comments are now enabled. @ dries thank you for your comments.. I will work on this asap.
t('play %link', array('%link' => $node->title))
this is used as title attribute for a link... maybe check_plain() should be used insted of theme('placeholder') as suggested. Also numeric node fields needs to be escaped? Fabio Varesano
t('play %link', array('%link' => $node->title))
this is used as title attribute for a link... maybe check_plain() should be used insted of theme('placeholder') as suggested.
Neither check_plain() or theme('placeholder') are necessary or even make sense here. Title attributes cannot contain HTML, so their content is passed as plain-text to l(). The attribute content is escaped right before outputting in drupal_attributes(). Steven Wittens
On 6/18/06, Steven Wittens <steven@acko.net> wrote:
t('play %link', array('%link' => $node->title))
this is used as title attribute for a link... maybe check_plain() should be used insted of theme('placeholder') as suggested.
Neither check_plain() or theme('placeholder') are necessary or even make sense here. Title attributes cannot contain HTML, so their content is passed as plain-text to l(). The attribute content is escaped right before outputting in drupal_attributes().
Steven l() does call check_plain() if the last argument ($html) is true. But Fabio and Dries are talking about t(), not l(), which does not call check_plain(). Or did I miss something?
Or did I miss something?
Yes. As Fabio said, the result of this t() call is used for the *title attribute of a link*. I imagine the surrounding code is something like l(.... , array('title' => t('play %link', array('% link' => $node->title)))). Title attributes (or any HTML attribute for that matter) can not contain mark-up. So, there is no reason to require individual attributes to be escaped by the caller. We escape all attribute contents in drupal_attributes(), which is what l() uses. As always, output context is everything. Steven
On 6/18/06, Steven Wittens <steven@acko.net> wrote:
Or did I miss something?
Yes. As Fabio said, the result of this t() call is used for the *title attribute of a link*. I imagine the surrounding code is something like l(.... , array('title' => t('play %link', array('% link' => $node->title)))).
Title attributes (or any HTML attribute for that matter) can not contain mark-up. So, there is no reason to require individual attributes to be escaped by the caller. We escape all attribute contents in drupal_attributes(), which is what l() uses.
As always, output context is everything.
I was commenting on the general issue, and not Fabio's specific case. You are right that that latter case is moot, since we get it for free by virtue of the l(). What about the rest of the list that Dries posted?
On 19 Jun 2006, at 06:05, Khalid B wrote:
What about the rest of the list that Dries posted?
Looks like these are still valid. -- Dries Buytaert :: http://www.buytaert.net/
What was the query you used to identify the problem? I think amazon.moduleis one of the false positives, but I want ot make sure I'm looking at the same thing you are. On 6/19/06, Dries Buytaert <dries.buytaert@gmail.com> wrote:
On 19 Jun 2006, at 06:05, Khalid B wrote:
What about the rest of the list that Dries posted?
Looks like these are still valid.
-- Dries Buytaert :: http://www.buytaert.net/
On 19 Jun 2006, at 16:50, Earl Dunovant wrote:
What was the query you used to identify the problem? I think amazon.module is one of the false positives, but I want ot make sure I'm looking at the same thing you are.
This line is vulnerable (amongst other): $datacell .= "<img src=\"$node->smallimageurl\" height=\"$node-
smallimageheight\" width=\"$node->smallimagewidth\" alt=\"cover of $node->title\" />"
-- Dries Buytaert :: http://www.buytaert.net/
These fields are coming from the database, and the table is populated with data from Amazon.com. I prefer scrubbing it on the way in (admittedly not doing that at the moment because I figured if you can hijack Amazon.com's servers you're going to get me if you want to anyway). The fewer places I have to worry about it, the better. On 6/19/06, Dries Buytaert <dries.buytaert@gmail.com> wrote:
On 19 Jun 2006, at 16:50, Earl Dunovant wrote:
What was the query you used to identify the problem? I think amazon.module is one of the false positives, but I want ot make sure I'm looking at the same thing you are.
This line is vulnerable (amongst other):
$datacell .= "<img src=\"$node->smallimageurl\" height=\"$node-
smallimageheight\" width=\"$node->smallimagewidth\" alt=\"cover of $node->title\" />"
-- Dries Buytaert :: http://www.buytaert.net/
On 19 Jun 2006, at 18:41, Earl Dunovant wrote:
These fields are coming from the database, and the table is populated with data from Amazon.com. I prefer scrubbing it on the way in (admittedly not doing that at the moment because I figured if you can hijack Amazon.com's servers you're going to get me if you want to anyway). The fewer places I have to worry about it, the better.
That doesn't work. People (or modules) could edit or modify the node at any time, and then you'd be toast. :-) -- Dries Buytaert :: http://www.buytaert.net/
Ok.. I'm working on escaping stuff from my video module... My question now is: numeric database fields (int) needs to get escaped? I think that this is not required becouse is not possible to store any XSS vulnerable value in it... but I can be wrong. Fabio
Serious question: if an attacker has the necessary access to modify the data in the table (because that is what it would take to cause a problem) or if someone installs a malicious module do I really have any way to stop it? On 6/19/06, Dries Buytaert <dries.buytaert@gmail.com> wrote:
On 19 Jun 2006, at 18:41, Earl Dunovant wrote:
These fields are coming from the database, and the table is populated with data from Amazon.com. I prefer scrubbing it on the way in (admittedly not doing that at the moment because I figured if you can hijack Amazon.com's servers you're going to get me if you want to anyway). The fewer places I have to worry about it, the better.
That doesn't work. People (or modules) could edit or modify the node at any time, and then you'd be toast. :-)
-- Dries Buytaert :: http://www.buytaert.net/
On 19 Jun 2006, at 22:25, Earl Dunovant wrote:
Serious question: if an attacker has the necessary access to modify the data in the table (because that is what it would take to cause a problem) or if someone installs a malicious module do I really have any way to stop it?
Yes. If someone has access to modify your amaozon-related nodes, he or she could hijack the session of uid #1. So in theory, the module is vulnerable. In practice, and depending on the assumptions you make about how your module is used/configured, it is unlikely to be exploited. Unlikely or not, it is best avoided, because you never really know how your module is going to be used by others. -- Dries Buytaert :: http://www.buytaert.net/
I added check_* to HTML printed variables. Diff between older version: http://cvs.drupal.org/viewcvs/drupal/contributions/modules/video/video.modul... Do you guys see other huge mistakes? Fabio
Earl, Amazon could give you data that contains special HTML chars like < or > which would break the display without any hacker getting into your DB. Goba On Mon, 19 Jun 2006, Earl Dunovant wrote:
Serious question: if an attacker has the necessary access to modify the data in the table (because that is what it would take to cause a problem) or if someone installs a malicious module do I really have any way to stop it?
On 6/19/06, Dries Buytaert <dries.buytaert@gmail.com> wrote:
On 19 Jun 2006, at 18:41, Earl Dunovant wrote:
These fields are coming from the database, and the table is populated with data from Amazon.com. I prefer scrubbing it on the way in (admittedly not doing that at the moment because I figured if you can hijack Amazon.com's servers you're going to get me if you want to anyway). The fewer places I have to worry about it, the better.
That doesn't work. People (or modules) could edit or modify the node at any time, and then you'd be toast. :-)
-- Dries Buytaert :: http://www.buytaert.net/
On 19 Jun 2006, at 01:42, Steven Wittens wrote:
t('play %link', array('%link' => $node->title))
this is used as title attribute for a link... maybe check_plain() should be used insted of theme('placeholder') as suggested.
Neither check_plain() or theme('placeholder') are necessary or even make sense here. Title attributes cannot contain HTML, so their content is passed as plain-text to l(). The attribute content is escaped right before outputting in drupal_attributes().
Yes, I picked the wrong example. There are still dozens of other security bugs in the video module though. drupal_set_title(t('Playing') . ' ' . $node->title); -- Dries Buytaert :: http://www.buytaert.net/
participants (7)
-
Dries Buytaert -
Dries Buytaert -
Earl Dunovant -
Fabio Varesano -
Gabor Hojtsy -
Khalid B -
Steven Wittens