Security flaw with a function

[SwalyAaron]

So I was using this function earlier today and noticed something

function pm_api($pmfromuserid, $pmfromusername, $pmtitle, $pmmessage, $username)
{
global $vbulletin, $botpermissions;

$pmdm =& datamanager_init('PM', $vbulletin, ERRTYPE_ARRAY);
$pmdm->set('fromuserid', $pmfromuserid);
$pmdm->set('fromusername', $pmfromusername);
$pmdm->set('title', $pmtitle);
$pmdm->set('message', $pmmessage);
$pmdm->set_recipients($username, $botpermissions);
$pmdm->set('dateline', TIMENOW);
$pmdm->save();
unset($pmdm);
return $pmdm;
}

If the $username doesn't exist, it will print out the whole database with all passwords and the password of the database in an error similar to this:

Fatal error:
The following users were not found:
-
Unable to proceed with save while $errors array is not empty in class vB_DataManager_PM in [path]/includes/class_dm.php on line 810
#0 vb_error_handler(256,
The following users were not found:
-

*prints database*

So anyway I can prevent it from revealing all this info if it can't find the username?

[kh99]

I was looking at the code trying to figure out why all that info would be in the error message, but I can't. In any case, if you're not planning to use the error messages, you could try using ERRTYPE_SILENT instead of ERRTYPE_ARRAY (ETA: although now I'm not sure it will actually stop that message from printing). Also, you should change the code to check $pmdm->errors, and don't call $pmdm->save() if errors is set.

[SwalyAaron]

ERRTYPE_SILENT did the job thanks, and I don't know why all that info was put out it was seriously everything in my DB + the db pass and user

[kh99]

You really should be doing the second part of that (checking errors before calling save()), because it's the call to save() that's triggering an exception and showing all that info.

[SwalyAaron]

Originally Posted by kh99

You really should be doing the second part of that (checking errors before calling save()), because it's the call to save() that's triggering an exception and showing all that info.

Uh excuse my stupid question but would the code be like this?

function pm_api($pmfromuserid, $pmfromusername, $pmtitle, $pmmessage, $username)
{
global $vbulletin, $botpermissions;

$pmdm =& datamanager_init('PM', $vbulletin, ERRTYPE_ARRAY);
$pmdm->set('fromuserid', $pmfromuserid);
$pmdm->set('fromusername', $pmfromusername);
$pmdm->set('title', $pmtitle);
$pmdm->set('message', $pmmessage);
$pmdm->set_recipients($username, $botpermissions);
$pmdm->set('dateline', TIMENOW);
if (!isset($pmdm->errors()))
{
$pmdm->save();
}
unset($pmdm);
return $pmdm;
}
Or simply replacing ->save() with errors() ?

[kh99]

errors isn't a function, so you can't put the parens after it. Also, the vb code uses empty() instead of !isset() (but I can't remember offhand what the difference is if any - probably either will work). Oh, and in the vb code it looks like they call pre_save() before checking the errors.

Anyway, in the vb code they use:

$pmdm->pre_save();
if (empty($pmdm->errors))
{
$pmdm->save();
}

[squidsk]

Originally Posted by kh99

errors isn't a function, so you can't put the parens after it. Also, the vb code uses empty() instead of !isset() (but I can't remember offhand what the difference is if any - probably either will work). Oh, and in the vb code it looks like they call pre_save() before checking the errors.

Anyway, in the vb code they use:

$pmdm->pre_save();
if (empty($pmdm->errors))
{
$pmdm->save();
}

The reason the code uses empty instead of isset is that isset just checks if the variable exists the other is used to see if an array has any values in it. An empty array (i.e. with no errors) will return true for empty but false for !isset.