drbowen wrote on Friday, May 02, 2008:
Most of the problems that David Uhlman is referring to above are the use of registering global variables which was typical of development in PHP 3. These "errors" were built into OpenEMR between 1999 - 2000 when OpenEMR was being developed by Synitech. I would think that the developers at Synitech are working on a professional level.
The issue that he is referring to is that using global variables is known to be a really bad idea from a security standpoint. PHP 3 has been deprecated at least 4 years for its severe security problems, register globals being chief on the list. Register globals "On" compounds the problem because it can give an attacker inappropriate system access including possible root access to the operating system.
The original OpenEMR was written in PHP 3 and has huge number of lines of code. (Over 450,000 lines not counting the calendar). Instead of trying to fix the OpenEMR PHP 3 base, David Uhlman and his group decided for this and I am sure for a variety of reasons, to start over from scratch and to write a new project.
The SourceForge OpenEMR project development made the decision to work on functionality first, realizing that sooner or later, we would need to root out the PHP 3 vestiges and to move system variables out of globals.php. In the short term, we have forced Register Globals to be turned Off. Rod Roark has already improved a several security issues and continues to work diligently on these as they are discovered. Having an external review of our code is invaluable.
There are ready is a set of coding guidelines at www.oemr.org. There will be I am sure for improvement. One of the problems we see the most frequently with the volunteer developers that we have is not everyone is scrupulous about scrubbing ALL user input. The first rule of security is to NEVER trust user input.
Preventing SQL injection starts with basic server side security and especially MySQL server security, running the web page MySQL user with limited privileges (As an example Do Not Allow the "GRANT" privilege).
There a number of forms of SQL injection techniques and the imagination of potential attackers seems to be limitless. Additional measures include not allowing special characters during user input, checking user input for valid data, preventing multiple queries from being executed since many of the SQL-injection techniques cause the execution of two queries. The first being a malformed query that allows access and the second holds a malicious payload. Newer PHP mysql extensions including mysqli do not allow multiple queries and provides some protection.
***not allowing special characters during user input***
addslashes( older )
mysql_real_escape_string( recommended )
Mysql_real_escape_string prepends backslashes to the following characters: \x00, \n, \r, , ', " and \x1a
Available in PHP >= 4.3.0
***checking user input for valid data***
date fields: check for valid characters, dashes and slashes / or \ where appropriate.
Username: Restrict usernames to upper case, lower case letters, and underscores
Passwords: Do not allow known bad characters ’ " \
***Check for size***
allow the smallest inserts of data that is appropriate for the situation. As an example a free form test box should not contain 10 megabytes of data.
Here is a review of preventing SQL-injection in a PHP and MySQL environment:
http://www.phpbuilder.com/columns/ProPHPSecurity_excerpt.php3
http://www.phpbuilder.com/columns/ProPHPSecurity_excerpt_part2.php3
And here is another:
http://www.askbee.net/articles/php/SQL_Injection/sql_injection.html
Sam Bowen, MD