Parsing an uploaded directxdiag.xml file

TWD

I've written a PHP script that can parse a dxdiag.xml file. The script can read this xml file to retrieve a user's system information.

I've tried to put some security in mind when creating the script. In particular I'm worried about people submitting bad files like an exe or something, and second someone editing the xml file before hand so they can inject code.

To combat this I read the first line of the file looking for the xml header to verify that it really is an xml file. Second I check all the fields that we are pulling out to make sure they fit certain size requirements and don't contain any nasty characters <> and ' " to be specific.

Eventually this information will be input into a database where it will later be output as part of a benchmark score.

What other security issues should I be aware of with a script of this nature?

<!-- PAGE CONTENT! -->
<?php
//The user has already submitted a file
if ($_POST['submit'] == "Upload")
{

//Get the file
$file = $_FILES['dxdiag']['tmp_name'];

//Ensure it is a proper xml file.
$fp = fopen($file, "r"); 
$docheader = fgets($fp, 22);
$nextline = fgets($fp, 22);
fclose($fp);
if($docheader != "<?xml version=\"1.0\"?>")
    new errror("You did not provide a valid xml file");

//get the info
$xml = simplexml_load_file($file) or die ("Unable to load XML file!");

$sysinfo['os'] = $xml->SystemInformation->OperatingSystem;
$sysinfo['dx'] = $xml->SystemInformation->DirectXVersion;
$sysinfo['processor'] = $xml->SystemInformation->Processor;
$sysinfo['memory'] = $xml->SystemInformation->Memory;
$sysinfo['videocard'] = $xml->DisplayDevices->DisplayDevice[0]->CardName;
$sysinfo['videomemory'] = $xml->DisplayDevices->DisplayDevice[0]->DisplayMemory;
$sysinfo['videodriver'] = $xml->DisplayDevices->DisplayDevice[0]->DriverVersion;
$sysinfo['soundcard'] = $xml->DirectSound->SoundDevices->SoundDevice[0]->Description;


//Filter info to prevent code injection.
foreach($sysinfo as $value)
{
    if ($value == "" || strlen($value) <= 5 && strlen($value) >= 35)
        new error("The dxdiag output is currupt.  Bad string length" . print_r($sysinfo));
    else if(!preg_match('/^[a-z _,\[\]\(\)~\+!@#$%^&*\.0-9-]+$/iD', $value))
        new error("The dxdiag output is currupt does not match format" . $value);
}    

//Output the user's system specs
echo " 
<table><tr><td>" .
$sysinfo['os'] . "</td></tr><tr><td>" .
$sysinfo['dx'] . "</td></tr><tr><td>" .
$sysinfo['processor'] . "</td></tr><tr><td>" .
$sysinfo['memory'] . "</td></tr><tr><td>" .
$sysinfo['videocard'] . "</td></tr><tr><td>" .
$sysinfo['videomemory'] . "</td></tr><tr><td>" .
$sysinfo['videodriver'] . "</td></tr><tr><td>" .
$sysinfo['soundcard'] . "</td></tr></table>";

?> 
<br><br><a href="/benchmark/source.phps">View the source code</a>
    <?php
}

//Display the form
else
{
?>

<li>Click Start -> Run</li>
<li>Type cmd and press enter</li>
<li>In the command prompt type:<br>
<code>ddxdiag -x %HOMEPATH%\Desktop\dxdiag.xml</code></li>
<li>The file should appear on your desktop (Note: this may take a few moments).</li>
<li>Upload the dxdiag.xml file using this form:</li>

<form enctype="multipart/form-data" action="submit.php" method="post">
    <input type="hidden" name="MAX_FILE_SIZE" value="1000000" />
    Choose a file to upload: <input name="dxdiag" type="file" />
    <input type="submit" name="submit" value="Upload" />
  </form> 
<?php
}
?>
<!-- END PAGE CONTENT! -->

Weedpacket

    if($docheader != "<?xml version=\"1.0\"?>")
        new errror("You did not provide a valid xml file");

Unnecessary; if it's not a valid XML file then simplexml_load_file() will fail on the very next line anyway; where....

    $xml = simplexml_load_file($file) or die ("Unable to load XML file!");

The error message may be wrong - it may have that line at the top but it may not be a valid XML file. So it should give the same error message as the previous test. In fact, it probably is wrong - as far as the user is concerned the file was loaded. In other words, all three lines could be replaced by

$xml = simplexml_load_file($file) or die ("You did not provide a valid XML file.");

and the error message would be more useful.

TWD wrote:
Second I check all the fields that we are pulling out to make sure they fit certain size requirements and don't contain any nasty characters <> and ' " to be specific.

Why worry? Use htmlspecialchars() when displaying the data and they won't be an issue (you forgot that & has a special meaning in HTML pages, by the way). If the output is corrupt then the only person who will be disadvantaged would be the person who posted the corrupted data. (I guess the same would go for the error message, which contains the corrupted data nasty characters and all). Of course, assuming that the uploaded XML document is indeed a well-formed XML document (which it would need to be if it is to be parsed in the first place), all such escaping would have already taken place.

You also make no attempt to verify that any of the elements you look for (DisplayDevices, etc.) actually exist in the uploaded document before using them; however, if they don't, then that is caught because the resulting array value is the empty string; it still makes for generating a bunch of Notice messages before getting that far, though (and it means that again the error message is incorrect).