Hi,
I would like to get contrib access to this project. I have a D7 port and a good idea how to fix the XSS issue. I am using this and like this player instead of jplayer.

CommentFileSizeAuthor
#12 mp3player_add_filters-2050425-12.patch14.31 KBmarkie
#7 mp3player_add_filters-2050425-7.patch13.7 KBmarkie
#5 mp3player_add-filters.2050425.5.patch5.42 KBmarkie
#3 mp3player_add-filters.2050425.3.patch55.26 KBmarkie
#1 mp3player_player-to-libraries.2050425.2.patch2.78 KBmarkie
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markie’s picture

markie CreditAttribution: markie commented
FileSize
mp3player_player-to-libraries.2050425.2.patch2.78 KB

Attached is a patch with a possible resolution to the XSS issue. I've moved the swf file to the libraries to separate the projects.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Version: » 6.x-1.x-dev
Status: Active » Needs work

Here's the original bug report. Now that it's been a pretty long time since the SA was made public it seems OK to post it here.

It seems you got some of the problem but not all of it.

Note also that the patch proposed below relies on validating inputs and while that's certainly a good idea, the Drupal standard is to also filter on output.

Hello Drupal Security,

     I've discovered an XSS vulnerability in the MP3Player 6.x-1.1
module. Below you will find the vulnerability report.

--
_____________

--redacted name and contact info--

------------------------------------------------------
Vulnerability Report

MP3Player 6.x-1.1 XSS Vulnerability

Date of Contact:  Jan 16th, 2013 11:55:00 GMT -0500
Author: Kyle Small <ks347@sas.upenn.edu>

Description of Vulnerability:
-----------------------------
     The MP3Player 6.x-1.1 module contains a cross site scripting (XSS)
vulnerability that can be exploited when a specially named mp3 file is
uploaded. This could result in administrative account compromise.

Systems affected:
-----------------
* Drupal 6.x
* MP3Player 6.x-1.1, Other versions may be affected as well.

Mitigating factors:
-------------------
In order to execute the proof of concept described below, malicious
users must have permissions for creating nodes and uploading files.

Proof of Concept:
-----------------
1. Install Drupal 6.x with the MP3Player 6.x-1.1 enabled.
2. Create/Edit a node with an mp3 file field.
3. Upload an mp3 with a name such as a"><body
onload=javascript:alert("XSS")>.mp3
4. View the node to see the exploit in action.

Patch:
------------------------------------------

--- mp3player.module    2011-03-02 17:04:01.000000000 -0500
+++ mp3player.module.fixed    2013-01-15 15:47:51.699606927 -0500
@@ -767,7 +767,7 @@
    if($form_state['values']['name'] != '' &&
strlen($form_state['values']['name']) >= 20) {
      form_set_error('name', t('Player name must be 20 characters long
or shorter.'));
    }
-  if ($form_state['values']['initialvolume'] < 0 ||
$form_state['values']['initialvolume'] > 100) {
+  if (!is_numeric($form_state['values']['initialvolume']) ||
$form_state['values']['initialvolume'] < 0 ||
$form_state['values']['initialvolume'] > 100) {
      form_set_error('initialvolume', t('Volume must be between 0 and
100.'));
    }
    if (!is_numeric($form_state['values']['buffer'])) {
@@ -777,53 +777,53 @@
    if (!is_numeric($form_state['values']['width']) && $percentage != '%') {
      form_set_error('width', t('Width must be an integer or percentage.'));
    }
-  if($form_state['values']['pagebg'] != '' &&
strlen($form_state['values']['pagebg']) != 6) {
+  if($form_state['values']['pagebg'] != '' &&
preg_match('/[^0-9a-fA-F]/',$form_state['values']['pagebg']) ||
strlen($form_state['values']['pagebg']) != 6) {
      form_set_error('pagebg', t('HEX code must be 6 characters long.'));
    }

-  if(strlen($form_state['values']['bg']) != 6) {
+  if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['bg']) ||
strlen($form_state['values']['bg']) != 6) {
      form_set_error('bg', t('HEX code must be 6 characters long.'));
    }
-  if(strlen($form_state['values']['leftbg']) != 6) {
+  if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['leftbg']) ||
strlen($form_state['values']['leftbg']) != 6) {
      form_set_error('leftbg', t('HEX code must be 6 characters long.'));
    }
-  if(strlen($form_state['values']['lefticon']) != 6) {
+  if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['lefticon']) ||
strlen($form_state['values']['lefticon']) != 6) {
      form_set_error('lefticon', t('HEX code must be 6 characters long.'));
    }
-  if(strlen($form_state['values']['voltrack']) != 6) {
+  if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['voltrack']) ||
strlen($form_state['values']['voltrack']) != 6) {
      form_set_error('voltrack', t('HEX code must be 6 characters long.'));
    }
-  if(strlen($form_state['values']['volslider']) != 6) {
+ if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['volslider']) ||
strlen($form_state['values']['volslider']) != 6) {
      form_set_error('volslider', t('HEX code must be 6 characters long.'));
    }
-  if(strlen($form_state['values']['rightbg']) != 6) {
+  if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['rightbg']) ||
strlen($form_state['values']['rightbg']) != 6) {
      form_set_error('rightbg', t('HEX code must be 6 characters long.'));
    }
-  if(strlen($form_state['values']['rightbghover']) != 6) {
+ if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['rightbghover'])
|| strlen($form_state['values']['rightbghover']) != 6) {
      form_set_error('rightbghover', t('HEX code must be 6 characters
long.'));
    }
-  if(strlen($form_state['values']['righticon']) != 6) {
+ if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['righticon']) ||
strlen($form_state['values']['righticon']) != 6) {
      form_set_error('righticon', t('HEX code must be 6 characters long.'));
    }
-  if(strlen($form_state['values']['righticonhover']) != 6) {
+
if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['righticonhover'])
|| strlen($form_state['values']['righticonhover']) != 6) {
      form_set_error('righticonhover', t('HEX code must be 6 characters
long.'));
    }
-  if(strlen($form_state['values']['loader']) != 6) {
+  if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['loader']) ||
strlen($form_state['values']['loader']) != 6) {
      form_set_error('loader', t('HEX code must be 6 characters long.'));
    }
-  if(strlen($form_state['values']['track']) != 6) {
+  if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['track']) ||
strlen($form_state['values']['track']) != 6) {
      form_set_error('track', t('HEX code must be 6 characters long.'));
    }
-  if(strlen($form_state['values']['tracker']) != 6) {
+  if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['tracker']) ||
strlen($form_state['values']['tracker']) != 6) {
      form_set_error('tracker', t('HEX code must be 6 characters long.'));
    }
-  if(strlen($form_state['values']['border']) != 6) {
+  if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['border']) ||
strlen($form_state['values']['border']) != 6) {
      form_set_error('border', t('HEX code must be 6 characters long.'));
    }
-  if(strlen($form_state['values']['skip']) != 6) {
+  if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['skip']) ||
strlen($form_state['values']['skip']) != 6) {
      form_set_error('skip', t('HEX code must be 6 characters long.'));
    }
-  if(strlen($form_state['values']['text']) != 6) {
+  if(preg_match('/[^0-9a-fA-F]/',$form_state['values']['text']) ||
strlen($form_state['values']['text']) != 6) {
      form_set_error('text', t('HEX code must be 6 characters long.'));
    }
  }
@@ -1189,7 +1189,7 @@
    $extras = str_replace(" ", "", $extras);
    $extras = str_replace(",", "&", $extras);

-  return '<p id="mp3player_'.$mp3_player_id.'" class="mp3player"
data="soundFile='.$audio_url.$extras.'">'.t("You may need: <a
href='http://get.adobe.com/flashplayer/'>Adobe Flash
Player</a>.").'</p>'. $description;
+  return '<p id="mp3player_'.$mp3_player_id.'" class="mp3player"
data="soundFile='.filter_xss($audio_url).$extras.'">'.t("You may need:
<a href='http://get.adobe.com/flashplayer/'>Adobe Flash
Player</a>.").'</p>'. $description;
  }

  /**
markie’s picture

markie CreditAttribution: markie commented
Assigned: Unassigned » markie
Status: Needs work » Needs review
FileSize
mp3player_add-filters.2050425.3.patch55.26 KB

Greggles
Thanks for working with me on this. Here's another patch that resolves the issues. I also did some code overhauls that were ticking me off. Please let me know what you think.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

I also did some code overhauls that were ticking me off.

That makes it much harder to review the patch and should be split to a "code cleanup" issue that gets committed after the security path :/

I suggest that you get a plan together to fix this issue and the other two issues as simply as you possibly can and make a release based on that. Then you can have other issues ready to go that fix other things. Make sense?

markie’s picture

markie CreditAttribution: markie commented
FileSize
mp3player_add-filters.2050425.5.patch5.42 KB

Makes perfect sense and as I thought about this last night, I assumed this would be the case. I reverted back to the original and added just the hex checks and library move to this patch. Thanks for your patience.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Title: Request to become a mainatainer » Fix security issue and become mainatainer
Category: support » bug
Status: Needs review » Needs work

Summarizing irc conversation - this still needs to filter on output - https://drupal.org/node/263002

markie’s picture

markie CreditAttribution: markie commented
Status: Needs work » Needs review
FileSize
mp3player_add_filters-2050425-7.patch13.7 KB

added checkplain to edit form and output as well as other changes in previous patche.

markie’s picture

markie CreditAttribution: markie commented

@greggles: Is this going to happen or should I punt this to github?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Status: Needs review » Needs work

@markie the patch in #7 includes a fair bit of changes unrelated to security which makes it hard to review. I'm sorry if this process is frustrating for you. It's hard for me to prioritize a review of this module since I don't use it. I did add this issue to the project page to try to get more reviews/eyeballs from other users of the module.

I tried to review it and I think the check_plaining you're doing and I can see how the changes help to centralize that function to make it easier. 

$audio_url = filter_xss($audio_url);

If that is truly a url I suggest using url() on it as that function includes some appropriate sanitizing. filter_xss isn't useful on random hunks of text, it only works on fully formed html. See http://drupalscout.com/knowledge-base/using-filter-functions-intended-fi... for an explanation with more details on when to use filter_xss or not.

From a security perspective: were you able to exploit the XSS issue and get a popup? Did you then test the changes post-patch and confirm that the xss no longer executed?

Thanks for your patience.

markie’s picture

markie CreditAttribution: markie commented

@greggles: Thanks for the feedback. I was getting frustrated because of a lack of movement. I appreciate you going out of your way to work with me on this. I apologize for the large amounts of differences on this module. I had to move things around to get it working in a way I could grock. If I were given VCS access I could be more iterative in my commits which could be easier. I don't want to release it till we all are happy, but I want to start tracking my changes. As far as the two points you bring up:

  1. $audio_url: it actually should be called $audio_file since this module only uses file field to upload the file. It's not really a URL, it's a path.
  2. I have not been able to replicate the XSS issue. Every time I try to name a file as suggested, both terminal and the OS X Finder UI refuses to name the file. And since the filefield is a file selector, I can't edit the name. I'd have to change the database to make that happen.
  3. That being said, I am making some tests to try theming the filed with custom data to see if it fails. I'll post those once I figure out how to make tests.

Again, thanks for your help on this.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

Thanks for the explanations.

1. filter_xss is definitely not the right thing for a path. I suggest either running filter_xss later when it is a fully formed html OR running url() or some other function that turns paths into urls and incorporates some sanitization.
2. I think for testing it would be fine to mess with the database values.
3. Sounds great!

markie’s picture

markie CreditAttribution: markie commented
Status: Needs work » Needs review
FileSize
mp3player_add_filters-2050425-12.patch14.31 KB

@greggles
I was able to update the patch as requested. There are still a lot of changes that don't pertain exclusively to the security issue, but I would like to point:

1) I was able to test the XSS venerability using the db update and changing the function to url() ad requested kills the player, producing a text error. Of course if someone has access to the database like this, there are more problems in our world.

2) I was able to hard code a tpl file to insert the venerability using the code suggested in the README.txt 

<?php 
if (module_exists('mp3player')) {
   print theme('mp3player', 'Default', '><body onload=javascript:alert("XSS")>.mp3', 'jerk', '', ''); 
}
?>

This also was captured and returns a text message.
3) Using the same code, but adding a few more evils, I was able to replicate the xss venerability and filter it out in the theme:

          <?php if (module_exists('mp3player')) {
            print theme('mp3player', 'Default',
              '><body onload=javascript:alert("XSS")>.mp3', '<body onload=javascript:alert("XSS")>',
              '<body onload=javascript:alert("XSS")>', '<body onload=javascript:alert("XSS")>'); } 
          ?>

I think the important bits that you want to see are starting on line 352 of the attached patch. Please let me know if you have any questions.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

Great, glad you were able to reproduce the problem.

From my perspective this seems as good as it can reasonably get. I'd like to keep it at needs review for someone else who uses mp3player module to test out the patch.

markie’s picture

markie CreditAttribution: markie commented

@greggles can I get commit access so I can push a 7.x branch?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

Sure. Done. Please don't create a stable release for a few weeks so we can try to get more testers.

dandaman’s picture

dandaman CreditAttribution: dandaman commented

Could we get a dev release or two up and available? It'd be much easier to test the latest dev version if I can download from Drush or directly from the web instead of having to do some Git checkout commands. Thanks!

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

6.x-1.x dev is at https://drupal.org/node/352937 which is visible under the "all releases" link on the project page.

@markie - making a 7.x-1.x-dev seems fine to me - your call on whether or not to do it, of course.

markie’s picture

markie CreditAttribution: markie commented

@greggles - I am testing the 7.x branch this week and will make a dev release.. Also started an 8.x release during badcamp so that could be up as well. Any chance I could get permission to edit the project page so I can update the instructions on loading the libraries module? There seems to be confusion and I would like to document it properly.

dandaman’s picture

dandaman CreditAttribution: dandaman commented

greggles,

I may be reading the "Commit Log" page wrong, but it seems most of the security fixes and features committed have been committed to a 6.x-2.x branch, so I don't think the 6.x-1.x-dev download will have those fixes.

I may get a chance later to pull from Git and test things out.

Thanks!

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

Fair enough. @markie - same advice applies to 6.x-2.x-dev.

dandaman’s picture

dandaman CreditAttribution: dandaman commented

I checked out 6.x-2.x-dev from Git. Here's a couple things I found:

First, on node page with a filefield getting the mp3 display, there is this PHP warning being thrown:

warning: unserialize() expects parameter 1 to be string, array given in /Users/dficker/Projects/cae/sites/all/modules/mp3player/filefieldmp3player/filefieldmp3player.module on line 65.

Also, in mp3player.module on line 89, there is debugging function that should be deleted:

  kpr($players);
markie’s picture

markie CreditAttribution: markie commented

@dandaman - I started a new issue for separate tracking https://drupal.org/node/2139711. Thanks for looking into it. I also created dev branches for easier updating (and issue tracking)

markie’s picture

markie CreditAttribution: markie commented

@greggles: Can you open this project to me so I can update the front page content? I'll not make any stable releases until we get the clear, but I would love to be able to fix the documentation to include the libraries requirement and other text.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

Sure, done.

markie’s picture

markie CreditAttribution: markie commented
Issue summary: View changes
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.