SA-CONTRIB-2014-061 , CVE-2014-2715 issue was addressed in commit:
https://www.drupal.org/commitlog/commit/42082/cd56eb5fbc0bc23ae79149c8c4...

http://cgit.drupalcode.org/videowhisper/diff/vwrooms/vwrooms.module?id=c...

Other issues were also addressed in later commits and more updates will follow.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

videowhisper’s picture

FileSize
7.64 KB
greggles’s picture

Status: Fixed » Needs work
function vwrooms_filedelete($type, $nid) {
  global $user;
  $types = array('vls', 'vconf', 'vconsult', 'v2wvc');
  if (!in_array($type, $types)) {
    return t('Invalid' . $type);
  }

  if (!user_access('administer vwrooms')) {
    // TODO Please convert this statement to the D7 database API syntax.
    $fr = db_query("select r.room,n.uid from {{$type}_rooms} r inner join {node} n on (n.nid=r.nid) and n.uid=$user->uid and n.nid=$nid")->fetchObject();
  }
  else {
    // TODO Please convert this statement to the D7 database API syntax.
    $fr = db_query("select r.room,n.uid from {{$type}_rooms} r inner join {node} n on (n.nid=r.nid) and n.nid=$nid")->fetchObject();
  }
  if (!$fr->room) {
    return t('Invalid');
  }
  ///echo "deleting $fr->room , type $type<br/>";
  vwrooms_removeroom($fr->room, $type);

  ///$p=variable_get($type."_path",'');


  return t('Deleted ');

}

This function looks to me like it contains access bypass, xss and CSRF. It's not changed by your proposed patch.

As I said on the private security issue, the whole module needs to be thoroughly reviewed and fixed up.

videowhisper’s picture

Assigned: lodidy » videowhisper
Status: Needs work » Needs review
FileSize
167.31 KB

Reviewed all modules, especially page callbacks and updated security as necessary.
As modules are reviewed and updated further, latest commit contains more updates than the patch.

Here's current version of that function:


function vwrooms_filedelete($type, $nid) {
  global $user;

  //filtering the 2 parameters
  $types = array('vls', 'vconf', 'vconsult', 'v2wvc');
  if (!in_array($type, $types)) {
    return t('Invalid type!');
  }
  $nid = (int) $nid;

  //select room for deletion based on access
  if (!user_access('administer vwrooms')) {
	//allow deletion only if owned by $user
    $fr = db_query("select r.room,n.uid from {{$type}_rooms} r inner join {node} n on (n.nid=r.nid) and n.uid=$user->uid and n.nid=$nid")->fetchObject();
  }
  else {
	//user has access to delete any room
    $fr = db_query("select r.room,n.uid from {{$type}_rooms} r inner join {node} n on (n.nid=r.nid) and n.nid=$nid")->fetchObject();
  }

 if (!$fr->room)   //no room selected
  {
    return t('Invalid room or permissions!');
  }
  else
  {
    vwrooms_removeroom($fr->room, $type);
    return t('Room was deleted.');
  }

}

  • videowhisper committed 1e9949d on
    Issue #2382229 by videowhisper: reviewed modules and secured as...
greggles’s picture

Status: Needs review » Needs work

I believe there is still a CSRF vulnerability.

The sql injection issue is improved, but it would be even better if you used the database api instead of casting arguments to an int.

videowhisper’s picture

Please elaborate on the CSRF vulnerability you mentioned and fix you require.
An extra dialog/confirmation step so trigger by link is not possible?

greggles’s picture

What elaboration do you desire? It's CSRF. The fixes are the standard fixes for CSRF. There are dozens of resources about fixing csrf in Drupal.

rooby’s picture

Check out the confirm_form() function for a simple solution.

videowhisper’s picture

Added an extra form to fix CSRF, as suggested :

 $items['vwrooms/deletefiles/%/%'] = array(
'page callback' => 'drupal_get_form',
'page arguments' => array('vwrooms_fdelete',2,3),
'access arguments' => array('access content'),
'access callback' => TRUE,
'type' => MENU_CALLBACK
);

Then used confirm_form() :

function vwrooms_fdelete($form, &$form_state, $type, $nid) {

  //filtering the 2 parameters
  $types = array('vls', 'vconf', 'vconsult', 'v2wvc');
  if (!in_array($type, $types)) {
    return t('Invalid type!');
  }
  $nid = (int) $nid;

	$form = array();

	$form['type'] = array(
		'#type' => 'value',
		'#value' => $type,
	);

	$form['nid'] = array(
		'#type' => 'value',
		'#value' => $nid,
	);

	return confirm_form(
		$form,
		t('Are you sure you want to delete room files?'),
		'node/'.$nid,
		t('This action cannot be undone.') . " ($type/$nid)",
		t('Delete Files'),
		t('Return to Room')
	);

	return $form;
}

function vwrooms_fdelete_submit($form, &$form_state)
{
	vwrooms_filedelete($form_state['values']['type'], $form_state['values']['nid'], $form_state);
}


function vwrooms_filedelete($type, $nid, &$form_state) {
  global $user;

  //filtering the 2 parameters
  $types = array('vls', 'vconf', 'vconsult', 'v2wvc');
  if (!in_array($type, $types)) {
    return t('Invalid type!');
  }
  $nid = (int) $nid;

  //select room for deletion based on access
  if (!user_access('administer vwrooms')) {
	//allow deletion only if owned by $user
    $fr = db_query("select r.room,n.uid from {{$type}_rooms} r inner join {node} n on (n.nid=r.nid) and n.uid=$user->uid and n.nid=$nid")->fetchObject();
  }
  else {
	//user has access to delete any room
    $fr = db_query("select r.room,n.uid from {{$type}_rooms} r inner join {node} n on (n.nid=r.nid) and n.nid=$nid")->fetchObject();
  }

  if (!$fr->room)   //no room selected
  {
  	drupal_set_message('Invalid room or permissions!');
  }
  else
  {
    vwrooms_removeroom($fr->room, $type);
	drupal_set_message('You confirmed file deletion for room ' .$fr->room . '.');
  }

	$form_state['redirect'] = 'node/'.$nid;

}

Update was pushed to repository so module users can secure their sites as soon as the project is restored.

greggles’s picture

Here's another example of a csrf vulnerability: http://cgit.drupalcode.org/videowhisper/tree/vconsult/vconsult/delete_al...

The code is just a bunch of security bugs. Saying that it's all been fixed is not accurate. You fixed the few things that were pointed out, but have not revisited the entire codebase to truly audit and fix it all. Until that is done the module should not be reinstated.

videowhisper’s picture

That script tool and all other similar were removed and latest version tagged as 7.x-1.8.

[7.x-1.x 011fd3c] Update
4 files changed, 228 deletions(-)
delete mode 100755 v2wvc/v2wvc/delete_all_uploads.php
delete mode 100755 vconf/vconf/delete_all_uploads.php
delete mode 100755 vconsult/vconsult/delete_all_uploads.php
delete mode 100755 vls/vls/delete_all_uploads.php

The script tool above was used to clean folder "uploads" containing chat logs and was provided as sample cleanup tool with original editions.
Was not affecting other files or data, just the application logs folder, so it's not a security issue for Drupal setup or a reason to close project.

The affirmation "The code is just a bunch of security bugs. " referring to contributed modules is not accurate, at least. All bugs reported until now were fixed and these contributed integration modules provide a lot of functionality as documented on project home page http://www.videowhisper.com/?p=Drupal-Pay-Per-View-Streaming .

Developers addressed all reported security issue and more that were identified. We did not state project is perfect. If anybody finds any issue, our developers will check and try to fix as they did before.
Finding a new flaw does not mean entire project is flawed and should be closed.
With that logic any project including Drupal should be closed when a flaw is found because developers "not revisited the entire codebase to truly audit and fix it all".

rooby’s picture

This is unrelated but for the sake of yourself and others it's good to use more descriptive commit messages.

If in future you come back to investigate commits and see something like this screenshot you may find it harder to audit things than it needs to be.

Commit messages screenshot

greggles’s picture

@videowhisper First, I apologize to you for my comment. It was inappropriate, unprofessional, rude to your contributions and unfair. I am sorry and I hope you will accept my apology. You are right that the presence of some number of bugs does not necessarily indicate that the whole of the software is bad.

Refocusing back on the issues at hand:

  • I see some issues where the code is unserializing a cookie. As heine has documented, this is dangerous with Drupal core as you can delete arbitrary files with Drupal core. When contribs are included it might be possible to do worse things including arbitrary code execution. Since this is possible with cookie values that are under control of the client it presents an access bypass security vulnerability.
  • I also noticed a query where arguments to the query are constructed without use of Drupal's db_query placeholders nor the dbtng system. This makes them vulnerable to sql injection attacks (e.g. a few examples here).
  • You said in comment #11 that some code was removed in a comment, but I don't see the commit 011fd3c in the repository on drupal.org:
    ➜ ~/c/videowhisper git:(7.x-1.x) git status
    On branch 7.x-1.x
    Your branch is up-to-date with 'origin/7.x-1.x'.
    nothing to commit, working directory clean
    ➜ ~/c/videowhisper git:(7.x-1.x) git remote -v
    origin	http://git.drupal.org/project/videowhisper.git (fetch)
    origin	http://git.drupal.org/project/videowhisper.git (push)
    ➜ ~/c/videowhisper git:(7.x-1.x) git checkout 011fd3c
    error: pathspec '011fd3c' did not match any file(s) known to git.
    ➜ ~/c/videowhisper git:(7.x-1.x)
    

One general note - many of these issues and many other issues could be cleaned up by making the module follow the Drupal coding standards. I've run those guidelines on the project and the results are at http://pareview.sh/pareview/httpgitdrupalorgprojectvideowhispergit-7x-1x